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@xxxxxxxxxx>
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@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
