Skip to contentSkip to conversation

pci: pci_register_bar_simple

PATCH In review Avi Kivity 5 participants 25 messages Apr 4, 2011 – May 8, 2011

lore
Share & tools
lore

Avi Kivity

author
#0 lore raw

cover pci: pci_register_bar_simple

Many PCI BARs that use the memory address space map a single MMIO region into the entire BAR range. Introduce an API pci_register_bar_simple() for that use case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also a nice code reduction in its own right.

Avi Kivity (10):
pci: add pci_register_bar_simple() API
rtl8139: convert to pci_register_bar_simple()
cirrus-vga: convert to pci_register_bar_simple()
eepro100: convert to pci_register_bar_simple() ich/ahci: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple() (partial)
pcnet-pci: convert to pci_register_bar_simple()
usb-ohci: convert to pci_register_bar_simple()
wdt_i6300esb: convert to pci_register_bar_simple()

 hw/cirrus_vga.c   |   13 ++-----------
 hw/eepro100.c     |   43 +++++++++++++------------------------------
 hw/ide/ahci.c     |    9 ---------
 hw/ide/ahci.h     |    3 ---
 hw/ide/ich.c      |    3 +--
 hw/intel-hda.c    |   11 +----------
 hw/lsi53c895a.c   |   12 +-----------
 hw/pci.c          |   17 +++++++++++++++++
 hw/pci.h          |    3 +++
 hw/pcnet-pci.c    |   16 +---------------
 hw/rtl8139.c      |   11 +----------
 hw/usb-ohci.c     |   10 +---------
 hw/wdt_i6300esb.c |   42 +++++++++++++++---------------------------
 13 files changed, 56 insertions(+), 137 deletions(-)
author
#1 lore raw

PATCH 1/10 pci: add pci_register_bar_simple() API

This is similar to pci_register_bar(), but automatically registers a single memory region spanning the entire BAR.

---
 hw/pci.c |   17 +++++++++++++++++
 hw/pci.h |    3 +++
 2 files changed, 20 insertions(+), 0 deletions(-)
hw/pci.c +17 -0
index 6b577e1..65ae196 100644
@@ -859,6 +859,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     r->filtered_size = size;
     r->type = type;
     r->map_func = map_func;
+    r->ram_addr = IO_MEM_UNASSIGNED;
 
     wmask = ~(size - 1);
     addr = pci_bar(pci_dev, region_num);
@@ -877,6 +878,22 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 }
 
+static void pci_simple_bar_mapfunc(PCIDevice *pci_dev, int region_num,
+                                   pcibus_t addr, pcibus_t size, int type)
+{
+    cpu_register_physical_memory(addr, size,
+                                 pci_dev->io_regions[region_num].ram_addr);
+}
+
+void pci_register_bar_simple(PCIDevice *pci_dev, int region_num,
+                             pcibus_t size,  uint8_t attr, ram_addr_t ram_addr)
+{
+    pci_register_bar(pci_dev, region_num, size,
+                     PCI_BASE_ADDRESS_SPACE_MEMORY | attr,
+                     pci_simple_bar_mapfunc);
+    pci_dev->io_regions[region_num].ram_addr = ram_addr;
+}
+
 static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
                               uint8_t type)
 {
hw/pci.h +3 -0
index 52ee8c9..3230764 100644
@@ -92,6 +92,7 @@ typedef struct PCIIORegion {
     pcibus_t filtered_size;
     uint8_t type;
     PCIMapIORegionFunc *map_func;
+    ram_addr_t ram_addr;
 } PCIIORegion;
 
 #define PCI_ROM_SLOT 6
@@ -200,6 +201,8 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
                             pcibus_t size, uint8_t type,
                             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);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size);
Signature · 1 line
1.7.1
author
#2 lore raw

PATCH 2/10 rtl8139: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/rtl8139.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)
hw/rtl8139.c +1 -10
index d545933..822038d 100644
@@ -3341,14 +3341,6 @@ static const VMStateDescription vmstate_rtl8139 = {
 /***********************************************************/
 /* PCI RTL8139 definitions */
 
-static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
-                       pcibus_t addr, pcibus_t size, int type)
-{
-    RTL8139State *s = DO_UPCAST(RTL8139State, dev, pci_dev);
-
-    cpu_register_physical_memory(addr + 0, 0x100, s->rtl8139_mmio_io_addr);
-}
-
 static void rtl8139_ioport_map(PCIDevice *pci_dev, int region_num,
                        pcibus_t addr, pcibus_t size, int type)
 {
@@ -3444,8 +3436,7 @@ static int pci_rtl8139_init(PCIDevice *dev)
     pci_register_bar(&s->dev, 0, 0x100,
                            PCI_BASE_ADDRESS_SPACE_IO,  rtl8139_ioport_map);
 
-    pci_register_bar(&s->dev, 1, 0x100,
-                           PCI_BASE_ADDRESS_SPACE_MEMORY, rtl8139_mmio_map);
+    pci_register_bar_simple(&s->dev, 1, 0x100, 0, s->rtl8139_mmio_io_addr);
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
 
Signature · 1 line
1.7.1
author
#3 lore raw

PATCH 3/10 cirrus-vga: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/cirrus_vga.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)
hw/cirrus_vga.c +2 -11
index 2724f7b..ff78335 100644
@@ -3076,15 +3076,6 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
     vga_dirty_log_start(&s->vga);
 }
 
-static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
-				pcibus_t addr, pcibus_t size, int type)
-{
-    CirrusVGAState *s = &DO_UPCAST(PCICirrusVGAState, dev, d)->cirrus_vga;
-
-    cpu_register_physical_memory(addr, CIRRUS_PNPMMIO_SIZE,
-				 s->cirrus_mmio_io_addr);
-}
-
 static void pci_cirrus_write_config(PCIDevice *d,
                                     uint32_t address, uint32_t val, int len)
 {
@@ -3123,8 +3114,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
      pci_register_bar(&d->dev, 0, 0x2000000,
                       PCI_BASE_ADDRESS_MEM_PREFETCH, cirrus_pci_lfb_map);
      if (device_id == CIRRUS_ID_CLGD5446) {
-         pci_register_bar(&d->dev, 1, CIRRUS_PNPMMIO_SIZE,
-                          PCI_BASE_ADDRESS_SPACE_MEMORY, cirrus_pci_mmio_map);
+         pci_register_bar_simple(&d->dev, 1, CIRRUS_PNPMMIO_SIZE, 0,
+                                 s->cirrus_mmio_io_addr);
      }
      return 0;
 }
Signature · 1 line
1.7.1
author
#4 lore raw

PATCH 4/10 eepro100: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/eepro100.c |   43 +++++++++++++------------------------------
 1 files changed, 13 insertions(+), 30 deletions(-)
hw/eepro100.c +13 -30
index edf48f6..f2505e4 100644
@@ -228,7 +228,7 @@ typedef struct {
     uint8_t scb_stat;           /* SCB stat/ack byte */
     uint8_t int_stat;           /* PCI interrupt status */
     /* region must not be saved by nic_save. */
-    uint32_t region[3];         /* PCI region addresses */
+    uint32_t region1;           /* PCI region 1 address */
     uint16_t mdimem[32];
     eeprom_t *eeprom;
     uint32_t device;            /* device variant */
@@ -1488,19 +1488,19 @@ static uint32_t ioport_read1(void *opaque, uint32_t addr)
 #if 0
     logout("addr=%s\n", regname(addr));
 #endif
-    return eepro100_read1(s, addr - s->region[1]);
+    return eepro100_read1(s, addr - s->region1);
 }
 
 static uint32_t ioport_read2(void *opaque, uint32_t addr)
 {
     EEPRO100State *s = opaque;
-    return eepro100_read2(s, addr - s->region[1]);
+    return eepro100_read2(s, addr - s->region1);
 }
 
 static uint32_t ioport_read4(void *opaque, uint32_t addr)
 {
     EEPRO100State *s = opaque;
-    return eepro100_read4(s, addr - s->region[1]);
+    return eepro100_read4(s, addr - s->region1);
 }
 
 static void ioport_write1(void *opaque, uint32_t addr, uint32_t val)
@@ -1509,19 +1509,19 @@ static void ioport_write1(void *opaque, uint32_t addr, uint32_t val)
 #if 0
     logout("addr=%s val=0x%02x\n", regname(addr), val);
 #endif
-    eepro100_write1(s, addr - s->region[1], val);
+    eepro100_write1(s, addr - s->region1, val);
 }
 
 static void ioport_write2(void *opaque, uint32_t addr, uint32_t val)
 {
     EEPRO100State *s = opaque;
-    eepro100_write2(s, addr - s->region[1], val);
+    eepro100_write2(s, addr - s->region1, val);
 }
 
 static void ioport_write4(void *opaque, uint32_t addr, uint32_t val)
 {
     EEPRO100State *s = opaque;
-    eepro100_write4(s, addr - s->region[1], val);
+    eepro100_write4(s, addr - s->region1, val);
 }
 
 /***********************************************************/
