[Qemu-devel] [PATCH 0/9] pci: pcie host and mmcfg support

[Qemu-devel] [PATCH 0/9] pci: pcie host and mmcfg support

[Isaku Yamahata]

This patch series clean ups pci code and then implements generic
routines for PCI express host and mmcfg.
The eventual goal is to implement q35 (or more later) based chipset
emulator and pcie native direct attach. This patch set is preliminary
for it.

Isaku Yamahata (9):
  pci: fix PCI_DPRINTF() wrt variadic macro.
  pci.c: use appropriate PRIs in PCI_DPRINTF().
  pci: define a constant to represent a unmapped bar and use it.
  pci: use uint64_t for bar addr and size instead of uint32_t.
  pci: 64bit bar support.
  pci.c: factor out while(bus) bus->next loop logic into
    pci_find_bus_from().
  pci: factor out the logic to get pci device from address.
  pci_host.h: split non-inline static function in pci_host.h into
    pci_host_c.h
  pci: pcie host and mmcfg support.

 hw/ac97.c                       |    2 +-
 hw/apb_pci.c                    |    2 +-
 hw/cirrus_vga.c                 |    6 +-
 hw/e1000.c                      |   12 +-
 hw/eepro100.c                   |    8 +-
 hw/es1370.c                     |    2 +-
 hw/grackle_pci.c                |    2 +-
 hw/gt64xxx.c                    |    2 +-
 hw/ide.c                        |    4 +-
 hw/lsi53c895a.c                 |    6 +-
 hw/macio.c                      |    2 +-
 hw/msix.c                       |    2 +-
 hw/msix.h                       |    2 +-
 hw/ne2000.c                     |    2 +-
 hw/openpic.c                    |    2 +-
 hw/pci.c                        |  429 +++++++++++++++++++++++++++++---------
 hw/pci.h                        |   47 ++++-
 hw/pci_host.h                   |  104 ++--------
 hw/{pci_host.h => pci_host_c.h} |    8 +-
 hw/pcnet.c                      |    9 +-
 hw/piix_pci.c                   |    2 +-
 hw/ppc4xx_pci.c                 |    2 +-
 hw/ppce500_pci.c                |    2 +-
 hw/prep_pci.c                   |    2 +-
 hw/rtl8139.c                    |    4 +-
 hw/sun4u.c                      |    2 +-
 hw/unin_pci.c                   |    2 +-
 hw/usb-ohci.c                   |    2 +-
 hw/usb-uhci.c                   |    2 +-
 hw/vga.c                        |    2 +-
 hw/virtio-pci.c                 |    2 +-
 hw/vmware_vga.c                 |    4 +-
 hw/wdt_i6300esb.c               |    5 +-
 33 files changed, 437 insertions(+), 249 deletions(-)
 copy hw/{pci_host.h => pci_host_c.h} (96%)

[Qemu-devel] [PATCH 1/9] pci: fix PCI_DPRINTF() wrt variadic macro.

[Isaku Yamahata]

add missing ## in PCI_DPRINTF() to compile.

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 c1c4f06..2963da2 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -29,7 +29,7 @@
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
-# define PCI_DPRINTF(format, ...)       printf(format, __VA_ARGS__)
+# define PCI_DPRINTF(format, ...)       printf(format, ## __VA_ARGS__)
 #else
 # define PCI_DPRINTF(format, ...)       do { } while (0)
 #endif
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 1/9] pci: fix PCI_DPRINTF() wrt variadic macro.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:01PM +0900, Isaku Yamahata wrote:
> add missing ## in PCI_DPRINTF() to compile.
> 
> 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 c1c4f06..2963da2 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -29,7 +29,7 @@
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> -# define PCI_DPRINTF(format, ...)       printf(format, __VA_ARGS__)
> +# define PCI_DPRINTF(format, ...)       printf(format, ## __VA_ARGS__)
>  #else
>  # define PCI_DPRINTF(format, ...)       do { } while (0)
>  #endif
> -- 
> 1.6.0.2
> 
> 

[Qemu-devel] [PATCH 2/9] pci.c: use appropriate PRIs in PCI_DPRINTF().

[Isaku Yamahata]

use appropriate PRIs in PCI_DPRINTF() for portability.

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

diff --git a/hw/pci.c b/hw/pci.c
index 2963da2..916938a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -564,7 +564,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
     int config_addr, bus_num;
 
 #if 0
-    PCI_DPRINTF("pci_data_write: addr=%08x val=%08x len=%d\n",
+    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
                 addr, val, len);
 #endif
     bus_num = (addr >> 16) & 0xff;
@@ -576,7 +576,8 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
     if (!pci_dev)
         return;
     config_addr = addr & 0xff;
-    PCI_DPRINTF("pci_config_write: %s: addr=%02x val=%08x len=%d\n",
+    PCI_DPRINTF("pci_config_write: %s: "
+                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
                 pci_dev->name, config_addr, val, len);
     pci_dev->config_write(pci_dev, config_addr, val, len);
 }
@@ -612,11 +613,12 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
     }
     config_addr = addr & 0xff;
     val = pci_dev->config_read(pci_dev, config_addr, len);
-    PCI_DPRINTF("pci_config_read: %s: addr=%02x val=%08x len=%d\n",
+    PCI_DPRINTF("pci_config_read: %s: "
+                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
                 pci_dev->name, config_addr, val, len);
  the_end:
 #if 0
-    PCI_DPRINTF("pci_data_read: addr=%08x val=%08x len=%d\n",
+    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
                 addr, val, len);
 #endif
     return val;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 2/9] pci.c: use appropriate PRIs in PCI_DPRINTF().

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:02PM +0900, Isaku Yamahata wrote:
> use appropriate PRIs in PCI_DPRINTF() for portability.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

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

> ---
>  hw/pci.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2963da2..916938a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -564,7 +564,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>      int config_addr, bus_num;
>  
>  #if 0
> -    PCI_DPRINTF("pci_data_write: addr=%08x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
>                  addr, val, len);
>  #endif
>      bus_num = (addr >> 16) & 0xff;
> @@ -576,7 +576,8 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>      if (!pci_dev)
>          return;
>      config_addr = addr & 0xff;
> -    PCI_DPRINTF("pci_config_write: %s: addr=%02x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_config_write: %s: "
> +                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
>                  pci_dev->name, config_addr, val, len);
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
> @@ -612,11 +613,12 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
>      }
>      config_addr = addr & 0xff;
>      val = pci_dev->config_read(pci_dev, config_addr, len);
> -    PCI_DPRINTF("pci_config_read: %s: addr=%02x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_config_read: %s: "
> +                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
>                  pci_dev->name, config_addr, val, len);
>   the_end:
>  #if 0
> -    PCI_DPRINTF("pci_data_read: addr=%08x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
>                  addr, val, len);
>  #endif
>      return val;
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 2/9] pci.c: use appropriate PRIs in PCI_DPRINTF().

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:02PM +0900, Isaku Yamahata wrote:
> use appropriate PRIs in PCI_DPRINTF() for portability.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Wait a second. So how about we really switch to
pcibus_t, and do

typedef unsigned long long pcibus_t

Now we can use a clean and portable "%llx"
to print it instead of the ugly PRIs.


> ---
>  hw/pci.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2963da2..916938a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -564,7 +564,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>      int config_addr, bus_num;
>  
>  #if 0
> -    PCI_DPRINTF("pci_data_write: addr=%08x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
>                  addr, val, len);
>  #endif
>      bus_num = (addr >> 16) & 0xff;
> @@ -576,7 +576,8 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>      if (!pci_dev)
>          return;
>      config_addr = addr & 0xff;
> -    PCI_DPRINTF("pci_config_write: %s: addr=%02x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_config_write: %s: "
> +                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
>                  pci_dev->name, config_addr, val, len);
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
> @@ -612,11 +613,12 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
>      }
>      config_addr = addr & 0xff;
>      val = pci_dev->config_read(pci_dev, config_addr, len);
> -    PCI_DPRINTF("pci_config_read: %s: addr=%02x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_config_read: %s: "
> +                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
>                  pci_dev->name, config_addr, val, len);
>   the_end:
>  #if 0
> -    PCI_DPRINTF("pci_data_read: addr=%08x val=%08x len=%d\n",
> +    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
>                  addr, val, len);
>  #endif
>      return val;
> -- 
> 1.6.0.2
> 
> 

[Qemu-devel] [PATCH 3/9] pci: define a constant to represent a unmapped bar and use it.

[Isaku Yamahata]

define a constant to represent a unmapped bar instead of -1 and use it.

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

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 1d800e9..88868b9 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3296,7 +3296,7 @@ static void pci_cirrus_write_config(PCIDevice *d,
     CirrusVGAState *s = &pvs->cirrus_vga;
 
     pci_default_write_config(d, address, val, len);
-    if (s->vga.map_addr && pvs->dev.io_regions[0].addr == -1)
+    if (s->vga.map_addr && pvs->dev.io_regions[0].addr == PCI_BAR_UNMAPPED)
         s->vga.map_addr = 0;
     cirrus_update_memory_access(s);
 }
diff --git a/hw/pci.c b/hw/pci.c
index 916938a..03241d4 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -369,7 +369,7 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
 
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &pci_dev->io_regions[i];
-        if (!r->size || r->addr == -1)
+        if (!r->size || r->addr == PCI_BAR_UNMAPPED)
             continue;
         if (r->type == PCI_ADDRESS_SPACE_IO) {
             isa_unassign_ioport(r->addr, r->size);
@@ -416,7 +416,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 
     r = &pci_dev->io_regions[region_num];
-    r->addr = -1;
+    r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
     r->type = type;
     r->map_func = map_func;
@@ -458,10 +458,10 @@ static void pci_update_mappings(PCIDevice *d)
                     /* NOTE: we have only 64K ioports on PC */
                     if (last_addr <= new_addr || new_addr == 0 ||
                         last_addr >= 0x10000) {
-                        new_addr = -1;
+                        new_addr = PCI_BAR_UNMAPPED;
                     }
                 } else {
-                    new_addr = -1;
+                    new_addr = PCI_BAR_UNMAPPED;
                 }
             } else {
                 if (cmd & PCI_COMMAND_MEMORY) {
@@ -477,17 +477,17 @@ static void pci_update_mappings(PCIDevice *d)
                        mappings, we handle specific values as invalid
                        mappings. */
                     if (last_addr <= new_addr || new_addr == 0 ||
-                        last_addr == -1) {
-                        new_addr = -1;
+                        last_addr == PCI_BAR_UNMAPPED) {
+                        new_addr = PCI_BAR_UNMAPPED;
                     }
                 } else {
                 no_mem_map:
-                    new_addr = -1;
+                    new_addr = PCI_BAR_UNMAPPED;
                 }
             }
             /* now do the real mapping */
             if (new_addr != r->addr) {
-                if (r->addr != -1) {
+                if (r->addr != PCI_BAR_UNMAPPED) {
                     if (r->type & PCI_ADDRESS_SPACE_IO) {
                         int class;
                         /* NOTE: specific hack for IDE in PC case:
@@ -506,7 +506,7 @@ static void pci_update_mappings(PCIDevice *d)
                     }
                 }
                 r->addr = new_addr;
-                if (r->addr != -1) {
+                if (r->addr != PCI_BAR_UNMAPPED) {
                     r->map_func(d, i, r->addr, r->size, r->type);
                 }
             }
diff --git a/hw/pci.h b/hw/pci.h
index 17563ed..9d60c36 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -84,6 +84,7 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
 
 typedef struct PCIIORegion {
     uint32_t addr; /* current PCI mapping address. -1 means not mapped */
+#define PCI_BAR_UNMAPPED        (~(uint32_t)0)
     uint32_t size;
     uint8_t type;
     PCIMapIORegionFunc *map_func;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 3/9] pci: define a constant to represent a unmapped bar and use it.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:03PM +0900, Isaku Yamahata wrote:
> define a constant to represent a unmapped bar instead of -1 and use it.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

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

> ---
>  hw/cirrus_vga.c |    2 +-
>  hw/pci.c        |   18 +++++++++---------
>  hw/pci.h        |    1 +
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 1d800e9..88868b9 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -3296,7 +3296,7 @@ static void pci_cirrus_write_config(PCIDevice *d,
>      CirrusVGAState *s = &pvs->cirrus_vga;
>  
>      pci_default_write_config(d, address, val, len);
> -    if (s->vga.map_addr && pvs->dev.io_regions[0].addr == -1)
> +    if (s->vga.map_addr && pvs->dev.io_regions[0].addr == PCI_BAR_UNMAPPED)
>          s->vga.map_addr = 0;
>      cirrus_update_memory_access(s);
>  }
> diff --git a/hw/pci.c b/hw/pci.c
> index 916938a..03241d4 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -369,7 +369,7 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
>  
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
>          r = &pci_dev->io_regions[i];
> -        if (!r->size || r->addr == -1)
> +        if (!r->size || r->addr == PCI_BAR_UNMAPPED)
>              continue;
>          if (r->type == PCI_ADDRESS_SPACE_IO) {
>              isa_unassign_ioport(r->addr, r->size);
> @@ -416,7 +416,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      }
>  
>      r = &pci_dev->io_regions[region_num];
> -    r->addr = -1;
> +    r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
>      r->type = type;
>      r->map_func = map_func;
> @@ -458,10 +458,10 @@ static void pci_update_mappings(PCIDevice *d)
>                      /* NOTE: we have only 64K ioports on PC */
>                      if (last_addr <= new_addr || new_addr == 0 ||
>                          last_addr >= 0x10000) {
> -                        new_addr = -1;
> +                        new_addr = PCI_BAR_UNMAPPED;
>                      }
>                  } else {
> -                    new_addr = -1;
> +                    new_addr = PCI_BAR_UNMAPPED;
>                  }
>              } else {
>                  if (cmd & PCI_COMMAND_MEMORY) {
> @@ -477,17 +477,17 @@ static void pci_update_mappings(PCIDevice *d)
>                         mappings, we handle specific values as invalid
>                         mappings. */
>                      if (last_addr <= new_addr || new_addr == 0 ||
> -                        last_addr == -1) {
> -                        new_addr = -1;
> +                        last_addr == PCI_BAR_UNMAPPED) {
> +                        new_addr = PCI_BAR_UNMAPPED;
>                      }
>                  } else {
>                  no_mem_map:
> -                    new_addr = -1;
> +                    new_addr = PCI_BAR_UNMAPPED;
>                  }
>              }
>              /* now do the real mapping */
>              if (new_addr != r->addr) {
> -                if (r->addr != -1) {
> +                if (r->addr != PCI_BAR_UNMAPPED) {
>                      if (r->type & PCI_ADDRESS_SPACE_IO) {
>                          int class;
>                          /* NOTE: specific hack for IDE in PC case:
> @@ -506,7 +506,7 @@ static void pci_update_mappings(PCIDevice *d)
>                      }
>                  }
>                  r->addr = new_addr;
> -                if (r->addr != -1) {
> +                if (r->addr != PCI_BAR_UNMAPPED) {
>                      r->map_func(d, i, r->addr, r->size, r->type);
>                  }
>              }
> diff --git a/hw/pci.h b/hw/pci.h
> index 17563ed..9d60c36 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -84,6 +84,7 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  
>  typedef struct PCIIORegion {
>      uint32_t addr; /* current PCI mapping address. -1 means not mapped */
> +#define PCI_BAR_UNMAPPED        (~(uint32_t)0)
>      uint32_t size;
>      uint8_t type;
>      PCIMapIORegionFunc *map_func;
> -- 
> 1.6.0.2
> 
> 

[Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Isaku Yamahata]

This patch is preliminary for 64bit bar.
For 64bit bar support, replace uint32_t with uint64_t for addr/size
to be able to represent 64bit width.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/ac97.c         |    2 +-
 hw/cirrus_vga.c   |    4 ++--
 hw/e1000.c        |   12 +++++++-----
 hw/eepro100.c     |    8 ++++----
 hw/es1370.c       |    2 +-
 hw/ide.c          |    4 ++--
 hw/lsi53c895a.c   |    6 +++---
 hw/macio.c        |    2 +-
 hw/msix.c         |    2 +-
 hw/msix.h         |    2 +-
 hw/ne2000.c       |    2 +-
 hw/openpic.c      |    2 +-
 hw/pci.c          |   24 +++++++++++++++---------
 hw/pci.h          |   10 +++++-----
 hw/pcnet.c        |    9 +++++----
 hw/rtl8139.c      |    4 ++--
 hw/sun4u.c        |    2 +-
 hw/usb-ohci.c     |    2 +-
 hw/usb-uhci.c     |    2 +-
 hw/vga.c          |    2 +-
 hw/virtio-pci.c   |    2 +-
 hw/vmware_vga.c   |    4 ++--
 hw/wdt_i6300esb.c |    5 +++--
 23 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index f18fa52..faf7444 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1267,7 +1267,7 @@ static int ac97_load (QEMUFile *f, void *opaque, int version_id)
 }
 
 static void ac97_map (PCIDevice *pci_dev, int region_num,
-                      uint32_t addr, uint32_t size, int type)
+                      uint64_t addr, uint64_t size, int type)
 {
     PCIAC97LinkState *d = (PCIAC97LinkState *) pci_dev;
     AC97LinkState *s = &d->ac97;
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 88868b9..afb8a2b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3260,7 +3260,7 @@ void isa_cirrus_vga_init(void)
  ***************************************/
 
 static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
-			       uint32_t addr, uint32_t size, int type)
+			       uint64_t addr, uint64_t size, int type)
 {
     CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
 
@@ -3281,7 +3281,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 }
 
 static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
-				uint32_t addr, uint32_t size, int type)
+				uint64_t addr, uint64_t size, int type)
 {
     CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
 
diff --git a/hw/e1000.c b/hw/e1000.c
index 4ac8918..149da98 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -143,10 +143,11 @@ static const char phy_regcap[0x20] = {
 };
 
 static void
-ioport_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
-           uint32_t size, int type)
+ioport_map(PCIDevice *pci_dev, int region_num, uint64_t addr,
+           uint64_t size, int type)
 {
-    DBGOUT(IO, "e1000_ioport_map addr=0x%04x size=0x%08x\n", addr, size);
+    DBGOUT(IO, "e1000_ioport_map addr=0x%04"PRIx64" size=0x%08"PRIx64"\n",
+           addr, size);
 }
 
 static void
@@ -1021,7 +1022,7 @@ static CPUReadMemoryFunc *e1000_mmio_read[] = {
 
 static void
 e1000_mmio_map(PCIDevice *pci_dev, int region_num,
-                uint32_t addr, uint32_t size, int type)
+                uint64_t addr, uint64_t size, int type)
 {
     E1000State *d = (E1000State *)pci_dev;
     int i;
@@ -1031,7 +1032,8 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
     };
 
 
-    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08x 0x%08x\n", addr, size);
+    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n",
+           addr, size);
 
     cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
     qemu_register_coalesced_mmio(addr, excluded_regs[0]);
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 85446ed..795acab 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1345,12 +1345,12 @@ typedef struct PCIEEPRO100State {
 } PCIEEPRO100State;
 
 static void pci_map(PCIDevice * pci_dev, int region_num,
-                    uint32_t addr, uint32_t size, int type)
+                    uint64_t addr, uint64_t size, int type)
 {
     PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
     EEPRO100State *s = &d->eepro100;
 
-    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
+    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
            region_num, addr, size, type);
 
     assert(region_num == 1);
@@ -1419,11 +1419,11 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
 };
 
 static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
-                         uint32_t addr, uint32_t size, int type)
+                         uint64_t addr, uint64_t size, int type)
 {
     PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
 
-    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
+    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
            region_num, addr, size, type);
 
     if (region_num == 0) {
diff --git a/hw/es1370.c b/hw/es1370.c
index 5c9af0e..6a3f3a6 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -913,7 +913,7 @@ static void es1370_adc_callback (void *opaque, int avail)
 }
 
 static void es1370_map (PCIDevice *pci_dev, int region_num,
-                        uint32_t addr, uint32_t size, int type)
+                        uint64_t addr, uint64_t size, int type)
 {
     PCIES1370State *d = (PCIES1370State *) pci_dev;
     ES1370State *s = &d->es1370;
diff --git a/hw/ide.c b/hw/ide.c
index 1e56786..d2f801a 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -2921,7 +2921,7 @@ void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
 static void cmd646_update_irq(PCIIDEState *d);
 
 static void ide_map(PCIDevice *pci_dev, int region_num,
-                    uint32_t addr, uint32_t size, int type)
+                    uint64_t addr, uint64_t size, int type)
 {
     PCIIDEState *d = (PCIIDEState *)pci_dev;
     IDEState *ide_state;
@@ -3152,7 +3152,7 @@ static void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val)
 }
 
 static void bmdma_map(PCIDevice *pci_dev, int region_num,
-                    uint32_t addr, uint32_t size, int type)
+                    uint64_t addr, uint64_t size, int type)
 {
     PCIIDEState *d = (PCIIDEState *)pci_dev;
     int i;
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 516a468..d9ebe6e 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1923,7 +1923,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
 }
 
 static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
-                           uint32_t addr, uint32_t size, int type)
+                           uint64_t addr, uint64_t size, int type)
 {
     LSIState *s = (LSIState *)pci_dev;
 
@@ -1938,7 +1938,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
 }
 
 static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
-                            uint32_t addr, uint32_t size, int type)
+                            uint64_t addr, uint64_t size, int type)
 {
     LSIState *s = (LSIState *)pci_dev;
 
@@ -1948,7 +1948,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
 }
 
 static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
-                             uint32_t addr, uint32_t size, int type)
+                             uint64_t addr, uint64_t size, int type)
 {
     LSIState *s = (LSIState *)pci_dev;
 
diff --git a/hw/macio.c b/hw/macio.c
index 8cfadfc..41412c3 100644
--- a/hw/macio.c
+++ b/hw/macio.c
@@ -40,7 +40,7 @@ struct macio_state_t {
 };
 
 static void macio_map (PCIDevice *pci_dev, int region_num,
-                       uint32_t addr, uint32_t size, int type)
+                       uint64_t addr, uint64_t size, int type)
 {
     macio_state_t *macio_state;
     int i;
diff --git a/hw/msix.c b/hw/msix.c
index 3420ce9..6e7cd7b 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -204,7 +204,7 @@ static CPUReadMemoryFunc *msix_mmio_read[] = {
 
 /* Should be called from device's map method. */
 void msix_mmio_map(PCIDevice *d, int region_num,
-                   uint32_t addr, uint32_t size, int type)
+                   uint64_t addr, uint64_t size, int type)
 {
     uint8_t *config = d->config + d->msix_cap;
     uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
diff --git a/hw/msix.h b/hw/msix.h
index 3427778..99f1647 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -10,7 +10,7 @@ void msix_write_config(PCIDevice *pci_dev, uint32_t address,
                        uint32_t val, int len);
 
 void msix_mmio_map(PCIDevice *pci_dev, int region_num,
-                   uint32_t addr, uint32_t size, int type);
+                   uint64_t addr, uint64_t size, int type);
 
 int msix_uninit(PCIDevice *d);
 
diff --git a/hw/ne2000.c b/hw/ne2000.c
index b9c018a..726d98a 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -777,7 +777,7 @@ typedef struct PCINE2000State {
 } PCINE2000State;
 
 static void ne2000_map(PCIDevice *pci_dev, int region_num,
-                       uint32_t addr, uint32_t size, int type)
+                       uint64_t addr, uint64_t size, int type)
 {
     PCINE2000State *d = (PCINE2000State *)pci_dev;
     NE2000State *s = &d->ne2000;
diff --git a/hw/openpic.c b/hw/openpic.c
index baa7ecc..276377e 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -1026,7 +1026,7 @@ static CPUReadMemoryFunc *openpic_read[] = {
 };
 
 static void openpic_map(PCIDevice *pci_dev, int region_num,
-                        uint32_t addr, uint32_t size, int type)
+                        uint64_t addr, uint64_t size, int type)
 {
     openpic_t *opp;
 
diff --git a/hw/pci.c b/hw/pci.c
index 03241d4..2d985f0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -399,19 +399,19 @@ int pci_unregister_device(PCIDevice *pci_dev)
 }
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            uint32_t size, int type,
+                            uint64_t size, int type,
                             PCIMapIORegionFunc *map_func)
 {
     PCIIORegion *r;
     uint32_t addr;
-    uint32_t wmask;
+    uint64_t wmask;
 
     if ((unsigned int)region_num >= PCI_NUM_REGIONS)
         return;
 
     if (size & (size-1)) {
         fprintf(stderr, "ERROR: PCI region size must be pow2 "
-                    "type=0x%x, size=0x%x\n", type, size);
+                    "type=0x%x, size=0x%"PRIx64"\n", type, size);
         exit(1);
     }
 
@@ -430,7 +430,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
         addr = 0x10 + region_num * 4;
     }
     *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
-    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask);
+    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
     *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
 }
 
@@ -438,7 +438,8 @@ static void pci_update_mappings(PCIDevice *d)
 {
     PCIIORegion *r;
     int cmd, i;
-    uint32_t last_addr, new_addr, config_ofs;
+    uint64_t last_addr, new_addr;
+    uint32_t config_ofs;
 
     cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -477,7 +478,11 @@ static void pci_update_mappings(PCIDevice *d)
                        mappings, we handle specific values as invalid
                        mappings. */
                     if (last_addr <= new_addr || new_addr == 0 ||
-                        last_addr == PCI_BAR_UNMAPPED) {
+                        last_addr == PCI_BAR_UNMAPPED ||
+
+                        /* keep old behaviour
+                         * without this, PC ide doesn't work well. */
+                        last_addr >= UINT32_MAX) {
                         new_addr = PCI_BAR_UNMAPPED;
                     }
                 } else {
@@ -733,10 +738,10 @@ static void pci_info_device(PCIDevice *d)
         if (r->size != 0) {
             monitor_printf(mon, "      BAR%d: ", i);
             if (r->type & PCI_ADDRESS_SPACE_IO) {
-                monitor_printf(mon, "I/O at 0x%04x [0x%04x].\n",
+                monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
                                r->addr, r->addr + r->size - 1);
             } else {
-                monitor_printf(mon, "32 bit memory at 0x%08x [0x%08x].\n",
+                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
                                r->addr, r->addr + r->size - 1);
             }
         }
@@ -1044,7 +1049,8 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
         r = &d->io_regions[i];
         if (!r->size)
             continue;
-        monitor_printf(mon, "%*sbar %d: %s at 0x%x [0x%x]\n", indent, "",
+        monitor_printf(mon, "%*sbar %d: %s at 0x%"PRIx64" [0x%"PRIx64"]\n",
+                       indent, "",
                        i, r->type & PCI_ADDRESS_SPACE_IO ? "i/o" : "mem",
                        r->addr, r->addr + r->size - 1);
     }
diff --git a/hw/pci.h b/hw/pci.h
index 9d60c36..fee9ed6 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -75,7 +75,7 @@ typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
 typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
                                    uint32_t address, int len);
 typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
-                                uint32_t addr, uint32_t size, int type);
+                                uint64_t addr, uint64_t size, int type);
 typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
 
 #define PCI_ADDRESS_SPACE_MEM		0x00
@@ -83,9 +83,9 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
 #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
 
 typedef struct PCIIORegion {
-    uint32_t addr; /* current PCI mapping address. -1 means not mapped */
-#define PCI_BAR_UNMAPPED        (~(uint32_t)0)
-    uint32_t size;
+    uint64_t addr; /* current PCI mapping address. -1 means not mapped */
+#define PCI_BAR_UNMAPPED        (~(uint64_t)0)
+    uint64_t size;
     uint8_t type;
     PCIMapIORegionFunc *map_func;
 } PCIIORegion;
@@ -219,7 +219,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
 int pci_unregister_device(PCIDevice *pci_dev);
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            uint32_t size, int type,
+                            uint64_t size, int type,
                             PCIMapIORegionFunc *map_func);
 
 int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
diff --git a/hw/pcnet.c b/hw/pcnet.c
index 22ab6be..e039d9f 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1762,12 +1762,13 @@ static uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
 }
 
 static void pcnet_ioport_map(PCIDevice *pci_dev, int region_num,
-                             uint32_t addr, uint32_t size, int type)
+                             uint64_t addr, uint64_t size, int type)
 {
     PCNetState *d = &((PCIPCNetState *)pci_dev)->state;
 
 #ifdef PCNET_DEBUG_IO
-    printf("pcnet_ioport_map addr=0x%04x size=0x%04x\n", addr, size);
+    printf("pcnet_ioport_map addr=0x%04"PRIx64" size=0x%04"PRIx64"\n",
+           addr, size);
 #endif
 
     register_ioport_write(addr, 16, 1, pcnet_aprom_writeb, d);
@@ -1976,12 +1977,12 @@ static CPUReadMemoryFunc *pcnet_mmio_read[] = {
 };
 
 static void pcnet_mmio_map(PCIDevice *pci_dev, int region_num,
-                            uint32_t addr, uint32_t size, int type)
+                            uint64_t addr, uint64_t size, int type)
 {
     PCIPCNetState *d = (PCIPCNetState *)pci_dev;
 
 #ifdef PCNET_DEBUG_IO
-    printf("pcnet_mmio_map addr=0x%08x 0x%08x\n", addr, size);
+    printf("pcnet_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n", addr, size);
 #endif
 
     cpu_register_physical_memory(addr, PCNET_PNPMMIO_SIZE, d->state.mmio_index);
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 91165db..6b8461c 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3329,7 +3329,7 @@ typedef struct PCIRTL8139State {
 } PCIRTL8139State;
 
 static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
-                       uint32_t addr, uint32_t size, int type)
+                       uint64_t addr, uint64_t size, int type)
 {
     PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
     RTL8139State *s = &d->rtl8139;
@@ -3338,7 +3338,7 @@ static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
 }
 
 static void rtl8139_ioport_map(PCIDevice *pci_dev, int region_num,
-                       uint32_t addr, uint32_t size, int type)
+                       uint64_t addr, uint64_t size, int type)
 {
     PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
     RTL8139State *s = &d->rtl8139;
diff --git a/hw/sun4u.c b/hw/sun4u.c
index e08ae23..eb7737f 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -288,7 +288,7 @@ static const int parallel_irq[MAX_PARALLEL_PORTS] = { 7, 7, 7 };
 static fdctrl_t *floppy_controller;
 
 static void ebus_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
-                              uint32_t addr, uint32_t size, int type)
+                              uint64_t addr, uint64_t size, int type)
 {
     DPRINTF("Mapping region %d registers at %08x\n", region_num, addr);
     switch (region_num) {
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 83d1a5c..f94f57a 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1705,7 +1705,7 @@ typedef struct {
 } OHCIPCIState;
 
 static void ohci_mapfunc(PCIDevice *pci_dev, int i,
-            uint32_t addr, uint32_t size, int type)
+            uint64_t addr, uint64_t size, int type)
 {
     OHCIPCIState *ohci = (OHCIPCIState *)pci_dev;
     cpu_register_physical_memory(addr, size, ohci->state.mem);
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 7b74207..5ffe612 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -1058,7 +1058,7 @@ static void uhci_frame_timer(void *opaque)
 }
 
 static void uhci_map(PCIDevice *pci_dev, int region_num,
-                    uint32_t addr, uint32_t size, int type)
+                    uint64_t addr, uint64_t size, int type)
 {
     UHCIState *s = (UHCIState *)pci_dev;
 
diff --git a/hw/vga.c b/hw/vga.c
index e1a470d..6e0d61a 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2236,7 +2236,7 @@ void vga_dirty_log_start(VGAState *s)
 }
 
 static void vga_map(PCIDevice *pci_dev, int region_num,
-                    uint32_t addr, uint32_t size, int type)
+                    uint64_t addr, uint64_t size, int type)
 {
     PCIVGAState *d = (PCIVGAState *)pci_dev;
     VGAState *s = &d->vga_state;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index d605b5f..f2fe42e 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -344,7 +344,7 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val)
 }
 
 static void virtio_map(PCIDevice *pci_dev, int region_num,
-                       uint32_t addr, uint32_t size, int type)
+                       uint64_t addr, uint64_t size, int type)
 {
     VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = proxy->vdev;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5ceebf1..97248a7 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1173,7 +1173,7 @@ static int pci_vmsvga_load(QEMUFile *f, void *opaque, int version_id)
 }
 
 static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
-                uint32_t addr, uint32_t size, int type)
+                uint64_t addr, uint64_t size, int type)
 {
     struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
     struct vmsvga_state_s *s = &d->chip;
@@ -1193,7 +1193,7 @@ static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
 }
 
 static void pci_vmsvga_map_mem(PCIDevice *pci_dev, int region_num,
-                uint32_t addr, uint32_t size, int type)
+                uint64_t addr, uint64_t size, int type)
 {
     struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
     struct vmsvga_state_s *s = &d->chip;
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 42642c7..678eb17 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -351,7 +351,7 @@ static void i6300esb_mem_writel(void *vp, target_phys_addr_t addr, uint32_t val)
 }
 
 static void i6300esb_map(PCIDevice *dev, int region_num,
-                         uint32_t addr, uint32_t size, int type)
+                         uint64_t addr, uint64_t size, int type)
 {
     static CPUReadMemoryFunc *mem_read[3] = {
         i6300esb_mem_readb,
@@ -366,7 +366,8 @@ static void i6300esb_map(PCIDevice *dev, int region_num,
     I6300State *d = (I6300State *) dev;
     int io_mem;
 
-    i6300esb_debug("addr = %x, size = %x, type = %d\n", addr, size, type);
+    i6300esb_debug("addr = %"PRIx64", size = %"PRIx64", type = %d\n",
+                   addr, size, type);
 
     io_mem = cpu_register_io_memory(mem_read, mem_write, d);
     cpu_register_physical_memory (addr, 0x10, io_mem);
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> This patch is preliminary for 64bit bar.
> For 64bit bar support, replace uint32_t with uint64_t for addr/size
> to be able to represent 64bit width.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

As long as we are touching all code: what do you think about
typedef uint64_t pcibus_t;
so that will be easier to find later?

> ---
>  hw/ac97.c         |    2 +-
>  hw/cirrus_vga.c   |    4 ++--
>  hw/e1000.c        |   12 +++++++-----
>  hw/eepro100.c     |    8 ++++----
>  hw/es1370.c       |    2 +-
>  hw/ide.c          |    4 ++--
>  hw/lsi53c895a.c   |    6 +++---
>  hw/macio.c        |    2 +-
>  hw/msix.c         |    2 +-
>  hw/msix.h         |    2 +-
>  hw/ne2000.c       |    2 +-
>  hw/openpic.c      |    2 +-
>  hw/pci.c          |   24 +++++++++++++++---------
>  hw/pci.h          |   10 +++++-----
>  hw/pcnet.c        |    9 +++++----
>  hw/rtl8139.c      |    4 ++--
>  hw/sun4u.c        |    2 +-
>  hw/usb-ohci.c     |    2 +-
>  hw/usb-uhci.c     |    2 +-
>  hw/vga.c          |    2 +-
>  hw/virtio-pci.c   |    2 +-
>  hw/vmware_vga.c   |    4 ++--
>  hw/wdt_i6300esb.c |    5 +++--
>  23 files changed, 62 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/ac97.c b/hw/ac97.c
> index f18fa52..faf7444 100644
> --- a/hw/ac97.c
> +++ b/hw/ac97.c
> @@ -1267,7 +1267,7 @@ static int ac97_load (QEMUFile *f, void *opaque, int version_id)
>  }
>  
>  static void ac97_map (PCIDevice *pci_dev, int region_num,
> -                      uint32_t addr, uint32_t size, int type)
> +                      uint64_t addr, uint64_t size, int type)
>  {
>      PCIAC97LinkState *d = (PCIAC97LinkState *) pci_dev;
>      AC97LinkState *s = &d->ac97;
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 88868b9..afb8a2b 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -3260,7 +3260,7 @@ void isa_cirrus_vga_init(void)
>   ***************************************/
>  
>  static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
> -			       uint32_t addr, uint32_t size, int type)
> +			       uint64_t addr, uint64_t size, int type)
>  {
>      CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
>  
> @@ -3281,7 +3281,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
>  }
>  
>  static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
> -				uint32_t addr, uint32_t size, int type)
> +				uint64_t addr, uint64_t size, int type)
>  {
>      CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
>  
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 4ac8918..149da98 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -143,10 +143,11 @@ static const char phy_regcap[0x20] = {
>  };
>  
>  static void
> -ioport_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
> -           uint32_t size, int type)
> +ioport_map(PCIDevice *pci_dev, int region_num, uint64_t addr,
> +           uint64_t size, int type)
>  {
> -    DBGOUT(IO, "e1000_ioport_map addr=0x%04x size=0x%08x\n", addr, size);
> +    DBGOUT(IO, "e1000_ioport_map addr=0x%04"PRIx64" size=0x%08"PRIx64"\n",
> +           addr, size);
>  }
>  
>  static void
> @@ -1021,7 +1022,7 @@ static CPUReadMemoryFunc *e1000_mmio_read[] = {
>  
>  static void
>  e1000_mmio_map(PCIDevice *pci_dev, int region_num,
> -                uint32_t addr, uint32_t size, int type)
> +                uint64_t addr, uint64_t size, int type)
>  {
>      E1000State *d = (E1000State *)pci_dev;
>      int i;
> @@ -1031,7 +1032,8 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
>      };
>  
>  
> -    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08x 0x%08x\n", addr, size);
> +    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n",
> +           addr, size);
>  
>      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
>      qemu_register_coalesced_mmio(addr, excluded_regs[0]);
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 85446ed..795acab 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -1345,12 +1345,12 @@ typedef struct PCIEEPRO100State {
>  } PCIEEPRO100State;
>  
>  static void pci_map(PCIDevice * pci_dev, int region_num,
> -                    uint32_t addr, uint32_t size, int type)
> +                    uint64_t addr, uint64_t size, int type)
>  {
>      PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
>      EEPRO100State *s = &d->eepro100;
>  
> -    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
> +    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
>             region_num, addr, size, type);
>  
>      assert(region_num == 1);
> @@ -1419,11 +1419,11 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
>  };
>  
>  static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
> -                         uint32_t addr, uint32_t size, int type)
> +                         uint64_t addr, uint64_t size, int type)
>  {
>      PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
>  
> -    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
> +    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
>             region_num, addr, size, type);
>  
>      if (region_num == 0) {
> diff --git a/hw/es1370.c b/hw/es1370.c
> index 5c9af0e..6a3f3a6 100644
> --- a/hw/es1370.c
> +++ b/hw/es1370.c
> @@ -913,7 +913,7 @@ static void es1370_adc_callback (void *opaque, int avail)
>  }
>  
>  static void es1370_map (PCIDevice *pci_dev, int region_num,
> -                        uint32_t addr, uint32_t size, int type)
> +                        uint64_t addr, uint64_t size, int type)
>  {
>      PCIES1370State *d = (PCIES1370State *) pci_dev;
>      ES1370State *s = &d->es1370;
> diff --git a/hw/ide.c b/hw/ide.c
> index 1e56786..d2f801a 100644
> --- a/hw/ide.c
> +++ b/hw/ide.c
> @@ -2921,7 +2921,7 @@ void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
>  static void cmd646_update_irq(PCIIDEState *d);
>  
>  static void ide_map(PCIDevice *pci_dev, int region_num,
> -                    uint32_t addr, uint32_t size, int type)
> +                    uint64_t addr, uint64_t size, int type)
>  {
>      PCIIDEState *d = (PCIIDEState *)pci_dev;
>      IDEState *ide_state;
> @@ -3152,7 +3152,7 @@ static void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val)
>  }
>  
>  static void bmdma_map(PCIDevice *pci_dev, int region_num,
> -                    uint32_t addr, uint32_t size, int type)
> +                    uint64_t addr, uint64_t size, int type)
>  {
>      PCIIDEState *d = (PCIIDEState *)pci_dev;
>      int i;
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index 516a468..d9ebe6e 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -1923,7 +1923,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
>  }
>  
>  static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
> -                           uint32_t addr, uint32_t size, int type)
> +                           uint64_t addr, uint64_t size, int type)
>  {
>      LSIState *s = (LSIState *)pci_dev;
>  
> @@ -1938,7 +1938,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
>  }
>  
>  static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
> -                            uint32_t addr, uint32_t size, int type)
> +                            uint64_t addr, uint64_t size, int type)
>  {
>      LSIState *s = (LSIState *)pci_dev;
>  
> @@ -1948,7 +1948,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
>  }
>  
>  static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
> -                             uint32_t addr, uint32_t size, int type)
> +                             uint64_t addr, uint64_t size, int type)
>  {
>      LSIState *s = (LSIState *)pci_dev;
>  
> diff --git a/hw/macio.c b/hw/macio.c
> index 8cfadfc..41412c3 100644
> --- a/hw/macio.c
> +++ b/hw/macio.c
> @@ -40,7 +40,7 @@ struct macio_state_t {
>  };
>  
>  static void macio_map (PCIDevice *pci_dev, int region_num,
> -                       uint32_t addr, uint32_t size, int type)
> +                       uint64_t addr, uint64_t size, int type)
>  {
>      macio_state_t *macio_state;
>      int i;
> diff --git a/hw/msix.c b/hw/msix.c
> index 3420ce9..6e7cd7b 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -204,7 +204,7 @@ static CPUReadMemoryFunc *msix_mmio_read[] = {
>  
>  /* Should be called from device's map method. */
>  void msix_mmio_map(PCIDevice *d, int region_num,
> -                   uint32_t addr, uint32_t size, int type)
> +                   uint64_t addr, uint64_t size, int type)
>  {
>      uint8_t *config = d->config + d->msix_cap;
>      uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> diff --git a/hw/msix.h b/hw/msix.h
> index 3427778..99f1647 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -10,7 +10,7 @@ void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
>  
>  void msix_mmio_map(PCIDevice *pci_dev, int region_num,
> -                   uint32_t addr, uint32_t size, int type);
> +                   uint64_t addr, uint64_t size, int type);
>  
>  int msix_uninit(PCIDevice *d);
>  
> diff --git a/hw/ne2000.c b/hw/ne2000.c
> index b9c018a..726d98a 100644
> --- a/hw/ne2000.c
> +++ b/hw/ne2000.c
> @@ -777,7 +777,7 @@ typedef struct PCINE2000State {
>  } PCINE2000State;
>  
>  static void ne2000_map(PCIDevice *pci_dev, int region_num,
> -                       uint32_t addr, uint32_t size, int type)
> +                       uint64_t addr, uint64_t size, int type)
>  {
>      PCINE2000State *d = (PCINE2000State *)pci_dev;
>      NE2000State *s = &d->ne2000;
> diff --git a/hw/openpic.c b/hw/openpic.c
> index baa7ecc..276377e 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -1026,7 +1026,7 @@ static CPUReadMemoryFunc *openpic_read[] = {
>  };
>  
>  static void openpic_map(PCIDevice *pci_dev, int region_num,
> -                        uint32_t addr, uint32_t size, int type)
> +                        uint64_t addr, uint64_t size, int type)
>  {
>      openpic_t *opp;
>  
> diff --git a/hw/pci.c b/hw/pci.c
> index 03241d4..2d985f0 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -399,19 +399,19 @@ int pci_unregister_device(PCIDevice *pci_dev)
>  }
>  
>  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> -                            uint32_t size, int type,
> +                            uint64_t size, int type,
>                              PCIMapIORegionFunc *map_func)
>  {
>      PCIIORegion *r;
>      uint32_t addr;
> -    uint32_t wmask;
> +    uint64_t wmask;
>  
>      if ((unsigned int)region_num >= PCI_NUM_REGIONS)
>          return;
>  
>      if (size & (size-1)) {
>          fprintf(stderr, "ERROR: PCI region size must be pow2 "
> -                    "type=0x%x, size=0x%x\n", type, size);
> +                    "type=0x%x, size=0x%"PRIx64"\n", type, size);
>          exit(1);
>      }
>  
> @@ -430,7 +430,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>          addr = 0x10 + region_num * 4;
>      }
>      *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
> -    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask);
> +    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
>      *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
>  }
>  
> @@ -438,7 +438,8 @@ static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
>      int cmd, i;
> -    uint32_t last_addr, new_addr, config_ofs;
> +    uint64_t last_addr, new_addr;
> +    uint32_t config_ofs;
>  
>      cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
> @@ -477,7 +478,11 @@ static void pci_update_mappings(PCIDevice *d)
>                         mappings, we handle specific values as invalid
>                         mappings. */
>                      if (last_addr <= new_addr || new_addr == 0 ||
> -                        last_addr == PCI_BAR_UNMAPPED) {
> +                        last_addr == PCI_BAR_UNMAPPED ||
> +
> +                        /* keep old behaviour
> +                         * without this, PC ide doesn't work well. */
> +                        last_addr >= UINT32_MAX) {
>                          new_addr = PCI_BAR_UNMAPPED;
>                      }
>                  } else {
> @@ -733,10 +738,10 @@ static void pci_info_device(PCIDevice *d)
>          if (r->size != 0) {
>              monitor_printf(mon, "      BAR%d: ", i);
>              if (r->type & PCI_ADDRESS_SPACE_IO) {
> -                monitor_printf(mon, "I/O at 0x%04x [0x%04x].\n",
> +                monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
>                                 r->addr, r->addr + r->size - 1);
>              } else {
> -                monitor_printf(mon, "32 bit memory at 0x%08x [0x%08x].\n",
> +                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
>                                 r->addr, r->addr + r->size - 1);
>              }
>          }
> @@ -1044,7 +1049,8 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>          r = &d->io_regions[i];
>          if (!r->size)
>              continue;
> -        monitor_printf(mon, "%*sbar %d: %s at 0x%x [0x%x]\n", indent, "",
> +        monitor_printf(mon, "%*sbar %d: %s at 0x%"PRIx64" [0x%"PRIx64"]\n",
> +                       indent, "",
>                         i, r->type & PCI_ADDRESS_SPACE_IO ? "i/o" : "mem",
>                         r->addr, r->addr + r->size - 1);
>      }
> diff --git a/hw/pci.h b/hw/pci.h
> index 9d60c36..fee9ed6 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -75,7 +75,7 @@ typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
>  typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
>                                     uint32_t address, int len);
>  typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
> -                                uint32_t addr, uint32_t size, int type);
> +                                uint64_t addr, uint64_t size, int type);
>  typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  
>  #define PCI_ADDRESS_SPACE_MEM		0x00
> @@ -83,9 +83,9 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
>  
>  typedef struct PCIIORegion {
> -    uint32_t addr; /* current PCI mapping address. -1 means not mapped */
> -#define PCI_BAR_UNMAPPED        (~(uint32_t)0)
> -    uint32_t size;
> +    uint64_t addr; /* current PCI mapping address. -1 means not mapped */
> +#define PCI_BAR_UNMAPPED        (~(uint64_t)0)
> +    uint64_t size;
>      uint8_t type;
>      PCIMapIORegionFunc *map_func;
>  } PCIIORegion;
> @@ -219,7 +219,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
>  int pci_unregister_device(PCIDevice *pci_dev);
>  
>  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> -                            uint32_t size, int type,
> +                            uint64_t size, int type,
>                              PCIMapIORegionFunc *map_func);
>  
>  int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
> diff --git a/hw/pcnet.c b/hw/pcnet.c
> index 22ab6be..e039d9f 100644
> --- a/hw/pcnet.c
> +++ b/hw/pcnet.c
> @@ -1762,12 +1762,13 @@ static uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
>  }
>  
>  static void pcnet_ioport_map(PCIDevice *pci_dev, int region_num,
> -                             uint32_t addr, uint32_t size, int type)
> +                             uint64_t addr, uint64_t size, int type)
>  {
>      PCNetState *d = &((PCIPCNetState *)pci_dev)->state;
>  
>  #ifdef PCNET_DEBUG_IO
> -    printf("pcnet_ioport_map addr=0x%04x size=0x%04x\n", addr, size);
> +    printf("pcnet_ioport_map addr=0x%04"PRIx64" size=0x%04"PRIx64"\n",
> +           addr, size);
>  #endif
>  
>      register_ioport_write(addr, 16, 1, pcnet_aprom_writeb, d);
> @@ -1976,12 +1977,12 @@ static CPUReadMemoryFunc *pcnet_mmio_read[] = {
>  };
>  
>  static void pcnet_mmio_map(PCIDevice *pci_dev, int region_num,
> -                            uint32_t addr, uint32_t size, int type)
> +                            uint64_t addr, uint64_t size, int type)
>  {
>      PCIPCNetState *d = (PCIPCNetState *)pci_dev;
>  
>  #ifdef PCNET_DEBUG_IO
> -    printf("pcnet_mmio_map addr=0x%08x 0x%08x\n", addr, size);
> +    printf("pcnet_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n", addr, size);
>  #endif
>  
>      cpu_register_physical_memory(addr, PCNET_PNPMMIO_SIZE, d->state.mmio_index);
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 91165db..6b8461c 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -3329,7 +3329,7 @@ typedef struct PCIRTL8139State {
>  } PCIRTL8139State;
>  
>  static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
> -                       uint32_t addr, uint32_t size, int type)
> +                       uint64_t addr, uint64_t size, int type)
>  {
>      PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
>      RTL8139State *s = &d->rtl8139;
> @@ -3338,7 +3338,7 @@ static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
>  }
>  
>  static void rtl8139_ioport_map(PCIDevice *pci_dev, int region_num,
> -                       uint32_t addr, uint32_t size, int type)
> +                       uint64_t addr, uint64_t size, int type)
>  {
>      PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
>      RTL8139State *s = &d->rtl8139;
> diff --git a/hw/sun4u.c b/hw/sun4u.c
> index e08ae23..eb7737f 100644
> --- a/hw/sun4u.c
> +++ b/hw/sun4u.c
> @@ -288,7 +288,7 @@ static const int parallel_irq[MAX_PARALLEL_PORTS] = { 7, 7, 7 };
>  static fdctrl_t *floppy_controller;
>  
>  static void ebus_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
> -                              uint32_t addr, uint32_t size, int type)
> +                              uint64_t addr, uint64_t size, int type)
>  {
>      DPRINTF("Mapping region %d registers at %08x\n", region_num, addr);
>      switch (region_num) {
> diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
> index 83d1a5c..f94f57a 100644
> --- a/hw/usb-ohci.c
> +++ b/hw/usb-ohci.c
> @@ -1705,7 +1705,7 @@ typedef struct {
>  } OHCIPCIState;
>  
>  static void ohci_mapfunc(PCIDevice *pci_dev, int i,
> -            uint32_t addr, uint32_t size, int type)
> +            uint64_t addr, uint64_t size, int type)
>  {
>      OHCIPCIState *ohci = (OHCIPCIState *)pci_dev;
>      cpu_register_physical_memory(addr, size, ohci->state.mem);
> diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
> index 7b74207..5ffe612 100644
> --- a/hw/usb-uhci.c
> +++ b/hw/usb-uhci.c
> @@ -1058,7 +1058,7 @@ static void uhci_frame_timer(void *opaque)
>  }
>  
>  static void uhci_map(PCIDevice *pci_dev, int region_num,
> -                    uint32_t addr, uint32_t size, int type)
> +                    uint64_t addr, uint64_t size, int type)
>  {
>      UHCIState *s = (UHCIState *)pci_dev;
>  
> diff --git a/hw/vga.c b/hw/vga.c
> index e1a470d..6e0d61a 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2236,7 +2236,7 @@ void vga_dirty_log_start(VGAState *s)
>  }
>  
>  static void vga_map(PCIDevice *pci_dev, int region_num,
> -                    uint32_t addr, uint32_t size, int type)
> +                    uint64_t addr, uint64_t size, int type)
>  {
>      PCIVGAState *d = (PCIVGAState *)pci_dev;
>      VGAState *s = &d->vga_state;
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index d605b5f..f2fe42e 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -344,7 +344,7 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val)
>  }
>  
>  static void virtio_map(PCIDevice *pci_dev, int region_num,
> -                       uint32_t addr, uint32_t size, int type)
> +                       uint64_t addr, uint64_t size, int type)
>  {
>      VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev);
>      VirtIODevice *vdev = proxy->vdev;
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 5ceebf1..97248a7 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1173,7 +1173,7 @@ static int pci_vmsvga_load(QEMUFile *f, void *opaque, int version_id)
>  }
>  
>  static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
> -                uint32_t addr, uint32_t size, int type)
> +                uint64_t addr, uint64_t size, int type)
>  {
>      struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
>      struct vmsvga_state_s *s = &d->chip;
> @@ -1193,7 +1193,7 @@ static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
>  }
>  
>  static void pci_vmsvga_map_mem(PCIDevice *pci_dev, int region_num,
> -                uint32_t addr, uint32_t size, int type)
> +                uint64_t addr, uint64_t size, int type)
>  {
>      struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
>      struct vmsvga_state_s *s = &d->chip;
> diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
> index 42642c7..678eb17 100644
> --- a/hw/wdt_i6300esb.c
> +++ b/hw/wdt_i6300esb.c
> @@ -351,7 +351,7 @@ static void i6300esb_mem_writel(void *vp, target_phys_addr_t addr, uint32_t val)
>  }
>  
>  static void i6300esb_map(PCIDevice *dev, int region_num,
> -                         uint32_t addr, uint32_t size, int type)
> +                         uint64_t addr, uint64_t size, int type)
>  {
>      static CPUReadMemoryFunc *mem_read[3] = {
>          i6300esb_mem_readb,
> @@ -366,7 +366,8 @@ static void i6300esb_map(PCIDevice *dev, int region_num,
>      I6300State *d = (I6300State *) dev;
>      int io_mem;
>  
> -    i6300esb_debug("addr = %x, size = %x, type = %d\n", addr, size, type);
> +    i6300esb_debug("addr = %"PRIx64", size = %"PRIx64", type = %d\n",
> +                   addr, size, type);
>  
>      io_mem = cpu_register_io_memory(mem_read, mem_write, d);
>      cpu_register_physical_memory (addr, 0x10, io_mem);
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:

> On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > This patch is preliminary for 64bit bar.
> > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > to be able to represent 64bit width.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> As long as we are touching all code: what do you think about
> typedef uint64_t pcibus_t;
> so that will be easier to find later?

Please read the CODING_STYLE on the subject of _t suffix.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Michael S. Tsirkin]

On Wed, Sep 30, 2009 at 07:25:45PM +0400, malc wrote:
> On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> 
> > On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > > This patch is preliminary for 64bit bar.
> > > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > > to be able to represent 64bit width.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > As long as we are touching all code: what do you think about
> > typedef uint64_t pcibus_t;
> > so that will be easier to find later?
> 
> Please read the CODING_STYLE on the subject of _t suffix.
> 
> [..snip..]

What do you refer to?

> -- 
> mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:

> On Wed, Sep 30, 2009 at 07:25:45PM +0400, malc wrote:
> > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > 
> > > On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > > > This patch is preliminary for 64bit bar.
> > > > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > > > to be able to represent 64bit width.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > 
> > > As long as we are touching all code: what do you think about
> > > typedef uint64_t pcibus_t;
> > > so that will be easier to find later?
> > 
> > Please read the CODING_STYLE on the subject of _t suffix.
> > 
> > [..snip..]
> 
> What do you refer to?
> 

_t suffix is reserved by POSIX.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Michael S. Tsirkin]

On Wed, Sep 30, 2009 at 08:51:02PM +0400, malc wrote:
> On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> 
> > On Wed, Sep 30, 2009 at 07:25:45PM +0400, malc wrote:
> > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > 
> > > > On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > > > > This patch is preliminary for 64bit bar.
> > > > > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > > > > to be able to represent 64bit width.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > 
> > > > As long as we are touching all code: what do you think about
> > > > typedef uint64_t pcibus_t;
> > > > so that will be easier to find later?
> > > 
> > > Please read the CODING_STYLE on the subject of _t suffix.
> > > 
> > > [..snip..]
> > 
> > What do you refer to?
> > 
> 
> _t suffix is reserved by POSIX.

There's still no better naming for scalars.  Worst case some platform
will fail to compile, and we'll rename.


> -- 
> mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:

> On Wed, Sep 30, 2009 at 08:51:02PM +0400, malc wrote:
> > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > 
> > > On Wed, Sep 30, 2009 at 07:25:45PM +0400, malc wrote:
> > > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > > 
> > > > > On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > > > > > This patch is preliminary for 64bit bar.
> > > > > > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > > > > > to be able to represent 64bit width.
> > > > > > 
> > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > 
> > > > > As long as we are touching all code: what do you think about
> > > > > typedef uint64_t pcibus_t;
> > > > > so that will be easier to find later?
> > > > 
> > > > Please read the CODING_STYLE on the subject of _t suffix.
> > > > 
> > > > [..snip..]
> > > 
> > > What do you refer to?
> > > 
> > 
> > _t suffix is reserved by POSIX.
> 
> There's still no better naming for scalars.  Worst case some platform
> will fail to compile, and we'll rename.
> 

Not good enough, if you are using something you have to abide the
constraints.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Michael S. Tsirkin]

On Wed, Sep 30, 2009 at 09:59:11PM +0400, malc wrote:
> On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> 
> > On Wed, Sep 30, 2009 at 08:51:02PM +0400, malc wrote:
> > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > 
> > > > On Wed, Sep 30, 2009 at 07:25:45PM +0400, malc wrote:
> > > > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > > > 
> > > > > > On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > > > > > > This patch is preliminary for 64bit bar.
> > > > > > > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > > > > > > to be able to represent 64bit width.
> > > > > > > 
> > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > > 
> > > > > > As long as we are touching all code: what do you think about
> > > > > > typedef uint64_t pcibus_t;
> > > > > > so that will be easier to find later?
> > > > > 
> > > > > Please read the CODING_STYLE on the subject of _t suffix.
> > > > > 
> > > > > [..snip..]
> > > > 
> > > > What do you refer to?
> > > > 
> > > 
> > > _t suffix is reserved by POSIX.
> > 
> > There's still no better naming for scalars.  Worst case some platform
> > will fail to compile, and we'll rename.
> > 
> 
> Not good enough, if you are using something you have to abide the
> constraints.

So, posix does not reserve all of *_t namespace, it would negate years
of C code. It simply says posix implementations can add only symbols
ending with _t in the headers: this is a constraint on posix headers,
not on applications that use them.  Thus you have to find some other
means to avoid conflict if you have symbols ending with _t. Using your
module name as a prefix is a classical way to do this, for pci prefixing
type name with pci_ should be enough to prevent any issues.


> -- 
> mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Thu, 1 Oct 2009, Michael S. Tsirkin wrote:

> On Wed, Sep 30, 2009 at 09:59:11PM +0400, malc wrote:
> > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > 

[..snip..]

> > > > 
> > > > _t suffix is reserved by POSIX.
> > > 
> > > There's still no better naming for scalars.  Worst case some platform
> > > will fail to compile, and we'll rename.
> > > 
> > 
> > Not good enough, if you are using something you have to abide the
> > constraints.
> 
> So, posix does not reserve all of *_t namespace, it would negate years
> of C code. It simply says posix implementations can add only symbols
> ending with _t in the headers: this is a constraint on posix headers,
> not on applications that use them.  Thus you have to find some other
> means to avoid conflict if you have symbols ending with _t. Using your
> module name as a prefix is a classical way to do this, for pci prefixing
> type name with pci_ should be enough to prevent any issues.

I don't understand this at all. POSIX says that if you include any of
it's headers be prepared to deal with it, i.e. any of your own
typedefs that end with _t are potentially clashing with what's defined
there, similarly identifiers that start with 'str', 'E', double
underscore and underscore followed by a capital letter are reserved by
C, you just don't go there.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Michael S. Tsirkin]

On Thu, Oct 01, 2009 at 04:15:59PM +0400, malc wrote:
> On Thu, 1 Oct 2009, Michael S. Tsirkin wrote:
> 
> > On Wed, Sep 30, 2009 at 09:59:11PM +0400, malc wrote:
> > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > 
> 
> [..snip..]
> 
> > > > > 
> > > > > _t suffix is reserved by POSIX.
> > > > 
> > > > There's still no better naming for scalars.  Worst case some platform
> > > > will fail to compile, and we'll rename.
> > > > 
> > > 
> > > Not good enough, if you are using something you have to abide the
> > > constraints.
> > 
> > So, posix does not reserve all of *_t namespace, it would negate years
> > of C code. It simply says posix implementations can add only symbols
> > ending with _t in the headers: this is a constraint on posix headers,
> > not on applications that use them.  Thus you have to find some other
> > means to avoid conflict if you have symbols ending with _t. Using your
> > module name as a prefix is a classical way to do this, for pci prefixing
> > type name with pci_ should be enough to prevent any issues.
> 
> I don't understand this at all. POSIX says that if you include any of
> it's headers be prepared to deal with it, i.e. any of your own
> typedefs that end with _t are potentially clashing with what's defined
> there,

In practice I don't expect any posix header to clash with
michael_s_tsirkin_t even though in theory it might.

So you can choose not to use any types that end with _t and have a
guarantee that you won't clash with posix, and just name types sanely so
they don't clash with posix in practice, on any platform people care
about, like 99% of the world does.

> similarly identifiers that start with 'str', 'E', double
> underscore and underscore followed by a capital letter are reserved by
> C, you just don't go there.

I agree, double underscores look ugly enough to avoid them without a special
reason, but for everyone reading C, *_t means "scalar type"
and this benefit of using it outweights, IMO, the danger of
potential clash with a header on a non-existing platform.

> -- 
> mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Thu, 1 Oct 2009, Michael S. Tsirkin wrote:

> On Thu, Oct 01, 2009 at 04:15:59PM +0400, malc wrote:
> > On Thu, 1 Oct 2009, Michael S. Tsirkin wrote:
> > 
> > > On Wed, Sep 30, 2009 at 09:59:11PM +0400, malc wrote:
> > > > On Wed, 30 Sep 2009, Michael S. Tsirkin wrote:
> > > > 

[..snip..]

> 
> In practice I don't expect any posix header to clash with
> michael_s_tsirkin_t even though in theory it might.
> 
> So you can choose not to use any types that end with _t and have a
> guarantee that you won't clash with posix, and just name types sanely so
> they don't clash with posix in practice, on any platform people care
> about, like 99% of the world does.
> 
> > similarly identifiers that start with 'str', 'E', double
> > underscore and underscore followed by a capital letter are reserved by
> > C, you just don't go there.
> 
> I agree, double underscores look ugly enough to avoid them without a special
> reason, but for everyone reading C, *_t means "scalar type"
> and this benefit of using it outweights, IMO, the danger of
> potential clash with a header on a non-existing platform.

That's that basically - we disagree.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Anthony Liguori]

malc wrote:
> I don't understand this at all. POSIX says that if you include any of
> it's headers be prepared to deal with it, i.e. any of your own
> typedefs that end with _t are potentially clashing with what's defined
> there, similarly identifiers that start with 'str', 'E', double
> underscore and underscore followed by a capital letter are reserved by
> C, you just don't go there.
>   

_t is used pervasively throughout the code right now.  In particular, we 
have ram_addr_t and target_phys_addr_t.  pci_addr_t is a logical addition.

I do agree that adhering to Posix is a good thing, but I think either we 
have to go through and fixup all of the _t's in the code today, or just 
accept the fact that we don't strictly adhere to Posix and deal with it 
if we ever have to.

The middle ground where we don't allow new uses of _t but keep the old 
ones is just going to result in mass confusion and bike shedding.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[malc]

On Thu, 1 Oct 2009, Anthony Liguori wrote:

> malc wrote:
> > I don't understand this at all. POSIX says that if you include any of
> > it's headers be prepared to deal with it, i.e. any of your own
> > typedefs that end with _t are potentially clashing with what's defined
> > there, similarly identifiers that start with 'str', 'E', double
> > underscore and underscore followed by a capital letter are reserved by
> > C, you just don't go there.
> >   
> 
> _t is used pervasively throughout the code right now.  In particular, we have
> ram_addr_t and target_phys_addr_t.  pci_addr_t is a logical addition.
> 
> I do agree that adhering to Posix is a good thing, but I think either we have
> to go through and fixup all of the _t's in the code today, or just accept the
> fact that we don't strictly adhere to Posix and deal with it if we ever have
> to.
> 
> The middle ground where we don't allow new uses of _t but keep the old ones is
> just going to result in mass confusion and bike shedding.
> 

Agreed. Let's see how removing it will not lead to bike shedding.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Jamie Lokier]

malc wrote:
> I don't understand this at all. POSIX says that if you include any of
> it's headers be prepared to deal with it, i.e. any of your own
> typedefs that end with _t are potentially clashing with what's defined
> there, similarly identifiers that start with 'str', 'E', double
> underscore and underscore followed by a capital letter are reserved by
> C, you just don't go there.

What's missing from that picture is there are _lots_ of other
identifiers not covered by the rule which also potentially clash with
your own, depending on vagaries of OSes, many extensions are not
covered by that POSIX rule but you still mustn't clash with them, and
the third-party libraries you typically link with have to be avoided too.

