Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration

[Anthony Liguori <anthony@codemonkey.ws>]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Avi Kivity wrote:
>>
>> That's because it's an internal performance hack.  We should just 
>> avoid the PCI routines for that device, if we can, although that 
>> suggests we need a map hook which is ugly.  Clever ideas are welcome.
>
> My original proposal.  Note it uses ram addresses, not cpu physical 
> addresses.

I've thought about it, and I think what I find confusing about your API 
is that pci_register_physical_memory includes the phrase "physical 
memory".  A PIO IO region on x86 is definitely not physical memory though.

It overloads the term physical memory and still requires separate APIs 
for IO regions and MEM regions.  I know you mentioned that ram_addr_t 
could be overloaded to also support IO regions but IMHO that's rather 
confusing.  If the new code looked like:

    s->rtl8139_mmio_io_addr =
    cpu_register_io_memory(0, rtl8139_mmio_read, rtl8139_mmio_write, s);

    s->rtl8139_io_io_addr =
    cpu_register_io_memory(0, rtl8139_ioport_read, rtl8139_ioport_write, s);

    pci_register_io_region(&d->dev, 0, 0x100,
                           PCI_ADDRESS_SPACE_IO,  s->rtl8139_io_io_addr);

    pci_register_io_region(&d->dev, 1, 0x100,
                           PCI_ADDRESS_SPACE_MEM, s->rtl8139_mmio_addr);

I think it would be more understandable.  However, I think that the 
normal case is exactly this work flow so I think it makes sense to 
collapse it into two function calls.  So it could look like:

    pci_register_io_region(&d->dev, 0, 0x100,
                           PCI_ADDRESS_SPACE_IO, rtl8139_ioport_read, 
rtl8139_ioport_write, s);

    pci_register_io_region(&d->dev, 1, 0x100,
                           PCI_ADDRESS_SPACE_MEM, rtl8139_mmio_read, 
rtl8139_mmio_write, s);

Moreover, you could probably drop the opaque parameter and and just use 
d->dev.  I hope it's possible to get from one to the other.

You could still have a two step process for where it's absolutely 
required (like VGA optimization).

I think it's worth looking at changing the signatures of the mem 
read/write functions.  Introducing a size parameter would greatly 
simplify adding 64-bit IO support, for instance.

I would argue that ram_addr_t is the wrong thing to overload for PIO but 
as long as it's not exposed in the common API, it doesn't matter that 
much to me.

Regards,

Anthony Liguori