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

[Avi Kivity <avi@redhat.com>]

Anthony Liguori wrote:
> 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.

Currently the API does not support PIO.

I wanted to make it an io_region as well, but pci_register_io_region() 
is taken.  We could rename it to pci_register_bar(), but that will cause 
too much churn IMO.

> 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);

That neglects the case of direct mapping.  We could add it as a helper 
though (which would also need to unregister the ram_addr when the 
io_region is unregistered).

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

Yes.

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

Well, let's separate those unrelated changes, otherwise nothing will get 
done.

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

Currently ram_addr_t is really a 64-bit encoding for a QEMUIORegion 
object, plus support for address arithmetic.  IMO we should drop it and 
move towards explicit object representation, and drop the page 
descriptor array.

One of the things that prevents this is that the page descriptor array 
is the only place holding the physical -> ioregion mapping.  Once we 
move this information to other objects, we can start on that.  Hence 
this patchset.

-- 
error compiling committee.c: too many arguments to function