Re: [PATCH] KVM: arm64: VHE: Compute fgt traps before activating them

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

Hi Oliver,

On Wed, Nov 12, 2025 at 02:40:15AM -0800, Oliver Upton wrote:
> Hi Alex,
> 
> On Wed, Nov 12, 2025 at 10:28:53AM +0000, Alexandru Elisei wrote:
> > On VHE, the Fine Grain Traps registers are written to hardware in
> > kvm_arch_vcpu_load()->..->__activate_traps_hfgxtr(), but the fgt array is
> > computed later, in kvm_vcpu_load_fgt(). This can lead to zero being written
> > to the FGT registers the first time a VCPU is loaded.
> 
> Yikes! This is no good, thank you for spotting it.
> 
> > Also, any changes to
> > the fgt array will be visible only after the VCPU is scheduled out, and
> > then back in, which is not the intended behaviour.
> > 
> > Fix it by computing the fgt array just before the fgt traps are written
> > to hardware.
> > 
> > Fixes: fb10ddf35c1c ("KVM: arm64: Compute per-vCPU FGTs at vcpu_load()")
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> Reviewed-by: Oliver Upton <oupton@kernel.org>
> 
> > ---
> > 
> > Stumbled upon this when running a Linux guest on FVP with FEAT_S1PIE
> > enabled.  Linux touches PIRE0_EL1 very early during boot, in __cpu_setup().
> > HFGWTR_EL2 was 0 the first time the VCPU was run, KVM would then trap
> > the access to PIR0_EL1 (PIRE0_EL1 is an inverted trap) and trigger the
> > BUG_ON(!r->access) from perform_access().
> > 
> > I hacked __activate_traps_hfgxtr() to print the register value for
> > HFGWTR_EL2. Before this patch, during the first vcpu_load(),
> > HFGWTR_EL2 is 0, then it has the correct value. After this patch, it
> > always has the correct value.
> > 
> > If I were to venture a shot in the dark, it might be that the name is a bit
> > misleading - it's kvm_vpcu_load_fgt(), but it doesn't load anything onto
> > hardware, it just computes values. Might be worth renaming to avoid
> > similar ordering issues in the future.
> 
> Ack, naming isn't quite the best here. The idea was for the name to make
> it obvious that it is meant to be used at vcpu_load().

Actually, having a look at the load functions from kvm_arch_vcpu_load(), the
vast majority don't actually write anything to hardware, so the name is actually
in keeping with the convention, and it was just me being confused by the naming
:)

Thanks,
Alex

> 
> Thanks,
> Oliver