[PATCH 0/4] KVM: arm64: vgic-v3: MMIO-based LPI invalidation and co

[PATCH 0/4] KVM: arm64: vgic-v3: MMIO-based LPI invalidation and co

[Marc Zyngier]

Since revision IHI0069G of the GICv3 spec, an implementation is
allowed to implement MMIO-based LPI invalidation, without having to
support RVPEI (which is essentially a GICv4.1 feature).

This has the potential to make workloads using heavy LPI invalidation
fare a bit better, as they don't need to lock the access to the
command queue.

Similarly, an implementation can now expose that it allows LPIs to be
turned off, something that we always supported.

This series implements both these features, exposing the new
GICR_{INVLPIR,INVALLR,SYNCR} registers, transitions of the
GICR_CTLR.RWP bit on LPI disabling, and finally exposes these to
userspace and the guest with a new GICD_IIDR revision (and the ability
to save/restore it).

This series has been extremely useful to debug related GIC features,
and will be complemented by a few GIC patches.

Patches on top of 5.17-rc3.

	M.

Marc Zyngier (4):
  irqchip/gic-v3: Exposes bit values for GICR_CTLR.{IR,CES}
  KVM: arm64: vgic-v3: Implement MMIO-based LPI invalidation
  KVM: arm64: vgic-v3: Expose GICR_CTLR.RWP when disabling LPIs
  KVM: arm64: vgic-v3: Advertise GICR_CTLR.{IR,CES} as a new GICD_IIDR
    revision

 arch/arm64/kvm/vgic/vgic-init.c    |   7 +-
 arch/arm64/kvm/vgic/vgic-its.c     |  64 ++++++++++------
 arch/arm64/kvm/vgic/vgic-mmio-v2.c |  18 ++++-
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 119 ++++++++++++++++++++++++++---
 arch/arm64/kvm/vgic/vgic.h         |   5 ++
 include/kvm/arm_vgic.h             |   8 +-
 include/linux/irqchip/arm-gic-v3.h |   2 +
 7 files changed, 184 insertions(+), 39 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] irqchip/gic-v3: Exposes bit values for GICR_CTLR.{IR, CES}

[Marc Zyngier]

As we're about to expose GICR_CTLR.{IR,CES} to guests, populate
the include file with the architectural values.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/irqchip/arm-gic-v3.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 12d91f0dedf9..aeb8ced53880 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -127,6 +127,8 @@
 #define GICR_PIDR2			GICD_PIDR2
 
 #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
+#define GICR_CTLR_IR			(1UL << 1)
+#define GICR_CTLR_CES			(1UL << 2)
 #define GICR_CTLR_RWP			(1UL << 3)
 
 #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] irqchip/gic-v3: Exposes bit values for GICR_CTLR.{IR, CES}

[Oliver Upton]

Hi Marc,

On Mon, Mar 14, 2022 at 04:40:41PM +0000, Marc Zyngier wrote:
> As we're about to expose GICR_CTLR.{IR,CES} to guests, populate
> the include file with the architectural values.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  include/linux/irqchip/arm-gic-v3.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 12d91f0dedf9..aeb8ced53880 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -127,6 +127,8 @@
>  #define GICR_PIDR2			GICD_PIDR2
>  
>  #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
> +#define GICR_CTLR_IR			(1UL << 1)
> +#define GICR_CTLR_CES			(1UL << 2)

I think these are backwards (IR is bit 2)

https://developer.arm.com/documentation/ddi0595/2021-12/External-Registers/GICR-CTLR--Redistributor-Control-Register?lang=en

--
Thanks,
Oliver

>  #define GICR_CTLR_RWP			(1UL << 3)
>  
>  #define GICR_TYPER_CPU_NUMBER(r)	(((r) >> 8) & 0xffff)
> -- 
> 2.34.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] irqchip/gic-v3: Exposes bit values for GICR_CTLR.{IR, CES}

[Marc Zyngier]

On Tue, 15 Mar 2022 23:16:05 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, Mar 14, 2022 at 04:40:41PM +0000, Marc Zyngier wrote:
> > As we're about to expose GICR_CTLR.{IR,CES} to guests, populate
> > the include file with the architectural values.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  include/linux/irqchip/arm-gic-v3.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> > index 12d91f0dedf9..aeb8ced53880 100644
> > --- a/include/linux/irqchip/arm-gic-v3.h
> > +++ b/include/linux/irqchip/arm-gic-v3.h
> > @@ -127,6 +127,8 @@
> >  #define GICR_PIDR2			GICD_PIDR2
> >  
> >  #define GICR_CTLR_ENABLE_LPIS		(1UL << 0)
> > +#define GICR_CTLR_IR			(1UL << 1)
> > +#define GICR_CTLR_CES			(1UL << 2)
> 
> I think these are backwards (IR is bit 2)

