[Qemu-devel] [0/2] pseries: Rework PCI code for handling multiple PHBs

[Qemu-devel] [0/2] pseries: Rework PCI code for handling multiple PHBs

[David Gibson]

Hi Michael, Alex,

This patch represents a compromise I hope will be acceptable after the
long thread discussing handling of multiple PCI host bridges on the
pseries machine.  Patch 1/1 is just a preliminary enforcing uniqueness
of LIOBNs in the IOMMU code.

Patch 2/2 is the meat.  It allows either explicit configuration of all
the properties, or the user can just set an abstract index which will
generate sensible and probably-unique values for all the rest.

With these patches I was able to do something like:
	qemu-system-ppc64 -M pseries -m 1024 -nographic \
	fc17-root.qcow2 -net none -device nec-usb-xhci -device \
	spapr-pci-host-bridge,index=1 -device e1000,netdev=fred \
	-netdev user,id=fred

I was able to see both the PCI domains in the guest, and use the NIC
on the secondary domain.

There are still some gotches with multiple domains though.  The domain
value in PCIHostBus is still always initialized to 0, and there are
other places in the PCI core where handling of multiple domains is
essentially stubbed out.

Michael, any thoughts on what to do about that?  I could fix up the
PCI code so that domain is actually set and used.  But I think the
whole notion of domain numbers is kind of bogus on the qemu side:
since PCI domains are completely independent from each other, it's
only platform convention which determines what the domain numbers are.
On platforms that don't have a strong convention, the guest will
number them itself and we have no way of knowing that.  So it seems to
me that the PCI code should instead of domain numbers just use the
device ID, or the bus name or some qemu side symbolic name.  For
platforms that do have a numbering convention those names can be
derived from the domain numbers, but it also works for platforms that
don't.

[Qemu-devel] [PATCH 1/2] pseries: Don't allow TCE (iommu) tables to be registered with duplicate LIOBNs

[David Gibson]

The PAPR specification requires that every bus or device mediated by the
IOMMU have a unique Logical IO Bus Number (LIOBN).  This patch adds a check
to enforce this, which will help catch errors in configuration earlier.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_iommu.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/spapr_iommu.c b/hw/spapr_iommu.c
index 02d78cc..3011b25 100644
--- a/hw/spapr_iommu.c
+++ b/hw/spapr_iommu.c
@@ -120,6 +120,12 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
 {
     sPAPRTCETable *tcet;
 
+    if (spapr_tce_find_by_liobn(liobn)) {
+        fprintf(stderr, "Attempted to create TCE table with duplicate"
+                " LIOBN 0x%x\n", liobn);
+        return NULL;
+    }
+
     if (!window_size) {
         return NULL;
     }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] pseries: Don't allow TCE (iommu) tables to be registered with duplicate LIOBNs

[Michael S. Tsirkin]

On Tue, Nov 27, 2012 at 05:07:32PM +1100, David Gibson wrote:
> The PAPR specification requires that every bus or device mediated by the
> IOMMU have a unique Logical IO Bus Number (LIOBN).  This patch adds a check
> to enforce this, which will help catch errors in configuration earlier.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

> ---
>  hw/spapr_iommu.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/spapr_iommu.c b/hw/spapr_iommu.c
> index 02d78cc..3011b25 100644
> --- a/hw/spapr_iommu.c
> +++ b/hw/spapr_iommu.c
> @@ -120,6 +120,12 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
>  {
>      sPAPRTCETable *tcet;
>  
> +    if (spapr_tce_find_by_liobn(liobn)) {
> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
> +                " LIOBN 0x%x\n", liobn);
> +        return NULL;
> +    }
> +
>      if (!window_size) {
>          return NULL;
>      }
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] pseries: Don't allow TCE (iommu) tables to be registered with duplicate LIOBNs

[Alexander Graf]

On 10.12.2012, at 14:00, Michael S. Tsirkin wrote:

> On Tue, Nov 27, 2012 at 05:07:32PM +1100, David Gibson wrote:
>> The PAPR specification requires that every bus or device mediated by the
>> IOMMU have a unique Logical IO Bus Number (LIOBN).  This patch adds a check
>> to enforce this, which will help catch errors in configuration earlier.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks, applied to ppc-next.


Alex

> 
>> ---
>> hw/spapr_iommu.c |    6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/spapr_iommu.c b/hw/spapr_iommu.c
>> index 02d78cc..3011b25 100644
>> --- a/hw/spapr_iommu.c
>> +++ b/hw/spapr_iommu.c
>> @@ -120,6 +120,12 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
>> {
>>     sPAPRTCETable *tcet;
>> 
>> +    if (spapr_tce_find_by_liobn(liobn)) {
>> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
>> +                " LIOBN 0x%x\n", liobn);
>> +        return NULL;
>> +    }
>> +
>>     if (!window_size) {
>>         return NULL;
>>     }
>> -- 
>> 1.7.10.4

