Re: [PATCH v3 3/4] KVM: arm64: Introduce attribute to control GICD_TYPER2.nASSGIcap

[Oliver Upton <oliver.upton@linux.dev>]

On Mon, Jun 23, 2025 at 10:05:42AM +0100, Marc Zyngier wrote:
> On Mon, 23 Jun 2025 09:40:46 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Sat, Jun 21, 2025 at 09:50:48AM +0100, Marc Zyngier wrote:
> > > On Fri, 13 Jun 2025 16:52:37 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote:
> > > > @@ -683,8 +714,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> > > >  			return 0;
> > > >  		case KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES:
> > > >  			return 0;
> > > > +		default:
> > > > +			return -ENXIO;
> > > >  		}
> > > > +	case KVM_DEV_ARM_VGIC_GRP_FEATURES:
> > > > +		return attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap ?
> > > > +		       -ENXIO : 0;
> > > 
> > > Do we really want to advertise KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap even
> > > when we don't have GICv4.1? This seems rather odd. My take on this API
> > > is that this should report whether the feature is configurable, making
> > > it backward compatible with older versions of KVM.
> > 
> > So this was because of me, as I wanted nASSGIcap to behave exactly like
> > the ID registers. I do think exposing the capability unconditionally is
> > useful, as otherwise there's no way to definitively say whether or not
> > the underlying platform supports GICv4.1.
> > 
> > KVM_HAS_DEVICE_ATTR can't be used alone for probing since old kernels
> > use GICv4.1 but don't expose the attribute.
> > 
> > Does that make sense?
> 
> My own reasoning is that if we expose the capability, userspace is
> able to use it and rely on it to take effect (VPE allocation error
> notwithstanding). This is not the case with this approach, and that's
> at odds with the other attributes.
> 
> But taking a step back: if we want to control the nASSGIcap bit, why
> don't we allow writing to GICD_TYPER2 from userspace? This does
> matches your view that we treat it as an ID register (GICD_TYPER2
> matches this definition if you squint hard enough). It also avoids
> adding new UAPI with unusual semantics.

This approach would bring its own set of complications. At least right
now we allocate vPEs at vgic_init() but prevent register accesses prior
to initialization. If we want to bake this thing into GICD_TYPER2
directly we either need to relax this register to be accessed before
init or defer the vPE allocation later on.

I'm worried that the latter approach is gonna be a mess, and the
attribute was done to avoid a one-off accessor in the VGIC state. But if
you'd like to see it done that way then that's OK with me.

Thanks,
Oliver