Re: [PATCH kvm-unit-tests v2] arm64: timer: Speed up gic-timer-state check

[Andrew Jones <drjones@redhat.com>]

On Tue, Feb 11, 2020 at 10:50:58PM +0800, Zenghui Yu wrote:
> Hi Drew,
> 
> On 2020/2/11 21:37, Andrew Jones wrote:
> > Let's bail out of the wait loop if we see the expected state
> > to save over six seconds of run time. Make sure we wait a bit
> > before reading the registers and double check again after,
> > though, to somewhat mitigate the chance of seeing the expected
> > state by accident.
> > 
> > We also take this opportunity to push more IRQ state code to
> > the library.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> [...]
> 
> > +
> > +enum gic_irq_state gic_irq_state(int irq)
> 
> This is a *generic* name while this function only deals with PPI.
> Maybe we can use something like gic_ppi_state() instead?  Or you
> will have to take all interrupt types into account in a single
> function, which is not a easy job I think.

Very good point.

> 
> > +{
> > +	enum gic_irq_state state;
> > +	bool pending = false, active = false;
> > +	void *base;
> > +
> > +	assert(gic_common_ops);
> > +
> > +	switch (gic_version()) {
> > +	case 2:
> > +		base = gicv2_dist_base();
> > +		pending = readl(base + GICD_ISPENDR) & (1 << PPI(irq));
> > +		active = readl(base + GICD_ISACTIVER) & (1 << PPI(irq));
> > +		break;
> > +	case 3:
> > +		base = gicv3_sgi_base();
> > +		pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq));
> > +		active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq));
> 
> And you may also want to ensure that the 'irq' is valid for PPI().
> Or personally, I'd just use a real PPI number (PPI(info->irq)) as
> the input parameter of this function.

Indeed, if we want to make this a general function we should require
the caller to pass PPI(irq).

> 
> > +		break;
> > +	}
> > +
> > +	if (!active && !pending)
> > +		state = GIC_IRQ_STATE_INACTIVE;
> > +	if (pending)
> > +		state = GIC_IRQ_STATE_PENDING;
> > +	if (active)
> > +		state = GIC_IRQ_STATE_ACTIVE;
> > +	if (active && pending)
> > +		state = GIC_IRQ_STATE_ACTIVE_PENDING;
> > +
> > +	return state;
> > +}
> > 
> 
> Otherwise,
> 
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
> Tested-by: Zenghui Yu <yuzenghui@huawei.com>

Thanks, but I guess I should squash in changes to make this function more
general. My GIC/SPI skills are weak, so I'm not sure this is right,
especially because the SPI stuff doesn't yet have a user to validate it.
However, if all reviewers think it's correct, then I'll squash it into
the arm/queue branch. I've added Andre and Eric to help review too.

Thanks,
drew


diff --git a/arm/timer.c b/arm/timer.c
index ae5fdbf54b35..44621b4f2967 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -189,9 +189,9 @@ static bool gic_timer_check_state(struct timer_info *info,
 	/* Wait for up to 1s for the GIC to sample the interrupt. */
 	for (i = 0; i < 10; i++) {
 		mdelay(100);
-		if (gic_irq_state(info->irq) == expected_state) {
+		if (gic_irq_state(PPI(info->irq)) == expected_state) {
 			mdelay(100);
-			if (gic_irq_state(info->irq) == expected_state)
+			if (gic_irq_state(PPI(info->irq)) == expected_state)
 				return true;
 		}
 	}
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 0563b31132c8..3924dd1d1ee6 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -150,22 +150,37 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
 enum gic_irq_state gic_irq_state(int irq)
 {
 	enum gic_irq_state state;
-	bool pending = false, active = false;
-	void *base;
+	void *ispendr, *isactiver;
+	bool pending, active;
 
 	assert(gic_common_ops);
 
 	switch (gic_version()) {
 	case 2:
-		base = gicv2_dist_base();
-		pending = readl(base + GICD_ISPENDR) & (1 << PPI(irq));
-		active = readl(base + GICD_ISACTIVER) & (1 << PPI(irq));
+		ispendr = gicv2_dist_base() + GICD_ISPENDR;
+		isactiver = gicv2_dist_base() + GICD_ISACTIVER;
 		break;
 	case 3:
-		base = gicv3_sgi_base();
-		pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq));
-		active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq));
+		if (irq < GIC_NR_PRIVATE_IRQS) {
+			ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
+			isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
+		} else {
+			ispendr = gicv3_dist_base() + GICD_ISPENDR;
+			isactiver = gicv3_dist_base() + GICD_ISACTIVER;
+		}
 		break;
+	default:
+		assert(0);
+	}
+
+	if (irq < GIC_NR_PRIVATE_IRQS) {
+		pending = readl(ispendr) & (1 << irq);
+		active = readl(isactiver) & (1 << irq);
+	} else {
+		int offset = (irq - GIC_FIRST_SPI) / 32;
+		int mask = 1 << ((irq - GIC_FIRST_SPI) % 32);
+		pending = readl(ispendr + offset) & mask;
+		active = readl(isactiver + offset) & mask;
 	}
 
 	if (!active && !pending)

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm