[Qemu-devel] [PATCH 00/20] PCI express clean up patches.

[Qemu-devel] [PATCH 00/20] PCI express clean up patches.

[Isaku Yamahata]

Here is the patch series to clean up PCI express patches.
Although there remained some issues to address, the PCI express
patches was commited while I wasn't responsive last week. (Sorry for that)
This patch series addresses the remained issues.

They are mostly trivial fixes or cosmetics suggested by Michael.
I think I've covered almost all the issues. If I missed anything,
please be kind to point it out again.

Some random comments:

- PCI configuration space register constant.
  Now they're defined in pci.h and their symbol name is same to Linux's
  pci_regs.h.
  So it would make sense to import Linux pci_regs.h and remove the
  definitions in pci.h. If this is acceptable, I'll create the patch.

- PCI configuration space helper functions.
  Now range checking helper functions are introduced.
  range_overlaps() and so on.
  So it's possible to clean up each PCI devices by using them.
  If acceptable, I'll create the patch.

- endian swap related to PCI host bridge/guest endian/host endian:
  I gave up to address this.
  I'll leave it to someone who knows PPC spec well and has access to
  a big endian host machine.

- PCI bridge clean up:
  PCI bridge stuff needs more clean up. Possibly it would result
  in a new file pci_bridge.c.
  I'd like to address it later. Anyway I have to do it when
  I implement PCI express hot plug.
  
- PCI express initialization:
  Since it uses PCI initialization code, so it isn't separated
  from PCI cleanly.
  One possible way is to introduce PCI express specific qdev
  register function (PCIEDeviceInfo, pcie_qdev_register() and
  pcie_qdev_init() which calls pci_qdev_init()).
  I'm not sure it's worth while at the moment so I'd like to
  leave it untouched for now.

thanks,

Isaku Yamahata (20):
  pci: fix pci_info_device().
  pci: move pci_data_{read, write}() declaration from pci.h to
    pci_host.h
  pci: simplify pci_data_read(), pcie_mmcfg_data_read().
  pci: remove pci_addr_to_config() by open code
  pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev().
  pci: shorten pci_host_{conf, data}_register_xxx function a bit.
  pci: remove pci_sub_bus() by open coding.
  pci: s/pci_find_host_bus/pci_find_root_bus/g
  pci_host: remove unnecessary & 0xff.
  pci: kill unnecessary included in pci.c
  pci: clean up of pci_init_wmask().
  pci: remove some unnecessary comment in pci.h
  pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h.
  pci: remove unused constants.
  pci: clean up of pci_update_mappings()
  pci: kill goto in pci_update_mappings()
  pci: remove magic number, 256 in pci.c
  pci: fix pci_config_get_io_base().
  pci: pci bridge related clean up.
  pci: remove goto in pci_bridge_filter().

 hw/apb_pci.c     |    4 +-
 hw/grackle_pci.c |    8 ++--
 hw/pci-hotplug.c |    4 +-
 hw/pci.c         |  126 ++++++++++++++++++++++++++++-------------------------
 hw/pci.h         |   25 ++---------
 hw/pci_host.c    |   44 +++++++------------
 hw/pci_host.h    |   15 ++++---
 hw/pcie_host.c   |   23 +++------
 hw/pcie_host.h   |    4 +-
 hw/piix_pci.c    |    2 +-
 hw/ppc4xx_pci.c  |    2 +-
 hw/ppce500_pci.c |    4 +-
 hw/prep_pci.c    |    2 +-
 hw/unin_pci.c    |   16 +++---
 qemu-common.h    |    2 +
 15 files changed, 129 insertions(+), 152 deletions(-)

[Qemu-devel] [PATCH 01/20] pci: fix pci_info_device().

[Isaku Yamahata]

It printed wrong limit value of bridge.
This patch fixes it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 2ab1117..4169d4f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -985,7 +985,7 @@ static void pci_info_device(PCIBus *bus, PCIDevice *d)
                        base, limit);
 
         base = pci_bridge_get_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
-        limit= pci_config_get_memory_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
+        limit= pci_bridge_get_limit(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
         monitor_printf(mon,
                        "      memory range [0x%08"PRIx64", 0x%08"PRIx64"]\n",
                        base, limit);
-- 
1.6.0.2

[Qemu-devel] [PATCH 02/20] pci: move pci_data_{read, write}() declaration from pci.h to pci_host.h

[Isaku Yamahata]

Now pci host stuff has been moved from pci.[hc] to pci_host.[hc]
so the declaration of pci_data_{read, write}() should be in
pci_host.h
This patch moves them from pci.h to pci_host.h for consistency.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.h      |    2 --
 hw/pci_host.h |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index 9a56d0d..d3378d3 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -293,8 +293,6 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr);
 PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
                                const char *default_devaddr);
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
-uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
 int pci_bus_num(PCIBus *s);
 void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
 PCIBus *pci_find_host_bus(int domain);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index e5e877f..7cfa693 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -36,6 +36,9 @@ typedef struct {
     PCIBus *bus;
 } PCIHostState;
 
+void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
+uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
+
 /* for mmio */
 int pci_host_config_register_io_memory(PCIHostState *s);
 int pci_host_config_register_io_memory_noswap(PCIHostState *s);
-- 
1.6.0.2

[Qemu-devel] [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

[Isaku Yamahata]

simplify ugly switch by memcpy trick.
And add one assert().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_host.c  |   16 ++++------------
 hw/pcie_host.c |   16 ++++------------
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index f4518dc..4196ebc 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     uint32_t config_addr = pci_addr_to_config(addr);
     uint32_t val;
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
+        val = 0;
+        memset(&val, 0xff, len);
+        val = le32_to_cpu(val);
     } else {
         val = pci_dev->config_read(pci_dev, config_addr, len);
         PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index b52fec6..61285da 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s,
     PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
     uint32_t val;
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
+        val = 0;
+        memset(&val, 0xff, len);
+        val = le32_to_cpu(val);
     } else {
         val = pci_dev->config_read(pci_dev,
                                    PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
-- 
1.6.0.2

[Qemu-devel] [PATCH 04/20] pci: remove pci_addr_to_config() by open code

[Isaku Yamahata]

This patch removes pci_addr_to_config() and open code it
as suggested by Michael S. Tsirkin <mst@redhat.com>.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_host.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 4196ebc..93c94e8 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -47,15 +47,10 @@ static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
     return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
-static inline uint32_t pci_addr_to_config(uint32_t addr)
-{
-    return addr & (PCI_CONFIG_SPACE_SIZE - 1);
-}
-
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 {
     PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
-    uint32_t config_addr = pci_addr_to_config(addr);
+    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev)
         return;
@@ -68,7 +63,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
-    uint32_t config_addr = pci_addr_to_config(addr);
+    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
     assert(len == 1 || len == 2 || len == 4);
-- 
1.6.0.2

[Qemu-devel] [PATCH 05/20] pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev().

[Isaku Yamahata]

This patch renames pci_addr_to_dev(), pcie_mmcfg_addr_to_dev()
to pci_dev_find_by_addr(), pcie_dev_find_by_mmcfg_addr()
as "Michael S. Tsirkin" <mst@redhat.com> suggested.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_host.c  |    6 +++---
 hw/pcie_host.c |    7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 93c94e8..adecd7e 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -40,7 +40,7 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
  */
 
 /* the helper functio to get a PCIDeice* for a given pci address */
-static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
+static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 {
     uint8_t bus_num = (addr >> 16) & 0xff;
     uint8_t devfn = (addr >> 8) & 0xff;
@@ -49,7 +49,7 @@ static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 {
-    PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
+    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev)
@@ -62,7 +62,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
-    PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
+    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index 61285da..08c3527 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -46,7 +46,8 @@
 
 
 /* a helper function to get a PCIDevice for a given mmconfig address */
-static inline PCIDevice *pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr)
+static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
+                                                     uint32_t mmcfg_addr)
 {
     return pci_find_device(s, PCIE_MMCFG_BUS(mmcfg_addr),
                            PCI_SLOT(PCIE_MMCFG_DEVFN(mmcfg_addr)),
@@ -56,7 +57,7 @@ static inline PCIDevice *pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr)
 static void pcie_mmcfg_data_write(PCIBus *s,
                                   uint32_t mmcfg_addr, uint32_t val, int len)
 {
-    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
+    PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
 
     if (!pci_dev)
         return;
@@ -68,7 +69,7 @@ static void pcie_mmcfg_data_write(PCIBus *s,
 static uint32_t pcie_mmcfg_data_read(PCIBus *s,
                                      uint32_t mmcfg_addr, int len)
 {
-    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
+    PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t val;
 
     assert(len == 1 || len == 2 || len == 4);
-- 
1.6.0.2

[Qemu-devel] [PATCH 06/20] pci: shorten pci_host_{conf, data}_register_xxx function a bit.

[Isaku Yamahata]

pci_host_data_register_io_memory and its variants are too long a bit.
So shorten them. Now they are
pci_host_{conf, data}_register_{mmio, mmio_noswap, ioport}()

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c     |    4 ++--
 hw/grackle_pci.c |    8 ++++----
 hw/pci_host.c    |    8 ++++----
 hw/pci_host.h    |    8 ++++----
 hw/piix_pci.c    |    2 +-
 hw/ppc4xx_pci.c  |    2 +-
 hw/ppce500_pci.c |    4 ++--
 hw/prep_pci.c    |    2 +-
 hw/unin_pci.c    |   16 ++++++++--------
 9 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 3999879..1a16a22 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -235,10 +235,10 @@ static int pci_pbm_init_device(SysBusDevice *dev)
                                           pci_apb_iowrite, s);
     sysbus_init_mmio(dev, 0x10000ULL, pci_ioport);
     /* mem_config  */
-    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
     /* mem_data */
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x10000000ULL, pci_mem_data);
     return 0;
 }
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index f3a8a7d..089d1fb 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -108,8 +108,8 @@ static int pci_grackle_init_device(SysBusDevice *dev)
 
     s = FROM_SYSBUS(GrackleState, dev);
 
-    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
 
@@ -126,8 +126,8 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
 
     s = FROM_SYSBUS(GrackleState, dev);
 
-    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
diff --git a/hw/pci_host.c b/hw/pci_host.c
index adecd7e..cd2ceb7 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -118,7 +118,7 @@ static CPUReadMemoryFunc * const pci_host_config_read[] = {
     &pci_host_config_readl,
 };
 
-int pci_host_config_register_io_memory(PCIHostState *s)
+int pci_host_conf_register_mmio(PCIHostState *s)
 {
     return cpu_register_io_memory(pci_host_config_read,
                                   pci_host_config_write, s);
@@ -158,7 +158,7 @@ static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
     &pci_host_config_readl_noswap,
 };
 
-int pci_host_config_register_io_memory_noswap(PCIHostState *s)
+int pci_host_conf_register_mmio_noswap(PCIHostState *s)
 {
     return cpu_register_io_memory(pci_host_config_read_noswap,
                                   pci_host_config_write_noswap, s);
@@ -182,7 +182,7 @@ static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
     return val;
 }
 
-void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s)
+void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
 {
     register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, s);
     register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
@@ -205,7 +205,7 @@ static CPUReadMemoryFunc * const pci_host_data_read_mmio[] = {
     pci_host_data_readl_mmio,
 };
 
-int pci_host_data_register_io_memory(PCIHostState *s)
+int pci_host_data_register_mmio(PCIHostState *s)
 {
     return cpu_register_io_memory(pci_host_data_read_mmio,
                                   pci_host_data_write_mmio,
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 7cfa693..cf3a339 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -40,12 +40,12 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
 
 /* for mmio */
-int pci_host_config_register_io_memory(PCIHostState *s);
-int pci_host_config_register_io_memory_noswap(PCIHostState *s);
-int pci_host_data_register_io_memory(PCIHostState *s);
+int pci_host_conf_register_mmio(PCIHostState *s);
+int pci_host_conf_register_mmio_noswap(PCIHostState *s);
+int pci_host_data_register_mmio(PCIHostState *s);
 
 /* for ioio */
-void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s);
+void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s);
 void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
 
 #endif /* PCI_HOST_H */
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 5fb7d7b..a44f941 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -180,7 +180,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
 {
     I440FXState *s = FROM_SYSBUS(I440FXState, dev);
 
-    pci_host_config_register_ioport(0xcf8, s);
+    pci_host_conf_register_ioport(0xcf8, s);
 
     pci_host_data_register_ioport(0xcfc, s);
     return 0;
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 3aa7489..2d00b61 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -378,7 +378,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
     cpu_register_physical_memory(config_space + PCIC0_CFGADDR, 4, index);
 
     /* CFGDATA */
-    index = pci_host_data_register_io_memory(&controller->pci_state);
+    index = pci_host_data_register_mmio(&controller->pci_state);
     if (index < 0)
         goto free;
     cpu_register_physical_memory(config_space + PCIC0_CFGDATA, 4, index);
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 223de3a..a72fb86 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -293,13 +293,13 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
     controller->pci_dev = d;
 
     /* CFGADDR */
-    index = pci_host_config_register_io_memory_noswap(&controller->pci_state);
+    index = pci_host_conf_register_mmio_noswap(&controller->pci_state);
     if (index < 0)
         goto free;
     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
 
     /* CFGDATA */
-    index = pci_host_data_register_io_memory(&controller->pci_state);
+    index = pci_host_data_register_mmio(&controller->pci_state);
     if (index < 0)
         goto free;
     cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index a338f81..aceb645 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -127,7 +127,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
     s->bus = pci_register_bus(NULL, "pci",
                               prep_set_irq, prep_map_irq, pic, 0, 4);
 
-    pci_host_config_register_ioport(0xcf8, s);
+    pci_host_conf_register_ioport(0xcf8, s);
 
     pci_host_data_register_ioport(0xcfc, s);
 
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index f089cbd..50d2897 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -84,8 +84,8 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
     /* Uninorth main bus */
     s = FROM_SYSBUS(UNINState, dev);
 
-    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
 
@@ -103,8 +103,8 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
     s = FROM_SYSBUS(UNINState, dev);
 
     // XXX: s = &pci_bridge[2];
-    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
@@ -118,8 +118,8 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
     /* Uninorth AGP bus */
     s = FROM_SYSBUS(UNINState, dev);
 
-    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
@@ -133,8 +133,8 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
     /* Uninorth internal bus */
     s = FROM_SYSBUS(UNINState, dev);
 
-    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
-    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
+    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
+    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
-- 
1.6.0.2

[Qemu-devel] [PATCH 07/20] pci: remove pci_sub_bus() by open coding.

[Isaku Yamahata]

Because pci_sub_bus() is used only once so eliminate it
by open coding as suggested by "Michael S. Tsirkin" <mst@redhat.com>.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 4169d4f..bdd4063 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -239,13 +239,6 @@ int pci_bus_num(PCIBus *s)
     return s->parent_dev->config[PCI_SECONDARY_BUS];
 }
 
-static uint8_t pci_sub_bus(PCIBus *s)
-{
-    if (!s->parent_dev)
-        return 255;     /* pci host bridge */
-    return s->parent_dev->config[PCI_SUBORDINATE_BUS];
-}
-
 static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 {
     PCIDevice *s = container_of(pv, PCIDevice, config);
@@ -1179,7 +1172,10 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 
     /* try child bus */
     QLIST_FOREACH(sec, &bus->child, sibling) {
-        if (pci_bus_num(sec) <= bus_num && bus_num <= pci_sub_bus(sec)) {
+
+        if (!bus->parent_dev /* pci host bridge */
+            || (pci_bus_num(sec) <= bus_num &&
+                bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
             return pci_find_bus(sec, bus_num);
         }
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH 08/20] pci: s/pci_find_host_bus/pci_find_root_bus/g

[Isaku Yamahata]

This patch renames pci_find_host_bus() to pci_find_root_bus()
as suggested by "Michael S. Tsirkin" <mst@redhat.com>.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci-hotplug.c |    4 ++--
 hw/pci.c         |    8 ++++----
 hw/pci.h         |    2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index a254498..081d6d1 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -113,7 +113,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
         if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
             goto err;
         }
-        dev = pci_find_device(pci_find_host_bus(0), pci_bus, slot, 0);
+        dev = pci_find_device(pci_find_root_bus(0), pci_bus, slot, 0);
         if (!dev) {
             monitor_printf(mon, "no pci device with address %s\n", pci_addr);
             goto err;
@@ -257,7 +257,7 @@ void pci_device_hot_remove(Monitor *mon, const char *pci_addr)
         return;
     }
 
-    d = pci_find_device(pci_find_host_bus(0), bus, slot, 0);
+    d = pci_find_device(pci_find_root_bus(0), bus, slot, 0);
     if (!d) {
         monitor_printf(mon, "slot %d empty\n", slot);
         return;
diff --git a/hw/pci.c b/hw/pci.c
index bdd4063..e73f07c 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -146,7 +146,7 @@ static void pci_host_bus_register(int domain, PCIBus *bus)
     QLIST_INSERT_HEAD(&host_buses, host, next);
 }
 
-PCIBus *pci_find_host_bus(int domain)
+PCIBus *pci_find_root_bus(int domain)
 {
     struct PCIHostBus *host;
 
@@ -372,7 +372,7 @@ static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *s
 	return -1;
 
     /* Note: QEMU doesn't implement domains other than 0 */
-    if (!pci_find_bus(pci_find_host_bus(dom), bus))
+    if (!pci_find_bus(pci_find_root_bus(dom), bus))
 	return -1;
 
     *domp = dom;
@@ -402,7 +402,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
 
     if (!devaddr) {
         *devfnp = -1;
-        return pci_find_bus(pci_find_host_bus(0), 0);
+        return pci_find_bus(pci_find_root_bus(0), 0);
     }
 
     if (pci_parse_devaddr(devaddr, &dom, &bus, &slot) < 0) {
@@ -410,7 +410,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
     }
 
     *devfnp = slot << 3;
-    return pci_find_bus(pci_find_host_bus(0), bus);
+    return pci_find_bus(pci_find_root_bus(0), bus);
 }
 
 static void pci_init_cmask(PCIDevice *dev)
diff --git a/hw/pci.h b/hw/pci.h
index d3378d3..cd04189 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -295,7 +295,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
                                const char *default_devaddr);
 int pci_bus_num(PCIBus *s);
 void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
-PCIBus *pci_find_host_bus(int domain);
+PCIBus *pci_find_root_bus(int domain);
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num);
 PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function);
 PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