How embarrassing... The whole thing only works because we always
advertise the two bits together, and that the GIC driver has the same
bug. Fortunately, I'm running low on paper bags... ;-)

I'll push a fix for that shortly.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] KVM: arm64: vgic-v3: Implement MMIO-based LPI invalidation

[Marc Zyngier]

Since GICv4.1, it has become legal for an implementation to advertise
GICR_{INVLPIR,INVALLR,SYNCR} while having an ITS, allowing for a more
efficient invalidation scheme (no guest command queue contention when
multiple CPUs are generating invalidations).

Provide the invalidation registers as a primitive to their ITS
counterpart. Note that we don't advertise them to the guest yet
(the architecture allows an implementation to do this).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-its.c     | 62 ++++++++++++++++++++----------
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 62 ++++++++++++++++++++++++++++++
 arch/arm64/kvm/vgic/vgic.h         |  4 ++
 include/kvm/arm_vgic.h             |  1 +
 4 files changed, 108 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 089fc2ffcb43..cc62d8a8180f 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1272,6 +1272,11 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
+int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq)
+{
+	return update_lpi_config(kvm, irq, NULL, true);
+}
+
 /*
  * The INV command syncs the configuration bits from the memory table.
  * Must be called with the its_lock mutex held.
@@ -1288,7 +1293,41 @@ static int vgic_its_cmd_handle_inv(struct kvm *kvm, struct vgic_its *its,
 	if (!ite)
 		return E_ITS_INV_UNMAPPED_INTERRUPT;
 
-	return update_lpi_config(kvm, ite->irq, NULL, true);
+	return vgic_its_inv_lpi(kvm, ite->irq);
+}
+
+/**
+ * vgic_its_invall - invalidate all LPIs targetting a given vcpu
+ * @vcpu: the vcpu for which the RD is targetted by an invalidation
+ *
+ * Contrary to the INVALL command, this targets a RD instead of a
+ * collection, and we don't need to hold the its_lock, since no ITS is
+ * involved here.
+ */
+int vgic_its_invall(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+	int irq_count, i = 0;
+	u32 *intids;
+
+	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
+	if (irq_count < 0)
+		return irq_count;
+
+	for (i = 0; i < irq_count; i++) {
+		struct vgic_irq *irq = vgic_get_irq(kvm, NULL, intids[i]);
+		if (!irq)
+			continue;
+		update_lpi_config(kvm, irq, vcpu, false);
+		vgic_put_irq(kvm, irq);
+	}
+
+	kfree(intids);
+
+	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
+		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
+
+	return 0;
 }
 
 /*
@@ -1305,32 +1344,13 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
 	u32 coll_id = its_cmd_get_collection(its_cmd);
 	struct its_collection *collection;
 	struct kvm_vcpu *vcpu;
-	struct vgic_irq *irq;
-	u32 *intids;
-	int irq_count, i;
 
 	collection = find_collection(its, coll_id);
 	if (!its_is_collection_mapped(collection))
 		return E_ITS_INVALL_UNMAPPED_COLLECTION;
 
 	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
-
-	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
-	if (irq_count < 0)
-		return irq_count;
-
-	for (i = 0; i < irq_count; i++) {
-		irq = vgic_get_irq(kvm, NULL, intids[i]);
-		if (!irq)
-			continue;
-		update_lpi_config(kvm, irq, vcpu, false);
-		vgic_put_irq(kvm, irq);
-	}
-
-	kfree(intids);
-
-	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
-		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
+	vgic_its_invall(vcpu);
 
 	return 0;
 }
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 58e40b4874f8..186bf35078bf 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -525,6 +525,59 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
 			   pendbaser) != old_pendbaser);
 }
 
+static unsigned long vgic_mmio_read_sync(struct kvm_vcpu *vcpu,
+					 gpa_t addr, unsigned int len)
+{
+	return !!atomic_read(&vcpu->arch.vgic_cpu.syncr_busy);
+}
+
+static void vgic_make_rdist_busy(struct kvm_vcpu *vcpu, bool busy)
+{
+	if (busy) {
+		atomic_inc(&vcpu->arch.vgic_cpu.syncr_busy);
+		smp_mb__after_atomic();
+	} else {
+		smp_mb__before_atomic();
+		atomic_dec(&vcpu->arch.vgic_cpu.syncr_busy);
+	}
+}
+
+static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
+				   gpa_t addr, unsigned int len,
+				   unsigned long val)
+{
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct vgic_irq *irq;
+
+	if (!vgic_cpu->lpis_enabled)
+		return;
+
+	vgic_make_rdist_busy(vcpu, true);
+
+	irq = vgic_get_irq(vcpu->kvm, NULL, val);
+	if (!irq)
+		return;
+
+	vgic_its_inv_lpi(vcpu->kvm, irq);
+	vgic_put_irq(vcpu->kvm, irq);
+
+	vgic_make_rdist_busy(vcpu, false);
+}
+
+static void vgic_mmio_write_invall(struct kvm_vcpu *vcpu,
+				   gpa_t addr, unsigned int len,
+				   unsigned long val)
+{
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+
+	if (!vgic_cpu->lpis_enabled)
+		return;
+
+	vgic_make_rdist_busy(vcpu, true);
+	vgic_its_invall(vcpu);
+	vgic_make_rdist_busy(vcpu, false);
+}
+
 /*
  * The GICv3 per-IRQ registers are split to control PPIs and SGIs in the
  * redistributors, while SPIs are covered by registers in the distributor
@@ -630,6 +683,15 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_PENDBASER,
 		vgic_mmio_read_pendbase, vgic_mmio_write_pendbase, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_INVLPIR,
+		vgic_mmio_read_raz, vgic_mmio_write_invlpi, 8,
+		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_INVALLR,
+		vgic_mmio_read_raz, vgic_mmio_write_invall, 8,
+		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_SYNCR,
+		vgic_mmio_read_sync, vgic_mmio_write_wi, 8,
+		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_IDREGS,
 		vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
 		VGIC_ACCESS_32bit),
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 3fd6c86a7ef3..53581e11f7c8 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -317,6 +317,10 @@ void vgic_lpi_translation_cache_init(struct kvm *kvm);
 void vgic_lpi_translation_cache_destroy(struct kvm *kvm);
 void vgic_its_invalidate_cache(struct kvm *kvm);
 
+/* GICv4.1 MMIO interface */
+int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq);
+int vgic_its_invall(struct kvm_vcpu *vcpu);
+
 bool vgic_supports_direct_msis(struct kvm *kvm);
 int vgic_v4_init(struct kvm *kvm);
 void vgic_v4_teardown(struct kvm *kvm);
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index bb30a6803d9f..d54bb44d6d98 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -344,6 +344,7 @@ struct vgic_cpu {
 	struct vgic_io_device	rd_iodev;
 	struct vgic_redist_region *rdreg;
 	u32 rdreg_index;
+	atomic_t syncr_busy;
 
 	/* Contains the attributes and gpa of the LPI pending tables. */
 	u64 pendbaser;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] KVM: arm64: vgic-v3: Implement MMIO-based LPI invalidation

