KVM: arm64: Prefix of fixes from HAFDBS series (for 7.4)
Subsystems (5)
Oliver Upton
maintainercover KVM: arm64: Prefix of fixes from HAFDBS series (for 7.4)
I kept on running into bugs as part of the HAFDBS series, especially in dealing with emulated cache maintenance instructions. Everything seems worthwhile for 7.4 so I've gone ahead and spun it off into a separate series.
The bulk of the changes are addressing a subtle gap in KVM's handling of coarse-grained traps. There could be instructions supported by the hardware but unsupported for the VM which intersect with a CGT. When the sysreg lacks an FGU description this has the effect of reinjecting architecturally-invalid sysreg traps to the L1 hypervisor for the visible feature set.
Besides that, abort injection has gotten a bit hairy, details found in the last patch.
Tested on M2 and QEMU-TCG since the M2 lacks a lot of the necessary arch features.
Oliver Upton (8):
KVM: arm64: nv: Disallow KVM_CAP_ARM_MTE + FEAT_EL2
KVM: arm64: nv: Add CGT description for FEAT_PoPS instructions
KVM: arm64: Separate user / guest visibility flags
KVM: arm64: nv: Treat the VNCR GICv3 registers as hidden from user
KVM: arm64: Use ->visibility() to filter MTE CMO instructions
KVM: arm64: Add sys_reg_descs for remaining MTE CMO instructions
KVM: arm64: nv: Enforce sysreg visibility before evaulating guest
traps
KVM: arm64: Stop leaning on __kvm_inject_sea() for abort injection
arch/arm64/kvm/arm.c | 10 ++- arch/arm64/kvm/emulate-nested.c | 26 +++++- arch/arm64/kvm/inject_fault.c | 133 ++++++++++++++++++------------- arch/arm64/kvm/sys_regs.c | 84 +++++++++++-------- arch/arm64/kvm/sys_regs.h | 27 +++++-- arch/arm64/kvm/vgic-sys-reg-v3.c | 2 +- arch/arm64/kvm/vgic-sys-reg-v5.c | 2 +- 7 files changed, 177 insertions(+), 107 deletions(-)
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
Signature · 1 line
2.47.3
Oliver Upton
maintainerPATCH 1/8 KVM: arm64: nv: Disallow KVM_CAP_ARM_MTE + FEAT_EL2
KVM's nested implementation already hides FEAT_MTE and its laundry list of derivatives from an NV guest. While good, this is insonsistent with the UAPI which does not explicitly prevent the combination of MTE and NV.
Bite the bullet and refuse to create a nested VM if KVM_CAP_ARM_MTE has been enabled.
--- arch/arm64/kvm/arm.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
arch/arm64/kvm/arm.c +6 -4
@@ -1793,13 +1793,15 @@ static int kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu, !test_bit(KVM_ARM_VCPU_PMU_V3, &features))return -EINVAL ; +/* MTE is incompatible with AArch32 and nested virt */ +if (kvm_has_mte (vcpu->kvm) && + (test_bit (KVM_ARM_VCPU_EL1_32BIT , &features) || +test_bit (KVM_ARM_VCPU_HAS_EL2 , &features))) +return -EINVAL ; +if (!test_bit (KVM_ARM_VCPU_EL1_32BIT , &features))return 0 ; -/* MTE is incompatible with AArch32 */ -if (kvm_has_mte (vcpu->kvm)) -return -EINVAL ; -/* NV is incompatible with AArch32 */ if (test_bit (KVM_ARM_VCPU_HAS_EL2 , &features))return -EINVAL ;
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH 1/8] KVM: arm64: nv: Disallow KVM_CAP_ARM_MTE + FEAT_EL2
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] KVM: arm64: rejecting MTE with EL2 breaks set_id_regs selftest on NV+MTE hosts --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: arm64: rejecting MTE with EL2 breaks set_id_regs selftest on NV+MTE hosts --
commit 6863183ae448414619eb0267884e7d66bfaf521c
Author: Oliver Upton <[email protected]>
KVM: arm64: nv: Disallow KVM_CAP_ARM_MTE + FEAT_EL2
This commit updates kvm_vcpu_init_check_features() to return -EINVAL when KVM_ARM_VCPU_HAS_EL2 is requested on a VM that has KVM_CAP_ARM_MTE enabled.
arch/arm64/kvm/arm.c static int kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu, · line 1800
Quoted 13 lines
arch/arm64/kvm/arm.c +5
-0@@ -1793,13 +1793,15 @@ static int kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu, !test_bit(KVM_ARM_VCPU_PMU_V3, &features))return -EINVAL ; +/* MTE is incompatible with AArch32 and nested virt */ +if (kvm_has_mte (vcpu->kvm) && + (test_bit (KVM_ARM_VCPU_EL1_32BIT , &features) || +test_bit (KVM_ARM_VCPU_HAS_EL2 , &features))) +return -EINVAL ;
[Severity: Medium]
Does rejecting KVM_ARM_VCPU_HAS_EL2 when kvm_has_mte(vcpu->kvm) is true break the arm64 set_id_regs selftest on hosts that support both MTE and nested virtualization?
In tools/testing/selftests/kvm/arm64/set_id_regs.c, main() enables MTE and then creates a default vCPU:
tools/testing/selftests/kvm/arm64/set_id_regs.c:main() {
...
test_wants_mte();
vm = vm_create(1);
vm_enable_cap(vm, KVM_CAP_ARM_WRITABLE_IMP_ID_REGS, 0);
vcpu = vm_vcpu_add(vm, 0, guest_code);
...
}During vm_create(), kvm_arch_vm_post_create() in tools/testing/selftests/kvm/lib/arm64/processor.c enables KVM_CAP_ARM_MTE on the VM when request_mte is set:
if (request_mte && vm_check_cap(vm, KVM_CAP_ARM_MTE)) vm_enable_cap(vm, KVM_CAP_ARM_MTE, 0);
Then during vm_vcpu_add() -> aarch64_vcpu_setup() -> kvm_get_default_vcpu_target(), the selftest library checks vm_supports_el2() and sets KVM_ARM_VCPU_HAS_EL2 by default on NV-capable hosts:
tools/testing/selftests/kvm/lib/arm64/processor.c:vm_supports_el2() {
return vm_check_cap(vm, KVM_CAP_ARM_EL2) && vm->arch.has_gic;
}tools/testing/selftests/kvm/lib/arm64/processor.c:kvm_get_default_vcpu_target() {
...
vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &preferred);
if (vm_supports_el2(vm))
preferred.features[0] |= BIT(KVM_ARM_VCPU_HAS_EL2);
...
}Because kvm_vm_ioctl_check_extension() in arch/arm64/kvm/arm.c still reports KVM_CAP_ARM_EL2 as supported on a VM fd where KVM_CAP_ARM_MTE was enabled:
case KVM_CAP_ARM_EL2: r = cpus_have_final_cap(ARM64_HAS_NESTED_VIRT); break;
and vm_supports_el2() does not check !request_mte, aarch64_vcpu_setup() passes KVM_ARM_VCPU_HAS_EL2 to KVM_ARM_VCPU_INIT, which now fails with -EINVAL unless the test is run with NV=0.
Should kvm_vm_ioctl_check_extension() gate KVM_CAP_ARM_EL2 on !kvm_has_mte(kvm) (or vm_supports_el2() check !request_mte) so userspace and selftests do not request EL2 on MTE-enabled VMs?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Oliver Upton
maintainerRe: [PATCH 1/8] KVM: arm64: nv: Disallow KVM_CAP_ARM_MTE + FEAT_EL2
Quoted 7 lines · On Wed, Nov 04, 2026 at 07:37:54PM +0000, [email protected] wrote:
Because
kvm_vm_ioctl_check_extension()inarch/arm64/kvm/arm.cstill reportsKVM_CAP_ARM_EL2as supported on a VM fd whereKVM_CAP_ARM_MTEwas enabled:case KVM_CAP_ARM_EL2: r = cpus_have_final_cap(ARM64_HAS_NESTED_VIRT); break;
Quoted 7 lines
and
vm_supports_el2()does not check !request_mte,aarch64_vcpu_setup()passesKVM_ARM_VCPU_HAS_EL2toKVM_ARM_VCPU_INIT, which now fails with-EINVALunless the test is run with NV=0.Should
kvm_vm_ioctl_check_extension()gateKVM_CAP_ARM_EL2on !kvm_has_mte(kvm) (orvm_supports_el2()check !request_mte) so userspace and selftests do not request EL2 on MTE-enabled VMs?
Sure, we can hide the cap but that's sensitive to ioctl ordering. Ultimately it's up to userspace to not do something stupid. So we should just document this rule.
Marc, if you feel like squashing:
Documentation/virt/kvm/api.rst +1 -0
@@ -3602,6 +3602,7 @@ Possible features: - KVM_ARM_VCPU_HAS_EL2: Enable Nested Virtualisation support, booting the guest from EL2 instead of EL1. Depends on KVM_CAP_ARM_EL2. + Cannot be enabled if KVM_CAP_ARM_MTE has been enabled for the VM. The VM is running with HCR_EL2.E2H being RES1 (VHE) unless KVM_ARM_VCPU_HAS_EL2_E2H0 is also set.
arch/arm64/kvm/arm.c +5 -1
@@ -480,7 +480,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) r =cpus_have_final_cap (ARM64_HAS_32BIT_EL1 );break ;case KVM_CAP_ARM_EL2 : - r =cpus_have_final_cap (ARM64_HAS_NESTED_VIRT ); +/* Nested virt is incompatible with MTE */ +if (kvm &&kvm_has_mte (kvm)) + r =0 ; +else + r =cpus_have_final_cap (ARM64_HAS_NESTED_VIRT );break ;case KVM_CAP_ARM_EL2_E2H0 : r =cpus_have_final_cap (ARM64_HAS_HCR_NV1 );
Thanks,
Oliver
Oliver Upton
maintainerPATCH 2/8 KVM: arm64: nv: Add CGT description for FEAT_PoPS instructions
In reply to Oliver UptonThe DC CIVAPS and DC CIGDVAPS instructions are both affected by HCR_EL2.TPCP. Add the missing CGT descriptors.
--- arch/arm64/kvm/emulate-nested.c | 2 ++ 1 file changed, 2 insertions(+)
arch/arm64/kvm/emulate-nested.c +2 -0
@@ -747,6 +747,8 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {SR_TRAP (SYS_DC_CGDVAP ,CGT_HCR_TPC ),SR_TRAP (SYS_DC_CGVADP ,CGT_HCR_TPC ),SR_TRAP (SYS_DC_CGDVADP ,CGT_HCR_TPC ), +SR_TRAP (SYS_DC_CIVAPS ,CGT_HCR_TPC ), +SR_TRAP (SYS_DC_CIGDVAPS ,CGT_HCR_TPC ),SR_TRAP (SYS_IC_IVAU ,CGT_HCR_TPU_TOCU ),SR_TRAP (SYS_IC_IALLU ,CGT_HCR_TPU_TOCU ), SR_TRAP(SYS_IC_IALLUIS, CGT_HCR_TPU_TICAB),
Signature · 1 line
2.47.3
Re: [PATCH 2/8] KVM: arm64: nv: Add CGT description for FEAT_PoPS instructions
Quoted 25 lines · Oliver Upton
The DC CIVAPS and DC CIGDVAPS instructions are both affected by HCR_EL2.TPCP. Add the missing CGT descriptors.
Fixes a7484c80e5ca ("KVM: arm64: Allow userspace to request KVM_ARM_VCPU_EL2*")Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/emulate-nested.c | 2 ++ 1 file changed, 2 insertions(+)arch/arm64/kvm/emulate-nested.c +2
-0@@ -747,6 +747,8 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {SR_TRAP (SYS_DC_CGDVAP ,CGT_HCR_TPC ),SR_TRAP (SYS_DC_CGVADP ,CGT_HCR_TPC ),SR_TRAP (SYS_DC_CGDVADP ,CGT_HCR_TPC ), +SR_TRAP (SYS_DC_CIVAPS ,CGT_HCR_TPC ), +SR_TRAP (SYS_DC_CIGDVAPS ,CGT_HCR_TPC ),SR_TRAP (SYS_IC_IVAU ,CGT_HCR_TPU_TOCU ),SR_TRAP (SYS_IC_IALLU ,CGT_HCR_TPU_TOCU ), SR_TRAP(SYS_IC_IALLUIS, CGT_HCR_TPU_TICAB),Signature · 1 line
2.47.3
LGTM. FWIW:
Thanks!
Leo
Oliver Upton
maintainerPATCH 3/8 KVM: arm64: Separate user / guest visibility flags
In reply to Oliver UptonThere's need for even more visibility hacks to differentiate between userspace and guest. Decompose REG_HIDDEN into two parts and test for the user-specific flag on all the appropriate UAPI surfaces.
--- arch/arm64/kvm/sys_regs.c | 10 +++++----- arch/arm64/kvm/sys_regs.h | 23 +++++++++++++++++------ arch/arm64/kvm/vgic-sys-reg-v3.c | 2 +- arch/arm64/kvm/vgic-sys-reg-v5.c | 2 +- 4 files changed, 24 insertions(+), 13 deletions(-)
arch/arm64/kvm/sys_regs.c +5 -5
@@ -4866,7 +4866,7 @@ static void perform_access(struct kvm_vcpu *vcpu,trace_kvm_sys_access (*vcpu_pc (vcpu), params, r);/* Check for regs disabled by runtime config */ -if (sysreg_hidden (vcpu, r)) { +if (sysreg_guest_hidden (vcpu, r)) {kvm_inject_undefined (vcpu);return ; }
@@ -5490,7 +5490,7 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, r =get_reg_by_id (id, table, num);/* Not saved in the sys_reg array and not otherwise accessible? */ -if (r && (!(r->reg || r->get_user) ||sysreg_hidden (vcpu, r))) +if (r && (!(r->reg || r->get_user) ||sysreg_user_hidden (vcpu, r))) r =NULL ;return r;
@@ -5571,7 +5571,7 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,int ret; r =id_to_sys_reg_desc (vcpu, id, table, num); -if (!r ||sysreg_hidden (vcpu, r)) +if (!r ||sysreg_user_hidden (vcpu, r))return -ENOENT ;if (r->get_user) {
@@ -5611,7 +5611,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,return -EFAULT ; r =id_to_sys_reg_desc (vcpu, id, table, num); -if (!r ||sysreg_hidden (vcpu, r)) +if (!r ||sysreg_user_hidden (vcpu, r))return -ENOENT ;if (sysreg_user_write_ignore (vcpu, r))
@@ -5705,7 +5705,7 @@ static int walk_one_sys_reg(const struct kvm_vcpu *vcpu,if (!(rd->reg || rd->get_user))return 0 ; -if (sysreg_hidden (vcpu, rd)) +if (sysreg_user_hidden (vcpu, rd))return 0 ;if (!copy_reg_to_user (rd, uind))
arch/arm64/kvm/sys_regs.h +17 -6
@@ -104,9 +104,14 @@ struct sys_reg_desc { const struct sys_reg_desc *rd); }; -#define REG_HIDDEN (1 << 0)/* hidden from userspace and guest */ -#define REG_RAZ (1 << 1)/* RAZ from userspace and guest */ -#define REG_USER_WI (1 << 2)/* WI from userspace only */ +#define REG_GUEST_HIDDEN BIT(0) +#define REG_USER_HIDDEN BIT(1) +/* Hidden from userspace and guest */ +#define REG_HIDDEN (REG_GUEST_HIDDEN | REG_USER_HIDDEN) +/* RAZ from userspace and guest */ +#define REG_RAZ BIT(2) +/* WI from userspace only */ +#define REG_USER_WI BIT(3) static __printf(2, 3) inline void print_sys_reg_msg(const struct sys_reg_params *p,
@@ -168,10 +173,16 @@ static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,return r->visibility (vcpu, r); } -static inline bool sysreg_hidden (const struct kvm_vcpu *vcpu, -const struct sys_reg_desc *r) +static inline bool sysreg_user_hidden (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) +{ +return sysreg_visibility (vcpu, r) ®_USER_HIDDEN ; +} + +static inline bool sysreg_guest_hidden (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) { -return sysreg_visibility (vcpu, r) ®_HIDDEN ; +return sysreg_visibility (vcpu, r) ®_GUEST_HIDDEN ; } static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
arch/arm64/kvm/vgic-sys-reg-v3.c +1 -1
@@ -465,7 +465,7 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr * r =get_reg_by_id (attr_to_id (attr->attr), gic_v3_icc_reg_descs,ARRAY_SIZE (gic_v3_icc_reg_descs)); -if (r && !sysreg_hidden (vcpu, r)) +if (r && !sysreg_user_hidden (vcpu, r))return 0 ;return -ENXIO ;
arch/arm64/kvm/vgic-sys-reg-v5.c +1 -1
@@ -495,7 +495,7 @@ int vgic_v5_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr * r =get_reg_by_id (attr_to_id (attr->attr), gic_v5_icc_reg_descs,ARRAY_SIZE (gic_v5_icc_reg_descs)); -if (r && !sysreg_hidden (vcpu, r)) +if (r && !sysreg_user_hidden (vcpu, r))return 0 ;return -ENXIO ;
Signature · 1 line
2.47.3
Re: [PATCH 3/8] KVM: arm64: Separate user / guest visibility flags
Quoted 70 lines · Oliver Upton
There's need for even more visibility hacks to differentiate between userspace and guest. Decompose
REG_HIDDENinto two parts and test for the user-specific flag on all the appropriate UAPI surfaces.Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 10 +++++----- arch/arm64/kvm/sys_regs.h | 23 +++++++++++++++++------ arch/arm64/kvm/vgic-sys-reg-v3.c | 2 +- arch/arm64/kvm/vgic-sys-reg-v5.c | 2 +- 4 files changed, 24 insertions(+), 13 deletions(-)arch/arm64/kvm/sys_regs.c +5
-5@@ -4866,7 +4866,7 @@ static void perform_access(struct kvm_vcpu *vcpu,trace_kvm_sys_access (*vcpu_pc (vcpu), params, r);/* Check for regs disabled by runtime config */ -if (sysreg_hidden (vcpu, r)) { +if (sysreg_guest_hidden (vcpu, r)) {kvm_inject_undefined (vcpu);return ; }@@ -5490,7 +5490,7 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, r =get_reg_by_id (id, table, num);/* Not saved in the sys_reg array and not otherwise accessible? */ -if (r && (!(r->reg || r->get_user) ||sysreg_hidden (vcpu, r))) +if (r && (!(r->reg || r->get_user) ||sysreg_user_hidden (vcpu, r))) r =NULL ;return r;@@ -5571,7 +5571,7 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,int ret; r =id_to_sys_reg_desc (vcpu, id, table, num); -if (!r ||sysreg_hidden (vcpu, r)) +if (!r ||sysreg_user_hidden (vcpu, r))return -ENOENT ;if (r->get_user) {@@ -5611,7 +5611,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,return -EFAULT ; r =id_to_sys_reg_desc (vcpu, id, table, num); -if (!r ||sysreg_hidden (vcpu, r)) +if (!r ||sysreg_user_hidden (vcpu, r))return -ENOENT ;if (sysreg_user_write_ignore (vcpu, r))@@ -5705,7 +5705,7 @@ static int walk_one_sys_reg(const struct kvm_vcpu *vcpu,if (!(rd->reg || rd->get_user))return 0 ; -if (sysreg_hidden (vcpu, rd)) +if (sysreg_user_hidden (vcpu, rd))return 0 ;if (!copy_reg_to_user (rd, uind))arch/arm64/kvm/sys_regs.h +0
-1@@ -104,9 +104,14 @@ struct sys_reg_desc { const struct sys_reg_desc *rd); }; -#define REG_HIDDEN (1 << 0)/* hidden from userspace and guest */
arch/arm64/kvm/sys_regs.h struct sys_reg_desc { · line 114
Quoted 10 lines
arch/arm64/kvm/sys_regs.h +8
-2-#define REG_RAZ (1 << 1)/* RAZ from userspace and guest */ -#define REG_USER_WI (1 << 2)/* WI from userspace only */ +#define REG_GUEST_HIDDEN BIT(0) +#define REG_USER_HIDDEN BIT(1) +/* Hidden from userspace and guest */ +#define REG_HIDDEN (REG_GUEST_HIDDEN | REG_USER_HIDDEN) +/* RAZ from userspace and guest */ +#define REG_RAZ BIT(2) +/* WI from userspace only */ +#define REG_USER_WI BIT(3)
So REG_HIDDEN returned by visibility functions now means that it's hidden from both guest access and usersace, and you changed the test helpers to check only one of them at a time.
IIUC 'user hidden' means "Userspace/VMM does not have access to this reg", while 'guest hidden' means "Guest does not have access to this reg". If that's the case, it makes sense all of the conversions that came from syscalls (getters/setters, etc) became 'user hidden' and the one that comes from kvm handlers became 'guest hidden'.
If I got that right, FWIW:
Thanks!
Leo
Quoted 52 lines
arch/arm64/kvm/sys_regs.h +9
-3static __printf(2, 3) inline void print_sys_reg_msg(const struct sys_reg_params *p,@@ -168,10 +173,16 @@ static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,return r->visibility (vcpu, r); } -static inline bool sysreg_hidden (const struct kvm_vcpu *vcpu, -const struct sys_reg_desc *r) +static inline bool sysreg_user_hidden (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) +{ +return sysreg_visibility (vcpu, r) ®_USER_HIDDEN ; +} + +static inline bool sysreg_guest_hidden (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) { -return sysreg_visibility (vcpu, r) ®_HIDDEN ; +return sysreg_visibility (vcpu, r) ®_GUEST_HIDDEN ; } static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,arch/arm64/kvm/vgic-sys-reg-v3.c +1
-1@@ -465,7 +465,7 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr * r =get_reg_by_id (attr_to_id (attr->attr), gic_v3_icc_reg_descs,ARRAY_SIZE (gic_v3_icc_reg_descs)); -if (r && !sysreg_hidden (vcpu, r)) +if (r && !sysreg_user_hidden (vcpu, r))return 0 ;return -ENXIO ;arch/arm64/kvm/vgic-sys-reg-v5.c +1
-1@@ -495,7 +495,7 @@ int vgic_v5_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr * r =get_reg_by_id (attr_to_id (attr->attr), gic_v5_icc_reg_descs,ARRAY_SIZE (gic_v5_icc_reg_descs)); -if (r && !sysreg_hidden (vcpu, r)) +if (r && !sysreg_user_hidden (vcpu, r))return 0 ;return -ENXIO ;Signature · 1 line
2.47.3
Oliver Upton
maintainerPATCH 4/8 KVM: arm64: nv: Treat the VNCR GICv3 registers as hidden from user
In reply to Oliver UptonKVM's emulation of sysreg traps will soon need to evaluate the visibility of a sysreg prior to the effects of the L1 hypervisor's traps to cope with instructions that intersect with a CGT but lack a corresponding FGU bit (I know, not enough TLAs).
The VNCR GICv3 registers are marked as hidden from both user/guest on the basis that KVM never handles these traps locally and unconditinally relies on VNCR redirection. But an L1 hypervisor could trap these.
Remap the visibility as being specific to user accesses, giving the indication that guest accesses to the sysreg are legal at least from an architecture POV. Note that the guest side of the visibility was a bit belt-and-braces as the bad_vncr_trap() accessor will act as a backstop against any unexpected local trap handling.
--- arch/arm64/kvm/sys_regs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/kvm/sys_regs.c +4 -4
@@ -1020,10 +1020,10 @@ static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)return mpidr; } -static unsigned int hidden_visibility (const struct kvm_vcpu *vcpu, -const struct sys_reg_desc *r) +static unsigned int hidden_user_visibility (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) { -return REG_HIDDEN ; +return REG_USER_HIDDEN ; } static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
@@ -2757,7 +2757,7 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,#define EL2_REG_VNCR_FILT (name, vis) \ EL2_REG_FILTERED(name, bad_vncr_trap, reset_val, 0, vis)#define EL2_REG_VNCR_GICv3 (name) \ - EL2_REG_VNCR_FILT(name, hidden_visibility) + EL2_REG_VNCR_FILT(name, hidden_user_visibility)#define EL2_REG_REDIR (name, rst, v) EL2_REG(name, bad_redir_trap, rst, v)#define TIMER_REG (name, vis) \
Signature · 1 line
2.47.3
Re: [PATCH 4/8] KVM: arm64: nv: Treat the VNCR GICv3 registers as hidden from user
Quoted 10 lines · Oliver Upton
KVM's emulation of sysreg traps will soon need to evaluate the visibility of a sysreg prior to the effects of the L1 hypervisor's traps to cope with instructions that intersect with a CGT but lack a corresponding FGU bit (I know, not enough TLAs).
The VNCR GICv3 registers are marked as hidden from both user/guest on the basis that KVM never handles these traps locally and unconditinally relies on VNCR redirection. But an L1 hypervisor could trap these.
Remap the visibility as being specific to user accesses, giving the
nit:
visibility is specific to guest accesses, right?
(non-visibility is specific to user accesses)
Quoted 40 lines
indication that guest accesses to the sysreg are legal at least from an architecture POV. Note that the guest side of the visibility was a bit belt-and-braces as the
bad_vncr_trap()accessor will act as a backstop against any unexpected local trap handling.Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)arch/arm64/kvm/sys_regs.c +4
-4@@ -1020,10 +1020,10 @@ static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)return mpidr; } -static unsigned int hidden_visibility (const struct kvm_vcpu *vcpu, -const struct sys_reg_desc *r) +static unsigned int hidden_user_visibility (const struct kvm_vcpu *vcpu, +const struct sys_reg_desc *r) { -return REG_HIDDEN ; +return REG_USER_HIDDEN ; } static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,@@ -2757,7 +2757,7 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,#define EL2_REG_VNCR_FILT (name, vis) \ EL2_REG_FILTERED(name, bad_vncr_trap, reset_val, 0, vis)#define EL2_REG_VNCR_GICv3 (name) \ - EL2_REG_VNCR_FILT(name, hidden_visibility) + EL2_REG_VNCR_FILT(name, hidden_user_visibility)#define EL2_REG_REDIR (name, rst, v) EL2_REG(name, bad_redir_trap, rst, v)#define TIMER_REG (name, vis) \Signature · 1 line
2.47.3
Other than the nit, LGTM.
Thanks!
Leo
Oliver Upton
maintainerPATCH 5/8 KVM: arm64: Use ->visibility() to filter MTE CMO instructions
In reply to Oliver UptonMore descriptors for MTE CMO instructions are on the way; prepare by using sys_reg_desc::visibility() to hide instructions when MTE isn't implemented.
--- arch/arm64/kvm/sys_regs.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
arch/arm64/kvm/sys_regs.c +13 -17
@@ -556,17 +556,6 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,return true; } -static bool access_dcgsw (struct kvm_vcpu *vcpu, -struct sys_reg_params *p, -const struct sys_reg_desc *r) -{ -if (!kvm_has_mte (vcpu->kvm)) -return undef_access (vcpu, p, r); - -/* Treat MTE S/W ops as we treat the classic ones: with contempt */ -return access_dcsw (vcpu, p, r); -} -static void get_access_mask (const struct sys_reg_desc *r,u64 *mask,u64 *shift) {switch (r->aarch32_map) {
@@ -4306,10 +4295,17 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, .access = (access_fn), \ } +#define MTE_CMO_INSN (insn, access_fn) \ + { \ + SYS_DESC(SYS_##insn), \ + .access = (access_fn), \ + .visibility = mte_visibility, \ + } +static struct sys_reg_desc sys_insn_descs[] = { {SYS_DESC (SYS_DC_ISW ), access_dcsw }, - {SYS_DESC (SYS_DC_IGSW ), access_dcgsw }, - {SYS_DESC (SYS_DC_IGDSW ), access_dcgsw }, +MTE_CMO_INSN (DC_IGSW , access_dcsw), +MTE_CMO_INSN (DC_IGDSW , access_dcsw),SYS_INSN (AT_S1E1R , handle_at_s1e01),SYS_INSN (AT_S1E1W , handle_at_s1e01),
@@ -4320,11 +4316,11 @@ static struct sys_reg_desc sys_insn_descs[] = {SYS_INSN (AT_S1E1A , handle_at_s1e01), { SYS_DESC(SYS_DC_CSW), access_dcsw }, - { SYS_DESC(SYS_DC_CGSW), access_dcgsw }, - { SYS_DESC(SYS_DC_CGDSW), access_dcgsw }, +MTE_CMO_INSN (DC_CGSW , access_dcsw), +MTE_CMO_INSN (DC_CGDSW , access_dcsw), { SYS_DESC(SYS_DC_CISW), access_dcsw }, - { SYS_DESC(SYS_DC_CIGSW), access_dcgsw }, - { SYS_DESC(SYS_DC_CIGDSW), access_dcgsw }, +MTE_CMO_INSN (DC_CIGSW , access_dcsw), +MTE_CMO_INSN (DC_CIGDSW , access_dcsw),SYS_INSN (TLBI_VMALLE1OS , handle_tlbi_el1), SYS_INSN(TLBI_VAE1OS, handle_tlbi_el1),
Signature · 1 line
2.47.3
Re: [PATCH 5/8] KVM: arm64: Use ->visibility() to filter MTE CMO instructions
Quoted 70 lines · Oliver Upton
More descriptors for MTE CMO instructions are on the way; prepare by using
sys_reg_desc::visibility()to hide instructions when MTE isn't implemented.Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)arch/arm64/kvm/sys_regs.c +13
-17@@ -556,17 +556,6 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,return true; } -static bool access_dcgsw (struct kvm_vcpu *vcpu, -struct sys_reg_params *p, -const struct sys_reg_desc *r) -{ -if (!kvm_has_mte (vcpu->kvm)) -return undef_access (vcpu, p, r); - -/* Treat MTE S/W ops as we treat the classic ones: with contempt */ -return access_dcsw (vcpu, p, r); -} -static void get_access_mask (const struct sys_reg_desc *r,u64 *mask,u64 *shift) {switch (r->aarch32_map) {@@ -4306,10 +4295,17 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, .access = (access_fn), \ } +#define MTE_CMO_INSN (insn, access_fn) \ + { \ + SYS_DESC(SYS_##insn), \ + .access = (access_fn), \ + .visibility = mte_visibility, \ + } +static struct sys_reg_desc sys_insn_descs[] = { {SYS_DESC (SYS_DC_ISW ), access_dcsw }, - {SYS_DESC (SYS_DC_IGSW ), access_dcgsw }, - {SYS_DESC (SYS_DC_IGDSW ), access_dcgsw }, +MTE_CMO_INSN (DC_IGSW , access_dcsw), +MTE_CMO_INSN (DC_IGDSW , access_dcsw),SYS_INSN (AT_S1E1R , handle_at_s1e01),SYS_INSN (AT_S1E1W , handle_at_s1e01),@@ -4320,11 +4316,11 @@ static struct sys_reg_desc sys_insn_descs[] = {SYS_INSN (AT_S1E1A , handle_at_s1e01), { SYS_DESC(SYS_DC_CSW), access_dcsw }, - { SYS_DESC(SYS_DC_CGSW), access_dcgsw }, - { SYS_DESC(SYS_DC_CGDSW), access_dcgsw }, +MTE_CMO_INSN (DC_CGSW , access_dcsw), +MTE_CMO_INSN (DC_CGDSW , access_dcsw), { SYS_DESC(SYS_DC_CISW), access_dcsw }, - { SYS_DESC(SYS_DC_CIGSW), access_dcgsw }, - { SYS_DESC(SYS_DC_CIGDSW), access_dcgsw }, +MTE_CMO_INSN (DC_CIGSW , access_dcsw), +MTE_CMO_INSN (DC_CIGDSW , access_dcsw),SYS_INSN (TLBI_VMALLE1OS , handle_tlbi_el1), SYS_INSN(TLBI_VAE1OS, handle_tlbi_el1),Signature · 1 line
2.47.3
I understand the MTE_CMO_INSN to be just about making this sys_insn_descs[] easier to read. I also noticed you changed access_dcgsw() to access_dcsw() when you removed the function.
The difference between the 2 is that access_dcgsw() checks for MTE before returning access_dcsw().
Since you now added .visibility = mte_visibility, which just returns REG_HIDDEN if !kvm_has_mte() it makes sense that access_dcgsw() became unnecessary.
FWIW:
Thanks!
Leo
Oliver Upton
maintainerPATCH 6/8 KVM: arm64: Add sys_reg_descs for remaining MTE CMO instructions
In reply to Oliver UptonFiltering unsupported cache maintenance instructions from a guest relies on either having a suitable FGU bit for the feature or a descriptor with a visibility() hook.
MTE CMO instructions can be trapped by HCR_EL2.TPCP and lack a suitable FGU. Add descriptors for the CMO instructions that UNDEF if MTE isn't implemented for the VM.
--- arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
arch/arm64/kvm/sys_regs.c +18 -0
@@ -4295,6 +4295,12 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, .access = (access_fn), \ } +static bool bad_cmo_trap (struct kvm_vcpu *vcpu,struct sys_reg_params *p, +const struct sys_reg_desc *r) +{ +return bad_trap (vcpu, p, r,"MTE CMO trap" ); +} +#define MTE_CMO_INSN (insn, access_fn) \ { \ SYS_DESC(SYS_##insn), \
@@ -4304,7 +4310,9 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,static struct sys_reg_desc sys_insn_descs[] = { {SYS_DESC (SYS_DC_ISW ), access_dcsw }, +MTE_CMO_INSN (DC_IGVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGSW , access_dcsw), +MTE_CMO_INSN (DC_IGDVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGDSW , access_dcsw),SYS_INSN (AT_S1E1R , handle_at_s1e01),
@@ -4321,6 +4329,7 @@ static struct sys_reg_desc sys_insn_descs[] = { { SYS_DESC(SYS_DC_CISW), access_dcsw },MTE_CMO_INSN (DC_CIGSW , access_dcsw),MTE_CMO_INSN (DC_CIGDSW , access_dcsw), +MTE_CMO_INSN (DC_CIGDVAPS , bad_cmo_trap),SYS_INSN (TLBI_VMALLE1OS , handle_tlbi_el1), SYS_INSN(TLBI_VAE1OS, handle_tlbi_el1),
@@ -4394,6 +4403,15 @@ static struct sys_reg_desc sys_insn_descs[] = {SYS_INSN (TLBI_VALE1NXS , handle_tlbi_el1),SYS_INSN (TLBI_VAALE1NXS , handle_tlbi_el1), +MTE_CMO_INSN (DC_CGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGDVAC , bad_cmo_trap), +SYS_INSN (AT_S1E2R , handle_at_s1e2),SYS_INSN (AT_S1E2W , handle_at_s1e2), SYS_INSN(AT_S12E1R, handle_at_s12),
Signature · 1 line
2.47.3
Re: [PATCH 6/8] KVM: arm64: Add sys_reg_descs for remaining MTE CMO instructions
Quoted 8 lines · Oliver Upton
Filtering unsupported cache maintenance instructions from a guest relies on either having a suitable FGU bit for the feature or a descriptor with a
visibility()hook.MTE CMO instructions can be trapped by HCR_EL2.TPCP and lack a suitable FGU. Add descriptors for the CMO instructions that UNDEF if MTE isn't implemented for the VM.
I think I get the idea of using MTE_CMO_INSN(): having the visibility test for MTE defined in the previous patch, which makes sense with the above commit message: if !mte -> hidden, then on access it injects undefined.
Please help me understand the other way: if we have mte, then it calls the 'access' callback, which is bad_cmo_trap(), which also inject undefined. What is the goal here?
Thanks!
Leo
Quoted 59 lines
Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm64/kvm/sys_regs.c +18
-0@@ -4295,6 +4295,12 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, .access = (access_fn), \ } +static bool bad_cmo_trap (struct kvm_vcpu *vcpu,struct sys_reg_params *p, +const struct sys_reg_desc *r) +{ +return bad_trap (vcpu, p, r,"MTE CMO trap" ); +} +#define MTE_CMO_INSN (insn, access_fn) \ { \ SYS_DESC(SYS_##insn), \@@ -4304,7 +4310,9 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,static struct sys_reg_desc sys_insn_descs[] = { {SYS_DESC (SYS_DC_ISW ), access_dcsw }, +MTE_CMO_INSN (DC_IGVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGSW , access_dcsw), +MTE_CMO_INSN (DC_IGDVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGDSW , access_dcsw),SYS_INSN (AT_S1E1R , handle_at_s1e01),@@ -4321,6 +4329,7 @@ static struct sys_reg_desc sys_insn_descs[] = { { SYS_DESC(SYS_DC_CISW), access_dcsw },MTE_CMO_INSN (DC_CIGSW , access_dcsw),MTE_CMO_INSN (DC_CIGDSW , access_dcsw), +MTE_CMO_INSN (DC_CIGDVAPS , bad_cmo_trap),SYS_INSN (TLBI_VMALLE1OS , handle_tlbi_el1), SYS_INSN(TLBI_VAE1OS, handle_tlbi_el1),@@ -4394,6 +4403,15 @@ static struct sys_reg_desc sys_insn_descs[] = {SYS_INSN (TLBI_VALE1NXS , handle_tlbi_el1),SYS_INSN (TLBI_VAALE1NXS , handle_tlbi_el1), +MTE_CMO_INSN (DC_CGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGDVAC , bad_cmo_trap), +SYS_INSN (AT_S1E2R , handle_at_s1e2),SYS_INSN (AT_S1E2W , handle_at_s1e2), SYS_INSN(AT_S12E1R, handle_at_s12),Signature · 1 line
2.47.3
Oliver Upton
maintainerRe: [PATCH 6/8] KVM: arm64: Add sys_reg_descs for remaining MTE CMO instructions
Quoted 10 lines · Leonardo Bras
Quoted 8 lines · Oliver Upton
Filtering unsupported cache maintenance instructions from a guest relies on either having a suitable FGU bit for the feature or a descriptor with a
visibility()hook.MTE CMO instructions can be trapped by HCR_EL2.TPCP and lack a suitable FGU. Add descriptors for the CMO instructions that UNDEF if MTE isn't implemented for the VM.
Quoted 7 lines
I think I get the idea of using
MTE_CMO_INSN(): having the visibility test for MTE defined in the previous patch, which makes sense with the above commit message: if !mte -> hidden, then on access it injects undefined.Please help me understand the other way: if we have mte, then it calls the 'access' callback, which is
bad_cmo_trap(), which also inject undefined. What is the goal here?
This is consistent with how we handle other unexpected traps, such as registers that redirect into the VNCR. If we take a trap for one of these instructions then either we UNDEF it early in triage or redirect the trap to the L1 hypervisor.
Under no circumstance do we expect to handle the trap locally, i.e. reach the ->access() function.
Thanks,
Oliver
Quoted 60 lines
Quoted 60 lines
Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm64/kvm/sys_regs.c +18
-0@@ -4295,6 +4295,12 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p, .access = (access_fn), \ } +static bool bad_cmo_trap (struct kvm_vcpu *vcpu,struct sys_reg_params *p, +const struct sys_reg_desc *r) +{ +return bad_trap (vcpu, p, r,"MTE CMO trap" ); +} +#define MTE_CMO_INSN (insn, access_fn) \ { \ SYS_DESC(SYS_##insn), \@@ -4304,7 +4310,9 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,static struct sys_reg_desc sys_insn_descs[] = { {SYS_DESC (SYS_DC_ISW ), access_dcsw }, +MTE_CMO_INSN (DC_IGVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGSW , access_dcsw), +MTE_CMO_INSN (DC_IGDVAC , bad_cmo_trap),MTE_CMO_INSN (DC_IGDSW , access_dcsw),SYS_INSN (AT_S1E1R , handle_at_s1e01),@@ -4321,6 +4329,7 @@ static struct sys_reg_desc sys_insn_descs[] = { { SYS_DESC(SYS_DC_CISW), access_dcsw },MTE_CMO_INSN (DC_CIGSW , access_dcsw),MTE_CMO_INSN (DC_CIGDSW , access_dcsw), +MTE_CMO_INSN (DC_CIGDVAPS , bad_cmo_trap),SYS_INSN (TLBI_VMALLE1OS , handle_tlbi_el1), SYS_INSN(TLBI_VAE1OS, handle_tlbi_el1),@@ -4394,6 +4403,15 @@ static struct sys_reg_desc sys_insn_descs[] = {SYS_INSN (TLBI_VALE1NXS , handle_tlbi_el1),SYS_INSN (TLBI_VAALE1NXS , handle_tlbi_el1), +MTE_CMO_INSN (DC_CGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVAP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CGDVADP , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGVAC , bad_cmo_trap), +MTE_CMO_INSN (DC_CIGDVAC , bad_cmo_trap), +SYS_INSN (AT_S1E2R , handle_at_s1e2),SYS_INSN (AT_S1E2W , handle_at_s1e2), SYS_INSN(AT_S12E1R, handle_at_s12),Signature · 1 line
2.47.3
Leonardo Bras
Reviewed-byRe: [PATCH 6/8] KVM: arm64: Add sys_reg_descs for remaining MTE CMO instructions
Quoted 24 lines · Oliver Upton
Quoted 10 lines · Leonardo Bras
Quoted 8 lines · Oliver Upton
Filtering unsupported cache maintenance instructions from a guest relies on either having a suitable FGU bit for the feature or a descriptor with a
visibility()hook.MTE CMO instructions can be trapped by HCR_EL2.TPCP and lack a suitable FGU. Add descriptors for the CMO instructions that UNDEF if MTE isn't implemented for the VM.
Quoted 7 lines
I think I get the idea of using
MTE_CMO_INSN(): having the visibility test for MTE defined in the previous patch, which makes sense with the above commit message: if !mte -> hidden, then on access it injects undefined.Please help me understand the other way: if we have mte, then it calls the 'access' callback, which is
bad_cmo_trap(), which also inject undefined. What is the goal here?This is consistent with how we handle other unexpected traps, such as registers that redirect into the VNCR. If we take a trap for one of these instructions then either we UNDEF it early in triage or redirect the trap to the L1 hypervisor.
Quoted 6 lines
Under no circumstance do we expect to handle the trap locally, i.e. reach the ->access() function.
Thanks,
Oliver
Ah, I see your point there :)
FWIW:
Thanks!
Leo
Oliver Upton
maintainerPATCH 7/8 KVM: arm64: nv: Enforce sysreg visibility before evaulating guest traps
In reply to Oliver UptonNot all sysregs / system instructions can be described by FGUs. Of these, there are some which may be not implemented for the VM (e.g. MTE cache maintenance instructions) yet intersect with valid trap bits for the VM (HCR_EL2.TPCP).
This is annoying to deal with since most of our UNDEF infrastructure is built around traps being RESx for unimplemented features. Test the visibility of the sys_reg_desc to avoid injecting traps for non-existent system registers into the guest hypervisor.
--- arch/arm64/kvm/emulate-nested.c | 24 +++++++++++++++++++++--- arch/arm64/kvm/sys_regs.c | 18 ++++++++++-------- arch/arm64/kvm/sys_regs.h | 4 +++- 3 files changed, 34 insertions(+), 12 deletions(-)
arch/arm64/kvm/emulate-nested.c +21 -3
@@ -2546,12 +2546,13 @@ static bool check_fgt_bit(struct kvm_vcpu *vcpu, enum vcpu_sysreg sr,return !(kvm_get_sysreg_res0 (kvm, sr) &BIT (tc.bit)); } -bool triage_sysreg_trap (struct kvm_vcpu *vcpu,int *sr_index) +bool triage_sysreg_trap (struct kvm_vcpu *vcpu,const struct sys_reg_desc **r) {enum vcpu_sysreg fgtreg;union trap_config tc;enum trap_behaviour b;bool is_read; +int sr_index;u32 sysreg;u64 esr;
@@ -2559,6 +2560,7 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) sysreg =esr_sys64_to_sysreg (esr); is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ; + *r =NULL ; tc =get_trap_config (sysreg);/*
@@ -2569,6 +2571,11 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)if (!tc.val) goto local; +if (tc.sri) { + sr_index = tc.sri -1 ; + *r =kvm_sys_reg_idx_to_desc (esr_sys64_to_sysreg (esr), sr_index); + } +/* * If a sysreg can be trapped using a FGT, first check whether we * trap for the purpose of forbidding the feature. In that case,
@@ -2595,6 +2602,18 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)if (is_hyp_ctxt (vcpu) && !vcpu_is_host_el0 (vcpu)) goto local; +/* +* Certain sysregs and system instructions could be UNDEF for the VM +* but lack a corresponding FGU description and intersect with a valid +* trap bit for the VM. Test the visibility the sysreg (if we have a +* sys_reg_desc) before determining the effects of the guest +* hypervisor's traps. +*/ +if (*r &&sysreg_guest_hidden (vcpu, *r)) { +kvm_inject_undefined (vcpu); +return true; + } +switch ((enum fgt_group_id )tc.fgt) {case __NO_FGT_GROUP__:break ;
@@ -2666,7 +2685,7 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) goto inject; local: -if (!tc.sri) { +if (!(*r)) {struct sys_reg_params params; params =esr_sys64_to_params (esr);
@@ -2697,7 +2716,6 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)return true; } - *sr_index = tc.sri -1 ;return false; inject:
arch/arm64/kvm/sys_regs.c +10 -8
@@ -5407,6 +5407,15 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); } +const struct sys_reg_desc *kvm_sys_reg_idx_to_desc (u32 encoding,int sr_idx) +{ +/* System registers have Op0=={2,3}, as per DDI487 J.a C5.1.2 */ +if (sys_reg_Op0 (encoding) ==2 ||sys_reg_Op0 (encoding) ==3 ) +return &sys_reg_descs[sr_idx]; +else +return &sys_insn_descs[sr_idx]; +} +/** * kvm_handle_sys_reg -- handles a system instruction or mrs/msr instruction * trap on a guest execution
@@ -5418,22 +5427,15 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)struct sys_reg_params params;unsigned long esr =kvm_vcpu_get_esr (vcpu);int Rt =kvm_vcpu_sys_get_rt (vcpu); -int sr_idx;trace_kvm_handle_sys_reg (esr); -if (triage_sysreg_trap (vcpu, &sr_idx)) +if (triage_sysreg_trap (vcpu, &desc))return 1 ; params =esr_sys64_to_params (esr); params.regval =vcpu_get_reg (vcpu, Rt); -/* System registers have Op0=={2,3}, as per DDI487 J.a C5.1.2 */ -if (params.Op0 ==2 || params.Op0 ==3 ) - desc = &sys_reg_descs[sr_idx]; -else - desc = &sys_insn_descs[sr_idx]; -perform_access (vcpu, ¶ms, desc);/* Read from system register? */
arch/arm64/kvm/sys_regs.h +3 -1
@@ -263,10 +263,12 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,int kvm_sys_reg_set_user (struct kvm_vcpu *vcpu,const struct kvm_one_reg *reg,const struct sys_reg_desc table[],unsigned int num); -bool triage_sysreg_trap (struct kvm_vcpu *vcpu,int *sr_index); +bool triage_sysreg_trap (struct kvm_vcpu *vcpu,const struct sys_reg_desc **r);int kvm_vcpu_finalize_sys_regs (struct kvm_vcpu *vcpu); +const struct sys_reg_desc *kvm_sys_reg_idx_to_desc (u32 encoding,int sr_idx); +#define AA32 (_x) .aarch32_map = AA32_##_x#define Op0 (_x) .Op0 = _x#define Op1 (_x) .Op1 = _x
Signature · 1 line
2.47.3
Re: [PATCH 7/8] KVM: arm64: nv: Enforce sysreg visibility before evaulating guest traps
Quoted 39 lines · Oliver Upton
Not all sysregs / system instructions can be described by FGUs. Of these, there are some which may be not implemented for the VM (e.g. MTE cache maintenance instructions) yet intersect with valid trap bits for the VM (HCR_EL2.TPCP).
This is annoying to deal with since most of our UNDEF infrastructure is built around traps being RESx for unimplemented features. Test the visibility of the
sys_reg_descto avoid injecting traps for non-existent system registers into the guest hypervisor.Fixes a7484c80e5ca ("KVM: arm64: Allow userspace to request KVM_ARM_VCPU_EL2*")Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/emulate-nested.c | 24 +++++++++++++++++++++--- arch/arm64/kvm/sys_regs.c | 18 ++++++++++-------- arch/arm64/kvm/sys_regs.h | 4 +++- 3 files changed, 34 insertions(+), 12 deletions(-)arch/arm64/kvm/emulate-nested.c +2
-1@@ -2546,12 +2546,13 @@ static bool check_fgt_bit(struct kvm_vcpu *vcpu, enum vcpu_sysreg sr,return !(kvm_get_sysreg_res0 (kvm, sr) &BIT (tc.bit)); } -bool triage_sysreg_trap (struct kvm_vcpu *vcpu,int *sr_index) +bool triage_sysreg_trap (struct kvm_vcpu *vcpu,const struct sys_reg_desc **r) {enum vcpu_sysreg fgtreg;union trap_config tc;enum trap_behaviour b;bool is_read; +int sr_index;u32 sysreg;u64 esr;
arch/arm64/kvm/emulate-nested.c bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) · line 2563
Quoted 4 lines
arch/arm64/kvm/emulate-nested.c +1
-0@@ -2559,6 +2560,7 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) sysreg =esr_sys64_to_sysreg (esr); is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ; + *r =NULL ;
In the caller, desc is already NULL, and here we are overwriting as NULL again. Is it needed here?
(For some reason it looks weird to me that the first thing we do with a parameter is to clean it's contents here)
arch/arm64/kvm/emulate-nested.c bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) · line 2576
Quoted 10 lines
arch/arm64/kvm/emulate-nested.c +3
-0tc =get_trap_config (sysreg);/* @@ -2569,6 +2571,11 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)if (!tc.val) goto local; +if (tc.sri) { + sr_index = tc.sri -1 ; + *r =kvm_sys_reg_idx_to_desc (esr_sys64_to_sysreg (esr), sr_index);
IIUC sr_index is used only in this block, maybe we could make it local to it?
Quoted 106 lines
arch/arm64/kvm/emulate-nested.c +15
-2+ } +/* * If a sysreg can be trapped using a FGT, first check whether we * trap for the purpose of forbidding the feature. In that case, @@ -2595,6 +2602,18 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)if (is_hyp_ctxt (vcpu) && !vcpu_is_host_el0 (vcpu)) goto local; +/* +* Certain sysregs and system instructions could be UNDEF for the VM +* but lack a corresponding FGU description and intersect with a valid +* trap bit for the VM. Test the visibility the sysreg (if we have a +* sys_reg_desc) before determining the effects of the guest +* hypervisor's traps. +*/ +if (*r &&sysreg_guest_hidden (vcpu, *r)) { +kvm_inject_undefined (vcpu); +return true; + } +switch ((enum fgt_group_id )tc.fgt) {case __NO_FGT_GROUP__:break ;@@ -2666,7 +2685,7 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index) goto inject; local: -if (!tc.sri) { +if (!(*r)) {struct sys_reg_params params; params =esr_sys64_to_params (esr);@@ -2697,7 +2716,6 @@ bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index)return true; } - *sr_index = tc.sri -1 ;return false; inject:arch/arm64/kvm/sys_regs.c +10
-8@@ -5407,6 +5407,15 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); } +const struct sys_reg_desc *kvm_sys_reg_idx_to_desc (u32 encoding,int sr_idx) +{ +/* System registers have Op0=={2,3}, as per DDI487 J.a C5.1.2 */ +if (sys_reg_Op0 (encoding) ==2 ||sys_reg_Op0 (encoding) ==3 ) +return &sys_reg_descs[sr_idx]; +else +return &sys_insn_descs[sr_idx]; +} +/** * kvm_handle_sys_reg -- handles a system instruction or mrs/msr instruction * trap on a guest execution @@ -5418,22 +5427,15 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)struct sys_reg_params params;unsigned long esr =kvm_vcpu_get_esr (vcpu);int Rt =kvm_vcpu_sys_get_rt (vcpu); -int sr_idx;trace_kvm_handle_sys_reg (esr); -if (triage_sysreg_trap (vcpu, &sr_idx)) +if (triage_sysreg_trap (vcpu, &desc))return 1 ; params =esr_sys64_to_params (esr); params.regval =vcpu_get_reg (vcpu, Rt); -/* System registers have Op0=={2,3}, as per DDI487 J.a C5.1.2 */ -if (params.Op0 ==2 || params.Op0 ==3 ) - desc = &sys_reg_descs[sr_idx]; -else - desc = &sys_insn_descs[sr_idx]; -perform_access (vcpu, ¶ms, desc);/* Read from system register? */ arch/arm64/kvm/sys_regs.h +3
-1@@ -263,10 +263,12 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,int kvm_sys_reg_set_user (struct kvm_vcpu *vcpu,const struct kvm_one_reg *reg,const struct sys_reg_desc table[],unsigned int num); -bool triage_sysreg_trap (struct kvm_vcpu *vcpu,int *sr_index); +bool triage_sysreg_trap (struct kvm_vcpu *vcpu,const struct sys_reg_desc **r);int kvm_vcpu_finalize_sys_regs (struct kvm_vcpu *vcpu); +const struct sys_reg_desc *kvm_sys_reg_idx_to_desc (u32 encoding,int sr_idx); +#define AA32 (_x) .aarch32_map = AA32_##_x#define Op0 (_x) .Op0 = _x#define Op1 (_x) .Op1 = _xSignature · 1 line
2.47.3
In general terms, I like that the change makes it easier to understand the function.
Thanks!
Leo
Oliver Upton
maintainerPATCH 8/8 KVM: arm64: Stop leaning on __kvm_inject_sea() for abort injection
In reply to Oliver UptonKVM's handling of local abort injection is, politely stated, a giant fscking mess. At surface level, things look somewhat innocuous: inject an SEA and directly update ESR_EL1 with the correct FSC afterwards. But this is broken for a couple reasons.
For one, on implementations with FEAT_DoubleFault2 KVM may inject the SEA as an SError depending on the falue of SCTLR2_EL1.NMEA, meaning that the eventual non-SEA abort is incorrectly taken to the SError vector.
AArch32 is also quite bitrotted. No surprise. The FSR value is written to IFSR not DFSR, when a prefetch abort is injected. So while DFSR is aliased to ESR_EL1 architecturally, fixing up ESR_EL1.FSC after injection has no effect on IFSR.
Finally, injecting aborts into non-LPAE relics may use an invalid value for xFSC.FS, as KVM unconditionally uses the LPAE/VMSAv8-64 format.
Fix the first two by tearing the whole mess, adding __kvm_inject_abort() for injecting non-SEA aborts which takes the desired FSC from the caller. Fix the third one by selecting a fault status value based on the LPAE-ness of the guest.
--- arch/arm64/kvm/inject_fault.c | 133 +++++++++++++++++++--------------- 1 file changed, 76 insertions(+), 57 deletions(-)
arch/arm64/kvm/inject_fault.c +76 -57
@@ -102,12 +102,32 @@ static bool effective_sctlr2_nmea(struct kvm_vcpu *vcpu)return __effective_sctlr2_bit (vcpu,SCTLR2_EL1_NMEA_SHIFT ); } -static void inject_abt64 (struct kvm_vcpu *vcpu,bool is_iabt,unsigned long addr) +static void __inject_abt64 (struct kvm_vcpu *vcpu,bool iabt,u32 iss,unsigned long addr) { -unsigned long cpsr = *vcpu_cpsr (vcpu); -bool is_aarch32 =vcpu_mode_is_32bit (vcpu); -u64 esr =0 , fsc; -int level; +u64 esr; +u8 ec; + +if (vcpu_mode_priv (vcpu)) + ec = iabt ? ESR_ELx_EC_IABT_CUR : ESR_ELx_EC_DABT_CUR; +else + ec = iabt ? ESR_ELx_EC_IABT_LOW : ESR_ELx_EC_DABT_LOW; + + esr =FIELD_PREP (ESR_ELx_EC_MASK, ec) |FIELD_PREP (ESR_ELx_ISS_MASK, iss) | + ESR_ELx_IL; + +vcpu_write_sys_reg (vcpu, addr,exception_far_elx (vcpu)); +vcpu_write_sys_reg (vcpu, esr,exception_esr_elx (vcpu)); +} + +static void inject_abt64 (struct kvm_vcpu *vcpu,bool iabt,u32 iss,u64 addr) +{ +pend_sync_exception (vcpu); +__inject_abt64 (vcpu, iabt, iss, addr); +} + +static void inject_sea64 (struct kvm_vcpu *vcpu,bool iabt,u64 addr) +{ +u8 fsc;/* * If injecting an abort from a failed S1PTW, rewalk the S1 PTs to
@@ -116,7 +136,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr*/ if (kvm_vcpu_abt_iss1tw (vcpu)) {u64 hpfar =kvm_vcpu_get_fault_ipa (vcpu); -int ret; +int ret, level;if (hpfar ==INVALID_GPA )return ;
@@ -137,28 +157,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addrelse pend_sync_exception (vcpu); -/* -* Build an {i,d}abort, depending on the level. -* Report an external synchronous abort. -*/ - esr |= ESR_ELx_IL; - -/* -* Here, the guest runs in AArch64 mode when in EL1. If we get -* an AArch32 fault, it means we managed to trap an EL0 fault. -*/ -if (is_aarch32 || (cpsr &PSR_MODE_MASK ) == PSR_MODE_EL0t) - esr |= (ESR_ELx_EC_IABT_LOW << ESR_ELx_EC_SHIFT); -else - esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT); - -if (!is_iabt) - esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT; - - esr |= fsc; - -vcpu_write_sys_reg (vcpu, addr,exception_far_elx (vcpu)); -vcpu_write_sys_reg (vcpu, esr,exception_esr_elx (vcpu)); +__inject_abt64 (vcpu, iabt, fsc, addr); }void kvm_inject_sync (struct kvm_vcpu *vcpu,u64 esr)
@@ -174,31 +173,41 @@ static void inject_undef64(struct kvm_vcpu *vcpu)kvm_inject_sync (vcpu, esr); } -#define DFSR_FSC_EXTABT_LPAE 0x10 -#define DFSR_FSC_EXTABT_nLPAE 0x08 -#define DFSR_LPAE BIT(9) -#define TTBCR_EAE BIT(31) +#define DFSR_FS_4 BIT(10) +#define DFSR_FS_3_0 GENMASK(3, 0) +#define DFSR_FS_EXTABT_nLPAE 0x08 +#define DFSR_FS_EXCL_ATOMIC_nLPAE 0x15 +#define DFSR_FSC GENMASK(5, 0) +#define DFSR_FSC_EXTABT_LPAE 0x10 +#define DFSR_LPAE BIT(9) +#define TTBCR_EAE BIT(31)static void inject_undef32 (struct kvm_vcpu *vcpu) {kvm_pend_exception (vcpu,EXCEPT_AA32_UND ); } +static bool vcpu_trans_32bit_non_lpae (struct kvm_vcpu *vcpu) +{ +return vcpu_el1_is_32bit (vcpu) && !(vcpu_read_sys_reg (vcpu,TCR_EL1 ) &TTBCR_EAE ); +} +/* * Modelled after TakeDataAbortException() and TakePrefetchAbortException * pseudocode. */ -static void inject_abt32 (struct kvm_vcpu *vcpu,bool is_pabt,u32 addr) +static void inject_abt32 (struct kvm_vcpu *vcpu,bool is_pabt,u8 status,u32 addr) {u64 far;u32 fsr; -/* Give the guest an IMPLEMENTATION DEFINED exception */ -if (vcpu_read_sys_reg (vcpu,TCR_EL1 ) &TTBCR_EAE ) { - fsr =DFSR_LPAE |DFSR_FSC_EXTABT_LPAE ; +if (vcpu_trans_32bit_non_lpae (vcpu)) { +/* Ensure status actually fits in DFSR.FS */ +KVM_BUG_ON (status >0x1F , vcpu->kvm); + fsr =FIELD_PREP (DFSR_FS_4 ,FIELD_GET (BIT (4 ), status)) | +FIELD_PREP (DFSR_FS_3_0 ,FIELD_GET (GENMASK (3 ,0 ), status)); }else { -/* no need to shuffle FS[4] into DFSR[10] as it's 0 */ - fsr = DFSR_FSC_EXTABT_nLPAE; + fsr =FIELD_PREP (DFSR_FSC , status) |DFSR_LPAE ; } far =vcpu_read_sys_reg (vcpu,FAR_EL1 );
@@ -218,12 +227,28 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt, u32 addr)vcpu_write_sys_reg (vcpu, far,FAR_EL1 ); } +static void inject_sea32 (struct kvm_vcpu *vcpu,bool is_pabt,u32 addr) +{ +if (vcpu_trans_32bit_non_lpae (vcpu)) +inject_abt32 (vcpu, is_pabt, DFSR_FS_EXTABT_nLPAE, addr); +else +inject_abt32 (vcpu, is_pabt,DFSR_FSC_EXTABT_LPAE , addr); +} +static void __kvm_inject_sea (struct kvm_vcpu *vcpu,bool iabt,u64 addr) {if (vcpu_el1_is_32bit (vcpu)) -inject_abt32 (vcpu, iabt, addr); +inject_sea32 (vcpu, iabt, addr);else -inject_abt64 (vcpu, iabt, addr); +inject_sea64 (vcpu, iabt, addr); +} + +static void __kvm_inject_abort (struct kvm_vcpu *vcpu,bool iabt,u8 fsc,u64 addr) +{ +if (vcpu_el1_is_32bit (vcpu)) +inject_abt32 (vcpu, iabt, fsc, addr); +else +inject_abt64 (vcpu, iabt, fsc, addr); }static bool kvm_sea_target_is_el2 (struct kvm_vcpu *vcpu)
@@ -270,28 +295,25 @@ static int kvm_inject_nested_excl_atomic(struct kvm_vcpu *vcpu, u64 addr)*/ int kvm_inject_dabt_excl_atomic (struct kvm_vcpu *vcpu,u64 addr) { -u64 esr; -if (is_nested_ctxt (vcpu) && (vcpu_read_sys_reg (vcpu,HCR_EL2 ) &HCR_EL2_VM ))return kvm_inject_nested_excl_atomic (vcpu, addr); -__kvm_inject_sea (vcpu, false, addr); - esr =vcpu_read_sys_reg (vcpu,exception_esr_elx (vcpu)); - esr &= ~ESR_ELx_FSC; - esr |= ESR_ELx_FSC_EXCL_ATOMIC; -vcpu_write_sys_reg (vcpu, esr,exception_esr_elx (vcpu)); +if (vcpu_trans_32bit_non_lpae (vcpu)) +__kvm_inject_abort (vcpu, false, DFSR_FS_EXCL_ATOMIC_nLPAE, addr); +else +__kvm_inject_abort (vcpu, false, ESR_ELx_FSC_EXCL_ATOMIC, addr); +return 1 ; }void kvm_inject_size_fault (struct kvm_vcpu *vcpu) { -unsigned long addr, esr; +bool iabt =kvm_vcpu_trap_is_iabt (vcpu); +unsigned long addr; addr =kvm_vcpu_get_fault_ipa (vcpu); addr |=FAR_TO_FIPA_OFFSET (kvm_vcpu_get_hfar (vcpu)); -__kvm_inject_sea (vcpu,kvm_vcpu_trap_is_iabt (vcpu), addr); -/* * If AArch64 or LPAE, set FSC to 0 to indicate an Address * Size Fault at level 0, as if exceeding PARange.
@@ -299,13 +321,10 @@ void kvm_inject_size_fault(struct kvm_vcpu *vcpu)* Non-LPAE guests will only get the external abort, as there * is no way to describe the ASF. */ -if (vcpu_el1_is_32bit (vcpu) && - !(vcpu_read_sys_reg (vcpu,TCR_EL1 ) &TTBCR_EAE )) -return ; - - esr =vcpu_read_sys_reg (vcpu,exception_esr_elx (vcpu)); - esr &= ~GENMASK_ULL (5 ,0 ); -vcpu_write_sys_reg (vcpu, esr,exception_esr_elx (vcpu)); +if (vcpu_trans_32bit_non_lpae (vcpu)) +__kvm_inject_sea (vcpu, iabt, addr); +else +__kvm_inject_abort (vcpu, iabt, ESR_ELx_FSC_ADDRSZ, addr); }/**
Signature · 1 line
2.47.3