@@ -1544,7 +1544,7 @@ static void pci_map(PCIDevice * pci_dev, int region_num,
     register_ioport_write(addr, size, 4, ioport_write4, s);
     register_ioport_read(addr, size, 4, ioport_read4, s);
 
-    s->region[region_num] = addr;
+    s->region1 = addr;
 }
 
 /*****************************************************************************
@@ -1619,22 +1619,6 @@ static CPUReadMemoryFunc * const pci_mmio_read[] = {
     pci_mmio_readl
 };
 
-static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
-                         pcibus_t addr, pcibus_t size, int type)
-{
-    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
-
-    TRACE(OTHER, logout("region %d, addr=0x%08"FMT_PCIBUS", "
-          "size=0x%08"FMT_PCIBUS", type=%d\n",
-          region_num, addr, size, type));
-
-    assert(region_num == 0 || region_num == 2);
-
-    /* Map control / status registers and flash. */
-    cpu_register_physical_memory(addr, size, s->mmio_index);
-    s->region[region_num] = addr;
-}
-
 static int nic_can_receive(VLANClientState *nc)
 {
     EEPRO100State *s = DO_UPCAST(NICState, nc, nc)->opaque;
@@ -1882,17 +1866,16 @@ static int e100_nic_init(PCIDevice *pci_dev)
         cpu_register_io_memory(pci_mmio_read, pci_mmio_write, s,
                                DEVICE_NATIVE_ENDIAN);
 
-    pci_register_bar(&s->dev, 0, PCI_MEM_SIZE,
-                           PCI_BASE_ADDRESS_SPACE_MEMORY |
-                           PCI_BASE_ADDRESS_MEM_PREFETCH, pci_mmio_map);
+    pci_register_bar_simple(&s->dev, 0, PCI_MEM_SIZE,
+                            PCI_BASE_ADDRESS_MEM_PREFETCH, s->mmio_index);
+
     pci_register_bar(&s->dev, 1, PCI_IO_SIZE, PCI_BASE_ADDRESS_SPACE_IO,
                            pci_map);
-    pci_register_bar(&s->dev, 2, PCI_FLASH_SIZE, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                           pci_mmio_map);
+    pci_register_bar_simple(&s->dev, 2, PCI_FLASH_SIZE, 0, s->mmio_index);
 
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     logout("macaddr: %s\n", nic_dump(&s->conf.macaddr.a[0], 6));
-    assert(s->region[1] == 0);
+    assert(s->region1 == 0);
 
     nic_reset(s);
 
Signature · 1 line
1.7.1
author
#5 lore raw

PATCH 5/10 ich/ahci: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/ide/ahci.c |    9 ---------
 hw/ide/ahci.h |    3 ---
 hw/ide/ich.c  |    3 +--
 3 files changed, 1 insertions(+), 14 deletions(-)
hw/ide/ahci.c +0 -9
index 98bdf70..c6e0c77 100644
@@ -1129,15 +1129,6 @@ void ahci_uninit(AHCIState *s)
     qemu_free(s->dev);
 }
 