-- 
1.6.0.2

[Qemu-devel] [PATCH 09/20] pci_host: remove unnecessary & 0xff.

[Isaku Yamahata]

This patch removes unnecessary & 0xff in pci_dev_find_by_addr().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_host.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index cd2ceb7..672d173 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -42,8 +42,9 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
 /* the helper functio to get a PCIDeice* for a given pci address */
 static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 {
-    uint8_t bus_num = (addr >> 16) & 0xff;
-    uint8_t devfn = (addr >> 8) & 0xff;
+    uint8_t bus_num = addr >> 16;
+    uint8_t devfn = addr >> 8;
+
     return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 10/20] pci: kill unnecessary included in pci.c

[Isaku Yamahata]

including pci_host.h isn't needed by pci.c.
This patch kills it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index e73f07c..67818b7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -23,7 +23,6 @@
  */
 #include "hw.h"
 #include "pci.h"
-#include "pci_host.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
-- 
1.6.0.2

[Qemu-devel] [PATCH 11/20] pci: clean up of pci_init_wmask().

[Isaku Yamahata]

This patch replaces for loop by memset in pci_init_wmask().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 67818b7..9698efb 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -426,15 +426,15 @@ static void pci_init_cmask(PCIDevice *dev)
 
 static void pci_init_wmask(PCIDevice *dev)
 {
-    int i;
     int config_size = pci_config_size(dev);
 
     dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
     dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
     pci_set_word(dev->wmask + PCI_COMMAND,
                  PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
-        dev->wmask[i] = 0xff;
+
+    memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
+           config_size - PCI_CONFIG_HEADER_SIZE);
 }
 
 static void pci_init_wmask_bridge(PCIDevice *d)
-- 
1.6.0.2

[Qemu-devel] [PATCH 12/20] pci: remove some unnecessary comment in pci.h

[Isaku Yamahata]

This patch removes some comment which should go into commit log
in pci.h.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.h |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index cd04189..988d2c0 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -382,17 +382,10 @@ typedef struct {
     PCIConfigWriteFunc *config_write;
 
     /* pci config header type */
-    uint8_t header_type;        /* this is necessary for initialization
-                                 * code to know its header type before
-                                 * device specific code can initialize
-                                 * configuration space.
-                                 */
+    uint8_t header_type;
 
     /* pcie stuff */
-    int is_express;   /* is this device pci express?
-                       * initialization code needs to know this before
-                       * each specific device initialization.
-                       */
+    int is_express;   /* is this device pci express? */
 } PCIDeviceInfo;
 
 void pci_qdev_register(PCIDeviceInfo *info);
-- 
1.6.0.2

[Qemu-devel] [PATCH 13/20] pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h.

[Isaku Yamahata]

This patch moves two typedefs, PCIHostState and PCIExpressHost to
qemu-common.h for consistency as PCIBus and PCIDevice are typedefed
in qemu-common.h.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci_host.h  |    4 ++--
 hw/pcie_host.h |    4 ++--
 qemu-common.h  |    2 ++
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/pci_host.h b/hw/pci_host.h
index cf3a339..a006687 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,11 +30,11 @@
 
 #include "sysbus.h"
 
-typedef struct {
+struct PCIHostState {
     SysBusDevice busdev;
     uint32_t config_reg;
     PCIBus *bus;
-} PCIHostState;
+};
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
diff --git a/hw/pcie_host.h b/hw/pcie_host.h
index a7771c9..7754ac9 100644
--- a/hw/pcie_host.h
+++ b/hw/pcie_host.h
@@ -24,7 +24,7 @@
 
 #include "pci_host.h"
 
-typedef struct {
+struct PCIExpressHost {
     PCIHostState pci;
 
     /* express part */
@@ -37,7 +37,7 @@ typedef struct {
 
     /* result of cpu_register_io_memory() to map MMCONFIG area */
     int mmio_index;
-} PCIExpressHost;
+};
 
 int pcie_host_init(PCIExpressHost *e);
 void pcie_host_mmcfg_unmap(PCIExpressHost *e);
diff --git a/qemu-common.h b/qemu-common.h
index b779cfe..8ecac61 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -198,6 +198,8 @@ typedef struct i2c_bus i2c_bus;
 typedef struct i2c_slave i2c_slave;
 typedef struct SMBusDevice SMBusDevice;
 typedef struct QEMUTimer QEMUTimer;
+typedef struct PCIHostState PCIHostState;
+typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIBus PCIBus;
 typedef struct PCIDevice PCIDevice;
 typedef struct SerialState SerialState;
-- 
1.6.0.2

[Qemu-devel] [PATCH 14/20] pci: remove unused constants.

[Isaku Yamahata]

This patch removes unused constants committed by
fb23162885f7fd8cf7334bed22c25ac32c7d8b9d.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.h |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index 988d2c0..72a476e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -101,14 +101,6 @@ typedef struct PCIIORegion {
 #define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
 #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
 #define  PCI_COMMAND_MASTER	0x4	/* Enable bus master */
-#define  PCI_COMMAND_SPECIAL	0x8	/* Enable response to special cycles */
-#define  PCI_COMMAND_INVALIDATE 0x10	/* Use memory write and invalidate */
-#define  PCI_COMMAND_VGA_PALETTE 0x20	/* Enable palette snooping */
-#define  PCI_COMMAND_PARITY	0x40	/* Enable parity checking */
-#define  PCI_COMMAND_WAIT	0x80	/* Enable address/data stepping */
-#define  PCI_COMMAND_SERR	0x100	/* Enable SERR */
-#define  PCI_COMMAND_FAST_BACK	0x200	/* Enable back-to-back writes */
-#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
 #define PCI_STATUS              0x06    /* 16 bits */
 #define PCI_REVISION_ID         0x08    /* 8 bits  */
 #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
@@ -128,7 +120,6 @@ typedef struct PCIIORegion {
 #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
 #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
 #define PCI_SUBORDINATE_BUS	0x1a	/* Highest bus number behind the bridge */
-#define PCI_SEC_LATENCY_TIMER   0x1b    /* Latency timer for secondary interface */
 #define PCI_IO_BASE             0x1c    /* I/O range behind the bridge */
 #define PCI_IO_LIMIT            0x1d
 #define  PCI_IO_RANGE_TYPE_32	0x01
-- 
1.6.0.2

[Qemu-devel] [PATCH 15/20] pci: clean up of pci_update_mappings()

[Isaku Yamahata]

This patch converts r->size == 0 to !r_size.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 9698efb..cae3d53 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -732,7 +732,7 @@ static void pci_update_mappings(PCIDevice *d)
         r = &d->io_regions[i];
 
         /* this region isn't registered */
-        if (r->size == 0)
+        if (!r->size)
             continue;
 
         if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-- 
1.6.0.2

[Qemu-devel] [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Isaku Yamahata]

This patch kills nasty goto in pci_update_mappings().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
 1 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index cae3d53..2eff7fe 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
                     new_addr = pci_get_long(d->config + pci_bar(d, i));
                 }
                 /* the ROM slot has a specific enable bit */
-                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
-                    goto no_mem_map;
-                new_addr = new_addr & ~(r->size - 1);
-                last_addr = new_addr + r->size - 1;
-                /* NOTE: we do not support wrapping */
-                /* XXX: as we cannot support really dynamic
-                   mappings, we handle specific values as invalid
-                   mappings. */
-                if (last_addr <= new_addr || new_addr == 0 ||
-                    last_addr == PCI_BAR_UNMAPPED ||
-
-                    /* Now pcibus_t is 64bit.
-                     * Check if 32 bit BAR wrap around explicitly.
-                     * Without this, PC ide doesn't work well.
-                     * TODO: remove this work around.
-                     */
-                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
-                     last_addr >= UINT32_MAX) ||
-
-                    /*
-                     * OS is allowed to set BAR beyond its addressable
-                     * bits. For example, 32 bit OS can set 64bit bar
-                     * to >4G. Check it.
-                     */
-                    last_addr >= TARGET_PHYS_ADDR_MAX) {
+                if (i == PCI_ROM_SLOT &&
+                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
                     new_addr = PCI_BAR_UNMAPPED;
+                } else {
+                    new_addr = new_addr & ~(r->size - 1);
+                    last_addr = new_addr + r->size - 1;
+                    /* NOTE: we do not support wrapping */
+                    /* XXX: as we cannot support really dynamic
+                       mappings, we handle specific values as invalid
+                       mappings. */
+                    if (last_addr <= new_addr || new_addr == 0 ||
+                        last_addr == PCI_BAR_UNMAPPED ||
+
+                        /* Now pcibus_t is 64bit.
+                         * Check if 32 bit BAR wrap around explicitly.
+                         * Without this, PC ide doesn't work well.
+                         * TODO: remove this work around.
+                         */
+                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
+                         last_addr >= UINT32_MAX) ||
+
+                        /*
+                         * OS is allowed to set BAR beyond its addressable
+                         * bits. For example, 32 bit OS can set 64bit bar
+                         * to >4G. Check it.
+                         */
+                        last_addr >= TARGET_PHYS_ADDR_MAX) {
+                        new_addr = PCI_BAR_UNMAPPED;
+                    }
                 }
             } else {
-            no_mem_map:
                 new_addr = PCI_BAR_UNMAPPED;
             }
         }
-- 
1.6.0.2

[Qemu-devel] [PATCH 17/20] pci: remove magic number, 256 in pci.c

[Isaku Yamahata]

This patch replaces magic number, 256, with ARRAY_SIZE().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 2eff7fe..dce445a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -487,7 +487,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          uint8_t header_type)
 {
     if (devfn < 0) {
-        for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
+        for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
+            devfn += 8) {
             if (!bus->devices[devfn])
                 goto found;
         }
@@ -1025,7 +1026,7 @@ static void pci_for_each_device_under_bus(PCIBus *bus,
     PCIDevice *d;
     int devfn;
 
-    for(devfn = 0; devfn < 256; devfn++) {
+    for(devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
         d = bus->devices[devfn];
         if (d)
             fn(bus, d);
-- 
1.6.0.2

[Qemu-devel] [PATCH 18/20] pci: fix pci_config_get_io_base().

[Isaku Yamahata]

fix typo in pci_config_get_io_base().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index dce445a..d1b884a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -629,7 +629,7 @@ static uint32_t pci_config_get_io_base(PCIDevice *d,
 
     val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
     if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
-        val |= (uint32_t)pci_get_word(d->config + PCI_IO_BASE_UPPER16) << 16;
+        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
     }
     return val;
 }
-- 
1.6.0.2

[Qemu-devel] [PATCH 19/20] pci: pci bridge related clean up.

[Isaku Yamahata]

- fix bridge prefetchable memory accesser to check 64bit or not.
- use pcibus_t consistently instead mixing pcibus_t and uint64_t.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   18 +++++++++++-------
 hw/pci.h |    1 +
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index d1b884a..add919b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -634,19 +634,23 @@ static uint32_t pci_config_get_io_base(PCIDevice *d,
     return val;
 }
 
-static uint64_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
+static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
 {
-    return ((uint64_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
+    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
         << 16;
 }
 
-static uint64_t pci_config_get_pref_base(PCIDevice *d,
+static pcibus_t pci_config_get_pref_base(PCIDevice *d,
                                          uint32_t base, uint32_t upper)
 {
-    uint64_t val;
-    val = ((uint64_t)pci_get_word(d->config + base) &
-           PCI_PREF_RANGE_MASK) << 16;
-    val |= (uint64_t)pci_get_long(d->config + upper) << 32;
+    pcibus_t tmp;
+    pcibus_t val;
+
+    tmp = (pcibus_t)pci_get_word(d->config + base);
+    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
+    if (tmp & PCI_PREF_RANGE_TYPE_64) {
+        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
+    }
     return val;
 }
 
diff --git a/hw/pci.h b/hw/pci.h
index 72a476e..03639b7 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -131,6 +131,7 @@ typedef struct PCIIORegion {
 #define PCI_PREF_MEMORY_BASE    0x24    /* Prefetchable memory range behind */
 #define PCI_PREF_MEMORY_LIMIT   0x26
 #define  PCI_PREF_RANGE_MASK    (~0x0fUL)
+#define  PCI_PREF_RANGE_TYPE_64 0x01
 #define PCI_PREF_BASE_UPPER32   0x28    /* Upper half of prefetchable memory range */
 #define PCI_PREF_LIMIT_UPPER32	0x2c
 #define PCI_SUBSYSTEM_VENDOR_ID 0x2c    /* 16 bits */
-- 
1.6.0.2

[Qemu-devel] [PATCH 20/20] pci: remove goto in pci_bridge_filter().

[Isaku Yamahata]

This patch removes ugly goto in pci_bridge_filter() by
introducing subfunction, pci_bridge_filter_nomap().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index add919b..90bdf5e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -691,6 +691,12 @@ static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
+static void pci_bridge_filter_nomap(pcibus_t *addr, pcibus_t *size)
+{
+    *addr = PCI_BAR_UNMAPPED;
+    *size = 0;
+}
+
 static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
                               uint8_t type)
 {
@@ -703,11 +709,13 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
 
         if (type & PCI_BASE_ADDRESS_SPACE_IO) {
             if (!(cmd & PCI_COMMAND_IO)) {
-                goto no_map;
+                pci_bridge_filter_nomap(addr, size);
+                return;
             }
         } else {
             if (!(cmd & PCI_COMMAND_MEMORY)) {
-                goto no_map;
+                pci_bridge_filter_nomap(addr, size);
+                return;
             }
         }
 
@@ -716,9 +724,7 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
     }
 
     if (base > limit) {
-    no_map:
-        *addr = PCI_BAR_UNMAPPED;
-        *size = 0;
+        pci_bridge_filter_nomap(addr, size);
     } else {
         *addr = base;
         *size = limit - base + 1;
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH 01/20] pci: fix pci_info_device().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:29PM +0900, Isaku Yamahata wrote:
> It printed wrong limit value of bridge.
> This patch fixes it.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2ab1117..4169d4f 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -985,7 +985,7 @@ static void pci_info_device(PCIBus *bus, PCIDevice *d)
>                         base, limit);
>  
>          base = pci_bridge_get_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
> -        limit= pci_config_get_memory_base(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
> +        limit= pci_bridge_get_limit(d, PCI_BASE_ADDRESS_SPACE_MEMORY);
>          monitor_printf(mon,
>                         "      memory range [0x%08"PRIx64", 0x%08"PRIx64"]\n",
>                         base, limit);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 02/20] pci: move pci_data_{read, write}() declaration from pci.h to pci_host.h

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:30PM +0900, Isaku Yamahata wrote:
> Now pci host stuff has been moved from pci.[hc] to pci_host.[hc]
> so the declaration of pci_data_{read, write}() should be in
> pci_host.h
> This patch moves them from pci.h to pci_host.h for consistency.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.h      |    2 --
>  hw/pci_host.h |    3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index 9a56d0d..d3378d3 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -293,8 +293,6 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
>                          const char *default_devaddr);
>  PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
>                                 const char *default_devaddr);
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  int pci_bus_num(PCIBus *s);
>  void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
>  PCIBus *pci_find_host_bus(int domain);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index e5e877f..7cfa693 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -36,6 +36,9 @@ typedef struct {
>      PCIBus *bus;
>  } PCIHostState;
>  
> +void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> +uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> +
>  /* for mmio */
>  int pci_host_config_register_io_memory(PCIHostState *s);
>  int pci_host_config_register_io_memory_noswap(PCIHostState *s);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 11/20] pci: clean up of pci_init_wmask().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:39PM +0900, Isaku Yamahata wrote:
> This patch replaces for loop by memset in pci_init_wmask().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 67818b7..9698efb 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -426,15 +426,15 @@ static void pci_init_cmask(PCIDevice *dev)
>  
>  static void pci_init_wmask(PCIDevice *dev)
>  {
> -    int i;
>      int config_size = pci_config_size(dev);
>  
>      dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
>      dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
>      pci_set_word(dev->wmask + PCI_COMMAND,
>                   PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
> -        dev->wmask[i] = 0xff;
> +
> +    memset(dev->wmask + PCI_CONFIG_HEADER_SIZE, 0xff,
> +           config_size - PCI_CONFIG_HEADER_SIZE);
>  }
>  
>  static void pci_init_wmask_bridge(PCIDevice *d)
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 06/20] pci: shorten pci_host_{conf, data}_register_xxx function a bit.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:34PM +0900, Isaku Yamahata wrote:
> pci_host_data_register_io_memory and its variants are too long a bit.
> So shorten them. Now they are
> pci_host_{conf, data}_register_{mmio, mmio_noswap, ioport}()
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/apb_pci.c     |    4 ++--
>  hw/grackle_pci.c |    8 ++++----
>  hw/pci_host.c    |    8 ++++----
>  hw/pci_host.h    |    8 ++++----
>  hw/piix_pci.c    |    2 +-
>  hw/ppc4xx_pci.c  |    2 +-
>  hw/ppce500_pci.c |    4 ++--
>  hw/prep_pci.c    |    2 +-
>  hw/unin_pci.c    |   16 ++++++++--------
>  9 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 3999879..1a16a22 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -235,10 +235,10 @@ static int pci_pbm_init_device(SysBusDevice *dev)
>                                            pci_apb_iowrite, s);
>      sysbus_init_mmio(dev, 0x10000ULL, pci_ioport);
>      /* mem_config  */
> -    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
>      /* mem_data */
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x10000000ULL, pci_mem_data);
>      return 0;
>  }
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index f3a8a7d..089d1fb 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -108,8 +108,8 @@ static int pci_grackle_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>  
> @@ -126,8 +126,8 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index adecd7e..cd2ceb7 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -118,7 +118,7 @@ static CPUReadMemoryFunc * const pci_host_config_read[] = {
>      &pci_host_config_readl,
>  };
>  
> -int pci_host_config_register_io_memory(PCIHostState *s)
> +int pci_host_conf_register_mmio(PCIHostState *s)
>  {
>      return cpu_register_io_memory(pci_host_config_read,
>                                    pci_host_config_write, s);
> @@ -158,7 +158,7 @@ static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
>      &pci_host_config_readl_noswap,
>  };
>  
> -int pci_host_config_register_io_memory_noswap(PCIHostState *s)
> +int pci_host_conf_register_mmio_noswap(PCIHostState *s)
>  {
>      return cpu_register_io_memory(pci_host_config_read_noswap,
>                                    pci_host_config_write_noswap, s);
> @@ -182,7 +182,7 @@ static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
>      return val;
>  }
>  
> -void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s)
> +void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
>  {
>      register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, s);
>      register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> @@ -205,7 +205,7 @@ static CPUReadMemoryFunc * const pci_host_data_read_mmio[] = {
>      pci_host_data_readl_mmio,
>  };
>  
> -int pci_host_data_register_io_memory(PCIHostState *s)
> +int pci_host_data_register_mmio(PCIHostState *s)
>  {
>      return cpu_register_io_memory(pci_host_data_read_mmio,
>                                    pci_host_data_write_mmio,
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 7cfa693..cf3a339 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -40,12 +40,12 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  
>  /* for mmio */
> -int pci_host_config_register_io_memory(PCIHostState *s);
> -int pci_host_config_register_io_memory_noswap(PCIHostState *s);
> -int pci_host_data_register_io_memory(PCIHostState *s);
> +int pci_host_conf_register_mmio(PCIHostState *s);
> +int pci_host_conf_register_mmio_noswap(PCIHostState *s);
> +int pci_host_data_register_mmio(PCIHostState *s);
>  
>  /* for ioio */
> -void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s);
> +void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  
>  #endif /* PCI_HOST_H */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 5fb7d7b..a44f941 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -180,7 +180,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
>  {
>      I440FXState *s = FROM_SYSBUS(I440FXState, dev);
>  
> -    pci_host_config_register_ioport(0xcf8, s);
> +    pci_host_conf_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>      return 0;
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 3aa7489..2d00b61 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -378,7 +378,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
>      cpu_register_physical_memory(config_space + PCIC0_CFGADDR, 4, index);
>  
>      /* CFGDATA */
> -    index = pci_host_data_register_io_memory(&controller->pci_state);
> +    index = pci_host_data_register_mmio(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(config_space + PCIC0_CFGDATA, 4, index);
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 223de3a..a72fb86 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -293,13 +293,13 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>      controller->pci_dev = d;
>  
>      /* CFGADDR */
> -    index = pci_host_config_register_io_memory_noswap(&controller->pci_state);
> +    index = pci_host_conf_register_mmio_noswap(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
>  
>      /* CFGDATA */
> -    index = pci_host_data_register_io_memory(&controller->pci_state);
> +    index = pci_host_data_register_mmio(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index a338f81..aceb645 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -127,7 +127,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
>      s->bus = pci_register_bus(NULL, "pci",
>                                prep_set_irq, prep_map_irq, pic, 0, 4);
>  
> -    pci_host_config_register_ioport(0xcf8, s);
> +    pci_host_conf_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>  
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index f089cbd..50d2897 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -84,8 +84,8 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>      /* Uninorth main bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>  
> @@ -103,8 +103,8 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>      s = FROM_SYSBUS(UNINState, dev);
>  
>      // XXX: s = &pci_bridge[2];
> -    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> @@ -118,8 +118,8 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
>      /* Uninorth AGP bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> @@ -133,8 +133,8 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
>      /* Uninorth internal bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = pci_host_config_register_io_memory_noswap(&s->host_state);
> -    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> +    pci_mem_config = pci_host_conf_register_mmio_noswap(&s->host_state);
> +    pci_mem_data = pci_host_data_register_mmio(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 09/20] pci_host: remove unnecessary & 0xff.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:37PM +0900, Isaku Yamahata wrote:
> This patch removes unnecessary & 0xff in pci_dev_find_by_addr().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci_host.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index cd2ceb7..672d173 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -42,8 +42,9 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>  /* the helper functio to get a PCIDeice* for a given pci address */
>  static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>  {
> -    uint8_t bus_num = (addr >> 16) & 0xff;
> -    uint8_t devfn = (addr >> 8) & 0xff;
> +    uint8_t bus_num = addr >> 16;
> +    uint8_t devfn = addr >> 8;
> +
>      return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 10/20] pci: kill unnecessary included in pci.c

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:38PM +0900, Isaku Yamahata wrote:
> including pci_host.h isn't needed by pci.c.
> This patch kills it.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index e73f07c..67818b7 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -23,7 +23,6 @@
>   */
>  #include "hw.h"
>  #include "pci.h"
> -#include "pci_host.h"
>  #include "monitor.h"
>  #include "net.h"
>  #include "sysemu.h"
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 12/20] pci: remove some unnecessary comment in pci.h

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:40PM +0900, Isaku Yamahata wrote:
> This patch removes some comment which should go into commit log
> in pci.h.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.h |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index cd04189..988d2c0 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -382,17 +382,10 @@ typedef struct {
>      PCIConfigWriteFunc *config_write;
>  
>      /* pci config header type */
> -    uint8_t header_type;        /* this is necessary for initialization
> -                                 * code to know its header type before
> -                                 * device specific code can initialize
> -                                 * configuration space.
> -                                 */
> +    uint8_t header_type;
>  
>      /* pcie stuff */
> -    int is_express;   /* is this device pci express?
> -                       * initialization code needs to know this before
> -                       * each specific device initialization.
> -                       */
> +    int is_express;   /* is this device pci express? */
>  } PCIDeviceInfo;
>  
>  void pci_qdev_register(PCIDeviceInfo *info);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 13/20] pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:41PM +0900, Isaku Yamahata wrote:
> This patch moves two typedefs, PCIHostState and PCIExpressHost to
> qemu-common.h for consistency as PCIBus and PCIDevice are typedefed
> in qemu-common.h.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci_host.h  |    4 ++--
>  hw/pcie_host.h |    4 ++--
>  qemu-common.h  |    2 ++
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index cf3a339..a006687 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -30,11 +30,11 @@
>  
>  #include "sysbus.h"
>  
> -typedef struct {
> +struct PCIHostState {
>      SysBusDevice busdev;
>      uint32_t config_reg;
>      PCIBus *bus;
> -} PCIHostState;
> +};
>  
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> diff --git a/hw/pcie_host.h b/hw/pcie_host.h
> index a7771c9..7754ac9 100644
> --- a/hw/pcie_host.h
> +++ b/hw/pcie_host.h
> @@ -24,7 +24,7 @@
>  
>  #include "pci_host.h"
>  
> -typedef struct {
> +struct PCIExpressHost {
>      PCIHostState pci;
>  
>      /* express part */
> @@ -37,7 +37,7 @@ typedef struct {
>  
>      /* result of cpu_register_io_memory() to map MMCONFIG area */
>      int mmio_index;
> -} PCIExpressHost;
> +};
>  
>  int pcie_host_init(PCIExpressHost *e);
>  void pcie_host_mmcfg_unmap(PCIExpressHost *e);
> diff --git a/qemu-common.h b/qemu-common.h
> index b779cfe..8ecac61 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -198,6 +198,8 @@ typedef struct i2c_bus i2c_bus;
>  typedef struct i2c_slave i2c_slave;
>  typedef struct SMBusDevice SMBusDevice;
>  typedef struct QEMUTimer QEMUTimer;
> +typedef struct PCIHostState PCIHostState;
> +typedef struct PCIExpressHost PCIExpressHost;
>  typedef struct PCIBus PCIBus;
>  typedef struct PCIDevice PCIDevice;
>  typedef struct SerialState SerialState;
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 14/20] pci: remove unused constants.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:42PM +0900, Isaku Yamahata wrote:
> This patch removes unused constants committed by
> fb23162885f7fd8cf7334bed22c25ac32c7d8b9d.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.h |    9 ---------
>  1 files changed, 0 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index 988d2c0..72a476e 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -101,14 +101,6 @@ typedef struct PCIIORegion {
>  #define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
>  #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
>  #define  PCI_COMMAND_MASTER	0x4	/* Enable bus master */
> -#define  PCI_COMMAND_SPECIAL	0x8	/* Enable response to special cycles */
> -#define  PCI_COMMAND_INVALIDATE 0x10	/* Use memory write and invalidate */
> -#define  PCI_COMMAND_VGA_PALETTE 0x20	/* Enable palette snooping */
> -#define  PCI_COMMAND_PARITY	0x40	/* Enable parity checking */
> -#define  PCI_COMMAND_WAIT	0x80	/* Enable address/data stepping */
> -#define  PCI_COMMAND_SERR	0x100	/* Enable SERR */
> -#define  PCI_COMMAND_FAST_BACK	0x200	/* Enable back-to-back writes */
> -#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
>  #define PCI_STATUS              0x06    /* 16 bits */
>  #define PCI_REVISION_ID         0x08    /* 8 bits  */
>  #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
> @@ -128,7 +120,6 @@ typedef struct PCIIORegion {
>  #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
>  #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
>  #define PCI_SUBORDINATE_BUS	0x1a	/* Highest bus number behind the bridge */
> -#define PCI_SEC_LATENCY_TIMER   0x1b    /* Latency timer for secondary interface */
>  #define PCI_IO_BASE             0x1c    /* I/O range behind the bridge */
>  #define PCI_IO_LIMIT            0x1d
>  #define  PCI_IO_RANGE_TYPE_32	0x01
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 15/20] pci: clean up of pci_update_mappings()

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:43PM +0900, Isaku Yamahata wrote:
> This patch converts r->size == 0 to !r_size.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9698efb..cae3d53 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -732,7 +732,7 @@ static void pci_update_mappings(PCIDevice *d)
>          r = &d->io_regions[i];
>  
>          /* this region isn't registered */
> -        if (r->size == 0)
> +        if (!r->size)
>              continue;
>  
>          if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 17/20] pci: remove magic number, 256 in pci.c

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:45PM +0900, Isaku Yamahata wrote:
> This patch replaces magic number, 256, with ARRAY_SIZE().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2eff7fe..dce445a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -487,7 +487,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           uint8_t header_type)
>  {
>      if (devfn < 0) {
> -        for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> +        for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> +            devfn += 8) {
>              if (!bus->devices[devfn])
>                  goto found;
>          }
> @@ -1025,7 +1026,7 @@ static void pci_for_each_device_under_bus(PCIBus *bus,
>      PCIDevice *d;
>      int devfn;
>  
> -    for(devfn = 0; devfn < 256; devfn++) {
> +    for(devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
>          d = bus->devices[devfn];
>          if (d)
>              fn(bus, d);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 18/20] pci: fix pci_config_get_io_base().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:46PM +0900, Isaku Yamahata wrote:
> fix typo in pci_config_get_io_base().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index dce445a..d1b884a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -629,7 +629,7 @@ static uint32_t pci_config_get_io_base(PCIDevice *d,
>  
>      val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
>      if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
> -        val |= (uint32_t)pci_get_word(d->config + PCI_IO_BASE_UPPER16) << 16;
> +        val |= (uint32_t)pci_get_word(d->config + base_upper16) << 16;
>      }
>      return val;
>  }
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 07/20] pci: remove pci_sub_bus() by open coding.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:35PM +0900, Isaku Yamahata wrote:
> Because pci_sub_bus() is used only once so eliminate it
> by open coding as suggested by "Michael S. Tsirkin" <mst@redhat.com>.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

As a separate note.  I wonder whether we can handle host bridge in a way
that is more uniform with pci bridges, so we don't have to special-case
it in e.g. pci_bus_num.
Not sure yet how to do this or whether this is a good idea at all.

> ---
>  hw/pci.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 4169d4f..bdd4063 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -239,13 +239,6 @@ int pci_bus_num(PCIBus *s)
>      return s->parent_dev->config[PCI_SECONDARY_BUS];
>  }
>  
> -static uint8_t pci_sub_bus(PCIBus *s)
> -{
> -    if (!s->parent_dev)
> -        return 255;     /* pci host bridge */
> -    return s->parent_dev->config[PCI_SUBORDINATE_BUS];
> -}
> -
>  static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>  {
>      PCIDevice *s = container_of(pv, PCIDevice, config);
> @@ -1179,7 +1172,10 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  
>      /* try child bus */
>      QLIST_FOREACH(sec, &bus->child, sibling) {
> -        if (pci_bus_num(sec) <= bus_num && bus_num <= pci_sub_bus(sec)) {
> +
> +        if (!bus->parent_dev /* pci host bridge */
> +            || (pci_bus_num(sec) <= bus_num &&
> +                bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
>              return pci_find_bus(sec, bus_num);
>          }
>      }
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 08/20] pci: s/pci_find_host_bus/pci_find_root_bus/g

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:36PM +0900, Isaku Yamahata wrote:
> This patch renames pci_find_host_bus() to pci_find_root_bus()
> as suggested by "Michael S. Tsirkin" <mst@redhat.com>.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci-hotplug.c |    4 ++--
>  hw/pci.c         |    8 ++++----
>  hw/pci.h         |    2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
> index a254498..081d6d1 100644
> --- a/hw/pci-hotplug.c
> +++ b/hw/pci-hotplug.c
> @@ -113,7 +113,7 @@ void drive_hot_add(Monitor *mon, const QDict *qdict)
>          if (pci_read_devaddr(mon, pci_addr, &dom, &pci_bus, &slot)) {
>              goto err;
>          }
> -        dev = pci_find_device(pci_find_host_bus(0), pci_bus, slot, 0);
> +        dev = pci_find_device(pci_find_root_bus(0), pci_bus, slot, 0);
>          if (!dev) {
>              monitor_printf(mon, "no pci device with address %s\n", pci_addr);
>              goto err;
> @@ -257,7 +257,7 @@ void pci_device_hot_remove(Monitor *mon, const char *pci_addr)
>          return;
>      }
>  
> -    d = pci_find_device(pci_find_host_bus(0), bus, slot, 0);
> +    d = pci_find_device(pci_find_root_bus(0), bus, slot, 0);
>      if (!d) {
>          monitor_printf(mon, "slot %d empty\n", slot);
>          return;
> diff --git a/hw/pci.c b/hw/pci.c
> index bdd4063..e73f07c 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -146,7 +146,7 @@ static void pci_host_bus_register(int domain, PCIBus *bus)
>      QLIST_INSERT_HEAD(&host_buses, host, next);
>  }
>  
> -PCIBus *pci_find_host_bus(int domain)
> +PCIBus *pci_find_root_bus(int domain)
>  {
>      struct PCIHostBus *host;
>  
> @@ -372,7 +372,7 @@ static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *s
>  	return -1;
>  
>      /* Note: QEMU doesn't implement domains other than 0 */
> -    if (!pci_find_bus(pci_find_host_bus(dom), bus))
> +    if (!pci_find_bus(pci_find_root_bus(dom), bus))
>  	return -1;
>  
>      *domp = dom;
> @@ -402,7 +402,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
>  
>      if (!devaddr) {
>          *devfnp = -1;
> -        return pci_find_bus(pci_find_host_bus(0), 0);
> +        return pci_find_bus(pci_find_root_bus(0), 0);
>      }
>  
>      if (pci_parse_devaddr(devaddr, &dom, &bus, &slot) < 0) {
> @@ -410,7 +410,7 @@ PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr)
>      }
>  
>      *devfnp = slot << 3;
> -    return pci_find_bus(pci_find_host_bus(0), bus);
> +    return pci_find_bus(pci_find_root_bus(0), bus);
>  }
>  
>  static void pci_init_cmask(PCIDevice *dev)
> diff --git a/hw/pci.h b/hw/pci.h
> index d3378d3..cd04189 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -295,7 +295,7 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
>                                 const char *default_devaddr);
>  int pci_bus_num(PCIBus *s);
>  void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
> -PCIBus *pci_find_host_bus(int domain);
> +PCIBus *pci_find_root_bus(int domain);
>  PCIBus *pci_find_bus(PCIBus *bus, int bus_num);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function);
>  PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 19/20] pci: pci bridge related clean up.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:47PM +0900, Isaku Yamahata wrote:
> - fix bridge prefetchable memory accesser to check 64bit or not.
> - use pcibus_t consistently instead mixing pcibus_t and uint64_t.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci.c |   18 +++++++++++-------
>  hw/pci.h |    1 +
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d1b884a..add919b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -634,19 +634,23 @@ static uint32_t pci_config_get_io_base(PCIDevice *d,
>      return val;
>  }
>  
> -static uint64_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
> +static pcibus_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
>  {
> -    return ((uint64_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
> +    return ((pcibus_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
>          << 16;
>  }
>  
> -static uint64_t pci_config_get_pref_base(PCIDevice *d,
> +static pcibus_t pci_config_get_pref_base(PCIDevice *d,
>                                           uint32_t base, uint32_t upper)
>  {
> -    uint64_t val;
> -    val = ((uint64_t)pci_get_word(d->config + base) &
> -           PCI_PREF_RANGE_MASK) << 16;
> -    val |= (uint64_t)pci_get_long(d->config + upper) << 32;
> +    pcibus_t tmp;
> +    pcibus_t val;
> +
> +    tmp = (pcibus_t)pci_get_word(d->config + base);
> +    val = (tmp & PCI_PREF_RANGE_MASK) << 16;
> +    if (tmp & PCI_PREF_RANGE_TYPE_64) {
> +        val |= (pcibus_t)pci_get_long(d->config + upper) << 32;
> +    }
>      return val;
>  }
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index 72a476e..03639b7 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -131,6 +131,7 @@ typedef struct PCIIORegion {
>  #define PCI_PREF_MEMORY_BASE    0x24    /* Prefetchable memory range behind */
>  #define PCI_PREF_MEMORY_LIMIT   0x26
>  #define  PCI_PREF_RANGE_MASK    (~0x0fUL)
> +#define  PCI_PREF_RANGE_TYPE_64 0x01
>  #define PCI_PREF_BASE_UPPER32   0x28    /* Upper half of prefetchable memory range */
>  #define PCI_PREF_LIMIT_UPPER32	0x2c
>  #define PCI_SUBSYSTEM_VENDOR_ID 0x2c    /* 16 bits */
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:31PM +0900, Isaku Yamahata wrote:
> simplify ugly switch by memcpy trick.
> And add one assert().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

In fact, there's no reason to be so careful about
zeroing out high bits as far as I can tell:
in the end, the value is assigned to uint32/uint16/uint8
as appropriate and high bits are truncated.
So this can simply be val = 0xffffffff;

> ---
>  hw/pci_host.c  |   16 ++++------------
>  hw/pcie_host.c |   16 ++++------------
>  2 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index f4518dc..4196ebc 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = pci_addr_to_config(addr);
>      uint32_t val;
>  
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> +        val = 0;
> +        memset(&val, 0xff, len);
> +        val = le32_to_cpu(val);


So the above will become simply
if (!pci_dev) {
	return ~0x0;
}


>      } else {
>          val = pci_dev->config_read(pci_dev, config_addr, len);
>          PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> index b52fec6..61285da 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s,
>      PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
>      uint32_t val;
>  
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> +        val = 0;
> +        memset(&val, 0xff, len);
> +        val = le32_to_cpu(val);
>      } else {
>          val = pci_dev->config_read(pci_dev,
>                                     PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 04/20] pci: remove pci_addr_to_config() by open code

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:32PM +0900, Isaku Yamahata wrote:
> This patch removes pci_addr_to_config() and open code it
> as suggested by Michael S. Tsirkin <mst@redhat.com>.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci_host.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 4196ebc..93c94e8 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -47,15 +47,10 @@ static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
>      return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
>  }
>  
> -static inline uint32_t pci_addr_to_config(uint32_t addr)
> -{
> -    return addr & (PCI_CONFIG_SPACE_SIZE - 1);
> -}
> -
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  {
>      PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
> -    uint32_t config_addr = pci_addr_to_config(addr);
> +    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>  
>      if (!pci_dev)
>          return;
> @@ -68,7 +63,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  {
>      PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
> -    uint32_t config_addr = pci_addr_to_config(addr);
> +    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
>  
>      assert(len == 1 || len == 2 || len == 4);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 05/20] pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:33PM +0900, Isaku Yamahata wrote:
> This patch renames pci_addr_to_dev(), pcie_mmcfg_addr_to_dev()
> to pci_dev_find_by_addr(), pcie_dev_find_by_mmcfg_addr()
> as "Michael S. Tsirkin" <mst@redhat.com> suggested.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci_host.c  |    6 +++---
>  hw/pcie_host.c |    7 ++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 93c94e8..adecd7e 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -40,7 +40,7 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>   */
>  
>  /* the helper functio to get a PCIDeice* for a given pci address */
> -static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> +static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>  {
>      uint8_t bus_num = (addr >> 16) & 0xff;
>      uint8_t devfn = (addr >> 8) & 0xff;
> @@ -49,7 +49,7 @@ static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
>  
>  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  {
> -    PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
> +    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>  
>      if (!pci_dev)
> @@ -62,7 +62,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  {
> -    PCIDevice *pci_dev = pci_addr_to_dev(s, addr);
> +    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
>  
> diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> index 61285da..08c3527 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -46,7 +46,8 @@
>  
>  
>  /* a helper function to get a PCIDevice for a given mmconfig address */
> -static inline PCIDevice *pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr)
> +static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
> +                                                     uint32_t mmcfg_addr)
>  {
>      return pci_find_device(s, PCIE_MMCFG_BUS(mmcfg_addr),
>                             PCI_SLOT(PCIE_MMCFG_DEVFN(mmcfg_addr)),
> @@ -56,7 +57,7 @@ static inline PCIDevice *pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr)
>  static void pcie_mmcfg_data_write(PCIBus *s,
>                                    uint32_t mmcfg_addr, uint32_t val, int len)
>  {
> -    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> +    PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>  
>      if (!pci_dev)
>          return;
> @@ -68,7 +69,7 @@ static void pcie_mmcfg_data_write(PCIBus *s,
>  static uint32_t pcie_mmcfg_data_read(PCIBus *s,
>                                       uint32_t mmcfg_addr, int len)
>  {
> -    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> +    PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>      uint32_t val;
>  
>      assert(len == 1 || len == 2 || len == 4);
> -- 
> 1.6.0.2

[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 01:01:09PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:31PM +0900, Isaku Yamahata wrote:
> > simplify ugly switch by memcpy trick.
> > And add one assert().
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> In fact, there's no reason to be so careful about
> zeroing out high bits as far as I can tell:
> in the end, the value is assigned to uint32/uint16/uint8
> as appropriate and high bits are truncated.
> So this can simply be val = 0xffffffff;
> 
> > ---
> >  hw/pci_host.c  |   16 ++++------------
> >  hw/pcie_host.c |   16 ++++------------
> >  2 files changed, 8 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/pci_host.c b/hw/pci_host.c
> > index f4518dc..4196ebc 100644
> > --- a/hw/pci_host.c
> > +++ b/hw/pci_host.c
> > @@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >      uint32_t config_addr = pci_addr_to_config(addr);
> >      uint32_t val;
> >  
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev) {
> > -        switch(len) {
> > -        case 1:
> > -            val = 0xff;
> > -            break;
> > -        case 2:
> > -            val = 0xffff;
> > -            break;
> > -        default:
> > -        case 4:
> > -            val = 0xffffffff;
> > -            break;
> > -        }
> > +        val = 0;
> > +        memset(&val, 0xff, len);
> > +        val = le32_to_cpu(val);
> 
> 
> So the above will become simply
> if (!pci_dev) {
> 	return ~0x0;
> }
> 
> 
> >      } else {
> >          val = pci_dev->config_read(pci_dev, config_addr, len);
> >          PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> > index b52fec6..61285da 100644
> > --- a/hw/pcie_host.c
> > +++ b/hw/pcie_host.c
> > @@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> >      PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> >      uint32_t val;
> >  
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev) {
> > -        switch(len) {
> > -        case 1:
> > -            val = 0xff;
> > -            break;
> > -        case 2:
> > -            val = 0xffff;
> > -            break;
> > -        default:
> > -        case 4:
> > -            val = 0xffffffff;
> > -            break;
> > -        }
> > +        val = 0;
> > +        memset(&val, 0xff, len);
> > +        val = le32_to_cpu(val);
> >      } else {
> >          val = pci_dev->config_read(pci_dev,
> >                                     PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
> > -- 
> > 1.6.0.2

I put this on my tree instead:

--->
pci: simplify (pci_/pcie_mmcfg_)data_read()

Remove switch on length: we don't care about
high bits for value, so just return all ones
if no device.  And add one assert().

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci_host.c b/hw/pci_host.c
index f4518dc..4a29f44 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     uint32_t config_addr = pci_addr_to_config(addr);
     uint32_t val;
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
-    } else {
-        val = pci_dev->config_read(pci_dev, config_addr, len);
-        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                    __func__, pci_dev->name, config_addr, val, len);
+        return ~0x0;
     }
 
+    val = pci_dev->config_read(pci_dev, config_addr, len);
+    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
+                __func__, pci_dev->name, config_addr, val, len);
+
     return val;
 }
 
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index b52fec6..9a5f135 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -65,31 +65,16 @@ static void pcie_mmcfg_data_write(PCIBus *s,
                           PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
 }
 
-static uint32_t pcie_mmcfg_data_read(PCIBus *s,
-                                     uint32_t mmcfg_addr, int len)
+static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
 {
-    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
+    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr);
     uint32_t val;
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
-    } else {
-        val = pci_dev->config_read(pci_dev,
-                                   PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
+        return ~0x0;
     }
-
-    return val;
+    return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
 }
 
 static void pcie_mmcfg_data_writeb(void *opaque,

[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 01:15:25PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 01:01:09PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2009 at 02:58:31PM +0900, Isaku Yamahata wrote:
> > > simplify ugly switch by memcpy trick.
> > > And add one assert().
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > In fact, there's no reason to be so careful about
> > zeroing out high bits as far as I can tell:
> > in the end, the value is assigned to uint32/uint16/uint8
> > as appropriate and high bits are truncated.
> > So this can simply be val = 0xffffffff;
> > 
> > > ---
> > >  hw/pci_host.c  |   16 ++++------------
> > >  hw/pcie_host.c |   16 ++++------------
> > >  2 files changed, 8 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/hw/pci_host.c b/hw/pci_host.c
> > > index f4518dc..4196ebc 100644
> > > --- a/hw/pci_host.c
> > > +++ b/hw/pci_host.c
> > > @@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> > >      uint32_t config_addr = pci_addr_to_config(addr);
> > >      uint32_t val;
> > >  
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev) {
> > > -        switch(len) {
> > > -        case 1:
> > > -            val = 0xff;
> > > -            break;
> > > -        case 2:
> > > -            val = 0xffff;
> > > -            break;
> > > -        default:
> > > -        case 4:
> > > -            val = 0xffffffff;
> > > -            break;
> > > -        }
> > > +        val = 0;
> > > +        memset(&val, 0xff, len);
> > > +        val = le32_to_cpu(val);
> > 
> > 
> > So the above will become simply
> > if (!pci_dev) {
> > 	return ~0x0;
> > }
> > 
> > 
> > >      } else {
> > >          val = pci_dev->config_read(pci_dev, config_addr, len);
> > >          PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > > diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> > > index b52fec6..61285da 100644
> > > --- a/hw/pcie_host.c
> > > +++ b/hw/pcie_host.c
> > > @@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> > >      PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> > >      uint32_t val;
> > >  
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev) {
> > > -        switch(len) {
> > > -        case 1:
> > > -            val = 0xff;
> > > -            break;
> > > -        case 2:
> > > -            val = 0xffff;
> > > -            break;
> > > -        default:
> > > -        case 4:
> > > -            val = 0xffffffff;
> > > -            break;
> > > -        }
> > > +        val = 0;
> > > +        memset(&val, 0xff, len);
> > > +        val = le32_to_cpu(val);
> > >      } else {
> > >          val = pci_dev->config_read(pci_dev,
> > >                                     PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
> > > -- 
> > > 1.6.0.2
> 
> I put this on my tree instead:
> 
> --->
> pci: simplify (pci_/pcie_mmcfg_)data_read()
> 
> Remove switch on length: we don't care about
> high bits for value, so just return all ones
> if no device.  And add one assert().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index f4518dc..4a29f44 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = pci_addr_to_config(addr);
>      uint32_t val;
>  
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> -    } else {
> -        val = pci_dev->config_read(pci_dev, config_addr, len);
> -        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> -                    __func__, pci_dev->name, config_addr, val, len);
> +        return ~0x0;
>      }
>  
> +    val = pci_dev->config_read(pci_dev, config_addr, len);
> +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, pci_dev->name, config_addr, val, len);
> +
>      return val;
>  }
>  
> diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> index b52fec6..9a5f135 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -65,31 +65,16 @@ static void pcie_mmcfg_data_write(PCIBus *s,
>                            PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
>  }
>  
> -static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> -                                     uint32_t mmcfg_addr, int len)
> +static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
>  {
> -    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> +    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr);
>      uint32_t val;

val is now unused, so it can be killed. so I ended up with
the below: ACK?

--->

pci: simplify (pci_/pcie_mmcfg_)data_read()

Remove switch on length: we don't care about
high bits for value, so just return all ones
if no device.  And add one assert().

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci_host.c b/hw/pci_host.c
index f4518dc..4a29f44 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     uint32_t config_addr = pci_addr_to_config(addr);
     uint32_t val;
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
-    } else {
-        val = pci_dev->config_read(pci_dev, config_addr, len);
-        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                    __func__, pci_dev->name, config_addr, val, len);
+        return ~0x0;
     }
 
+    val = pci_dev->config_read(pci_dev, config_addr, len);
+    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
+                __func__, pci_dev->name, config_addr, val, len);
+
     return val;
 }
 
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index b52fec6..1dbc94e 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -65,31 +65,15 @@ static void pcie_mmcfg_data_write(PCIBus *s,
                           PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
 }
 
-static uint32_t pcie_mmcfg_data_read(PCIBus *s,
-                                     uint32_t mmcfg_addr, int len)
+static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
 {
-    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
-    uint32_t val;
+    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr);
 
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
-    } else {
-        val = pci_dev->config_read(pci_dev,
-                                   PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
+        return ~0x0;
     }
-
-    return val;
+    return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
 }
 
 static void pcie_mmcfg_data_writeb(void *opaque,

[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> This patch kills nasty goto in pci_update_mappings().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
>  1 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index cae3d53..2eff7fe 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
>                      new_addr = pci_get_long(d->config + pci_bar(d, i));
>                  }
>                  /* the ROM slot has a specific enable bit */
> -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> -                    goto no_mem_map;
> -                new_addr = new_addr & ~(r->size - 1);
> -                last_addr = new_addr + r->size - 1;
> -                /* NOTE: we do not support wrapping */
> -                /* XXX: as we cannot support really dynamic
> -                   mappings, we handle specific values as invalid
> -                   mappings. */
> -                if (last_addr <= new_addr || new_addr == 0 ||

By the way, any idea why do we need new_addr == 0
and last_addr == PCI_BAR_UNMAPPED?
What would it take to fix these?

> -                    last_addr == PCI_BAR_UNMAPPED ||
> -
> -                    /* Now pcibus_t is 64bit.
> -                     * Check if 32 bit BAR wrap around explicitly.
> -                     * Without this, PC ide doesn't work well.
> -                     * TODO: remove this work around.
> -                     */
> -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> -                     last_addr >= UINT32_MAX) ||
> -
> -                    /*
> -                     * OS is allowed to set BAR beyond its addressable
> -                     * bits. For example, 32 bit OS can set 64bit bar
> -                     * to >4G. Check it.
> -                     */
> -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> +                if (i == PCI_ROM_SLOT &&
> +                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
>                      new_addr = PCI_BAR_UNMAPPED;
> +                } else {
> +                    new_addr = new_addr & ~(r->size - 1);
> +                    last_addr = new_addr + r->size - 1;
> +                    /* NOTE: we do not support wrapping */
> +                    /* XXX: as we cannot support really dynamic
> +                       mappings, we handle specific values as invalid
> +                       mappings. */
> +                    if (last_addr <= new_addr || new_addr == 0 ||
> +                        last_addr == PCI_BAR_UNMAPPED ||
> +
> +                        /* Now pcibus_t is 64bit.
> +                         * Check if 32 bit BAR wrap around explicitly.
> +                         * Without this, PC ide doesn't work well.
> +                         * TODO: remove this work around.
> +                         */
> +                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> +                         last_addr >= UINT32_MAX) ||
> +
> +                        /*
> +                         * OS is allowed to set BAR beyond its addressable
> +                         * bits. For example, 32 bit OS can set 64bit bar
> +                         * to >4G. Check it.
> +                         */
> +                        last_addr >= TARGET_PHYS_ADDR_MAX) {
> +                        new_addr = PCI_BAR_UNMAPPED;
> +                    }
>                  }
>              } else {
> -            no_mem_map:
>                  new_addr = PCI_BAR_UNMAPPED;
>              }
>          }

