[Qemu-devel] [RFC PATCH 00/15] spapr: add support for PHB hotplug

[Qemu-devel] [RFC PATCH 00/15] spapr: add support for PHB hotplug

[Michael Roth]

These patches are based on latest spapr-hotplug-pci patches, and
can also be obtained from:

  https://github.com/mdroth/qemu/commits/spapr-hotplug-phb

These patches implement support for hotplug/unplug of PCI host-bridges.
The main use cases are:

 - allowing for VFIO PCI hotplug for host kernels that still require a
   1:1 mapping between guest PHB/TCE table and an iommu group (a
   requirement that will be relaxed with Alexey Kardashevskiy's VFIO
   rework for DDW support)
 - allocating new PHBs/TCE tables for hotplugging/distributing VFIO
   devices that have different NUMA affinities associated with them
   for performance reason
 - expanding hotplug capacity for passthrough/emulated PCI devices

With these patches we support the following:

(qemu) device_add spapr-pci-host-bridge,index=2,id=phb2
(qemu) device_add virtio-net-pci,id=hp2.0,bus=phb2.0
(qemu) device_del hp2.0
(qemu) device_del phb2

Automatic add/remove of PHBs based on EPOW event mechanism require
updated versions of powerpc-utils, rtas_errd, and librtas. Patches
are forthcoming and will be available in future versions, but for now
we can add them manually by executing the following in the guest
after/before hotplug/unplug, respectively:

  # add PHB
  drmgr -c PHB -s "PHB 2" -a -n

  # remove PHB
  drmgr -c PHB -s "PHB 2" -r -n

Feedback/comments are very much appreciated.

 hw/core/qdev.c              |  24 ++++++++++++------
 hw/pci/pci.c                |  33 +++++++++++++++++++++++++
 hw/ppc/spapr.c              | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_drc.c          |   1 +
 hw/ppc/spapr_events.c       |   5 ++++
 hw/ppc/spapr_iommu.c        |   1 +
 hw/ppc/spapr_pci.c          |  66 ++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/pci-host/spapr.h |   3 ++-
 include/hw/pci/pci.h        |   3 +++
 include/hw/ppc/spapr.h      |   1 +
 include/hw/qdev-core.h      |   3 +++
 11 files changed, 310 insertions(+), 13 deletions(-)

[Qemu-devel] [RFC PATCH 01/15] pci: allow cleanup/unregistration of PCI buses

[Michael Roth]

This adds cleanup counterparts to pci_register_bus(),
pci_bus_new(), and pci_bus_irqs().

These cleanup routines are needed in the case of hotpluggable
PCIHostBridge implementations. Currently we can rely on the
object_unparent()'ing of the PCIHostState recursively unparenting
and cleaning up it's child buses, but we need explicit calls
to also:

  1) remove the PCIHostState from pci_host_bridges global list.
     otherwise, we risk accessing freed memory when we access
     the list later
  2) clean up memory allocated in pci_bus_irqs()

Both are handled outside the context of any particular bus or
host bridge's init/realize functions, making it difficult to
avoid the need for explicit cleanup functions without remodeling
how PCIHostBridges are created. So keep it simple and just add
them for now.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/pci/pci.c         | 33 +++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h |  3 +++
 2 files changed, 36 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a88c8f3..201ad49 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -258,6 +258,13 @@ static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
     QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
 }
 
+static void pci_host_bus_unregister(DeviceState *parent)
+{
+    PCIHostState *host_bridge = PCI_HOST_BRIDGE(parent);
+
+    QLIST_REMOVE(host_bridge, next);
+}
+
 PCIBus *pci_find_primary_bus(void)
 {
     PCIBus *primary_bus = NULL;
@@ -318,6 +325,11 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
     pci_host_bus_register(bus, parent);
 }
 
+static void pci_bus_uninit(PCIBus *bus)
+{
+    pci_host_bus_unregister(BUS(bus)->parent);
+}
+
 bool pci_bus_is_express(PCIBus *bus)
 {
     return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
@@ -352,6 +364,12 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
     return bus;
 }
 
+void pci_bus_cleanup(PCIBus *bus)
+{
+    pci_bus_uninit(bus);
+    object_unparent(OBJECT(bus));
+}
+
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq)
 {
@@ -362,6 +380,15 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
+void pci_bus_irqs_cleanup(PCIBus *bus)
+{
+    bus->set_irq = NULL;
+    bus->map_irq = NULL;
+    bus->irq_opaque = NULL;
+    bus->nirq = 0;
+    g_free(bus->irq_count);
+}
+
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque,
@@ -377,6 +404,12 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     return bus;
 }
 
+void pci_unregister_bus(PCIBus *bus)
+{
+    pci_bus_irqs_cleanup(bus);
+    pci_bus_cleanup(bus);
+}
+
 int pci_bus_num(PCIBus *s)
 {
     if (pci_bus_is_root(s))
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 1a4e0be..eb4a292 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -353,8 +353,10 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     uint8_t devfn_min, const char *typename);
+void pci_bus_cleanup(PCIBus *bus);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
+void pci_bus_irqs_cleanup(PCIBus *bus);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
@@ -364,6 +366,7 @@ PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, int nirq, const char *typename);
+void pci_unregister_bus(PCIBus *bus);
 void pci_bus_set_route_irq_fn(PCIBus *, pci_route_irq_fn);
 PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin);
 bool pci_intx_route_changed(PCIINTxRoute *old, PCIINTxRoute *new);
-- 
1.9.1

[Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Michael Roth]

device_unparent(dev, ...) is called when a device is unparented,
either directly, or as a result of a parent device being
finalized, and handles some final cleanup for the device. Part
of this includes emiting a DEVICE_DELETED QMP event to notify
management, which includes the device's path in the composition
tree as provided by object_get_canonical_path().

object_get_canonical_path() assumes the device is still connected
to the machine/root container, and will assert otherwise, but
in some situations this isn't the case:

If the parent is finalized as a result of object_unparent(), it
will still be attached to the composition tree at the time any
children are unparented as a result of that same call to
object_unparent(). However, in some cases, object_unparent()
will complete without finalizing the parent device, due to
lingering references that won't be released till some time later.
One such example is if the parent has MemoryRegion children (which
take a ref on their parent), who in turn have AddressSpace's (which
take a ref on their regions), since those AddressSpaces get cleaned
up asynchronously by the RCU thread.

In this case qdev:device_unparent() may be called for a child Device
that no longer has a path to the root/machine container, causing
object_get_canonical_path() to assert.

Fix this by storing the canonical path during realize() so the
information will still be available for device_unparent() in such
cases.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/core/qdev.c         | 15 ++++++++++++---
 include/hw/qdev-core.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6be5866..fda1d2f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1072,6 +1072,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             goto post_realize_fail;
         }
 
+        /* always re-initialize since we clean up in device_unparent() instead
+         * of unrealize()
+         */
+        g_free(dev->canonical_path);
+        dev->canonical_path = object_get_canonical_path(OBJECT(dev));
+
         if (qdev_get_vmsd(dev)) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
@@ -1125,6 +1131,7 @@ child_realize_fail:
     }
 
 post_realize_fail:
+    g_free(dev->canonical_path);
     if (dc->unrealize) {
         dc->unrealize(dev, NULL);
     }
@@ -1248,10 +1255,12 @@ static void device_unparent(Object *obj)
 
     /* Only send event if the device had been completely realized */
     if (dev->pending_deleted_event) {
-        gchar *path = object_get_canonical_path(OBJECT(dev));
+        g_assert(dev->canonical_path);
 
-        qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
-        g_free(path);
+        qapi_event_send_device_deleted(!!dev->id, dev->id, dev->canonical_path,
+                                       &error_abort);
+        g_free(dev->canonical_path);
+        dev->canonical_path = NULL;
     }
 }
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..17f805e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -153,6 +153,7 @@ struct DeviceState {
     /*< public >*/
 
     const char *id;
+    char *canonical_path;
     bool realized;
     bool pending_deleted_event;
     QemuOpts *opts;
-- 
1.9.1

[Qemu-devel] [RFC PATCH 03/15] spapr_drc: pass object ownership to parent/owner

[Michael Roth]

DRC objects attach themselves to an owner as a child
property. unref afterward to allow them to be finalized
when their owner is finalized.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_drc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 48bf193..396a03b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -456,6 +456,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
     drc->id = id;
     drc->owner = owner;
     object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
+    object_unref(OBJECT(drc));
     object_property_set_bool(OBJECT(drc), true, "realized", NULL);
 
     /* human-readable name for a DRC to encode into the DT
-- 
1.9.1

[Qemu-devel] [RFC PATCH 04/15] spapr_iommu: pass object ownership to parent/owner

[Michael Roth]

DRC objects attach themselves to an owner as a child
property. unref afterward to allow them to be finalized
when their owner is finalized.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index a14cdc4..79e998b 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -182,6 +182,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
 
     snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
     object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
+    object_unref(OBJECT(tcet));
 
     object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
 
-- 
1.9.1

[Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize

[Michael Roth]

To support PHB hotplug we need to clean up lingering references,
memory, child properties, etc. prior to the PHB object being
finalized. Generally this will be called as a result of calling
object_unref() on the PHB object, which in turn would normally
be called as the result of an unplug() operation.

When the PHB is finalized, child objects will be unparented in
turn, and finalized if the PHB was the only reference holder. so
we don't bother to explicitly unparent child objects of the PHB
(spapr_iommu, spapr_drc, etc).

We do need to handle memory regions explicitly however, since
they also take a reference on the PHB, and won't allow it to
be finalized otherwise.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2e7590c..25a738c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
     }
 }
 
+static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *s = SYS_BUS_DEVICE(dev);
+    PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
+    sPAPRTCETable *tcet;
+
+    pci_unregister_bus(phb->bus);
+
+    g_free(sphb->dtbusname);
+    sphb->dtbusname = NULL;
+
+    /* remove IO/MMIO subregions and aliases, rest should get cleaned
+     * via PHB's unrealize->object_finalize
+     */
+    memory_region_del_subregion(get_system_memory(), &sphb->iowindow);
+    object_unparent(OBJECT(&sphb->iowindow));
+    object_unparent(OBJECT(&sphb->iospace));
+
+    memory_region_del_subregion(get_system_memory(), &sphb->memwindow);
+    object_unparent(OBJECT(&sphb->memwindow));
+    object_unparent(OBJECT(&sphb->memspace));
+
+    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
+    memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
+    memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet));
+    address_space_destroy(&sphb->iommu_as);
+
+    QLIST_REMOVE(sphb, list);
+}
+
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
@@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
 
     hc->root_bus_path = spapr_phb_root_bus_path;
     dc->realize = spapr_phb_realize;