-void ahci_pci_map(PCIDevice *pci_dev, int region_num,
-        pcibus_t addr, pcibus_t size, int type)
-{
-    struct AHCIPCIState *d = (struct AHCIPCIState *)pci_dev;
-    AHCIState *s = &d->ahci;
-
-    cpu_register_physical_memory(addr, size, s->mem);
-}
-
 void ahci_reset(void *opaque)
 {
     struct AHCIPCIState *d = opaque;
hw/ide/ahci.h +0 -3
index a4560c4..dc86951 100644
@@ -325,9 +325,6 @@ typedef struct NCQFrame {
 void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
 void ahci_uninit(AHCIState *s);
 
-void ahci_pci_map(PCIDevice *pci_dev, int region_num,
-        pcibus_t addr, pcibus_t size, int type);
-
 void ahci_reset(void *opaque);
 
 #endif /* HW_IDE_AHCI_H */
hw/ide/ich.c +1 -2
index f242d7a..eb00f03 100644
@@ -95,8 +95,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     qemu_register_reset(ahci_reset, d);
 
     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
-    pci_register_bar(&d->card, 5, 0x1000, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                     ahci_pci_map);
+    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
 
     msi_init(dev, 0x50, 1, true, false);
 
Signature · 1 line
1.7.1
#6 lore raw

PATCH 1/1 ahci: Unbreak bar registration (was: Re: [PATCH 05/10] ich/ahci: convert to pci_register_bar_simple())

Quoted 46 lines · Avi Kivity
---
 hw/ide/ahci.c |    9 ---------
 hw/ide/ahci.h |    3 ---
 hw/ide/ich.c  |    3 +--
 3 files changed, 1 insertions(+), 14 deletions(-)
hw/ide/ahci.c +0 -9
index 98bdf70..c6e0c77 100644
@@ -1129,15 +1129,6 @@ void ahci_uninit(AHCIState *s)
     qemu_free(s->dev);
 }
 
-void ahci_pci_map(PCIDevice *pci_dev, int region_num,
-        pcibus_t addr, pcibus_t size, int type)
-{
-    struct AHCIPCIState *d = (struct AHCIPCIState *)pci_dev;
-    AHCIState *s = &d->ahci;
-
-    cpu_register_physical_memory(addr, size, s->mem);
-}
-
 void ahci_reset(void *opaque)
 {
     struct AHCIPCIState *d = opaque;
hw/ide/ahci.h +0 -3
index a4560c4..dc86951 100644
@@ -325,9 +325,6 @@ typedef struct NCQFrame {
 void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
 void ahci_uninit(AHCIState *s);
 
-void ahci_pci_map(PCIDevice *pci_dev, int region_num,
-        pcibus_t addr, pcibus_t size, int type);
-
 void ahci_reset(void *opaque);
 
 #endif /* HW_IDE_AHCI_H */
hw/ide/ich.c +0 -0
index f242d7a..eb00f03 100644

hw/ide/ich.c static int pci_ich9_ahci_init(PCIDevice *dev) · line 101

Quoted 9 lines
hw/ide/ich.c +1 -2
@@ -95,8 +95,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     qemu_register_reset(ahci_reset, d);
 
     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
-    pci_register_bar(&d->card, 5, 0x1000, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                     ahci_pci_map);
+    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
 
     msi_init(dev, 0x50, 1, true, false);
 

This breaks AHCI as ahci.mem is not initialized at this point. Fix below.

Did you check the rest of your series regarding such issues?

Jan

-----------8<----------

From: Jan Kiszka <[email protected]>

Fix regression of 667bb59: ahci_init initializes ahci.mem, so we have to move bar registration after it.

---
 hw/ide/ich.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
hw/ide/ich.c +3 -3
index e44339b..6150ce3 100644
@@ -93,14 +93,14 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
 
     qemu_register_reset(ahci_reset, d);
 
-    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
-    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
-
     msi_init(dev, 0x50, 1, true, false);
 
     ahci_init(&d->ahci, &dev->qdev, 6);
     d->ahci.irq = d->card.irq[0];
 
+    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
+    pci_register_bar_simple(&d->card, 5, 0x1000, 0, d->ahci.mem);
+
     return 0;
 }
 
author
#7 lore raw

PATCH 6/10 hda-intel: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/intel-hda.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)
hw/intel-hda.c +1 -10
index b0b1d12..7f83745 100644
@@ -1109,14 +1109,6 @@ static CPUWriteMemoryFunc * const intel_hda_mmio_write[3] = {
     intel_hda_mmio_writel,
 };
 
-static void intel_hda_map(PCIDevice *pci, int region_num,
-                          pcibus_t addr, pcibus_t size, int type)
-{
-    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
-
-    cpu_register_physical_memory(addr, 0x4000, d->mmio_addr);
-}
-
 /* --------------------------------------------------------------------- */
 
 static void intel_hda_reset(DeviceState *dev)
@@ -1158,8 +1150,7 @@ static int intel_hda_init(PCIDevice *pci)
     d->mmio_addr = cpu_register_io_memory(intel_hda_mmio_read,
                                           intel_hda_mmio_write, d,
                                           DEVICE_NATIVE_ENDIAN);
-    pci_register_bar(&d->pci, 0, 0x4000, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                     intel_hda_map);
+    pci_register_bar_simple(&d->pci, 0, 0x4000, 0, d->mmio_addr);
     if (d->msi) {
         msi_init(&d->pci, 0x50, 1, true, false);
     }
Signature · 1 line
1.7.1
author
#8 lore raw

PATCH 7/10 hda-intel: convert to pci_register_bar_simple() (partial)

In reply to Avi Kivity
---
 hw/lsi53c895a.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)
hw/lsi53c895a.c +1 -11
index e4b51a8..be4df58 100644
@@ -2094,15 +2094,6 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
     cpu_register_physical_memory(addr + 0, 0x2000, s->ram_io_addr);
 }
 
