Re: [PATCH 21/23] pci: add MemoryRegion based BAR management API

[Anthony Liguori <anthony@codemonkey.ws>]

On 07/25/2011 09:03 AM, Avi Kivity wrote:
> Allow registering a BAR using a MemoryRegion.  Once all users are converted,
> pci_register_bar() and pci_register_bar_simple() will be removed.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

> diff --git a/hw/pci.h b/hw/pci.h
> index cfeb042..c51156d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -94,6 +94,7 @@ typedef struct PCIIORegion {
>       uint8_t type;
>       PCIMapIORegionFunc *map_func;
>       ram_addr_t ram_addr;
> +    MemoryRegion *memory;
>   } PCIIORegion;
>
>   #define PCI_ROM_SLOT 6
> @@ -204,6 +205,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>                               PCIMapIORegionFunc *map_func);
>   void pci_register_bar_simple(PCIDevice *pci_dev, int region_num,
>                                pcibus_t size, uint8_t attr, ram_addr_t ram_addr);
> +void pci_register_bar_region(PCIDevice *pci_dev, int region_num,
> +                             uint8_t attr, MemoryRegion *memory);

This ends up being a very nice API.  I had always thought this should be 
a PCI specific set of callbacks but I do see the benefits of having the 
callbacks be generic.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

One thing I'm curious about, what's the symmetric view of this API?

Would you see a device doing something like:

memory_region_read(&dev->pci_bus->memory, addr, &data, sizeof(data))

?

Regards,

Anthony Liguori

>   int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>                          uint8_t offset, uint8_t size);