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

[Eric Auger <eauger@redhat.com>]

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?
>  	dist->initialized = true;
> -
>  out:
>  	return ret;
>  }
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index e28cf68a49c3..15d9772a53c8 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2015 ARM Ltd.
>   * Author: Marc Zyngier <marc.zyngier@arm.com>
>   */
> +#include <linux/irqchip/arm-gic-v3.h>
>  #include <linux/kvm_host.h>
>  #include <kvm/arm_vgic.h>
>  #include <linux/uaccess.h>
> @@ -503,6 +504,23 @@ int vgic_v3_parse_attr(struct kvm_device *dev, struct kvm_device_attr *attr,
>  	return 0;
>  }
>  
> +/*
> + * Allow access to certain ID-like registers prior to VGIC initialization,
> + * thereby allowing the VMM to provision the features / sizing of the VGIC.
> + */
> +static bool reg_allowed_pre_init(struct kvm_device_attr *attr)
> +{
> +	if (attr->group != KVM_DEV_ARM_VGIC_GRP_DIST_REGS)
> +		return false;
> +
> +	switch (attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK) {
> +	case GICD_IIDR:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /*
>   * vgic_v3_attr_regs_access - allows user space to access VGIC v3 state
>   *
> @@ -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>

Eric

>  		ret = -EBUSY;
>  		goto out;
>  	}