Re: [PATCH 06/12] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest

[Marc Zyngier <maz@kernel.org>]

On Wed, 21 Aug 2024 00:50:18 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Aug 20, 2024 at 11:03:43AM +0100, Marc Zyngier wrote:
> > In order to be consistent, we shouldn't advertise a GICv3 when none
> > is actually usable by the guest.
> > 
> > Wipe the feature when these conditions apply, and allow the field
> > to be written from userspace.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index bc2d54da3827..7d00d7e359e1 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2365,7 +2365,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  		   ID_AA64PFR0_EL1_MPAM |
> >  		   ID_AA64PFR0_EL1_SVE |
> >  		   ID_AA64PFR0_EL1_RAS |
> > -		   ID_AA64PFR0_EL1_GIC |
> >  		   ID_AA64PFR0_EL1_AdvSIMD |
> >  		   ID_AA64PFR0_EL1_FP), },
> >  	ID_SANITISED(ID_AA64PFR1_EL1),
> > @@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
> >  
> >  	guard(mutex)(&kvm->arch.config_lock);
> >  
> > +	if (!kvm_has_gicv3(kvm)) {
> > +		kvm->arch.id_regs[IDREG_IDX(SYS_ID_AA64PFR0_EL1)] &= ~ID_AA64PFR0_EL1_GIC_MASK;
> > +		kvm->arch.id_regs[IDREG_IDX(SYS_ID_PFR1_EL1)] &= ~ID_PFR1_EL1_GIC_MASK;
> > +	}
> > +
> 
> Hmm, should we use the ID register field as the source of truth for
> kvm_has_gicv3() at this point?
> 
> I think what you have in patch #1 makes good sense for a stable
> backport. Using the ID register from this point forward would make the
> behavior consistent for a stupid userspace instantiated GICv3 for the VM
> but clobbered it from the ID register.
> 
> AFAICT all other usage of kvm_has_gicv3() happens after
> kvm_finalize_sys_regs(), so it should take this last-minute fixup into
> account.

Right, so changing the definition of kvm_has_gicv3() in this patch,
and using the "old" definition here only. Yup, this seems like a
reasonable thing to do.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.