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

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

On Tue, Jun 03, 2025 at 11:33:08AM -0700, Raghavendra Rao Ananta wrote:
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > index b0baad68777c..c9d41ae9fe4f 100644
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > @@ -626,6 +626,26 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> >                 dev->kvm->arch.vgic.mi_intid = val;
> >                 return 0;
> >         }
> > +       case KVM_DEV_ARM_VGIC_GRP_FEATURES: {
> > +               u8 __user *uaddr = (u8 __user *)attr->addr;
> > +               u8 val;
> > +
> > +               if (attr->attr != KVM_DEV_ARM_VGIC_FEATURE_nASSGIcap)
> > +                       return -ENXIO;
> > +
> > +               if (get_user(val, uaddr))
> > +                       return -EFAULT;
> > +
> > +               guard(mutex)(&dev->kvm->arch.config_lock);
> > +               if (vgic_initialized(dev->kvm))
> > +                       return -EBUSY;
> > +
> Do we want to return success if userspace isn't trying to change the
> value (similar to sysreg/fw-reg writes)?

I'd rather not. Things that are accessible through KVM_*_ONE_REG get
special treatment since VMMs blindly save/restore the entire list of
system registers. The general expectation there is that the VMM and
always write back what it has read.

Since this is an entirely new UAPI I want to impose as much limitation
as possible on userspace :)

> >  bool vgic_supports_direct_msis(struct kvm *kvm)
> >  {
> > +       /*
> > +        * Deliberately conflate vLPI and vSGI support on GICv4.1 hardware,
> > +        * indirectly allowing userspace to control whether or not vPEs are
> > +        * allocated for the VM.
> > +        */
> > +       if (kvm_vgic_global_state.has_gicv4_1 && !vgic_supports_direct_sgis(kvm))
> > +               return false;
> > +
> Since vgic_supports_direct_sgis() is derived from
> 'kvm_vgic_global_state.has_gicv4_1', do we still need to depend on the
> latter?

Yes, this is a very subtle choice.

GICv4 does not support vSGIs, only vLPIs. If we omit "has_gicv4_1", we
break vLPIs on this hardware.

OTOH, GICv4.1 supports both vSGIs and vLPIs, and the dirty secret here
is we make vLPI support follow userspace's wishes for vSGIs. With that
being said, this should be predicated on both the ITS and the CPUIF
supporting vSGIs.

> Also just to double-check, there are other references that directly
> read 'kvm_vgic_global_state.has_gicv4_1' (vgic_v3_map_resources() for
> instance). Do we not want them to be replaced with
> vgic_supports_direct_sgis()?

There are a few screw ups on my part but most of the remaining checks
are for hardware support (i.e. before fiddling with the GIC), not VM
support.

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index b9ad7c42c5b0..13b561a2c90b 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -404,7 +404,7 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	 * The above vgic initialized check also ensures that the allocation
 	 * and enabling of the doorbells have already been done.
 	 */
-	if (kvm_vgic_global_state.has_gicv4_1) {
+	if (kvm_vgic_global_state.has_gicv4_1 && vgic_supports_direct_irqs(kvm)) {
 		unmap_all_vpes(kvm);
 		vlpi_avail = true;
 	}
@@ -581,7 +581,7 @@ int vgic_v3_map_resources(struct kvm *kvm)
 		return -EBUSY;
 	}
 
-	if (kvm_vgic_global_state.has_gicv4_1)
+	if (vgic_supports_direct_sgis(kvm))
 		vgic_v4_configure_vsgis(kvm);
 
 	return 0;
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index a9c772697a43..053d4fe7de8b 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -245,7 +245,7 @@ int vgic_v4_init(struct kvm *kvm)
 
 	lockdep_assert_held(&kvm->arch.config_lock);
 
-	if (!kvm_vgic_global_state.has_gicv4)
+	if (!vgic_supports_direct_irqs(kvm))
 		return 0; /* Nothing to see here... move along. */
 
 	if (dist->its_vm.vpes)