On Mon, May 23, 2016 at 05:17:16PM +0200, Andrew Jones wrote:
> > > > +static void pio_writel(u32 value, volatile void *addr)
> > > > +{
> > > > + outl(value, (unsigned long)addr);
> > > > +}
> > >
> > > Ah, so the above is why you were attempting to add the io-accessor
> > > wrappers around mmio-accessors to asm-generic. Rather than do that,
> >
> > I also tried to unify io-accessors across archs Linux style.
>
> Yeah, but not everything Linux does is nice :-)
>
> >
> > > I think we can just allow the unit test to populate
> > > pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I
> > > think) like
> > >
> > > .io_readb = __raw_readb,
> > > .io_readw = __raw_readw,
> > > .io_readl = __raw_readl,
> > > .io_writeb = __raw_writeb,
> > > .io_writew = __raw_writew,
> > > .io_writel = __raw_writel,
> > >
> > > while an x86 unit test would use ins and outs.
> > >
> > > You could maybe even still provide the ops structs pre-populated,
> > > but you'll need to use some #ifdef __arm__ type of stuff then.
> >
> > Out of these two options the latter makes more sense to me, since
> > type of access to PCI resources is derived from that resource type
> > rather than from unit test parameters. And that deriving could be
> > done "automatically".
> >
> > So I would do the #ifdefs, but that will look like a worsened
> > version of generic io-accessors ;)
> >
> > So what is your preference here?
>
> My latter suggestion, and I think I was wrong about needing ifdefs.
> Anyway, let's see how it turns out.
I am struggling to make it any better. Ifdefs would look less than nice
indeed, but the only alternative I can think of is putting pci-testdev
io accessors to <arch>/asm. But pci-testdev io accessors in asm would
be just ugly, because there is nothing special about pci-testdev
specific io-accessors. They are still arch's io accessors. So why not
just make io accessors unified and Linux-style?
> > > > diff --git a/lib/pci.h b/lib/pci.h
> > > > index 36dd67e19838..03452b059412 100644
> > > > --- a/lib/pci.h
> > > > +++ b/lib/pci.h
> > > > @@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> > > > void pci_type_desc(int type, char *desc, int len);
> > > > void pci_print(void);
> > > >
> > > > +int pci_testdev(void);
> > > > +
> > > > /*
> > > > * pci-testdev is a driver for the pci-testdev qemu pci device. The
> > > > * device enables testing mmio and portio exits, and measuring their
> > > > @@ -33,7 +35,12 @@ void pci_print(void);
> > > > #define PCI_VENDOR_ID_REDHAT 0x1b36
> > > > #define PCI_DEVICE_ID_REDHAT_TEST 0x0005
> > > >
> > > > +/*
> > > > + * pci-testdev supports at least three types of tests (via mmio and
> > > > + * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
> > > > + */
> > > > #define PCI_TESTDEV_NUM_BARS 2
> > > > +#define PCI_TESTDEV_NUM_TESTS 3
> > >
> > > you don't seem to use this define anywhere. maybe the next patch
Yeah, PCI_TESTDEV_NUM_TESTS is used by the next patch. But it looks
cleaner to me to introduce it here - as this patch kind of makes
pci-testdev "complete". While the next patch is just pci-testdev
enabler for ARM.
Leave it here or move?
Thanks!
> > > thanks,
> > > drew
--
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