Not just header file clashes, but link-time or run-time problems due
to clashing with names not declared in the header files you included
too.  Defining your own 'valloc' function (with it's own meaning) will
probably cause a run-time failure in some library you might link to,
even though that's not a POSIX reserved identifier so nobody broke the
official rules.  Even defining your own 'error' function can be a
problem - I've seen applications break because they did that, and some
library they were linked to depended on the function of that name in
libc.

In other words, POSIX isn't a complete rule - it's a guideline, but
following it won't make you safe, and breaking it won't put you in
serious danger if you pick sensible names.  There is no absolute.

Since there are no absolutes anyway, sometimes the clarify benefits of
a readable name like 'ram_addr_t' outweight the concerns about
potential clash on some platform - even though it's not hard to
imagine some obscure system header file which has a definition of that name.

Also, Consider what happens when you make a library that's intended to
be used by lots of different applications.  Is it better to declare
that your library is in fact part of the "system" and therefore
justify using names like mylib_thing_t and _mylib_private_thing, so
that user apps are unlikely to clash with your library (but you have
to check compatibility with actual OSes), or is it better to use
mylib_thing and mylib_private_thing only, reduce POSIX clashes (not
100% avoidance due to OS extensions), and risk increased amounts of
clashes with apps using your library instead?  The answer is there
isn't a clear answer, you have to be pragmatic.  Depending on sane
prefixes that are unlikely to clash is a common pragmatic choice.

Of course if writing code which is guaranteed to work on any POSIX
platform is you're overriding objective, then following the POSIX rule
strictly makes sense.

-- Jamie

Re: [Qemu-devel] [PATCH 4/9] pci: use uint64_t for bar addr and size instead of uint32_t.

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:41:22PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2009 at 08:15:04PM +0900, Isaku Yamahata wrote:
> > This patch is preliminary for 64bit bar.
> > For 64bit bar support, replace uint32_t with uint64_t for addr/size
> > to be able to represent 64bit width.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> As long as we are touching all code: what do you think about
> typedef uint64_t pcibus_t;
> so that will be easier to find later?

That's a good idea.
Although pcibus_t is opposed to because of contradiction to POSIX,
I can't find any better name.
So I'll go with pcibus_t unless better name is proposed.

I also checked pciutils(libpci) that it uses pciaddr_t which doesn't
conflict with pcibus_t.

> 
> > ---
> >  hw/ac97.c         |    2 +-
> >  hw/cirrus_vga.c   |    4 ++--
> >  hw/e1000.c        |   12 +++++++-----
> >  hw/eepro100.c     |    8 ++++----
> >  hw/es1370.c       |    2 +-
> >  hw/ide.c          |    4 ++--
> >  hw/lsi53c895a.c   |    6 +++---
> >  hw/macio.c        |    2 +-
> >  hw/msix.c         |    2 +-
> >  hw/msix.h         |    2 +-
> >  hw/ne2000.c       |    2 +-
> >  hw/openpic.c      |    2 +-
> >  hw/pci.c          |   24 +++++++++++++++---------
> >  hw/pci.h          |   10 +++++-----
> >  hw/pcnet.c        |    9 +++++----
> >  hw/rtl8139.c      |    4 ++--
> >  hw/sun4u.c        |    2 +-
> >  hw/usb-ohci.c     |    2 +-
> >  hw/usb-uhci.c     |    2 +-
> >  hw/vga.c          |    2 +-
> >  hw/virtio-pci.c   |    2 +-
> >  hw/vmware_vga.c   |    4 ++--
> >  hw/wdt_i6300esb.c |    5 +++--
> >  23 files changed, 62 insertions(+), 52 deletions(-)
> > 
> > diff --git a/hw/ac97.c b/hw/ac97.c
> > index f18fa52..faf7444 100644
> > --- a/hw/ac97.c
> > +++ b/hw/ac97.c
> > @@ -1267,7 +1267,7 @@ static int ac97_load (QEMUFile *f, void *opaque, int version_id)
> >  }
> >  
> >  static void ac97_map (PCIDevice *pci_dev, int region_num,
> > -                      uint32_t addr, uint32_t size, int type)
> > +                      uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIAC97LinkState *d = (PCIAC97LinkState *) pci_dev;
> >      AC97LinkState *s = &d->ac97;
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index 88868b9..afb8a2b 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -3260,7 +3260,7 @@ void isa_cirrus_vga_init(void)
> >   ***************************************/
> >  
> >  static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
> > -			       uint32_t addr, uint32_t size, int type)
> > +			       uint64_t addr, uint64_t size, int type)
> >  {
> >      CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
> >  
> > @@ -3281,7 +3281,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
> >  }
> >  
> >  static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
> > -				uint32_t addr, uint32_t size, int type)
> > +				uint64_t addr, uint64_t size, int type)
> >  {
> >      CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
> >  
> > diff --git a/hw/e1000.c b/hw/e1000.c
> > index 4ac8918..149da98 100644
> > --- a/hw/e1000.c
> > +++ b/hw/e1000.c
> > @@ -143,10 +143,11 @@ static const char phy_regcap[0x20] = {
> >  };
> >  
> >  static void
> > -ioport_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
> > -           uint32_t size, int type)
> > +ioport_map(PCIDevice *pci_dev, int region_num, uint64_t addr,
> > +           uint64_t size, int type)
> >  {
> > -    DBGOUT(IO, "e1000_ioport_map addr=0x%04x size=0x%08x\n", addr, size);
> > +    DBGOUT(IO, "e1000_ioport_map addr=0x%04"PRIx64" size=0x%08"PRIx64"\n",
> > +           addr, size);
> >  }
> >  
> >  static void
> > @@ -1021,7 +1022,7 @@ static CPUReadMemoryFunc *e1000_mmio_read[] = {
> >  
> >  static void
> >  e1000_mmio_map(PCIDevice *pci_dev, int region_num,
> > -                uint32_t addr, uint32_t size, int type)
> > +                uint64_t addr, uint64_t size, int type)
> >  {
> >      E1000State *d = (E1000State *)pci_dev;
> >      int i;
> > @@ -1031,7 +1032,8 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
> >      };
> >  
> >  
> > -    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08x 0x%08x\n", addr, size);
> > +    DBGOUT(MMIO, "e1000_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n",
> > +           addr, size);
> >  
> >      cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
> >      qemu_register_coalesced_mmio(addr, excluded_regs[0]);
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index 85446ed..795acab 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -1345,12 +1345,12 @@ typedef struct PCIEEPRO100State {
> >  } PCIEEPRO100State;
> >  
> >  static void pci_map(PCIDevice * pci_dev, int region_num,
> > -                    uint32_t addr, uint32_t size, int type)
> > +                    uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
> >      EEPRO100State *s = &d->eepro100;
> >  
> > -    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
> > +    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
> >             region_num, addr, size, type);
> >  
> >      assert(region_num == 1);
> > @@ -1419,11 +1419,11 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
> >  };
> >  
> >  static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
> > -                         uint32_t addr, uint32_t size, int type)
> > +                         uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
> >  
> > -    logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
> > +    logout("region %d, addr=0x%08"PRIx64", size=0x%08"PRIx64", type=%d\n",
> >             region_num, addr, size, type);
> >  
> >      if (region_num == 0) {
> > diff --git a/hw/es1370.c b/hw/es1370.c
> > index 5c9af0e..6a3f3a6 100644
> > --- a/hw/es1370.c
> > +++ b/hw/es1370.c
> > @@ -913,7 +913,7 @@ static void es1370_adc_callback (void *opaque, int avail)
> >  }
> >  
> >  static void es1370_map (PCIDevice *pci_dev, int region_num,
> > -                        uint32_t addr, uint32_t size, int type)
> > +                        uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIES1370State *d = (PCIES1370State *) pci_dev;
> >      ES1370State *s = &d->es1370;
> > diff --git a/hw/ide.c b/hw/ide.c
> > index 1e56786..d2f801a 100644
> > --- a/hw/ide.c
> > +++ b/hw/ide.c
> > @@ -2921,7 +2921,7 @@ void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
> >  static void cmd646_update_irq(PCIIDEState *d);
> >  
> >  static void ide_map(PCIDevice *pci_dev, int region_num,
> > -                    uint32_t addr, uint32_t size, int type)
> > +                    uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIIDEState *d = (PCIIDEState *)pci_dev;
> >      IDEState *ide_state;
> > @@ -3152,7 +3152,7 @@ static void bmdma_addr_writel(void *opaque, uint32_t addr, uint32_t val)
> >  }
> >  
> >  static void bmdma_map(PCIDevice *pci_dev, int region_num,
> > -                    uint32_t addr, uint32_t size, int type)
> > +                    uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIIDEState *d = (PCIIDEState *)pci_dev;
> >      int i;
> > diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> > index 516a468..d9ebe6e 100644
> > --- a/hw/lsi53c895a.c
> > +++ b/hw/lsi53c895a.c
> > @@ -1923,7 +1923,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
> >  }
> >  
> >  static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
> > -                           uint32_t addr, uint32_t size, int type)
> > +                           uint64_t addr, uint64_t size, int type)
> >  {
> >      LSIState *s = (LSIState *)pci_dev;
> >  
> > @@ -1938,7 +1938,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
> >  }
> >  
> >  static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
> > -                            uint32_t addr, uint32_t size, int type)
> > +                            uint64_t addr, uint64_t size, int type)
> >  {
> >      LSIState *s = (LSIState *)pci_dev;
> >  
> > @@ -1948,7 +1948,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
> >  }
> >  
> >  static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
> > -                             uint32_t addr, uint32_t size, int type)
> > +                             uint64_t addr, uint64_t size, int type)
> >  {
> >      LSIState *s = (LSIState *)pci_dev;
> >  
> > diff --git a/hw/macio.c b/hw/macio.c
> > index 8cfadfc..41412c3 100644
> > --- a/hw/macio.c
> > +++ b/hw/macio.c
> > @@ -40,7 +40,7 @@ struct macio_state_t {
> >  };
> >  
> >  static void macio_map (PCIDevice *pci_dev, int region_num,
> > -                       uint32_t addr, uint32_t size, int type)
> > +                       uint64_t addr, uint64_t size, int type)
> >  {
> >      macio_state_t *macio_state;
> >      int i;
> > diff --git a/hw/msix.c b/hw/msix.c
> > index 3420ce9..6e7cd7b 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -204,7 +204,7 @@ static CPUReadMemoryFunc *msix_mmio_read[] = {
> >  
> >  /* Should be called from device's map method. */
> >  void msix_mmio_map(PCIDevice *d, int region_num,
> > -                   uint32_t addr, uint32_t size, int type)
> > +                   uint64_t addr, uint64_t size, int type)
> >  {
> >      uint8_t *config = d->config + d->msix_cap;
> >      uint32_t table = pci_get_long(config + MSIX_TABLE_OFFSET);
> > diff --git a/hw/msix.h b/hw/msix.h
> > index 3427778..99f1647 100644
> > --- a/hw/msix.h
> > +++ b/hw/msix.h
> > @@ -10,7 +10,7 @@ void msix_write_config(PCIDevice *pci_dev, uint32_t address,
> >                         uint32_t val, int len);
> >  
> >  void msix_mmio_map(PCIDevice *pci_dev, int region_num,
> > -                   uint32_t addr, uint32_t size, int type);
> > +                   uint64_t addr, uint64_t size, int type);
> >  
> >  int msix_uninit(PCIDevice *d);
> >  
> > diff --git a/hw/ne2000.c b/hw/ne2000.c
> > index b9c018a..726d98a 100644
> > --- a/hw/ne2000.c
> > +++ b/hw/ne2000.c
> > @@ -777,7 +777,7 @@ typedef struct PCINE2000State {
> >  } PCINE2000State;
> >  
> >  static void ne2000_map(PCIDevice *pci_dev, int region_num,
> > -                       uint32_t addr, uint32_t size, int type)
> > +                       uint64_t addr, uint64_t size, int type)
> >  {
> >      PCINE2000State *d = (PCINE2000State *)pci_dev;
> >      NE2000State *s = &d->ne2000;
> > diff --git a/hw/openpic.c b/hw/openpic.c
> > index baa7ecc..276377e 100644
> > --- a/hw/openpic.c
> > +++ b/hw/openpic.c
> > @@ -1026,7 +1026,7 @@ static CPUReadMemoryFunc *openpic_read[] = {
> >  };
> >  
> >  static void openpic_map(PCIDevice *pci_dev, int region_num,
> > -                        uint32_t addr, uint32_t size, int type)
> > +                        uint64_t addr, uint64_t size, int type)
> >  {
> >      openpic_t *opp;
> >  
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 03241d4..2d985f0 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -399,19 +399,19 @@ int pci_unregister_device(PCIDevice *pci_dev)
> >  }
> >  
> >  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> > -                            uint32_t size, int type,
> > +                            uint64_t size, int type,
> >                              PCIMapIORegionFunc *map_func)
> >  {
> >      PCIIORegion *r;
> >      uint32_t addr;
> > -    uint32_t wmask;
> > +    uint64_t wmask;
> >  
> >      if ((unsigned int)region_num >= PCI_NUM_REGIONS)
> >          return;
> >  
> >      if (size & (size-1)) {
> >          fprintf(stderr, "ERROR: PCI region size must be pow2 "
> > -                    "type=0x%x, size=0x%x\n", type, size);
> > +                    "type=0x%x, size=0x%"PRIx64"\n", type, size);
> >          exit(1);
> >      }
> >  
> > @@ -430,7 +430,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> >          addr = 0x10 + region_num * 4;
> >      }
> >      *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
> > -    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask);
> > +    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
> >      *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
> >  }
> >  
> > @@ -438,7 +438,8 @@ static void pci_update_mappings(PCIDevice *d)
> >  {
> >      PCIIORegion *r;
> >      int cmd, i;
> > -    uint32_t last_addr, new_addr, config_ofs;
> > +    uint64_t last_addr, new_addr;
> > +    uint32_t config_ofs;
> >  
> >      cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
> >      for(i = 0; i < PCI_NUM_REGIONS; i++) {
> > @@ -477,7 +478,11 @@ static void pci_update_mappings(PCIDevice *d)
> >                         mappings, we handle specific values as invalid
> >                         mappings. */
> >                      if (last_addr <= new_addr || new_addr == 0 ||
> > -                        last_addr == PCI_BAR_UNMAPPED) {
> > +                        last_addr == PCI_BAR_UNMAPPED ||
> > +
> > +                        /* keep old behaviour
> > +                         * without this, PC ide doesn't work well. */
> > +                        last_addr >= UINT32_MAX) {
> >                          new_addr = PCI_BAR_UNMAPPED;
> >                      }
> >                  } else {
> > @@ -733,10 +738,10 @@ static void pci_info_device(PCIDevice *d)
> >          if (r->size != 0) {
> >              monitor_printf(mon, "      BAR%d: ", i);
> >              if (r->type & PCI_ADDRESS_SPACE_IO) {
> > -                monitor_printf(mon, "I/O at 0x%04x [0x%04x].\n",
> > +                monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
> >                                 r->addr, r->addr + r->size - 1);
> >              } else {
> > -                monitor_printf(mon, "32 bit memory at 0x%08x [0x%08x].\n",
> > +                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
> >                                 r->addr, r->addr + r->size - 1);
> >              }
> >          }
> > @@ -1044,7 +1049,8 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> >          r = &d->io_regions[i];
> >          if (!r->size)
> >              continue;
> > -        monitor_printf(mon, "%*sbar %d: %s at 0x%x [0x%x]\n", indent, "",
> > +        monitor_printf(mon, "%*sbar %d: %s at 0x%"PRIx64" [0x%"PRIx64"]\n",
> > +                       indent, "",
> >                         i, r->type & PCI_ADDRESS_SPACE_IO ? "i/o" : "mem",
> >                         r->addr, r->addr + r->size - 1);
> >      }
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 9d60c36..fee9ed6 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -75,7 +75,7 @@ typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
> >  typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
> >                                     uint32_t address, int len);
> >  typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
> > -                                uint32_t addr, uint32_t size, int type);
> > +                                uint64_t addr, uint64_t size, int type);
> >  typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
> >  
> >  #define PCI_ADDRESS_SPACE_MEM		0x00
> > @@ -83,9 +83,9 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
> >  #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
> >  
> >  typedef struct PCIIORegion {
> > -    uint32_t addr; /* current PCI mapping address. -1 means not mapped */
> > -#define PCI_BAR_UNMAPPED        (~(uint32_t)0)
> > -    uint32_t size;
> > +    uint64_t addr; /* current PCI mapping address. -1 means not mapped */
> > +#define PCI_BAR_UNMAPPED        (~(uint64_t)0)
> > +    uint64_t size;
> >      uint8_t type;
> >      PCIMapIORegionFunc *map_func;
> >  } PCIIORegion;
> > @@ -219,7 +219,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
> >  int pci_unregister_device(PCIDevice *pci_dev);
> >  
> >  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> > -                            uint32_t size, int type,
> > +                            uint64_t size, int type,
> >                              PCIMapIORegionFunc *map_func);
> >  
> >  int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
> > diff --git a/hw/pcnet.c b/hw/pcnet.c
> > index 22ab6be..e039d9f 100644
> > --- a/hw/pcnet.c
> > +++ b/hw/pcnet.c
> > @@ -1762,12 +1762,13 @@ static uint32_t pcnet_ioport_readl(void *opaque, uint32_t addr)
> >  }
> >  
> >  static void pcnet_ioport_map(PCIDevice *pci_dev, int region_num,
> > -                             uint32_t addr, uint32_t size, int type)
> > +                             uint64_t addr, uint64_t size, int type)
> >  {
> >      PCNetState *d = &((PCIPCNetState *)pci_dev)->state;
> >  
> >  #ifdef PCNET_DEBUG_IO
> > -    printf("pcnet_ioport_map addr=0x%04x size=0x%04x\n", addr, size);
> > +    printf("pcnet_ioport_map addr=0x%04"PRIx64" size=0x%04"PRIx64"\n",
> > +           addr, size);
> >  #endif
> >  
> >      register_ioport_write(addr, 16, 1, pcnet_aprom_writeb, d);
> > @@ -1976,12 +1977,12 @@ static CPUReadMemoryFunc *pcnet_mmio_read[] = {
> >  };
> >  
> >  static void pcnet_mmio_map(PCIDevice *pci_dev, int region_num,
> > -                            uint32_t addr, uint32_t size, int type)
> > +                            uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIPCNetState *d = (PCIPCNetState *)pci_dev;
> >  
> >  #ifdef PCNET_DEBUG_IO
> > -    printf("pcnet_mmio_map addr=0x%08x 0x%08x\n", addr, size);
> > +    printf("pcnet_mmio_map addr=0x%08"PRIx64" 0x%08"PRIx64"\n", addr, size);
> >  #endif
> >  
> >      cpu_register_physical_memory(addr, PCNET_PNPMMIO_SIZE, d->state.mmio_index);
> > diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> > index 91165db..6b8461c 100644
> > --- a/hw/rtl8139.c
> > +++ b/hw/rtl8139.c
> > @@ -3329,7 +3329,7 @@ typedef struct PCIRTL8139State {
> >  } PCIRTL8139State;
> >  
> >  static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
> > -                       uint32_t addr, uint32_t size, int type)
> > +                       uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
> >      RTL8139State *s = &d->rtl8139;
> > @@ -3338,7 +3338,7 @@ static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
> >  }
> >  
> >  static void rtl8139_ioport_map(PCIDevice *pci_dev, int region_num,
> > -                       uint32_t addr, uint32_t size, int type)
> > +                       uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
> >      RTL8139State *s = &d->rtl8139;
> > diff --git a/hw/sun4u.c b/hw/sun4u.c
> > index e08ae23..eb7737f 100644
> > --- a/hw/sun4u.c
> > +++ b/hw/sun4u.c
> > @@ -288,7 +288,7 @@ static const int parallel_irq[MAX_PARALLEL_PORTS] = { 7, 7, 7 };
> >  static fdctrl_t *floppy_controller;
> >  
> >  static void ebus_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
> > -                              uint32_t addr, uint32_t size, int type)
> > +                              uint64_t addr, uint64_t size, int type)
> >  {
> >      DPRINTF("Mapping region %d registers at %08x\n", region_num, addr);
> >      switch (region_num) {
> > diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
> > index 83d1a5c..f94f57a 100644
> > --- a/hw/usb-ohci.c
> > +++ b/hw/usb-ohci.c
> > @@ -1705,7 +1705,7 @@ typedef struct {
> >  } OHCIPCIState;
> >  
> >  static void ohci_mapfunc(PCIDevice *pci_dev, int i,
> > -            uint32_t addr, uint32_t size, int type)
> > +            uint64_t addr, uint64_t size, int type)
> >  {
> >      OHCIPCIState *ohci = (OHCIPCIState *)pci_dev;
> >      cpu_register_physical_memory(addr, size, ohci->state.mem);
> > diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
> > index 7b74207..5ffe612 100644
> > --- a/hw/usb-uhci.c
> > +++ b/hw/usb-uhci.c
> > @@ -1058,7 +1058,7 @@ static void uhci_frame_timer(void *opaque)
> >  }
> >  
> >  static void uhci_map(PCIDevice *pci_dev, int region_num,
> > -                    uint32_t addr, uint32_t size, int type)
> > +                    uint64_t addr, uint64_t size, int type)
> >  {
> >      UHCIState *s = (UHCIState *)pci_dev;
> >  
> > diff --git a/hw/vga.c b/hw/vga.c
> > index e1a470d..6e0d61a 100644
> > --- a/hw/vga.c
> > +++ b/hw/vga.c
> > @@ -2236,7 +2236,7 @@ void vga_dirty_log_start(VGAState *s)
> >  }
> >  
> >  static void vga_map(PCIDevice *pci_dev, int region_num,
> > -                    uint32_t addr, uint32_t size, int type)
> > +                    uint64_t addr, uint64_t size, int type)
> >  {
> >      PCIVGAState *d = (PCIVGAState *)pci_dev;
> >      VGAState *s = &d->vga_state;
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index d605b5f..f2fe42e 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -344,7 +344,7 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val)
> >  }
> >  
> >  static void virtio_map(PCIDevice *pci_dev, int region_num,
> > -                       uint32_t addr, uint32_t size, int type)
> > +                       uint64_t addr, uint64_t size, int type)
> >  {
> >      VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev);
> >      VirtIODevice *vdev = proxy->vdev;
> > diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> > index 5ceebf1..97248a7 100644
> > --- a/hw/vmware_vga.c
> > +++ b/hw/vmware_vga.c
> > @@ -1173,7 +1173,7 @@ static int pci_vmsvga_load(QEMUFile *f, void *opaque, int version_id)
> >  }
> >  
> >  static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
> > -                uint32_t addr, uint32_t size, int type)
> > +                uint64_t addr, uint64_t size, int type)
> >  {
> >      struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
> >      struct vmsvga_state_s *s = &d->chip;
> > @@ -1193,7 +1193,7 @@ static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
> >  }
> >  
> >  static void pci_vmsvga_map_mem(PCIDevice *pci_dev, int region_num,
> > -                uint32_t addr, uint32_t size, int type)
> > +                uint64_t addr, uint64_t size, int type)
> >  {
> >      struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
> >      struct vmsvga_state_s *s = &d->chip;
> > diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
> > index 42642c7..678eb17 100644
> > --- a/hw/wdt_i6300esb.c
> > +++ b/hw/wdt_i6300esb.c
> > @@ -351,7 +351,7 @@ static void i6300esb_mem_writel(void *vp, target_phys_addr_t addr, uint32_t val)
> >  }
> >  
> >  static void i6300esb_map(PCIDevice *dev, int region_num,
> > -                         uint32_t addr, uint32_t size, int type)
> > +                         uint64_t addr, uint64_t size, int type)
> >  {
> >      static CPUReadMemoryFunc *mem_read[3] = {
> >          i6300esb_mem_readb,
> > @@ -366,7 +366,8 @@ static void i6300esb_map(PCIDevice *dev, int region_num,
> >      I6300State *d = (I6300State *) dev;
> >      int io_mem;
> >  
> > -    i6300esb_debug("addr = %x, size = %x, type = %d\n", addr, size, type);
> > +    i6300esb_debug("addr = %"PRIx64", size = %"PRIx64", type = %d\n",
> > +                   addr, size, type);
> >  
> >      io_mem = cpu_register_io_memory(mem_read, mem_write, d);
> >      cpu_register_physical_memory (addr, 0x10, io_mem);
> 

-- 
yamahata

[Qemu-devel] [PATCH 5/9] pci: 64bit bar support.

[Isaku Yamahata]

implemented pci 64bit bar support.

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

diff --git a/hw/pci.c b/hw/pci.c
index 2d985f0..9639a32 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -429,9 +429,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     } else {
         addr = 0x10 + region_num * 4;
     }
+
     *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
-    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
-    *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
+    if (pci_bar_is_64bit(r)) {
+        *(uint64_t *)(pci_dev->wmask + addr) = cpu_to_le64(wmask);
+        *(uint64_t *)(pci_dev->cmask + addr) = ~0ULL;
+    } else {
+        *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
+        *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
+    }
 }
 
 static void pci_update_mappings(PCIDevice *d)
@@ -466,8 +472,14 @@ static void pci_update_mappings(PCIDevice *d)
                 }
             } else {
                 if (cmd & PCI_COMMAND_MEMORY) {
-                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
-                                                         config_ofs));
+
+                    if (pci_bar_is_64bit(r)) {
+                        new_addr = le64_to_cpu(*(uint64_t *)(d->config +
+                                                             config_ofs));
+                    } else {
+                        new_addr = le32_to_cpu(*(uint32_t *)(d->config +
+                                                             config_ofs));
+                    }
                     /* the ROM slot has a specific enable bit */
                     if (i == PCI_ROM_SLOT && !(new_addr & 1))
                         goto no_mem_map;
@@ -482,7 +494,7 @@ static void pci_update_mappings(PCIDevice *d)
 
                         /* keep old behaviour
                          * without this, PC ide doesn't work well. */
-                        last_addr >= UINT32_MAX) {
+                        (!pci_bar_is_64bit(r) && last_addr >= UINT32_MAX)) {
                         new_addr = PCI_BAR_UNMAPPED;
                     }
                 } else {
@@ -741,7 +753,29 @@ static void pci_info_device(PCIDevice *d)
                 monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
                                r->addr, r->addr + r->size - 1);
             } else {
-                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
+                const char *type;
+                const char* prefetch;
+
+                switch (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) {
+                case PCI_ADDRESS_SPACE_MEM:
+                    type = "32 bit";
+                    break;
+                case PCI_ADDRESS_SPACE_MEM_64:
+                    type = "64 bit";
+                    break;
+                default:
+                    type = "unknown";
+                    break;
+                }
+
+                prefetch = "";
+                if (r->type & PCI_ADDRESS_SPACE_MEM_PREFETCH) {
+                    prefetch = " prefetchable";
+                }
+
+                monitor_printf(mon, "%s%s memory at "
+                               "0x%08"PRIx64" [0x%08"PRIx64"].\n",
+                               type, prefetch,
                                r->addr, r->addr + r->size - 1);
             }
         }
diff --git a/hw/pci.h b/hw/pci.h
index fee9ed6..33e2ef2 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -80,6 +80,8 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
 
 #define PCI_ADDRESS_SPACE_MEM		0x00
 #define PCI_ADDRESS_SPACE_IO		0x01
+#define PCI_ADDRESS_SPACE_MEM_64        0x04    /* 64 bit address */
+#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06
 #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
 
 typedef struct PCIIORegion {
@@ -90,6 +92,13 @@ typedef struct PCIIORegion {
     PCIMapIORegionFunc *map_func;
 } PCIIORegion;
 
+static inline int pci_bar_is_64bit(const PCIIORegion *r)
+{
+    return !(r->type & PCI_ADDRESS_SPACE_IO) &&
+        ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) ==
+         PCI_ADDRESS_SPACE_MEM_64);
+}
+
 #define PCI_ROM_SLOT 6
 #define PCI_NUM_REGIONS 7
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 5/9] pci: 64bit bar support.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:05PM +0900, Isaku Yamahata wrote:
> implemented pci 64bit bar support.

Minor style suggestions below.
Acked-by: Michael S. Tsirkin <mst@redhat.com>


> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   46 ++++++++++++++++++++++++++++++++++++++++------
>  hw/pci.h |    9 +++++++++
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2d985f0..9639a32 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -429,9 +429,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      } else {
>          addr = 0x10 + region_num * 4;
>      }
> +

don't add empty lines after } - block stands out enough already.

>      *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
> -    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
> -    *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
> +    if (pci_bar_is_64bit(r)) {
> +        *(uint64_t *)(pci_dev->wmask + addr) = cpu_to_le64(wmask);
> +        *(uint64_t *)(pci_dev->cmask + addr) = ~0ULL;
> +    } else {
> +        *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
> +        *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
> +    }
>  }
>  
>  static void pci_update_mappings(PCIDevice *d)
> @@ -466,8 +472,14 @@ static void pci_update_mappings(PCIDevice *d)
>                  }
>              } else {
>                  if (cmd & PCI_COMMAND_MEMORY) {
> -                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> -                                                         config_ofs));
> +
> +                    if (pci_bar_is_64bit(r)) {
> +                        new_addr = le64_to_cpu(*(uint64_t *)(d->config +
> +                                                             config_ofs));

Use pci_get_quad here?

> +                    } else {
> +                        new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> +                                                             config_ofs));
> +                    }
>                      /* the ROM slot has a specific enable bit */
>                      if (i == PCI_ROM_SLOT && !(new_addr & 1))
>                          goto no_mem_map;
> @@ -482,7 +494,7 @@ static void pci_update_mappings(PCIDevice *d)
>  
>                          /* keep old behaviour
>                           * without this, PC ide doesn't work well. */
> -                        last_addr >= UINT32_MAX) {
> +                        (!pci_bar_is_64bit(r) && last_addr >= UINT32_MAX)) {
>                          new_addr = PCI_BAR_UNMAPPED;
>                      }
>                  } else {
> @@ -741,7 +753,29 @@ static void pci_info_device(PCIDevice *d)
>                  monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
>                                 r->addr, r->addr + r->size - 1);
>              } else {
> -                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
> +                const char *type;
> +                const char* prefetch;
> +
> +                switch (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) {
> +                case PCI_ADDRESS_SPACE_MEM:
> +                    type = "32 bit";
> +                    break;
> +                case PCI_ADDRESS_SPACE_MEM_64:
> +                    type = "64 bit";
> +                    break;
> +                default:
> +                    type = "unknown";
> +                    break;
> +                }
> +
> +                prefetch = "";
> +                if (r->type & PCI_ADDRESS_SPACE_MEM_PREFETCH) {
> +                    prefetch = " prefetchable";
> +                }
> +
> +                monitor_printf(mon, "%s%s memory at "
> +                               "0x%08"PRIx64" [0x%08"PRIx64"].\n",
> +                               type, prefetch,
>                                 r->addr, r->addr + r->size - 1);
>              }
>          }
> diff --git a/hw/pci.h b/hw/pci.h
> index fee9ed6..33e2ef2 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -80,6 +80,8 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  
>  #define PCI_ADDRESS_SPACE_MEM		0x00
>  #define PCI_ADDRESS_SPACE_IO		0x01
> +#define PCI_ADDRESS_SPACE_MEM_64        0x04    /* 64 bit address */
> +#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06
>  #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
>  
>  typedef struct PCIIORegion {
> @@ -90,6 +92,13 @@ typedef struct PCIIORegion {
>      PCIMapIORegionFunc *map_func;
>  } PCIIORegion;
>  
> +static inline int pci_bar_is_64bit(const PCIIORegion *r)
> +{
> +    return !(r->type & PCI_ADDRESS_SPACE_IO) &&
> +        ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) ==
> +         PCI_ADDRESS_SPACE_MEM_64);
> +}
> +
>  #define PCI_ROM_SLOT 6
>  #define PCI_NUM_REGIONS 7
>  
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 5/9] pci: 64bit bar support.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:05PM +0900, Isaku Yamahata wrote:
> implemented pci 64bit bar support.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   46 ++++++++++++++++++++++++++++++++++++++++------
>  hw/pci.h |    9 +++++++++
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 2d985f0..9639a32 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -429,9 +429,15 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      } else {
>          addr = 0x10 + region_num * 4;
>      }
> +
>      *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
> -    *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
> -    *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
> +    if (pci_bar_is_64bit(r)) {
> +        *(uint64_t *)(pci_dev->wmask + addr) = cpu_to_le64(wmask);
> +        *(uint64_t *)(pci_dev->cmask + addr) = ~0ULL;
> +    } else {
> +        *(uint32_t *)(pci_dev->wmask + addr) = cpu_to_le32(wmask & 0xffffffff);
> +        *(uint32_t *)(pci_dev->cmask + addr) = 0xffffffff;
> +    }
>  }
>  
>  static void pci_update_mappings(PCIDevice *d)
> @@ -466,8 +472,14 @@ static void pci_update_mappings(PCIDevice *d)
>                  }
>              } else {
>                  if (cmd & PCI_COMMAND_MEMORY) {
> -                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> -                                                         config_ofs));
> +
> +                    if (pci_bar_is_64bit(r)) {
> +                        new_addr = le64_to_cpu(*(uint64_t *)(d->config +
> +                                                             config_ofs));
> +                    } else {
> +                        new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> +                                                             config_ofs));
> +                    }

shouldn't new_addr be uint64_t then?


>                      /* the ROM slot has a specific enable bit */
>                      if (i == PCI_ROM_SLOT && !(new_addr & 1))
>                          goto no_mem_map;
> @@ -482,7 +494,7 @@ static void pci_update_mappings(PCIDevice *d)
>  
>                          /* keep old behaviour
>                           * without this, PC ide doesn't work well. */
> -                        last_addr >= UINT32_MAX) {
> +                        (!pci_bar_is_64bit(r) && last_addr >= UINT32_MAX)) {
>                          new_addr = PCI_BAR_UNMAPPED;
>                      }
>                  } else {
> @@ -741,7 +753,29 @@ static void pci_info_device(PCIDevice *d)
>                  monitor_printf(mon, "I/O at 0x%04"PRIx64" [0x%04"PRIx64"].\n",
>                                 r->addr, r->addr + r->size - 1);
>              } else {
> -                monitor_printf(mon, "32 bit memory at 0x%08"PRIx64" [0x%08"PRIx64"].\n",
> +                const char *type;
> +                const char* prefetch;
> +
> +                switch (r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) {
> +                case PCI_ADDRESS_SPACE_MEM:
> +                    type = "32 bit";
> +                    break;
> +                case PCI_ADDRESS_SPACE_MEM_64:
> +                    type = "64 bit";
> +                    break;
> +                default:
> +                    type = "unknown";
> +                    break;
> +                }
> +
> +                prefetch = "";
> +                if (r->type & PCI_ADDRESS_SPACE_MEM_PREFETCH) {
> +                    prefetch = " prefetchable";
> +                }
> +
> +                monitor_printf(mon, "%s%s memory at "
> +                               "0x%08"PRIx64" [0x%08"PRIx64"].\n",
> +                               type, prefetch,
>                                 r->addr, r->addr + r->size - 1);
>              }
>          }
> diff --git a/hw/pci.h b/hw/pci.h
> index fee9ed6..33e2ef2 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -80,6 +80,8 @@ typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
>  
>  #define PCI_ADDRESS_SPACE_MEM		0x00
>  #define PCI_ADDRESS_SPACE_IO		0x01
> +#define PCI_ADDRESS_SPACE_MEM_64        0x04    /* 64 bit address */
> +#define PCI_ADDRESS_SPACE_MEM_TYPE_MASK 0x06
>  #define PCI_ADDRESS_SPACE_MEM_PREFETCH	0x08
>  
>  typedef struct PCIIORegion {
> @@ -90,6 +92,13 @@ typedef struct PCIIORegion {
>      PCIMapIORegionFunc *map_func;
>  } PCIIORegion;
>  
> +static inline int pci_bar_is_64bit(const PCIIORegion *r)
> +{
> +    return !(r->type & PCI_ADDRESS_SPACE_IO) &&
> +        ((r->type & PCI_ADDRESS_SPACE_MEM_TYPE_MASK) ==
> +         PCI_ADDRESS_SPACE_MEM_64);
> +}
> +
>  #define PCI_ROM_SLOT 6
>  #define PCI_NUM_REGIONS 7
>  
> -- 
> 1.6.0.2
> 
> 

[Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Isaku Yamahata]

factor out while(bus) bus->next loop logic into pci_find_bus_from()
which will be used later.

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

diff --git a/hw/pci.c b/hw/pci.c
index 9639a32..f06e1da 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
         pci_update_mappings(d);
 }
 
+static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
+{
+    PCIBus *s = from;
+
+    while (s && s->bus_num != bus_num)
+        s = s->next;
+
+    return s;
+}
+
 void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
 {
     PCIBus *s = opaque;
@@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
                 addr, val, len);
 #endif
     bus_num = (addr >> 16) & 0xff;
-    while (s && s->bus_num != bus_num)
-        s = s->next;
+    s = pci_find_bus_from(s, bus_num);
     if (!s)
         return;
     pci_dev = s->devices[(addr >> 8) & 0xff];
@@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
     uint32_t val;
 
     bus_num = (addr >> 16) & 0xff;
-    while (s && s->bus_num != bus_num)
-        s= s->next;
+    s = pci_find_bus_from(s, bus_num);
     if (!s)
         goto fail;
     pci_dev = s->devices[(addr >> 8) & 0xff];
@@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
     PCIDevice *d;
     int devfn;
 
-    while (bus && bus->bus_num != bus_num)
-        bus = bus->next;
+    bus = pci_find_bus_from(bus, bus_num);
     if (bus) {
         for(devfn = 0; devfn < 256; devfn++) {
             d = bus->devices[devfn];
@@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
 
 PCIBus *pci_find_bus(int bus_num)
 {
-    PCIBus *bus = first_bus;
-
-    while (bus && bus->bus_num != bus_num)
-        bus = bus->next;
-
-    return bus;
+    return pci_find_bus_from(first_bus, bus_num);
 }
 
 PCIDevice *pci_find_device(int bus_num, int slot, int function)
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> factor out while(bus) bus->next loop logic into pci_find_bus_from()
> which will be used later.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   26 ++++++++++++++------------
>  1 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9639a32..f06e1da 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>          pci_update_mappings(d);
>  }
>  
> +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)

Why what is "from"? pci_find_parent_bus a better name?

> +{
> +    PCIBus *s = from;
> +
> +    while (s && s->bus_num != bus_num)

No space after while.

> +        s = s->next;
> +
> +    return s;
> +}
> +
>  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>  {
>      PCIBus *s = opaque;
> @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
>                  addr, val, len);
>  #endif
>      bus_num = (addr >> 16) & 0xff;
> -    while (s && s->bus_num != bus_num)
> -        s = s->next;
> +    s = pci_find_bus_from(s, bus_num);
>      if (!s)
>          return;
>      pci_dev = s->devices[(addr >> 8) & 0xff];
> @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
>      uint32_t val;
>  
>      bus_num = (addr >> 16) & 0xff;
> -    while (s && s->bus_num != bus_num)
> -        s= s->next;
> +    s = pci_find_bus_from(s, bus_num);
>      if (!s)
>          goto fail;
>      pci_dev = s->devices[(addr >> 8) & 0xff];
> @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
>      PCIDevice *d;
>      int devfn;
>  
> -    while (bus && bus->bus_num != bus_num)
> -        bus = bus->next;
> +    bus = pci_find_bus_from(bus, bus_num);
>      if (bus) {
>          for(devfn = 0; devfn < 256; devfn++) {
>              d = bus->devices[devfn];
> @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
>  
>  PCIBus *pci_find_bus(int bus_num)
>  {
> -    PCIBus *bus = first_bus;
> -
> -    while (bus && bus->bus_num != bus_num)
> -        bus = bus->next;
> -
> -    return bus;
> +    return pci_find_bus_from(first_bus, bus_num);
>  }
>  
>  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > which will be used later.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |   26 ++++++++++++++------------
> >  1 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 9639a32..f06e1da 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >          pci_update_mappings(d);
> >  }
> >  
> > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> 
> Why what is "from"? pci_find_parent_bus a better name?

Its intention is to go down from a given parent bus
to find child bus of a given bus number, bus_num.

The current implementation arranges PCIBus in a single linked list,
and a single linked list doesn't represent pci bus topology well.
So it may find a pci bus which isn't child of a given bus.
So far it hasn't been a issue because only a single bus PCI.0 is supported.

Given that several people including me want multiple PCI bus,
your question arises that the linked list should be changed/enhanced to
some kind of tree structure to represent PCI bus topology accurately.
Does it sound a good idea?


> > +{
> > +    PCIBus *s = from;
> > +
> > +    while (s && s->bus_num != bus_num)
> 
> No space after while.
> 
> > +        s = s->next;
> > +
> > +    return s;
> > +}
> > +
> >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> >  {
> >      PCIBus *s = opaque;
> > @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> >                  addr, val, len);
> >  #endif
> >      bus_num = (addr >> 16) & 0xff;
> > -    while (s && s->bus_num != bus_num)
> > -        s = s->next;
> > +    s = pci_find_bus_from(s, bus_num);
> >      if (!s)
> >          return;
> >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> >      uint32_t val;
> >  
> >      bus_num = (addr >> 16) & 0xff;
> > -    while (s && s->bus_num != bus_num)
> > -        s= s->next;
> > +    s = pci_find_bus_from(s, bus_num);
> >      if (!s)
> >          goto fail;
> >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
> >      PCIDevice *d;
> >      int devfn;
> >  
> > -    while (bus && bus->bus_num != bus_num)
> > -        bus = bus->next;
> > +    bus = pci_find_bus_from(bus, bus_num);
> >      if (bus) {
> >          for(devfn = 0; devfn < 256; devfn++) {
> >              d = bus->devices[devfn];
> > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> >  
> >  PCIBus *pci_find_bus(int bus_num)
> >  {
> > -    PCIBus *bus = first_bus;
> > -
> > -    while (bus && bus->bus_num != bus_num)
> > -        bus = bus->next;
> > -
> > -    return bus;
> > +    return pci_find_bus_from(first_bus, bus_num);
> >  }
> >  
> >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Michael S. Tsirkin]

On Thu, Oct 01, 2009 at 12:29:34PM +0900, Isaku Yamahata wrote:
> On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > > which will be used later.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   26 ++++++++++++++------------
> > >  1 files changed, 14 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 9639a32..f06e1da 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > >          pci_update_mappings(d);
> > >  }
> > >  
> > > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > 
> > Why what is "from"? pci_find_parent_bus a better name?
> 
> Its intention is to go down from a given parent bus
> to find child bus of a given bus number, bus_num.
> The current implementation arranges PCIBus in a single linked list,
> and a single linked list doesn't represent pci bus topology well.
> So it may find a pci bus which isn't child of a given bus.

Sounds very messy. Let's try to define sane semantics, otherwise maybe
we don't want to factor this out in a function after all.

> So far it hasn't been a issue because only a single bus PCI.0 is supported.
> 
> Given that several people including me want multiple PCI bus,
> your question arises that the linked list should be changed/enhanced to
> some kind of tree structure to represent PCI bus topology accurately.
> Does it sound a good idea?

I won't be surprised if we need a tree to represent PCI bus topology.
But we already have it, don't we: each bus lists devices behind it, some
of these could be buses.

OTOH a list of all buses is currently used to look up device by bus
number.  Since there are only 256 possible buses, how about an array indexing
buses by bus number? Will be simpler and cleaner than what we have,
and the api would be pci_find_bus(int bus_num) with no confusing from?


> 
> > > +{
> > > +    PCIBus *s = from;
> > > +
> > > +    while (s && s->bus_num != bus_num)
> > 
> > No space after while.
> > 
> > > +        s = s->next;
> > > +
> > > +    return s;
> > > +}
> > > +
> > >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > >  {
> > >      PCIBus *s = opaque;
> > > @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > >                  addr, val, len);
> > >  #endif
> > >      bus_num = (addr >> 16) & 0xff;
> > > -    while (s && s->bus_num != bus_num)
> > > -        s = s->next;
> > > +    s = pci_find_bus_from(s, bus_num);
> > >      if (!s)
> > >          return;
> > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > >      uint32_t val;
> > >  
> > >      bus_num = (addr >> 16) & 0xff;
> > > -    while (s && s->bus_num != bus_num)
> > > -        s= s->next;
> > > +    s = pci_find_bus_from(s, bus_num);
> > >      if (!s)
> > >          goto fail;
> > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
> > >      PCIDevice *d;
> > >      int devfn;
> > >  
> > > -    while (bus && bus->bus_num != bus_num)
> > > -        bus = bus->next;
> > > +    bus = pci_find_bus_from(bus, bus_num);
> > >      if (bus) {
> > >          for(devfn = 0; devfn < 256; devfn++) {
> > >              d = bus->devices[devfn];
> > > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> > >  
> > >  PCIBus *pci_find_bus(int bus_num)
> > >  {
> > > -    PCIBus *bus = first_bus;
> > > -
> > > -    while (bus && bus->bus_num != bus_num)
> > > -        bus = bus->next;
> > > -
> > > -    return bus;
> > > +    return pci_find_bus_from(first_bus, bus_num);
> > >  }
> > >  
> > >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> > 
> 
> -- 
> yamahata

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Isaku Yamahata]

On Thu, Oct 01, 2009 at 08:28:26AM +0200, Michael S. Tsirkin wrote:
> On Thu, Oct 01, 2009 at 12:29:34PM +0900, Isaku Yamahata wrote:
> > On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > > > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > > > which will be used later.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > ---
> > > >  hw/pci.c |   26 ++++++++++++++------------
> > > >  1 files changed, 14 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index 9639a32..f06e1da 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > > >          pci_update_mappings(d);
> > > >  }
> > > >  
> > > > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > > 
> > > Why what is "from"? pci_find_parent_bus a better name?
> > 
> > Its intention is to go down from a given parent bus
> > to find child bus of a given bus number, bus_num.
> > The current implementation arranges PCIBus in a single linked list,
> > and a single linked list doesn't represent pci bus topology well.
> > So it may find a pci bus which isn't child of a given bus.
> 
> Sounds very messy. Let's try to define sane semantics, otherwise maybe
> we don't want to factor this out in a function after all.
> 
> > So far it hasn't been a issue because only a single bus PCI.0 is supported.
> > 
> > Given that several people including me want multiple PCI bus,
> > your question arises that the linked list should be changed/enhanced to
> > some kind of tree structure to represent PCI bus topology accurately.
> > Does it sound a good idea?
> 
> I won't be surprised if we need a tree to represent PCI bus topology.
> But we already have it, don't we: each bus lists devices behind it, some
> of these could be buses.
> 
> OTOH a list of all buses is currently used to look up device by bus
> number.  Since there are only 256 possible buses, how about an array indexing
> buses by bus number? Will be simpler and cleaner than what we have,
> and the api would be pci_find_bus(int bus_num) with no confusing from?

Static array sound good. however I'm not sure bus number indexing
is appropriate because bus number can be changed.
Anyway I'll switch it from list to array.

thanks,

> > > > +{
> > > > +    PCIBus *s = from;
> > > > +
> > > > +    while (s && s->bus_num != bus_num)
> > > 
> > > No space after while.
> > > 
> > > > +        s = s->next;
> > > > +
> > > > +    return s;
> > > > +}
> > > > +
> > > >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > >  {
> > > >      PCIBus *s = opaque;
> > > > @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > >                  addr, val, len);
> > > >  #endif
> > > >      bus_num = (addr >> 16) & 0xff;
> > > > -    while (s && s->bus_num != bus_num)
> > > > -        s = s->next;
> > > > +    s = pci_find_bus_from(s, bus_num);
> > > >      if (!s)
> > > >          return;
> > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > > >      uint32_t val;
> > > >  
> > > >      bus_num = (addr >> 16) & 0xff;
> > > > -    while (s && s->bus_num != bus_num)
> > > > -        s= s->next;
> > > > +    s = pci_find_bus_from(s, bus_num);
> > > >      if (!s)
> > > >          goto fail;
> > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
> > > >      PCIDevice *d;
> > > >      int devfn;
> > > >  
> > > > -    while (bus && bus->bus_num != bus_num)
> > > > -        bus = bus->next;
> > > > +    bus = pci_find_bus_from(bus, bus_num);
> > > >      if (bus) {
> > > >          for(devfn = 0; devfn < 256; devfn++) {
> > > >              d = bus->devices[devfn];
> > > > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> > > >  
> > > >  PCIBus *pci_find_bus(int bus_num)
> > > >  {
> > > > -    PCIBus *bus = first_bus;
> > > > -
> > > > -    while (bus && bus->bus_num != bus_num)
> > > > -        bus = bus->next;
> > > > -
> > > > -    return bus;
> > > > +    return pci_find_bus_from(first_bus, bus_num);
> > > >  }
> > > >  
> > > >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> > > 
> > 
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Michael S. Tsirkin]

On Thu, Oct 01, 2009 at 04:00:59PM +0900, Isaku Yamahata wrote:
> On Thu, Oct 01, 2009 at 08:28:26AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Oct 01, 2009 at 12:29:34PM +0900, Isaku Yamahata wrote:
> > > On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > > > > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > > > > which will be used later.
> > > > > 
> > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > > > ---
> > > > >  hw/pci.c |   26 ++++++++++++++------------
> > > > >  1 files changed, 14 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > > index 9639a32..f06e1da 100644
> > > > > --- a/hw/pci.c
> > > > > +++ b/hw/pci.c
> > > > > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > > > >          pci_update_mappings(d);
> > > > >  }
> > > > >  
> > > > > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > > > 
> > > > Why what is "from"? pci_find_parent_bus a better name?
> > > 
> > > Its intention is to go down from a given parent bus
> > > to find child bus of a given bus number, bus_num.
> > > The current implementation arranges PCIBus in a single linked list,
> > > and a single linked list doesn't represent pci bus topology well.
> > > So it may find a pci bus which isn't child of a given bus.
> > 
> > Sounds very messy. Let's try to define sane semantics, otherwise maybe
> > we don't want to factor this out in a function after all.
> > 
> > > So far it hasn't been a issue because only a single bus PCI.0 is supported.
> > > 
> > > Given that several people including me want multiple PCI bus,
> > > your question arises that the linked list should be changed/enhanced to
> > > some kind of tree structure to represent PCI bus topology accurately.
> > > Does it sound a good idea?
> > 
> > I won't be surprised if we need a tree to represent PCI bus topology.
> > But we already have it, don't we: each bus lists devices behind it, some
> > of these could be buses.
> > 
> > OTOH a list of all buses is currently used to look up device by bus
> > number.  Since there are only 256 possible buses, how about an array indexing
> > buses by bus number? Will be simpler and cleaner than what we have,
> > and the api would be pci_find_bus(int bus_num) with no confusing from?
> 
> Static array sound good. however I'm not sure bus number indexing
> is appropriate because bus number can be changed.
> 
> Anyway I'll switch it from list to array.

By the way, bus number might not technically be unique: a child bus
not reacheable by number from the root might have the same number
as another one reacheable: set secondary bus number to 1, subordinate 
bus number to 2, and you can have child bus with number 3 behind it
which is not reacheable.

So my idea was that we should have an array indexed by bus number for
reacheable child busses.  This array would be used only for fast address
decoding.  Each time you change some bus numbers, scan the tree and
rebuild the whole array from scratch.


> thanks,
> 
> > > > > +{
> > > > > +    PCIBus *s = from;
> > > > > +
> > > > > +    while (s && s->bus_num != bus_num)
> > > > 
> > > > No space after while.
> > > > 
> > > > > +        s = s->next;
> > > > > +
> > > > > +    return s;
> > > > > +}
> > > > > +
> > > > >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > > >  {
> > > > >      PCIBus *s = opaque;
> > > > > @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > > >                  addr, val, len);
> > > > >  #endif
> > > > >      bus_num = (addr >> 16) & 0xff;
> > > > > -    while (s && s->bus_num != bus_num)
> > > > > -        s = s->next;
> > > > > +    s = pci_find_bus_from(s, bus_num);
> > > > >      if (!s)
> > > > >          return;
> > > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > > > >      uint32_t val;
> > > > >  
> > > > >      bus_num = (addr >> 16) & 0xff;
> > > > > -    while (s && s->bus_num != bus_num)
> > > > > -        s= s->next;
> > > > > +    s = pci_find_bus_from(s, bus_num);
> > > > >      if (!s)
> > > > >          goto fail;
> > > > >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > > > > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d))
> > > > >      PCIDevice *d;
> > > > >      int devfn;
> > > > >  
> > > > > -    while (bus && bus->bus_num != bus_num)
> > > > > -        bus = bus->next;
> > > > > +    bus = pci_find_bus_from(bus, bus_num);
> > > > >      if (bus) {
> > > > >          for(devfn = 0; devfn < 256; devfn++) {
> > > > >              d = bus->devices[devfn];
> > > > > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> > > > >  
> > > > >  PCIBus *pci_find_bus(int bus_num)
> > > > >  {
> > > > > -    PCIBus *bus = first_bus;
> > > > > -
> > > > > -    while (bus && bus->bus_num != bus_num)
> > > > > -        bus = bus->next;
> > > > > -
> > > > > -    return bus;
> > > > > +    return pci_find_bus_from(first_bus, bus_num);
> > > > >  }
> > > > >  
> > > > >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> > > > 
> > > 
> > 
> 
> -- 
> yamahata

Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Gerd Hoffmann]

On 10/01/09 08:28, Michael S. Tsirkin wrote:
> On Thu, Oct 01, 2009 at 12:29:34PM +0900, Isaku Yamahata wrote:
>>>> +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
>>>
>>> Why what is "from"? pci_find_parent_bus a better name?
>>
>> Its intention is to go down from a given parent bus
>> to find child bus of a given bus number, bus_num.
>> The current implementation arranges PCIBus in a single linked list,
>> and a single linked list doesn't represent pci bus topology well.
>> So it may find a pci bus which isn't child of a given bus.

Can we have two pci busses with the same bus number in a system?

> I won't be surprised if we need a tree to represent PCI bus topology.
> But we already have it, don't we: each bus lists devices behind it, some
> of these could be buses.

Yes, the qdev device tree has all the needed info already.

cheers,
   Gerd

[Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address.

[Isaku Yamahata]

factor out conversion logic from io port address into bus+dev+func
with bit shift operation and conversion bus+dev+func into pci device.
They will be used later.
This patch also eliminates the logic duplication.

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

diff --git a/hw/pci.c b/hw/pci.c
index f06e1da..3b19c3d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -584,71 +584,82 @@ static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
     return s;
 }
 
-void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
+static PCIDevice *pci_bdf_to_dev(PCIBus *s, int bus_num, unsigned int devfn)
 {
-    PCIBus *s = opaque;
-    PCIDevice *pci_dev;
-    int config_addr, bus_num;
-
-#if 0
-    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
-                addr, val, len);
-#endif
-    bus_num = (addr >> 16) & 0xff;
     s = pci_find_bus_from(s, bus_num);
     if (!s)
-        return;
-    pci_dev = s->devices[(addr >> 8) & 0xff];
+        return NULL;
+
+    return s->devices[devfn];
+}
+
+static void pci_dev_data_write(PCIDevice *pci_dev,
+                               uint32_t config_addr, uint32_t val, int len)
+{
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev)
         return;
-    config_addr = addr & 0xff;
-    PCI_DPRINTF("pci_config_write: %s: "
-                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
-                pci_dev->name, config_addr, val, len);
+
+    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
+                __func__, pci_dev->name, config_addr, val, len);
     pci_dev->config_write(pci_dev, config_addr, val, len);
 }
 
-uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
+static uint32_t pci_dev_data_read(PCIDevice *pci_dev,
+                                  uint32_t config_addr, int len)
 {
-    PCIBus *s = opaque;
-    PCIDevice *pci_dev;
-    int config_addr, bus_num;
     uint32_t val;
 
-    bus_num = (addr >> 16) & 0xff;
-    s = pci_find_bus_from(s, bus_num);
-    if (!s)
-        goto fail;
-    pci_dev = s->devices[(addr >> 8) & 0xff];
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-    fail:
-        switch(len) {
-        case 1:
-            val = 0xff;
-            break;
-        case 2:
-            val = 0xffff;
-            break;
-        default:
-        case 4:
-            val = 0xffffffff;
-            break;
-        }
-        goto the_end;
+        val = (1 << (len * 8)) - 1;
+    } 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);
     }
-    config_addr = addr & 0xff;
-    val = pci_dev->config_read(pci_dev, config_addr, len);
-    PCI_DPRINTF("pci_config_read: %s: "
-                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                pci_dev->name, config_addr, val, len);
- the_end:
+
 #if 0
-    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
-                addr, val, len);
+    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
+                __func__, addr, val, len);
 #endif
     return val;
 }
 
+static void pci_addr_to_dev(PCIBus *s, uint32_t addr,
+                            PCIDevice **pci_dev, uint32_t *config_addr)
+{
+    int bus_num = (addr >> 16) & 0xff;
+    unsigned int devfn = (addr >> 8) & 0xff;
+
+    *pci_dev = pci_bdf_to_dev(s, bus_num, devfn);
+    *config_addr = addr & 0xff;
+}
+
+void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
+{
+    PCIBus *s = opaque;
+    PCIDevice *pci_dev;
+    uint32_t config_addr;
+
+#if 0
+    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
+                addr, val, len);
+#endif
+
+    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
+    pci_dev_data_write(pci_dev, config_addr, val, len);
+}
+
+uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
+{
+    PCIBus *s = opaque;
+    PCIDevice *pci_dev;
+    uint32_t config_addr;
+
+    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
+    return pci_dev_data_read(pci_dev, config_addr, len);
+}
 /***********************************************************/
 /* generic PCI irq support */
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:07PM +0900, Isaku Yamahata wrote:
> factor out conversion logic from io port address into bus+dev+func
> with bit shift operation and conversion bus+dev+func into pci device.
> They will be used later.
> This patch also eliminates the logic duplication.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |  105 ++++++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 58 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index f06e1da..3b19c3d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -584,71 +584,82 @@ static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
>      return s;
>  }
>  
> -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +static PCIDevice *pci_bdf_to_dev(PCIBus *s, int bus_num, unsigned int devfn)

This name is pretty bad.

>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
> -
> -#if 0
> -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> -#endif
> -    bus_num = (addr >> 16) & 0xff;
>      s = pci_find_bus_from(s, bus_num);
>      if (!s)
> -        return;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +        return NULL;
> +
> +    return s->devices[devfn];
> +}
> +static void pci_dev_data_write(PCIDevice *pci_dev,
> +                               uint32_t config_addr, uint32_t val, int len)


There seems to be 1 caller, and it's confusing to
have 2 functions with basically the same nhame.
Please open-code it.

> +{
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev)
>          return;
> -    config_addr = addr & 0xff;
> -    PCI_DPRINTF("pci_config_write: %s: "
> -                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> +
> +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> +                __func__, pci_dev->name, config_addr, val, len);
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
>  
> -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +static uint32_t pci_dev_data_read(PCIDevice *pci_dev,
> +                                  uint32_t config_addr, int len)
>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
>      uint32_t val;
>  
> -    bus_num = (addr >> 16) & 0xff;
> -    s = pci_find_bus_from(s, bus_num);
> -    if (!s)
> -        goto fail;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -    fail:
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> -        goto the_end;
> +        val = (1 << (len * 8)) - 1;

You can't do this: len * 8 will be 32 if len is 4,
and shift by 32 is undefined in C.

> +    } 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);
>      }
> -    config_addr = addr & 0xff;
> -    val = pci_dev->config_read(pci_dev, config_addr, len);
> -    PCI_DPRINTF("pci_config_read: %s: "
> -                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> - the_end:
> +
>  #if 0
> -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, addr, val, len);
>  #endif
>      return val;
>  }
>  
> +static void pci_addr_to_dev(PCIBus *s, uint32_t addr,
> +                            PCIDevice **pci_dev, uint32_t *config_addr)
> +{
> +    int bus_num = (addr >> 16) & 0xff;
> +    unsigned int devfn = (addr >> 8) & 0xff;
> +
> +    *pci_dev = pci_bdf_to_dev(s, bus_num, devfn);
> +    *config_addr = addr & 0xff;
> +}

Just open-code this function.
If you really must, add ADDR_TO_DEVFN and ADDR_TO_BUS macros.

> +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +#if 0
> +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                addr, val, len);
> +#endif
> +
> +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);

It's better to have 2 incline functions that return a single
value each here.

> +    pci_dev_data_write(pci_dev, config_addr, val, len);
> +}
> +
> +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
> +    return pci_dev_data_read(pci_dev, config_addr, len);
> +}
>  /***********************************************************/
>  /* generic PCI irq support */
>  
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address.

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:30:01PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2009 at 08:15:07PM +0900, Isaku Yamahata wrote:
> > factor out conversion logic from io port address into bus+dev+func
> > with bit shift operation and conversion bus+dev+func into pci device.
> > They will be used later.
> > This patch also eliminates the logic duplication.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |  105 ++++++++++++++++++++++++++++++++++---------------------------
> >  1 files changed, 58 insertions(+), 47 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index f06e1da..3b19c3d 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -584,71 +584,82 @@ static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> >      return s;
> >  }
> >  
> > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > +static PCIDevice *pci_bdf_to_dev(PCIBus *s, int bus_num, unsigned int devfn)
> 
> This name is pretty bad.

pci_find_device() is almost same function.
Will replace it with pci_find_device().

> 
> >  {
> > -    PCIBus *s = opaque;
> > -    PCIDevice *pci_dev;
> > -    int config_addr, bus_num;
> > -
> > -#if 0
> > -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                addr, val, len);
> > -#endif
> > -    bus_num = (addr >> 16) & 0xff;
> >      s = pci_find_bus_from(s, bus_num);
> >      if (!s)
> > -        return;
> > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > +        return NULL;
> > +
> > +    return s->devices[devfn];
> > +}
> > +static void pci_dev_data_write(PCIDevice *pci_dev,
> > +                               uint32_t config_addr, uint32_t val, int len)
> 
> 
> There seems to be 1 caller, and it's confusing to
> have 2 functions with basically the same nhame.
> Please open-code it.

OK


> > +{
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev)
> >          return;
> > -    config_addr = addr & 0xff;
> > -    PCI_DPRINTF("pci_config_write: %s: "
> > -                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> > -                pci_dev->name, config_addr, val, len);
> > +
> > +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> > +                __func__, pci_dev->name, config_addr, val, len);
> >      pci_dev->config_write(pci_dev, config_addr, val, len);
> >  }
> >  
> > -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > +static uint32_t pci_dev_data_read(PCIDevice *pci_dev,
> > +                                  uint32_t config_addr, int len)
> >  {
> > -    PCIBus *s = opaque;
> > -    PCIDevice *pci_dev;
> > -    int config_addr, bus_num;
> >      uint32_t val;
> >  
> > -    bus_num = (addr >> 16) & 0xff;
> > -    s = pci_find_bus_from(s, bus_num);
> > -    if (!s)
> > -        goto fail;
> > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev) {
> > -    fail:
> > -        switch(len) {
> > -        case 1:
> > -            val = 0xff;
> > -            break;
> > -        case 2:
> > -            val = 0xffff;
> > -            break;
> > -        default:
> > -        case 4:
> > -            val = 0xffffffff;
> > -            break;
> > -        }
> > -        goto the_end;
> > +        val = (1 << (len * 8)) - 1;
> 
> You can't do this: len * 8 will be 32 if len is 4,
> and shift by 32 is undefined in C.

Oh silly me. Nice catch.


> > +    } 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);
> >      }
> > -    config_addr = addr & 0xff;
> > -    val = pci_dev->config_read(pci_dev, config_addr, len);
> > -    PCI_DPRINTF("pci_config_read: %s: "
> > -                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                pci_dev->name, config_addr, val, len);
> > - the_end:
> > +
> >  #if 0
> > -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                addr, val, len);
> > +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                __func__, addr, val, len);
> >  #endif
> >      return val;
> >  }
> >  
> > +static void pci_addr_to_dev(PCIBus *s, uint32_t addr,
> > +                            PCIDevice **pci_dev, uint32_t *config_addr)
> > +{
> > +    int bus_num = (addr >> 16) & 0xff;
> > +    unsigned int devfn = (addr >> 8) & 0xff;
> > +
> > +    *pci_dev = pci_bdf_to_dev(s, bus_num, devfn);
> > +    *config_addr = addr & 0xff;
> > +}
> 
> Just open-code this function.
> If you really must, add ADDR_TO_DEVFN and ADDR_TO_BUS macros.
> 
> > +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > +{
> > +    PCIBus *s = opaque;
> > +    PCIDevice *pci_dev;
> > +    uint32_t config_addr;
> > +
> > +#if 0
> > +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                addr, val, len);
> > +#endif
> > +
> > +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
> 
> It's better to have 2 incline functions that return a single
> value each here.

Will introduce 2 static inline functions.


> > +    pci_dev_data_write(pci_dev, config_addr, val, len);
> > +}
> > +
> > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > +{
> > +    PCIBus *s = opaque;
> > +    PCIDevice *pci_dev;
> > +    uint32_t config_addr;
> > +
> > +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
> > +    return pci_dev_data_read(pci_dev, config_addr, len);
> > +}
> >  /***********************************************************/
> >  /* generic PCI irq support */
> >  
> 

-- 
yamahata

[Qemu-devel] [PATCH 8/9] pci_host.h: split non-inline static function in pci_host.h into pci_host_c.h

[Isaku Yamahata]

Later a structures declared in pci_host.h, PCIHostState, will be used.
However pci_host.h doesn't allow to include itself easily. This patches
addresses it.

pci_host.h includes non-inline static functions which are instantiated
in .c by including pci_host.h. That prevents from including pci_host.h
to use PCIHostState.
So split pci_host.h non-inline static functions into pci_host_c.h.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/apb_pci.c                    |    2 +-
 hw/grackle_pci.c                |    2 +-
 hw/gt64xxx.c                    |    2 +-
 hw/pci_host.h                   |   88 +-------------------------------------
 hw/{pci_host.h => pci_host_c.h} |    8 +---
 hw/piix_pci.c                   |    2 +-
 hw/ppc4xx_pci.c                 |    2 +-
 hw/ppce500_pci.c                |    2 +-
 hw/prep_pci.c                   |    2 +-
 hw/unin_pci.c                   |    2 +-
 10 files changed, 12 insertions(+), 100 deletions(-)
 copy hw/{pci_host.h => pci_host_c.h} (96%)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 5f411aa..9732159 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -40,7 +40,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
 #endif
 
 typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 typedef PCIHostState APBState;
 
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index fde9fea..87ab1b4 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -38,7 +38,7 @@
 #endif
 
 typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 typedef PCIHostState GrackleState;
 
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 3b44fc9..b3da0fa 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -28,7 +28,7 @@
 #include "pc.h"
 
 typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 //#define DEBUG
 
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 48862b5..9f272a7 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -25,97 +25,15 @@
 /* Worker routines for a PCI host controller that uses an {address,data}
    register pair to access PCI configuration space.  */
 
-/* debug PCI */
-//#define DEBUG_PCI
+#ifndef PCI_HOST_H
+#define PCI_HOST_H
 
 #include "sysbus.h"
 
-#ifdef DEBUG_PCI
-#define PCI_DPRINTF(fmt, ...) \
-do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define PCI_DPRINTF(fmt, ...)
-#endif
-
 typedef struct {
     SysBusDevice busdev;
     uint32_t config_reg;
     PCIBus *bus;
 } PCIHostState;
 
-static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-
-    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
-}
-
-static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
-}
-
-static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg, val, 4);
-}
-
-static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-
-    if (!(s->config_reg & (1 << 31)))
-        return 0xff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
-    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    return val;
-}
-
-static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
-        return 0xffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
-    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    return val;
-}
-
-static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
-        return 0xffffffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
-    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    return val;
-}
+#endif /* PCI_HOST_H */
diff --git a/hw/pci_host.h b/hw/pci_host_c.h
similarity index 96%
copy from hw/pci_host.h
copy to hw/pci_host_c.h
index 48862b5..fcd7e6e 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host_c.h
@@ -28,7 +28,7 @@
 /* debug PCI */
 //#define DEBUG_PCI
 
-#include "sysbus.h"
+#include "pci_host.h"
 
 #ifdef DEBUG_PCI
 #define PCI_DPRINTF(fmt, ...) \
@@ -37,12 +37,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
 #define PCI_DPRINTF(fmt, ...)
 #endif
 
-typedef struct {
-    SysBusDevice busdev;
-    uint32_t config_reg;
-    PCIBus *bus;
-} PCIHostState;
-
 static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
 {
     PCIHostState *s = opaque;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index a5d42d1..e67a7dd 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -28,7 +28,7 @@
 #include "sysbus.h"
 
 typedef uint32_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 typedef PCIHostState I440FXState;
 
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 87c44f8..27ea87e 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -26,7 +26,7 @@
 
 typedef target_phys_addr_t pci_addr_t;
 #include "pci.h"
-#include "pci_host.h"
+#include "pci_host_c.h"
 #include "bswap.h"
 
 #undef DEBUG
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 1a8a6c9..9258524 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -19,7 +19,7 @@
 #include "ppce500.h"
 typedef target_phys_addr_t pci_addr_t;
 #include "pci.h"
-#include "pci_host.h"
+#include "pci_host_c.h"
 #include "bswap.h"
 #include "qemu-log.h"
 
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 80058b1..7051417 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -26,7 +26,7 @@
 #include "pci.h"
 
 typedef uint32_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 typedef PCIHostState PREPPCIState;
 
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 0ad0cd3..4d44008 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -36,7 +36,7 @@
 #endif
 
 typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
+#include "pci_host_c.h"
 
 typedef PCIHostState UNINState;
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 8/9] pci_host.h: split non-inline static function in pci_host.h into pci_host_c.h

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:08PM +0900, Isaku Yamahata wrote:
> Later a structures declared in pci_host.h, PCIHostState, will be used.
> However pci_host.h doesn't allow to include itself easily. This patches
> addresses it.
> 
> pci_host.h includes non-inline static functions which are instantiated
> in .c by including pci_host.h. That prevents from including pci_host.h
> to use PCIHostState.
> So split pci_host.h non-inline static functions into pci_host_c.h.

This is quite ugly. We need to split the users properly, and put c in a
.c file, not in an _c.h file.  If you can't do that now, just mark the
extra stuff inline, compiler can ignore it.

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/apb_pci.c                    |    2 +-
>  hw/grackle_pci.c                |    2 +-
>  hw/gt64xxx.c                    |    2 +-
>  hw/pci_host.h                   |   88 +-------------------------------------
>  hw/{pci_host.h => pci_host_c.h} |    8 +---
>  hw/piix_pci.c                   |    2 +-
>  hw/ppc4xx_pci.c                 |    2 +-
>  hw/ppce500_pci.c                |    2 +-
>  hw/prep_pci.c                   |    2 +-
>  hw/unin_pci.c                   |    2 +-
>  10 files changed, 12 insertions(+), 100 deletions(-)
>  copy hw/{pci_host.h => pci_host_c.h} (96%)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 5f411aa..9732159 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -40,7 +40,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
>  #endif
>  
>  typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  typedef PCIHostState APBState;
>  
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index fde9fea..87ab1b4 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -38,7 +38,7 @@
>  #endif
>  
>  typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  typedef PCIHostState GrackleState;
>  
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index 3b44fc9..b3da0fa 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -28,7 +28,7 @@
>  #include "pc.h"
>  
>  typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  //#define DEBUG
>  
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 48862b5..9f272a7 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -25,97 +25,15 @@
>  /* Worker routines for a PCI host controller that uses an {address,data}
>     register pair to access PCI configuration space.  */
>  
> -/* debug PCI */
> -//#define DEBUG_PCI
> +#ifndef PCI_HOST_H
> +#define PCI_HOST_H
>  
>  #include "sysbus.h"
>  
> -#ifdef DEBUG_PCI
> -#define PCI_DPRINTF(fmt, ...) \
> -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define PCI_DPRINTF(fmt, ...)
> -#endif
> -
>  typedef struct {
>      SysBusDevice busdev;
>      uint32_t config_reg;
>      PCIBus *bus;
>  } PCIHostState;
>  
> -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -
> -    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
> -}
> -
> -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap16(val);
> -#endif
> -    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
> -}
> -
> -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg, val, 4);
> -}
> -
> -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
> -    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    return val;
> -}
> -
> -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
> -    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap16(val);
> -#endif
> -    return val;
> -}
> -
> -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xffffffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
> -    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    return val;
> -}
> +#endif /* PCI_HOST_H */
> diff --git a/hw/pci_host.h b/hw/pci_host_c.h
> similarity index 96%
> copy from hw/pci_host.h
> copy to hw/pci_host_c.h
> index 48862b5..fcd7e6e 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host_c.h
> @@ -28,7 +28,7 @@
>  /* debug PCI */
>  //#define DEBUG_PCI
>  
> -#include "sysbus.h"
> +#include "pci_host.h"
>  
>  #ifdef DEBUG_PCI
>  #define PCI_DPRINTF(fmt, ...) \
> @@ -37,12 +37,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>  #define PCI_DPRINTF(fmt, ...)
>  #endif
>  
> -typedef struct {
> -    SysBusDevice busdev;
> -    uint32_t config_reg;
> -    PCIBus *bus;
> -} PCIHostState;
> -
>  static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index a5d42d1..e67a7dd 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -28,7 +28,7 @@
>  #include "sysbus.h"
>  
>  typedef uint32_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  typedef PCIHostState I440FXState;
>  
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 87c44f8..27ea87e 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -26,7 +26,7 @@
>  
>  typedef target_phys_addr_t pci_addr_t;
>  #include "pci.h"
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  #include "bswap.h"
>  
>  #undef DEBUG
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 1a8a6c9..9258524 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -19,7 +19,7 @@
>  #include "ppce500.h"
>  typedef target_phys_addr_t pci_addr_t;
>  #include "pci.h"
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  #include "bswap.h"
>  #include "qemu-log.h"
>  
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 80058b1..7051417 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -26,7 +26,7 @@
>  #include "pci.h"
>  
>  typedef uint32_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  typedef PCIHostState PREPPCIState;
>  
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 0ad0cd3..4d44008 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -36,7 +36,7 @@
>  #endif
>  
>  typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> +#include "pci_host_c.h"
>  
>  typedef PCIHostState UNINState;
>  
> -- 
> 1.6.0.2
> 
> 

Re: [Qemu-devel] [PATCH 8/9] pci_host.h: split non-inline static function in pci_host.h into pci_host_c.h

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:47:07PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2009 at 08:15:08PM +0900, Isaku Yamahata wrote:
> > Later a structures declared in pci_host.h, PCIHostState, will be used.
> > However pci_host.h doesn't allow to include itself easily. This patches
> > addresses it.
> > 
> > pci_host.h includes non-inline static functions which are instantiated
> > in .c by including pci_host.h. That prevents from including pci_host.h
> > to use PCIHostState.
> > So split pci_host.h non-inline static functions into pci_host_c.h.
> 
> This is quite ugly. We need to split the users properly, and put c in a
> .c file, not in an _c.h file.  If you can't do that now, just mark the
> extra stuff inline, compiler can ignore it.

Okay, then let's whole clean up.

> 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/apb_pci.c                    |    2 +-
> >  hw/grackle_pci.c                |    2 +-
> >  hw/gt64xxx.c                    |    2 +-
> >  hw/pci_host.h                   |   88 +-------------------------------------
> >  hw/{pci_host.h => pci_host_c.h} |    8 +---
> >  hw/piix_pci.c                   |    2 +-
> >  hw/ppc4xx_pci.c                 |    2 +-
> >  hw/ppce500_pci.c                |    2 +-
> >  hw/prep_pci.c                   |    2 +-
> >  hw/unin_pci.c                   |    2 +-
> >  10 files changed, 12 insertions(+), 100 deletions(-)
> >  copy hw/{pci_host.h => pci_host_c.h} (96%)
> > 
> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > index 5f411aa..9732159 100644
> > --- a/hw/apb_pci.c
> > +++ b/hw/apb_pci.c
> > @@ -40,7 +40,7 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
> >  #endif
> >  
> >  typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  typedef PCIHostState APBState;
> >  
> > diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> > index fde9fea..87ab1b4 100644
> > --- a/hw/grackle_pci.c
> > +++ b/hw/grackle_pci.c
> > @@ -38,7 +38,7 @@
> >  #endif
> >  
> >  typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  typedef PCIHostState GrackleState;
> >  
> > diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> > index 3b44fc9..b3da0fa 100644
> > --- a/hw/gt64xxx.c
> > +++ b/hw/gt64xxx.c
> > @@ -28,7 +28,7 @@
> >  #include "pc.h"
> >  
> >  typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  //#define DEBUG
> >  
> > diff --git a/hw/pci_host.h b/hw/pci_host.h
> > index 48862b5..9f272a7 100644
> > --- a/hw/pci_host.h
> > +++ b/hw/pci_host.h
> > @@ -25,97 +25,15 @@
> >  /* Worker routines for a PCI host controller that uses an {address,data}
> >     register pair to access PCI configuration space.  */
> >  
> > -/* debug PCI */
> > -//#define DEBUG_PCI
> > +#ifndef PCI_HOST_H
> > +#define PCI_HOST_H
> >  
> >  #include "sysbus.h"
> >  
> > -#ifdef DEBUG_PCI
> > -#define PCI_DPRINTF(fmt, ...) \
> > -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > -#else
> > -#define PCI_DPRINTF(fmt, ...)
> > -#endif
> > -
> >  typedef struct {
> >      SysBusDevice busdev;
> >      uint32_t config_reg;
> >      PCIBus *bus;
> >  } PCIHostState;
> >  
> > -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -
> > -    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
> > -}
> > -
> > -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap16(val);
> > -#endif
> > -    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
> > -}
> > -
> > -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap32(val);
> > -#endif
> > -    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg, val, 4);
> > -}
> > -
> > -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
> > -    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    return val;
> > -}
> > -
> > -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xffff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
> > -    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap16(val);
> > -#endif
> > -    return val;
> > -}
> > -
> > -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xffffffff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
> > -    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap32(val);
> > -#endif
> > -    return val;
> > -}
> > +#endif /* PCI_HOST_H */
> > diff --git a/hw/pci_host.h b/hw/pci_host_c.h
> > similarity index 96%
> > copy from hw/pci_host.h
> > copy to hw/pci_host_c.h
> > index 48862b5..fcd7e6e 100644
> > --- a/hw/pci_host.h
> > +++ b/hw/pci_host_c.h
> > @@ -28,7 +28,7 @@
> >  /* debug PCI */
> >  //#define DEBUG_PCI
> >  
> > -#include "sysbus.h"
> > +#include "pci_host.h"
> >  
> >  #ifdef DEBUG_PCI
> >  #define PCI_DPRINTF(fmt, ...) \
> > @@ -37,12 +37,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> >  #define PCI_DPRINTF(fmt, ...)
> >  #endif
> >  
> > -typedef struct {
> > -    SysBusDevice busdev;
> > -    uint32_t config_reg;
> > -    PCIBus *bus;
> > -} PCIHostState;
> > -
> >  static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> >  {
> >      PCIHostState *s = opaque;
> > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > index a5d42d1..e67a7dd 100644
> > --- a/hw/piix_pci.c
> > +++ b/hw/piix_pci.c
> > @@ -28,7 +28,7 @@
> >  #include "sysbus.h"
> >  
> >  typedef uint32_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  typedef PCIHostState I440FXState;
> >  
> > diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> > index 87c44f8..27ea87e 100644
> > --- a/hw/ppc4xx_pci.c
> > +++ b/hw/ppc4xx_pci.c
> > @@ -26,7 +26,7 @@
> >  
> >  typedef target_phys_addr_t pci_addr_t;
> >  #include "pci.h"
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  #include "bswap.h"
> >  
> >  #undef DEBUG
> > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> > index 1a8a6c9..9258524 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -19,7 +19,7 @@
> >  #include "ppce500.h"
> >  typedef target_phys_addr_t pci_addr_t;
> >  #include "pci.h"
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  #include "bswap.h"
> >  #include "qemu-log.h"
> >  
> > diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> > index 80058b1..7051417 100644
> > --- a/hw/prep_pci.c
> > +++ b/hw/prep_pci.c
> > @@ -26,7 +26,7 @@
> >  #include "pci.h"
> >  
> >  typedef uint32_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  typedef PCIHostState PREPPCIState;
> >  
> > diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> > index 0ad0cd3..4d44008 100644
> > --- a/hw/unin_pci.c
> > +++ b/hw/unin_pci.c
> > @@ -36,7 +36,7 @@
> >  #endif
> >  
> >  typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > +#include "pci_host_c.h"
> >  
> >  typedef PCIHostState UNINState;
> >  
> 

-- 
yamahata

[Qemu-devel] [PATCH 9/9] [RFC] pci: pcie host and mmcfg support.

[Isaku Yamahata]

This patch adds common routines for pcie host bridge and pcie mmcfg.
This will be used by q35 based chipset emulation.

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

diff --git a/hw/pci.c b/hw/pci.c
index 3b19c3d..eb761ce 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -23,6 +23,7 @@
  */
 #include "hw.h"
 #include "pci.h"
+#include "pci_host.h"
 #include "monitor.h"
 #include "net.h"
 #include "sysemu.h"
@@ -166,27 +167,32 @@ int pci_bus_num(PCIBus *s)
 void pci_device_save(PCIDevice *s, QEMUFile *f)
 {
     int i;
+    uint32_t config_size = pcie_config_size(s);
 
     qemu_put_be32(f, 2); /* PCI device version */
-    qemu_put_buffer(f, s->config, 256);
+    qemu_put_buffer(f, s->config, config_size);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
 }
 
 int pci_device_load(PCIDevice *s, QEMUFile *f)
 {
-    uint8_t config[PCI_CONFIG_SPACE_SIZE];
+    uint32_t config_size = pcie_config_size(s);
+    uint8_t *config;
     uint32_t version_id;
     int i;
 
     version_id = qemu_get_be32(f);
     if (version_id > 2)
         return -EINVAL;
-    qemu_get_buffer(f, config, sizeof config);
-    for (i = 0; i < sizeof config; ++i)
+
+    config = qemu_malloc(config_size);
+    qemu_get_buffer(f, config, config_size);
+    for (i = 0; i < config_size; ++i)
         if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i])
             return -EINVAL;
-    memcpy(s->config, config, sizeof config);
+    memcpy(s->config, config, config_size);
+    qemu_free(config);
 
     pci_update_mappings(s);
 
@@ -302,14 +308,31 @@ static void pci_init_cmask(PCIDevice *dev)
 static void pci_init_wmask(PCIDevice *dev)
 {
     int i;
+    uint32_t config_size = pcie_config_size(dev);
+
     dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
     dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
     dev->wmask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
                               | PCI_COMMAND_MASTER;
-    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
         dev->wmask[i] = 0xff;
 }
 
+static void pci_config_init(PCIDevice *pci_dev)
+{
+    int config_size = pcie_config_size(pci_dev);
+#define PCI_CONFIG_ALLOC(d, member, size)                               \
+    do {                                                                \
+        (d)->member =                                                   \
+            (typeof((d)->member))qemu_mallocz(sizeof((d)->member[0]) *  \
+                                              size);                    \
+    } while (0)
+    PCI_CONFIG_ALLOC(pci_dev, config, config_size);
+    PCI_CONFIG_ALLOC(pci_dev, cmask, config_size);
+    PCI_CONFIG_ALLOC(pci_dev, wmask, config_size);
+    PCI_CONFIG_ALLOC(pci_dev, used, config_size);
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn,
@@ -330,6 +353,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     pci_dev->devfn = devfn;
     pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
     memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
+    pci_config_init(pci_dev);
     pci_set_default_subsystem_id(pci_dev);
     pci_init_cmask(pci_dev);
     pci_init_wmask(pci_dev);
@@ -531,40 +555,48 @@ static void pci_update_mappings(PCIDevice *d)
     }
 }
 
+static uint8_t pcie_config_get_byte(PCIDevice *d, uint32_t addr)
+{
+    uint8_t *conf = &d->config[addr];
+    if (conf != NULL)
+        return *conf;
+    return 0;
+}
+
+static uint32_t pcie_config_get(PCIDevice *d, uint32_t addr, int len)
+{
+    int i;
+    union {
+        uint8_t val8[4];
+        uint32_t val32;
+    } v = { .val32 = 0 };
+
+    for (i = 0; i < len; i++) {
+        v.val8[i] = pcie_config_get_byte(d, addr + i);
+    }
+
+    return le32_to_cpu(v.val32);
+}
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
-    uint32_t val;
+    uint32_t config_size = pcie_config_size(d);
 
-    switch(len) {
-    default:
-    case 4:
-	if (address <= 0xfc) {
-	    val = le32_to_cpu(*(uint32_t *)(d->config + address));
-	    break;
-	}
-	/* fall through */
-    case 2:
-        if (address <= 0xfe) {
-	    val = le16_to_cpu(*(uint16_t *)(d->config + address));
-	    break;
-	}
-	/* fall through */
-    case 1:
-        val = d->config[address];
-        break;
-    }
-    return val;
+    assert(len == 1 || len == 2 || len == 4);
+    len = MIN(len, config_size - address);
+    return pcie_config_get(d, address, len);
 }
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
     uint8_t orig[PCI_CONFIG_SPACE_SIZE];
     int i;
+    uint32_t config_size = pcie_config_size(d);
 
     /* not efficient, but simple */
     memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
-    for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
+    for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
         uint8_t wmask = d->wmask[addr];
         d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
     }
@@ -660,6 +692,143 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
     pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
     return pci_dev_data_read(pci_dev, config_addr, len);
 }
+
+static void pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr,
+                                   PCIDevice **pci_dev, uint32_t *conf_addr)
+{
+    int bus_num;
+    unsigned int dev;
+    uint8_t func;
+
+#define PCIE_MASK(val, hi_bit, low_bit)                 \
+    (((val) & (((1ULL << (hi_bit)) - 1))) >> (low_bit))
+#define PCIE_VAL(VAL, val)                                              \
+    PCIE_MASK((val), PCIE_MMCFG_ ## VAL ## _HI, PCIE_MMCFG_ ## VAL ## _LOW)
+#define PCIE_MMCFG_BUS_HI               27
+#define PCIE_MMCFG_BUS_LOW              20
+#define PCIE_MMCFG_DEV_HI               19
+#define PCIE_MMCFG_DEV_LOW              15
+#define PCIE_MMCFG_FUNC_HI              14
+#define PCIE_MMCFG_FUNC_LOW             12
+#define PCIE_MMCFG_CONFADDR_HI          11
+#define PCIE_MMCFG_CONFADDR_LOW         0
+#define PCIE_MMCFG_BUS(addr)            PCIE_VAL(BUS, (addr))
+#define PCIE_MMCFG_DEV(addr)            PCIE_VAL(DEV, (addr))
+#define PCIE_MMCFG_FUNC(addr)           PCIE_VAL(FUNC, (addr))
+#define PCIE_MMCFG_CONFADDR(addr)       PCIE_VAL(CONFADDR, (addr))
+
+    bus_num = PCIE_MMCFG_BUS(mmcfg_addr);
+    dev = PCIE_MMCFG_DEV(mmcfg_addr);
+    func = PCIE_MMCFG_FUNC(mmcfg_addr);
+    *conf_addr = PCIE_MMCFG_CONFADDR(mmcfg_addr);
+
+    *pci_dev = pci_bdf_to_dev(s, bus_num, PCI_DEVFN(dev, func));
+}
+
+void pcie_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
+{
+    PCIBus *s = opaque;
+    PCIDevice *pci_dev;
+    uint32_t config_addr;
+
+#if 0
+    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
+                __func__, addr, val, len);
+#endif
+    pcie_mmcfg_addr_to_dev(s, addr, &pci_dev, &config_addr);
+    pci_dev_data_write(pci_dev, config_addr, val, len);
+}
+
+uint32_t pcie_data_read(void *opaque, uint32_t addr, int len)
+{
+    PCIBus *s = opaque;
+    PCIDevice *pci_dev;
+    uint32_t config_addr;
+
+    pcie_mmcfg_addr_to_dev(s, addr, &pci_dev, &config_addr);
+    return pci_dev_data_read(pci_dev, config_addr, len);
+}
+
+#define DEFINE_PCIE_HOST_DATA_READ(len)                         \
+    static uint32_t pcie_host_data_read_ ## len (               \
+        void *opaque, target_phys_addr_t addr)                  \
+    {                                                           \
+        PCIExpressHost *e = (PCIExpressHost *)opaque;           \
+        return pcie_data_read(e->pci.bus,                       \
+                              addr - e->base_addr, (len));      \
+    }
+
+#define DEFINE_PCIE_HOST_DATA_WRITE(len)                        \
+    static void pcie_host_data_write_ ## len (                  \
+        void *opaque, target_phys_addr_t addr, uint32_t value)  \
+    {                                                           \
+        PCIExpressHost *e = (PCIExpressHost *)opaque;           \
+        pcie_data_write(e->pci.bus,                             \
+                        addr - e->base_addr, value, (len));     \
+    }
+
+#define DEFINE_PCIE_HOST_DATA_MMIO(len)      \
+        DEFINE_PCIE_HOST_DATA_READ(len)      \
+        DEFINE_PCIE_HOST_DATA_WRITE(len)
+
+DEFINE_PCIE_HOST_DATA_MMIO(1)
+DEFINE_PCIE_HOST_DATA_MMIO(2)
+DEFINE_PCIE_HOST_DATA_MMIO(4)
+
+#define DEFINE_PCIE_MEMORY_FUNCS(Type, type)                            \
+    static CPU ## Type ## MemoryFunc *pcie_host_data_ ## type [] =      \
+    {                                                                   \
+        &pcie_host_data_ ## type ## _1,                                 \
+        &pcie_host_data_ ## type ## _2,                                 \
+        &pcie_host_data_ ## type ## _4,                                 \
+    };
+
+DEFINE_PCIE_MEMORY_FUNCS(Read, read)
+DEFINE_PCIE_MEMORY_FUNCS(Write, write)
+
+PCIExpressHost *pcie_host_init(CPUReadMemoryFunc **mmcfg_read,
+                               CPUWriteMemoryFunc **mmcfg_write)
+{
+    PCIExpressHost *e;
+
+    e = qemu_mallocz(sizeof(*e));
+    e->base_addr = PCIE_BASE_ADDR_INVALID;
+
+    if (mmcfg_read == NULL)
+        mmcfg_read = pcie_host_data_read;
+    if (mmcfg_write == NULL)
+        mmcfg_write = pcie_host_data_write;
+    e->mmio_index = cpu_register_io_memory(mmcfg_read, mmcfg_write, e);
+    if (e->mmio_index < 0) {
+        qemu_free(e);
+        return NULL;
+    }
+
+    return e;
+}
+
+void pcie_host_mmcfg_map(PCIExpressHost *e,
+                         target_phys_addr_t addr, int bus_num_order)
+{
+    int size_order = 20 + bus_num_order - 1;
+
+    /* [(20 + n - 1):20] bit: 2^n bus where 1 <= n <= 8 */
+#define PCIE_BUS_NUM_ORDER_MIN  1
+#define PCIE_BUS_NUM_ORDER_MAX  (PCIE_MMCFG_BUS_HI - PCIE_MMCFG_BUS_LOW + 1)
+    assert(PCIE_BUS_NUM_ORDER_MIN <= bus_num_order);
+    assert(bus_num_order <= PCIE_BUS_NUM_ORDER_MAX);
+    assert((addr & ((1ULL << size_order) - 1)) == 0);
+
+    if (e->base_addr != PCIE_BASE_ADDR_INVALID) {
+        cpu_register_physical_memory(e->base_addr, e->size, IO_MEM_UNASSIGNED);
+    }
+
+    e->base_addr = addr;
+    e->size = 1ULL << size_order;
+    e->bus_num_order = bus_num_order;
+    cpu_register_physical_memory(e->base_addr, e->size, e->mmio_index);
+}
+
 /***********************************************************/
 /* generic PCI irq support */
 
