Re: [PATCH 1/2] KVM: arm64: Copy FGT traps to unprotected pKVM VCPU on VCPU load

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Will,

On Fri, Dec 12, 2025 at 05:36:01AM +0900, Will Deacon wrote:
> Hey Alexandru,
> 
> [+Fuad as we ran into this last week]
> 
> On Wed, Dec 10, 2025 at 01:21:01PM +0000, Alexandru Elisei wrote:
> > Commit fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> > introduced per-VCPU FGT traps. For an unprotected pKVM VCPU, the untrusted
> > host FGT configuration is copied in pkvm_vcpu_init_traps(), which is called
> > from __pkvm_init_vcpu(). __pkvm_init_vcpu() is called once per VCPU (when
> > the VCPU is first run) which means that the uninitialized, zero, values for
> > the FGT registers end up being used for the entire lifetime of the VCPU.
> > This causes both unwanted traps (for the inverse polarity trap bits) and
> > the guest being allowed to access registers it shouldn't.
> > 
> > Fix it by copying the FGT traps for unprotected pKVM VCPUs when the
> > untrusted host loads the VCPU.
> > 
> > Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 ++++++++++--
> >  arch/arm64/kvm/hyp/nvhe/pkvm.c     |  1 -
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index 29430c031095..ee0f1343100c 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -167,6 +167,7 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
> >  	DECLARE_REG(unsigned int, vcpu_idx, host_ctxt, 2);
> >  	DECLARE_REG(u64, hcr_el2, host_ctxt, 3);
> >  	struct pkvm_hyp_vcpu *hyp_vcpu;
> > +	struct kvm_vcpu *hyp_kvm_vcpu;
> >  
> >  	if (!is_protected_kvm_enabled())
> >  		return;
> > @@ -175,10 +176,17 @@ static void handle___pkvm_vcpu_load(struct kvm_cpu_context *host_ctxt)
> >  	if (!hyp_vcpu)
> >  		return;
> >  
> > +	hyp_kvm_vcpu = &hyp_vcpu->vcpu;
> > +
> >  	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
> >  		/* Propagate WFx trapping flags */
> > -		hyp_vcpu->vcpu.arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> > -		hyp_vcpu->vcpu.arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> > +		hyp_kvm_vcpu->arch.hcr_el2 &= ~(HCR_TWE | HCR_TWI);
> > +		hyp_kvm_vcpu->arch.hcr_el2 |= hcr_el2 & (HCR_TWE | HCR_TWI);
> 
> I don't think the 'hyp_kvm_vcpu' really makes this much clearer and it's
> the line only ends up being a single character shorter. Just continue to
> use 'hyp_vcpu->vcpu.'?

That makes sense, I'll respin after rc1.

> 
> > +	} else {
> > +		struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> > +
> > +		memcpy(&hyp_kvm_vcpu->arch.fgt, host_vcpu->arch.fgt,
> > +		       sizeof(hyp_kvm_vcpu->arch.fgt));
> >  	}
> >  }
> >  
> > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > index 43bde061b65d..05774aed09cb 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> > @@ -172,7 +172,6 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu)
> >  
> >  		/* Trust the host for non-protected vcpu features. */
> >  		vcpu->arch.hcrx_el2 = host_vcpu->arch.hcrx_el2;
> > -		memcpy(vcpu->arch.fgt, host_vcpu->arch.fgt, sizeof(vcpu->arch.fgt));
> >  		return 0;
> >  	}
> 
> Otherwise, looks good to me:
> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks,
Alex