Re: [PATCH 4/4] KVM: arm64: vgic-v3: Advertise GICR_CTLR.{IR, CES} as a new GICD_IIDR revision

From: Oliver Upton <oupton@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: Andre Przywara <andre.przywara@arm.com>,
	kernel-team@android.com, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 4/4] KVM: arm64: vgic-v3: Advertise GICR_CTLR.{IR, CES} as a new GICD_IIDR revision
Date: Tue, 15 Mar 2022 23:13:09 +0000	[thread overview]
Message-ID: <YjEdhVFKTkS4GiIS@google.com> (raw)
In-Reply-To: <20220314164044.772709-5-maz@kernel.org>

Hi Marc,

On Mon, Mar 14, 2022 at 04:40:44PM +0000, Marc Zyngier wrote:
> Since adversising GICR_CTLR.{IC,CES} is directly observable from
> a guest, we need to make it selectable from userspace.
> 
> For that, bump the default GICD_IIDR revision and let userspace
> downgrade it to the previous default. For GICv2, the two distributor
> revisions are strictly equivalent.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c    |  7 ++++++-
>  arch/arm64/kvm/vgic/vgic-mmio-v2.c | 18 +++++++++++++++---
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 23 +++++++++++++++++++++--
>  include/kvm/arm_vgic.h             |  3 +++
>  4 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index fc00304fe7d8..f84e04f334c6 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -319,7 +319,12 @@ int vgic_init(struct kvm *kvm)
>  
>  	vgic_debug_init(kvm);
>  
> -	dist->implementation_rev = 2;
> +	/*
> +	 * 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;
>  	dist->initialized = true;
>  
>  out:
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> index 12e4c223e6b8..f2246c4ca812 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> @@ -73,9 +73,13 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
>  					   gpa_t addr, unsigned int len,
>  					   unsigned long val)
>  {
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +	u32 reg;
> +
>  	switch (addr & 0x0c) {
>  	case GIC_DIST_IIDR:
> -		if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
> +		reg = vgic_mmio_read_v2_misc(vcpu, addr, len);
> +		if ((reg ^ val) & ~GICD_IIDR_REVISION_MASK)
>  			return -EINVAL;
>  
>  		/*
> @@ -87,8 +91,16 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
>  		 * migration from old kernels to new kernels with legacy
>  		 * userspace.
>  		 */
> -		vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
> -		return 0;
> +		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
> +		switch (reg) {
> +		case KVM_VGIC_IMP_REV_2:
> +		case KVM_VGIC_IMP_REV_3:
> +			dist->v2_groups_user_writable = true;

Could you eliminate this bool and just pivot off of the implementation
version?

> +			dist->implementation_rev = reg;
> +			return 0;
> +		default:
> +			return -EINVAL;
> +		}
>  	}
>  
>  	vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index a6be403996c6..4c8e4f83e3d1 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -155,13 +155,27 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
>  					   unsigned long val)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +	u32 reg;
>  
>  	switch (addr & 0x0c) {
>  	case GICD_TYPER2:
> -	case GICD_IIDR:
>  		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
>  			return -EINVAL;
>  		return 0;
> +	case GICD_IIDR:
> +		reg = vgic_mmio_read_v3_misc(vcpu, addr, len);
> +		if ((reg ^ val) & ~GICD_IIDR_REVISION_MASK)
> +			return -EINVAL;
> +
> +		reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
> +		switch (reg) {
> +		case KVM_VGIC_IMP_REV_2:
> +		case KVM_VGIC_IMP_REV_3:
> +			dist->implementation_rev = reg;
> +			return 0;
> +		default:
> +			return -EINVAL;
> +		}
>  	case GICD_CTLR:
>  		/* Not a GICv4.1? No HW SGIs */
>  		if (!kvm_vgic_global_state.has_gicv4_1)
> @@ -232,8 +246,13 @@ static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>  					     gpa_t addr, unsigned int len)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	unsigned long val;
> +
> +	val = atomic_read(&vgic_cpu->ctlr);
> +	if (vcpu->kvm->arch.vgic.implementation_rev >= KVM_VGIC_IMP_REV_3)

That's a lot of indirection :) Could you make a helper for getting at
the implementation revision from a vCPU pointer?

> +		val |= GICR_CTLR_IR | GICR_CTLR_CES;
>  
> -	return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
> +	return val;
>  }
>  
>  static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 401236f97cf2..2d8f2e90edc2 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -231,6 +231,9 @@ struct vgic_dist {
>  
>  	/* Implementation revision as reported in the GICD_IIDR */
>  	u32			implementation_rev;
> +#define KVM_VGIC_IMP_REV_2	2 /* GICv2 restorable groups */
> +#define KVM_VGIC_IMP_REV_3	3 /* GICv3 GICR_CTLR.{IW,CES,RWP} */
> +#define KVM_VGIC_IMP_REV_LATEST	KVM_VGIC_IMP_REV_3
>  
>  	/* Userspace can write to GICv2 IGROUPR */
>  	bool			v2_groups_user_writable;
> -- 
> 2.34.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm