[PATCH v10 00/59] KVM: arm64: ARMv8.3/8.4 Nested Virtualization support

[Eric Auger]

Hi Marc,

On 5/16/23 22:28, Marc Zyngier wrote:
> On Tue, 16 May 2023 17:53:14 +0100,
> Eric Auger <eauger at redhat.com> wrote:
>>
>> Hi Marc,
>>
>> On 5/15/23 19:30, Marc Zyngier wrote:
>>> This is the 4th drop of NV support on arm64 for this year.
>>>
>>> For the previous episodes, see [1].
>>>
>>> What's changed:
>>>
>>> - New framework to track system register traps that are reinjected in
>>>   guest EL2. It is expected to replace the discrete handling we have
>>>   enjoyed so far, which didn't scale at all. This has already fixed a
>>>   number of bugs that were hidden (a bunch of traps were never
>>>   forwarded...). Still a work in progress, but this is going in the
>>>   right direction.
>>>
>>> - Allow the L1 hypervisor to have a S2 that has an input larger than
>>>   the L0 IPA space. This fixes a number of subtle issues, depending on
>>>   how the initial guest was created.
>>>
>>> - Consequently, the patch series has gone longer again. Boo. But
>>>   hopefully some of it is easier to review...
>>>
>>> [1] https://lore.kernel.org/r/20230405154008.3552854-1-maz@kernel.org
>>
>> I have started testing this and when booting my fedora guest I get
>>
>> [  151.796544] kvm [7617]: Unsupported guest sys_reg access at:
>> 23f425fd0 [80000209]
>> [  151.796544]  { Op0( 3), Op1( 3), CRn(14), CRm( 3), Op2( 1), func_write },
>>
>> as soon as the host has kvm-arm.mode=nested
>>
>> This seems to be triggered very early by EDK2
>> (ArmPkg/Drivers/TimerDxe/TimerDxe.c).
>>
>> If I am not wrong this CNTV_CTL_EL0. Do you have any idea?
> 
> So here's my current analysis:
> 
> I assume you are running EDK2 as the L1 guest in a nested
> configuration. I also assume that you are not running on an Apple
> CPU. If these assumptions are correct, then EDK2 runs at vEL2, and is
> in nVHE mode.
> 
> Finally, I'm going to assume that your implementation has FEAT_ECV and
> FEAT_NV2, because I can't see how it could fail otherwise.
all the above is correct.
> 
> In these precise conditions, KVM sets the CNTHCTL_EL2.EL1TVT bit so
> that we can trap the EL0 virtual timer and faithfully emulate it (it
> is otherwise written to memory, which isn't very helpful).

indeed
> 
> As it turns out, we don't handle these traps. I didn't spot it because
> my test machines are all Apple boxes that don't have a nVHE mode, so
> nothing on the nVHE path is getting *ANY* coverage. Hint: having
> access to such a machine would help (shipping address on request!).
> Otherwise, I'll eventually kill the nVHE support altogether.
> 
> I have written the following patch, which compiles, but that I cannot
> test with my current setup. Could you please give it a go?

with the patch below, my guest boots nicely. You did it great on the 1st
shot!!! So this fixes my issue. I will continue testing the v10.

Thank you again!

Eric


> 
> Thanks again,
> 
> 	M.
> 
> From feb03b57de0bcb83254a2d6a3ce320f5e39434b6 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz at kernel.org>
> Date: Tue, 16 May 2023 21:06:20 +0100
> Subject: [PATCH] KVM: arm64: Handle virtual timer traps when
>  CNTHCTL_EL2.EL1TVT is set
> 
> Signed-off-by: Marc Zyngier <maz at kernel.org>
> ---
>  arch/arm64/include/asm/sysreg.h |  1 +
>  arch/arm64/kvm/sys_regs.c       | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 72ff6df5d75b..77a61179ea37 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -436,6 +436,7 @@
>  #define SYS_CNTP_CTL_EL0		sys_reg(3, 3, 14, 2, 1)
>  #define SYS_CNTP_CVAL_EL0		sys_reg(3, 3, 14, 2, 2)
>  
> +#define SYS_CNTV_TVAL_EL0		sys_reg(3, 3, 14, 3, 0)
>  #define SYS_CNTV_CTL_EL0		sys_reg(3, 3, 14, 3, 1)
>  #define SYS_CNTV_CVAL_EL0		sys_reg(3, 3, 14, 3, 2)
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 27a29dcbfcd2..9aa9c4e4b4d6 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1328,6 +1328,14 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  		treg = TIMER_REG_TVAL;
>  		break;
>  
> +	case SYS_CNTV_TVAL_EL0:
> +		if (is_hyp_ctxt(vcpu) && vcpu_el2_e2h_is_set(vcpu))
> +			tmr = TIMER_HVTIMER;
> +		else
> +			tmr = TIMER_VTIMER;
> +		treg = TIMER_REG_TVAL;
> +		break;
> +
>  	case SYS_AARCH32_CNTP_TVAL:
>  	case SYS_CNTP_TVAL_EL02:
>  		tmr = TIMER_PTIMER;
> @@ -1357,6 +1365,14 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  		treg = TIMER_REG_CTL;
>  		break;
>  
> +	case SYS_CNTV_CTL_EL0:
> +		if (is_hyp_ctxt(vcpu) && vcpu_el2_e2h_is_set(vcpu))
> +			tmr = TIMER_HVTIMER;
> +		else
> +			tmr = TIMER_VTIMER;
> +		treg = TIMER_REG_CTL;
> +		break;
> +
>  	case SYS_AARCH32_CNTP_CTL:
>  	case SYS_CNTP_CTL_EL02:
>  		tmr = TIMER_PTIMER;
> @@ -1386,6 +1402,14 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  		treg = TIMER_REG_CVAL;
>  		break;
>  
> +	case SYS_CNTV_CVAL_EL0:
> +		if (is_hyp_ctxt(vcpu) && vcpu_el2_e2h_is_set(vcpu))
> +			tmr = TIMER_HVTIMER;
> +		else
> +			tmr = TIMER_VTIMER;
> +		treg = TIMER_REG_CVAL;
> +		break;
> +
>  	case SYS_AARCH32_CNTP_CVAL:
>  	case SYS_CNTP_CVAL_EL02:
>  		tmr = TIMER_PTIMER;
> @@ -2510,6 +2534,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_CNTP_CTL_EL0), access_arch_timer },
>  	{ SYS_DESC(SYS_CNTP_CVAL_EL0), access_arch_timer },
>  
> +	{ SYS_DESC(SYS_CNTV_TVAL_EL0), access_arch_timer },
> +	{ SYS_DESC(SYS_CNTV_CTL_EL0), access_arch_timer },
> +	{ SYS_DESC(SYS_CNTV_CVAL_EL0), access_arch_timer },
> +
>  	/* PMEVCNTRn_EL0 */
>  	PMU_PMEVCNTR_EL0(0),
>  	PMU_PMEVCNTR_EL0(1),