[Oliver Upton]

Hi Marc,

On Mon, Mar 14, 2022 at 04:40:42PM +0000, Marc Zyngier wrote:
> Since GICv4.1, it has become legal for an implementation to advertise
> GICR_{INVLPIR,INVALLR,SYNCR} while having an ITS, allowing for a more
> efficient invalidation scheme (no guest command queue contention when
> multiple CPUs are generating invalidations).
> 
> Provide the invalidation registers as a primitive to their ITS
> counterpart. Note that we don't advertise them to the guest yet
> (the architecture allows an implementation to do this).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c     | 62 ++++++++++++++++++++----------
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 62 ++++++++++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic.h         |  4 ++
>  include/kvm/arm_vgic.h             |  1 +
>  4 files changed, 108 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 089fc2ffcb43..cc62d8a8180f 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1272,6 +1272,11 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
>  	return 0;
>  }
>  
> +int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq)
> +{
> +	return update_lpi_config(kvm, irq, NULL, true);
> +}
> +
>  /*
>   * The INV command syncs the configuration bits from the memory table.
>   * Must be called with the its_lock mutex held.
> @@ -1288,7 +1293,41 @@ static int vgic_its_cmd_handle_inv(struct kvm *kvm, struct vgic_its *its,
>  	if (!ite)
>  		return E_ITS_INV_UNMAPPED_INTERRUPT;
>  
> -	return update_lpi_config(kvm, ite->irq, NULL, true);
> +	return vgic_its_inv_lpi(kvm, ite->irq);
> +}
> +
> +/**
> + * vgic_its_invall - invalidate all LPIs targetting a given vcpu
> + * @vcpu: the vcpu for which the RD is targetted by an invalidation
> + *
> + * Contrary to the INVALL command, this targets a RD instead of a
> + * collection, and we don't need to hold the its_lock, since no ITS is
> + * involved here.
> + */
> +int vgic_its_invall(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	int irq_count, i = 0;
> +	u32 *intids;
> +
> +	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
> +	if (irq_count < 0)
> +		return irq_count;
> +
> +	for (i = 0; i < irq_count; i++) {
> +		struct vgic_irq *irq = vgic_get_irq(kvm, NULL, intids[i]);
> +		if (!irq)
> +			continue;
> +		update_lpi_config(kvm, irq, vcpu, false);
> +		vgic_put_irq(kvm, irq);
> +	}
> +
> +	kfree(intids);
> +
> +	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
> +		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
> +
> +	return 0;
>  }

