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

[Marc Zyngier <maz@kernel.org>]

On Mon, 23 Jun 2025 10:25:53 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> 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.

I'm not convinced we need to change much. For example, we already
allow userspace writes to GICD_IIDR to set the version of the
emulation prior to vgic_init(). It doesn't feel like allowing TYPER2
writes to occur in a similar spot require anything invasive.

Thanks,

	M.

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