+    dc->unrealize = spapr_phb_unrealize;
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
     dc->vmsd = &vmstate_spapr_pci;
-- 
1.9.1

[Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs

[Michael Roth]

Prior to this patch 'index' is purely a shorthand for specifying
MMIO windows, BUIDs, and other configuration values for a PHB.

With the addition of PHB hotplug, we have a static number of DRCs
that can be used to handle hotplug/unplug operations on our PHBs,
and need a consistent way to map PHBs to these connectors, and
assign a unique identifiers for the connectors.

BUIDs would be a good choice, however, those are 64-bit values,
whereas DRC indexes are 32-bit.

'index' serves this purpose nicely, and also allows us to align
the maximum number PHBs that can be plugged with the maximum
'index' value we allow (255).

This means that when PHB hotplug is enabled (2.4+), 'index' is
now always a required value, regardless of whether or not other
configuration properties are specified explicitly. We could
potentially arrange for 'index'-less PHBs to be added in an
'unpluggable' fashion via command-line, and have checks to
generate an error when hotplugged via device_add, but the simpler
path seems to be to just make it required now.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 25a738c..e37de28 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1)
             || (sphb->mem_win_addr != (hwaddr)-1)
             || (sphb->io_win_addr != (hwaddr)-1)) {
-            error_setg(errp, "Either \"index\" or other parameters must"
-                       " be specified for PAPR PHB, not both");
+            if (!spapr->dr_phb_enabled) {
+                /* if they aren't potentially using index as an identifier for
+                 * the PHB's DR connector, enforce the old semantics of index
+                 * being purely a shorthand for PHB configuration options.
+                 */
+                error_setg(errp, "Either \"index\" or other parameters must"
+                           " be specified for PAPR PHB, not both");
+            }
             return;
         }
 
@@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
             + 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;
