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

[Anthony Liguori <anthony@codemonkey.ws>]

Avi Kivity wrote:
>
> 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.

bar is okay by me.

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

I was thinking that the first API (which is very similar to your
original API) would be the implementation with the second API being
helpers that everyone actually used.

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

As long as we agree on the final API, we can take incremental steps there.

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

So let's pick a better name then physical_memory and I think we've got a
good starting point.

Regards,

Anthony Liguori