[Qemu-devel] [PATCH 2/2] pseries: Properly handle allocation of multiple PCI host bridges

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

Multiple - even many - PCI host bridges (i.e. PCI domains) are very
common on real PAPR compliant hardware.  For reasons related to the
PAPR specified IOMMU interfaces, PCI device assignment with VFIO will
generally require at least two (virtual) PHBs and possibly more
depending on which devices are assigned.

At the moment the qemu PAPR PCI code will not deal with this well,
leaving several crucial parameters of PHBs other than the default one
uninitialized.  This patch reworks the code to allow this.

Every PHB needs a unique BUID (Bus Unit Identifier, the id used for
the PAPR PCI related interfaces) and a unique LIOBN (Logical IO Bus
Number, the id used for the PAPR IOMMU related interfaces).  In
addition they need windows in CPU real address space to access PCI
memory space, PCI IO space and MSIs.  Properties are added to the PCI
host bridge qdevice to allow configuration of all these.

To simplify configuration of multiple PHBs for common cases, a
convenience "index" property is also added.  This can be set instead
of the low-level properties, and will generate suitable values for the
other parameters, different for each index valu.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr.c     |   13 +-------
 hw/spapr_pci.c |   97 +++++++++++++++++++++++++++++++++++++++++---------------
 hw/spapr_pci.h |   21 ++++++++----
 3 files changed, 87 insertions(+), 44 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index d23aa9d..1609f73 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -76,12 +76,6 @@
 #define MAX_CPUS                256
 #define XICS_IRQS               1024
 
-#define SPAPR_PCI_BUID          0x800000020000001ULL
-#define SPAPR_PCI_MEM_WIN_ADDR  (0x10000000000ULL + 0xA0000000)
-#define SPAPR_PCI_MEM_WIN_SIZE  0x20000000
-#define SPAPR_PCI_IO_WIN_ADDR   (0x10000000000ULL + 0x80000000)
-#define SPAPR_PCI_MSI_WIN_ADDR  (0x10000000000ULL + 0x90000000)
-
 #define PHANDLE_XICP            0x00001111
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
@@ -854,12 +848,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     /* Set up PCI */
     spapr_pci_rtas_init();
 
-    spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
-                     SPAPR_PCI_MEM_WIN_ADDR,
-                     SPAPR_PCI_MEM_WIN_SIZE,
-                     SPAPR_PCI_IO_WIN_ADDR,
-                     SPAPR_PCI_MSI_WIN_ADDR);
-    phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
+    phb = spapr_create_phb(spapr, 0);
 
     for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 3c5b855..dc2ffaf 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -435,7 +435,7 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
      */
     sPAPRPHBState *phb = opaque;
 
-    trace_spapr_pci_lsi_set(phb->busname, irq_num, phb->lsi_table[irq_num].irq);
+    trace_spapr_pci_lsi_set(phb->dtbusname, irq_num, phb->lsi_table[irq_num].irq);
     qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
 }
 
@@ -522,6 +522,58 @@ static int spapr_phb_init(SysBusDevice *s)
     int i;
     PCIBus *bus;
 
+    if (sphb->index != -1) {
+        hwaddr windows_base;
+
+        if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
+            || (sphb->mem_win_addr != -1)
+            || (sphb->io_win_addr != -1)
+            || (sphb->msi_win_addr != -1)) {
+            fprintf(stderr, "Either \"index\" or other parameters must"
+                    " be specified for PAPR PHB, not both\n");
+            return -1;
+        }
+
+        sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
+        sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
+
+        windows_base = SPAPR_PCI_WINDOW_BASE
+            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
+        sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
+        sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
+        sphb->msi_win_addr = windows_base + SPAPR_PCI_MSI_WIN_OFF;
+    }
+
+    if (sphb->buid == -1) {
+        fprintf(stderr, "BUID not specified for PHB\n");
+        return -1;
+    }
+
+    if (sphb->dma_liobn == -1) {
+        fprintf(stderr, "LIOBN not specified for PHB\n");
+        return -1;
+    }
+
+    if (sphb->mem_win_addr == -1) {
+        fprintf(stderr, "Memory window address not specified for PHB\n");
+        return -1;
+    }
+
+    if (sphb->io_win_addr == -1) {
+        fprintf(stderr, "IO window address not specified for PHB\n");
+        return -1;
+    }
+
+    if (sphb->msi_win_addr == -1) {
+        fprintf(stderr, "MSI window address not specified for PHB\n");
+        return -1;
+    }
+
+    if (find_phb(spapr, sphb->buid)) {
+        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
+        return -1;
+    }               
+
     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
     namebuf = alloca(strlen(sphb->dtbusname) + 32);
 