+    } else {
+        if (spapr->dr_phb_enabled) {
+            error_setg(errp, "The \"index\" property is required for machine"
+                       " types that support PHB hotplug (and in such cases"
+                       " can be used alongside \"buid\" and other"
+                       " configuration properties)");
+            return;
+        }
     }
 
     if (sphb->buid == (uint64_t)-1) {
-- 
1.9.1

[Qemu-devel] [RFC PATCH 07/15] spapr: enable PHB hotplug for pseries-2.4

[Michael Roth]

Introduce an sPAPRMachineClass sub-class of MachineClass to
handle sPAPR-specific machine configuration properties.

The 'dr_phb_enabled' field of that class can be set as
part of machine-specific init code, and is then propagated
to sPAPREnvironment to conditionally enable creation of DRC
objects and device-tree description to facilitate hotplug
of PHBs.

Since we can't migrate this state to older machine types,
default the option to false and only enable it for new
machine types.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         | 24 ++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 837b36d..c8ad5b0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -90,11 +90,27 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+typedef struct sPAPRMachineClass sPAPRMachineClass;
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 #define TYPE_SPAPR_MACHINE      "spapr-machine"
 #define SPAPR_MACHINE(obj) \
     OBJECT_CHECK(sPAPRMachineState, (obj), TYPE_SPAPR_MACHINE)
+#define SPAPR_MACHINE_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(sPAPRMachineClass, obj, TYPE_SPAPR_MACHINE)
+#define SPAPR_MACHINE_CLASS(klass) \
+        OBJECT_CLASS_CHECK(sPAPRMachineClass, klass, TYPE_SPAPR_MACHINE)
+
+/**
+ * sPAPRMachineClass:
+ */
+struct sPAPRMachineClass {
+    /*< private >*/
+    MachineClass parent_class;
+
+    /*< public >*/
+    bool dr_phb_enabled; /* enable dynamic-reconfig/hotplug of PHBs */
+};
 
 /**
  * sPAPRMachineState:
@@ -1374,6 +1390,7 @@ static SaveVMHandlers savevm_htab_handlers = {
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
     ram_addr_t ram_size = machine->ram_size;
     const char *cpu_model = machine->cpu_model;
     const char *kernel_filename = machine->kernel_filename;
@@ -1542,6 +1559,8 @@ static void ppc_spapr_init(MachineState *machine)
     /* We always have at least the nvram device on VIO */
     spapr_create_nvram(spapr);
 
+    spapr->dr_phb_enabled = smc->dr_phb_enabled;
+
     /* Set up PCI */
     spapr_pci_rtas_init();
 
@@ -1776,6 +1795,7 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
 
@@ -1787,6 +1807,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = NULL;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
+    smc->dr_phb_enabled = false;
 
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;
@@ -1798,6 +1819,7 @@ static const TypeInfo spapr_machine_info = {
     .abstract      = true,
     .instance_size = sizeof(sPAPRMachineState),
     .instance_init = spapr_machine_initfn,
+    .class_size    = sizeof(sPAPRMachineClass),
     .class_init    = spapr_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_FW_PATH_PROVIDER },
@@ -1887,11 +1909,13 @@ static const TypeInfo spapr_machine_2_3_info = {
 static void spapr_machine_2_4_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
 
     mc->name = "pseries-2.4";
     mc->desc = "pSeries Logical Partition (PAPR compliant) v2.4";
     mc->alias = "pseries";
     mc->is_default = 1;
+    smc->dr_phb_enabled = true;
 }
 
 static const TypeInfo spapr_machine_2_4_info = {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 3ea0c5b..be8973e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -33,6 +33,7 @@ typedef struct sPAPREnvironment {
     uint64_t rtc_offset; /* Now used only during incoming migration */
     struct PPCTimebase tb;
     bool has_graphics;
+    bool dr_phb_enabled; /* hotplug / dynamic-reconfiguration of PHBs */
 
     uint32_t check_exception_irq;
     Notifier epow_notifier;
-- 
1.9.1

[Qemu-devel] [RFC PATCH 08/15] spapr: create DR connectors for PHBs and register reset hooks

[Michael Roth]

Since we route hotplugged PHBs to their DR connector using their
PHB.index value, we align the number of DR connectors with the
maximum index value: SPAPR_PCI_MAX_INDEX.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c8ad5b0..c539932 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -90,6 +90,9 @@
 
 #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
 
+/* maximum number of hotpluggable PHBs */
+#define SPAPR_DRC_MAX_PHB       (SPAPR_PCI_MAX_INDEX + 1)
+
 typedef struct sPAPRMachineClass sPAPRMachineClass;
 typedef struct sPAPRMachineState sPAPRMachineState;
 
@@ -1387,6 +1390,16 @@ static SaveVMHandlers savevm_htab_handlers = {
     .load_state = htab_load,
 };
 
+static void spapr_drc_reset(void *opaque)
+{
+    sPAPRDRConnector *drc = opaque;
+    DeviceState *d = DEVICE(drc);
+
+    if (d) {
+        device_reset(d);
+    }
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(MachineState *machine)
 {
@@ -1561,6 +1574,21 @@ static void ppc_spapr_init(MachineState *machine)
 
     spapr->dr_phb_enabled = smc->dr_phb_enabled;
 
+    /* Setup hotplug / dynamic-reconfiguration connectors. top-level
+     * connectors (described in root DT node's "ibm,drc-types" property)
+     * are pre-initialized here. additional child connectors (such as
+     * connectors for a PHBs PCI slots) are added as needed during their
+     * parent's realization.
+     */
+    if (spapr->dr_phb_enabled) {
+        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
+            sPAPRDRConnector *drc =
+                spapr_dr_connector_new(OBJECT(machine),
+                                       SPAPR_DR_CONNECTOR_TYPE_PHB, i);
+            qemu_register_reset(spapr_drc_reset, drc);
+        }
+    }
+
     /* Set up PCI */
     spapr_pci_rtas_init();
 
-- 
1.9.1

[Qemu-devel] [RFC PATCH 09/15] spapr: populate PHB DRC entries for root DT node

[Michael Roth]

From: Nathan Fontenot <nfont@linux.vnet.ibm.com>

This add entries to the root OF node to advertise our PHBs as being
DR-capable in accordance with PAPR specification.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c539932..a7af332 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -57,6 +57,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "hw/nmi.h"
+#include "hw/ppc/spapr_drc.h"
 
 #include "hw/compat.h"
 
@@ -745,6 +746,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     size_t cb = 0;
     char *bootlist;
     void *fdt;
+    int fdt_offset;
     sPAPRPHBState *phb;
 
     fdt = g_malloc(FDT_MAX_SIZE);
@@ -804,6 +806,13 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
         spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
     }
 
+    fdt_offset = fdt_path_offset(fdt, "/");
+    ret = spapr_drc_populate_dt(fdt, fdt_offset, NULL,
+                                SPAPR_DR_CONNECTOR_TYPE_PHB);
+    if (ret < 0) {
+        fprintf(stderr, "Couldn't set up RTAS device tree properties\n");
+    }
+
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
-- 
1.9.1

[Qemu-devel] [RFC PATCH 10/15] spapr_events: add support for phb hotplug events

[Michael Roth]

Extend the existing EPOW event format we use for PCI
devices to emit PHB plug/unplug events.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_events.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index be82815..5d7cfac 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -429,6 +429,11 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
         hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
         break;
+    case SPAPR_DR_CONNECTOR_TYPE_PHB:
+        hp->drc.index = cpu_to_be32(drck->get_index(drc));
+        hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
+        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PHB;
+        break;
     default:
         /* we shouldn't be signaling hotplug events for resources
          * that don't support them
-- 
1.9.1

[Qemu-devel] [RFC PATCH 11/15] qdev: add qbus_set_hotplug_handler_generic()

[Michael Roth]

Certain devices types, like memory/CPU, are now being handled using a
hotplug interface provided by a top-level MachineClass. Hotpluggable
host bridges are another such device where it makes sense to use a
machine-level hotplug handler. However, unlike those devices,
host-bridges have a parent bus (the main system bus), and devices with
a parent bus use a different mechanism for registering their hotplug
handlers: qbus_set_hotplug_handler(). This interface currently expects
a handler to be a subclass of DeviceClass, but this is not the case
for MachineClass, which derives directly from ObjectClass.

Internally, the interface only requires an ObjectClass, so expose that
support via a new qbus_set_hotplug_handler_generic().

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/core/qdev.c         | 9 ++++-----
 include/hw/qdev-core.h | 2 ++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index fda1d2f..8fd9320 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -108,22 +108,21 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     bus_add_child(bus, dev);
 }
 
-static void qbus_set_hotplug_handler_internal(BusState *bus, Object *handler,
-                                              Error **errp)
+void qbus_set_hotplug_handler_generic(BusState *bus, Object *handler,
+                                      Error **errp)
 {
-
     object_property_set_link(OBJECT(bus), OBJECT(handler),
                              QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
 }
 
 void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error **errp)
 {
-    qbus_set_hotplug_handler_internal(bus, OBJECT(handler), errp);
+    qbus_set_hotplug_handler_generic(bus, OBJECT(handler), errp);
 }
 
 void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
 {
-    qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
+    qbus_set_hotplug_handler_generic(bus, OBJECT(bus), errp);
 }
 
 /* Create a new device.  This only initializes the device state structure
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 17f805e..3b210bf 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -377,6 +377,8 @@ GSList *qdev_build_hotpluggable_device_list(Object *peripheral);
 
 void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
                               Error **errp);
+void qbus_set_hotplug_handler_generic(BusState *bus, Object *handler,
+                                      Error **errp);
 
 void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp);
 
-- 
1.9.1

[Qemu-devel] [RFC PATCH 12/15] spapr: stub implementation of machine-level HotplugHandler interface

[Michael Roth]

This is mostly a shim patch to base more easily on CPU/MEM patches.
In that case it can most likely be dropped, otherwise it should be
squashed into actual hotplug handler implementation that will follow.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a7af332..042e7a9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1829,12 +1829,32 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+}
+
+static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
+                                                 DeviceState *dev)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+        return HOTPLUG_HANDLER(machine);
+    }
+    return NULL;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc);
     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = ppc_spapr_init;
     mc->reset = ppc_spapr_reset;
@@ -1844,6 +1864,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = NULL;
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
+    mc->get_hotplug_handler = spapr_get_hotplug_handler;
+    hc->plug = spapr_machine_device_plug;
+    hc->unplug = spapr_machine_device_unplug;
     smc->dr_phb_enabled = false;
 
     fwc->get_dev_path = spapr_get_fw_dev_path;
@@ -1861,6 +1884,7 @@ static const TypeInfo spapr_machine_info = {
     .interfaces = (InterfaceInfo[]) {
         { TYPE_FW_PATH_PROVIDER },
         { TYPE_NMI },
+        { TYPE_HOTPLUG_HANDLER },
         { }
     },
 };
-- 
1.9.1

[Qemu-devel] [RFC PATCH 13/15] spapr_pci: provide node start offset via spapr_populate_pci_dt()

[Michael Roth]

PHB hotplug re-uses PHB device tree generation code and passes
it to a guest via RTAS. Doing this requires knowledge of where
exactly in the device tree the node describing the PHB begins.

Provide this via a new optional pointer that can be used to
store the PHB node's start offset.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c              | 2 +-
 hw/ppc/spapr_pci.c          | 6 +++++-
 include/hw/pci-host/spapr.h | 3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 042e7a9..ecf40e4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -767,7 +767,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
     }
 
     QLIST_FOREACH(phb, &spapr->phbs, list) {
-        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
+        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt, NULL);
     }
 
     if (ret < 0) {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e37de28..66fe85f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1534,7 +1534,8 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
 
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
-                          void *fdt)
+                          void *fdt,
+                          int *node_offset)
 {
     int bus_off, i, j, ret;
     char nodename[256];
@@ -1578,6 +1579,9 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
     if (bus_off < 0) {
         return bus_off;
     }
+    if (node_offset) {
+        *node_offset = bus_off;
+    }
 
     /* Write PHB properties */
     _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 9dca388..32a9213 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -126,7 +126,8 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
 
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
-                          void *fdt);
+                          void *fdt,
+                          int *node_offset);
 
 void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr);
 
-- 
1.9.1

[Qemu-devel] [RFC PATCH 14/15] spapr_pci: add ibm, my-drc-index property for PHB hotplug

[Michael Roth]

This is needed to denote a boot-time PHB as being hot-pluggable.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 66fe85f..91dfd96 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1572,6 +1572,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
         cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
     uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
     sPAPRTCETable *tcet;
+    sPAPRDRConnector *drc;
 
     /* Start populating the FDT */
     sprintf(nodename, "pci@%" PRIx64, phb->buid);
@@ -1624,6 +1625,15 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
                  tcet->liobn, tcet->bus_offset,
                  tcet->nb_table << tcet->page_shift);
 
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PHB, phb->index);
+    if (drc) {
+        sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+        uint32_t drc_index = cpu_to_be32(drck->get_index(drc));
+
+        _FDT(fdt_setprop(fdt, bus_off, "ibm,my-drc-index", &drc_index,
+                         sizeof(drc_index)));
+    }
+
     ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
                                 SPAPR_DR_CONNECTOR_TYPE_PCI);
     if (ret) {
-- 
1.9.1

[Qemu-devel] [RFC PATCH 15/15] spapr: add hotplug hooks for PHB hotplug

[Michael Roth]

Hotplugging PHBs is a machine-level operation, but PHBs reside on the
main system bus, so we register spapr machine as the handler for the
main system bus. The entry point for plug/unplug is shared by all
such machine-level hotplug operations (memory, CPU, PHB, etc), and
from there we branch off to specific hotplug callbacks based on the
object type.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ecf40e4..25c46bb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -62,6 +62,7 @@
 #include "hw/compat.h"
 
 #include <libfdt.h>
+#include "sysemu/device_tree.h"
 
 /* SLOF memory layout:
  *
@@ -1711,6 +1712,11 @@ static void ppc_spapr_init(MachineState *machine)
     /* used by RTAS */
     QTAILQ_INIT(&spapr->ccs_list);
     qemu_register_reset(spapr_ccs_reset_hook, spapr);
+
+    if (spapr->dr_phb_enabled) {
+        qbus_set_hotplug_handler_generic(sysbus_get_default(), OBJECT(machine),
+                                         NULL);
+    }
 }
 
 static int spapr_kvm_type(const char *vm_type)
@@ -1829,14 +1835,104 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static void spapr_machine_phb_plug(HotplugHandler *hotplug_dev,
+                                   DeviceState *dev, Error **errp)
+{
+    sPAPRPHBState *sphb;
+    void *fdt = NULL;
+    int fdt_start_offset = 0;
+    int fdt_size = 0;
+    Error *local_err = NULL;
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+
+    sphb = SPAPR_PCI_HOST_BRIDGE(dev);
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PHB, sphb->index);
+    /* hotplug hooks should check it's enabled before getting this far */
+    g_assert(drc);
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+    /* boot-time devices get their device tree node created by SLOF, but for
+     * hotplugged devices we need QEMU to generate it so the guest can fetch
+     * it via RTAS
+     */
+    if (dev->hotplugged) {
+        int ret;
+        fdt = create_device_tree(&fdt_size);
+        ret = spapr_populate_pci_dt(sphb, PHANDLE_XICP, fdt, &fdt_start_offset);
+        if (ret < 0) {
+            error_setg(&local_err, "unable to create FDT for hotplugged PHB");
+            goto out;
+        }
+
+        /* generally SLOF creates these, for hotplug it's up to QEMU */
+        _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
+    }
+
+    /* boot-time devices still get associated with a DRC to allow for unplug,
+     * but since we use SLOF-generated DT here we don't need to re-generate it
+     */
+    drck->attach(drc, DEVICE(dev),
+                 fdt, fdt_start_offset, !dev->hotplugged, &local_err);
+out:
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(fdt);
+        return;
+    }
+
+    if (dev->hotplugged) {
+        spapr_hotplug_req_add_event(drc);
+    }
+}
+
+static void spapr_machine_phb_remove_cb(DeviceState *dev, void *opaque)
+{
+    object_unparent(OBJECT(dev));
+}
+
+static void spapr_machine_phb_unplug(HotplugHandler *hotplug_dev,
+                                            DeviceState *dev, Error **errp)
+{
+    sPAPRPHBState *sphb;
+    sPAPRDRConnector *drc;
+    sPAPRDRConnectorClass *drck;
+    Error *local_err = NULL;
+
+    sphb = SPAPR_PCI_HOST_BRIDGE(dev);
+    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PHB, sphb->index);
+    g_assert(drc);
+    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+    if (!drck->release_pending(drc)) {
+        drck->detach(drc, DEVICE(dev), spapr_machine_phb_remove_cb, NULL,
+                     &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        spapr_hotplug_req_remove_event(drc);
+    }
+}
+
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+        if (spapr->dr_phb_enabled) {
+            spapr_machine_phb_plug(hotplug_dev, dev, errp);
+        }
+    }
 }
 
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+        if (spapr->dr_phb_enabled) {
+            spapr_machine_phb_unplug(hotplug_dev, dev, errp);
+        }
+    }
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
-- 
1.9.1

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> If the parent is finalized as a result of object_unparent(), it
> will still be attached to the composition tree at the time any
> children are unparented as a result of that same call to
> object_unparent(). However, in some cases, object_unparent()
> will complete without finalizing the parent device, due to
> lingering references that won't be released till some time later.
> One such example is if the parent has MemoryRegion children (which
> take a ref on their parent), who in turn have AddressSpace's (which
> take a ref on their regions), since those AddressSpaces get cleaned
> up asynchronously by the RCU thread.
> 
> In this case qdev:device_unparent() may be called for a child Device
> that no longer has a path to the root/machine container, causing
> object_get_canonical_path() to assert.

This doesn't seem right.  Unparent callbacks are _not_ called when you 
finalize, they are called in post-order as soon as you unplug a device 
(the call tree is object_unparent ==> device_unparent(parent) ==> 
bus_unparent(parent->bus) ==> device_unparent(parent->bus->child[0]) 
and so on).

DEVICE_DELETED is called after a device's children have been 
unparented.  It could be called after a bus is dead though.  Could it 
be that the patch you want is simply this:

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6e6a65d..46019c4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1241,11 +1241,6 @@ static void device_unparent(Object *obj)
         bus = QLIST_FIRST(&dev->child_bus);
         object_unparent(OBJECT(bus));
     }
-    if (dev->parent_bus) {
-        bus_remove_child(dev->parent_bus, dev);
-        object_unref(OBJECT(dev->parent_bus));
-        dev->parent_bus = NULL;
-    }
 
     /* Only send event if the device had been completely realized */
     if (dev->pending_deleted_event) {
@@ -1254,6 +1249,12 @@ static void device_unparent(Object *obj)
         qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
         g_free(path);
     }
+
+    if (dev->parent_bus) {
+        bus_remove_child(dev->parent_bus, dev);
+        object_unref(OBJECT(dev->parent_bus));
+        dev->parent_bus = NULL;
+    }
 }
 
 static void device_class_init(ObjectClass *class, void *data)

?

Paolo

Re: [Qemu-devel] [RFC PATCH 03/15] spapr_drc: pass object ownership to parent/owner

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> DRC objects attach themselves to an owner as a child
> property. unref afterward to allow them to be finalized
> when their owner is finalized.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_drc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 48bf193..396a03b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -456,6 +456,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>      drc->id = id;
>      drc->owner = owner;
>      object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> +    object_unref(OBJECT(drc));
>      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
>  
>      /* human-readable name for a DRC to encode into the DT
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [RFC PATCH 04/15] spapr_iommu: pass object ownership to parent/owner

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> DRC objects attach themselves to an owner as a child
> property. unref afterward to allow them to be finalized
> when their owner is finalized.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_iommu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index a14cdc4..79e998b 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -182,6 +182,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>  
>      snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
>      object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> +    object_unref(OBJECT(tcet));
>  
>      object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> To support PHB hotplug we need to clean up lingering references,
> memory, child properties, etc. prior to the PHB object being
> finalized. Generally this will be called as a result of calling
> object_unref() on the PHB object, which in turn would normally

s/object_unref/object_unparent/

> be called as the result of an unplug() operation.
> 
> When the PHB is finalized, child objects will be unparented in
> turn, and finalized if the PHB was the only reference holder. so
> we don't bother to explicitly unparent child objects of the PHB
> (spapr_iommu, spapr_drc, etc).
> 
> We do need to handle memory regions explicitly however, since
> they also take a reference on the PHB, and won't allow it to
> be finalized otherwise.

They shouldn't hold a reference anymore as soon as the regions are not
visible in an AddressSpace (and the RCU thread has picked up the changes).

In fact, docs/memory.txt documents (!) that you must call
object_unparent() for memory regions in the instance_finalize function,
not in the unrealize function.

> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2e7590c..25a738c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
>      }
>  }
>  
> +static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
> +    sPAPRTCETable *tcet;
> +
> +    pci_unregister_bus(phb->bus);
> +
> +    g_free(sphb->dtbusname);
> +    sphb->dtbusname = NULL;

This g_free can probably also be moved for simplicity to instance_finalize.

> +    /* remove IO/MMIO subregions and aliases, rest should get cleaned
> +     * via PHB's unrealize->object_finalize
> +     */
> +    memory_region_del_subregion(get_system_memory(), &sphb->iowindow);

^^ You should indeed do this here.

> +    object_unparent(OBJECT(&sphb->iowindow));
> +    object_unparent(OBJECT(&sphb->iospace));
> +
> +    memory_region_del_subregion(get_system_memory(), &sphb->memwindow);

^^ and this

> +    object_unparent(OBJECT(&sphb->memwindow));
> +    object_unparent(OBJECT(&sphb->memspace));
> +
> +    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> +    memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
> +    memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet));
> +    address_space_destroy(&sphb->iommu_as);

^^ and these three.  However, the object_unparents should be in
instance_finalize.

Paolo

> +    QLIST_REMOVE(sphb, list);
> +}
> +
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
> @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>  
>      hc->root_bus_path = spapr_phb_root_bus_path;
>      dc->realize = spapr_phb_realize;
> +    dc->unrealize = spapr_phb_unrealize;
>      dc->props = spapr_phb_properties;
>      dc->reset = spapr_phb_reset;
>      dc->vmsd = &vmstate_spapr_pci;
> 

Re: [Qemu-devel] [RFC PATCH 08/15] spapr: create DR connectors for PHBs and register reset hooks

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> +    if (spapr->dr_phb_enabled) {
> +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> +            sPAPRDRConnector *drc =
> +                spapr_dr_connector_new(OBJECT(machine),
> +                                       SPAPR_DR_CONNECTOR_TYPE_PHB, i);
> +            qemu_register_reset(spapr_drc_reset, drc);
> +        }
> +    }

Is this needed because drc is busless?  Then I think it should be done
in device_set_realized (and the matching qemu_unregister_reset too).

Paolo

Re: [Qemu-devel] [RFC PATCH 11/15] qdev: add qbus_set_hotplug_handler_generic()

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
>  void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error **errp)
>  {
> -    qbus_set_hotplug_handler_internal(bus, OBJECT(handler), errp);
> +    qbus_set_hotplug_handler_generic(bus, OBJECT(handler), errp);
>  }
>  

I think it's okay to just change the type of qbus_set_hotplug_handler's
handler argument, and get rid altogether of
qbus_set_hotplug_handler_internal.

Paolo

Re: [Qemu-devel] [RFC PATCH 00/15] spapr: add support for PHB hotplug

[Paolo Bonzini]

On 29/04/2015 21:20, Michael Roth wrote:
> These patches are based on latest spapr-hotplug-pci patches, and
> can also be obtained from:
> 
>   https://github.com/mdroth/qemu/commits/spapr-hotplug-phb
> 
> These patches implement support for hotplug/unplug of PCI host-bridges.
> The main use cases are:
> 
>  - allowing for VFIO PCI hotplug for host kernels that still require a
>    1:1 mapping between guest PHB/TCE table and an iommu group (a
>    requirement that will be relaxed with Alexey Kardashevskiy's VFIO
>    rework for DDW support)
>  - allocating new PHBs/TCE tables for hotplugging/distributing VFIO
>    devices that have different NUMA affinities associated with them
>    for performance reason
>  - expanding hotplug capacity for passthrough/emulated PCI devices
> 
> With these patches we support the following:
> 
> (qemu) device_add spapr-pci-host-bridge,index=2,id=phb2
> (qemu) device_add virtio-net-pci,id=hp2.0,bus=phb2.0
> (qemu) device_del hp2.0
> (qemu) device_del phb2
> 
> Automatic add/remove of PHBs based on EPOW event mechanism require
> updated versions of powerpc-utils, rtas_errd, and librtas. Patches
> are forthcoming and will be available in future versions, but for now
> we can add them manually by executing the following in the guest
> after/before hotplug/unplug, respectively:
> 
>   # add PHB
>   drmgr -c PHB -s "PHB 2" -a -n
> 
>   # remove PHB
>   drmgr -c PHB -s "PHB 2" -r -n
> 
> Feedback/comments are very much appreciated.

I reviewed the QOM/qdev/memory parts.  Don't know much about the rest,
sorry. :)

Paolo

>  hw/core/qdev.c              |  24 ++++++++++++------
>  hw/pci/pci.c                |  33 +++++++++++++++++++++++++
>  hw/ppc/spapr.c              | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ppc/spapr_drc.c          |   1 +
>  hw/ppc/spapr_events.c       |   5 ++++
>  hw/ppc/spapr_iommu.c        |   1 +
>  hw/ppc/spapr_pci.c          |  66 ++++++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/pci-host/spapr.h |   3 ++-
>  include/hw/pci/pci.h        |   3 +++
>  include/hw/ppc/spapr.h      |   1 +
>  include/hw/qdev-core.h      |   3 +++
>  11 files changed, 310 insertions(+), 13 deletions(-)
> 
> 
> 

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Michael Roth]

Quoting Paolo Bonzini (2015-04-30 08:35:13)
> 
> 
> On 29/04/2015 21:20, Michael Roth wrote:
> > If the parent is finalized as a result of object_unparent(), it
> > will still be attached to the composition tree at the time any
> > children are unparented as a result of that same call to
> > object_unparent(). However, in some cases, object_unparent()
> > will complete without finalizing the parent device, due to
> > lingering references that won't be released till some time later.
> > One such example is if the parent has MemoryRegion children (which
> > take a ref on their parent), who in turn have AddressSpace's (which
> > take a ref on their regions), since those AddressSpaces get cleaned
> > up asynchronously by the RCU thread.
> > 
> > In this case qdev:device_unparent() may be called for a child Device
> > that no longer has a path to the root/machine container, causing
> > object_get_canonical_path() to assert.
> 
> This doesn't seem right.  Unparent callbacks are _not_ called when you 
> finalize, they are called in post-order as soon as you unplug a device 
> (the call tree is object_unparent ==> device_unparent(parent) ==> 
> bus_unparent(parent->bus) ==> device_unparent(parent->bus->child[0]) 
> and so on).

Hmm, well, that does seem to be the case for devices that reside on a
bus, since the post-order traversal from the parent will eventually
reach any such devices.

And for a device that gets unparented before it's parent bus, I think
the fix you posted ends up not being needed because the child is
actually a link property of the parent bus, as opposed to an actual
child property, so removing the property doesn't "disconnect" the
device from the composition tree: presumably the *actual* parent
object/container (/machine/unattached I believe?) is still around
when the DEVICE_DELETED event is sent, so it still has a canonical
path and we don't get the assert from object_get_canonical_path().

In my case though I have a couple device types (spapr_drc, and
spapr_iommu) that are direct child properties of the PHB, and
from what I can tell the clean up path for those when we do
object_unparent(phb) goes something like:

object_unparent(o):
  object_property_del_child(o->parent, o, NULL):
    object_property_del(p, prop_name):
      prop->release(p, prop_name, prop_opaque):
      | object_finalize_child_property(p, prop_name, o):
      |   o->class->unparent(o):
      |     device_unparent(o) <- (post-order clean-up, but skips
      |                            direct children like spapr_drc/spapr_iommu)
      |   o->parent = NULL
      |   object_unref(o):
      |     object_finalize(o): <- may happen asynchronously due to RCU cleanup
      |                            for AddressSpace/MemoryRegion ref holder.
      |                            object o will no longer be child prop of
      |                            o->parent. actually, this seems like it would
      |                            be the case during synchronous finalization
      |                            as well now that I look at it more closely...
      |       object_property_del_all(o)
      |         for prop in o->properties:
      |           prop->release(o, prop->name, prop->opaque/o->child)
      |             object_finalize_child_property(o, prop_name, c):
      |               o->class->unparent(c):
      |                 device_unparent(c) <- (spapr_drc/spapr_iommu children
      |                                        get their callbacks after being
      |                                        disconnected, no longer have
      |                                        canonical paths)
      |           QTAILQ_REMOVE(&o->properties, prop, node)
      |       object_deinit(o)
      |       o->free(o)
      QTAILQ_REMOVE(&o->properties, prop, node)

I played around with the idea of temporarilly moving unparented, unfinalized
objects to an "orphan" container. It seemed like a fun way of tracking leaked
objects, and avoids the assert, but that got wierd pretty quickly... and
having DEVICE_DELETED randomly change up the device path didn't seem like
the intended behavior, so this hack ended up seeming pretty reasonable.

The other approach, which I hadn't looked into too closely, was to defer
unparenting an object until it's ref count goes to 0. Could maybe look into
that instead if it seems less hacky.

> 
> DEVICE_DELETED is called after a device's children have been 
> unparented.  It could be called after a bus is dead though.  Could it 
> be that the patch you want is simply this:
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6e6a65d..46019c4 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1241,11 +1241,6 @@ static void device_unparent(Object *obj)
>          bus = QLIST_FIRST(&dev->child_bus);
>          object_unparent(OBJECT(bus));
>      }
> -    if (dev->parent_bus) {
> -        bus_remove_child(dev->parent_bus, dev);
> -        object_unref(OBJECT(dev->parent_bus));
> -        dev->parent_bus = NULL;
> -    }
> 
>      /* Only send event if the device had been completely realized */
>      if (dev->pending_deleted_event) {
> @@ -1254,6 +1249,12 @@ static void device_unparent(Object *obj)
>          qapi_event_send_device_deleted(!!dev->id, dev->id, path, &error_abort);
>          g_free(path);
>      }
> +
> +    if (dev->parent_bus) {
> +        bus_remove_child(dev->parent_bus, dev);
> +        object_unref(OBJECT(dev->parent_bus));
> +        dev->parent_bus = NULL;
> +    }
>  }
> 
>  static void device_class_init(ObjectClass *class, void *data)
> 
> ?
> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize

[Michael Roth]

Quoting Paolo Bonzini (2015-04-30 09:05:17)
> 
> 
> On 29/04/2015 21:20, Michael Roth wrote:
> > To support PHB hotplug we need to clean up lingering references,
> > memory, child properties, etc. prior to the PHB object being
> > finalized. Generally this will be called as a result of calling
> > object_unref() on the PHB object, which in turn would normally
> 
> s/object_unref/object_unparent/
> 
> > be called as the result of an unplug() operation.
> > 
> > When the PHB is finalized, child objects will be unparented in
> > turn, and finalized if the PHB was the only reference holder. so
> > we don't bother to explicitly unparent child objects of the PHB
> > (spapr_iommu, spapr_drc, etc).
> > 
> > We do need to handle memory regions explicitly however, since
> > they also take a reference on the PHB, and won't allow it to
> > be finalized otherwise.
> 
> They shouldn't hold a reference anymore as soon as the regions are not
> visible in an AddressSpace (and the RCU thread has picked up the changes).

Sorry, I mixed up memory regions with memory region alias. Memory region
aliases do a memory_region_ref() on the original MR, similar to
memory_region_add_subregion(), so that's what ends up creating the
reference to the owner/PHB.