nit: the refactoring happening at the same time as the functional change
is a bit distracting. Looks fine though.

>  /*
> @@ -1305,32 +1344,13 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
>  	u32 coll_id = its_cmd_get_collection(its_cmd);
>  	struct its_collection *collection;
>  	struct kvm_vcpu *vcpu;
> -	struct vgic_irq *irq;
> -	u32 *intids;
> -	int irq_count, i;
>  
>  	collection = find_collection(its, coll_id);
>  	if (!its_is_collection_mapped(collection))
>  		return E_ITS_INVALL_UNMAPPED_COLLECTION;
>  
>  	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
> -
> -	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
> -	if (irq_count < 0)
> -		return irq_count;
> -
> -	for (i = 0; i < irq_count; i++) {
> -		irq = vgic_get_irq(kvm, NULL, intids[i]);
> -		if (!irq)
> -			continue;
> -		update_lpi_config(kvm, irq, vcpu, false);
> -		vgic_put_irq(kvm, irq);
> -	}
> -
> -	kfree(intids);
> -
> -	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
> -		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
> +	vgic_its_invall(vcpu);
>  
>  	return 0;
>  }
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 58e40b4874f8..186bf35078bf 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -525,6 +525,59 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>  			   pendbaser) != old_pendbaser);
>  }
>  
> +static unsigned long vgic_mmio_read_sync(struct kvm_vcpu *vcpu,
> +					 gpa_t addr, unsigned int len)
> +{
> +	return !!atomic_read(&vcpu->arch.vgic_cpu.syncr_busy);
> +}
> +
> +static void vgic_make_rdist_busy(struct kvm_vcpu *vcpu, bool busy)

nit: s/make/set, since you use this helper to decrement the counter too.

> +{
> +	if (busy) {
> +		atomic_inc(&vcpu->arch.vgic_cpu.syncr_busy);
> +		smp_mb__after_atomic();
> +	} else {
> +		smp_mb__before_atomic();
> +		atomic_dec(&vcpu->arch.vgic_cpu.syncr_busy);
> +	}
> +}
> +
> +static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
> +				   gpa_t addr, unsigned int len,
> +				   unsigned long val)
> +{
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct vgic_irq *irq;
> +
> +	if (!vgic_cpu->lpis_enabled)
> +		return;
> +
> +	vgic_make_rdist_busy(vcpu, true);
> +
> +	irq = vgic_get_irq(vcpu->kvm, NULL, val);
> +	if (!irq)
> +		return;

Isn't the busy counter unbalanced if you return early?

--
Thanks,
Oliver

> +
> +	vgic_its_inv_lpi(vcpu->kvm, irq);
> +	vgic_put_irq(vcpu->kvm, irq);
> +
> +	vgic_make_rdist_busy(vcpu, false);
> +}
> +
> +static void vgic_mmio_write_invall(struct kvm_vcpu *vcpu,
> +				   gpa_t addr, unsigned int len,
> +				   unsigned long val)
> +{
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +
> +	if (!vgic_cpu->lpis_enabled)
> +		return;
> +
> +	vgic_make_rdist_busy(vcpu, true);
> +	vgic_its_invall(vcpu);
> +	vgic_make_rdist_busy(vcpu, false);
> +}
> +
>  /*
>   * The GICv3 per-IRQ registers are split to control PPIs and SGIs in the
>   * redistributors, while SPIs are covered by registers in the distributor
> @@ -630,6 +683,15 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_PENDBASER,
>  		vgic_mmio_read_pendbase, vgic_mmio_write_pendbase, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH(GICR_INVLPIR,
> +		vgic_mmio_read_raz, vgic_mmio_write_invlpi, 8,
> +		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH(GICR_INVALLR,
> +		vgic_mmio_read_raz, vgic_mmio_write_invall, 8,
> +		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH(GICR_SYNCR,
> +		vgic_mmio_read_sync, vgic_mmio_write_wi, 8,
> +		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_IDREGS,
>  		vgic_mmio_read_v3_idregs, vgic_mmio_write_wi, 48,
>  		VGIC_ACCESS_32bit),
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 3fd6c86a7ef3..53581e11f7c8 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -317,6 +317,10 @@ void vgic_lpi_translation_cache_init(struct kvm *kvm);
>  void vgic_lpi_translation_cache_destroy(struct kvm *kvm);
>  void vgic_its_invalidate_cache(struct kvm *kvm);
>  
> +/* GICv4.1 MMIO interface */
> +int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq);
> +int vgic_its_invall(struct kvm_vcpu *vcpu);
> +
>  bool vgic_supports_direct_msis(struct kvm *kvm);
>  int vgic_v4_init(struct kvm *kvm);
>  void vgic_v4_teardown(struct kvm *kvm);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index bb30a6803d9f..d54bb44d6d98 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -344,6 +344,7 @@ struct vgic_cpu {
>  	struct vgic_io_device	rd_iodev;
>  	struct vgic_redist_region *rdreg;
>  	u32 rdreg_index;
> +	atomic_t syncr_busy;
>  
>  	/* Contains the attributes and gpa of the LPI pending tables. */
>  	u64 pendbaser;
> -- 
> 2.34.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] KVM: arm64: vgic-v3: Implement MMIO-based LPI invalidation

