On Sun, May 29, 2016 at 07:48:16PM +0200, Alexander Gordeev wrote:
> 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?
OK, let's give the io function wrappers another round.
>
> > > > > 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?
It's fine here.
Thanks,
drew
>
> 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
--
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