-static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
-                             pcibus_t addr, pcibus_t size, int type)
-{
-    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);
-
-    DPRINTF("Mapping registers at %08"FMT_PCIBUS"\n", addr);
-    cpu_register_physical_memory(addr + 0, 0x400, s->mmio_io_addr);
-}
-
 static void lsi_scsi_reset(DeviceState *dev)
 {
     LSIState *s = DO_UPCAST(LSIState, dev.qdev, dev);
@@ -2245,8 +2236,7 @@ static int lsi_scsi_init(PCIDevice *dev)
 
     pci_register_bar(&s->dev, 0, 256,
                            PCI_BASE_ADDRESS_SPACE_IO, lsi_io_mapfunc);
-    pci_register_bar(&s->dev, 1, 0x400,
-                           PCI_BASE_ADDRESS_SPACE_MEMORY, lsi_mmio_mapfunc);
+    pci_register_bar_simple(&s->dev, 1, 0x400, 0, s->mmio_io_addr);
     pci_register_bar(&s->dev, 2, 0x2000,
                            PCI_BASE_ADDRESS_SPACE_MEMORY, lsi_ram_mapfunc);
     QTAILQ_INIT(&s->queue);
Signature · 1 line
1.7.1
author
#9 lore raw

PATCH 8/10 pcnet-pci: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/pcnet-pci.c |   16 +---------------
 1 files changed, 1 insertions(+), 15 deletions(-)
hw/pcnet-pci.c +1 -15
index 339a401..4ac3e32 100644
@@ -214,19 +214,6 @@ static CPUReadMemoryFunc * const pcnet_mmio_read[] = {
     &pcnet_mmio_readl
 };
 
-static void pcnet_mmio_map(PCIDevice *pci_dev, int region_num,
-                            pcibus_t addr, pcibus_t size, int type)
-{
-    PCIPCNetState *d = DO_UPCAST(PCIPCNetState, pci_dev, pci_dev);
-
-#ifdef PCNET_DEBUG_IO
-    printf("pcnet_mmio_map addr=0x%08"FMT_PCIBUS" 0x%08"FMT_PCIBUS"\n",
-           addr, size);
-#endif
-
-    cpu_register_physical_memory(addr, PCNET_PNPMMIO_SIZE, d->state.mmio_index);
-}
-
 static void pci_physical_memory_write(void *dma_opaque, target_phys_addr_t addr,
                                       uint8_t *buf, int len, int do_bswap)
 {
@@ -300,8 +287,7 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
     pci_register_bar(pci_dev, 0, PCNET_IOPORT_SIZE,
                            PCI_BASE_ADDRESS_SPACE_IO, pcnet_ioport_map);
 
-    pci_register_bar(pci_dev, 1, PCNET_PNPMMIO_SIZE,
-                           PCI_BASE_ADDRESS_SPACE_MEMORY, pcnet_mmio_map);
+    pci_register_bar_simple(pci_dev, 1, PCNET_PNPMMIO_SIZE, 0, s->mmio_index);
 
     s->irq = pci_dev->irq[0];
     s->phys_mem_read = pci_physical_memory_read;
Signature · 1 line
1.7.1
author
#10 lore raw

PATCH 9/10 usb-ohci: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/usb-ohci.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)
hw/usb-ohci.c +1 -9
index d2b14f7..73d47b8 100644
@@ -1713,13 +1713,6 @@ typedef struct {
     OHCIState state;
 } OHCIPCIState;
 
-static void ohci_mapfunc(PCIDevice *pci_dev, int i,
-            pcibus_t addr, pcibus_t size, int type)
-{
-    OHCIPCIState *ohci = DO_UPCAST(OHCIPCIState, pci_dev, pci_dev);
-    cpu_register_physical_memory(addr, size, ohci->state.mem);
-}
-
 static int usb_ohci_initfn_pci(struct PCIDevice *dev)
 {
     OHCIPCIState *ohci = DO_UPCAST(OHCIPCIState, pci_dev, dev);
@@ -1737,8 +1730,7 @@ static int usb_ohci_initfn_pci(struct PCIDevice *dev)
     ohci->state.irq = ohci->pci_dev.irq[0];
 
     /* TODO: avoid cast below by using dev */
-    pci_register_bar(&ohci->pci_dev, 0, 256,
-                           PCI_BASE_ADDRESS_SPACE_MEMORY, ohci_mapfunc);
+    pci_register_bar_simple(&ohci->pci_dev, 0, 256, 0, ohci->state.mem);
     return 0;
 }
 
Signature · 1 line
1.7.1
author
#11 lore raw

PATCH 10/10 wdt_i6300esb: convert to pci_register_bar_simple()

In reply to Avi Kivity
---
 hw/wdt_i6300esb.c |   42 +++++++++++++++---------------------------
 1 files changed, 15 insertions(+), 27 deletions(-)
hw/wdt_i6300esb.c +15 -27
index 4a7fba7..0791721 100644
@@ -355,31 +355,6 @@ static void i6300esb_mem_writel(void *vp, target_phys_addr_t addr, uint32_t val)
     }
 }
 