[Marc Zyngier]

On Wed, 16 Mar 2022 05:26:06 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, Mar 14, 2022 at 04:40:42PM +0000, Marc Zyngier wrote:
> > Since GICv4.1, it has become legal for an implementation to advertise
> > GICR_{INVLPIR,INVALLR,SYNCR} while having an ITS, allowing for a more
> > efficient invalidation scheme (no guest command queue contention when
> > multiple CPUs are generating invalidations).
> > 
> > Provide the invalidation registers as a primitive to their ITS
> > counterpart. Note that we don't advertise them to the guest yet
> > (the architecture allows an implementation to do this).
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c     | 62 ++++++++++++++++++++----------
> >  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 62 ++++++++++++++++++++++++++++++
> >  arch/arm64/kvm/vgic/vgic.h         |  4 ++
> >  include/kvm/arm_vgic.h             |  1 +
> >  4 files changed, 108 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 089fc2ffcb43..cc62d8a8180f 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -1272,6 +1272,11 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
> >  	return 0;
> >  }
> >  
> > +int vgic_its_inv_lpi(struct kvm *kvm, struct vgic_irq *irq)
> > +{
> > +	return update_lpi_config(kvm, irq, NULL, true);
> > +}
> > +
> >  /*
> >   * The INV command syncs the configuration bits from the memory table.
> >   * Must be called with the its_lock mutex held.
> > @@ -1288,7 +1293,41 @@ static int vgic_its_cmd_handle_inv(struct kvm *kvm, struct vgic_its *its,
> >  	if (!ite)
> >  		return E_ITS_INV_UNMAPPED_INTERRUPT;
> >  
> > -	return update_lpi_config(kvm, ite->irq, NULL, true);
> > +	return vgic_its_inv_lpi(kvm, ite->irq);
> > +}
> > +
> > +/**
> > + * vgic_its_invall - invalidate all LPIs targetting a given vcpu
> > + * @vcpu: the vcpu for which the RD is targetted by an invalidation
> > + *
> > + * Contrary to the INVALL command, this targets a RD instead of a
> > + * collection, and we don't need to hold the its_lock, since no ITS is
> > + * involved here.
> > + */
> > +int vgic_its_invall(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm *kvm = vcpu->kvm;
> > +	int irq_count, i = 0;
> > +	u32 *intids;
> > +
> > +	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
> > +	if (irq_count < 0)
> > +		return irq_count;
> > +
> > +	for (i = 0; i < irq_count; i++) {
> > +		struct vgic_irq *irq = vgic_get_irq(kvm, NULL, intids[i]);
> > +		if (!irq)
> > +			continue;
> > +		update_lpi_config(kvm, irq, vcpu, false);
> > +		vgic_put_irq(kvm, irq);
> > +	}
> > +
> > +	kfree(intids);
> > +
> > +	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
> > +		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
> > +
> > +	return 0;
> >  }
> 
> nit: the refactoring happening at the same time as the functional change
> is a bit distracting. Looks fine though.

Yeah, it didn't seem to warrant an extra patch, given that we're
really only moving things about.

