| Message ID | 1550568271-5319-2-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ARMv8.3 pointer authentication for kvm guest | expand |
Hi, On Tue, Feb 19, 2019 at 02:54:26PM +0530, Amit Daniel Kachhap wrote: > From: Mark Rutland <mark.rutland@arm.com> > > When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which > is a constant value. This works today, as the host HCR_EL2 value is > always the same, but this will get in the way of supporting extensions > that require HCR_EL2 bits to be set conditionally for the host. > > To allow such features to work without KVM having to explicitly handle > every possible host feature combination, this patch has KVM save/restore > for the host HCR when switching to/from a guest HCR. The saving of the > register is done once during cpu hypervisor initialization state and is > just restored after switch from guest. > > For fetching HCR_EL2 during kvm initialisation, a hyp call is made using > kvm_call_hyp and is helpful in NHVE case. > > For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated > to toggle the TGE bit with a RMW sequence, as we already do in > __tlb_switch_to_guest_vhe(). > > The value of hcr_el2 is now stored in struct kvm_cpu_context as both host > and guest can now use this field in a common way. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > [Added __cpu_copy_hyp_conf, hcr_el2 field in struct kvm_cpu_context] > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Christoffer Dall <christoffer.dall@arm.com> > Cc: kvmarm@lists.cs.columbia.edu [...] > +/** > + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers > + * > + * It is called once per-cpu during CPU hyp initialisation. > + */ > +static inline void __cpu_copy_hyp_conf(void) I think this would be better named as something like: cpu_init_host_ctxt() ... as that makes it a bit clearer as to what is being initialized. [...] > +/** > + * __kvm_populate_host_regs - Stores host register values > + * > + * This function acts as a function handler parameter for kvm_call_hyp and > + * may be called from EL1 exception level to fetch the register value. > + */ > +void __hyp_text __kvm_populate_host_regs(void) > +{ > + struct kvm_cpu_context *host_ctxt; > + > + if (has_vhe()) > + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); > + else > + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); Do we need the has_vhe() check here? Can't we always do: host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); ... regardless of VHE? Or is that broken for VHE somehow? Thanks, Mark.
On Tue, Feb 19, 2019 at 02:54:26PM +0530, Amit Daniel Kachhap wrote: > From: Mark Rutland <mark.rutland@arm.com> > > When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which > is a constant value. This works today, as the host HCR_EL2 value is > always the same, but this will get in the way of supporting extensions > that require HCR_EL2 bits to be set conditionally for the host. > > To allow such features to work without KVM having to explicitly handle > every possible host feature combination, this patch has KVM save/restore > for the host HCR when switching to/from a guest HCR. The saving of the > register is done once during cpu hypervisor initialization state and is > just restored after switch from guest. > > For fetching HCR_EL2 during kvm initialisation, a hyp call is made using > kvm_call_hyp and is helpful in NHVE case. Minor nit: NVHE misspelled. This looks a bit like it's naming an arch feature rather than a kernel implementation detail though. Maybe write "non-VHE". > For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated > to toggle the TGE bit with a RMW sequence, as we already do in > __tlb_switch_to_guest_vhe(). > > The value of hcr_el2 is now stored in struct kvm_cpu_context as both host > and guest can now use this field in a common way. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > [Added __cpu_copy_hyp_conf, hcr_el2 field in struct kvm_cpu_context] > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Christoffer Dall <christoffer.dall@arm.com> > Cc: kvmarm@lists.cs.columbia.edu > --- > arch/arm/include/asm/kvm_host.h | 2 ++ > arch/arm64/include/asm/kvm_asm.h | 2 ++ > arch/arm64/include/asm/kvm_emulate.h | 22 +++++++++++----------- > arch/arm64/include/asm/kvm_host.h | 13 ++++++++++++- > arch/arm64/include/asm/kvm_hyp.h | 2 +- > arch/arm64/kvm/guest.c | 2 +- > arch/arm64/kvm/hyp/switch.c | 23 +++++++++++++---------- > arch/arm64/kvm/hyp/sysreg-sr.c | 21 ++++++++++++++++++++- > arch/arm64/kvm/hyp/tlb.c | 6 +++++- > virt/kvm/arm/arm.c | 1 + > 10 files changed, 68 insertions(+), 26 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index ca56537..05706b4 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) > kvm_call_hyp(__init_stage2_translation); > } > > +static inline void __cpu_copy_hyp_conf(void) {} > + > static inline int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext) > { > return 0; > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index f5b79e9..8acd73f 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void); > > extern u32 __kvm_get_mdcr_el2(void); > > +extern void __kvm_populate_host_regs(void); > + > /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */ > #define __hyp_this_cpu_ptr(sym) \ > ({ \ > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 506386a..0dbe795 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -50,25 +50,25 @@ void kvm_inject_pabt32(struct kvm_vcpu *vcpu, unsigned long addr); > > static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) > { > - return !(vcpu->arch.hcr_el2 & HCR_RW); > + return !(vcpu->arch.ctxt.hcr_el2 & HCR_RW); Putting hcr_el2 into struct kvm_cpu_context creates a lot of splatter here, and I'm wondering whether it's really necessary. Otherwise, we could just put the per-vcpu guest HCR_EL2 value in struct kvm_vcpu_arch. Is the *host* hcr_el2 value really different per-vcpu? That looks odd. I would have thought this is fixed across the system at KVM startup time. Having a single global host hcr_el2 would also avoid the need for __kvm_populate_host_regs(): instead, we just decide what HCR_EL2 is to be ahead of time and set a global variable that we map into Hyp. Or does the host HCR_EL2 need to vary at runtime for some reason I've missed? [...] +void __hyp_text __kvm_populate_host_regs(void) +{ + struct kvm_cpu_context *host_ctxt; + + if (has_vhe()) + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); + else + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); According to the comment by the definition of __hyp_this_cpu_ptr(), this always works at Hyp. I also see other calls with no fallback this_cpu_ptr() call like we have here. So, can we simply always call __hyp_this_cpu_ptr() here? (I'm not familiar with this, myself.) Cheers ---Dave
Hi Amit, On 19/02/2019 09:24, Amit Daniel Kachhap wrote: > From: Mark Rutland <mark.rutland@arm.com> > > When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which > is a constant value. This works today, as the host HCR_EL2 value is > always the same, but this will get in the way of supporting extensions > that require HCR_EL2 bits to be set conditionally for the host. > > To allow such features to work without KVM having to explicitly handle > every possible host feature combination, this patch has KVM save/restore > for the host HCR when switching to/from a guest HCR. The saving of the > register is done once during cpu hypervisor initialization state and is > just restored after switch from guest. > > For fetching HCR_EL2 during kvm initialisation, a hyp call is made using > kvm_call_hyp and is helpful in NHVE case. > > For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated > to toggle the TGE bit with a RMW sequence, as we already do in > __tlb_switch_to_guest_vhe(). > > The value of hcr_el2 is now stored in struct kvm_cpu_context as both host > and guest can now use this field in a common way. > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index ca56537..05706b4 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) > kvm_call_hyp(__init_stage2_translation); > } > > +static inline void __cpu_copy_hyp_conf(void) {} > + I agree Mark's suggestion of adding 'host_ctxt' in here makes it clearer what it is. > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 506386a..0dbe795 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h Hmmm, there is still a fair amount of churn due to moving the struct definition, but its easy enough to ignore as its mechanical. A preparatory patch that switched as may as possible to '*vcpu_hcr() = ' would cut the churn down some more, but I don't think its worth the extra effort. > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h > index a80a7ef..6e65cad 100644 > --- a/arch/arm64/include/asm/kvm_hyp.h > +++ b/arch/arm64/include/asm/kvm_hyp.h > @@ -151,7 +151,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs); > bool __fpsimd_enabled(void); > > void activate_traps_vhe_load(struct kvm_vcpu *vcpu); > -void deactivate_traps_vhe_put(void); > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu); I've forgotten why this is needed. You don't add a user of vcpu to deactivate_traps_vhe_put() in this patch. > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index b0b1478..006bd33 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -191,7 +194,7 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu) > -void deactivate_traps_vhe_put(void) > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu) > { > u64 mdcr_el2 = read_sysreg(mdcr_el2); > Why does deactivate_traps_vhe_put() need the vcpu? > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 7732d0b..1b2e05b 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -458,6 +459,16 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > > static inline void __cpu_init_stage2(void) {} > > +/** > + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers > + * > + * It is called once per-cpu during CPU hyp initialisation. > + */ Is it just the boot cpu? > +static inline void __cpu_copy_hyp_conf(void) > +{ > + kvm_call_hyp(__kvm_populate_host_regs); > +} > + > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c > index 68d6f7c..68ddc0f 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -21,6 +21,7 @@ > #include <asm/kvm_asm.h> > #include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > +#include <asm/kvm_mmu.h> ... what's kvm_mmu.h needed for? The __hyp_this_cpu_ptr() you add comes from kvm_asm.h. /me tries it. Heh, hyp_symbol_addr(). kvm_asm.h should include this, but can't because the kvm_ksym_ref() dependency is the other-way round. This is just going to bite us somewhere else later! If we want to fix it now, moving hyp_symbol_addr() to kvm_asm.h would fix it. It's generating adrp/add so the 'asm' label is fair, and it really should live with its EL1 counterpart kvm_ksym_ref(). > @@ -294,7 +295,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) > if (!has_vhe()) > return; > > - deactivate_traps_vhe_put(); > + deactivate_traps_vhe_put(vcpu); > > __sysreg_save_el1_state(guest_ctxt); > __sysreg_save_user_state(guest_ctxt); > @@ -316,3 +317,21 @@ void __hyp_text __kvm_enable_ssbs(void) > "msr sctlr_el2, %0" > : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS)); > } > + > +/** > + * __kvm_populate_host_regs - Stores host register values > + * > + * This function acts as a function handler parameter for kvm_call_hyp and > + * may be called from EL1 exception level to fetch the register value. > + */ > +void __hyp_text __kvm_populate_host_regs(void) > +{ > + struct kvm_cpu_context *host_ctxt; > + if (has_vhe()) > + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); > + else > + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); You can use __hyp_this_cpu_ptr() here, even on VHE. For VHE the guts are the same and its simpler to use the same version in both cases. __hyp_this_cpu_ptr(sym) == hyp_symbol_addr(sym) + tpidr_el2; hyp_symbol_addr() here is just to guarantee the address is generated based on where we're executing from, not loaded from a literal pool which would give us the link-time address. (or whenever kaslr applied the relocations). This matters for non-VHE because the compiler can't know the code has an EL2 address as well as its link-time address. This doesn't matter for VHE, as there is no additional different address. (the other trickery is on non-VHE the tpidr_el2 value isn't actually the same as the hosts.. but on VHE it is) > + host_ctxt->hcr_el2 = read_sysreg(hcr_el2); > +} > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 9e350fd3..8e18f7f 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -1328,6 +1328,7 @@ static void cpu_hyp_reinit(void) > cpu_init_hyp_mode(NULL); > > kvm_arm_init_debug(); > + __cpu_copy_hyp_conf(); Your commit message says: | The saving of the register is done once during cpu hypervisor initialization state But cpu_hyp_reinit() is called each time secondary CPUs come online. Its also called as part of the cpu-idle mechanism via hyp_init_cpu_pm_notifier(). cpu-idle can ask the firmware to power-off the CPU until an interrupt becomes pending for it. KVM's EL2 state disappears when this happens, these calls take care of setting it back up again. On Juno, this can happen tens of times a second, and this adds an extra call to EL2. init_subsystems() would be the alternative place for this, but it wouldn't catch CPUs that came online after booting. I think you need something in cpu_hyp_reinit() or __cpu_copy_hyp_conf() to ensure it only happens once per CPU. I think you can test whether the HCR_EL2 value is zero, assuming zero means uninitialised. A VHE system would always set E2H, and a non-VHE system has to set RW. > if (vgic_present) > kvm_vgic_init_cpu_hardware(); > Thanks, James
On 21/02/2019 11:50, Mark Rutland wrote: > Hi, > > On Tue, Feb 19, 2019 at 02:54:26PM +0530, Amit Daniel Kachhap wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> >> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which >> is a constant value. This works today, as the host HCR_EL2 value is >> always the same, but this will get in the way of supporting extensions >> that require HCR_EL2 bits to be set conditionally for the host. >> >> To allow such features to work without KVM having to explicitly handle >> every possible host feature combination, this patch has KVM save/restore >> for the host HCR when switching to/from a guest HCR. The saving of the >> register is done once during cpu hypervisor initialization state and is >> just restored after switch from guest. >> >> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using >> kvm_call_hyp and is helpful in NHVE case. >> >> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated >> to toggle the TGE bit with a RMW sequence, as we already do in >> __tlb_switch_to_guest_vhe(). >> >> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host >> and guest can now use this field in a common way. >> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> [Added __cpu_copy_hyp_conf, hcr_el2 field in struct kvm_cpu_context] >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Christoffer Dall <christoffer.dall@arm.com> >> Cc: kvmarm@lists.cs.columbia.edu > > [...] > >> +/** >> + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers >> + * >> + * It is called once per-cpu during CPU hyp initialisation. >> + */ >> +static inline void __cpu_copy_hyp_conf(void) > > I think this would be better named as something like: > > cpu_init_host_ctxt() > > ... as that makes it a bit clearer as to what is being initialized. > > [...] > >> +/** >> + * __kvm_populate_host_regs - Stores host register values >> + * >> + * This function acts as a function handler parameter for kvm_call_hyp and >> + * may be called from EL1 exception level to fetch the register value. >> + */ >> +void __hyp_text __kvm_populate_host_regs(void) >> +{ >> + struct kvm_cpu_context *host_ctxt; >> + >> + if (has_vhe()) >> + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); >> + else >> + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > Do we need the has_vhe() check here? > > Can't we always do: > > host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > ... regardless of VHE? Or is that broken for VHE somehow? The whole point of __hyp_this_cpu_ptr is that it is always valid... See 85478bab40917 for details. Thanks, M.
Hi Amit, On 25/02/2019 17:39, James Morse wrote: > On 19/02/2019 09:24, Amit Daniel Kachhap wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which >> is a constant value. This works today, as the host HCR_EL2 value is >> always the same, but this will get in the way of supporting extensions >> that require HCR_EL2 bits to be set conditionally for the host. >> >> To allow such features to work without KVM having to explicitly handle >> every possible host feature combination, this patch has KVM save/restore >> for the host HCR when switching to/from a guest HCR. The saving of the >> register is done once during cpu hypervisor initialization state and is >> just restored after switch from guest. >> >> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using >> kvm_call_hyp and is helpful in NHVE case. >> >> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated >> to toggle the TGE bit with a RMW sequence, as we already do in >> __tlb_switch_to_guest_vhe(). >> >> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host >> and guest can now use this field in a common way. >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 9e350fd3..8e18f7f 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -1328,6 +1328,7 @@ static void cpu_hyp_reinit(void) >> cpu_init_hyp_mode(NULL); >> >> kvm_arm_init_debug(); >> + __cpu_copy_hyp_conf(); > > Your commit message says: > | The saving of the register is done once during cpu hypervisor initialization state > > But cpu_hyp_reinit() is called each time secondary CPUs come online. Its also called as > part of the cpu-idle mechanism via hyp_init_cpu_pm_notifier(). cpu-idle can ask the > firmware to power-off the CPU until an interrupt becomes pending for it. KVM's EL2 state > disappears when this happens, these calls take care of setting it back up again. On Juno, > this can happen tens of times a second, and this adds an extra call to EL2. The bit I missed was the MDCR_EL2 copy is behind kvm_arm_init_debug(), so we already have an unnecessary EL2 call here, so its nothing new. Assuming the deactivate_traps_vhe_put() vcpu isn't needed, and with Mark's comments addressed: Reviewed-by: James Morse <james.morse@arm.com> If we can avoid repeated calls to EL2 once we've got HCR_EL2+MDCR_EL2, even better! Thanks, James
Hi, On 2/21/19 5:20 PM, Mark Rutland wrote: > Hi, > > On Tue, Feb 19, 2019 at 02:54:26PM +0530, Amit Daniel Kachhap wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> >> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which >> is a constant value. This works today, as the host HCR_EL2 value is >> always the same, but this will get in the way of supporting extensions >> that require HCR_EL2 bits to be set conditionally for the host. >> >> To allow such features to work without KVM having to explicitly handle >> every possible host feature combination, this patch has KVM save/restore >> for the host HCR when switching to/from a guest HCR. The saving of the >> register is done once during cpu hypervisor initialization state and is >> just restored after switch from guest. >> >> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using >> kvm_call_hyp and is helpful in NHVE case. >> >> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated >> to toggle the TGE bit with a RMW sequence, as we already do in >> __tlb_switch_to_guest_vhe(). >> >> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host >> and guest can now use this field in a common way. >> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> [Added __cpu_copy_hyp_conf, hcr_el2 field in struct kvm_cpu_context] >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Christoffer Dall <christoffer.dall@arm.com> >> Cc: kvmarm@lists.cs.columbia.edu > > [...] > >> +/** >> + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers >> + * >> + * It is called once per-cpu during CPU hyp initialisation. >> + */ >> +static inline void __cpu_copy_hyp_conf(void) > > I think this would be better named as something like: > > cpu_init_host_ctxt() > > ... as that makes it a bit clearer as to what is being initialized. ok, Agreed with the name. > > [...] > >> +/** >> + * __kvm_populate_host_regs - Stores host register values >> + * >> + * This function acts as a function handler parameter for kvm_call_hyp and >> + * may be called from EL1 exception level to fetch the register value. >> + */ >> +void __hyp_text __kvm_populate_host_regs(void) >> +{ >> + struct kvm_cpu_context *host_ctxt; >> + >> + if (has_vhe()) >> + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); >> + else >> + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > Do we need the has_vhe() check here? > > Can't we always do: > > host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > ... regardless of VHE? Or is that broken for VHE somehow? Yes it works fine for VHE. I missed this. Thanks, Amit > > Thanks, > Mark. >
Hi, On 2/21/19 9:19 PM, Dave Martin wrote: > On Tue, Feb 19, 2019 at 02:54:26PM +0530, Amit Daniel Kachhap wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> >> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which >> is a constant value. This works today, as the host HCR_EL2 value is >> always the same, but this will get in the way of supporting extensions >> that require HCR_EL2 bits to be set conditionally for the host. >> >> To allow such features to work without KVM having to explicitly handle >> every possible host feature combination, this patch has KVM save/restore >> for the host HCR when switching to/from a guest HCR. The saving of the >> register is done once during cpu hypervisor initialization state and is >> just restored after switch from guest. >> >> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using >> kvm_call_hyp and is helpful in NHVE case. > > Minor nit: NVHE misspelled. This looks a bit like it's naming an arch > feature rather than a kernel implementation detail though. Maybe write > "non-VHE". yes. > >> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated >> to toggle the TGE bit with a RMW sequence, as we already do in >> __tlb_switch_to_guest_vhe(). >> >> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host >> and guest can now use this field in a common way. >> >> Signed-off-by: Mark Rutland <mark.rutland@arm.com> >> [Added __cpu_copy_hyp_conf, hcr_el2 field in struct kvm_cpu_context] >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> Cc: Marc Zyngier <marc.zyngier@arm.com> >> Cc: Christoffer Dall <christoffer.dall@arm.com> >> Cc: kvmarm@lists.cs.columbia.edu >> --- >> arch/arm/include/asm/kvm_host.h | 2 ++ >> arch/arm64/include/asm/kvm_asm.h | 2 ++ >> arch/arm64/include/asm/kvm_emulate.h | 22 +++++++++++----------- >> arch/arm64/include/asm/kvm_host.h | 13 ++++++++++++- >> arch/arm64/include/asm/kvm_hyp.h | 2 +- >> arch/arm64/kvm/guest.c | 2 +- >> arch/arm64/kvm/hyp/switch.c | 23 +++++++++++++---------- >> arch/arm64/kvm/hyp/sysreg-sr.c | 21 ++++++++++++++++++++- >> arch/arm64/kvm/hyp/tlb.c | 6 +++++- >> virt/kvm/arm/arm.c | 1 + >> 10 files changed, 68 insertions(+), 26 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index ca56537..05706b4 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) >> kvm_call_hyp(__init_stage2_translation); >> } >> >> +static inline void __cpu_copy_hyp_conf(void) {} >> + >> static inline int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext) >> { >> return 0; >> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h >> index f5b79e9..8acd73f 100644 >> --- a/arch/arm64/include/asm/kvm_asm.h >> +++ b/arch/arm64/include/asm/kvm_asm.h >> @@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void); >> >> extern u32 __kvm_get_mdcr_el2(void); >> >> +extern void __kvm_populate_host_regs(void); >> + >> /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */ >> #define __hyp_this_cpu_ptr(sym) \ >> ({ \ >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h >> index 506386a..0dbe795 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> @@ -50,25 +50,25 @@ void kvm_inject_pabt32(struct kvm_vcpu *vcpu, unsigned long addr); >> >> static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) >> { >> - return !(vcpu->arch.hcr_el2 & HCR_RW); >> + return !(vcpu->arch.ctxt.hcr_el2 & HCR_RW); > > Putting hcr_el2 into struct kvm_cpu_context creates a lot of splatter > here, and I'm wondering whether it's really necessary. Otherwise, > we could just put the per-vcpu guest HCR_EL2 value in struct > kvm_vcpu_arch. I did like that in V4 version [1] but comments were raised that this was repetition of hcr_el2 field in 2 places and may be avoided. [1]: https://lkml.org/lkml/2019/1/4/433 > > Is the *host* hcr_el2 value really different per-vcpu? That looks > odd. I would have thought this is fixed across the system at KVM > startup time. > > Having a single global host hcr_el2 would also avoid the need for > __kvm_populate_host_regs(): instead, we just decide what HCR_EL2 is to > be ahead of time and set a global variable that we map into Hyp. > > > Or does the host HCR_EL2 need to vary at runtime for some reason I've > missed? This patch basically makes host hcr_el2 not to use fixed values like HCR_HOST_NVHE_FLAGS/HCR_HOST_VHE_FLAGS during context switch and hence saves those values at boot time. This patch is just preparation to configure host hcr_el2 dynamically. However currently it is same for all cpus. I suppose it is better to have host hcr_el2 as percpu to take care of heterogeneous systems. Currently even host mdcr_el2 is stored on percpu basis(arch/arm64/kvm/debug.c). > > [...] > > +void __hyp_text __kvm_populate_host_regs(void) > +{ > + struct kvm_cpu_context *host_ctxt; > + > + if (has_vhe()) > + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); > + else > + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > According to the comment by the definition of __hyp_this_cpu_ptr(), this > always works at Hyp. I also see other calls with no fallback > this_cpu_ptr() call like we have here. > > So, can we simply always call __hyp_this_cpu_ptr() here? Yes i missed this. Thanks, Amit D > > (I'm not familiar with this, myself.) > > Cheers > ---Dave >
Hi, On 2/25/19 11:09 PM, James Morse wrote: > Hi Amit, > > On 19/02/2019 09:24, Amit Daniel Kachhap wrote: >> From: Mark Rutland <mark.rutland@arm.com> >> >> When restoring HCR_EL2 for the host, KVM uses HCR_HOST_VHE_FLAGS, which >> is a constant value. This works today, as the host HCR_EL2 value is >> always the same, but this will get in the way of supporting extensions >> that require HCR_EL2 bits to be set conditionally for the host. >> >> To allow such features to work without KVM having to explicitly handle >> every possible host feature combination, this patch has KVM save/restore >> for the host HCR when switching to/from a guest HCR. The saving of the >> register is done once during cpu hypervisor initialization state and is >> just restored after switch from guest. >> >> For fetching HCR_EL2 during kvm initialisation, a hyp call is made using >> kvm_call_hyp and is helpful in NHVE case. >> >> For the hyp TLB maintenance code, __tlb_switch_to_host_vhe() is updated >> to toggle the TGE bit with a RMW sequence, as we already do in >> __tlb_switch_to_guest_vhe(). >> >> The value of hcr_el2 is now stored in struct kvm_cpu_context as both host >> and guest can now use this field in a common way. > > >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index ca56537..05706b4 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) >> kvm_call_hyp(__init_stage2_translation); >> } >> >> +static inline void __cpu_copy_hyp_conf(void) {} >> + > > I agree Mark's suggestion of adding 'host_ctxt' in here makes it clearer what it is. ok. > > >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h >> index 506386a..0dbe795 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h > > Hmmm, there is still a fair amount of churn due to moving the struct definition, but its > easy enough to ignore as its mechanical. A preparatory patch that switched as may as > possible to '*vcpu_hcr() = ' would cut the churn down some more, but I don't think its > worth the extra effort. > > >> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h >> index a80a7ef..6e65cad 100644 >> --- a/arch/arm64/include/asm/kvm_hyp.h >> +++ b/arch/arm64/include/asm/kvm_hyp.h >> @@ -151,7 +151,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs); >> bool __fpsimd_enabled(void); >> >> void activate_traps_vhe_load(struct kvm_vcpu *vcpu); >> -void deactivate_traps_vhe_put(void); >> +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu); > > I've forgotten why this is needed. You don't add a user of vcpu to > deactivate_traps_vhe_put() in this patch. > > >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >> index b0b1478..006bd33 100644 >> --- a/arch/arm64/kvm/hyp/switch.c >> +++ b/arch/arm64/kvm/hyp/switch.c >> @@ -191,7 +194,7 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu) > >> -void deactivate_traps_vhe_put(void) >> +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu) >> { >> u64 mdcr_el2 = read_sysreg(mdcr_el2); >> > > Why does deactivate_traps_vhe_put() need the vcpu? vcpu is needed for the next patch which saves/restore mdcr_el2. I will add this in that patch. > > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 7732d0b..1b2e05b 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -458,6 +459,16 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, >> >> static inline void __cpu_init_stage2(void) {} >> >> +/** >> + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers >> + * >> + * It is called once per-cpu during CPU hyp initialisation. >> + */ > > Is it just the boot cpu? > > >> +static inline void __cpu_copy_hyp_conf(void) >> +{ >> + kvm_call_hyp(__kvm_populate_host_regs); >> +} >> + > > >> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c >> index 68d6f7c..68ddc0f 100644 >> --- a/arch/arm64/kvm/hyp/sysreg-sr.c >> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c >> @@ -21,6 +21,7 @@ >> #include <asm/kvm_asm.h> >> #include <asm/kvm_emulate.h> >> #include <asm/kvm_hyp.h> >> +#include <asm/kvm_mmu.h> > > ... what's kvm_mmu.h needed for? > The __hyp_this_cpu_ptr() you add comes from kvm_asm.h. > > /me tries it. > > Heh, hyp_symbol_addr(). kvm_asm.h should include this, but can't because the > kvm_ksym_ref() dependency is the other-way round. This is just going to bite us somewhere > else later! > If we want to fix it now, moving hyp_symbol_addr() to kvm_asm.h would fix it. It's > generating adrp/add so the 'asm' label is fair, and it really should live with its EL1 > counterpart kvm_ksym_ref(). > Yes moving hyp_symbol_addr() fixes the dependency error. > >> @@ -294,7 +295,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) >> if (!has_vhe()) >> return; >> >> - deactivate_traps_vhe_put(); >> + deactivate_traps_vhe_put(vcpu); >> >> __sysreg_save_el1_state(guest_ctxt); >> __sysreg_save_user_state(guest_ctxt); >> @@ -316,3 +317,21 @@ void __hyp_text __kvm_enable_ssbs(void) >> "msr sctlr_el2, %0" >> : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS)); >> } >> + >> +/** >> + * __kvm_populate_host_regs - Stores host register values >> + * >> + * This function acts as a function handler parameter for kvm_call_hyp and >> + * may be called from EL1 exception level to fetch the register value. >> + */ >> +void __hyp_text __kvm_populate_host_regs(void) >> +{ >> + struct kvm_cpu_context *host_ctxt; > > >> + if (has_vhe()) >> + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); >> + else >> + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); > > You can use __hyp_this_cpu_ptr() here, even on VHE. > > For VHE the guts are the same and its simpler to use the same version in both cases. > > > __hyp_this_cpu_ptr(sym) == hyp_symbol_addr(sym) + tpidr_el2; > > hyp_symbol_addr() here is just to guarantee the address is generated based on where we're > executing from, not loaded from a literal pool which would give us the link-time address. > (or whenever kaslr applied the relocations). This matters for non-VHE because the compiler > can't know the code has an EL2 address as well as its link-time address. > > This doesn't matter for VHE, as there is no additional different address. > > (the other trickery is on non-VHE the tpidr_el2 value isn't actually the same as the > hosts.. but on VHE it is) > > Thanks for the details. >> + host_ctxt->hcr_el2 = read_sysreg(hcr_el2); >> +} > > >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 9e350fd3..8e18f7f 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -1328,6 +1328,7 @@ static void cpu_hyp_reinit(void) >> cpu_init_hyp_mode(NULL); >> >> kvm_arm_init_debug(); >> + __cpu_copy_hyp_conf(); > > Your commit message says: > | The saving of the register is done once during cpu hypervisor initialization state > > But cpu_hyp_reinit() is called each time secondary CPUs come online. Its also called as > part of the cpu-idle mechanism via hyp_init_cpu_pm_notifier(). cpu-idle can ask the > firmware to power-off the CPU until an interrupt becomes pending for it. KVM's EL2 state > disappears when this happens, these calls take care of setting it back up again. On Juno, > this can happen tens of times a second, and this adds an extra call to EL2. > > init_subsystems() would be the alternative place for this, but it wouldn't catch CPUs that > came online after booting. I think you need something in cpu_hyp_reinit() or > __cpu_copy_hyp_conf() to ensure it only happens once per CPU. ok i will check on it. > > I think you can test whether the HCR_EL2 value is zero, assuming zero means uninitialised. > A VHE system would always set E2H, and a non-VHE system has to set RW. It is not zero and is set to initial values. Thanks, Amit D > > >> if (vgic_present) >> kvm_vgic_init_cpu_hardware(); >> > > > Thanks, > > James >
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index ca56537..05706b4 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -273,6 +273,8 @@ static inline void __cpu_init_stage2(void) kvm_call_hyp(__init_stage2_translation); } +static inline void __cpu_copy_hyp_conf(void) {} + static inline int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext) { return 0; diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index f5b79e9..8acd73f 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -80,6 +80,8 @@ extern void __vgic_v3_init_lrs(void); extern u32 __kvm_get_mdcr_el2(void); +extern void __kvm_populate_host_regs(void); + /* Home-grown __this_cpu_{ptr,read} variants that always work at HYP */ #define __hyp_this_cpu_ptr(sym) \ ({ \ diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 506386a..0dbe795 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -50,25 +50,25 @@ void kvm_inject_pabt32(struct kvm_vcpu *vcpu, unsigned long addr); static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) { - return !(vcpu->arch.hcr_el2 & HCR_RW); + return !(vcpu->arch.ctxt.hcr_el2 & HCR_RW); } static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) { - vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; + vcpu->arch.ctxt.hcr_el2 = HCR_GUEST_FLAGS; if (is_kernel_in_hyp_mode()) - vcpu->arch.hcr_el2 |= HCR_E2H; + vcpu->arch.ctxt.hcr_el2 |= HCR_E2H; if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN)) { /* route synchronous external abort exceptions to EL2 */ - vcpu->arch.hcr_el2 |= HCR_TEA; + vcpu->arch.ctxt.hcr_el2 |= HCR_TEA; /* trap error record accesses */ - vcpu->arch.hcr_el2 |= HCR_TERR; + vcpu->arch.ctxt.hcr_el2 |= HCR_TERR; } if (cpus_have_const_cap(ARM64_HAS_STAGE2_FWB)) - vcpu->arch.hcr_el2 |= HCR_FWB; + vcpu->arch.ctxt.hcr_el2 |= HCR_FWB; if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) - vcpu->arch.hcr_el2 &= ~HCR_RW; + vcpu->arch.ctxt.hcr_el2 &= ~HCR_RW; /* * TID3: trap feature register accesses that we virtualise. @@ -76,22 +76,22 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) * are currently virtualised. */ if (!vcpu_el1_is_32bit(vcpu)) - vcpu->arch.hcr_el2 |= HCR_TID3; + vcpu->arch.ctxt.hcr_el2 |= HCR_TID3; } static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu) { - return (unsigned long *)&vcpu->arch.hcr_el2; + return (unsigned long *)&vcpu->arch.ctxt.hcr_el2; } static inline void vcpu_clear_wfe_traps(struct kvm_vcpu *vcpu) { - vcpu->arch.hcr_el2 &= ~HCR_TWE; + vcpu->arch.ctxt.hcr_el2 &= ~HCR_TWE; } static inline void vcpu_set_wfe_traps(struct kvm_vcpu *vcpu) { - vcpu->arch.hcr_el2 |= HCR_TWE; + vcpu->arch.ctxt.hcr_el2 |= HCR_TWE; } static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7732d0b..1b2e05b 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -203,6 +203,8 @@ struct kvm_cpu_context { u32 copro[NR_COPRO_REGS]; }; + /* HYP host/guest configuration */ + u64 hcr_el2; struct kvm_vcpu *__hyp_running_vcpu; }; @@ -212,7 +214,6 @@ struct kvm_vcpu_arch { struct kvm_cpu_context ctxt; /* HYP configuration */ - u64 hcr_el2; u32 mdcr_el2; /* Exception Information */ @@ -458,6 +459,16 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, static inline void __cpu_init_stage2(void) {} +/** + * __cpu_copy_hyp_conf - copy the boot hyp configuration registers + * + * It is called once per-cpu during CPU hyp initialisation. + */ +static inline void __cpu_copy_hyp_conf(void) +{ + kvm_call_hyp(__kvm_populate_host_regs); +} + /* Guest/host FPSIMD coordination helpers */ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h index a80a7ef..6e65cad 100644 --- a/arch/arm64/include/asm/kvm_hyp.h +++ b/arch/arm64/include/asm/kvm_hyp.h @@ -151,7 +151,7 @@ void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs); bool __fpsimd_enabled(void); void activate_traps_vhe_load(struct kvm_vcpu *vcpu); -void deactivate_traps_vhe_put(void); +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu); u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt); void __noreturn __hyp_do_panic(unsigned long, ...); diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index dd436a5..e2f0268 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -345,7 +345,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu, struct kvm_vcpu_events *events) { - events->exception.serror_pending = !!(vcpu->arch.hcr_el2 & HCR_VSE); + events->exception.serror_pending = !!(vcpu->arch.ctxt.hcr_el2 & HCR_VSE); events->exception.serror_has_esr = cpus_have_const_cap(ARM64_HAS_RAS_EXTN); if (events->exception.serror_pending && events->exception.serror_has_esr) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index b0b1478..006bd33 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -126,7 +126,7 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) { - u64 hcr = vcpu->arch.hcr_el2; + u64 hcr = vcpu->arch.ctxt.hcr_el2; write_sysreg(hcr, hcr_el2); @@ -139,10 +139,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) __activate_traps_nvhe(vcpu); } -static void deactivate_traps_vhe(void) +static void deactivate_traps_vhe(struct kvm_cpu_context *host_ctxt) { extern char vectors[]; /* kernel exception vectors */ - write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); + write_sysreg(host_ctxt->hcr_el2, hcr_el2); /* * ARM erratum 1165522 requires the actual execution of the above @@ -155,7 +155,7 @@ static void deactivate_traps_vhe(void) write_sysreg(vectors, vbar_el1); } -static void __hyp_text __deactivate_traps_nvhe(void) +static void __hyp_text __deactivate_traps_nvhe(struct kvm_cpu_context *host_ctxt) { u64 mdcr_el2 = read_sysreg(mdcr_el2); @@ -165,25 +165,28 @@ static void __hyp_text __deactivate_traps_nvhe(void) mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT; write_sysreg(mdcr_el2, mdcr_el2); - write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); + write_sysreg(host_ctxt->hcr_el2, hcr_el2); write_sysreg(CPTR_EL2_DEFAULT, cptr_el2); } static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) { + struct kvm_cpu_context *host_ctxt; + + host_ctxt = vcpu->arch.host_cpu_context; /* * If we pended a virtual abort, preserve it until it gets * cleared. See D1.14.3 (Virtual Interrupts) for details, but * the crucial bit is "On taking a vSError interrupt, * HCR_EL2.VSE is cleared to 0." */ - if (vcpu->arch.hcr_el2 & HCR_VSE) - vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); + if (vcpu->arch.ctxt.hcr_el2 & HCR_VSE) + vcpu->arch.ctxt.hcr_el2 = read_sysreg(hcr_el2); if (has_vhe()) - deactivate_traps_vhe(); + deactivate_traps_vhe(host_ctxt); else - __deactivate_traps_nvhe(); + __deactivate_traps_nvhe(host_ctxt); } void activate_traps_vhe_load(struct kvm_vcpu *vcpu) @@ -191,7 +194,7 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu) __activate_traps_common(vcpu); } -void deactivate_traps_vhe_put(void) +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu) { u64 mdcr_el2 = read_sysreg(mdcr_el2); diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c index 68d6f7c..68ddc0f 100644 --- a/arch/arm64/kvm/hyp/sysreg-sr.c +++ b/arch/arm64/kvm/hyp/sysreg-sr.c @@ -21,6 +21,7 @@ #include <asm/kvm_asm.h> #include <asm/kvm_emulate.h> #include <asm/kvm_hyp.h> +#include <asm/kvm_mmu.h> /* * Non-VHE: Both host and guest must save everything. @@ -294,7 +295,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) if (!has_vhe()) return; - deactivate_traps_vhe_put(); + deactivate_traps_vhe_put(vcpu); __sysreg_save_el1_state(guest_ctxt); __sysreg_save_user_state(guest_ctxt); @@ -316,3 +317,21 @@ void __hyp_text __kvm_enable_ssbs(void) "msr sctlr_el2, %0" : "=&r" (tmp) : "L" (SCTLR_ELx_DSSBS)); } + +/** + * __kvm_populate_host_regs - Stores host register values + * + * This function acts as a function handler parameter for kvm_call_hyp and + * may be called from EL1 exception level to fetch the register value. + */ +void __hyp_text __kvm_populate_host_regs(void) +{ + struct kvm_cpu_context *host_ctxt; + + if (has_vhe()) + host_ctxt = this_cpu_ptr(&kvm_host_cpu_state); + else + host_ctxt = __hyp_this_cpu_ptr(kvm_host_cpu_state); + + host_ctxt->hcr_el2 = read_sysreg(hcr_el2); +} diff --git a/arch/arm64/kvm/hyp/tlb.c b/arch/arm64/kvm/hyp/tlb.c index 76c3086..c5e7144 100644 --- a/arch/arm64/kvm/hyp/tlb.c +++ b/arch/arm64/kvm/hyp/tlb.c @@ -86,12 +86,16 @@ static hyp_alternate_select(__tlb_switch_to_guest, static void __hyp_text __tlb_switch_to_host_vhe(struct kvm *kvm, struct tlb_inv_context *cxt) { + u64 val; + /* * We're done with the TLB operation, let's restore the host's * view of HCR_EL2. */ write_sysreg(0, vttbr_el2); - write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); + val = read_sysreg(hcr_el2); + val |= HCR_TGE; + write_sysreg(val, hcr_el2); isb(); if (cpus_have_const_cap(ARM64_WORKAROUND_1165522)) { diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index 9e350fd3..8e18f7f 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -1328,6 +1328,7 @@ static void cpu_hyp_reinit(void) cpu_init_hyp_mode(NULL); kvm_arm_init_debug(); + __cpu_copy_hyp_conf(); if (vgic_present) kvm_vgic_init_cpu_hardware();