On Tue, Apr 02, 2019 at 07:57:12AM +0530, Amit Daniel Kachhap wrote:
> From: Mark Rutland <mark.rutland@xxxxxxx>
>
> 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 non-VHE 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@xxxxxxx>
> [Added cpu_init_host_ctxt, hcr_el2 field in struct kvm_cpu_context,
> save hcr_el2 in hyp init stage]
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@xxxxxxx>
> Reviewed-by: James Morse <james.morse@xxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Cc: Christoffer Dall <christoffer.dall@xxxxxxx>
> Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx
[...]
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a01fe087..3b09fd0 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -209,6 +209,8 @@ struct kvm_cpu_context {
> u32 copro[NR_COPRO_REGS];
> };
>
> + /* HYP host/guest configuration */
> + u64 hcr_el2;
Minor nit: You could delete "host/guest" from the comment here. This is
implied by the fact that the member is in struct kvm_cpu_context in the
first place.
> struct kvm_vcpu *__hyp_running_vcpu;
> };
[...]
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 3563fe6..f5cefa1 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
[...]
> @@ -159,9 +159,10 @@ static void deactivate_traps_vhe(void)
> }
> NOKPROBE_SYMBOL(deactivate_traps_vhe);
>
> -static void __hyp_text __deactivate_traps_nvhe(void)
> +static void __hyp_text __deactivate_traps_nvhe(struct kvm_cpu_context *host_ctxt)
Where __hyp_text functions accept pointer arguments, they are usually
hyp pointers already... (see below)
> {
> u64 mdcr_el2 = read_sysreg(mdcr_el2);
> + struct kvm_cpu_context *hyp_host_ctxt = kern_hyp_va(host_ctxt);
>
> __deactivate_traps_common();
>
> @@ -169,25 +170,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(hyp_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;
host_ctxt is not otherwise used here, so can we convert it up-front so
that the argument to __deactivate_traps_nvhe() and
deactivate_traps_vhe() is a hyp pointer already?
So:
struct kvm_cpu_context *hyp_host_ctxt;
hyp_host_ctxt = kern_hyp_va(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);
Then just pass hyp_host_ctxt to both of these, and drop the
kern_hyp_va() conversion from __deactivate_traps_nvhe().
This may be a bit less confusing.
Alternatively, just pass in the vcpu pointer (since this pattern is
already well established all over the place).
Another option could be to pull the hcr_el2 write out of the backends
entirely and put it in this common code instead. This doesn't look
straightforward though (or at least, I don't remember enough about how
all these traps handling functions fit together...)
[...]
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
