On Fri, May 05, 2017 at 04:20:42PM +0200, Auger Eric wrote:

Hi Christoffer, Marc,

On 05/05/2017 12:35, Marc Zyngier wrote:

quoted

On 05/05/17 11:10, Christoffer Dall wrote:

quoted

On Fri, May 05, 2017 at 10:59:09AM +0100, Marc Zyngier wrote:

quoted

On 05/05/17 10:45, Auger Eric wrote:

quoted

Hi,

On 05/05/2017 10:11, Christoffer Dall wrote:

quoted

On Thu, May 04, 2017 at 01:44:34PM +0200, Eric Auger wrote:

quoted

this new helper synchronizes the irq pending_latch
with the LPI pending bit status found in rdist pending table.
As the status is consumed, we reset the bit in pending table.

As we need the PENDBASER_ADDRESS() in vgic-v3, let's move its
definition in the irqchip header. We restore the full length
of the field, ie [51:16]. Same for PROPBASER_ADDRESS with full
field length of [51:12].

why into irqchip and not just the vgic header file?

Well most register field shift/masks are located there. This may be
useful as well for the ITS driver if power management gets implemented

Yeah, I'm fine with that. Having all of the HW description in one single
place makes sense.

quoted

quoted

quoted

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v6: new
---
 include/linux/irqchip/arm-gic-v3.h |  2 ++
 virt/kvm/arm/vgic/vgic-its.c       |  6 ++----
 virt/kvm/arm/vgic/vgic-v3.c        | 44 ++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h           |  1 +
 4 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 9519c7b..e09e5d7 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h

@@ -159,6 +159,8 @@
 #define GICR_PROPBASER_RaWaWb	GIC_BASER_CACHEABILITY(GICR_PROPBASER, INNER, RaWaWb)
 
 #define GICR_PROPBASER_IDBITS_MASK			(0x1f)
+#define GICR_PROPBASER_ADDRESS(x)	((x) & GENMASK_ULL(51, 12))
+#define GICR_PENDBASER_ADDRESS(x)	((x) & GENMASK_ULL(51, 16))
 
 #define GICR_PENDBASER_SHAREABILITY_SHIFT		(10)
 #define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT		(7)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e7bb86a..f43ea30c 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c

@@ -198,8 +198,6 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
  */
 #define BASER_ADDRESS(x)	((x) & GENMASK_ULL(47, 16))
 #define CBASER_ADDRESS(x)	((x) & GENMASK_ULL(47, 12))
-#define PENDBASER_ADDRESS(x)	((x) & GENMASK_ULL(47, 16))
-#define PROPBASER_ADDRESS(x)	((x) & GENMASK_ULL(47, 12))
 
 #define GIC_LPI_OFFSET 8192
 

@@ -234,7 +232,7 @@ static struct its_collection *find_collection(struct vgic_its *its, int coll_id)
 static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
 			     struct kvm_vcpu *filter_vcpu)
 {
-	u64 propbase = PROPBASER_ADDRESS(kvm->arch.vgic.propbaser);
+	u64 propbase = GICR_PROPBASER_ADDRESS(kvm->arch.vgic.propbaser);
 	u8 prop;
 	int ret;
 

@@ -346,7 +344,7 @@ static u32 max_lpis_propbaser(u64 propbaser)
  */
 static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
 {
-	gpa_t pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
+	gpa_t pendbase = GICR_PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
 	struct vgic_irq *irq;
 	int last_byte_offset = -1;
 	int ret = 0;

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index be0f4c3..0d753ae 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c

@@ -252,6 +252,50 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
 	vgic_v3->vgic_hcr = ICH_HCR_EN;
 }
 
+int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
+{
+	struct kvm_vcpu *vcpu;
+	int byte_offset, bit_nr;
+	gpa_t pendbase, ptr;
+	bool status;
+	u8 val;
+	int ret;
+
+retry:
+	vcpu = irq->target_vcpu;
+	if (!vcpu)
+		return 0;
+
+	pendbase = GICR_PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
+
+	byte_offset = irq->intid / BITS_PER_BYTE;
+	bit_nr = irq->intid % BITS_PER_BYTE;
+	ptr = pendbase + byte_offset;
+
+	ret = kvm_read_guest(kvm, ptr, &val, 1);
+	if (ret)
+		return ret;
+
+	status = val & (1 << bit_nr);
+
+	spin_lock(&irq->irq_lock);
+	if (irq->target_vcpu != vcpu) {
+		spin_unlock(&irq->irq_lock);
+		goto retry;

Can the guest be continuously changing the configuration of the LPI and
cause this function to be called, which will efficiently hog this CPU
from the system, or am I being overly cautious here?

Yes but on the other hand, there is a risk the target_vcpu has changed,
isn't it? So the alternative you be to return -EBUSY. and caller, ie.
its_add_lpi would return that error?

For the guest to be changing the LPI configuration, it would take a
command to be issued. If there is a concern that we're racing against a
command, can we take the command queue mutex?

So this is a redistributor thing, not an ITS thing, so I'd prefer not
solving it that way.

Indeed.

quoted

I was just thinking if we should do a limit of the number of times we'll
do this or check if we have a pending signal and just return, but
actually, I don't think this is a problem, because the path that keeps
changing the configuration will eventually run out of CPU resources and
this thread would have forward progress.

Yes, assuming we're not holding any other spinlock on the same path.
Looking at the code, we only seem to come here from handling MAPI/MAPTI,
so we should be good.

quoted

quoted

quoted

quoted

quoted

+	}
+	irq->pending_latch = status;
+	vgic_queue_irq_unlock(vcpu->kvm, irq);
+
+	if (status) {
+		/* clear consumed data */
+		val &= ~(1 << bit_nr);
+		ret = kvm_write_guest(kvm, ptr, &val, 1);
+		if (ret)
+			return ret;

Do we have a problem that if this is done twice within the same byte (on
different LPIs) then the data could be strangely out of sync?

Not sure I get what you mean here? I reset a single bit within the byte.
Do you mean there could be a concurrency issue?

In principle we are not obliged to reset the bit, right? Why do we care?
The table will be updated on next pending table save.

1) the guest should never write to the pending table.
2) if there is an interrupt being injected, it will be made pending in
the irq structure, and not in the PT.

If you have tGwo separate cores running this function at the same time,
trying to clear each a bit in the same word, wouldn't you loose one of
the cleared bits?

Ah, I see what you mean now (I was obviously looking at the wrong end of
the problem).

quoted

Maybe that can never happen because the commands would be serialized and
we should be restoring the system in serial as well?

I think that should be the case. What does it mean to do two restore in
parallel? We should probably enforce this. Eric?

We can't have 2 restores in parallel as we hold the kvm->lock all along.
MAPI commands are serialized by its_cmd_lock. MAPI and restore cannot
happen concurrently since vcpu is stopped and kvm_lock is held during
GITS_CWRITER setting too. So my understanding is it can't have this race.

Agreed.  Thanks for helping me figure this out.

-Christoffer