> 
> >  /*
> > @@ -1305,32 +1344,13 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
> >  	u32 coll_id = its_cmd_get_collection(its_cmd);
> >  	struct its_collection *collection;
> >  	struct kvm_vcpu *vcpu;
> > -	struct vgic_irq *irq;
> > -	u32 *intids;
> > -	int irq_count, i;
> >  
> >  	collection = find_collection(its, coll_id);
> >  	if (!its_is_collection_mapped(collection))
> >  		return E_ITS_INVALL_UNMAPPED_COLLECTION;
> >  
> >  	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
> > -
> > -	irq_count = vgic_copy_lpi_list(kvm, vcpu, &intids);
> > -	if (irq_count < 0)
> > -		return irq_count;
> > -
> > -	for (i = 0; i < irq_count; i++) {
> > -		irq = vgic_get_irq(kvm, NULL, intids[i]);
> > -		if (!irq)
> > -			continue;
> > -		update_lpi_config(kvm, irq, vcpu, false);
> > -		vgic_put_irq(kvm, irq);
> > -	}
> > -
> > -	kfree(intids);
> > -
> > -	if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.its_vm)
> > -		its_invall_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe);
> > +	vgic_its_invall(vcpu);
> >  
> >  	return 0;
> >  }
> > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > index 58e40b4874f8..186bf35078bf 100644
> > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> > @@ -525,6 +525,59 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> >  			   pendbaser) != old_pendbaser);
> >  }
> >  
> > +static unsigned long vgic_mmio_read_sync(struct kvm_vcpu *vcpu,
> > +					 gpa_t addr, unsigned int len)
> > +{
> > +	return !!atomic_read(&vcpu->arch.vgic_cpu.syncr_busy);
> > +}
> > +
> > +static void vgic_make_rdist_busy(struct kvm_vcpu *vcpu, bool busy)
> 
> nit: s/make/set, since you use this helper to decrement the counter too.

Sure, works for me.

> > +{
> > +	if (busy) {
> > +		atomic_inc(&vcpu->arch.vgic_cpu.syncr_busy);
> > +		smp_mb__after_atomic();
> > +	} else {
> > +		smp_mb__before_atomic();
> > +		atomic_dec(&vcpu->arch.vgic_cpu.syncr_busy);
> > +	}
> > +}
> > +
> > +static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
> > +				   gpa_t addr, unsigned int len,
> > +				   unsigned long val)
> > +{
> > +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> > +	struct vgic_irq *irq;
> > +
> > +	if (!vgic_cpu->lpis_enabled)
> > +		return;
> > +
> > +	vgic_make_rdist_busy(vcpu, true);
> > +
> > +	irq = vgic_get_irq(vcpu->kvm, NULL, val);
> > +	if (!irq)
> > +		return;
> 
> Isn't the busy counter unbalanced if you return early?

Huh, well caught. Fix incoming.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] KVM: arm64: vgic-v3: Expose GICR_CTLR.RWP when disabling LPIs

[Marc Zyngier]

When disabling LPIs, a guest needs to poll GICR_CTLR.RWP in order
to be sure that the write has taken effect. We so far reported it
as 0, as we didn't advertise that LPIs could be turned off the
first place.

Start tracking this state during which LPIs are being disabled,
and expose the 'in progress' state via the RWP bit.

We also take this opportunity to disallow enabling LPIs and programming
GICR_{PEND,PROP}BASER while LPI disabling is in progress, as allowed by
the architecture (UNPRED behaviour).

We don't advertise the feature to the guest yet (which is allowed by
the architecture).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-its.c     |  2 +-
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 44 ++++++++++++++++++++----------
 arch/arm64/kvm/vgic/vgic.h         |  1 +
 include/kvm/arm_vgic.h             |  4 +--
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index cc62d8a8180f..9f51d624730f 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -683,7 +683,7 @@ int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
 	if (!vcpu)
 		return E_ITS_INT_UNMAPPED_INTERRUPT;
 
-	if (!vcpu->arch.vgic_cpu.lpis_enabled)
+	if (!vgic_lpis_enabled(vcpu))
 		return -EBUSY;
 
 	vgic_its_cache_translation(kvm, its, devid, eventid, ite->irq);
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 186bf35078bf..a6be403996c6 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -221,6 +221,13 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
 	vgic_put_irq(vcpu->kvm, irq);
 }
 
+bool vgic_lpis_enabled(struct kvm_vcpu *vcpu)
+{
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+
+	return atomic_read(&vgic_cpu->ctlr) == GICR_CTLR_ENABLE_LPIS;
+}
+
 static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
 					     gpa_t addr, unsigned int len)
 {
@@ -229,26 +236,39 @@ static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
 	return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
 }
 
-
 static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
 				     gpa_t addr, unsigned int len,
 				     unsigned long val)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-	bool was_enabled = vgic_cpu->lpis_enabled;
+	u32 ctlr;
 
 	if (!vgic_has_its(vcpu->kvm))
 		return;
 
-	vgic_cpu->lpis_enabled = val & GICR_CTLR_ENABLE_LPIS;
+	if (!(val & GICR_CTLR_ENABLE_LPIS)) {
+		/*
+		 * Don't disable if RWP is set, as there already an
+		 * ongoing disable. Funky guest...
+		 */
+		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
+					      GICR_CTLR_ENABLE_LPIS,
+					      GICR_CTLR_RWP);
+		if (ctlr != GICR_CTLR_ENABLE_LPIS)
+			return;
 
-	if (was_enabled && !vgic_cpu->lpis_enabled) {
 		vgic_flush_pending_lpis(vcpu);
 		vgic_its_invalidate_cache(vcpu->kvm);
-	}
+		smp_mb__before_atomic();
+		atomic_set(&vgic_cpu->ctlr, 0);
+	} else {
+		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
+					      GICR_CTLR_ENABLE_LPIS);
+		if (ctlr != 0)
+			return;
 
-	if (!was_enabled && vgic_cpu->lpis_enabled)
 		vgic_enable_lpis(vcpu);
+	}
 }
 
 static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu)
@@ -478,11 +498,10 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
 				     unsigned long val)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	u64 old_propbaser, propbaser;
 
 	/* Storing a value with LPIs already enabled is undefined */
-	if (vgic_cpu->lpis_enabled)
+	if (vgic_lpis_enabled(vcpu))
 		return;
 
 	do {
@@ -513,7 +532,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
 	u64 old_pendbaser, pendbaser;
 
 	/* Storing a value with LPIs already enabled is undefined */
-	if (vgic_cpu->lpis_enabled)
+	if (vgic_lpis_enabled(vcpu))
 		return;
 
 	do {
@@ -546,10 +565,9 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
 				   gpa_t addr, unsigned int len,
 				   unsigned long val)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_irq *irq;
 
-	if (!vgic_cpu->lpis_enabled)
+	if (!vgic_lpis_enabled(vcpu))
 		return;
 
 	vgic_make_rdist_busy(vcpu, true);
@@ -568,9 +586,7 @@ static void vgic_mmio_write_invall(struct kvm_vcpu *vcpu,
 				   gpa_t addr, unsigned int len,
 				   unsigned long val)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-
-	if (!vgic_cpu->lpis_enabled)
+	if (!vgic_lpis_enabled(vcpu))
 		return;
 
 	vgic_make_rdist_busy(vcpu, true);
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index 53581e11f7c8..1d04a900f3e3 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -308,6 +308,7 @@ static inline bool vgic_dist_overlap(struct kvm *kvm, gpa_t base, size_t size)
 		(base < d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE);
 }
 
+bool vgic_lpis_enabled(struct kvm_vcpu *vcpu);
 int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr);
 int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
 			 u32 devid, u32 eventid, struct vgic_irq **irq);
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index d54bb44d6d98..401236f97cf2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -348,8 +348,8 @@ struct vgic_cpu {
 
 	/* Contains the attributes and gpa of the LPI pending tables. */
 	u64 pendbaser;
-
-	bool lpis_enabled;
+	/* GICR_CTLR.{ENABLE_LPIS,RWP} */
+	atomic_t ctlr;
 
 	/* Cache guest priority bits */
 	u32 num_pri_bits;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] KVM: arm64: vgic-v3: Expose GICR_CTLR.RWP when disabling LPIs

[Oliver Upton]