-static void i6300esb_map(PCIDevice *dev, int region_num,
-                         pcibus_t addr, pcibus_t size, int type)
-{
-    static CPUReadMemoryFunc * const mem_read[3] = {
-        i6300esb_mem_readb,
-        i6300esb_mem_readw,
-        i6300esb_mem_readl,
-    };
-    static CPUWriteMemoryFunc * const mem_write[3] = {
-        i6300esb_mem_writeb,
-        i6300esb_mem_writew,
-        i6300esb_mem_writel,
-    };
-    I6300State *d = DO_UPCAST(I6300State, dev, dev);
-    int io_mem;
-
-    i6300esb_debug("addr = %"FMT_PCIBUS", size = %"FMT_PCIBUS", type = %d\n",
-                   addr, size, type);
-
-    io_mem = cpu_register_io_memory(mem_read, mem_write, d,
-                                    DEVICE_NATIVE_ENDIAN);
-    cpu_register_physical_memory (addr, 0x10, io_mem);
-    /* qemu_register_coalesced_mmio (addr, 0x10); ? */
-}
-
 static const VMStateDescription vmstate_i6300esb = {
     .name = "i6300esb_wdt",
     .version_id = sizeof(I6300State),
@@ -407,6 +382,17 @@ static int i6300esb_init(PCIDevice *dev)
 {
     I6300State *d = DO_UPCAST(I6300State, dev, dev);
     uint8_t *pci_conf;
+    int io_mem;
+    static CPUReadMemoryFunc * const mem_read[3] = {
+        i6300esb_mem_readb,
+        i6300esb_mem_readw,
+        i6300esb_mem_readl,
+    };
+    static CPUWriteMemoryFunc * const mem_write[3] = {
+        i6300esb_mem_writeb,
+        i6300esb_mem_writew,
+        i6300esb_mem_writel,
+    };
 
     i6300esb_debug("I6300State = %p\n", d);
 
@@ -418,8 +404,10 @@ static int i6300esb_init(PCIDevice *dev)
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_ESB_9);
     pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER);
 
-    pci_register_bar(&d->dev, 0, 0x10,
-                            PCI_BASE_ADDRESS_SPACE_MEMORY, i6300esb_map);
+    io_mem = cpu_register_io_memory(mem_read, mem_write, d,
+                                    DEVICE_NATIVE_ENDIAN);
+    pci_register_bar_simple(&d->dev, 0, 0x10, 0, io_mem);
+    /* qemu_register_coalesced_mmio (addr, 0x10); ? */
 
     return 0;
 }
Signature · 1 line
1.7.1
Acked-by on the v1 series
#12 lore raw
In reply to Avi Kivity
Quoted 6 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into the entire BAR range. Introduce an API pci_register_bar_simple() for that use case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also a nice code reduction in its own right.

This will save some code, so

I really hope the rest of devices will follow.

Quoted 26 lines

Avi Kivity (10):
pci: add pci_register_bar_simple() API
rtl8139: convert to pci_register_bar_simple()
cirrus-vga: convert to pci_register_bar_simple()
eepro100: convert to pci_register_bar_simple() ich/ahci: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple() (partial)
pcnet-pci: convert to pci_register_bar_simple()
usb-ohci: convert to pci_register_bar_simple()
wdt_i6300esb: convert to pci_register_bar_simple()

 hw/cirrus_vga.c   |   13 ++-----------
 hw/eepro100.c     |   43 +++++++++++++------------------------------
 hw/ide/ahci.c     |    9 ---------
 hw/ide/ahci.h     |    3 ---
 hw/ide/ich.c      |    3 +--
 hw/intel-hda.c    |   11 +----------
 hw/lsi53c895a.c   |   12 +-----------
 hw/pci.c          |   17 +++++++++++++++++
 hw/pci.h          |    3 +++
 hw/pcnet-pci.c    |   16 +---------------
 hw/rtl8139.c      |   11 +----------
 hw/usb-ohci.c     |   10 +---------
 hw/wdt_i6300esb.c |   42 +++++++++++++++---------------------------
 13 files changed, 56 insertions(+), 137 deletions(-)
author
#13 lore raw
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into the entire BAR range. Introduce an API pci_register_bar_simple() for that use case, and convert all users where this can be done trivially.

Quoted 7 lines
Quoted 2 lines

This will reduce the work required to introduce a PCI memory API; it's also a nice code reduction in its own right.

This will save some code, so

I really hope the rest of devices will follow.

Do you see more devices which can be converted to this API? AFAICT, the others will need something more complicated.

Signature · 1 line
error compiling committee.c: too many arguments to function
#14 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin
Quoted 6 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into the entire BAR range. Introduce an API pci_register_bar_simple() for that use case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also a nice code reduction in its own right.

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

From a couple of quick greps, it looks like this covers most everything that it can.

Regards,

Anthony Liguori

Quoted 26 lines
Quoted 26 lines

Avi Kivity (10):
pci: add pci_register_bar_simple() API
rtl8139: convert to pci_register_bar_simple()
cirrus-vga: convert to pci_register_bar_simple()
eepro100: convert to pci_register_bar_simple() ich/ahci: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple()
hda-intel: convert to pci_register_bar_simple() (partial)
pcnet-pci: convert to pci_register_bar_simple()
usb-ohci: convert to pci_register_bar_simple()
wdt_i6300esb: convert to pci_register_bar_simple()

  hw/cirrus_vga.c   |   13 ++-----------
  hw/eepro100.c     |   43 +++++++++++++------------------------------
  hw/ide/ahci.c     |    9 ---------
  hw/ide/ahci.h     |    3 ---
  hw/ide/ich.c      |    3 +--
  hw/intel-hda.c    |   11 +----------
  hw/lsi53c895a.c   |   12 +-----------
  hw/pci.c          |   17 +++++++++++++++++
  hw/pci.h          |    3 +++
  hw/pcnet-pci.c    |   16 +---------------
  hw/rtl8139.c      |   11 +----------
  hw/usb-ohci.c     |   10 +---------
  hw/wdt_i6300esb.c |   42 +++++++++++++++---------------------------
  13 files changed, 56 insertions(+), 137 deletions(-)
author
#15 lore raw
Quoted 6 lines · Anthony Liguori
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into the entire BAR range. Introduce an API pci_register_bar_simple() for that use case, and convert all users where this can be done trivially.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

This will reduce the work required to introduce a PCI memory API; it's also a nice code reduction in its own right.

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert. There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API). The rest likely need to stick with the callback-based API.

Signature · 1 line
error compiling committee.c: too many arguments to function
#16 lore raw
Quoted 19 lines · Avi Kivity
Quoted 18 lines · Anthony Liguori
Quoted 16 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

This will save some code, so

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

#17 lore raw
Quoted 24 lines · Blue Swirl
Quoted 23 lines · Avi Kivity
Quoted 21 lines · Anthony Liguori
Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

Quoted 8 lines
Quoted 3 lines

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

Quoted 7 lines
Quoted 4 lines

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Yes, I liked that patchset too. What happened to it?

Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

Is that really that fundamental? What I do care about is making pci.c track and register all device memory so that we can finally implement pci bridge features such as master abort handling and unmapped memory.

Signature · 1 line
MST
#18 lore raw
Quoted 30 lines · Michael S. Tsirkin
Quoted 29 lines · Blue Swirl
Quoted 27 lines · Avi Kivity
Quoted 15 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

Quoted 6 lines
Quoted 4 lines

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

Quoted 4 lines
Quoted 2 lines

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Yes, I liked that patchset too. What happened to it?

Nothing, but I thought that there could be a "perfect" solution.

I like in Avi's version that unnecessary API changes are avoided.

Quoted 9 lines
Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

Is that really that fundamental? What I do care about is making pci.c track and register all device memory so that we can finally implement pci bridge features such as master abort handling and unmapped memory.

The structure version can be done later. Right, pci.c should manage the device mappings.

#19 lore raw
Quoted 31 lines · Blue Swirl
Quoted 30 lines · Michael S. Tsirkin
Quoted 29 lines · Blue Swirl
Quoted 27 lines · Avi Kivity
Quoted 15 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

Quoted 6 lines
Quoted 4 lines

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Yes, I liked that patchset too. What happened to it?

Nothing, but I thought that there could be a "perfect" solution.

I like in Avi's version that unnecessary API changes are avoided.

Yes, it's nice that it's incremental.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

Quoted 7 lines
Quoted 4 lines

Is that really that fundamental? What I do care about is making pci.c track and register all device memory so that we can finally implement pci bridge features such as master abort handling and unmapped memory.

The structure version can be done later. Right, pci.c should manage the device mappings.

OK, so applying Avi's patchset and building on that is your preferred approach too?

Signature · 1 line
MST
#20 lore raw
Quoted 53 lines · Michael S. Tsirkin
Quoted 31 lines · Blue Swirl
Quoted 30 lines · Michael S. Tsirkin
Quoted 29 lines · Blue Swirl
Quoted 27 lines · Avi Kivity
Quoted 15 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

Quoted 6 lines
Quoted 4 lines

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Yes, I liked that patchset too. What happened to it?

Nothing, but I thought that there could be a "perfect" solution.

I like in Avi's version that unnecessary API changes are avoided.

Yes, it's nice that it's incremental.

Quoted 10 lines
Quoted 9 lines
Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

Is that really that fundamental? What I do care about is making pci.c track and register all device memory so that we can finally implement pci bridge features such as master abort handling and unmapped memory.

Quoted 5 lines
Quoted 2 lines

The structure version can be done later. Right, pci.c should manage the device mappings.

OK, so applying Avi's patchset and building on that is your preferred approach too?

Avi's version is a bit too simple, at least multiple regions in a BAR should be handled (for example macio.c needs that). But also that can be added later (pci_register_bar_not_so_simple()?), so Avi's version could be used as the starting point.

#21 lore raw
Quoted 57 lines · Blue Swirl
Quoted 56 lines · Michael S. Tsirkin
Quoted 31 lines · Blue Swirl
Quoted 30 lines · Michael S. Tsirkin
Quoted 29 lines · Blue Swirl
Quoted 27 lines · Avi Kivity
Quoted 15 lines · Anthony Liguori
Quoted 13 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range.  Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

Quoted 6 lines
Quoted 4 lines

This will save some code, so

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert.  There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API).  The rest likely need to stick with the callback-based API.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Yes, I liked that patchset too. What happened to it?

Nothing, but I thought that there could be a "perfect" solution.

I like in Avi's version that unnecessary API changes are avoided.

Yes, it's nice that it's incremental.

Quoted 12 lines
Quoted 9 lines
Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

Is that really that fundamental? What I do care about is making pci.c track and register all device memory so that we can finally implement pci bridge features such as master abort handling and unmapped memory.

The structure version can be done later. Right, pci.c should manage the device mappings.

Quoted 7 lines
Quoted 2 lines

OK, so applying Avi's patchset and building on that is your preferred approach too?

Avi's version is a bit too simple, at least multiple regions in a BAR should be handled (for example macio.c needs that). But also that can be added later (pci_register_bar_not_so_simple()?), so Avi's version could be used as the starting point.

OK I've put it on my branch so people have the chance to comment, and they can be built upon.

Signature · 1 line
MST
author
#22 lore raw
In reply to Blue Swirl
Quoted 29 lines · Blue Swirl
Quoted 19 lines · Avi Kivity
Quoted 18 lines · Anthony Liguori
Quoted 16 lines · Michael S. Tsirkin
Quoted 10 lines · Avi Kivity

Many PCI BARs that use the memory address space map a single MMIO region into
the entire BAR range. Introduce an API pci_register_bar_simple() for that use
case, and convert all users where this can be done trivially.

This will reduce the work required to introduce a PCI memory API; it's also
a nice code reduction in its own right.

This will save some code, so

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I really hope the rest of devices will follow.

How complete is this?

I converted all devices which were easy to convert. There may be one or two more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing() API). The rest likely need to stick with the callback-based API.

Quoted 7 lines

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

The more complicated BARs cannot be described declaratively (at least without a lot of complicated infrastructure). They can switch from RAM to MMIO mappings at runtime, and have different sub-regions.

Signature · 1 line
error compiling committee.c: too many arguments to function
#23 lore raw
Quoted 39 lines · Avi Kivity
Quoted 38 lines · Blue Swirl
Quoted 24 lines · Avi Kivity
Quoted 23 lines · Anthony Liguori
Quoted 20 lines · Michael S. Tsirkin
Quoted 9 lines · Avi Kivity

 Many PCI BARs that use the memory address space map a single MMIO region
 into
 the entire BAR range.  Introduce an API pci_register_bar_simple() for
 that use
 case, and convert all users where this can be done trivially.

Quoted 4 lines

 This will reduce the work required to introduce a PCI memory API; it's
 also
 a nice code reduction in its own right.

 This will save some code, so

 I really hope the rest of devices will follow.

Quoted 8 lines
Quoted 1 line

 How complete is this?

 I converted all devices which were easy to convert.  There may be one or two
 more that can be converted with additional work (and perhaps with an additional pic_bar_get_current_address() API, and a pci_bar_set_coalescing()
 API).  The rest likely need to stick with the callback-based API.

In my version which I sent earlier but didn't commit, also other BARs besides the first one and also tricky devices like VGA were handled.

Quoted 8 lines
Quoted 4 lines

But I didn't commit it because I felt it was not going to right direction. I think the BARs should be specified in PCIDeviceInfo instead of adding more function calls. The same applies to this patch set.

The more complicated BARs cannot be described declaratively (at least without a lot of complicated infrastructure).  They can switch from RAM to MMIO mappings at runtime, and have different sub-regions.

Subregions should be possible, but I agree run time switch will not. Those should be pretty rare, though.

author
#24 lore raw
Quoted 7 lines · Blue Swirl
Quoted 4 lines

The more complicated BARs cannot be described declaratively (at least without a lot of complicated infrastructure). They can switch from RAM to MMIO mappings at runtime, and have different sub-regions.

Subregions should be possible, but I agree run time switch will not. Those should be pretty rare, though.

Right. I think the first priority is to have a pci specific API instead of calling cpu_register_physical_memory(); moving to a declarative API is nice but less important IMO.

Signature · 1 line
error compiling committee.c: too many arguments to function