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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 12, 2020 at 10:57:09AM +0800, Zenghui Yu wrote:
> Hi Drew,
> 
> On 2020/2/11 23:41, Andrew Jones wrote:
> > 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@xxxxxxxxxx>
> > > 
> > > [...]
> > > 
> > > > +
> > > > +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@xxxxxxxxxx>
> > > Tested-by: Zenghui Yu <yuzenghui@xxxxxxxxxx>
> > 
> > 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.
> 
> (I guess the PL031 can be another new user.)
> 
> > 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);
> 
> We can also assert that only interrupts with ID smaller than 1020
> will be handled.

Good idea

> 
> >   	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);
> 
> No need to minus GIC_FIRST_SPI.  And therefore these two cases
> can actually be merged.

Yup, will do

> 
> > +		pending = readl(ispendr + offset) & mask;
> > +		active = readl(isactiver + offset) & mask;
> >   	}
> >   	if (!active && !pending)
> 
> Otherwise this looks good enough (to me) for now, and let's wait
> other reviewers to comment.  I've used the following diff to give
> the pl031 test a go (roughly written, not dig into PL031 so much),
> it just works fine :)
> 
> 
> Thanks,
> Zenghui
> 
> diff --git a/arm/pl031.c b/arm/pl031.c
> index 86035fa..2d4160f 100644
> --- a/arm/pl031.c
> +++ b/arm/pl031.c
> @@ -118,11 +118,12 @@ static int check_rtc_freq(void)
>  	return 0;
>  }
> 
> -static bool gic_irq_pending(void)
> +static bool gic_pl031_pending(void)
>  {
> -	uint32_t offset = (pl031_irq / 32) * 4;
> +	enum gic_irq_state state = gic_irq_state(pl031_irq);
> 
> -	return readl(gic_ispendr + offset) & (1 << (pl031_irq & 31));
> +	return state == GIC_IRQ_STATE_PENDING ||
> +		state == GIC_IRQ_STATE_ACTIVE_PENDING;

Nice way to test, but I'll leave this change out.

>  }
> 
>  static void gic_irq_unmask(void)
> 
> [...]
> /* replace all gic_irq_pending() with gic_pl031_pending() */
> [...]
> 
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 3924dd1..34d77e3 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -152,6 +152,7 @@ enum gic_irq_state gic_irq_state(int irq)
>  	enum gic_irq_state state;
>  	void *ispendr, *isactiver;
>  	bool pending, active;
> +	int offset, mask;
> 
>  	assert(gic_common_ops);
> 
> @@ -173,15 +174,10 @@ enum gic_irq_state gic_irq_state(int irq)
>  		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;
> -	}
> +	offset = (irq / 32) * 4;
> +	mask = 1 << (irq % 32);
> +	pending = readl(ispendr + offset) & mask;
> +	active = readl(isactiver + offset) & mask;
> 
>  	if (!active && !pending)
>  		state = GIC_IRQ_STATE_INACTIVE;
>

I'll send a final patch now for review, but I'm pretty happy with this so
I've gone ahead and squashed it into arm/queue already. I kept Alex's
r-b as there shouldn't be any functional change with respect to what
he reviewed.

Thanks,
drew




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux