Re: [PATCH v15 12/37] KVM: arm64: CCA: Support the VGIC in realms

On 07/15 15:28, Steven Price wrote:
> The RMM provides emulation of a VGIC to the realm guest. With RMM v2.0
> the registers are passed in the system registers so this works similar
> to a normal guest, but kvm_arch_vcpu_put() need reordering to early out,
> and realm guests don't support GICv2 even if the host does.
> 
> Signed-off-by: Steven Price <[email protected]>

Hi Steven,

I've been testing this series and found that when the host CPU doesn't have
ARM64_HAS_ICH_HCR_EL2_TDIR this series doesn't work as expected.

Since commit 2a28810cbb8b ("KVM: arm64: GICv3: Detect and work around the lack
of ICV_DIR_EL1 trapping"), when the host CPU doesn't support this feature, KVM
traps all GIC sysreg accesses in the common group. However, currently trap
handlers for ICC_{PMR,RPR,CTLR}_EL1 registers are missing [0]. So when Realm
guests try to access those registers, KVM traps them but just emits the warning
shown in [1], and Realm guests fail to boot.

As far as I can tell, the CCA requirements don't require the
ARM64_HAS_ICH_HCR_EL2_TDIR feature. If that's the case, this seems to be a
problem. Is there any workaround for this issue, or should we implement trap
handlers for those registers?

Thanks,
Kohei

[0]
static const struct sys_reg_desc sys_reg_descs[] = {
    ...
	{ SYS_DESC(SYS_ICC_PMR_EL1), undef_access },
	{ SYS_DESC(SYS_ICC_RPR_EL1), undef_access },
	{ SYS_DESC(SYS_ICC_CTLR_EL1), undef_access },
};

[1]
------------[ cut here ]------------
Unexpected undefined exception injection to REC
WARNING: arch/arm64/kvm/inject_fault.c:330 at kvm_inject_undefined+0x100/0x148, CPU#1: kvm-vcpu-0/370
Modules linked in:
CPU: 1 UID: 0 PID: 370 Comm: kvm-vcpu-0 Not tainted 7.2.0-rc1-00096-gd6d40e6a3103 #161 PREEMPT(lazy)
[...]
Call trace:
 kvm_inject_undefined+0x100/0x148 (P)
 undef_access+0x18/0x30
 perform_access+0x54/0x1d0
 kvm_handle_sys_reg+0xfc/0x308
 rec_exit_sys_reg+0x34/0x168
 handle_rec_exit+0x12c/0x4c8
 kvm_arch_vcpu_ioctl_run+0x7a0/0x9a8
 kvm_vcpu_ioctl+0x180/0xb50
 __arm64_sys_ioctl+0xb4/0x118
 invoke_syscall.constprop.0+0xb8/0x120
 do_el0_svc+0x48/0xc8
 el0_svc+0x44/0x248
 el0t_64_sync_handler+0xa0/0xe8
 el0t_64_sync+0x198/0x1a0
---[ end trace 0000000000000000 ]---
 [...]

> ---
> Changes from v12:
>  * GIC registers are now passed in the system registers rather than via
>    rec_entry/rec_exit which removes most of the changes.
> Changes from v11:
>  * Minor changes to align with the previous patches. Note that the VGIC
>    handling will change with RMM v2.0.
> Changes from v10:
>  * Make sure we sync the VGIC v4 state, and only populate valid lrs from
>    the list.
> Changes from v9:
>  * Copy gicv3_vmcr from the RMM at the same time as gicv3_hcr rather
>    than having to handle that as a special case.
> Changes from v8:
>  * Propagate gicv3_hcr to from the RMM.
> Changes from v5:
>  * Handle RMM providing fewer GIC LRs than the hardware supports.
> ---
>  arch/arm64/kvm/arm.c            | 11 ++++++++---
>  arch/arm64/kvm/vgic/vgic-init.c |  2 +-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 38fac98cd6a4..2959a1451232 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -791,19 +791,24 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  		kvm_call_hyp_nvhe(__pkvm_vcpu_put);
>  	}
>  
> +	kvm_timer_vcpu_put(vcpu);
> +	kvm_vgic_put(vcpu);
>  	kvm_vcpu_put_debug(vcpu);
> +
> +	vcpu->cpu = -1;
> +
> +	if (vcpu_is_rec(vcpu))
> +		return;
> +
>  	kvm_arch_vcpu_put_fp(vcpu);
>  	if (has_vhe())
>  		kvm_vcpu_put_vhe(vcpu);
> -	kvm_timer_vcpu_put(vcpu);
> -	kvm_vgic_put(vcpu);
>  	kvm_vcpu_pmu_restore_host(vcpu);
>  	if (vcpu_has_nv(vcpu))
>  		kvm_vcpu_put_hw_mmu(vcpu);
>  	kvm_arm_vmid_clear_active();
>  
>  	vcpu_clear_on_unsupported_cpu(vcpu);
> -	vcpu->cpu = -1;
>  }
>  
>  static void __kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 907057881b26..fd77db35ef02 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -81,7 +81,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>  	 * the proper checks already.
>  	 */
>  	if (type == KVM_DEV_TYPE_ARM_VGIC_V2 &&
> -		!kvm_vgic_global_state.can_emulate_gicv2)
> +	    (!kvm_vgic_global_state.can_emulate_gicv2 || kvm_is_realm(kvm)))
>  		return -ENODEV;
>  
>  	/*
> -- 
> 2.43.0
> 
>