So I think I do need to object_unparent() the 2 MR aliases in realize
(otherwise the PHB doesn't get finalized), but everything else can
get moved to instance_finalize() as you suggested and that seems to
do the trick.

> 
> In fact, docs/memory.txt documents (!) that you must call
> object_unparent() for memory regions in the instance_finalize function,
> not in the unrealize function.

They seem to hint that creation should follow the same guidelines, so I
assume I should probably moved all the memory_region_init()'s to
instance_init()? I see a lot of counter-examples elsewhere, but not
sure if those are intended or not.

> 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 2e7590c..25a738c 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> >      }
> >  }
> >  
> > +static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
> > +{
> > +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> > +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
> > +    sPAPRTCETable *tcet;
> > +
> > +    pci_unregister_bus(phb->bus);
> > +
> > +    g_free(sphb->dtbusname);
> > +    sphb->dtbusname = NULL;
> 
> This g_free can probably also be moved for simplicity to instance_finalize.
> 
> > +    /* remove IO/MMIO subregions and aliases, rest should get cleaned
> > +     * via PHB's unrealize->object_finalize
> > +     */
> > +    memory_region_del_subregion(get_system_memory(), &sphb->iowindow);
> 
> ^^ You should indeed do this here.
> 
> > +    object_unparent(OBJECT(&sphb->iowindow));
> > +    object_unparent(OBJECT(&sphb->iospace));
> > +
> > +    memory_region_del_subregion(get_system_memory(), &sphb->memwindow);
> 
> ^^ and this
> 
> > +    object_unparent(OBJECT(&sphb->memwindow));
> > +    object_unparent(OBJECT(&sphb->memspace));
> > +
> > +    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> > +    memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
> > +    memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet));
> > +    address_space_destroy(&sphb->iommu_as);
> 
> ^^ and these three.  However, the object_unparents should be in
> instance_finalize.
> 
> Paolo
> 
> > +    QLIST_REMOVE(sphb, list);
> > +}
> > +
> >  static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >  {
> >      SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> >  
> >      hc->root_bus_path = spapr_phb_root_bus_path;
> >      dc->realize = spapr_phb_realize;
> > +    dc->unrealize = spapr_phb_unrealize;
> >      dc->props = spapr_phb_properties;
> >      dc->reset = spapr_phb_reset;
> >      dc->vmsd = &vmstate_spapr_pci;
> > 
> 

Re: [Qemu-devel] [RFC PATCH 08/15] spapr: create DR connectors for PHBs and register reset hooks

[Michael Roth]

Quoting Paolo Bonzini (2015-04-30 09:08:10)
> 
> 
> On 29/04/2015 21:20, Michael Roth wrote:
> > +    if (spapr->dr_phb_enabled) {
> > +        for (i = 0; i < SPAPR_DRC_MAX_PHB; i++) {
> > +            sPAPRDRConnector *drc =
> > +                spapr_dr_connector_new(OBJECT(machine),
> > +                                       SPAPR_DR_CONNECTOR_TYPE_PHB, i);
> > +            qemu_register_reset(spapr_drc_reset, drc);
> > +        }
> > +    }
> 
> Is this needed because drc is busless?  Then I think it should be done
> in device_set_realized (and the matching qemu_unregister_reset too).

You mean move the qemu_register_reset() to DRC->realize()? Don't recall
if there was a reason I did it this way, but that does seem a lot
cleaner.

And yah, no bus for DRCs, so we're not hooked into qbus_reset_all_fn()

> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH 00/15] spapr: add support for PHB hotplug

[Michael Roth]

Quoting Paolo Bonzini (2015-04-30 09:10:30)
> 
> 
> On 29/04/2015 21:20, Michael Roth wrote:
> > These patches are based on latest spapr-hotplug-pci patches, and
> > can also be obtained from:
> > 
> >   https://github.com/mdroth/qemu/commits/spapr-hotplug-phb
> > 
> > These patches implement support for hotplug/unplug of PCI host-bridges.
> > The main use cases are:
> > 
> >  - allowing for VFIO PCI hotplug for host kernels that still require a
> >    1:1 mapping between guest PHB/TCE table and an iommu group (a
> >    requirement that will be relaxed with Alexey Kardashevskiy's VFIO
> >    rework for DDW support)
> >  - allocating new PHBs/TCE tables for hotplugging/distributing VFIO
> >    devices that have different NUMA affinities associated with them
> >    for performance reason
> >  - expanding hotplug capacity for passthrough/emulated PCI devices
> > 
> > With these patches we support the following:
> > 
> > (qemu) device_add spapr-pci-host-bridge,index=2,id=phb2
> > (qemu) device_add virtio-net-pci,id=hp2.0,bus=phb2.0
> > (qemu) device_del hp2.0
> > (qemu) device_del phb2
> > 
> > Automatic add/remove of PHBs based on EPOW event mechanism require
> > updated versions of powerpc-utils, rtas_errd, and librtas. Patches
> > are forthcoming and will be available in future versions, but for now
> > we can add them manually by executing the following in the guest
> > after/before hotplug/unplug, respectively:
> > 
> >   # add PHB
> >   drmgr -c PHB -s "PHB 2" -a -n
> > 
> >   # remove PHB
> >   drmgr -c PHB -s "PHB 2" -r -n
> > 
> > Feedback/comments are very much appreciated.
> 
> I reviewed the QOM/qdev/memory parts.  Don't know much about the rest,
> sorry. :)

No problem, those seemed to be the scarier bits for me :) Thanks!

> 
> Paolo
> 
> >  hw/core/qdev.c              |  24 ++++++++++++------
> >  hw/pci/pci.c                |  33 +++++++++++++++++++++++++
> >  hw/ppc/spapr.c              | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ppc/spapr_drc.c          |   1 +
> >  hw/ppc/spapr_events.c       |   5 ++++
> >  hw/ppc/spapr_iommu.c        |   1 +
> >  hw/ppc/spapr_pci.c          |  66 ++++++++++++++++++++++++++++++++++++++++++++++---
> >  include/hw/pci-host/spapr.h |   3 ++-
> >  include/hw/pci/pci.h        |   3 +++
> >  include/hw/ppc/spapr.h      |   1 +
> >  include/hw/qdev-core.h      |   3 +++
> >  11 files changed, 310 insertions(+), 13 deletions(-)
> > 
> > 
> > 
> 

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Paolo Bonzini]

On 01/05/2015 01:03, Michael Roth wrote:
> 
> I played around with the idea of temporarilly moving unparented, unfinalized
> objects to an "orphan" container. It seemed like a fun way of tracking leaked
> objects, and avoids the assert, but that got wierd pretty quickly... and
> having DEVICE_DELETED randomly change up the device path didn't seem like
> the intended behavior, so this hack ended up seeming pretty reasonable.
> 
> The other approach, which I hadn't looked into too closely, was to defer
> unparenting an object until it's ref count goes to 0. Could maybe look into
> that instead if it seems less hacky.

What about unparenting children devices in the device's unrealize
callback?  It sucks that you have to do it manually, but using stale
canonical paths isn't the nicest thing either.

Paolo

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Michael Roth]

Quoting Paolo Bonzini (2015-05-01 15:43:45)
> 
> 
> On 01/05/2015 01:03, Michael Roth wrote:
> > 
> > I played around with the idea of temporarilly moving unparented, unfinalized
> > objects to an "orphan" container. It seemed like a fun way of tracking leaked
> > objects, and avoids the assert, but that got wierd pretty quickly... and
> > having DEVICE_DELETED randomly change up the device path didn't seem like
> > the intended behavior, so this hack ended up seeming pretty reasonable.
> > 
> > The other approach, which I hadn't looked into too closely, was to defer
> > unparenting an object until it's ref count goes to 0. Could maybe look into
> > that instead if it seems less hacky.
> 
> What about unparenting children devices in the device's unrealize
> callback?  It sucks that you have to do it manually, but using stale
> canonical paths isn't the nicest thing either.

That does seems to do the trick. It felt wrong when I first looked at
it because in some cases the children attach themselves to the parent
without making making the parent aware, but using the child property
list ends up seeming pretty reasonable:

static int spapr_phb_unparent_child_devices(Object *child, void *opaque)
{
    DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
    if (dev) {
        object_unparent(child);
    }                                                                                                      
    return 0;
}

static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
{
    ...
    /* clean up child devices (spapr_drc, spapr_iommu, etc.).
     * non-DeviceState's will be handled explicitly
     */
    object_child_foreach(OBJECT(dev), spapr_phb_unparent_child_devices, NULL);
    ...
}

Maybe if we have some more examples pop up it might make sense to
move that to qdev:device_unparent(), since we sort of expect that
for Devices.

Either way this does seem nicer. Thanks!

> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize

[Paolo Bonzini]

On 01/05/2015 03:18, Michael Roth wrote:
> Sorry, I mixed up memory regions with memory region alias. Memory region
> aliases do a memory_region_ref() on the original MR, similar to
> memory_region_add_subregion(), so that's what ends up creating the
> reference to the owner/PHB.
> 
> So I think I do need to object_unparent() the 2 MR aliases in realize
> (otherwise the PHB doesn't get finalized), but everything else can
> get moved to instance_finalize() as you suggested and that seems to
> do the trick.

Yes, unparenting the aliases in unrealize is okay and in fact may be
required to avoid a leak.  Patching docs/memory.txt to point it out
would be great. :)

Paolo

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Paolo Bonzini]

On 02/05/2015 00:54, Michael Roth wrote:
>> > 
>> > What about unparenting children devices in the device's unrealize
>> > callback?  It sucks that you have to do it manually, but using stale
>> > canonical paths isn't the nicest thing either.
> That does seems to do the trick. It felt wrong when I first looked at
> it because in some cases the children attach themselves to the parent
> without making making the parent aware,

Can you point to an example?  The parent "owns" its property namespace,
so it would be wrong for a child to attach itself unbeknownst to the parent.

There are a couple cases where an object adds a property to another
object, e.g. the rtc-time property of /machine, but in that case the
property is a well-known name whose setting is "outsourced" by /machine
to the device.

Paolo