@@ -991,9 +1160,10 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 
 static int pci_find_space(PCIDevice *pdev, uint8_t size)
 {
+    int config_size = pcie_config_size(pdev);
     int offset = PCI_CONFIG_HEADER_SIZE;
     int i;
-    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
         if (pdev->used[i])
             offset = i + 1;
         else if (i - offset + 1 == size)
diff --git a/hw/pci.h b/hw/pci.h
index 33e2ef2..ff8dbad 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -170,20 +170,26 @@ enum {
     QEMU_PCI_CAP_MSIX = 0x1,
 };
 
+/* Size of the standart PCIe config space: 4KB */
+#define PCIE_CONFIG_SPACE_SIZE  0x1000
+#define PCIE_EXT_CONFIG_SPACE_SIZE                      \
+    (PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE)
+
 struct PCIDevice {
     DeviceState qdev;
+
     /* PCI config space */
-    uint8_t config[PCI_CONFIG_SPACE_SIZE];
+    uint8_t *config;
 
     /* Used to enable config checks on load. Note that writeable bits are
      * never checked even if set in cmask. */
-    uint8_t cmask[PCI_CONFIG_SPACE_SIZE];
+    uint8_t *cmask;
 
     /* Used to implement R/W bytes */
-    uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
+    uint8_t *wmask;
 
     /* Used to allocate config space for capabilities. */
-    uint8_t used[PCI_CONFIG_SPACE_SIZE];
+    uint8_t *used;
 
     /* the following fields are read only */
     PCIBus *bus;
@@ -257,6 +263,8 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr);
 void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(void *opaque, uint32_t addr, int len);
+void pcie_data_write(void *opaque, uint32_t addr, uint32_t val, int len);
+uint32_t pcie_data_read(void *opaque, uint32_t addr, int len);
 int pci_bus_num(PCIBus *s);
 void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d));
 PCIBus *pci_find_bus(int bus_num);
@@ -329,6 +337,9 @@ typedef struct {
     pci_qdev_initfn init;
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
+
+    /* pcie stuff */
+    int pcie;
 } PCIDeviceInfo;
 
 void pci_qdev_register(PCIDeviceInfo *info);
@@ -337,6 +348,16 @@ void pci_qdev_register_many(PCIDeviceInfo *info);
 DeviceState *pci_create(const char *name, const char *devaddr);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
+static inline int pci_is_pcie(PCIDevice *d)
+{
+    return container_of(d->qdev.info, PCIDeviceInfo, qdev)->pcie;
+}
+
+static inline uint32_t pcie_config_size(PCIDevice *d)
+{
+    return pci_is_pcie(d)? PCIE_CONFIG_SPACE_SIZE: PCI_CONFIG_SPACE_SIZE;
+}
+
 /* lsi53c895a.c */
 #define LSI_MAX_DEVS 7
 void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id);
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 9f272a7..84e3b08 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -36,4 +36,20 @@ typedef struct {
     PCIBus *bus;
 } PCIHostState;
 
+typedef struct {
+    PCIHostState pci;
+
+    /* express part */
+    target_phys_addr_t  base_addr;
+#define PCIE_BASE_ADDR_INVALID  ((target_phys_addr_t)-1ULL)
+    target_phys_addr_t  size;
+    int bus_num_order;
+    int mmio_index;
+} PCIExpressHost;
+
+PCIExpressHost *pcie_host_init(CPUReadMemoryFunc **mmcfg_read,
+                               CPUWriteMemoryFunc **mmcfg_write);
+void pcie_host_mmcfg_map(PCIExpressHost *e,
+                         target_phys_addr_t addr, int bus_num_order);
+
 #endif /* PCI_HOST_H */
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 9/9] [RFC] pci: pcie host and mmcfg support.

[Michael S. Tsirkin]

On Wed, Jul 15, 2009 at 08:15:09PM +0900, Isaku Yamahata wrote:
> This patch adds common routines for pcie host bridge and pcie mmcfg.
> This will be used by q35 based chipset emulation.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c      |  226 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  hw/pci.h      |   29 ++++++-
>  hw/pci_host.h |   16 ++++
>  3 files changed, 239 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 3b19c3d..eb761ce 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -23,6 +23,7 @@
>   */
>  #include "hw.h"
>  #include "pci.h"
> +#include "pci_host.h"
>  #include "monitor.h"
>  #include "net.h"
>  #include "sysemu.h"
> @@ -166,27 +167,32 @@ int pci_bus_num(PCIBus *s)
>  void pci_device_save(PCIDevice *s, QEMUFile *f)
>  {
>      int i;
> +    uint32_t config_size = pcie_config_size(s);
>  
>      qemu_put_be32(f, 2); /* PCI device version */
> -    qemu_put_buffer(f, s->config, 256);
> +    qemu_put_buffer(f, s->config, config_size);
>      for (i = 0; i < 4; i++)
>          qemu_put_be32(f, s->irq_state[i]);
>  }
>  
>  int pci_device_load(PCIDevice *s, QEMUFile *f)
>  {
> -    uint8_t config[PCI_CONFIG_SPACE_SIZE];
> +    uint32_t config_size = pcie_config_size(s);
> +    uint8_t *config;
>      uint32_t version_id;
>      int i;
>  
>      version_id = qemu_get_be32(f);
>      if (version_id > 2)
>          return -EINVAL;
> -    qemu_get_buffer(f, config, sizeof config);
> -    for (i = 0; i < sizeof config; ++i)
> +
> +    config = qemu_malloc(config_size);
> +    qemu_get_buffer(f, config, config_size);
> +    for (i = 0; i < config_size; ++i)
>          if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i])
>              return -EINVAL;
> -    memcpy(s->config, config, sizeof config);
> +    memcpy(s->config, config, config_size);
> +    qemu_free(config);
>  
>      pci_update_mappings(s);
>  
> @@ -302,14 +308,31 @@ static void pci_init_cmask(PCIDevice *dev)
>  static void pci_init_wmask(PCIDevice *dev)
>  {
>      int i;
> +    uint32_t config_size = pcie_config_size(dev);
> +
>      dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
>      dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
>      dev->wmask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
>                                | PCI_COMMAND_MASTER;
> -    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
>          dev->wmask[i] = 0xff;
>  }
>  
> +static void pci_config_init(PCIDevice *pci_dev)
> +{
> +    int config_size = pcie_config_size(pci_dev);
> +#define PCI_CONFIG_ALLOC(d, member, size)                               \
> +    do {                                                                \
> +        (d)->member =                                                   \
> +            (typeof((d)->member))qemu_mallocz(sizeof((d)->member[0]) *  \
> +                                              size);                    \
> +    } while (0)
> +    PCI_CONFIG_ALLOC(pci_dev, config, config_size);
> +    PCI_CONFIG_ALLOC(pci_dev, cmask, config_size);
> +    PCI_CONFIG_ALLOC(pci_dev, wmask, config_size);
> +    PCI_CONFIG_ALLOC(pci_dev, used, config_size);
> +}
> +
>  /* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn,
> @@ -330,6 +353,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      pci_dev->devfn = devfn;
>      pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>      memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
> +    pci_config_init(pci_dev);
>      pci_set_default_subsystem_id(pci_dev);
>      pci_init_cmask(pci_dev);
>      pci_init_wmask(pci_dev);
> @@ -531,40 +555,48 @@ static void pci_update_mappings(PCIDevice *d)
>      }
>  }
>  
> +static uint8_t pcie_config_get_byte(PCIDevice *d, uint32_t addr)

We don't need these wrappers. From software perspective pci express
looks just like pci: little endian, so use pci_get_byte.

> +{
> +    uint8_t *conf = &d->config[addr];
> +    if (conf != NULL)

Please just write if (conf)
But in this case: conf can not ever be NULL, can it?
And if it's NULL, device was not initialized yet,
so returning 0 does not make sense either?

> +        return *conf;
> +    return 0;
> +}
> +
> +static uint32_t pcie_config_get(PCIDevice *d, uint32_t addr, int len)
> +{
> +    int i;
> +    union {
> +        uint8_t val8[4];
> +        uint32_t val32;
> +    } v = { .val32 = 0 };
> +
> +    for (i = 0; i < len; i++) {
> +        v.val8[i] = pcie_config_get_byte(d, addr + i);
> +    }
> +
> +    return le32_to_cpu(v.val32);
> +}
> +
>  uint32_t pci_default_read_config(PCIDevice *d,
>                                   uint32_t address, int len)
>  {
> -    uint32_t val;
> +    uint32_t config_size = pcie_config_size(d);
>  
> -    switch(len) {
> -    default:
> -    case 4:
> -	if (address <= 0xfc) {
> -	    val = le32_to_cpu(*(uint32_t *)(d->config + address));
> -	    break;
> -	}
> -	/* fall through */
> -    case 2:
> -        if (address <= 0xfe) {
> -	    val = le16_to_cpu(*(uint16_t *)(d->config + address));
> -	    break;
> -	}
> -	/* fall through */
> -    case 1:
> -        val = d->config[address];
> -        break;
> -    }
> -    return val;
> +    assert(len == 1 || len == 2 || len == 4);
> +    len = MIN(len, config_size - address);
> +    return pcie_config_get(d, address, len);
>  }

If you unwrap all the extra layers that were added,
this will become even simpler:
uint32_t pci_default_read_config(PCIDevice *d,
                                   uint32_t address, int len)
{
	uint32_t val = 0;
	assert(len == 1 || len == 2 || len == 4);
	len = MIN(len, pcie_config_size(d) - address);
	memcpy(&val, d->config + addr, len);
	return le32_to_cpu(val);
}

which would be a good cleanup.

>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
>      uint8_t orig[PCI_CONFIG_SPACE_SIZE];
>      int i;
> +    uint32_t config_size = pcie_config_size(d);
>  
>      /* not efficient, but simple */
>      memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> -    for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, ++addr) {
> +    for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
>          uint8_t wmask = d->wmask[addr];
>          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
>      }
> @@ -660,6 +692,143 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
>      pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
>      return pci_dev_data_read(pci_dev, config_addr, len);
>  }
> +
> +static void pcie_mmcfg_addr_to_dev(PCIBus *s, uint32_t mmcfg_addr,
> +                                   PCIDevice **pci_dev, uint32_t *conf_addr)

Can you document what this does please?
And this probably should go into pcie-host.c -
it's only used there.

> +{
> +    int bus_num;
> +    unsigned int dev;
> +    uint8_t func;
> +
> +#define PCIE_MASK(val, hi_bit, low_bit)                 \
> +    (((val) & (((1ULL << (hi_bit)) - 1))) >> (low_bit))
> +#define PCIE_VAL(VAL, val)                                              \
> +    PCIE_MASK((val), PCIE_MMCFG_ ## VAL ## _HI, PCIE_MMCFG_ ## VAL ## _LOW)

This is just too tricky, and HI is ambigous: is it the last bit,
or the next on top of last? For the 4 users below, it's easier
to just define mask and bit offset and open-code it.


> +#define PCIE_MMCFG_BUS_HI               27
> +#define PCIE_MMCFG_BUS_LOW              20
> +#define PCIE_MMCFG_DEV_HI               19
> +#define PCIE_MMCFG_DEV_LOW              15
> +#define PCIE_MMCFG_FUNC_HI              14
> +#define PCIE_MMCFG_FUNC_LOW             12


Device+function come together, same as conventional
pci: you can just define a macro to get both and avoid
decoding them and then encoding back with PCI_DEVFN.

> +#define PCIE_MMCFG_CONFADDR_HI          11
> +#define PCIE_MMCFG_CONFADDR_LOW         0
> +#define PCIE_MMCFG_BUS(addr)            PCIE_VAL(BUS, (addr))
> +#define PCIE_MMCFG_DEV(addr)            PCIE_VAL(DEV, (addr))
> +#define PCIE_MMCFG_FUNC(addr)           PCIE_VAL(FUNC, (addr))
> +#define PCIE_MMCFG_CONFADDR(addr)       PCIE_VAL(CONFADDR, (addr))

So you would have (if I understood the code correctly).

#define PCIE_MMCFG_FUNC_BIT 12
#define PCIE_MMCFG_FUNC_MASK 0x7

#define PCIE_MMCFG_FUNC(addr) (((addr) >> PCIE_MMCFG_FUNC_BIT) & PCIE_MMCFG_FUNC_MASK)

> +
> +    bus_num = PCIE_MMCFG_BUS(mmcfg_addr);
> +    dev = PCIE_MMCFG_DEV(mmcfg_addr);
> +    func = PCIE_MMCFG_FUNC(mmcfg_addr);
> +    *conf_addr = PCIE_MMCFG_CONFADDR(mmcfg_addr);
> +
> +    *pci_dev = pci_bdf_to_dev(s, bus_num, PCI_DEVFN(dev, func));
> +}
> +
> +void pcie_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +#if 0
> +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, addr, val, len);
> +#endif
> +    pcie_mmcfg_addr_to_dev(s, addr, &pci_dev, &config_addr);
> +    pci_dev_data_write(pci_dev, config_addr, val, len);
> +}
> +
> +uint32_t pcie_data_read(void *opaque, uint32_t addr, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +    pcie_mmcfg_addr_to_dev(s, addr, &pci_dev, &config_addr);
> +    return pci_dev_data_read(pci_dev, config_addr, len);
> +}
> +
> +#define DEFINE_PCIE_HOST_DATA_READ(len)                         \
> +    static uint32_t pcie_host_data_read_ ## len (               \
> +        void *opaque, target_phys_addr_t addr)                  \
> +    {                                                           \
> +        PCIExpressHost *e = (PCIExpressHost *)opaque;           \
> +        return pcie_data_read(e->pci.bus,                       \
> +                              addr - e->base_addr, (len));      \
> +    }
> +
> +#define DEFINE_PCIE_HOST_DATA_WRITE(len)                        \
> +    static void pcie_host_data_write_ ## len (                  \
> +        void *opaque, target_phys_addr_t addr, uint32_t value)  \
> +    {                                                           \
> +        PCIExpressHost *e = (PCIExpressHost *)opaque;           \
> +        pcie_data_write(e->pci.bus,                             \
> +                        addr - e->base_addr, value, (len));     \
> +    }
> +
> +#define DEFINE_PCIE_HOST_DATA_MMIO(len)      \
> +        DEFINE_PCIE_HOST_DATA_READ(len)      \
> +        DEFINE_PCIE_HOST_DATA_WRITE(len)
> +
> +DEFINE_PCIE_HOST_DATA_MMIO(1)
> +DEFINE_PCIE_HOST_DATA_MMIO(2)
> +DEFINE_PCIE_HOST_DATA_MMIO(4)
> +
> +#define DEFINE_PCIE_MEMORY_FUNCS(Type, type)                            \
> +    static CPU ## Type ## MemoryFunc *pcie_host_data_ ## type [] =      \
> +    {                                                                   \
> +        &pcie_host_data_ ## type ## _1,                                 \
> +        &pcie_host_data_ ## type ## _2,                                 \
> +        &pcie_host_data_ ## type ## _4,                                 \
> +    };
> +
> +DEFINE_PCIE_MEMORY_FUNCS(Read, read)
> +DEFINE_PCIE_MEMORY_FUNCS(Write, write)
> +
> +PCIExpressHost *pcie_host_init(CPUReadMemoryFunc **mmcfg_read,
> +                               CPUWriteMemoryFunc **mmcfg_write)
> +{
> +    PCIExpressHost *e;
> +
> +    e = qemu_mallocz(sizeof(*e));
> +    e->base_addr = PCIE_BASE_ADDR_INVALID;
> +
> +    if (mmcfg_read == NULL)
> +        mmcfg_read = pcie_host_data_read;
> +    if (mmcfg_write == NULL)
> +        mmcfg_write = pcie_host_data_write;
> +    e->mmio_index = cpu_register_io_memory(mmcfg_read, mmcfg_write, e);
> +    if (e->mmio_index < 0) {
> +        qemu_free(e);
> +        return NULL;
> +    }
> +
> +    return e;
> +}
> +
> +void pcie_host_mmcfg_map(PCIExpressHost *e,
> +                         target_phys_addr_t addr, int bus_num_order)
> +{
> +    int size_order = 20 + bus_num_order - 1;
> +
> +    /* [(20 + n - 1):20] bit: 2^n bus where 1 <= n <= 8 */
> +#define PCIE_BUS_NUM_ORDER_MIN  1
> +#define PCIE_BUS_NUM_ORDER_MAX  (PCIE_MMCFG_BUS_HI - PCIE_MMCFG_BUS_LOW + 1)
> +    assert(PCIE_BUS_NUM_ORDER_MIN <= bus_num_order);
> +    assert(bus_num_order <= PCIE_BUS_NUM_ORDER_MAX);
> +    assert((addr & ((1ULL << size_order) - 1)) == 0);
> +
> +    if (e->base_addr != PCIE_BASE_ADDR_INVALID) {
> +        cpu_register_physical_memory(e->base_addr, e->size, IO_MEM_UNASSIGNED);
> +    }
> +
> +    e->base_addr = addr;
> +    e->size = 1ULL << size_order;
> +    e->bus_num_order = bus_num_order;
> +    cpu_register_physical_memory(e->base_addr, e->size, e->mmio_index);
> +}
> +
>  /***********************************************************/
>  /* generic PCI irq support */
>  
> @@ -991,9 +1160,10 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
>  
>  static int pci_find_space(PCIDevice *pdev, uint8_t size)
>  {
> +    int config_size = pcie_config_size(pdev);
>      int offset = PCI_CONFIG_HEADER_SIZE;
>      int i;
> -    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
>          if (pdev->used[i])
>              offset = i + 1;
>          else if (i - offset + 1 == size)
> diff --git a/hw/pci.h b/hw/pci.h
> index 33e2ef2..ff8dbad 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -170,20 +170,26 @@ enum {
>      QEMU_PCI_CAP_MSIX = 0x1,
>  };
>  
> +/* Size of the standart PCIe config space: 4KB */
> +#define PCIE_CONFIG_SPACE_SIZE  0x1000
> +#define PCIE_EXT_CONFIG_SPACE_SIZE                      \
> +    (PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE)
> +
>  struct PCIDevice {
>      DeviceState qdev;
> +
>      /* PCI config space */
> -    uint8_t config[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t *config;
>  
>      /* Used to enable config checks on load. Note that writeable bits are
>       * never checked even if set in cmask. */
> -    uint8_t cmask[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t *cmask;
>  
>      /* Used to implement R/W bytes */
> -    uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t *wmask;
>  
>      /* Used to allocate config space for capabilities. */
> -    uint8_t used[PCI_CONFIG_SPACE_SIZE];
> +    uint8_t *used;
>  
>      /* the following fields are read only */
>      PCIBus *bus;
> @@ -257,6 +263,8 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
>                          const char *default_devaddr);
>  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len);
>  uint32_t pci_data_read(void *opaque, uint32_t addr, int len);
> +void pcie_data_write(void *opaque, uint32_t addr, uint32_t val, int len);
> +uint32_t pcie_data_read(void *opaque, uint32_t addr, int len);
>  int pci_bus_num(PCIBus *s);
>  void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d));
>  PCIBus *pci_find_bus(int bus_num);
> @@ -329,6 +337,9 @@ typedef struct {
>      pci_qdev_initfn init;
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;
> +
> +    /* pcie stuff */
> +    int pcie;

Just add a bit in cap_present field.

>  } PCIDeviceInfo;
>  
>  void pci_qdev_register(PCIDeviceInfo *info);
> @@ -337,6 +348,16 @@ void pci_qdev_register_many(PCIDeviceInfo *info);
>  DeviceState *pci_create(const char *name, const char *devaddr);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>  
> +static inline int pci_is_pcie(PCIDevice *d)
> +{
> +    return container_of(d->qdev.info, PCIDeviceInfo, qdev)->pcie;
> +}
> +
> +static inline uint32_t pcie_config_size(PCIDevice *d)
> +{
> +    return pci_is_pcie(d)? PCIE_CONFIG_SPACE_SIZE: PCI_CONFIG_SPACE_SIZE;

space before ? and before :

> +}
> +

rename to pci_config_size?
See also my previous mail how we can keep the
same size for PCI and express and get rid of this
completely.


>  /* lsi53c895a.c */
>  #define LSI_MAX_DEVS 7
>  void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 9f272a7..84e3b08 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -36,4 +36,20 @@ typedef struct {
>      PCIBus *bus;
>  } PCIHostState;
>  
> +typedef struct {
> +    PCIHostState pci;
> +
> +    /* express part */

Document these fields?

> +    target_phys_addr_t  base_addr;
> +#define PCIE_BASE_ADDR_INVALID  ((target_phys_addr_t)-1ULL)

What's this?

> +    target_phys_addr_t  size;
> +    int bus_num_order;
> +    int mmio_index;
> +} PCIExpressHost;


> +
> +PCIExpressHost *pcie_host_init(CPUReadMemoryFunc **mmcfg_read,
> +                               CPUWriteMemoryFunc **mmcfg_write);
> +void pcie_host_mmcfg_map(PCIExpressHost *e,
> +                         target_phys_addr_t addr, int bus_num_order);
> +
>  #endif /* PCI_HOST_H */
> -- 
> 1.6.0.2
> 
>