Re: [PATCH 09/10] KVM: arm64: Handle PIR{,E0}_EL2 traps

[Joey Gouly <joey.gouly@arm.com>]

On Tue, Aug 13, 2024 at 04:45:46PM +0100, Marc Zyngier wrote:
> On Tue, 13 Aug 2024 16:24:52 +0100,
> Joey Gouly <joey.gouly@arm.com> wrote:
> > 
> > On Tue, Aug 13, 2024 at 03:47:37PM +0100, Marc Zyngier wrote:
> > > Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
> > > they can be handled as a trap.
> > > 
> > > Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
> > > being advertised.
> > > 
> > > Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
> > > thanks to the D22677 update to the architecture..
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 52250db3c122..a5f604e24e05 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -346,6 +346,18 @@ static bool access_rw(struct kvm_vcpu *vcpu,
> > >  	return true;
> > >  }
> > >  
> > > +static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
> > > +				  struct sys_reg_params *p,
> > > +				  const struct sys_reg_desc *r)
> > > +{
> > > +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
> > > +		kvm_inject_undefined(vcpu);
> > > +		return false;
> > > +	}
> > > +
> > > +	return access_rw(vcpu, p, r);
> > > +}
> > > +
> > >  /*
> > >   * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
> > >   */
> > > @@ -2827,6 +2839,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >  	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
> > >  
> > >  	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
> > > +	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
> > > +	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
> > >  	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
> > >  
> > >  	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),
> > 
> > I think we should also use this for PIR_EL1 / PIRE0_EL1? We have NULL for their access field.
> > 
> > 	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
> 
> I don't think we need this. In general, the EL1 FEAT_S1PIE registers
> are directly accessed by the VM, and do not trap.
> 
> However, if the VM has been configured to not expose S1PIE, then we
> set the corresponding FGU bits in kvm_calculate_traps():
> 
> 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
> 		kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nPIRE0_EL1 |
> 						HFGxTR_EL2_nPIR_EL1);
> 
> The effect of this is that we don't even make to the sysreg array, and
> inject an UNDEF directly from the point of decoding the trap (see the
> beginning of triage_sysreg_trap()).
> 
> For EL2 registers, there is no concept of FGT since they always trap,
> so no architectural trick we can play to shortcut the handling.
> Therefore we make it to the handler and have to triage things there.
> 
> Does it make sense?

Ah yes, forgot how that worked, thanks for the reminder!

There's another 'conditionned' typo in the commit message, but otherwise:

Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks,
Joey