Re: [Qemu-devel] [RFC PATCH 01/15] pci: allow cleanup/unregistration of PCI buses

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Wed, Apr 29, 2015 at 02:20:10PM -0500, Michael Roth wrote:
> This adds cleanup counterparts to pci_register_bus(),
> pci_bus_new(), and pci_bus_irqs().
> 
> These cleanup routines are needed in the case of hotpluggable
> PCIHostBridge implementations. Currently we can rely on the
> object_unparent()'ing of the PCIHostState recursively unparenting
> and cleaning up it's child buses, but we need explicit calls
> to also:
> 
>   1) remove the PCIHostState from pci_host_bridges global list.
>      otherwise, we risk accessing freed memory when we access
>      the list later
>   2) clean up memory allocated in pci_bus_irqs()
> 
> Both are handled outside the context of any particular bus or
> host bridge's init/realize functions, making it difficult to
> avoid the need for explicit cleanup functions without remodeling
> how PCIHostBridges are created. So keep it simple and just add
> them for now.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

As with Bharata's cpu and memory hotplug series, you may want to split
out those patches which are reasonable cleanups regardless of exactly
what happens with the hotplug code itself.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 03/15] spapr_drc: pass object ownership to parent/owner

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Wed, Apr 29, 2015 at 02:20:12PM -0500, Michael Roth wrote:
> DRC objects attach themselves to an owner as a child
> property. unref afterward to allow them to be finalized
> when their owner is finalized.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_drc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 48bf193..396a03b 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -456,6 +456,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>      drc->id = id;
>      drc->owner = owner;
>      object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> +    object_unref(OBJECT(drc));
>      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
>  
>      /* human-readable name for a DRC to encode into the DT

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 04/15] spapr_iommu: pass object ownership to parent/owner

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Wed, Apr 29, 2015 at 02:20:13PM -0500, Michael Roth wrote:
> DRC objects attach themselves to an owner as a child
  ^^^

Copy and paste error in the commit message.


> property. unref afterward to allow them to be finalized
> when their owner is finalized.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

But otherwise,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_iommu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index a14cdc4..79e998b 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -182,6 +182,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>  
>      snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
>      object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> +    object_unref(OBJECT(tcet));
>  
>      object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
>  

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]

On Wed, Apr 29, 2015 at 02:20:15PM -0500, Michael Roth wrote:
> Prior to this patch 'index' is purely a shorthand for specifying
> MMIO windows, BUIDs, and other configuration values for a PHB.
> 
> With the addition of PHB hotplug, we have a static number of DRCs
> that can be used to handle hotplug/unplug operations on our PHBs,
> and need a consistent way to map PHBs to these connectors, and
> assign a unique identifiers for the connectors.
> 
> BUIDs would be a good choice, however, those are 64-bit values,
> whereas DRC indexes are 32-bit.
> 
> 'index' serves this purpose nicely, and also allows us to align
> the maximum number PHBs that can be plugged with the maximum
> 'index' value we allow (255).
> 
> This means that when PHB hotplug is enabled (2.4+), 'index' is
> now always a required value, regardless of whether or not other
> configuration properties are specified explicitly. We could
> potentially arrange for 'index'-less PHBs to be added in an
> 'unpluggable' fashion via command-line, and have checks to
> generate an error when hotplugged via device_add, but the simpler
> path seems to be to just make it required now.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

So, if I was doing this again from scratch, I'd probably just make
index mandatory.  But being where we already are..

I'd prefer to add an explicit "drc_id" (or whatever) property and have
that set to index by default (just as index provides defaults for the
other window properties).

> ---
>  hw/ppc/spapr_pci.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 25a738c..e37de28 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1)
>              || (sphb->mem_win_addr != (hwaddr)-1)
>              || (sphb->io_win_addr != (hwaddr)-1)) {
> -            error_setg(errp, "Either \"index\" or other parameters must"
> -                       " be specified for PAPR PHB, not both");
> +            if (!spapr->dr_phb_enabled) {
> +                /* if they aren't potentially using index as an identifier for
> +                 * the PHB's DR connector, enforce the old semantics of index
> +                 * being purely a shorthand for PHB configuration options.
> +                 */
> +                error_setg(errp, "Either \"index\" or other parameters must"
> +                           " be specified for PAPR PHB, not both");
> +            }
>              return;
>          }
>  
> @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              + 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;
> +    } else {
> +        if (spapr->dr_phb_enabled) {
> +            error_setg(errp, "The \"index\" property is required for machine"
> +                       " types that support PHB hotplug (and in such cases"
> +                       " can be used alongside \"buid\" and other"
> +                       " configuration properties)");
> +            return;
> +        }
>      }
>  
>      if (sphb->buid == (uint64_t)-1) {

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 07/15] spapr: enable PHB hotplug for pseries-2.4

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Wed, Apr 29, 2015 at 02:20:16PM -0500, Michael Roth wrote:
> Introduce an sPAPRMachineClass sub-class of MachineClass to
> handle sPAPR-specific machine configuration properties.
> 
> The 'dr_phb_enabled' field of that class can be set as
> part of machine-specific init code, and is then propagated
> to sPAPREnvironment to conditionally enable creation of DRC
> objects and device-tree description to facilitate hotplug
> of PHBs.
> 
> Since we can't migrate this state to older machine types,
> default the option to false and only enable it for new
> machine types.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

This will obviously conflict with Bharata's patch doing much the same
thing for cpu and memory hotplug.

I'll try to put a patch in spapr-next tomorrow which adds an empty
sPAPRMachineClass which you can both then add fields to.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 08/15] spapr: create DR connectors for PHBs and register reset hooks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

On Wed, Apr 29, 2015 at 02:20:17PM -0500, Michael Roth wrote:
> Since we route hotplugged PHBs to their DR connector using their
> PHB.index value, we align the number of DR connectors with the
> maximum index value: SPAPR_PCI_MAX_INDEX.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

As with Bharata's similar patch, I'm not sure why the DRC needs an
explicitly registered reset, rather than ->reset being called
automatically by the qdev model, but otherwise

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 09/15] spapr: populate PHB DRC entries for root DT node

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

On Wed, Apr 29, 2015 at 02:20:18PM -0500, Michael Roth wrote:
> From: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> 
> This add entries to the root OF node to advertise our PHBs as being
> DR-capable in accordance with PAPR specification.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c539932..a7af332 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -57,6 +57,7 @@
>  #include "qemu/error-report.h"
>  #include "trace.h"
>  #include "hw/nmi.h"
> +#include "hw/ppc/spapr_drc.h"
>  
>  #include "hw/compat.h"
>  
> @@ -745,6 +746,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>      size_t cb = 0;
>      char *bootlist;
>      void *fdt;
> +    int fdt_offset;
>      sPAPRPHBState *phb;
>  
>      fdt = g_malloc(FDT_MAX_SIZE);
> @@ -804,6 +806,13 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>          spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
>      }
>  
> +    fdt_offset = fdt_path_offset(fdt, "/");
> +    ret = spapr_drc_populate_dt(fdt, fdt_offset, NULL,
> +                                SPAPR_DR_CONNECTOR_TYPE_PHB);
> +    if (ret < 0) {
> +        fprintf(stderr, "Couldn't set up RTAS device tree properties\n");

This error message isn't right.

Otherwise,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> +    }
> +
>      _FDT((fdt_pack(fdt)));
>  
>      if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 10/15] spapr_events: add support for phb hotplug events

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

On Wed, Apr 29, 2015 at 02:20:19PM -0500, Michael Roth wrote:
> Extend the existing EPOW event format we use for PCI
> devices to emit PHB plug/unplug events.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 11/15] qdev: add qbus_set_hotplug_handler_generic()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]

On Wed, Apr 29, 2015 at 02:20:20PM -0500, Michael Roth wrote:
> Certain devices types, like memory/CPU, are now being handled using a
> hotplug interface provided by a top-level MachineClass. Hotpluggable
> host bridges are another such device where it makes sense to use a
> machine-level hotplug handler. However, unlike those devices,
> host-bridges have a parent bus (the main system bus), and devices with
> a parent bus use a different mechanism for registering their hotplug
> handlers: qbus_set_hotplug_handler(). This interface currently expects
> a handler to be a subclass of DeviceClass, but this is not the case
> for MachineClass, which derives directly from ObjectClass.
> 
> Internally, the interface only requires an ObjectClass, so expose that
> support via a new qbus_set_hotplug_handler_generic().
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Seems odd to have to interfaces to me.  Why not just change
qbus_set_hotplug_handler() to take any ObjectClass?

