Re: [kvm-unit-tests PATCH 14/14] x86: intel-iommu: add IR test

[Peter Xu <peterx@redhat.com>]

On Thu, Oct 20, 2016 at 03:45:14PM +0200, Andrew Jones wrote:

[...]

> > --- a/lib/pci-edu.c
> > +++ b/lib/pci-edu.c
> > @@ -35,9 +35,6 @@
> >  #define EDU_CMD_DMA_FROM            (0x02)
> >  #define EDU_CMD_DMA_TO              (0x00)
> >  
> > -#define EDU_STATUS_FACTORIAL        (0x1)
> > -#define EDU_STATUS_INT_ENABLE       (0x80)
> > -
> 
> I think these and all EDU_ defines should have been in
> pci-edu.h in the first place.

Will move them to header file.

> 
> >  #define EDU_DMA_START               (0x40000)
> >  #define EDU_DMA_SIZE_MAX            (4096)
> >  
> > @@ -94,6 +91,21 @@ uint32_t edu_status(pci_edu_dev_t *dev)
> >  	return edu_reg_read(dev, EDU_REG_STATUS);
> >  }
> >  
> > +void edu_setup_intr(pci_edu_dev_t *dev, bool enabled)
> > +{
> > +	edu_reg_write(dev, EDU_REG_STATUS, EDU_STATUS_INT_ENABLE);
> > +}
> > +
> > +void edu_fact_write(pci_edu_dev_t *dev, uint32_t in)
> > +{
> > +	edu_reg_write(dev, EDU_REG_FACTORIAL, in);
> > +}
> > +
> > +uint32_t edu_fact_read(pci_edu_dev_t *dev)
> > +{
> > +	return edu_reg_read(dev, EDU_REG_FACTORIAL);
> > +}
> 
> More wrappers...

Removing...

[...]

> > +static uint32_t factorial(uint32_t in)
> > +{
> > +	uint32_t v = 1;
> > +	while (in) v *= in--;
> > +	return v;
> > +}
> 
> What, no recursion! That's a disgrace to this classic
> problem! Actually I don't think you should bother with
> the function at all. You hard code the input, so just
> precalculate the result.

Ok, and... yes I can use 0x60 to trigger the interrupt, so no need for
any factorial stuffs. Thanks for mentioning. :)

> 
> > +
> > +static volatile int edu_done;
> > +
> > +static void edu_fact_isr(isr_regs_t *regs)
> > +{
> > +	edu_done = 1;
> 
> nit: bool/true

(will fix this)

> 
> > +	eoi();
> > +}
> > +
> > +static void vtd_test_ir(pci_edu_dev_t *dev)
> > +{
> > +#define VTD_TEST_VECTOR (0xee)
> > +	/* Choose any number to calculate the factorial value. */
> > +	const uint32_t fact = 0x8;
> 
> Nobody is choosing anything. How about just
> 
>  const uint32_t fact_input = 8; /* fact(8) == 40320 */

(will skip since I will use 0x60 later...)

> 
> > +
> > +	/*
> > +	 * Enable EDU device interrupt, so when factorial task
> > +	 * finishes, IRQ will be triggered.
> > +	 */
> > +	edu_setup_intr(dev, 1);
> > +
> > +	/*
> > +	 * Setup EDU PCI device MSI, using interrupt remapping. By
> > +	 * default, EDU device is using INTx.
> > +	 */
> > +	vtd_setup_msi(&dev->pci_dev, VTD_TEST_VECTOR, 0);
> > +
> > +	handle_irq(VTD_TEST_VECTOR, edu_fact_isr);
> > +	irq_enable();
> > +
> > +	edu_fact_write(dev, fact);
> > +	while (!edu_done);
> 
> I guess you depend on the test framework's timeout support to
> deal with never getting the interrupt. Make sure you set a
> reasonable timeout in unittests.cfg

Will do.

> 
> > +	/* Now results should be put back to edu fact register */
> > +	report("EDU factorial INTR test",
> > +	       edu_fact_read(dev) == factorial(fact));
> 
> Do we really need this factorial stuff? Can't we just raise an
> interrupt with the EDU register offset 0x60?

As mentioned, will switch to 0x60.

Thanks!

-- peterx