Re: [kvm-unit-tests PATCH v2 01/10] asm-generic: add portio accessors to io.h

[Andrew Jones <drjones@redhat.com>]

On Fri, Jan 15, 2016 at 10:34:09PM +0100, Radim Krčmář wrote:
> 2016-01-15 17:51+0100, Andrew Jones:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > diff --git a/lib/asm-generic/io.h b/lib/asm-generic/io.h
> > @@ -152,6 +152,58 @@ static inline u64 __bswap64(u64 x)
> > +#ifndef PCI_IOBASE
> > +#define PCI_IOBASE ((void *)0)
> > +#endif
> > +
> > +#ifndef inb
> > +#define inb inb
> 
> (I consider this repeated pattern to be very ugly, even by C standards.)
> 
> > +static inline u8 inb(unsigned long addr)
> > +{
> > +	return readb(PCI_IOBASE + addr);
> > +}
> 
> My first reaction was "throw this abomination out!", but Drew explained
> that in*/out* is here because we'll also generalize the x86 PCI code
> (which uses PIO and MMIO) and that Linux has the same code.
> 
> arm, arm64, and unicore32 define PCI_IOBASE in Linux.  I didn't figure
> out why they want to use a PIO based abstraction for MMIO, so the
> interface is fine with me as long as

Having these aliases for read*/write* are less evil than attempting to
share code between arm and x86 with a bunch of #ifdefs. As I didn't yet
write a shared pci-testdev driver though, then this patch can be dropped
for now, if you'd prefer. However at some point it may need to come back.

>  - functions are hidden behind a single #ifndef, like ARCH_HAS_PORT_IO.
>    (Ideally defined as part of global configuration, because it's harder
>     to fail that way.)

I prefer to stay consistent with Linux. It's ugly, but we already have
the ugliness for read* and write*.

>  - "unsigned long addr" is changed to "u16 port";
>    x86 ought to have that and we should use different names if we need
>    different types, because behavior couldn't be the same then.

in*/out* should be portio for x86, and just aliases for read*/write*
mmio for other architectures. For mmio to work we may need 'unsigned long
addr', but that shouldn't stop x86 from defining its version as 'u16 port'.
(Although Linux defines it as 'int' actually).

It does appear that in Linux arch/x86/include/asm/io.h doesn't include
asm-generic/io.h. So, if you'd prefer x86 to not include it here either,
then we can drop the inclusion and the '#define inb inb' type stuff from
lib/x86/asm/io.h.

Thanks,
drew