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

[Anthony Liguori <aliguori@us.ibm.com>]

Avi Kivity wrote:
> A registered PCI memory region will be automatically mapped and unmapped
> as the associated BAR is manipulated.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/pci.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/pci.h |   26 ++++++++++++++++++++
>  2 files changed, 103 insertions(+), 1 deletions(-)
>
> diff --git a/hw/pci.h b/hw/pci.h
> index ff858a1..b27e8fd 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -2,6 +2,7 @@
>  #define QEMU_PCI_H
>  
>  #include "qemu-common.h"
> +#include "sys-queue.h"
>  
>  /* PCI includes legacy ISA access.  */
>  #include "isa.h"
> @@ -80,11 +81,21 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  #define PCI_ADDRESS_SPACE_IO		0x01
>  #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
>  
> +typedef struct PCIIORegionComponent {
> +    PhysicalMemoryRegion *pmr;
> +    target_phys_addr_t offset;
> +    target_phys_addr_t size;
> +    ram_addr_t ram_addr;
> +    ram_addr_t region_offset;
> +    LIST_ENTRY(PCIIORegionComponent) link;
> +} PCIIORegionComponent;
> +
>  typedef struct PCIIORegion {
>      uint32_t addr; /* current PCI mapping address. -1 means not mapped */
>      uint32_t size;
>      uint8_t type;
>      PCIMapIORegionFunc *map_func;
> +    LIST_HEAD(pci_region_list, PCIIORegionComponent) components;
>  } PCIIORegion;
>  
>  #define PCI_ROM_SLOT 6
> @@ -171,6 +182,21 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
>                              uint32_t size, int type,
>                              PCIMapIORegionFunc *map_func);
>  
> +PCIIORegionComponent *pci_register_physical_memory(PCIDevice *pci_dev,
> +                                                   int region_num,
> +                                                   target_phys_addr_t size,
> +                                                   ram_addr_t ram_addr);
>   

PCI devices should not care about physical memory.  They should only 
care about IO regions.  It ought to look something like:

enum {
   PCI_REGION_IO,
   PCI_REGION_MEM,
};

typedef struct PCIIOFunction
{
   uint32_t (*read)(void *opaque, uint64_t offset, int size);
   void (*write)(void *opaque, uint64_t offset, int size, uint64_t value);
} PCIIOFunction;

pci_register_io_region(PCIDevice *pci_dev,
                                     int region_num,
                                     int region_type,
                                     int order,
                                     PCIIOFunction *io_func,
                                     void *opaque);

The current PCI map API goes away.  Devices don't have to know anything 
about that.  Special handling for PIO/MMIO for PCI devices also go 
away.  Devices can register PCI_REGION_IO regions and they'll be 
automatically mapped to MMIO for non-x86 architectures.  This means a 
lot of current PCI devices can be built once instead of being built for 
each architecture.

If you want, we can setup a temporary branch in qemu to stage the 
conversion.  I'm willing to help do the conversion too.

Regards,

Anthony Liguori