On Mon, Mar 14, 2022 at 04:40:43PM +0000, Marc Zyngier wrote:
> When disabling LPIs, a guest needs to poll GICR_CTLR.RWP in order
> to be sure that the write has taken effect. We so far reported it
> as 0, as we didn't advertise that LPIs could be turned off the
> first place.
> 
> Start tracking this state during which LPIs are being disabled,
> and expose the 'in progress' state via the RWP bit.
> 
> We also take this opportunity to disallow enabling LPIs and programming
> GICR_{PEND,PROP}BASER while LPI disabling is in progress, as allowed by
> the architecture (UNPRED behaviour).
> 
> We don't advertise the feature to the guest yet (which is allowed by
> the architecture).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c     |  2 +-
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 44 ++++++++++++++++++++----------
>  arch/arm64/kvm/vgic/vgic.h         |  1 +
>  include/kvm/arm_vgic.h             |  4 +--
>  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index cc62d8a8180f..9f51d624730f 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -683,7 +683,7 @@ int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
>  	if (!vcpu)
>  		return E_ITS_INT_UNMAPPED_INTERRUPT;
>  
> -	if (!vcpu->arch.vgic_cpu.lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return -EBUSY;
>  
>  	vgic_its_cache_translation(kvm, its, devid, eventid, ite->irq);
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 186bf35078bf..a6be403996c6 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -221,6 +221,13 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
>  	vgic_put_irq(vcpu->kvm, irq);
>  }
>  
> +bool vgic_lpis_enabled(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +
> +	return atomic_read(&vgic_cpu->ctlr) == GICR_CTLR_ENABLE_LPIS;
> +}
> +
>  static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>  					     gpa_t addr, unsigned int len)
>  {
> @@ -229,26 +236,39 @@ static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>  	return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
>  }
>  
> -
>  static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
>  				     gpa_t addr, unsigned int len,
>  				     unsigned long val)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	bool was_enabled = vgic_cpu->lpis_enabled;
> +	u32 ctlr;
>  
>  	if (!vgic_has_its(vcpu->kvm))
>  		return;
>  
> -	vgic_cpu->lpis_enabled = val & GICR_CTLR_ENABLE_LPIS;
> +	if (!(val & GICR_CTLR_ENABLE_LPIS)) {
> +		/*
> +		 * Don't disable if RWP is set, as there already an
> +		 * ongoing disable. Funky guest...
> +		 */
> +		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
> +					      GICR_CTLR_ENABLE_LPIS,
> +					      GICR_CTLR_RWP);
> +		if (ctlr != GICR_CTLR_ENABLE_LPIS)
> +			return;
>  
> -	if (was_enabled && !vgic_cpu->lpis_enabled) {
>  		vgic_flush_pending_lpis(vcpu);
>  		vgic_its_invalidate_cache(vcpu->kvm);
> -	}
> +		smp_mb__before_atomic();
> +		atomic_set(&vgic_cpu->ctlr, 0);
> +	} else {
> +		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
> +					      GICR_CTLR_ENABLE_LPIS);
> +		if (ctlr != 0)
> +			return;
>  
> -	if (!was_enabled && vgic_cpu->lpis_enabled)
>  		vgic_enable_lpis(vcpu);
> +	}
>  }
>  
>  static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu)
> @@ -478,11 +498,10 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
>  				     unsigned long val)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	u64 old_propbaser, propbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
> -	if (vgic_cpu->lpis_enabled)
> +	if (vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	do {
> @@ -513,7 +532,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>  	u64 old_pendbaser, pendbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
> -	if (vgic_cpu->lpis_enabled)
> +	if (vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	do {
> @@ -546,10 +565,9 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
>  				   gpa_t addr, unsigned int len,
>  				   unsigned long val)
>  {
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_irq *irq;
>  
> -	if (!vgic_cpu->lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	vgic_make_rdist_busy(vcpu, true);
> @@ -568,9 +586,7 @@ static void vgic_mmio_write_invall(struct kvm_vcpu *vcpu,
>  				   gpa_t addr, unsigned int len,
>  				   unsigned long val)
>  {
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -
> -	if (!vgic_cpu->lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return;
>  

nit: could you reorder the series to avoid rewriting parts of patch 2
again?


Otherwise:

Reviewed-by: Oliver Upton <oupton@google.com>

>  	vgic_make_rdist_busy(vcpu, true);
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 53581e11f7c8..1d04a900f3e3 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -308,6 +308,7 @@ static inline bool vgic_dist_overlap(struct kvm *kvm, gpa_t base, size_t size)
>  		(base < d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE);
>  }
>  
> +bool vgic_lpis_enabled(struct kvm_vcpu *vcpu);
>  int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr);
>  int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
>  			 u32 devid, u32 eventid, struct vgic_irq **irq);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index d54bb44d6d98..401236f97cf2 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -348,8 +348,8 @@ struct vgic_cpu {
>  
>  	/* Contains the attributes and gpa of the LPI pending tables. */
>  	u64 pendbaser;
> -
> -	bool lpis_enabled;
> +	/* GICR_CTLR.{ENABLE_LPIS,RWP} */
> +	atomic_t ctlr;
>  
>  	/* Cache guest priority bits */
>  	u32 num_pri_bits;
> -- 
> 2.34.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

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;
+			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)
+		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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Oliver Upton]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On Tue, 15 Mar 2022 23:13:09 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> 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?

Good point. Having a non-zero implementation will serve the same
purpose. The drawback is that we lose the documentation aspect of the
field, but we can probably work around that.

> 
> > +			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?

Sure, as there will be two users now.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

Hi Oliver,

On Tue, 15 Mar 2022 23:13:09 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, Mar 14, 2022 at 04:40:44PM +0000, Marc Zyngier wrote:
> > @@ -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?

[coming back to this]

Now I remember why this doesn't work. The established behaviour is
that it takes a write to IIDR to switch to the 'writable groups'
mode. If we base the switch on the implementation version, we don't
need a write anymore (we always allow groups to be writable), and old
guests cannot be reliably restored.

32f8777ed92d has the gory details, and that's really not old enough
that we can turn a blind eye to it, unfortunately.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel