[Qemu-devel] [PATCH 00/10] pci: pci_register_bar_simple

[Qemu-devel] [PATCH 00/10] pci: pci_register_bar_simple

[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.

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(-)

[Qemu-devel] [PATCH 01/10] pci: add pci_register_bar_simple() API

[Avi Kivity]

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

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/pci.c |   17 +++++++++++++++++
 hw/pci.h |    3 +++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 6b577e1..65ae196 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -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)
 {
diff --git a/hw/pci.h b/hw/pci.h
index 52ee8c9..3230764 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -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);
-- 
1.7.1

[Qemu-devel] [PATCH 02/10] rtl8139: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/rtl8139.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d545933..822038d 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -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);
 
-- 
1.7.1

[Qemu-devel] [PATCH 03/10] cirrus-vga: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/cirrus_vga.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 2724f7b..ff78335 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -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;
 }
-- 
1.7.1

[Qemu-devel] [PATCH 04/10] eepro100: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/eepro100.c |   43 +++++++++++++------------------------------
 1 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index edf48f6..f2505e4 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -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);
 
-- 
1.7.1

[Qemu-devel] [PATCH 05/10] ich/ahci: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/ide/ahci.c |    9 ---------
 hw/ide/ahci.h |    3 ---
 hw/ide/ich.c  |    3 +--
 3 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 98bdf70..c6e0c77 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -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;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index a4560c4..dc86951 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -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 */
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index f242d7a..eb00f03 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -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);
 
-- 
1.7.1

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

[Jan Kiszka]

On 2011-04-04 17:28, Avi Kivity wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  hw/ide/ahci.c |    9 ---------
>  hw/ide/ahci.h |    3 ---
>  hw/ide/ich.c  |    3 +--
>  3 files changed, 1 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 98bdf70..c6e0c77 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -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;
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index a4560c4..dc86951 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -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 */
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index f242d7a..eb00f03 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -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 <jan.kiszka@siemens.com>

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

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ide/ich.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index e44339b..6150ce3 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -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;
 }
 

[Qemu-devel] [PATCH 06/10] hda-intel: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/intel-hda.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index b0b1d12..7f83745 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -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);
     }
-- 
1.7.1

[Qemu-devel] [PATCH 07/10] hda-intel: convert to pci_register_bar_simple() (partial)

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/lsi53c895a.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index e4b51a8..be4df58 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -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);
-- 
1.7.1

[Qemu-devel] [PATCH 08/10] pcnet-pci: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/pcnet-pci.c |   16 +---------------
 1 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/hw/pcnet-pci.c b/hw/pcnet-pci.c
index 339a401..4ac3e32 100644
--- a/hw/pcnet-pci.c
+++ b/hw/pcnet-pci.c
@@ -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;
-- 
1.7.1

[Qemu-devel] [PATCH 09/10] usb-ohci: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/usb-ohci.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index d2b14f7..73d47b8 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -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;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 10/10] wdt_i6300esb: convert to pci_register_bar_simple()

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/wdt_i6300esb.c |   42 +++++++++++++++---------------------------
 1 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 4a7fba7..0791721 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -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;
 }
-- 
1.7.1

[Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Michael S. Tsirkin]

On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> 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
Acked-by: Michael S. Tsirkin <mst@redhat.com>

I really hope the rest of devices will follow.

> 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(-)

[Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Avi Kivity]

On 04/04/2011 06:59 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >  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
> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>
> 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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Anthony Liguori]

On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>> 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
> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>
> 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.

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

Regards,

Anthony Liguori

>> 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(-)

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Avi Kivity]

On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>>> 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
>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>
>> 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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Blue Swirl]

On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
> On 04/04/2011 07:22 PM, Anthony Liguori wrote:
>>
>> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
>>>
>>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>>>>
>>>> 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
>>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>>
>>> 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.

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Michael S. Tsirkin]

On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> >>
> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> >>>
> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >>>>
> >>>> 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
> >>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
> >>>
> >>> 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.

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

> 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.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Blue Swirl]

On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
>> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
>> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
>> >>
>> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
>> >>>
>> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>> >>>>
>> >>>> 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
>> >>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>> >>>
>> >>> 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.
>
> 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.

>> 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.

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Michael S. Tsirkin]

On Tue, Apr 05, 2011 at 12:05:08AM +0300, Blue Swirl wrote:
> On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
> >> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
> >> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> >> >>
> >> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> >> >>>
> >> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >> >>>>
> >> >>>> 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
> >> >>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
> >> >>>
> >> >>> 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.
> >
> > 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.

> >> 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.

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

-- 
MST

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Blue Swirl]

On Tue, Apr 5, 2011 at 12:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 05, 2011 at 12:05:08AM +0300, Blue Swirl wrote:
>> On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
>> >> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
>> >> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
>> >> >>
>> >> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
>> >> >>>
>> >> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>> >> >>>>
>> >> >>>> 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
>> >> >>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>> >> >>>
>> >> >>> 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.
>> >
>> > 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.
>
>> >> 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.
>
> 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.

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Michael S. Tsirkin]

On Tue, Apr 05, 2011 at 09:02:45PM +0300, Blue Swirl wrote:
> On Tue, Apr 5, 2011 at 12:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Apr 05, 2011 at 12:05:08AM +0300, Blue Swirl wrote:
> >> On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
> >> >> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <avi@redhat.com> wrote:
> >> >> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> >> >> >>
> >> >> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> >> >> >>>
> >> >> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >> >> >>>>
> >> >> >>>> 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
> >> >> >>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
> >> >> >>>
> >> >> >>> 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.
> >> >
> >> > 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.
> >
> >> >> 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.
> >
> > 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.

-- 
MST

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Avi Kivity]

On 04/04/2011 08:02 PM, Blue Swirl wrote:
> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity<avi@redhat.com>  wrote:
> >  On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> >>
> >>  On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> >>>
> >>>  On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >>>>
> >>>>  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
> >>>  Acked-by: Michael S. Tsirkin<mst@redhat.com>
> >>>
> >>>  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.

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Blue Swirl]

On Tue, Apr 5, 2011 at 10:39 AM, Avi Kivity <avi@redhat.com> wrote:
> On 04/04/2011 08:02 PM, Blue Swirl wrote:
>>
>> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity<avi@redhat.com>  wrote:
>> >  On 04/04/2011 07:22 PM, Anthony Liguori wrote:
>> >>
>> >>  On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
>> >>>
>> >>>  On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
>> >>>>
>> >>>>  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
>> >>>  Acked-by: Michael S. Tsirkin<mst@redhat.com>
>> >>>
>> >>>  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.
>
> 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.

Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Avi Kivity]

On 04/05/2011 09:06 PM, Blue Swirl wrote:
> >
> >  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.

-- 
error compiling committee.c: too many arguments to function