@@ -565,17 +617,19 @@ static int spapr_phb_init(SysBusDevice *s)
                                     &sphb->msiwindow);
     }
 
-    bus = pci_register_bus(DEVICE(s),
-                           sphb->busname ? sphb->busname : sphb->dtbusname,
+    bus = pci_register_bus(DEVICE(s), sphb->dtbusname,
                            pci_spapr_set_irq, pci_spapr_map_irq, sphb,
                            &sphb->memspace, &sphb->iospace,
                            PCI_DEVFN(0, 0), PCI_NUM_PINS);
     phb->bus = bus;
 
-    sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
     sphb->dma_window_start = 0;
     sphb->dma_window_size = 0x40000000;
     sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, sphb->dma_window_size);
+    if (!sphb->dma) {
+        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
+        return -1;
+    }
     pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
 
     QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
@@ -605,13 +659,16 @@ static void spapr_phb_reset(DeviceState *qdev)
 }
 
 static Property spapr_phb_properties[] = {
-    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
-    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
-    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, 0),
-    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 0x20000000),
-    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
-    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
-    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, 0),
+    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
+    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
+    DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
+    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
+    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size,
+                      SPAPR_PCI_MMIO_WIN_SIZE),
+    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
+    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size,
+                      SPAPR_PCI_IO_WIN_SIZE),
+    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -632,25 +689,15 @@ static const TypeInfo spapr_phb_info = {
     .class_init    = spapr_phb_class_init,
 };
 
-void spapr_create_phb(sPAPREnvironment *spapr,
-                      const char *busname, uint64_t buid,
-                      uint64_t mem_win_addr, uint64_t mem_win_size,
-                      uint64_t io_win_addr, uint64_t msi_win_addr)
+PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
 {
     DeviceState *dev;
 
     dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
-
-    if (busname) {
-        qdev_prop_set_string(dev, "busname", g_strdup(busname));
-    }
-    qdev_prop_set_uint64(dev, "buid", buid);
-    qdev_prop_set_uint64(dev, "mem_win_addr", mem_win_addr);
-    qdev_prop_set_uint64(dev, "mem_win_size", mem_win_size);
-    qdev_prop_set_uint64(dev, "io_win_addr", io_win_addr);
-    qdev_prop_set_uint64(dev, "msi_win_addr", msi_win_addr);
-
+    qdev_prop_set_uint32(dev, "index", index);
     qdev_init_nofail(dev);
+
+    return PCI_HOST_BRIDGE(dev);
 }
 
 /* Macros to operate with address in OF binding to PCI */
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index e307ac8..76ba48a 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -37,8 +37,8 @@
 typedef struct sPAPRPHBState {
     PCIHostState parent_obj;
 
+    int32_t index;
     uint64_t buid;
-    char *busname;
     char *dtbusname;
 
     MemoryRegion memspace, iospace;
@@ -64,18 +64,25 @@ typedef struct sPAPRPHBState {
     QLIST_ENTRY(sPAPRPHBState) list;
 } sPAPRPHBState;
 
+#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
+
+#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
+#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
+#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
+#define SPAPR_PCI_MMIO_WIN_SIZE      0x20000000
+#define SPAPR_PCI_IO_WIN_OFF         0x80000000
+#define SPAPR_PCI_IO_WIN_SIZE        0x10000
+#define SPAPR_PCI_MSI_WIN_OFF        0x90000000
+
+#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
+
 static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
 {
     return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
 }
 
-#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
-#define SPAPR_PCI_IO_WIN_SIZE        0x10000
 
-void spapr_create_phb(sPAPREnvironment *spapr,
-                      const char *busname, uint64_t buid,
-                      uint64_t mem_win_addr, uint64_t mem_win_size,
-                      uint64_t io_win_addr, uint64_t msi_win_addr);
+PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
 
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
-- 
1.7.10.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] pseries: Properly handle allocation of multiple PCI host bridges

[David Gibson]

Alex,

If mst should happen to ack this in the next little while, please
don't merge it.  I've spotted a couple of little bugs with it.


On Tue, Nov 27, 2012 at 05:07:33PM +1100, David Gibson wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Multiple - even many - PCI host bridges (i.e. PCI domains) are very
> common on real PAPR compliant hardware.  For reasons related to the
> PAPR specified IOMMU interfaces, PCI device assignment with VFIO will
> generally require at least two (virtual) PHBs and possibly more
> depending on which devices are assigned.
> 
> At the moment the qemu PAPR PCI code will not deal with this well,
> leaving several crucial parameters of PHBs other than the default one
> uninitialized.  This patch reworks the code to allow this.
> 
> Every PHB needs a unique BUID (Bus Unit Identifier, the id used for
> the PAPR PCI related interfaces) and a unique LIOBN (Logical IO Bus
> Number, the id used for the PAPR IOMMU related interfaces).  In
> addition they need windows in CPU real address space to access PCI
> memory space, PCI IO space and MSIs.  Properties are added to the PCI
> host bridge qdevice to allow configuration of all these.
> 
> To simplify configuration of multiple PHBs for common cases, a
> convenience "index" property is also added.  This can be set instead
> of the low-level properties, and will generate suitable values for the
> other parameters, different for each index valu.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/spapr.c     |   13 +-------
>  hw/spapr_pci.c |   97 +++++++++++++++++++++++++++++++++++++++++---------------
>  hw/spapr_pci.h |   21 ++++++++----
>  3 files changed, 87 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index d23aa9d..1609f73 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -76,12 +76,6 @@
>  #define MAX_CPUS                256
>  #define XICS_IRQS               1024
>  
> -#define SPAPR_PCI_BUID          0x800000020000001ULL
> -#define SPAPR_PCI_MEM_WIN_ADDR  (0x10000000000ULL + 0xA0000000)
> -#define SPAPR_PCI_MEM_WIN_SIZE  0x20000000
> -#define SPAPR_PCI_IO_WIN_ADDR   (0x10000000000ULL + 0x80000000)
> -#define SPAPR_PCI_MSI_WIN_ADDR  (0x10000000000ULL + 0x90000000)
> -
>  #define PHANDLE_XICP            0x00001111
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
> @@ -854,12 +848,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      /* Set up PCI */
>      spapr_pci_rtas_init();
>  
> -    spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
> -                     SPAPR_PCI_MEM_WIN_ADDR,
> -                     SPAPR_PCI_MEM_WIN_SIZE,
> -                     SPAPR_PCI_IO_WIN_ADDR,
> -                     SPAPR_PCI_MSI_WIN_ADDR);
> -    phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
> +    phb = spapr_create_phb(spapr, 0);
>  
>      for (i = 0; i < nb_nics; i++) {
>          NICInfo *nd = &nd_table[i];
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 3c5b855..dc2ffaf 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -435,7 +435,7 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
>       */
>      sPAPRPHBState *phb = opaque;
>  
> -    trace_spapr_pci_lsi_set(phb->busname, irq_num, phb->lsi_table[irq_num].irq);
> +    trace_spapr_pci_lsi_set(phb->dtbusname, irq_num, phb->lsi_table[irq_num].irq);
>      qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>  }
>  
> @@ -522,6 +522,58 @@ static int spapr_phb_init(SysBusDevice *s)
>      int i;
>      PCIBus *bus;
>  
> +    if (sphb->index != -1) {
> +        hwaddr windows_base;
> +
> +        if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
> +            || (sphb->mem_win_addr != -1)
> +            || (sphb->io_win_addr != -1)
> +            || (sphb->msi_win_addr != -1)) {
> +            fprintf(stderr, "Either \"index\" or other parameters must"
> +                    " be specified for PAPR PHB, not both\n");
> +            return -1;
> +        }
> +
> +        sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> +        sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
> +
> +        windows_base = SPAPR_PCI_WINDOW_BASE
> +            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> +        sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> +        sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
> +        sphb->msi_win_addr = windows_base + SPAPR_PCI_MSI_WIN_OFF;
> +    }
> +
> +    if (sphb->buid == -1) {
> +        fprintf(stderr, "BUID not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->dma_liobn == -1) {
> +        fprintf(stderr, "LIOBN not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->mem_win_addr == -1) {
> +        fprintf(stderr, "Memory window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->io_win_addr == -1) {
> +        fprintf(stderr, "IO window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->msi_win_addr == -1) {
> +        fprintf(stderr, "MSI window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (find_phb(spapr, sphb->buid)) {
> +        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
> +        return -1;
> +    }               
> +
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
>      namebuf = alloca(strlen(sphb->dtbusname) + 32);
>  
> @@ -565,17 +617,19 @@ static int spapr_phb_init(SysBusDevice *s)
>                                      &sphb->msiwindow);
>      }
>  
> -    bus = pci_register_bus(DEVICE(s),
> -                           sphb->busname ? sphb->busname : sphb->dtbusname,
> +    bus = pci_register_bus(DEVICE(s), sphb->dtbusname,
>                             pci_spapr_set_irq, pci_spapr_map_irq, sphb,
>                             &sphb->memspace, &sphb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
>      phb->bus = bus;
>  
> -    sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
>      sphb->dma_window_start = 0;
>      sphb->dma_window_size = 0x40000000;
>      sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, sphb->dma_window_size);
> +    if (!sphb->dma) {
> +        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
> +        return -1;
> +    }
>      pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> @@ -605,13 +659,16 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
>  
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
> -    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
> -    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, 0),
> -    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 0x20000000),
> -    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
> -    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
> -    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, 0),
> +    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
> +    DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
> +    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> +    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size,
> +                      SPAPR_PCI_MMIO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
> +    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size,
> +                      SPAPR_PCI_IO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -632,25 +689,15 @@ static const TypeInfo spapr_phb_info = {
>      .class_init    = spapr_phb_class_init,
>  };
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr)
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
> -
> -    if (busname) {
> -        qdev_prop_set_string(dev, "busname", g_strdup(busname));
> -    }
> -    qdev_prop_set_uint64(dev, "buid", buid);
> -    qdev_prop_set_uint64(dev, "mem_win_addr", mem_win_addr);
> -    qdev_prop_set_uint64(dev, "mem_win_size", mem_win_size);
> -    qdev_prop_set_uint64(dev, "io_win_addr", io_win_addr);
> -    qdev_prop_set_uint64(dev, "msi_win_addr", msi_win_addr);
> -
> +    qdev_prop_set_uint32(dev, "index", index);
>      qdev_init_nofail(dev);
> +
> +    return PCI_HOST_BRIDGE(dev);
>  }
>  
>  /* Macros to operate with address in OF binding to PCI */
> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> index e307ac8..76ba48a 100644
> --- a/hw/spapr_pci.h
> +++ b/hw/spapr_pci.h
> @@ -37,8 +37,8 @@
>  typedef struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
> +    int32_t index;
>      uint64_t buid;
> -    char *busname;
>      char *dtbusname;
>  
>      MemoryRegion memspace, iospace;
> @@ -64,18 +64,25 @@ typedef struct sPAPRPHBState {
>      QLIST_ENTRY(sPAPRPHBState) list;
>  } sPAPRPHBState;
>  
> +#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
> +
> +#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> +#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> +#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> +#define SPAPR_PCI_MMIO_WIN_SIZE      0x20000000
> +#define SPAPR_PCI_IO_WIN_OFF         0x80000000
> +#define SPAPR_PCI_IO_WIN_SIZE        0x10000
> +#define SPAPR_PCI_MSI_WIN_OFF        0x90000000
> +
> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> +
>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
>  }
>  
> -#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> -#define SPAPR_PCI_IO_WIN_SIZE        0x10000
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr);
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
>  
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 2/2] pseries: Properly handle allocation of multiple PCI host bridges

[Michael S. Tsirkin]

On Tue, Nov 27, 2012 at 05:07:33PM +1100, David Gibson wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Multiple - even many - PCI host bridges (i.e. PCI domains) are very
> common on real PAPR compliant hardware.  For reasons related to the
> PAPR specified IOMMU interfaces, PCI device assignment with VFIO will
> generally require at least two (virtual) PHBs and possibly more
> depending on which devices are assigned.
> 
> At the moment the qemu PAPR PCI code will not deal with this well,
> leaving several crucial parameters of PHBs other than the default one
> uninitialized.  This patch reworks the code to allow this.
> 
> Every PHB needs a unique BUID (Bus Unit Identifier, the id used for
> the PAPR PCI related interfaces) and a unique LIOBN (Logical IO Bus
> Number, the id used for the PAPR IOMMU related interfaces).  In
> addition they need windows in CPU real address space to access PCI
> memory space, PCI IO space and MSIs.  Properties are added to the PCI
> host bridge qdevice to allow configuration of all these.
> 
> To simplify configuration of multiple PHBs for common cases, a
> convenience "index" property is also added.  This can be set instead
> of the low-level properties, and will generate suitable values for the
> other parameters, different for each index valu.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

> ---
>  hw/spapr.c     |   13 +-------
>  hw/spapr_pci.c |   97 +++++++++++++++++++++++++++++++++++++++++---------------
>  hw/spapr_pci.h |   21 ++++++++----
>  3 files changed, 87 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index d23aa9d..1609f73 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -76,12 +76,6 @@
>  #define MAX_CPUS                256
>  #define XICS_IRQS               1024
>  
> -#define SPAPR_PCI_BUID          0x800000020000001ULL
> -#define SPAPR_PCI_MEM_WIN_ADDR  (0x10000000000ULL + 0xA0000000)
> -#define SPAPR_PCI_MEM_WIN_SIZE  0x20000000
> -#define SPAPR_PCI_IO_WIN_ADDR   (0x10000000000ULL + 0x80000000)
> -#define SPAPR_PCI_MSI_WIN_ADDR  (0x10000000000ULL + 0x90000000)
> -
>  #define PHANDLE_XICP            0x00001111
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
> @@ -854,12 +848,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      /* Set up PCI */
>      spapr_pci_rtas_init();
>  
> -    spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
> -                     SPAPR_PCI_MEM_WIN_ADDR,
> -                     SPAPR_PCI_MEM_WIN_SIZE,
> -                     SPAPR_PCI_IO_WIN_ADDR,
> -                     SPAPR_PCI_MSI_WIN_ADDR);
> -    phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
> +    phb = spapr_create_phb(spapr, 0);
>  
>      for (i = 0; i < nb_nics; i++) {
>          NICInfo *nd = &nd_table[i];
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 3c5b855..dc2ffaf 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -435,7 +435,7 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
>       */
>      sPAPRPHBState *phb = opaque;
>  
> -    trace_spapr_pci_lsi_set(phb->busname, irq_num, phb->lsi_table[irq_num].irq);
> +    trace_spapr_pci_lsi_set(phb->dtbusname, irq_num, phb->lsi_table[irq_num].irq);
>      qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>  }
>  
> @@ -522,6 +522,58 @@ static int spapr_phb_init(SysBusDevice *s)
>      int i;
>      PCIBus *bus;
>  
> +    if (sphb->index != -1) {
> +        hwaddr windows_base;
> +
> +        if ((sphb->buid != -1) || (sphb->dma_liobn != -1)
> +            || (sphb->mem_win_addr != -1)
> +            || (sphb->io_win_addr != -1)
> +            || (sphb->msi_win_addr != -1)) {
> +            fprintf(stderr, "Either \"index\" or other parameters must"
> +                    " be specified for PAPR PHB, not both\n");
> +            return -1;
> +        }
> +
> +        sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> +        sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
> +
> +        windows_base = SPAPR_PCI_WINDOW_BASE
> +            + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> +        sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
> +        sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;
> +        sphb->msi_win_addr = windows_base + SPAPR_PCI_MSI_WIN_OFF;
> +    }
> +
> +    if (sphb->buid == -1) {
> +        fprintf(stderr, "BUID not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->dma_liobn == -1) {
> +        fprintf(stderr, "LIOBN not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->mem_win_addr == -1) {
> +        fprintf(stderr, "Memory window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->io_win_addr == -1) {
> +        fprintf(stderr, "IO window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (sphb->msi_win_addr == -1) {
> +        fprintf(stderr, "MSI window address not specified for PHB\n");
> +        return -1;
> +    }
> +
> +    if (find_phb(spapr, sphb->buid)) {
> +        fprintf(stderr, "PCI host bridges must have unique BUIDs\n");
> +        return -1;
> +    }               
> +
>      sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
>      namebuf = alloca(strlen(sphb->dtbusname) + 32);
>  
> @@ -565,17 +617,19 @@ static int spapr_phb_init(SysBusDevice *s)
>                                      &sphb->msiwindow);
>      }
>  
> -    bus = pci_register_bus(DEVICE(s),
> -                           sphb->busname ? sphb->busname : sphb->dtbusname,
> +    bus = pci_register_bus(DEVICE(s), sphb->dtbusname,
>                             pci_spapr_set_irq, pci_spapr_map_irq, sphb,
>                             &sphb->memspace, &sphb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS);
>      phb->bus = bus;
>  
> -    sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
>      sphb->dma_window_start = 0;
>      sphb->dma_window_size = 0x40000000;
>      sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, sphb->dma_window_size);
> +    if (!sphb->dma) {
> +        fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
> +        return -1;
> +    }
>      pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> @@ -605,13 +659,16 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
>  
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
> -    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
> -    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, 0),
> -    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 0x20000000),
> -    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
> -    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
> -    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, 0),
> +    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
> +    DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
> +    DEFINE_PROP_HEX64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> +    DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size,
> +                      SPAPR_PCI_MMIO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
> +    DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size,
> +                      SPAPR_PCI_IO_WIN_SIZE),
> +    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -632,25 +689,15 @@ static const TypeInfo spapr_phb_info = {
>      .class_init    = spapr_phb_class_init,
>  };
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr)
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>  {
>      DeviceState *dev;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
> -
> -    if (busname) {
> -        qdev_prop_set_string(dev, "busname", g_strdup(busname));
> -    }
> -    qdev_prop_set_uint64(dev, "buid", buid);
> -    qdev_prop_set_uint64(dev, "mem_win_addr", mem_win_addr);
> -    qdev_prop_set_uint64(dev, "mem_win_size", mem_win_size);
> -    qdev_prop_set_uint64(dev, "io_win_addr", io_win_addr);
> -    qdev_prop_set_uint64(dev, "msi_win_addr", msi_win_addr);
> -
> +    qdev_prop_set_uint32(dev, "index", index);
>      qdev_init_nofail(dev);
> +
> +    return PCI_HOST_BRIDGE(dev);
>  }
>  
>  /* Macros to operate with address in OF binding to PCI */
> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> index e307ac8..76ba48a 100644
> --- a/hw/spapr_pci.h
> +++ b/hw/spapr_pci.h
> @@ -37,8 +37,8 @@
>  typedef struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
> +    int32_t index;
>      uint64_t buid;
> -    char *busname;
>      char *dtbusname;
>  
>      MemoryRegion memspace, iospace;
> @@ -64,18 +64,25 @@ typedef struct sPAPRPHBState {
>      QLIST_ENTRY(sPAPRPHBState) list;
>  } sPAPRPHBState;
>  
> +#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
> +
> +#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> +#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
> +#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
> +#define SPAPR_PCI_MMIO_WIN_SIZE      0x20000000
> +#define SPAPR_PCI_IO_WIN_OFF         0x80000000
> +#define SPAPR_PCI_IO_WIN_SIZE        0x10000
> +#define SPAPR_PCI_MSI_WIN_OFF        0x90000000
> +
> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> +
>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
>  }
>  
> -#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
> -#define SPAPR_PCI_IO_WIN_SIZE        0x10000
>  
> -void spapr_create_phb(sPAPREnvironment *spapr,
> -                      const char *busname, uint64_t buid,
> -                      uint64_t mem_win_addr, uint64_t mem_win_size,
> -                      uint64_t io_win_addr, uint64_t msi_win_addr);
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
>  
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,
> -- 
> 1.7.10.4

Re: [Qemu-devel] [0/2] pseries: Rework PCI code for handling multiple PHBs

[Michael S. Tsirkin]

On Tue, Nov 27, 2012 at 05:07:31PM +1100, David Gibson wrote:
> Hi Michael, Alex,
> 
> This patch represents a compromise I hope will be acceptable after the
> long thread discussing handling of multiple PCI host bridges on the
> pseries machine.  Patch 1/1 is just a preliminary enforcing uniqueness
> of LIOBNs in the IOMMU code.
> 
> Patch 2/2 is the meat.  It allows either explicit configuration of all
> the properties, or the user can just set an abstract index which will
> generate sensible and probably-unique values for all the rest.
> 
> With these patches I was able to do something like:
> 	qemu-system-ppc64 -M pseries -m 1024 -nographic \
> 	fc17-root.qcow2 -net none -device nec-usb-xhci -device \
> 	spapr-pci-host-bridge,index=1 -device e1000,netdev=fred \
> 	-netdev user,id=fred
> 
> I was able to see both the PCI domains in the guest, and use the NIC
> on the secondary domain.
> 
> There are still some gotches with multiple domains though.  The domain
> value in PCIHostBus is still always initialized to 0, and there are
> other places in the PCI core where handling of multiple domains is
> essentially stubbed out.
> 
> Michael, any thoughts on what to do about that?  I could fix up the
> PCI code so that domain is actually set and used.  But I think the
> whole notion of domain numbers is kind of bogus on the qemu side:
> since PCI domains are completely independent from each other, it's
> only platform convention which determines what the domain numbers are.
> On platforms that don't have a strong convention, the guest will
> number them itself and we have no way of knowing that.  So it seems to
> me that the PCI code should instead of domain numbers just use the
> device ID, or the bus name or some qemu side symbolic name.  For
> platforms that do have a numbering convention those names can be
> derived from the domain numbers, but it also works for platforms that
> don't.

I agree with this last statement: using bus numbers and domain
numbers is not a good idea. We mostly need to support domain 0
to mean "default" for backwards compatibility.
I'll review the patchset shortly.

-- 
MST

Re: [Qemu-devel] [Qemu-ppc] [0/2] pseries: Rework PCI code for handling multiple PHBs

[David Gibson]

On Tue, Nov 27, 2012 at 02:36:57PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 27, 2012 at 05:07:31PM +1100, David Gibson wrote:
> > Hi Michael, Alex,
> > 
> > This patch represents a compromise I hope will be acceptable after the
> > long thread discussing handling of multiple PCI host bridges on the
> > pseries machine.  Patch 1/1 is just a preliminary enforcing uniqueness
> > of LIOBNs in the IOMMU code.
> > 
> > Patch 2/2 is the meat.  It allows either explicit configuration of all
> > the properties, or the user can just set an abstract index which will
> > generate sensible and probably-unique values for all the rest.
> > 
> > With these patches I was able to do something like:
> > 	qemu-system-ppc64 -M pseries -m 1024 -nographic \
> > 	fc17-root.qcow2 -net none -device nec-usb-xhci -device \
> > 	spapr-pci-host-bridge,index=1 -device e1000,netdev=fred \
> > 	-netdev user,id=fred
> > 
> > I was able to see both the PCI domains in the guest, and use the NIC
> > on the secondary domain.
> > 
> > There are still some gotches with multiple domains though.  The domain
> > value in PCIHostBus is still always initialized to 0, and there are
> > other places in the PCI core where handling of multiple domains is
> > essentially stubbed out.
> > 
> > Michael, any thoughts on what to do about that?  I could fix up the
> > PCI code so that domain is actually set and used.  But I think the
> > whole notion of domain numbers is kind of bogus on the qemu side:
> > since PCI domains are completely independent from each other, it's
> > only platform convention which determines what the domain numbers are.
> > On platforms that don't have a strong convention, the guest will
> > number them itself and we have no way of knowing that.  So it seems to
> > me that the PCI code should instead of domain numbers just use the
> > device ID, or the bus name or some qemu side symbolic name.  For
> > platforms that do have a numbering convention those names can be
> > derived from the domain numbers, but it also works for platforms that
> > don't.
> 
> I agree with this last statement: using bus numbers and domain
> numbers is not a good idea. We mostly need to support domain 0
> to mean "default" for backwards compatibility.