> ---
>  hw/core/qdev.c         | 9 ++++-----
>  include/hw/qdev-core.h | 2 ++
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index fda1d2f..8fd9320 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -108,22 +108,21 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>      bus_add_child(bus, dev);
>  }
>  
> -static void qbus_set_hotplug_handler_internal(BusState *bus, Object *handler,
> -                                              Error **errp)
> +void qbus_set_hotplug_handler_generic(BusState *bus, Object *handler,
> +                                      Error **errp)
>  {
> -
>      object_property_set_link(OBJECT(bus), OBJECT(handler),
>                               QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
>  }
>  
>  void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error **errp)
>  {
> -    qbus_set_hotplug_handler_internal(bus, OBJECT(handler), errp);
> +    qbus_set_hotplug_handler_generic(bus, OBJECT(handler), errp);
>  }
>  
>  void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
>  {
> -    qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
> +    qbus_set_hotplug_handler_generic(bus, OBJECT(bus), errp);
>  }
>  
>  /* Create a new device.  This only initializes the device state structure
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 17f805e..3b210bf 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -377,6 +377,8 @@ GSList *qdev_build_hotpluggable_device_list(Object *peripheral);
>  
>  void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
>                                Error **errp);
> +void qbus_set_hotplug_handler_generic(BusState *bus, Object *handler,
> +                                      Error **errp);
>  
>  void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp);
>  

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 13/15] spapr_pci: provide node start offset via spapr_populate_pci_dt()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]

On Wed, Apr 29, 2015 at 02:20:22PM -0500, Michael Roth wrote:
> PHB hotplug re-uses PHB device tree generation code and passes
> it to a guest via RTAS. Doing this requires knowledge of where
> exactly in the device tree the node describing the PHB begins.
> 
> Provide this via a new optional pointer that can be used to
> store the PHB node's start offset.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c              | 2 +-
>  hw/ppc/spapr_pci.c          | 6 +++++-
>  include/hw/pci-host/spapr.h | 3 ++-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 042e7a9..ecf40e4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -767,7 +767,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>      }
>  
>      QLIST_FOREACH(phb, &spapr->phbs, list) {
> -        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
> +        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt, NULL);
>      }
>  
>      if (ret < 0) {
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e37de28..66fe85f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1534,7 +1534,8 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
>  
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,
> -                          void *fdt)
> +                          void *fdt,
> +                          int *node_offset)
>  {
>      int bus_off, i, j, ret;
>      char nodename[256];
> @@ -1578,6 +1579,9 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>      if (bus_off < 0) {
>          return bus_off;
>      }
> +    if (node_offset) {
> +        *node_offset = bus_off;
> +    }
>  
>      /* Write PHB properties */
>      _FDT(fdt_setprop_string(fdt, bus_off, "device_type", "pci"));
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 9dca388..32a9213 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -126,7 +126,8 @@ PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
>  
>  int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                            uint32_t xics_phandle,
> -                          void *fdt);
> +                          void *fdt,
> +                          int *node_offset);
>  
>  void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr);
>  

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 14/15] spapr_pci: add ibm, my-drc-index property for PHB hotplug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

On Wed, Apr 29, 2015 at 02:20:23PM -0500, Michael Roth wrote:
> This is needed to denote a boot-time PHB as being hot-pluggable.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr_pci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 66fe85f..91dfd96 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1572,6 +1572,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>          cpu_to_be32(b_ddddd(-1)|b_fff(0)), 0x0, 0x0, cpu_to_be32(-1)};
>      uint32_t interrupt_map[PCI_SLOT_MAX * PCI_NUM_PINS][7];
>      sPAPRTCETable *tcet;
> +    sPAPRDRConnector *drc;
>  
>      /* Start populating the FDT */
>      sprintf(nodename, "pci@%" PRIx64, phb->buid);
> @@ -1624,6 +1625,15 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                   tcet->liobn, tcet->bus_offset,
>                   tcet->nb_table << tcet->page_shift);
>  
> +    drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PHB, phb->index);
> +    if (drc) {
> +        sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +        uint32_t drc_index = cpu_to_be32(drck->get_index(drc));
> +
> +        _FDT(fdt_setprop(fdt, bus_off, "ibm,my-drc-index", &drc_index,
> +                         sizeof(drc_index)));
> +    }
> +
>      ret = spapr_drc_populate_dt(fdt, bus_off, OBJECT(phb),
>                                  SPAPR_DR_CONNECTOR_TYPE_PCI);
>      if (ret) {

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 15/15] spapr: add hotplug hooks for PHB hotplug

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Wed, Apr 29, 2015 at 02:20:24PM -0500, Michael Roth wrote:
> Hotplugging PHBs is a machine-level operation, but PHBs reside on the
> main system bus, so we register spapr machine as the handler for the
> main system bus. The entry point for plug/unplug is shared by all
> such machine-level hotplug operations (memory, CPU, PHB, etc), and
> from there we branch off to specific hotplug callbacks based on the
> object type.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Although it will almost certainly need adjustment to fit with the cpu
and hotplug patches that are also pending.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Michael Roth]

Quoting Paolo Bonzini (2015-05-04 04:35:11)
> 
> 
> On 02/05/2015 00:54, Michael Roth wrote:
> >> > 
> >> > What about unparenting children devices in the device's unrealize
> >> > callback?  It sucks that you have to do it manually, but using stale
> >> > canonical paths isn't the nicest thing either.
> > That does seems to do the trick. It felt wrong when I first looked at
> > it because in some cases the children attach themselves to the parent
> > without making making the parent aware,
> 
> Can you point to an example?  The parent "owns" its property namespace,
> so it would be wrong for a child to attach itself unbeknownst to the parent.

Well, I was referring to:

  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)

and:

  sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
                                           sPAPRDRConnectorType type,
                                           uint32_t id)

It wasn't immediately obvious to me that this would result in the
resulting objects attaching themselves as children, but in retrospect
that does seem to be implied by the 'owner' parameter.

If there are cases where this is done with a parameter that isn't explicitly
named 'owner' though it might be somewhat ambiguous (could be pulling out
individual fields as opposed to attaching itself), but I don't see any
examples outside of local functions or qdev-managed devices.

> 
> There are a couple cases where an object adds a property to another
> object, e.g. the rtc-time property of /machine, but in that case the
> property is a well-known name whose setting is "outsourced" by /machine
> to the device.
> 
> Paolo
> 

Re: [Qemu-devel] [RFC PATCH 06/15] spapr_pci: also use 'index' property as DRC index for PHBs

[Michael Roth]

Quoting David Gibson (2015-05-05 06:34:15)
> On Wed, Apr 29, 2015 at 02:20:15PM -0500, Michael Roth wrote:
> > Prior to this patch 'index' is purely a shorthand for specifying
> > MMIO windows, BUIDs, and other configuration values for a PHB.
> > 
> > With the addition of PHB hotplug, we have a static number of DRCs
> > that can be used to handle hotplug/unplug operations on our PHBs,
> > and need a consistent way to map PHBs to these connectors, and
> > assign a unique identifiers for the connectors.
> > 
> > BUIDs would be a good choice, however, those are 64-bit values,
> > whereas DRC indexes are 32-bit.
> > 
> > 'index' serves this purpose nicely, and also allows us to align
> > the maximum number PHBs that can be plugged with the maximum
> > 'index' value we allow (255).
> > 
> > This means that when PHB hotplug is enabled (2.4+), 'index' is
> > now always a required value, regardless of whether or not other
> > configuration properties are specified explicitly. We could
> > potentially arrange for 'index'-less PHBs to be added in an
> > 'unpluggable' fashion via command-line, and have checks to
> > generate an error when hotplugged via device_add, but the simpler
> > path seems to be to just make it required now.
> > 
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> So, if I was doing this again from scratch, I'd probably just make
> index mandatory.  But being where we already are..
> 
> I'd prefer to add an explicit "drc_id" (or whatever) property and have
> that set to index by default (just as index provides defaults for the
> other window properties).

Hmm, yah that's much less of a headache. Thanks!

> 
> > ---
> >  hw/ppc/spapr_pci.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 25a738c..e37de28 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1156,8 +1156,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >          if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1)
> >              || (sphb->mem_win_addr != (hwaddr)-1)
> >              || (sphb->io_win_addr != (hwaddr)-1)) {
> > -            error_setg(errp, "Either \"index\" or other parameters must"
> > -                       " be specified for PAPR PHB, not both");
> > +            if (!spapr->dr_phb_enabled) {
> > +                /* if they aren't potentially using index as an identifier for
> > +                 * the PHB's DR connector, enforce the old semantics of index
> > +                 * being purely a shorthand for PHB configuration options.
> > +                 */
> > +                error_setg(errp, "Either \"index\" or other parameters must"
> > +                           " be specified for PAPR PHB, not both");
> > +            }
> >              return;
> >          }
> >  
> > @@ -1174,6 +1180,14 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >              + 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;
> > +    } else {
> > +        if (spapr->dr_phb_enabled) {
> > +            error_setg(errp, "The \"index\" property is required for machine"
> > +                       " types that support PHB hotplug (and in such cases"
> > +                       " can be used alongside \"buid\" and other"
> > +                       " configuration properties)");
> > +            return;
> > +        }
> >      }
> >  
> >      if (sphb->buid == (uint64_t)-1) {
> 
> -- 
> 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] [RFC PATCH 02/15] qdev: store DeviceState's canonical path to use when unparenting

[Paolo Bonzini]

On 05/05/2015 17:48, Michael Roth wrote:
> Well, I was referring to:
> 
>   sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
> 
> and:
> 
>   sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>                                            sPAPRDRConnectorType type,
>                                            uint32_t id)
> 
> It wasn't immediately obvious to me that this would result in the
> resulting objects attaching themselves as children, but in retrospect
> that does seem to be implied by the 'owner' parameter.

I guess that's okay, as long as the callers of these functions pass
their "this" object (or "self", or whatever :)) as the owner.

Paolo

> If there are cases where this is done with a parameter that isn't explicitly
> named 'owner' though it might be somewhat ambiguous (could be pulling out
> individual fields as opposed to attaching itself), but I don't see any
> examples outside of local functions or qdev-managed devices.