| Message ID | 20240514072252.5657-2-sebott@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: emulation for CTR_EL0 | expand |
Hi Sebastian, On 5/14/24 15:22, Sebastian Ott wrote: > There are 2 functions to calculate traps via HCR_EL2: > * kvm_init_sysreg() called via KVM_RUN (before the 1st run or when > the pid changes) > * vcpu_reset_hcr() called via KVM_ARM_VCPU_INIT > > To unify these 2 and to support traps that are dependent on the > ID register configuration, move the code from vcpu_reset_hcr() > to sys_regs.c and call it via kvm_init_sysreg(). > > We still have to keep the non-FWB handling stuff in vcpu_reset_hcr(). > Also the initialization with HCR_GUEST_FLAGS is kept there but guarded > by !vcpu_has_run_once() to ensure that previous calculated values > don't get overwritten. > > While at it rename kvm_init_sysreg() to kvm_calculate_traps() to > better reflect what it's doing. > > Signed-off-by: Sebastian Ott <sebott@redhat.com> > --- > arch/arm64/include/asm/kvm_emulate.h | 40 +++++++--------------------- > arch/arm64/include/asm/kvm_host.h | 2 +- > arch/arm64/kvm/arm.c | 2 +- > arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++-- > 4 files changed, 43 insertions(+), 35 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 501e3e019c93..84dc3fac9711 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -69,39 +69,17 @@ static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) > > static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > { > - vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; > - if (has_vhe() || has_hvhe()) > - vcpu->arch.hcr_el2 |= HCR_E2H; > - if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { > - /* route synchronous external abort exceptions to EL2 */ > - vcpu->arch.hcr_el2 |= HCR_TEA; > - /* trap error record accesses */ > - vcpu->arch.hcr_el2 |= HCR_TERR; > - } > + if (!vcpu_has_run_once(vcpu)) > + vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; Could you give more explaination in your comments about why we still keep the non-FWB handling in vcpu_reset_hcr()? That would be better for understanding the special case. Thanks, Shaoqin > > - if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) { > - vcpu->arch.hcr_el2 |= HCR_FWB; > - } else { > - /* > - * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C > - * get set in SCTLR_EL1 such that we can detect when the guest > - * MMU gets turned on and do the necessary cache maintenance > - * then. > - */ > + /* > + * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C > + * get set in SCTLR_EL1 such that we can detect when the guest > + * MMU gets turned on and do the necessary cache maintenance > + * then. > + */ > + if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) > vcpu->arch.hcr_el2 |= HCR_TVM; > - } > - > - if (cpus_have_final_cap(ARM64_HAS_EVT) && > - !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) > - vcpu->arch.hcr_el2 |= HCR_TID4; > - else > - vcpu->arch.hcr_el2 |= HCR_TID2; > - > - if (vcpu_el1_is_32bit(vcpu)) > - vcpu->arch.hcr_el2 &= ~HCR_RW; > - > - if (kvm_has_mte(vcpu->kvm)) > - vcpu->arch.hcr_el2 |= HCR_ATA; > } > > static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8170c04fde91..212ae77eefaf 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1122,7 +1122,7 @@ int __init populate_nv_trap_config(void); > bool lock_all_vcpus(struct kvm *kvm); > void unlock_all_vcpus(struct kvm *kvm); > > -void kvm_init_sysreg(struct kvm_vcpu *); > +void kvm_calculate_traps(struct kvm_vcpu *); > > /* MMIO helpers */ > void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data); > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 9996a989b52e..6b217afb4e8e 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -797,7 +797,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu) > * This needs to happen after NV has imposed its own restrictions on > * the feature set > */ > - kvm_init_sysreg(vcpu); > + kvm_calculate_traps(vcpu); > > ret = kvm_timer_enable(vcpu); > if (ret) > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 22b45a15d068..41741bf4d2b2 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -4041,11 +4041,33 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range * > return 0; > } > > -void kvm_init_sysreg(struct kvm_vcpu *vcpu) > +static void vcpu_set_hcr(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > > - mutex_lock(&kvm->arch.config_lock); > + if (has_vhe() || has_hvhe()) > + vcpu->arch.hcr_el2 |= HCR_E2H; > + if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { > + /* route synchronous external abort exceptions to EL2 */ > + vcpu->arch.hcr_el2 |= HCR_TEA; > + /* trap error record accesses */ > + vcpu->arch.hcr_el2 |= HCR_TERR; > + } > + > + if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) > + vcpu->arch.hcr_el2 |= HCR_FWB; > + > + if (cpus_have_final_cap(ARM64_HAS_EVT) && > + !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) > + vcpu->arch.hcr_el2 |= HCR_TID4; > + else > + vcpu->arch.hcr_el2 |= HCR_TID2; > + > + if (vcpu_el1_is_32bit(vcpu)) > + vcpu->arch.hcr_el2 &= ~HCR_RW; > + > + if (kvm_has_mte(vcpu->kvm)) > + vcpu->arch.hcr_el2 |= HCR_ATA; > > /* > * In the absence of FGT, we cannot independently trap TLBI > @@ -4054,6 +4076,14 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > */ > if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS)) > vcpu->arch.hcr_el2 |= HCR_TTLBOS; > +} > + > +void kvm_calculate_traps(struct kvm_vcpu *vcpu) > +{ > + struct kvm *kvm = vcpu->kvm; > + > + mutex_lock(&kvm->arch.config_lock); > + vcpu_set_hcr(vcpu); > > if (cpus_have_final_cap(ARM64_HAS_HCX)) { > vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
Hi Shaoqin, On Mon, 27 May 2024, Shaoqin Huang wrote: > On 5/14/24 15:22, Sebastian Ott wrote: >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) >> { >> - vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; >> - if (has_vhe() || has_hvhe()) >> - vcpu->arch.hcr_el2 |= HCR_E2H; >> - if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { >> - /* route synchronous external abort exceptions to EL2 */ >> - vcpu->arch.hcr_el2 |= HCR_TEA; >> - /* trap error record accesses */ >> - vcpu->arch.hcr_el2 |= HCR_TERR; >> - } >> + if (!vcpu_has_run_once(vcpu)) >> + vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; > > Could you give more explaination in your comments about why we still keep the > non-FWB handling in vcpu_reset_hcr()? That would be better for understanding > the special case. > VM ops trapping needs to be toggled via KVM_ARM_VCPU_INIT for this case to catch when the guest activates its MMU. This is different to the other traps that are setup once before the guest runs for the first time. Sebastian
Hi Sebastian, On 5/14/24 09:22, Sebastian Ott wrote: > There are 2 functions to calculate traps via HCR_EL2: > * kvm_init_sysreg() called via KVM_RUN (before the 1st run or when > the pid changes) > * vcpu_reset_hcr() called via KVM_ARM_VCPU_INIT > > To unify these 2 and to support traps that are dependent on the > ID register configuration, move the code from vcpu_reset_hcr() > to sys_regs.c and call it via kvm_init_sysreg(). > > We still have to keep the non-FWB handling stuff in vcpu_reset_hcr(). > Also the initialization with HCR_GUEST_FLAGS is kept there but guarded > by !vcpu_has_run_once() to ensure that previous calculated values > don't get overwritten. > > While at it rename kvm_init_sysreg() to kvm_calculate_traps() to > better reflect what it's doing. > > Signed-off-by: Sebastian Ott <sebott@redhat.com> Looks good to me Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > --- > arch/arm64/include/asm/kvm_emulate.h | 40 +++++++--------------------- > arch/arm64/include/asm/kvm_host.h | 2 +- > arch/arm64/kvm/arm.c | 2 +- > arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++-- > 4 files changed, 43 insertions(+), 35 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 501e3e019c93..84dc3fac9711 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -69,39 +69,17 @@ static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) > > static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > { > - vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; > - if (has_vhe() || has_hvhe()) > - vcpu->arch.hcr_el2 |= HCR_E2H; > - if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { > - /* route synchronous external abort exceptions to EL2 */ > - vcpu->arch.hcr_el2 |= HCR_TEA; > - /* trap error record accesses */ > - vcpu->arch.hcr_el2 |= HCR_TERR; > - } > + if (!vcpu_has_run_once(vcpu)) > + vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; > > - if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) { > - vcpu->arch.hcr_el2 |= HCR_FWB; > - } else { > - /* > - * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C > - * get set in SCTLR_EL1 such that we can detect when the guest > - * MMU gets turned on and do the necessary cache maintenance > - * then. > - */ > + /* > + * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C > + * get set in SCTLR_EL1 such that we can detect when the guest > + * MMU gets turned on and do the necessary cache maintenance > + * then. > + */ > + if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) > vcpu->arch.hcr_el2 |= HCR_TVM; > - } > - > - if (cpus_have_final_cap(ARM64_HAS_EVT) && > - !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) > - vcpu->arch.hcr_el2 |= HCR_TID4; > - else > - vcpu->arch.hcr_el2 |= HCR_TID2; > - > - if (vcpu_el1_is_32bit(vcpu)) > - vcpu->arch.hcr_el2 &= ~HCR_RW; > - > - if (kvm_has_mte(vcpu->kvm)) > - vcpu->arch.hcr_el2 |= HCR_ATA; > } > > static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8170c04fde91..212ae77eefaf 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1122,7 +1122,7 @@ int __init populate_nv_trap_config(void); > bool lock_all_vcpus(struct kvm *kvm); > void unlock_all_vcpus(struct kvm *kvm); > > -void kvm_init_sysreg(struct kvm_vcpu *); > +void kvm_calculate_traps(struct kvm_vcpu *); > > /* MMIO helpers */ > void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data); > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 9996a989b52e..6b217afb4e8e 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -797,7 +797,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu) > * This needs to happen after NV has imposed its own restrictions on > * the feature set > */ > - kvm_init_sysreg(vcpu); > + kvm_calculate_traps(vcpu); > > ret = kvm_timer_enable(vcpu); > if (ret) > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 22b45a15d068..41741bf4d2b2 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -4041,11 +4041,33 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range * > return 0; > } > > -void kvm_init_sysreg(struct kvm_vcpu *vcpu) > +static void vcpu_set_hcr(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > > - mutex_lock(&kvm->arch.config_lock); > + if (has_vhe() || has_hvhe()) > + vcpu->arch.hcr_el2 |= HCR_E2H; > + if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { > + /* route synchronous external abort exceptions to EL2 */ > + vcpu->arch.hcr_el2 |= HCR_TEA; > + /* trap error record accesses */ > + vcpu->arch.hcr_el2 |= HCR_TERR; > + } > + > + if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) > + vcpu->arch.hcr_el2 |= HCR_FWB; > + > + if (cpus_have_final_cap(ARM64_HAS_EVT) && > + !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) > + vcpu->arch.hcr_el2 |= HCR_TID4; > + else > + vcpu->arch.hcr_el2 |= HCR_TID2; > + > + if (vcpu_el1_is_32bit(vcpu)) > + vcpu->arch.hcr_el2 &= ~HCR_RW; > + > + if (kvm_has_mte(vcpu->kvm)) > + vcpu->arch.hcr_el2 |= HCR_ATA; > > /* > * In the absence of FGT, we cannot independently trap TLBI > @@ -4054,6 +4076,14 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) > */ > if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS)) > vcpu->arch.hcr_el2 |= HCR_TTLBOS; > +} > + > +void kvm_calculate_traps(struct kvm_vcpu *vcpu) > +{ > + struct kvm *kvm = vcpu->kvm; > + > + mutex_lock(&kvm->arch.config_lock); > + vcpu_set_hcr(vcpu); > > if (cpus_have_final_cap(ARM64_HAS_HCX)) { > vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 501e3e019c93..84dc3fac9711 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -69,39 +69,17 @@ static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) { - vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; - if (has_vhe() || has_hvhe()) - vcpu->arch.hcr_el2 |= HCR_E2H; - if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { - /* route synchronous external abort exceptions to EL2 */ - vcpu->arch.hcr_el2 |= HCR_TEA; - /* trap error record accesses */ - vcpu->arch.hcr_el2 |= HCR_TERR; - } + if (!vcpu_has_run_once(vcpu)) + vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; - if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) { - vcpu->arch.hcr_el2 |= HCR_FWB; - } else { - /* - * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C - * get set in SCTLR_EL1 such that we can detect when the guest - * MMU gets turned on and do the necessary cache maintenance - * then. - */ + /* + * For non-FWB CPUs, we trap VM ops (HCR_EL2.TVM) until M+C + * get set in SCTLR_EL1 such that we can detect when the guest + * MMU gets turned on and do the necessary cache maintenance + * then. + */ + if (!cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) vcpu->arch.hcr_el2 |= HCR_TVM; - } - - if (cpus_have_final_cap(ARM64_HAS_EVT) && - !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) - vcpu->arch.hcr_el2 |= HCR_TID4; - else - vcpu->arch.hcr_el2 |= HCR_TID2; - - if (vcpu_el1_is_32bit(vcpu)) - vcpu->arch.hcr_el2 &= ~HCR_RW; - - if (kvm_has_mte(vcpu->kvm)) - vcpu->arch.hcr_el2 |= HCR_ATA; } static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 8170c04fde91..212ae77eefaf 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -1122,7 +1122,7 @@ int __init populate_nv_trap_config(void); bool lock_all_vcpus(struct kvm *kvm); void unlock_all_vcpus(struct kvm *kvm); -void kvm_init_sysreg(struct kvm_vcpu *); +void kvm_calculate_traps(struct kvm_vcpu *); /* MMIO helpers */ void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data); diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 9996a989b52e..6b217afb4e8e 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -797,7 +797,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu) * This needs to happen after NV has imposed its own restrictions on * the feature set */ - kvm_init_sysreg(vcpu); + kvm_calculate_traps(vcpu); ret = kvm_timer_enable(vcpu); if (ret) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 22b45a15d068..41741bf4d2b2 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -4041,11 +4041,33 @@ int kvm_vm_ioctl_get_reg_writable_masks(struct kvm *kvm, struct reg_mask_range * return 0; } -void kvm_init_sysreg(struct kvm_vcpu *vcpu) +static void vcpu_set_hcr(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; - mutex_lock(&kvm->arch.config_lock); + if (has_vhe() || has_hvhe()) + vcpu->arch.hcr_el2 |= HCR_E2H; + if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) { + /* route synchronous external abort exceptions to EL2 */ + vcpu->arch.hcr_el2 |= HCR_TEA; + /* trap error record accesses */ + vcpu->arch.hcr_el2 |= HCR_TERR; + } + + if (cpus_have_final_cap(ARM64_HAS_STAGE2_FWB)) + vcpu->arch.hcr_el2 |= HCR_FWB; + + if (cpus_have_final_cap(ARM64_HAS_EVT) && + !cpus_have_final_cap(ARM64_MISMATCHED_CACHE_TYPE)) + vcpu->arch.hcr_el2 |= HCR_TID4; + else + vcpu->arch.hcr_el2 |= HCR_TID2; + + if (vcpu_el1_is_32bit(vcpu)) + vcpu->arch.hcr_el2 &= ~HCR_RW; + + if (kvm_has_mte(vcpu->kvm)) + vcpu->arch.hcr_el2 |= HCR_ATA; /* * In the absence of FGT, we cannot independently trap TLBI @@ -4054,6 +4076,14 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu) */ if (!kvm_has_feat(kvm, ID_AA64ISAR0_EL1, TLB, OS)) vcpu->arch.hcr_el2 |= HCR_TTLBOS; +} + +void kvm_calculate_traps(struct kvm_vcpu *vcpu) +{ + struct kvm *kvm = vcpu->kvm; + + mutex_lock(&kvm->arch.config_lock); + vcpu_set_hcr(vcpu); if (cpus_have_final_cap(ARM64_HAS_HCX)) { vcpu->arch.hcrx_el2 = HCRX_GUEST_FLAGS;
There are 2 functions to calculate traps via HCR_EL2: * kvm_init_sysreg() called via KVM_RUN (before the 1st run or when the pid changes) * vcpu_reset_hcr() called via KVM_ARM_VCPU_INIT To unify these 2 and to support traps that are dependent on the ID register configuration, move the code from vcpu_reset_hcr() to sys_regs.c and call it via kvm_init_sysreg(). We still have to keep the non-FWB handling stuff in vcpu_reset_hcr(). Also the initialization with HCR_GUEST_FLAGS is kept there but guarded by !vcpu_has_run_once() to ensure that previous calculated values don't get overwritten. While at it rename kvm_init_sysreg() to kvm_calculate_traps() to better reflect what it's doing. Signed-off-by: Sebastian Ott <sebott@redhat.com> --- arch/arm64/include/asm/kvm_emulate.h | 40 +++++++--------------------- arch/arm64/include/asm/kvm_host.h | 2 +- arch/arm64/kvm/arm.c | 2 +- arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++-- 4 files changed, 43 insertions(+), 35 deletions(-)