Re: [PATCH v4 3/6] KVM: arm64: vgic-v3: Allow access to GICD_IIDR prior to initialization

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

On Mon, Jul 14, 2025 at 04:41:45PM +0200, Eric Auger wrote:
> Hi Oliver,
> 
> On 7/9/25 11:14 PM, Oliver Upton wrote:
> > KVM allows userspace to write GICD_IIDR for backwards-compatibility with
> > older kernels, where new implementation revisions have new features.
> > Unfortunately this is allowed to happen at runtime, and ripping features
> > out from underneath a running guest is a terrible idea.
> > 
> > While we can't do anything about the ABI, prepare for more ID-like
> > registers by allowing access to GICD_IIDR prior to VGIC initialization.
> > Subsequent changes will allow the VMM to further provision the GIC
> > feature set, e.g. the presence of nASSGIcap.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/vgic/vgic-init.c       |  9 +--------
> >  arch/arm64/kvm/vgic/vgic-kvm-device.c | 20 +++++++++++++++++++-
> >  2 files changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > index 5e0e4559004b..487e902b040c 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -157,6 +157,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
> >  
> >  	kvm->arch.vgic.in_kernel = true;
> >  	kvm->arch.vgic.vgic_model = type;
> > +	kvm->arch.vgic.implementation_rev = KVM_VGIC_IMP_REV_LATEST;
> >  
> >  	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
> >  
> > @@ -409,15 +410,7 @@ int vgic_init(struct kvm *kvm)
> >  		goto out;
> >  
> >  	vgic_debug_init(kvm);
> > -
> > -	/*
> > -	 * If userspace didn't set the GIC implementation revision,
> > -	 * default to the latest and greatest. You know want it.
> > -	 */
> > -	if (!dist->implementation_rev)
> > -		dist->implementation_rev = KVM_VGIC_IMP_REV_LATEST;
> The change related to KVM_VGIC_IMP_REV_LATEST looks unrelated to the
> commit title/msg, isn't it?

It is deliberate here to make IIDR have a similar discovery model to the
CPU feature ID registers where the value after creation represents the
maximum feature set.

> > @@ -552,7 +570,7 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> >  
> >  	mutex_lock(&dev->kvm->arch.config_lock);
> >  
> > -	if (!vgic_initialized(dev->kvm)) {
> > +	if (!(vgic_initialized(dev->kvm) || reg_allowed_pre_init(attr))) {
> nit: !vgic_initialized(dev->kvm) && !reg_allowed_pre_init() is easier to
> compute for me
> 
> 
> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Oliver