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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