Ok.  Where are domain numbers actually specified by the qemu user?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [0/2] pseries: Rework PCI code for handling multiple PHBs

[Michael S. Tsirkin]

On Wed, Nov 28, 2012 at 01:50:53PM +1100, David Gibson wrote:
> On Tue, Nov 27, 2012 at 02:36:57PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 27, 2012 at 05:07:31PM +1100, David Gibson wrote:
> > > Hi Michael, Alex,
> > > 
> > > This patch represents a compromise I hope will be acceptable after the
> > > long thread discussing handling of multiple PCI host bridges on the
> > > pseries machine.  Patch 1/1 is just a preliminary enforcing uniqueness
> > > of LIOBNs in the IOMMU code.
> > > 
> > > Patch 2/2 is the meat.  It allows either explicit configuration of all
> > > the properties, or the user can just set an abstract index which will
> > > generate sensible and probably-unique values for all the rest.
> > > 
> > > With these patches I was able to do something like:
> > > 	qemu-system-ppc64 -M pseries -m 1024 -nographic \
> > > 	fc17-root.qcow2 -net none -device nec-usb-xhci -device \
> > > 	spapr-pci-host-bridge,index=1 -device e1000,netdev=fred \
> > > 	-netdev user,id=fred
> > > 
> > > I was able to see both the PCI domains in the guest, and use the NIC
> > > on the secondary domain.
> > > 
> > > There are still some gotches with multiple domains though.  The domain
> > > value in PCIHostBus is still always initialized to 0, and there are
> > > other places in the PCI core where handling of multiple domains is
> > > essentially stubbed out.
> > > 
> > > Michael, any thoughts on what to do about that?  I could fix up the
> > > PCI code so that domain is actually set and used.  But I think the
> > > whole notion of domain numbers is kind of bogus on the qemu side:
> > > since PCI domains are completely independent from each other, it's
> > > only platform convention which determines what the domain numbers are.
> > > On platforms that don't have a strong convention, the guest will
> > > number them itself and we have no way of knowing that.  So it seems to
> > > me that the PCI code should instead of domain numbers just use the
> > > device ID, or the bus name or some qemu side symbolic name.  For
> > > platforms that do have a numbering convention those names can be
> > > derived from the domain numbers, but it also works for platforms that
> > > don't.
> > 
> > I agree with this last statement: using bus numbers and domain
> > numbers is not a good idea. We mostly need to support domain 0
> > to mean "default" for backwards compatibility.
> 
> Ok.  Where are domain numbers actually specified by the qemu user?

pci_parse_devaddr

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson