diff mbox

[7/7] pseries: Configure PCI bridge using properties

Message ID 1330399093-20384-8-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Feb. 28, 2012, 3:18 a.m. UTC 
From: Alexey Kardashevskiy <aik@ozlabs.ru>

Currently, the function spapr_create_phb() uses its parameters to
initialize the correct memory windows for the new PCI Host Bridge
(PHB).  This is not the way things are supposed to be done with qdevs,
and means you can't create extra PHBs easily using -device.

Since pSeries machines can and do have many PHBs with various
configurations, this is a real limitation, not just a theoretical.
This patch, therefore, alters the PHB initialization code to use qdev
properties to set these parameters of the new bridge, moving most of
the code from spapr_create_phb() to spapr_phb_init().

While we're at it, we change the naming of each PCI bus and its
associated memory regions to be less arbitrary and make it easier to
relate the guest and qemu views of memory to each other.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr.c     |    2 +-
 hw/spapr_pci.c |  149 ++++++++++++++++++++++++++++++-------------------------
 hw/spapr_pci.h |    5 +-
 3 files changed, 84 insertions(+), 72 deletions(-)

Comments

Andreas Färber March 3, 2012, 6:42 p.m. UTC | #1 
Am 28.02.2012 04:18, schrieb David Gibson:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> Currently, the function spapr_create_phb() uses its parameters to
> initialize the correct memory windows for the new PCI Host Bridge
> (PHB).  This is not the way things are supposed to be done with qdevs,
> and means you can't create extra PHBs easily using -device.
> 
> Since pSeries machines can and do have many PHBs with various
> configurations, this is a real limitation, not just a theoretical.
> This patch, therefore, alters the PHB initialization code to use qdev
> properties to set these parameters of the new bridge, moving most of
> the code from spapr_create_phb() to spapr_phb_init().
> 
> While we're at it, we change the naming of each PCI bus and its
> associated memory regions to be less arbitrary and make it easier to
> relate the guest and qemu views of memory to each other.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/spapr.c     |    2 +-
>  hw/spapr_pci.c |  149 ++++++++++++++++++++++++++++++-------------------------
>  hw/spapr_pci.h |    5 +-
>  3 files changed, 84 insertions(+), 72 deletions(-)

> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index b0254ee..654cb56 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -161,49 +161,6 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
>      qemu_set_irq(phb->lsi_table[irq_num].qirq, level);
>  }
>  
> -static int spapr_phb_init(SysBusDevice *s)
> -{
> -    sPAPRPHBState *phb = FROM_SYSBUS(sPAPRPHBState, s);
> -    int i;
> -
> -    /* Initialize the LSI table */
> -    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
> -        qemu_irq qirq;
> -        uint32_t num;
> -
> -        qirq = spapr_allocate_lsi(0, &num);
> -        if (!qirq) {
> -            return -1;
> -        }
> -
> -        phb->lsi_table[i].dt_irq = num;
> -        phb->lsi_table[i].qirq = qirq;
> -    }
> -
> -    return 0;
> -}
> -
> -static void spapr_phb_class_init(ObjectClass *klass, void *data)
> -{
> -    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> -
> -    sdc->init = spapr_phb_init;
> -}
> -
> -static TypeInfo spapr_phb_info = {
> -    .name          = "spapr-pci-host-bridge",
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(sPAPRPHBState),
> -    .class_init    = spapr_phb_class_init,
> -};
> -
> -static void spapr_register_types(void)
> -{
> -    type_register_static(&spapr_phb_info);
> -}
> -
> -type_init(spapr_register_types)
> -
>  static uint64_t spapr_io_read(void *opaque, target_phys_addr_t addr,
>                                unsigned size)
>  {

> @@ -297,14 +248,76 @@ void spapr_create_phb(sPAPREnvironment *spapr,
>                                                   PCI_DEVFN(0, 0),
>                                                   SPAPR_PCI_NUM_LSI);
>  
> +    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
> +
> +    /* Initialize the LSI table */
> +    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
> +        qemu_irq qirq;
> +        uint32_t num;
> +
> +        qirq = spapr_allocate_lsi(0, &num);
> +        if (!qirq) {
> +            return -1;
> +        }
> +
> +        phb->lsi_table[i].dt_irq = num;
> +        phb->lsi_table[i].qirq = qirq;
> +    }
> +
> +    return 0;
> +}
> +
> +static Property spapr_phb_properties[] = {
> +    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
> +    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_END_OF_LIST(),
> +};
> +
> +static void spapr_phb_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    sdc->init = spapr_phb_init;
> +    dc->props = spapr_phb_properties;
> +}
> +
> +static TypeInfo spapr_phb_info = {
> +    .name          = "spapr-pci-host-bridge",
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(sPAPRPHBState),
> +    .class_init    = spapr_phb_class_init,
> +};
> +
> +static void spapr_register_pci_type(void)
> +{
> +    type_register_static(&spapr_phb_info);
> +
>      spapr_rtas_register("read-pci-config", rtas_read_pci_config);
>      spapr_rtas_register("write-pci-config", rtas_write_pci_config);
>      spapr_rtas_register("ibm,read-pci-config", rtas_ibm_read_pci_config);
>      spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
> +}
>  
> -    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
> +type_init(spapr_register_pci_type)

Please respect the recently enforced convention of naming the function
..._register_types as before and best putting it and type_init() in the
bottom so that we don't end up with multiple ones per file.

And registering types is all such a function can safely do since the
call of these constructors is about to be moved (spapr_rtas_register
looks fishy in that aspect).

Andreas
David Gibson March 6, 2012, 5:22 a.m. UTC | #2 
On Sat, Mar 03, 2012 at 07:42:09PM +0100, Andreas Färber wrote:
> Am 28.02.2012 04:18, schrieb David Gibson:
> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
[snip]
> > -    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
> > +type_init(spapr_register_pci_type)
> 
> Please respect the recently enforced convention of naming the function
> ..._register_types as before and best putting it and type_init() in the
> bottom so that we don't end up with multiple ones per file.

Now that I'm aware of the convention, sure.

> And registering types is all such a function can safely do since the
> call of these constructors is about to be moved (spapr_rtas_register
> looks fishy in that aspect).

Ok, I'll do the rename and move the rtas register calls into the
class_init function and respin.
diff mbox

Patch

diff --git a/hw/spapr.c b/hw/spapr.c
index 828bc53..d1cb6cd 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -629,7 +629,7 @@  static void ppc_spapr_init(ram_addr_t ram_size,
     }
 
     /* Set up PCI */
-    spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
+    spapr_create_phb(spapr, SPAPR_PCI_BUID,
                      SPAPR_PCI_MEM_WIN_ADDR,
                      SPAPR_PCI_MEM_WIN_SIZE,
                      SPAPR_PCI_IO_WIN_ADDR);
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index b0254ee..654cb56 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -161,49 +161,6 @@  static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(phb->lsi_table[irq_num].qirq, level);
 }
 
-static int spapr_phb_init(SysBusDevice *s)
-{
-    sPAPRPHBState *phb = FROM_SYSBUS(sPAPRPHBState, s);
-    int i;
-
-    /* Initialize the LSI table */
-    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
-        qemu_irq qirq;
-        uint32_t num;
-
-        qirq = spapr_allocate_lsi(0, &num);
-        if (!qirq) {
-            return -1;
-        }
-
-        phb->lsi_table[i].dt_irq = num;
-        phb->lsi_table[i].qirq = qirq;
-    }
-
-    return 0;
-}
-
-static void spapr_phb_class_init(ObjectClass *klass, void *data)
-{
-    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
-
-    sdc->init = spapr_phb_init;
-}
-
-static TypeInfo spapr_phb_info = {
-    .name          = "spapr-pci-host-bridge",
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(sPAPRPHBState),
-    .class_init    = spapr_phb_class_init,
-};
-
-static void spapr_register_types(void)
-{
-    type_register_static(&spapr_phb_info);
-}
-
-type_init(spapr_register_types)
-
 static uint64_t spapr_io_read(void *opaque, target_phys_addr_t addr,
                               unsigned size)
 {
@@ -241,35 +198,29 @@  static MemoryRegionOps spapr_io_ops = {
     .write = spapr_io_write
 };
 
-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)
+/*
+ * PHB PCI device
+ */
+static int spapr_phb_init(SysBusDevice *s)
 {
-    DeviceState *dev;
-    SysBusDevice *s;
-    sPAPRPHBState *phb;
+    sPAPRPHBState *phb = FROM_SYSBUS(sPAPRPHBState, s);
+    char busname[32];
+    char namebuf[64];
+    int i;
     PCIBus *bus;
-    char namebuf[strlen(busname)+11];
 
-    dev = qdev_create(NULL, "spapr-pci-host-bridge");
-    qdev_init_nofail(dev);
-    s = sysbus_from_qdev(dev);
-    phb = FROM_SYSBUS(sPAPRPHBState, s);
+    sprintf(busname, "pci@%" PRIx64, phb->buid);
 
-    phb->mem_win_addr = mem_win_addr;
-
-    sprintf(namebuf, "%s-mem", busname);
+    /* Initialize memory regions */
+    sprintf(namebuf, "%s.mmio", busname);
     memory_region_init(&phb->memspace, namebuf, INT64_MAX);
 
-    sprintf(namebuf, "%s-memwindow", busname);
+    sprintf(namebuf, "%s.mmio-alias", busname);
     memory_region_init_alias(&phb->memwindow, namebuf, &phb->memspace,
-                             SPAPR_PCI_MEM_WIN_BUS_OFFSET, mem_win_size);
-    memory_region_add_subregion(get_system_memory(), mem_win_addr,
+                             SPAPR_PCI_MEM_WIN_BUS_OFFSET, phb->mem_win_size);
+    memory_region_add_subregion(get_system_memory(), phb->mem_win_addr,
                                 &phb->memwindow);
 
-    phb->io_win_addr = io_win_addr;
-
     /* On ppc, we only have MMIO no specific IO space from the CPU
      * perspective.  In theory we ought to be able to embed the PCI IO
      * memory region direction in the system memory space.  However,
@@ -278,15 +229,15 @@  void spapr_create_phb(sPAPREnvironment *spapr,
      * system io address space.  This hack to bounce things via
      * system_io works around the problem until all the users of
      * old_portion are updated */
-    sprintf(namebuf, "%s-io", busname);
+    sprintf(namebuf, "%s.io", busname);
     memory_region_init(&phb->iospace, namebuf, SPAPR_PCI_IO_WIN_SIZE);
     /* FIXME: fix to support multiple PHBs */
     memory_region_add_subregion(get_system_io(), 0, &phb->iospace);
 
-    sprintf(namebuf, "%s-iowindow", busname);
+    sprintf(namebuf, "%s.io-alias", busname);
     memory_region_init_io(&phb->iowindow, &spapr_io_ops, phb,
                           namebuf, SPAPR_PCI_IO_WIN_SIZE);
-    memory_region_add_subregion(get_system_memory(), io_win_addr,
+    memory_region_add_subregion(get_system_memory(), phb->io_win_addr,
                                 &phb->iowindow);
 
     phb->host_state.bus = bus = pci_register_bus(&phb->busdev.qdev, busname,
@@ -297,14 +248,76 @@  void spapr_create_phb(sPAPREnvironment *spapr,
                                                  PCI_DEVFN(0, 0),
                                                  SPAPR_PCI_NUM_LSI);
 
+    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
+
+    /* Initialize the LSI table */
+    for (i = 0; i < SPAPR_PCI_NUM_LSI; i++) {
+        qemu_irq qirq;
+        uint32_t num;
+
+        qirq = spapr_allocate_lsi(0, &num);
+        if (!qirq) {
+            return -1;
+        }
+
+        phb->lsi_table[i].dt_irq = num;
+        phb->lsi_table[i].qirq = qirq;
+    }
+
+    return 0;
+}
+
+static Property spapr_phb_properties[] = {
+    DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, 0),
+    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_END_OF_LIST(),
+};
+
+static void spapr_phb_class_init(ObjectClass *klass, void *data)
+{
+    SysBusDeviceClass *sdc = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    sdc->init = spapr_phb_init;
+    dc->props = spapr_phb_properties;
+}
+
+static TypeInfo spapr_phb_info = {
+    .name          = "spapr-pci-host-bridge",
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(sPAPRPHBState),
+    .class_init    = spapr_phb_class_init,
+};
+
+static void spapr_register_pci_type(void)
+{
+    type_register_static(&spapr_phb_info);
+
     spapr_rtas_register("read-pci-config", rtas_read_pci_config);
     spapr_rtas_register("write-pci-config", rtas_write_pci_config);
     spapr_rtas_register("ibm,read-pci-config", rtas_ibm_read_pci_config);
     spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
+}
 
-    QLIST_INSERT_HEAD(&spapr->phbs, phb, list);
+type_init(spapr_register_pci_type)
+
+void spapr_create_phb(sPAPREnvironment *spapr, uint64_t buid,
+                      uint64_t mem_win_addr, uint64_t mem_win_size,
+                      uint64_t io_win_addr)
+{
+    DeviceState *dev;
 
-    /* pci_bus_set_mem_base(bus, mem_va_start - SPAPR_PCI_MEM_BAR_START); */
+    dev = qdev_create(NULL, spapr_phb_info.name);
+
+    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_init_nofail(dev);
 }
 
 /* Macros to operate with address in OF binding to PCI */
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index 213340c..b4b8a73 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -35,7 +35,7 @@  typedef struct sPAPRPHBState {
     uint64_t buid;
 
     MemoryRegion memspace, iospace;
-    target_phys_addr_t mem_win_addr, io_win_addr;
+    target_phys_addr_t mem_win_addr, mem_win_size, io_win_addr, io_win_size;
     MemoryRegion memwindow, iowindow;
 
     struct {
@@ -49,8 +49,7 @@  typedef struct sPAPRPHBState {
 #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,
+void spapr_create_phb(sPAPREnvironment *spapr, uint64_t buid,
                       uint64_t mem_win_addr, uint64_t mem_win_size,
                       uint64_t io_win_addr);