[PATCH v11 00/43] KVM: arm64: Nested Virtualization support (FEAT_NV2 only)

[Ganapatrao Kulkarni]

On 24-11-2023 08:02 pm, Marc Zyngier wrote:
> On Fri, 24 Nov 2023 13:22:22 +0000,
> Ganapatrao Kulkarni <gankulkarni at os.amperecomputing.com> wrote:
>>
>>> How is this value possible if the write to HCR_EL2 has taken place?
>>> When do you sample this?
>>
>> I am not sure how and where it got set. I think, whatever it is set,
>> it is due to false return of vcpu_el2_e2h_is_set(). Need to
>> understand/debug.
>> The vhcr_el2 value I have shared is traced along with hcr in function
>> __activate_traps/__compute_hcr.
> 
> Here's my hunch:
> 
> The guest boots with E2H=0, because we don't advertise anything else
> on your HW. So we run with NV1=1 until we try to *upgrade* to VHE. NV2
> means that HCR_EL2 is writable (to memory) without a trap. But we're
> still running with NV1=1.
> 
> Subsequently, we access a sysreg that should never trap for a VHE
> guest, but we're with the wrong config. Bad things happen.
> 
> Unfortunately, NV2 is pretty much incompatible with E2H being updated,
> because it cannot perform the changes that this would result into at
> the point where they should happen. We can try and do a best effort
> handling, but you can always trick it.
> 
> Anyway, can you see if the hack below helps? I'm not keen on it at
> all, but this would be a good data point.

Thanks Marc, this diff fixes the issue.
Just wondering what is changed w.r.t to L1 handling from V10 to V11 that 
it requires this trick?
Also why this was not seen on your platform, is it E2H0 enabled?

> 
> 	M.
> 
>  From c4b856221661393b884cbf673d100faaa8dc018a Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz at kernel.org>
> Date: Fri, 26 May 2023 12:16:05 +0100
> Subject: [PATCH] KVM: arm64: Opportunistically track HCR_EL2.E2H being flipped
> 
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>   arch/arm64/include/asm/kvm_host.h       |  9 +++++++--
>   arch/arm64/kvm/hyp/include/hyp/switch.h | 13 +++++++++++++
>   arch/arm64/kvm/hyp/vhe/switch.c         | 10 ++++++++--
>   3 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c91f607e989d..d45ef41de5fb 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -655,6 +655,9 @@ struct kvm_vcpu_arch {
>   	/* State flags for kernel bookkeeping, unused by the hypervisor code */
>   	u8 sflags;
>   
> +	/* Bookkeeping flags for NV */
> +	u8 nvflags;
> +
>   	/*
>   	 * Don't run the guest (internal implementation need).
>   	 *
> @@ -858,8 +861,6 @@ struct kvm_vcpu_arch {
>   #define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
>   /* Save TRBE context if active  */
>   #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
> -/* vcpu running in HYP context */
> -#define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
>   
>   /* SVE enabled for host EL0 */
>   #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> @@ -878,6 +879,10 @@ struct kvm_vcpu_arch {
>   /* WFI instruction trapped */
>   #define IN_WFI			__vcpu_single_flag(sflags, BIT(7))
>   
> +/* vcpu running in HYP context */
> +#define VCPU_HYP_CONTEXT	__vcpu_single_flag(nvflags, BIT(0))
> +/* vcpu entered with HCR_EL2.E2H set */
> +#define VCPU_HCR_E2H		__vcpu_single_flag(nvflags, BIT(1))
>   
>   /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>   #define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) +	\
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index aed2ea35082c..9c1346116d61 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -669,6 +669,19 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
>   	 */
>   	synchronize_vcpu_pstate(vcpu, exit_code);
>   
> +	if (vcpu_has_nv(vcpu) &&
> +	    (!!vcpu_get_flag(vcpu, VCPU_HCR_E2H) ^ vcpu_el2_e2h_is_set(vcpu))) {
> +		if (vcpu_el2_e2h_is_set(vcpu)) {
> +			sysreg_clear_set(hcr_el2, HCR_NV1, 0);
> +			vcpu_set_flag(vcpu, VCPU_HCR_E2H);
> +		} else {
> +			sysreg_clear_set(hcr_el2, 0, HCR_NV1);
> +			vcpu_clear_flag(vcpu, VCPU_HCR_E2H);
> +		}
> +
> +		return true;
> +	}
> +
>   	/*
>   	 * Check whether we want to repaint the state one way or
>   	 * another.
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 8d1e9d1adabe..395aaa06f358 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -447,10 +447,16 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>   	sysreg_restore_guest_state_vhe(guest_ctxt);
>   	__debug_switch_to_guest(vcpu);
>   
> -	if (is_hyp_ctxt(vcpu))
> +	if (is_hyp_ctxt(vcpu)) {
> +		if (vcpu_el2_e2h_is_set(vcpu))
> +			vcpu_set_flag(vcpu, VCPU_HCR_E2H);
> +		else
> +			vcpu_clear_flag(vcpu, VCPU_HCR_E2H);
> +
>   		vcpu_set_flag(vcpu, VCPU_HYP_CONTEXT);
> -	else
> +	} else {
>   		vcpu_clear_flag(vcpu, VCPU_HYP_CONTEXT);
> +	}
>   
>   	do {
>   		/* Jump in the fire! */

Thanks,
Ganapat