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

[Andrew Jones <drjones@redhat.com>]

On Mon, Jan 18, 2016 at 05:40:04PM +0100, Radim Krčmář wrote:
> 2016-01-18 14:52+0100, Andrew Jones:
> > On Fri, Jan 15, 2016 at 10:34:09PM +0100, Radim Krčmář wrote:
> >> 2016-01-15 17:51+0100, Andrew Jones:
> >> 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.
> 
> #ifs in plain sight are definitely evil, but we could have wrapped
> read*/write*/in*/out* in an interface that doesn't violate most basic
> types and is hidden with at most one #ifdef.
> 
> >                                                         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.
> 
> I'd continue doing what ioread*() in x86/vmexit.c does.
> Using a loosely-tagged union also means that we don't need to implement
> in*/out* for everyone.
> 
> And because structs are a pain to use, having two separate values
> instead of
>   struct {
>    enum {PIO, MMIO} space;
>    union {u16 port; void * address;};};
> 
> can be excused.
> 
> >>  - 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*.
> 
> Good point, copy-pasting code is convenient and we don't plan to outlive
> Linux anyway ...
> 
> I'd rather drop this patch now and wait for the first use, but it has my
> 
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Thanks, but I'll just drop this patch for now. We'll see how Alex
implements the common pci-testdev. If he needs this stuff to allow it to more
easily be shared with x86, then he can start this battle up again. I'll get
some popcorn and observe from the sideline :-)

v3 on the way.

drew


> 
> >>  - "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.
> 
> Compile error would be the best implementation on arches without port
> space, because they shouldn't access interface for something that
> doesn't have a meaning for them.  Aliasing read*/write* is reasonable
> for arches that map port space into memory space.
> 
> >                               For mmio to work we may need 'unsigned long
> > addr', but that shouldn't stop x86 from defining its version as 'u16 port'.
> 
> It should.  The type system makes certain things easier, like using the
> expected value, and having two separate types for in*/out* defeats it
> without bringing any benefit.
> 
> If it turns out that we need more than u16 addr, x86 in*/out* would be
> better as 'unsigned long port' with 'BUG(port >= 64k)' inside.
> 
> > (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.
> 
> Hm, I'd pick the mistake we do now.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html