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

login
register
mail settings
Submitter Avi Kivity
Date July 25, 2011, 2:03 p.m.
Message ID <1311602584-23409-22-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/106704/
State New
Headers show

Comments

Avi Kivity - July 25, 2011, 2:03 p.m.
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>
---
 hw/pci.c |   47 +++++++++++++++++++++++++++++++++++++++--------
 hw/pci.h |    3 +++
 2 files changed, 42 insertions(+), 8 deletions(-)
Anthony Liguori - July 25, 2011, 8:20 p.m.
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);
Avi Kivity - July 26, 2011, 11:06 a.m.
On 07/25/2011 11:20 PM, Anthony Liguori wrote:
> 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))
>

I haven't really considered this, but it makes sense to consider the 
initiating point for DMA.  This allows us to do IOMMU transformations 
naturally.

Patch

diff --git a/hw/pci.c b/hw/pci.c
index cf16f3b..36db58b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -844,10 +844,15 @@  static void pci_unregister_io_regions(PCIDevice *pci_dev)
         if (r->type == PCI_BASE_ADDRESS_SPACE_IO) {
             isa_unassign_ioport(r->addr, r->filtered_size);
         } else {
-            cpu_register_physical_memory(pci_to_cpu_addr(pci_dev->bus,
-                                                         r->addr),
-                                         r->filtered_size,
-                                         IO_MEM_UNASSIGNED);
+            if (r->memory) {
+                memory_region_del_subregion(pci_dev->bus->address_space,
+                                            r->memory);
+            } else {
+                cpu_register_physical_memory(pci_to_cpu_addr(pci_dev->bus,
+                                                             r->addr),
+                                             r->filtered_size,
+                                             IO_MEM_UNASSIGNED);
+            }
         }
     }
 }
@@ -893,6 +898,7 @@  void pci_register_bar(PCIDevice *pci_dev, int region_num,
     r->type = type;
     r->map_func = map_func;
     r->ram_addr = IO_MEM_UNASSIGNED;
+    r->memory = NULL;
 
     wmask = ~(size - 1);
     addr = pci_bar(pci_dev, region_num);
@@ -918,6 +924,16 @@  static void pci_simple_bar_mapfunc(PCIDevice *pci_dev, int region_num,
                                  pci_dev->io_regions[region_num].ram_addr);
 }
 
+static void pci_simple_bar_mapfunc_region(PCIDevice *pci_dev, int region_num,
+                                          pcibus_t addr, pcibus_t size,
+                                          int type)
+{
+    memory_region_add_subregion_overlap(pci_dev->bus->address_space,
+                                        addr,
+                                        pci_dev->io_regions[region_num].memory,
+                                        1);
+}
+
 void pci_register_bar_simple(PCIDevice *pci_dev, int region_num,
                              pcibus_t size,  uint8_t attr, ram_addr_t ram_addr)
 {
@@ -927,6 +943,15 @@  void pci_register_bar_simple(PCIDevice *pci_dev, int region_num,
     pci_dev->io_regions[region_num].ram_addr = ram_addr;
 }
 
+void pci_register_bar_region(PCIDevice *pci_dev, int region_num,
+                             uint8_t attr, MemoryRegion *memory)
+{
+    pci_register_bar(pci_dev, region_num, memory_region_size(memory),
+                     PCI_BASE_ADDRESS_SPACE_MEMORY | attr,
+                     pci_simple_bar_mapfunc_region);
+    pci_dev->io_regions[region_num].memory = memory;
+}
+
 static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
                               uint8_t type)
 {
@@ -1065,10 +1090,16 @@  static void pci_update_mappings(PCIDevice *d)
                     isa_unassign_ioport(r->addr, r->filtered_size);
                 }
             } else {
-                cpu_register_physical_memory(pci_to_cpu_addr(d->bus, r->addr),
-                                             r->filtered_size,
-                                             IO_MEM_UNASSIGNED);
-                qemu_unregister_coalesced_mmio(r->addr, r->filtered_size);
+                if (r->memory) {
+                    memory_region_del_subregion(d->bus->address_space,
+                                                r->memory);
+                } else {
+                    cpu_register_physical_memory(pci_to_cpu_addr(d->bus,
+                                                                 r->addr),
+                                                 r->filtered_size,
+                                                 IO_MEM_UNASSIGNED);
+                    qemu_unregister_coalesced_mmio(r->addr, r->filtered_size);
+                }
             }
         }
         r->addr = new_addr;
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);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size);