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 Tue, Feb 11, 2020 at 01:52:35PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 2/11/20 1:37 PM, 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>
> > ---
> >  arm/timer.c       | 30 ++++++++++++------------------
> >  lib/arm/asm/gic.h | 11 ++++++-----
> >  lib/arm/gic.c     | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 51 insertions(+), 23 deletions(-)
> >
> > diff --git a/arm/timer.c b/arm/timer.c
> > index f5cf775ce50f..3c4e27f20e2e 100644
> > --- a/arm/timer.c
> > +++ b/arm/timer.c
> > @@ -183,28 +183,22 @@ static bool timer_pending(struct timer_info *info)
> >  		(info->read_ctl() & ARCH_TIMER_CTL_ISTATUS);
> >  }
> >  
> > -static enum gic_state gic_timer_state(struct timer_info *info)
> > +static bool gic_timer_check_state(struct timer_info *info,
> > +				  enum gic_irq_state expected_state)
> >  {
> > -	enum gic_state state = GIC_STATE_INACTIVE;
> >  	int i;
> > -	bool pending, active;
> >  
> >  	/* Wait for up to 1s for the GIC to sample the interrupt. */
> >  	for (i = 0; i < 10; i++) {
> > -		pending = readl(gic_ispendr) & (1 << PPI(info->irq));
> > -		active = readl(gic_isactiver) & (1 << PPI(info->irq));
> > -		if (!active && !pending)
> > -			state = GIC_STATE_INACTIVE;
> > -		if (pending)
> > -			state = GIC_STATE_PENDING;
> > -		if (active)
> > -			state = GIC_STATE_ACTIVE;
> > -		if (active && pending)
> > -			state = GIC_STATE_ACTIVE_PENDING;
> >  		mdelay(100);
> > +		if (gic_irq_state(info->irq) == expected_state) {
> > +			mdelay(100);
> > +			if (gic_irq_state(info->irq) == expected_state)
> > +				return true;
> > +		}
> >  	}
> >  
> > -	return state;
> > +	return false;
> >  }
> >  
> >  static bool test_cval_10msec(struct timer_info *info)
> > @@ -253,11 +247,11 @@ static void test_timer(struct timer_info *info)
> >  	/* Enable the timer, but schedule it for much later */
> >  	info->write_cval(later);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE);
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
> >  			"not pending before");
> >  
> >  	info->write_cval(now - 1);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_PENDING,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_PENDING),
> >  			"interrupt signal pending");
> >  
> >  	/* Disable the timer again and prepare to take interrupts */
> > @@ -265,12 +259,12 @@ static void test_timer(struct timer_info *info)
> >  	info->irq_received = false;
> >  	set_timer_irq_enabled(info, true);
> >  	report(!info->irq_received, "no interrupt when timer is disabled");
> > -	report(!timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
> >  			"interrupt signal no longer pending");
> >  
> >  	info->write_cval(now - 1);
> >  	info->write_ctl(ARCH_TIMER_CTL_ENABLE | ARCH_TIMER_CTL_IMASK);
> > -	report(timer_pending(info) && gic_timer_state(info) == GIC_STATE_INACTIVE,
> > +	report(timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE),
> >  			"interrupt signal not pending");
> >  
> >  	report(test_cval_10msec(info), "latency within 10 ms");
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index a72e0cde4e9c..922cbe95750c 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -47,11 +47,11 @@
> >  #ifndef __ASSEMBLY__
> >  #include <asm/cpumask.h>
> >  
> > -enum gic_state {
> > -	GIC_STATE_INACTIVE,
> > -	GIC_STATE_PENDING,
> > -	GIC_STATE_ACTIVE,
> > -	GIC_STATE_ACTIVE_PENDING,
> > +enum gic_irq_state {
> > +	GIC_IRQ_STATE_INACTIVE,
> > +	GIC_IRQ_STATE_PENDING,
> > +	GIC_IRQ_STATE_ACTIVE,
> > +	GIC_IRQ_STATE_ACTIVE_PENDING,
> >  };
> >  
> >  /*
> > @@ -80,6 +80,7 @@ extern u32 gic_iar_irqnr(u32 iar);
> >  extern void gic_write_eoir(u32 irqstat);
> >  extern void gic_ipi_send_single(int irq, int cpu);
> >  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
> > +extern enum gic_irq_state gic_irq_state(int irq);
> >  
> >  #endif /* !__ASSEMBLY__ */
> >  #endif /* _ASMARM_GIC_H_ */
> > diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> > index 94301169215c..0563b31132c8 100644
> > --- a/lib/arm/gic.c
> > +++ b/lib/arm/gic.c
> > @@ -146,3 +146,36 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
> >  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
> >  	gic_common_ops->ipi_send_mask(irq, dest);
> >  }
> > +
> > +enum gic_irq_state gic_irq_state(int irq)
> > +{
> > +	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));
> > +		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;
> > +}
> 
> Looks good. The gic_ispendr and gic_isactiver variables are not used anymore and
> could be removed, but it's not a big deal. Either way:
> 
> Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>

Indeed. I'll remove them and add your r-b while applying.

Thanks,
drew

> 
> Thanks,
> Alex
> 




[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