Nesting is too deep in pci_update_mappings already.
And this makes it worse. I split out the math into
a subfunction, and it looks better IMO:

--->

From: Michael S. Tsirkin <mst@redhat.com>
Subject: pci: split up up pci_update mappings

Split bar address math into a separate function.
In particular, this gets rid of an ugly forward goto
into scope that we have there.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci.c b/hw/pci.c
index 380d43c..847d31e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
     }
 }
 
+static pcibus_t pci_bar_address(PCIDevice *d,
+				int reg, uint8_t type, pcibus_t size)
+{
+    pcibus_t new_addr, last_addr;
+    int bar = pci_bar(d, reg);
+    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
+
+    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
+        if (!(cmd & PCI_COMMAND_IO)) {
+            return PCI_BAR_UNMAPPED;
+        }
+        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
+        last_addr = new_addr + size - 1;
+        /* NOTE: we have only 64K ioports on PC */
+        if (last_addr <= new_addr || new_addr == 0 || last_addr > UINT16_MAX) {
+            return PCI_BAR_UNMAPPED;
+        }
+        return new_addr;
+    }
+
+    if (!(cmd & PCI_COMMAND_MEMORY)) {
+        return PCI_BAR_UNMAPPED;
+    }
+    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+        new_addr = pci_get_quad(d->config + bar);
+    } else {
+        new_addr = pci_get_long(d->config + bar);
+    }
+    /* the ROM slot has a specific enable bit */
+    if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
+        return PCI_BAR_UNMAPPED;
+    }
+    new_addr &= ~(size - 1);
+    last_addr = new_addr + size - 1;
+    /* NOTE: we do not support wrapping */
+    /* XXX: as we cannot support really dynamic
+       mappings, we handle specific values as invalid
+       mappings. */
+    if (last_addr <= new_addr || new_addr == 0 ||
+        last_addr == PCI_BAR_UNMAPPED) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    /* Now pcibus_t is 64bit.
+     * Check if 32 bit BAR wraps around explicitly.
+     * Without this, PC ide doesn't work well.
+     * TODO: remove this work around.
+     */
+    if  (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >= UINT32_MAX) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    /*
+     * OS is allowed to set BAR beyond its addressable
+     * bits. For example, 32 bit OS can set 64bit bar
+     * to >4G. Check it. TODO: we might need to support
+     * it in the future for e.g. PAE.
+     */
+    if (last_addr >= TARGET_PHYS_ADDR_MAX) {
+        return PCI_BAR_UNMAPPED;
+    }
+
+    return new_addr;
+}
+
 static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
-    int cmd, i;
-    pcibus_t last_addr, new_addr;
-    pcibus_t filtered_size;
+    int i;
+    pcibus_t new_addr, filtered_size;
 
-    cmd = pci_get_word(d->config + PCI_COMMAND);
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &d->io_regions[i];
 
@@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
         if (!r->size)
             continue;
 
-        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-            if (cmd & PCI_COMMAND_IO) {
-                new_addr = pci_get_long(d->config + pci_bar(d, i));
-                new_addr = new_addr & ~(r->size - 1);
-                last_addr = new_addr + r->size - 1;
-                /* NOTE: we have only 64K ioports on PC */
-                if (last_addr <= new_addr || new_addr == 0 ||
-                    last_addr >= 0x10000) {
-                    new_addr = PCI_BAR_UNMAPPED;
-                }
-            } else {
-                new_addr = PCI_BAR_UNMAPPED;
-            }
-        } else {
-            if (cmd & PCI_COMMAND_MEMORY) {
-                if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
-                    new_addr = pci_get_quad(d->config + pci_bar(d, i));
-                } else {
-                    new_addr = pci_get_long(d->config + pci_bar(d, i));
-                }
-                /* the ROM slot has a specific enable bit */
-                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
-                    goto no_mem_map;
-                new_addr = new_addr & ~(r->size - 1);
-                last_addr = new_addr + r->size - 1;
-                /* NOTE: we do not support wrapping */
-                /* XXX: as we cannot support really dynamic
-                   mappings, we handle specific values as invalid
-                   mappings. */
-                if (last_addr <= new_addr || new_addr == 0 ||
-                    last_addr == PCI_BAR_UNMAPPED ||
-
-                    /* Now pcibus_t is 64bit.
-                     * Check if 32 bit BAR wrap around explicitly.
-                     * Without this, PC ide doesn't work well.
-                     * TODO: remove this work around.
-                     */
-                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
-                     last_addr >= UINT32_MAX) ||
-
-                    /*
-                     * OS is allowed to set BAR beyond its addressable
-                     * bits. For example, 32 bit OS can set 64bit bar
-                     * to >4G. Check it.
-                     */
-                    last_addr >= TARGET_PHYS_ADDR_MAX) {
-                    new_addr = PCI_BAR_UNMAPPED;
-                }
-            } else {
-            no_mem_map:
-                new_addr = PCI_BAR_UNMAPPED;
-            }
-        }
+        new_addr = pci_bar_address(d, i, r->type, r->size);
 
         /* bridge filtering */
         filtered_size = r->size;

[Qemu-devel] Re: [PATCH 20/20] pci: remove goto in pci_bridge_filter().

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:48PM +0900, Isaku Yamahata wrote:
> This patch removes ugly goto in pci_bridge_filter() by
> introducing subfunction, pci_bridge_filter_nomap().
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

goto on error is actually cleaner IMO.
just *not into scope*.

> ---
>  hw/pci.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index add919b..90bdf5e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -691,6 +691,12 @@ static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
>      return limit;
>  }
>  
> +static void pci_bridge_filter_nomap(pcibus_t *addr, pcibus_t *size)
> +{
> +    *addr = PCI_BAR_UNMAPPED;
> +    *size = 0;
> +}
> +
>  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>                                uint8_t type)
>  {
> @@ -703,11 +709,13 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>  
>          if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>              if (!(cmd & PCI_COMMAND_IO)) {
> -                goto no_map;
> +                pci_bridge_filter_nomap(addr, size);
> +                return;
>              }
>          } else {
>              if (!(cmd & PCI_COMMAND_MEMORY)) {
> -                goto no_map;
> +                pci_bridge_filter_nomap(addr, size);
> +                return;
>              }
>          }
>  
> @@ -716,9 +724,7 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>      }
>  
>      if (base > limit) {
> -    no_map:
> -        *addr = PCI_BAR_UNMAPPED;
> -        *size = 0;
> +        pci_bridge_filter_nomap(addr, size);
>      } else {
>          *addr = base;
>          *size = limit - base + 1;

Here's what I came up with:

--->
From: Michael S. Tsirkin <mst@redhat.com>
Subject: pci: convert goto into scope in bridge_filter
    
goto into scope is evil. rearrange pci_bridge_filter
so that we always go to end of function on error.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci.c b/hw/pci.c
index 14de2d1..6e5d57b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -716,13 +716,14 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
     }
 
     if (base > limit) {
-    no_map:
-        *addr = PCI_BAR_UNMAPPED;
-        *size = 0;
-    } else {
-        *addr = base;
-        *size = limit - base + 1;
+        goto no_map;
     }
+    *addr = base;
+    *size = limit - base + 1;
+    return;
+no_map:
+    *addr = PCI_BAR_UNMAPPED;
+    *size = 0;
 }
 
 static pcibus_t pci_bar_address(PCIDevice *d,

[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

[Isaku Yamahata]

On Thu, Nov 12, 2009 at 02:02:45PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 01:15:25PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2009 at 01:01:09PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 12, 2009 at 02:58:31PM +0900, Isaku Yamahata wrote:
> > > > simplify ugly switch by memcpy trick.
> > > > And add one assert().
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > 
> > > In fact, there's no reason to be so careful about
> > > zeroing out high bits as far as I can tell:
> > > in the end, the value is assigned to uint32/uint16/uint8
> > > as appropriate and high bits are truncated.
> > > So this can simply be val = 0xffffffff;
> > > 
> > > > ---
> > > >  hw/pci_host.c  |   16 ++++------------
> > > >  hw/pcie_host.c |   16 ++++------------
> > > >  2 files changed, 8 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/hw/pci_host.c b/hw/pci_host.c
> > > > index f4518dc..4196ebc 100644
> > > > --- a/hw/pci_host.c
> > > > +++ b/hw/pci_host.c
> > > > @@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> > > >      uint32_t config_addr = pci_addr_to_config(addr);
> > > >      uint32_t val;
> > > >  
> > > > +    assert(len == 1 || len == 2 || len == 4);
> > > >      if (!pci_dev) {
> > > > -        switch(len) {
> > > > -        case 1:
> > > > -            val = 0xff;
> > > > -            break;
> > > > -        case 2:
> > > > -            val = 0xffff;
> > > > -            break;
> > > > -        default:
> > > > -        case 4:
> > > > -            val = 0xffffffff;
> > > > -            break;
> > > > -        }
> > > > +        val = 0;
> > > > +        memset(&val, 0xff, len);
> > > > +        val = le32_to_cpu(val);
> > > 
> > > 
> > > So the above will become simply
> > > if (!pci_dev) {
> > > 	return ~0x0;
> > > }
> > > 
> > > 
> > > >      } else {
> > > >          val = pci_dev->config_read(pci_dev, config_addr, len);
> > > >          PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > > > diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> > > > index b52fec6..61285da 100644
> > > > --- a/hw/pcie_host.c
> > > > +++ b/hw/pcie_host.c
> > > > @@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> > > >      PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> > > >      uint32_t val;
> > > >  
> > > > +    assert(len == 1 || len == 2 || len == 4);
> > > >      if (!pci_dev) {
> > > > -        switch(len) {
> > > > -        case 1:
> > > > -            val = 0xff;
> > > > -            break;
> > > > -        case 2:
> > > > -            val = 0xffff;
> > > > -            break;
> > > > -        default:
> > > > -        case 4:
> > > > -            val = 0xffffffff;
> > > > -            break;
> > > > -        }
> > > > +        val = 0;
> > > > +        memset(&val, 0xff, len);
> > > > +        val = le32_to_cpu(val);
> > > >      } else {
> > > >          val = pci_dev->config_read(pci_dev,
> > > >                                     PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
> > 
> > I put this on my tree instead:
> > 
> > --->
> > pci: simplify (pci_/pcie_mmcfg_)data_read()
> > 
> > Remove switch on length: we don't care about
> > high bits for value, so just return all ones
> > if no device.  And add one assert().
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> > 
> > diff --git a/hw/pci_host.c b/hw/pci_host.c
> > index f4518dc..4a29f44 100644
> > --- a/hw/pci_host.c
> > +++ b/hw/pci_host.c
> > @@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >      uint32_t config_addr = pci_addr_to_config(addr);
> >      uint32_t val;
> >  
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev) {
> > -        switch(len) {
> > -        case 1:
> > -            val = 0xff;
> > -            break;
> > -        case 2:
> > -            val = 0xffff;
> > -            break;
> > -        default:
> > -        case 4:
> > -            val = 0xffffffff;
> > -            break;
> > -        }
> > -    } else {
> > -        val = pci_dev->config_read(pci_dev, config_addr, len);
> > -        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                    __func__, pci_dev->name, config_addr, val, len);
> > +        return ~0x0;
> >      }
> >  
> > +    val = pci_dev->config_read(pci_dev, config_addr, len);
> > +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                __func__, pci_dev->name, config_addr, val, len);
> > +
> >      return val;
> >  }
> >  
> > diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> > index b52fec6..9a5f135 100644
> > --- a/hw/pcie_host.c
> > +++ b/hw/pcie_host.c
> > @@ -65,31 +65,16 @@ static void pcie_mmcfg_data_write(PCIBus *s,
> >                            PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
> >  }
> >  
> > -static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> > -                                     uint32_t mmcfg_addr, int len)
> > +static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
> >  {
> > -    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> > +    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr);
> >      uint32_t val;
> 
> val is now unused, so it can be killed. so I ended up with
> the below: ACK?

Yes.
Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>


> 
> --->
> 
> pci: simplify (pci_/pcie_mmcfg_)data_read()
> 
> Remove switch on length: we don't care about
> high bits for value, so just return all ones
> if no device.  And add one assert().
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index f4518dc..4a29f44 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = pci_addr_to_config(addr);
>      uint32_t val;
>  
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> -    } else {
> -        val = pci_dev->config_read(pci_dev, config_addr, len);
> -        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> -                    __func__, pci_dev->name, config_addr, val, len);
> +        return ~0x0;
>      }
>  
> +    val = pci_dev->config_read(pci_dev, config_addr, len);
> +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, pci_dev->name, config_addr, val, len);
> +
>      return val;
>  }
>  
> diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> index b52fec6..1dbc94e 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -65,31 +65,15 @@ static void pcie_mmcfg_data_write(PCIBus *s,
>                            PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
>  }
>  
> -static uint32_t pcie_mmcfg_data_read(PCIBus *s,
> -                                     uint32_t mmcfg_addr, int len)
> +static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
>  {
> -    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
> -    uint32_t val;
> +    PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr);
>  
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> -    } else {
> -        val = pci_dev->config_read(pci_dev,
> -                                   PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len);
> +        return ~0x0;
>      }
> -
> -    return val;
> +    return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
>  }
>  
>  static void pcie_mmcfg_data_writeb(void *opaque,
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 02/20] pci: move pci_data_{read, write}() declaration from pci.h to pci_host.h

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:30PM +0900, Isaku Yamahata wrote:
> Now pci host stuff has been moved from pci.[hc] to pci_host.[hc]
> so the declaration of pci_data_{read, write}() should be in
> pci_host.h
> This patch moves them from pci.h to pci_host.h for consistency.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.h      |    2 --
>  hw/pci_host.h |    3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.h b/hw/pci.h
> index 9a56d0d..d3378d3 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -293,8 +293,6 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
>                          const char *default_devaddr);
>  PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
>                                 const char *default_devaddr);
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
>  int pci_bus_num(PCIBus *s);
>  void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
>  PCIBus *pci_find_host_bus(int domain);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index e5e877f..7cfa693 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -36,6 +36,9 @@ typedef struct {
>      PCIBus *bus;
>  } PCIHostState;
>  
> +void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> +uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> +
>  /* for mmio */
>  int pci_host_config_register_io_memory(PCIHostState *s);
>  int pci_host_config_register_io_memory_noswap(PCIHostState *s);
> -- 
> 1.6.0.2


