Re: [PATCH v1 3/4] KVM: arm64: Use the host's HCR_EL2 for non-protected VMs in pKVM

[Marc Zyngier <maz@kernel.org>]

On Fri, 25 Sep 2026 10:06:18 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
> 
> In pKVM, a non-protected VM gets only TWI, TWE and VSE from the HCR_EL2
> the host computes for it. EL2 sets the rest in pkvm_vcpu_reset_hcr(),
> which covers only part of vcpu_set_hcr(). On a CPU with MTE the VM can
> then read GMID_EL1, on one without FGT it can execute a TLBI OS its ID
> registers hide, and it never gets the host's TVM, VI or VF.
> 
> Use the host's HCR_EL2 on every entry instead, except for the bits EL2
> owns. The other bits only control what the VM's own execution traps on
> and which virtual exceptions are pending for it. The host computes them
> from the vCPU's features, ID registers and flags, which EL2 already
> takes from the host for a non-protected VM, as it takes MDCR_EL2,
> HCRX_EL2 and the fine-grained traps.
> 
> ATA, an owned bit, stays clear, as pKVM doesn't support MTE for any
> guest. TID2 and TID4 move to pvm_init_traps_hcr(), since EL2 now sets
> them only for a protected VM. A protected VM's HCR_EL2 is unchanged.

But the host does set these bits for non-protected VMs. What is going
to honor these traps?

No mention of why you are adding HCR_EL2_GPF here?

>
> Fixes: b56680de9c648 ("KVM: arm64: Initialize trap register values in hyp in pKVM")
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_arm.h       |  1 +
>  arch/arm64/kvm/hyp/include/nvhe/pkvm.h | 13 +++++++++++++
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c     |  7 ++++---
>  arch/arm64/kvm/hyp/nvhe/pkvm.c         | 18 ++++++++----------
>  4 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 4bfbd827c5aa7..8d187650e463e 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -30,6 +30,7 @@
>  #define HCR_AMVOFFEN	__HCR(AMVOFFEN)
>  #define HCR_TICAB	__HCR(TICAB)
>  #define HCR_TID4	__HCR(TID4)
> +#define HCR_GPF		__HCR(GPF)
>  #define HCR_FIEN	__HCR(FIEN)
>  #define HCR_FWB		__HCR(FWB)
>  #define HCR_NV2		__HCR(NV2)
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/pkvm.h b/arch/arm64/kvm/hyp/include/nvhe/pkvm.h
> index c904647d2f760..75b1122db4c91 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/pkvm.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/pkvm.h
> @@ -12,6 +12,19 @@
>  #include <nvhe/gfp.h>
>  #include <nvhe/spinlock.h>
>  
> +/*
> + * HCR_EL2 bits EL2 owns for a non-protected VM, whatever the host sets: those
> + * that restrict the guest, configure EL2 or what it switches (E2H, RW), or
> + * enable state EL2 doesn't switch or support. RES0 is included, so a bit comes
> + * from the host only once arch/arm64/tools/sysreg describes it.
> + */
> +#define PKVM_HCR_EL2_OWNED	((HCR_GUEST_FLAGS & ~(HCR_TWI | HCR_TWE)) | HCR_BSU |	\
> +				 HCR_E2H | HCR_TGE | HCR_TEA | HCR_GPF | HCR_TERR |	\
> +				 HCR_FWB | HCR_DC | HCR_ID | HCR_CD | HCR_NV |		\
> +				 HCR_NV1 | HCR_NV2 | HCR_API | HCR_APK | HCR_ATA |	\
> +				 HCR_DCT | HCR_FIEN | HCR_AMVOFFEN | HCR_ENSCXT |	\
> +				 HCR_EL2_RES0)
> +

The name of the macro doesn't indicate that this only applies to
protected VM.

Also, please don't add new uses of the compat HCR macros. I really
want to remove them (probably post -rc1).

>  /*
>   * Holds the relevant data for maintaining the vcpu state completely at hyp.
>   */
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 9a3b92e626adb..dec99d5bbee78 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -216,6 +216,7 @@ static void sync_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
>  static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  {
>  	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +	u64 host_hcr_mask = HCR_TWI | HCR_TWE | HCR_VSE;
>  
>  	fpsimd_sve_flush();
>  	flush_debug_state(hyp_vcpu);
> @@ -228,6 +229,7 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	if (!pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
>  		if (vcpu_get_flag(host_vcpu, PKVM_HOST_STATE_DIRTY))
>  			flush_hyp_vcpu_state(hyp_vcpu);
> +		host_hcr_mask = ~PKVM_HCR_EL2_OWNED;

This feels fragile. You start with a restrictive set (TWI, TWE, VSE),
and then drop it all. At this point, I have no idea what you are
letting in.

It would be better to express things in a consistent way:

- either the bits that are controlled by the host for either protected
  and non-protected guests,

- or the bits that are controlled by the hypervisor for either cases.

Here, you're mixing both, and that's confusing.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.