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>
>> - "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