This breaks arm build (versatile_pci), ppc build (sh_pci).
I added #include "pci_host.h" there as well.

We end up with the following (this is on top of my cleanup patches):

--->

From: Isaku Yamahata <yamahata@valinux.co.jp>
Subject: pci: move pci_data_{read, write}() declaration from pci.h to pci_host.h
    
Now pci host stuff has been moved from pci.[hc] to pci_host.[hc]
so the declaration of pci_data_{read, write}() should be in
pci_host.h
This patch moves them from pci.h to pci_host.h for consistency.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Acked-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci.h b/hw/pci.h
index c649cfe..686221b 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -293,8 +293,6 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr);
 PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model,
                                const char *default_devaddr);
-void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
-uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
 int pci_bus_num(PCIBus *s);
 void pci_for_each_device(PCIBus *bus, int bus_num, void (*fn)(PCIBus *bus, PCIDevice *d));
 PCIBus *pci_find_host_bus(int domain);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index e5e877f..7cfa693 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -36,6 +36,9 @@ typedef struct {
     PCIBus *bus;
 } PCIHostState;
 
+void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
+uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
+
 /* for mmio */
 int pci_host_config_register_io_memory(PCIHostState *s);
 int pci_host_config_register_io_memory_noswap(PCIHostState *s);
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 52dc02e..abe4c75 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -24,6 +24,7 @@
 #include "hw.h"
 #include "sh.h"
 #include "pci.h"
+#include "pci_host.h"
 #include "sh_pci.h"
 #include "bswap.h"
 
diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
index a0d7d07..153c651 100644
--- a/hw/versatile_pci.c
+++ b/hw/versatile_pci.c
@@ -9,6 +9,7 @@
 
 #include "sysbus.h"
 #include "pci.h"
+#include "pci_host.h"
 
 typedef struct {
     SysBusDevice busdev;

[Qemu-devel] Re: [PATCH 00/20] PCI express clean up patches.

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 02:58:28PM +0900, Isaku Yamahata wrote:
> Here is the patch series to clean up PCI express patches.
> Although there remained some issues to address, the PCI express
> patches was commited while I wasn't responsive last week. (Sorry for that)
> This patch series addresses the remained issues.
> 
> They are mostly trivial fixes or cosmetics suggested by Michael.
> I think I've covered almost all the issues. If I missed anything,
> please be kind to point it out again.

One small comment: please keep patch subject to 50 chars max.
Otherwise it overflows with e.g. git log --oneline.

> Some random comments:
> 
> - PCI configuration space register constant.
>   Now they're defined in pci.h and their symbol name is same to Linux's
>   pci_regs.h.
>   So it would make sense to import Linux pci_regs.h and remove the
>   definitions in pci.h. If this is acceptable, I'll create the patch.

Yes, good idea.

> - PCI configuration space helper functions.
>   Now range checking helper functions are introduced.
>   range_overlaps() and so on.
>   So it's possible to clean up each PCI devices by using them.
>   If acceptable, I'll create the patch.

idem

> - endian swap related to PCI host bridge/guest endian/host endian:
>   I gave up to address this.
>   I'll leave it to someone who knows PPC spec well and has access to
>   a big endian host machine.
> 
> - PCI bridge clean up:
>   PCI bridge stuff needs more clean up. Possibly it would result
>   in a new file pci_bridge.c.
>   I'd like to address it later. Anyway I have to do it when
>   I implement PCI express hot plug.

+1 to pci_bridge.c

> - PCI express initialization:
>   Since it uses PCI initialization code, so it isn't separated
>   from PCI cleanly.
>   One possible way is to introduce PCI express specific qdev
>   register function (PCIEDeviceInfo, pcie_qdev_register() and
>   pcie_qdev_init() which calls pci_qdev_init()).
>   I'm not sure it's worth while at the moment so I'd like to
>   leave it untouched for now.

Right. It's pretty clean IMO, no need to touch just yet.

> thanks,
> 
> Isaku Yamahata (20):
>   pci: fix pci_info_device().
>   pci: move pci_data_{read, write}() declaration from pci.h to
>     pci_host.h
>   pci: simplify pci_data_read(), pcie_mmcfg_data_read().
>   pci: remove pci_addr_to_config() by open code
>   pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev().
>   pci: shorten pci_host_{conf, data}_register_xxx function a bit.
>   pci: remove pci_sub_bus() by open coding.
>   pci: s/pci_find_host_bus/pci_find_root_bus/g
>   pci_host: remove unnecessary & 0xff.
>   pci: kill unnecessary included in pci.c
>   pci: clean up of pci_init_wmask().
>   pci: remove some unnecessary comment in pci.h
>   pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h.
>   pci: remove unused constants.
>   pci: clean up of pci_update_mappings()
>   pci: kill goto in pci_update_mappings()
>   pci: remove magic number, 256 in pci.c
>   pci: fix pci_config_get_io_base().
>   pci: pci bridge related clean up.
>   pci: remove goto in pci_bridge_filter().

I put these on top of my pci patches, and tweaked
the following ones:
   pci: simplify pci_data_read(), pcie_mmcfg_data_read().
   pci: kill goto in pci_update_mappings()
   pci: remove goto in pci_bridge_filter().
   pci: move pci_data_{read, write}() declaration from pci.h to
     pci_host.h

- in the way that I have specified in reply to individual patches.

The combined tree is here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git pci

(or will be in a couple of minutes).

Please take a look, and if this all makes sense, please use them. I'll
be on vacation starting tomorrow, maybe just include this whole tree in
patch series you send out.

-- 
MST

[Qemu-devel] Re: [PATCH 07/20] pci: remove pci_sub_bus() by open coding.

[Isaku Yamahata]

On Thu, Nov 12, 2009 at 12:45:08PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:35PM +0900, Isaku Yamahata wrote:
> > Because pci_sub_bus() is used only once so eliminate it
> > by open coding as suggested by "Michael S. Tsirkin" <mst@redhat.com>.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> As a separate note.  I wonder whether we can handle host bridge in a way
> that is more uniform with pci bridges, so we don't have to special-case
> it in e.g. pci_bus_num.
> Not sure yet how to do this or whether this is a good idea at all.

PCI host device has header type of 0, normal device.
So it doesn't have secondary bus register.

-- 
yamahata

[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Isaku Yamahata]

On Thu, Nov 12, 2009 at 02:06:22PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> > This patch kills nasty goto in pci_update_mappings().
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
> >  1 files changed, 28 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index cae3d53..2eff7fe 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
> >                      new_addr = pci_get_long(d->config + pci_bar(d, i));
> >                  }
> >                  /* the ROM slot has a specific enable bit */
> > -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> > -                    goto no_mem_map;
> > -                new_addr = new_addr & ~(r->size - 1);
> > -                last_addr = new_addr + r->size - 1;
> > -                /* NOTE: we do not support wrapping */
> > -                /* XXX: as we cannot support really dynamic
> > -                   mappings, we handle specific values as invalid
> > -                   mappings. */
> > -                if (last_addr <= new_addr || new_addr == 0 ||
> 
> By the way, any idea why do we need new_addr == 0
> and last_addr == PCI_BAR_UNMAPPED?

As for new_addr == 0, since default BAR value is zero, plural BARs can
overlap with each other. I think qemu can't handle BAR overlap anyway.

As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
around 4GB as discussed before. I observed that guest doesn't boot
without the check. However I didn't track it down further.
Now it's 64bit though.

> What would it take to fix these?
> 
> > -                    last_addr == PCI_BAR_UNMAPPED ||
> > -
> > -                    /* Now pcibus_t is 64bit.
> > -                     * Check if 32 bit BAR wrap around explicitly.
> > -                     * Without this, PC ide doesn't work well.
> > -                     * TODO: remove this work around.
> > -                     */
> > -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > -                     last_addr >= UINT32_MAX) ||
> > -
> > -                    /*
> > -                     * OS is allowed to set BAR beyond its addressable
> > -                     * bits. For example, 32 bit OS can set 64bit bar
> > -                     * to >4G. Check it.
> > -                     */
> > -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> > +                if (i == PCI_ROM_SLOT &&
> > +                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> >                      new_addr = PCI_BAR_UNMAPPED;
> > +                } else {
> > +                    new_addr = new_addr & ~(r->size - 1);
> > +                    last_addr = new_addr + r->size - 1;
> > +                    /* NOTE: we do not support wrapping */
> > +                    /* XXX: as we cannot support really dynamic
> > +                       mappings, we handle specific values as invalid
> > +                       mappings. */
> > +                    if (last_addr <= new_addr || new_addr == 0 ||
> > +                        last_addr == PCI_BAR_UNMAPPED ||
> > +
> > +                        /* Now pcibus_t is 64bit.
> > +                         * Check if 32 bit BAR wrap around explicitly.
> > +                         * Without this, PC ide doesn't work well.
> > +                         * TODO: remove this work around.
> > +                         */
> > +                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > +                         last_addr >= UINT32_MAX) ||
> > +
> > +                        /*
> > +                         * OS is allowed to set BAR beyond its addressable
> > +                         * bits. For example, 32 bit OS can set 64bit bar
> > +                         * to >4G. Check it.
> > +                         */
> > +                        last_addr >= TARGET_PHYS_ADDR_MAX) {
> > +                        new_addr = PCI_BAR_UNMAPPED;
> > +                    }
> >                  }
> >              } else {
> > -            no_mem_map:
> >                  new_addr = PCI_BAR_UNMAPPED;
> >              }
> >          }
> 
> Nesting is too deep in pci_update_mappings already.
> And this makes it worse. I split out the math into
> a subfunction, and it looks better IMO:
> 
> --->
> 
> From: Michael S. Tsirkin <mst@redhat.com>
> Subject: pci: split up up pci_update mappings
> 
> Split bar address math into a separate function.
> In particular, this gets rid of an ugly forward goto
> into scope that we have there.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Makes sense. Much more readable.
Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>


> 
> ---
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 380d43c..847d31e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>      }
>  }
>  
> +static pcibus_t pci_bar_address(PCIDevice *d,
> +				int reg, uint8_t type, pcibus_t size)
> +{
> +    pcibus_t new_addr, last_addr;
> +    int bar = pci_bar(d, reg);
> +    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        if (!(cmd & PCI_COMMAND_IO)) {
> +            return PCI_BAR_UNMAPPED;
> +        }
> +        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> +        last_addr = new_addr + size - 1;
> +        /* NOTE: we have only 64K ioports on PC */
> +        if (last_addr <= new_addr || new_addr == 0 || last_addr > UINT16_MAX) {
> +            return PCI_BAR_UNMAPPED;
> +        }
> +        return new_addr;
> +    }
> +
> +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +        new_addr = pci_get_quad(d->config + bar);
> +    } else {
> +        new_addr = pci_get_long(d->config + bar);
> +    }
> +    /* the ROM slot has a specific enable bit */
> +    if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +    new_addr &= ~(size - 1);
> +    last_addr = new_addr + size - 1;
> +    /* NOTE: we do not support wrapping */
> +    /* XXX: as we cannot support really dynamic
> +       mappings, we handle specific values as invalid
> +       mappings. */
> +    if (last_addr <= new_addr || new_addr == 0 ||
> +        last_addr == PCI_BAR_UNMAPPED) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    /* Now pcibus_t is 64bit.
> +     * Check if 32 bit BAR wraps around explicitly.
> +     * Without this, PC ide doesn't work well.
> +     * TODO: remove this work around.
> +     */
> +    if  (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >= UINT32_MAX) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    /*
> +     * OS is allowed to set BAR beyond its addressable
> +     * bits. For example, 32 bit OS can set 64bit bar
> +     * to >4G. Check it. TODO: we might need to support
> +     * it in the future for e.g. PAE.
> +     */
> +    if (last_addr >= TARGET_PHYS_ADDR_MAX) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    return new_addr;
> +}
> +
>  static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
> -    int cmd, i;
> -    pcibus_t last_addr, new_addr;
> -    pcibus_t filtered_size;
> +    int i;
> +    pcibus_t new_addr, filtered_size;
>  
> -    cmd = pci_get_word(d->config + PCI_COMMAND);
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
>          r = &d->io_regions[i];
>  
> @@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
>          if (!r->size)
>              continue;
>  
> -        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -            if (cmd & PCI_COMMAND_IO) {
> -                new_addr = pci_get_long(d->config + pci_bar(d, i));
> -                new_addr = new_addr & ~(r->size - 1);
> -                last_addr = new_addr + r->size - 1;
> -                /* NOTE: we have only 64K ioports on PC */
> -                if (last_addr <= new_addr || new_addr == 0 ||
> -                    last_addr >= 0x10000) {
> -                    new_addr = PCI_BAR_UNMAPPED;
> -                }
> -            } else {
> -                new_addr = PCI_BAR_UNMAPPED;
> -            }
> -        } else {
> -            if (cmd & PCI_COMMAND_MEMORY) {
> -                if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> -                    new_addr = pci_get_quad(d->config + pci_bar(d, i));
> -                } else {
> -                    new_addr = pci_get_long(d->config + pci_bar(d, i));
> -                }
> -                /* the ROM slot has a specific enable bit */
> -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> -                    goto no_mem_map;
> -                new_addr = new_addr & ~(r->size - 1);
> -                last_addr = new_addr + r->size - 1;
> -                /* NOTE: we do not support wrapping */
> -                /* XXX: as we cannot support really dynamic
> -                   mappings, we handle specific values as invalid
> -                   mappings. */
> -                if (last_addr <= new_addr || new_addr == 0 ||
> -                    last_addr == PCI_BAR_UNMAPPED ||
> -
> -                    /* Now pcibus_t is 64bit.
> -                     * Check if 32 bit BAR wrap around explicitly.
> -                     * Without this, PC ide doesn't work well.
> -                     * TODO: remove this work around.
> -                     */
> -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> -                     last_addr >= UINT32_MAX) ||
> -
> -                    /*
> -                     * OS is allowed to set BAR beyond its addressable
> -                     * bits. For example, 32 bit OS can set 64bit bar
> -                     * to >4G. Check it.
> -                     */
> -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> -                    new_addr = PCI_BAR_UNMAPPED;
> -                }
> -            } else {
> -            no_mem_map:
> -                new_addr = PCI_BAR_UNMAPPED;
> -            }
> -        }
> +        new_addr = pci_bar_address(d, i, r->type, r->size);
>  
>          /* bridge filtering */
>          filtered_size = r->size;
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 10:12:07PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 12, 2009 at 02:06:22PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> > > This patch kills nasty goto in pci_update_mappings().
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
> > >  1 files changed, 28 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index cae3d53..2eff7fe 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
> > >                      new_addr = pci_get_long(d->config + pci_bar(d, i));
> > >                  }
> > >                  /* the ROM slot has a specific enable bit */
> > > -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> > > -                    goto no_mem_map;
> > > -                new_addr = new_addr & ~(r->size - 1);
> > > -                last_addr = new_addr + r->size - 1;
> > > -                /* NOTE: we do not support wrapping */
> > > -                /* XXX: as we cannot support really dynamic
> > > -                   mappings, we handle specific values as invalid
> > > -                   mappings. */
> > > -                if (last_addr <= new_addr || new_addr == 0 ||
> > 
> > By the way, any idea why do we need new_addr == 0
> > and last_addr == PCI_BAR_UNMAPPED?
> 
> As for new_addr == 0, since default BAR value is zero, plural BARs can
> overlap with each other. I think qemu can't handle BAR overlap anyway.

Heh, for that matter, BARs can overlap anyway.
0 just makes it more likely.

> As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
> around 4GB as discussed before. I observed that guest doesn't boot
> without the check. However I didn't track it down further.
> Now it's 64bit though.

Hmm. Would be nice to figure all this out.

> > What would it take to fix these?
> > 
> > > -                    last_addr == PCI_BAR_UNMAPPED ||
> > > -
> > > -                    /* Now pcibus_t is 64bit.
> > > -                     * Check if 32 bit BAR wrap around explicitly.
> > > -                     * Without this, PC ide doesn't work well.
> > > -                     * TODO: remove this work around.
> > > -                     */
> > > -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > > -                     last_addr >= UINT32_MAX) ||
> > > -
> > > -                    /*
> > > -                     * OS is allowed to set BAR beyond its addressable
> > > -                     * bits. For example, 32 bit OS can set 64bit bar
> > > -                     * to >4G. Check it.
> > > -                     */
> > > -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> > > +                if (i == PCI_ROM_SLOT &&
> > > +                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> > >                      new_addr = PCI_BAR_UNMAPPED;
> > > +                } else {
> > > +                    new_addr = new_addr & ~(r->size - 1);
> > > +                    last_addr = new_addr + r->size - 1;
> > > +                    /* NOTE: we do not support wrapping */
> > > +                    /* XXX: as we cannot support really dynamic
> > > +                       mappings, we handle specific values as invalid
> > > +                       mappings. */
> > > +                    if (last_addr <= new_addr || new_addr == 0 ||
> > > +                        last_addr == PCI_BAR_UNMAPPED ||
> > > +
> > > +                        /* Now pcibus_t is 64bit.
> > > +                         * Check if 32 bit BAR wrap around explicitly.
> > > +                         * Without this, PC ide doesn't work well.
> > > +                         * TODO: remove this work around.
> > > +                         */
> > > +                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > > +                         last_addr >= UINT32_MAX) ||
> > > +
> > > +                        /*
> > > +                         * OS is allowed to set BAR beyond its addressable
> > > +                         * bits. For example, 32 bit OS can set 64bit bar
> > > +                         * to >4G. Check it.
> > > +                         */
> > > +                        last_addr >= TARGET_PHYS_ADDR_MAX) {
> > > +                        new_addr = PCI_BAR_UNMAPPED;
> > > +                    }
> > >                  }
> > >              } else {
> > > -            no_mem_map:
> > >                  new_addr = PCI_BAR_UNMAPPED;
> > >              }
> > >          }
> > 
> > Nesting is too deep in pci_update_mappings already.
> > And this makes it worse. I split out the math into
> > a subfunction, and it looks better IMO:
> > 
> > --->
> > 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Subject: pci: split up up pci_update mappings
> > 
> > Split bar address math into a separate function.
> > In particular, this gets rid of an ugly forward goto
> > into scope that we have there.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Makes sense. Much more readable.
> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> 
> > 
> > ---
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 380d43c..847d31e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >      }
> >  }
> >  
> > +static pcibus_t pci_bar_address(PCIDevice *d,
> > +				int reg, uint8_t type, pcibus_t size)
> > +{
> > +    pcibus_t new_addr, last_addr;
> > +    int bar = pci_bar(d, reg);
> > +    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> > +
> > +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> > +        if (!(cmd & PCI_COMMAND_IO)) {
> > +            return PCI_BAR_UNMAPPED;
> > +        }
> > +        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> > +        last_addr = new_addr + size - 1;
> > +        /* NOTE: we have only 64K ioports on PC */
> > +        if (last_addr <= new_addr || new_addr == 0 || last_addr > UINT16_MAX) {
> > +            return PCI_BAR_UNMAPPED;
> > +        }
> > +        return new_addr;
> > +    }
> > +
> > +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> > +        return PCI_BAR_UNMAPPED;
> > +    }
> > +    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > +        new_addr = pci_get_quad(d->config + bar);
> > +    } else {
> > +        new_addr = pci_get_long(d->config + bar);
> > +    }
> > +    /* the ROM slot has a specific enable bit */
> > +    if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> > +        return PCI_BAR_UNMAPPED;
> > +    }
> > +    new_addr &= ~(size - 1);
> > +    last_addr = new_addr + size - 1;
> > +    /* NOTE: we do not support wrapping */
> > +    /* XXX: as we cannot support really dynamic
> > +       mappings, we handle specific values as invalid
> > +       mappings. */
> > +    if (last_addr <= new_addr || new_addr == 0 ||
> > +        last_addr == PCI_BAR_UNMAPPED) {
> > +        return PCI_BAR_UNMAPPED;
> > +    }
> > +
> > +    /* Now pcibus_t is 64bit.
> > +     * Check if 32 bit BAR wraps around explicitly.
> > +     * Without this, PC ide doesn't work well.
> > +     * TODO: remove this work around.
> > +     */
> > +    if  (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >= UINT32_MAX) {
> > +        return PCI_BAR_UNMAPPED;
> > +    }
> > +
> > +    /*
> > +     * OS is allowed to set BAR beyond its addressable
> > +     * bits. For example, 32 bit OS can set 64bit bar
> > +     * to >4G. Check it. TODO: we might need to support
> > +     * it in the future for e.g. PAE.
> > +     */
> > +    if (last_addr >= TARGET_PHYS_ADDR_MAX) {
> > +        return PCI_BAR_UNMAPPED;
> > +    }
> > +
> > +    return new_addr;
> > +}
> > +
> >  static void pci_update_mappings(PCIDevice *d)
> >  {
> >      PCIIORegion *r;
> > -    int cmd, i;
> > -    pcibus_t last_addr, new_addr;
> > -    pcibus_t filtered_size;
> > +    int i;
> > +    pcibus_t new_addr, filtered_size;
> >  
> > -    cmd = pci_get_word(d->config + PCI_COMMAND);
> >      for(i = 0; i < PCI_NUM_REGIONS; i++) {
> >          r = &d->io_regions[i];
> >  
> > @@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
> >          if (!r->size)
> >              continue;
> >  
> > -        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> > -            if (cmd & PCI_COMMAND_IO) {
> > -                new_addr = pci_get_long(d->config + pci_bar(d, i));
> > -                new_addr = new_addr & ~(r->size - 1);
> > -                last_addr = new_addr + r->size - 1;
> > -                /* NOTE: we have only 64K ioports on PC */
> > -                if (last_addr <= new_addr || new_addr == 0 ||
> > -                    last_addr >= 0x10000) {
> > -                    new_addr = PCI_BAR_UNMAPPED;
> > -                }
> > -            } else {
> > -                new_addr = PCI_BAR_UNMAPPED;
> > -            }
> > -        } else {
> > -            if (cmd & PCI_COMMAND_MEMORY) {
> > -                if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > -                    new_addr = pci_get_quad(d->config + pci_bar(d, i));
> > -                } else {
> > -                    new_addr = pci_get_long(d->config + pci_bar(d, i));
> > -                }
> > -                /* the ROM slot has a specific enable bit */
> > -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> > -                    goto no_mem_map;
> > -                new_addr = new_addr & ~(r->size - 1);
> > -                last_addr = new_addr + r->size - 1;
> > -                /* NOTE: we do not support wrapping */
> > -                /* XXX: as we cannot support really dynamic
> > -                   mappings, we handle specific values as invalid
> > -                   mappings. */
> > -                if (last_addr <= new_addr || new_addr == 0 ||
> > -                    last_addr == PCI_BAR_UNMAPPED ||
> > -
> > -                    /* Now pcibus_t is 64bit.
> > -                     * Check if 32 bit BAR wrap around explicitly.
> > -                     * Without this, PC ide doesn't work well.
> > -                     * TODO: remove this work around.
> > -                     */
> > -                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> > -                     last_addr >= UINT32_MAX) ||
> > -
> > -                    /*
> > -                     * OS is allowed to set BAR beyond its addressable
> > -                     * bits. For example, 32 bit OS can set 64bit bar
> > -                     * to >4G. Check it.
> > -                     */
> > -                    last_addr >= TARGET_PHYS_ADDR_MAX) {
> > -                    new_addr = PCI_BAR_UNMAPPED;
> > -                }
> > -            } else {
> > -            no_mem_map:
> > -                new_addr = PCI_BAR_UNMAPPED;
> > -            }
> > -        }
> > +        new_addr = pci_bar_address(d, i, r->type, r->size);
> >  
> >          /* bridge filtering */
> >          filtered_size = r->size;
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH 20/20] pci: remove goto in pci_bridge_filter().

[Isaku Yamahata]

On Thu, Nov 12, 2009 at 02:08:05PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:48PM +0900, Isaku Yamahata wrote:
> > This patch removes ugly goto in pci_bridge_filter() by
> > introducing subfunction, pci_bridge_filter_nomap().
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> goto on error is actually cleaner IMO.
> just *not into scope*.
> 
> > ---
> >  hw/pci.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index add919b..90bdf5e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -691,6 +691,12 @@ static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
> >      return limit;
> >  }
> >  
> > +static void pci_bridge_filter_nomap(pcibus_t *addr, pcibus_t *size)
> > +{
> > +    *addr = PCI_BAR_UNMAPPED;
> > +    *size = 0;
> > +}
> > +
> >  static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >                                uint8_t type)
> >  {
> > @@ -703,11 +709,13 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >  
> >          if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> >              if (!(cmd & PCI_COMMAND_IO)) {
> > -                goto no_map;
> > +                pci_bridge_filter_nomap(addr, size);
> > +                return;
> >              }
> >          } else {
> >              if (!(cmd & PCI_COMMAND_MEMORY)) {
> > -                goto no_map;
> > +                pci_bridge_filter_nomap(addr, size);
> > +                return;
> >              }
> >          }
> >  
> > @@ -716,9 +724,7 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> >      }
> >  
> >      if (base > limit) {
> > -    no_map:
> > -        *addr = PCI_BAR_UNMAPPED;
> > -        *size = 0;
> > +        pci_bridge_filter_nomap(addr, size);
> >      } else {
> >          *addr = base;
> >          *size = limit - base + 1;
> 
> Here's what I came up with:
> 
> --->
> From: Michael S. Tsirkin <mst@redhat.com>
> Subject: pci: convert goto into scope in bridge_filter
>     
> goto into scope is evil. rearrange pci_bridge_filter
> so that we always go to end of function on error.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>


> 
> ---
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 14de2d1..6e5d57b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -716,13 +716,14 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
>      }
>  
>      if (base > limit) {
> -    no_map:
> -        *addr = PCI_BAR_UNMAPPED;
> -        *size = 0;
> -    } else {
> -        *addr = base;
> -        *size = limit - base + 1;
> +        goto no_map;
>      }
> +    *addr = base;
> +    *size = limit - base + 1;
> +    return;
> +no_map:
> +    *addr = PCI_BAR_UNMAPPED;
> +    *size = 0;
>  }
>  
>  static pcibus_t pci_bar_address(PCIDevice *d,
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Michael S. Tsirkin]

On Thu, Nov 12, 2009 at 10:12:07PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 12, 2009 at 02:06:22PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> > > This patch kills nasty goto in pci_update_mappings().
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
> > >  1 files changed, 28 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index cae3d53..2eff7fe 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
> > >                      new_addr = pci_get_long(d->config + pci_bar(d, i));
> > >                  }
> > >                  /* the ROM slot has a specific enable bit */
> > > -                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> > > -                    goto no_mem_map;
> > > -                new_addr = new_addr & ~(r->size - 1);
> > > -                last_addr = new_addr + r->size - 1;
> > > -                /* NOTE: we do not support wrapping */
> > > -                /* XXX: as we cannot support really dynamic
> > > -                   mappings, we handle specific values as invalid
> > > -                   mappings. */
> > > -                if (last_addr <= new_addr || new_addr == 0 ||
> > 
> > By the way, any idea why do we need new_addr == 0
> > and last_addr == PCI_BAR_UNMAPPED?
> 
> As for new_addr == 0, since default BAR value is zero, plural BARs can
> overlap with each other. I think qemu can't handle BAR overlap anyway.
> 
> As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
> around 4GB as discussed before.

So it should be ~0x0ull and not PCI_BAR_UNMAPPED,
PCI_BAR_UNMAPPED could be any value e.g. 0x1 would do
as well.

> I observed that guest doesn't boot
> without the check. However I didn't track it down further.
> Now it's 64bit though.

I really think we need to move mapping regions out of devices
and into pci.c. Then we can finally support overlapping BARs
(by being careful not to map common regions).


-- 
MST