[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration |
Date: |
Wed, 27 May 2009 17:33:37 -0500 |
User-agent: |
Thunderbird 2.0.0.21 (X11/20090409) |
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
- [Qemu-devel] [PATCH 0/3] Object-based physical memory management, Avi Kivity, 2009/05/24
- [Qemu-devel] [PATCH 1/3] Add PhysicalMemoryRegion type, Avi Kivity, 2009/05/24
- [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/24
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Anthony Liguori, 2009/05/27
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/27
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Anthony Liguori, 2009/05/27
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/27
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration,
Anthony Liguori <=
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Christoph Hellwig, 2009/05/31
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/31
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/31
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Anthony Liguori, 2009/05/31
- Re: [Qemu-devel] [PATCH 2/3] Add PCI memory region registration, Avi Kivity, 2009/05/31
[Qemu-devel] [PATCH 3/3] Convert RTL8139 to use PCI memory regitration facility, Avi Kivity, 2009/05/24
Re: [Qemu-devel] [PATCH 0/3] Object-based physical memory management, Anthony Liguori, 2009/05/27