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.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
- rebased against ppc-for-2.10
- converted to unplug_request
- handle drc_id at pre-plug
- reset hotplugged PHB at plug
- compatibility with older machine types
---
hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_drc.c | 1
hw/ppc/spapr_pci.c | 2 -
include/hw/pci-host/spapr.h | 2 +
include/hw/ppc/spapr.h | 1
5 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 90485054c2e7..589f76ef9fb8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
register_savevm_live(NULL, "spapr/htab", -1, 1,
&savevm_htab_handlers, spapr);
+ if (spapr->dr_phb_enabled) {
+ qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
+ }
+
qemu_register_boot_set(spapr_boot_set, spapr);
if (kvm_enabled()) {
@@ -3238,6 +3242,103 @@ out:
error_propagate(errp, local_err);
}
+static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+ sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
+
+ if (sphb->drc_id == (uint32_t)-1) {
+ sphb->drc_id = sphb->index;
+ }
+
+ if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
+ error_setg(errp, "PHB id %d out of range", sphb->drc_id);
+ }
+}
+
+static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+ sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
+ sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
+ void *fdt = NULL;
+ int fdt_start_offset;
+ int fdt_size;
+ Error *local_err = NULL;
+ sPAPRDRConnector *drc;
+ uint32_t phandle;
+ int ret;
+ bool hotplugged = spapr_drc_hotplugged(dev);
+
+ if (!spapr->dr_phb_enabled) {
+ return;
+ }
+
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
+ /* hotplug hooks should check it's enabled before getting this far */
+ g_assert(drc);
+
+ if (hotplugged) {
+ if (spapr->xics_phandle == UINT32_MAX) {
+ error_setg(&local_err,
+ "SLOF didn't update the XICS phandle. PHB hotplug cancelled");
+ goto out;
+ }
+ phandle = spapr->xics_phandle;
+
+ spapr_phb_reset(dev);
+ } else {
+ phandle = PHANDLE_XICP;
+ }
+
+ fdt = create_device_tree(&fdt_size);
+ ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
+ if (ret < 0) {
+ error_setg(&local_err, "unable to create FDT for %sPHB",
+ dev->hotplugged ? "hotplugged " : "");
+ goto out;
+ }
+
+ if (hotplugged) {
+ /* generally SLOF creates these, for hotplug it's up to QEMU */
+ _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
+ }
+
+ spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
+out:
+ if (local_err) {
+ error_propagate(errp, local_err);
+ g_free(fdt);
+ return;
+ }
+
+ if (hotplugged) {
+ spapr_hotplug_req_add_by_index(drc);
+ } else if (drc) {
+ spapr_drc_reset(drc);
+ }
+}
+
+void spapr_phb_release(DeviceState *dev)
+{
+ object_unparent(OBJECT(dev));
+}
+
+static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
+ sPAPRDRConnector *drc;
+
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
+ g_assert(drc);
+
+ if (!spapr_drc_unplug_requested(drc)) {
+ spapr_drc_detach(drc);
+ spapr_hotplug_req_remove_by_index(drc);
+ }
+}
+
static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -3284,6 +3385,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
spapr_memory_plug(hotplug_dev, dev, node, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
spapr_core_plug(hotplug_dev, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+ spapr_phb_plug(hotplug_dev, dev, errp);
}
}
@@ -3311,6 +3414,12 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
return;
}
spapr_core_unplug_request(hotplug_dev, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+ if (sms->dr_phb_enabled) {
+ spapr_phb_unplug_request(hotplug_dev, dev, errp);
+ } else {
+ error_setg(errp, "PHB hot unplug not supported on this machine");
+ }
}
}
@@ -3321,6 +3430,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
spapr_memory_pre_plug(hotplug_dev, dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
spapr_core_pre_plug(hotplug_dev, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
+ spapr_phb_pre_plug(hotplug_dev, dev, errp);
}
}
@@ -3328,7 +3439,8 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
DeviceState *dev)
{
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
- object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+ object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
return HOTPLUG_HANDLER(machine);
}
return NULL;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2e1049ce61c7..845fcf70b932 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -704,6 +704,7 @@ static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
drck->typename = "PHB";
drck->drc_name_prefix = "PHB ";
+ drck->release = spapr_phb_release;
}
static const TypeInfo spapr_dr_connector_info = {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 157867af8178..c12f71ae3e2d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1833,7 +1833,7 @@ void spapr_phb_dma_reset(sPAPRPHBState *sphb)
sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
}
-static void spapr_phb_reset(DeviceState *qdev)
+void spapr_phb_reset(DeviceState *qdev)
{
sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7837fb0b1110..15799cee4280 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -120,6 +120,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
void spapr_pci_rtas_init(void);
+void spapr_phb_reset(DeviceState *qdev);
+
sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
uint32_t config_addr);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f09c54d5bb94..a2f6782bdbbf 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -673,6 +673,7 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
/* CPU and LMB DRC release callbacks. */
void spapr_core_release(DeviceState *dev);
void spapr_lmb_release(DeviceState *dev);
+void spapr_phb_release(DeviceState *dev);
void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
On 26/07/17 18:40, Greg Kurz 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.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> - rebased against ppc-for-2.10
> - converted to unplug_request
> - handle drc_id at pre-plug
> - reset hotplugged PHB at plug
> - compatibility with older machine types
> ---
> hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_drc.c | 1
> hw/ppc/spapr_pci.c | 2 -
> include/hw/pci-host/spapr.h | 2 +
> include/hw/ppc/spapr.h | 1
> 5 files changed, 118 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 90485054c2e7..589f76ef9fb8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> register_savevm_live(NULL, "spapr/htab", -1, 1,
> &savevm_htab_handlers, spapr);
>
> + if (spapr->dr_phb_enabled) {
> + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> + }
> +
> qemu_register_boot_set(spapr_boot_set, spapr);
>
> if (kvm_enabled()) {
> @@ -3238,6 +3242,103 @@ out:
> error_propagate(errp, local_err);
> }
>
> +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> +
> + if (sphb->drc_id == (uint32_t)-1) {
> + sphb->drc_id = sphb->index;
> + }
> +
> + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> + }
sphb->index in considered 16bits in the existing code (even though it is
defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
same limit for both, either 256 or 65536 is fine for me.
It is actually a bit weird - it is possible to completely configure few
PHBs in the command line so they will have index==-1 but PCI hotplug code -
spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
this and just does (sphb->index << 16). May be just ditch drc_id, enforce
index not to be -1 and use it as drc_id?
> +}
> +
> +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> + void *fdt = NULL;
> + int fdt_start_offset;
> + int fdt_size;
> + Error *local_err = NULL;
> + sPAPRDRConnector *drc;
> + uint32_t phandle;
> + int ret;
> + bool hotplugged = spapr_drc_hotplugged(dev);
> +
> + if (!spapr->dr_phb_enabled) {
> + return;
> + }
> +
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> + /* hotplug hooks should check it's enabled before getting this far */
> + g_assert(drc);
> +
> + if (hotplugged) {
> + if (spapr->xics_phandle == UINT32_MAX) {
> + error_setg(&local_err,
> + "SLOF didn't update the XICS phandle. PHB hotplug cancelled");
> + goto out;
> + }
> + phandle = spapr->xics_phandle;
> +
> + spapr_phb_reset(dev);
It could be DEVICE_GET_CLASS(dev)->reset(dev) without exporting
spapr_phb_reset. Not sure how this fits into the current QEMU coding style
though.
> + } else {
> + phandle = PHANDLE_XICP;
> + }
> +
> + fdt = create_device_tree(&fdt_size);
> + ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
> + if (ret < 0) {
> + error_setg(&local_err, "unable to create FDT for %sPHB",
> + dev->hotplugged ? "hotplugged " : "");
> + goto out;
> + }
> +
> + if (hotplugged) {
> + /* generally SLOF creates these, for hotplug it's up to QEMU */
> + _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
> + }
> +
> + spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
> +out:
> + if (local_err) {
> + error_propagate(errp, local_err);
> + g_free(fdt);
> + return;
> + }
> +
> + if (hotplugged) {
> + spapr_hotplug_req_add_by_index(drc);
> + } else if (drc) {
> + spapr_drc_reset(drc);
> + }
> +}
> +
> +void spapr_phb_release(DeviceState *dev)
> +{
> + object_unparent(OBJECT(dev));
> +}
> +
> +static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> + sPAPRDRConnector *drc;
> +
> + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> + g_assert(drc);
> +
> + if (!spapr_drc_unplug_requested(drc)) {
> + spapr_drc_detach(drc);
> + spapr_hotplug_req_remove_by_index(drc);
> + }
> +}
> +
> static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> @@ -3284,6 +3385,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> spapr_memory_plug(hotplug_dev, dev, node, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> spapr_core_plug(hotplug_dev, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> + spapr_phb_plug(hotplug_dev, dev, errp);
> }
> }
>
> @@ -3311,6 +3414,12 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> return;
> }
> spapr_core_unplug_request(hotplug_dev, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> + if (sms->dr_phb_enabled) {
> + spapr_phb_unplug_request(hotplug_dev, dev, errp);
> + } else {
> + error_setg(errp, "PHB hot unplug not supported on this machine");
> + }
> }
> }
>
> @@ -3321,6 +3430,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> spapr_memory_pre_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> spapr_core_pre_plug(hotplug_dev, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> + spapr_phb_pre_plug(hotplug_dev, dev, errp);
> }
> }
>
> @@ -3328,7 +3439,8 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> DeviceState *dev)
> {
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> - object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE) ||
> + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> return HOTPLUG_HANDLER(machine);
> }
> return NULL;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2e1049ce61c7..845fcf70b932 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -704,6 +704,7 @@ static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> drck->typename = "PHB";
> drck->drc_name_prefix = "PHB ";
> + drck->release = spapr_phb_release;
> }
>
> static const TypeInfo spapr_dr_connector_info = {
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 157867af8178..c12f71ae3e2d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1833,7 +1833,7 @@ void spapr_phb_dma_reset(sPAPRPHBState *sphb)
> sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
> }
>
> -static void spapr_phb_reset(DeviceState *qdev)
> +void spapr_phb_reset(DeviceState *qdev)
> {
> sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
>
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7837fb0b1110..15799cee4280 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -120,6 +120,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
>
> void spapr_pci_rtas_init(void);
>
> +void spapr_phb_reset(DeviceState *qdev);
> +
> sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> uint32_t config_addr);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f09c54d5bb94..a2f6782bdbbf 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -673,6 +673,7 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> /* CPU and LMB DRC release callbacks. */
> void spapr_core_release(DeviceState *dev);
> void spapr_lmb_release(DeviceState *dev);
> +void spapr_phb_release(DeviceState *dev);
>
> void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>
>
--
Alexey
On Thu, 27 Jul 2017 14:41:31 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 26/07/17 18:40, Greg Kurz 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.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > - rebased against ppc-for-2.10
> > - converted to unplug_request
> > - handle drc_id at pre-plug
> > - reset hotplugged PHB at plug
> > - compatibility with older machine types
> > ---
> > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > hw/ppc/spapr_drc.c | 1
> > hw/ppc/spapr_pci.c | 2 -
> > include/hw/pci-host/spapr.h | 2 +
> > include/hw/ppc/spapr.h | 1
> > 5 files changed, 118 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 90485054c2e7..589f76ef9fb8 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > &savevm_htab_handlers, spapr);
> >
> > + if (spapr->dr_phb_enabled) {
> > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > + }
> > +
> > qemu_register_boot_set(spapr_boot_set, spapr);
> >
> > if (kvm_enabled()) {
> > @@ -3238,6 +3242,103 @@ out:
> > error_propagate(errp, local_err);
> > }
> >
> > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > + Error **errp)
> > +{
> > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > +
> > + if (sphb->drc_id == (uint32_t)-1) {
> > + sphb->drc_id = sphb->index;
> > + }
> > +
> > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > + }
>
>
> sphb->index in considered 16bits in the existing code (even though it is
> defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> same limit for both, either 256 or 65536 is fine for me.
>
> It is actually a bit weird - it is possible to completely configure few
> PHBs in the command line so they will have index==-1 but PCI hotplug code -
> spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> this and just does (sphb->index << 16).
You're right and this looks like a bug... I'll try to come up with a fix.
> May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
>
This was how Mike did it in the original patchset but David suggested
to introduce drc_id (to preserve existing setups I guess):
http://patchwork.ozlabs.org/patch/466262/
>
>
> > +}
> > +
> > +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > + Error **errp)
> > +{
> > + sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > + void *fdt = NULL;
> > + int fdt_start_offset;
> > + int fdt_size;
> > + Error *local_err = NULL;
> > + sPAPRDRConnector *drc;
> > + uint32_t phandle;
> > + int ret;
> > + bool hotplugged = spapr_drc_hotplugged(dev);
> > +
> > + if (!spapr->dr_phb_enabled) {
> > + return;
> > + }
> > +
> > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > + /* hotplug hooks should check it's enabled before getting this far */
> > + g_assert(drc);
> > +
> > + if (hotplugged) {
> > + if (spapr->xics_phandle == UINT32_MAX) {
> > + error_setg(&local_err,
> > + "SLOF didn't update the XICS phandle. PHB hotplug cancelled");
> > + goto out;
> > + }
> > + phandle = spapr->xics_phandle;
> > +
> > + spapr_phb_reset(dev);
>
>
> It could be DEVICE_GET_CLASS(dev)->reset(dev) without exporting
> spapr_phb_reset. Not sure how this fits into the current QEMU coding style
> though.
>
>
>
> > + } else {
> > + phandle = PHANDLE_XICP;
> > + }
> > +
> > + fdt = create_device_tree(&fdt_size);
> > + ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
> > + if (ret < 0) {
> > + error_setg(&local_err, "unable to create FDT for %sPHB",
> > + dev->hotplugged ? "hotplugged " : "");
> > + goto out;
> > + }
> > +
> > + if (hotplugged) {
> > + /* generally SLOF creates these, for hotplug it's up to QEMU */
> > + _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
> > + }
> > +
> > + spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
> > +out:
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + g_free(fdt);
> > + return;
> > + }
> > +
> > + if (hotplugged) {
> > + spapr_hotplug_req_add_by_index(drc);
> > + } else if (drc) {
> > + spapr_drc_reset(drc);
> > + }
> > +}
> > +
> > +void spapr_phb_release(DeviceState *dev)
> > +{
> > + object_unparent(OBJECT(dev));
> > +}
> > +
> > +static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> > + DeviceState *dev, Error **errp)
> > +{
> > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > + sPAPRDRConnector *drc;
> > +
> > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > + g_assert(drc);
> > +
> > + if (!spapr_drc_unplug_requested(drc)) {
> > + spapr_drc_detach(drc);
> > + spapr_hotplug_req_remove_by_index(drc);
> > + }
> > +}
> > +
> > static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > @@ -3284,6 +3385,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > spapr_memory_plug(hotplug_dev, dev, node, errp);
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > spapr_core_plug(hotplug_dev, dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > + spapr_phb_plug(hotplug_dev, dev, errp);
> > }
> > }
> >
> > @@ -3311,6 +3414,12 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> > return;
> > }
> > spapr_core_unplug_request(hotplug_dev, dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > + if (sms->dr_phb_enabled) {
> > + spapr_phb_unplug_request(hotplug_dev, dev, errp);
> > + } else {
> > + error_setg(errp, "PHB hot unplug not supported on this machine");
> > + }
> > }
> > }
> >
> > @@ -3321,6 +3430,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > spapr_memory_pre_plug(hotplug_dev, dev, errp);
> > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > spapr_core_pre_plug(hotplug_dev, dev, errp);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > + spapr_phb_pre_plug(hotplug_dev, dev, errp);
> > }
> > }
> >
> > @@ -3328,7 +3439,8 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> > DeviceState *dev)
> > {
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > - object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE) ||
> > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > return HOTPLUG_HANDLER(machine);
> > }
> > return NULL;
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 2e1049ce61c7..845fcf70b932 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -704,6 +704,7 @@ static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > drck->typename = "PHB";
> > drck->drc_name_prefix = "PHB ";
> > + drck->release = spapr_phb_release;
> > }
> >
> > static const TypeInfo spapr_dr_connector_info = {
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 157867af8178..c12f71ae3e2d 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1833,7 +1833,7 @@ void spapr_phb_dma_reset(sPAPRPHBState *sphb)
> > sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
> > }
> >
> > -static void spapr_phb_reset(DeviceState *qdev)
> > +void spapr_phb_reset(DeviceState *qdev)
> > {
> > sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
> >
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index 7837fb0b1110..15799cee4280 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -120,6 +120,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
> >
> > void spapr_pci_rtas_init(void);
> >
> > +void spapr_phb_reset(DeviceState *qdev);
> > +
> > sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> > PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> > uint32_t config_addr);
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index f09c54d5bb94..a2f6782bdbbf 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -673,6 +673,7 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> > /* CPU and LMB DRC release callbacks. */
> > void spapr_core_release(DeviceState *dev);
> > void spapr_lmb_release(DeviceState *dev);
> > +void spapr_phb_release(DeviceState *dev);
> >
> > void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> > int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
> >
> >
>
>
Quoting Greg Kurz (2017-07-27 12:09:55)
> On Thu, 27 Jul 2017 14:41:31 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> > On 26/07/17 18:40, Greg Kurz 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.
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > - rebased against ppc-for-2.10
> > > - converted to unplug_request
> > > - handle drc_id at pre-plug
> > > - reset hotplugged PHB at plug
> > > - compatibility with older machine types
> > > ---
> > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > hw/ppc/spapr_drc.c | 1
> > > hw/ppc/spapr_pci.c | 2 -
> > > include/hw/pci-host/spapr.h | 2 +
> > > include/hw/ppc/spapr.h | 1
> > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 90485054c2e7..589f76ef9fb8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > &savevm_htab_handlers, spapr);
> > >
> > > + if (spapr->dr_phb_enabled) {
> > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > + }
> > > +
> > > qemu_register_boot_set(spapr_boot_set, spapr);
> > >
> > > if (kvm_enabled()) {
> > > @@ -3238,6 +3242,103 @@ out:
> > > error_propagate(errp, local_err);
> > > }
> > >
> > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > + Error **errp)
> > > +{
> > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > +
> > > + if (sphb->drc_id == (uint32_t)-1) {
> > > + sphb->drc_id = sphb->index;
> > > + }
> > > +
> > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > + }
> >
> >
> > sphb->index in considered 16bits in the existing code (even though it is
> > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > same limit for both, either 256 or 65536 is fine for me.
> >
> > It is actually a bit weird - it is possible to completely configure few
> > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > this and just does (sphb->index << 16).
>
> You're right and this looks like a bug... I'll try to come up with a fix.
Yup, that's a bug, and we can trigger it currently by adding 2
additional PHBs that don't have an index specified. QOM catches
and reports them as "attempt to add duplicate property", but it's
just reported by spapr_dr_connector_new() and doesn't seem to be
treated as fatal (and probably should be).
Might also see this more in practice now with the multi-phb support
in libvirt, though I'd imagine those would tend to rely on phb->index
being set.
Now that phb->drc_id is available though we can just use that instead.
I agree it should be limited to 16-bit or smaller to avoid any
possibility of overlap.
>
> > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> >
>
> This was how Mike did it in the original patchset but David suggested
> to introduce drc_id (to preserve existing setups I guess):
>
> http://patchwork.ozlabs.org/patch/466262/
Althrough IIRC what David proposed was to handle it like the other
properties which are filled in automatically when 'index' is specified,
i.e. 'drc_id' would be set automatically by 'index' if index is
specified, or we can set it explicitly if 'index' not specified, but
if we try to set both 'index' and 'drc_id' we trigger the "Either "index"
or other parameters must be specified for PAPR PHB, not both" error.
And it looks like 'index' is limited to 30, so it fits within our
16-bit limit for drc_id.
>
> >
> >
> > > +}
> > > +
> > > +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > + Error **errp)
> > > +{
> > > + sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > + void *fdt = NULL;
> > > + int fdt_start_offset;
> > > + int fdt_size;
> > > + Error *local_err = NULL;
> > > + sPAPRDRConnector *drc;
> > > + uint32_t phandle;
> > > + int ret;
> > > + bool hotplugged = spapr_drc_hotplugged(dev);
> > > +
> > > + if (!spapr->dr_phb_enabled) {
> > > + return;
> > > + }
> > > +
> > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > > + /* hotplug hooks should check it's enabled before getting this far */
> > > + g_assert(drc);
> > > +
> > > + if (hotplugged) {
> > > + if (spapr->xics_phandle == UINT32_MAX) {
> > > + error_setg(&local_err,
> > > + "SLOF didn't update the XICS phandle. PHB hotplug cancelled");
> > > + goto out;
> > > + }
> > > + phandle = spapr->xics_phandle;
> > > +
> > > + spapr_phb_reset(dev);
> >
> >
> > It could be DEVICE_GET_CLASS(dev)->reset(dev) without exporting
> > spapr_phb_reset. Not sure how this fits into the current QEMU coding style
> > though.
> >
> >
> >
> > > + } else {
> > > + phandle = PHANDLE_XICP;
> > > + }
> > > +
> > > + fdt = create_device_tree(&fdt_size);
> > > + ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
> > > + if (ret < 0) {
> > > + error_setg(&local_err, "unable to create FDT for %sPHB",
> > > + dev->hotplugged ? "hotplugged " : "");
> > > + goto out;
> > > + }
> > > +
> > > + if (hotplugged) {
> > > + /* generally SLOF creates these, for hotplug it's up to QEMU */
> > > + _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
> > > + }
> > > +
> > > + spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
> > > +out:
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + g_free(fdt);
> > > + return;
> > > + }
> > > +
> > > + if (hotplugged) {
> > > + spapr_hotplug_req_add_by_index(drc);
> > > + } else if (drc) {
> > > + spapr_drc_reset(drc);
> > > + }
> > > +}
> > > +
> > > +void spapr_phb_release(DeviceState *dev)
> > > +{
> > > + object_unparent(OBJECT(dev));
> > > +}
> > > +
> > > +static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> > > + DeviceState *dev, Error **errp)
> > > +{
> > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > + sPAPRDRConnector *drc;
> > > +
> > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > > + g_assert(drc);
> > > +
> > > + if (!spapr_drc_unplug_requested(drc)) {
> > > + spapr_drc_detach(drc);
> > > + spapr_hotplug_req_remove_by_index(drc);
> > > + }
> > > +}
> > > +
> > > static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev, Error **errp)
> > > {
> > > @@ -3284,6 +3385,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > spapr_memory_plug(hotplug_dev, dev, node, errp);
> > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > spapr_core_plug(hotplug_dev, dev, errp);
> > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > + spapr_phb_plug(hotplug_dev, dev, errp);
> > > }
> > > }
> > >
> > > @@ -3311,6 +3414,12 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> > > return;
> > > }
> > > spapr_core_unplug_request(hotplug_dev, dev, errp);
> > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > + if (sms->dr_phb_enabled) {
> > > + spapr_phb_unplug_request(hotplug_dev, dev, errp);
> > > + } else {
> > > + error_setg(errp, "PHB hot unplug not supported on this machine");
> > > + }
> > > }
> > > }
> > >
> > > @@ -3321,6 +3430,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > spapr_memory_pre_plug(hotplug_dev, dev, errp);
> > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > spapr_core_pre_plug(hotplug_dev, dev, errp);
> > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > + spapr_phb_pre_plug(hotplug_dev, dev, errp);
> > > }
> > > }
> > >
> > > @@ -3328,7 +3439,8 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> > > DeviceState *dev)
> > > {
> > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > - object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE) ||
> > > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > return HOTPLUG_HANDLER(machine);
> > > }
> > > return NULL;
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 2e1049ce61c7..845fcf70b932 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -704,6 +704,7 @@ static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > drck->typename = "PHB";
> > > drck->drc_name_prefix = "PHB ";
> > > + drck->release = spapr_phb_release;
> > > }
> > >
> > > static const TypeInfo spapr_dr_connector_info = {
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 157867af8178..c12f71ae3e2d 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -1833,7 +1833,7 @@ void spapr_phb_dma_reset(sPAPRPHBState *sphb)
> > > sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
> > > }
> > >
> > > -static void spapr_phb_reset(DeviceState *qdev)
> > > +void spapr_phb_reset(DeviceState *qdev)
> > > {
> > > sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
> > >
> > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > > index 7837fb0b1110..15799cee4280 100644
> > > --- a/include/hw/pci-host/spapr.h
> > > +++ b/include/hw/pci-host/spapr.h
> > > @@ -120,6 +120,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
> > >
> > > void spapr_pci_rtas_init(void);
> > >
> > > +void spapr_phb_reset(DeviceState *qdev);
> > > +
> > > sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> > > PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> > > uint32_t config_addr);
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index f09c54d5bb94..a2f6782bdbbf 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -673,6 +673,7 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> > > /* CPU and LMB DRC release callbacks. */
> > > void spapr_core_release(DeviceState *dev);
> > > void spapr_lmb_release(DeviceState *dev);
> > > +void spapr_phb_release(DeviceState *dev);
> > >
> > > void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> > > int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
> > >
> > >
> >
> >
>
On Thu, 27 Jul 2017 13:37:24 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> Quoting Greg Kurz (2017-07-27 12:09:55)
> > On Thu, 27 Jul 2017 14:41:31 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > > On 26/07/17 18:40, Greg Kurz 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.
> > > >
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > - rebased against ppc-for-2.10
> > > > - converted to unplug_request
> > > > - handle drc_id at pre-plug
> > > > - reset hotplugged PHB at plug
> > > > - compatibility with older machine types
> > > > ---
> > > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > > hw/ppc/spapr_drc.c | 1
> > > > hw/ppc/spapr_pci.c | 2 -
> > > > include/hw/pci-host/spapr.h | 2 +
> > > > include/hw/ppc/spapr.h | 1
> > > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 90485054c2e7..589f76ef9fb8 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > > &savevm_htab_handlers, spapr);
> > > >
> > > > + if (spapr->dr_phb_enabled) {
> > > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > > + }
> > > > +
> > > > qemu_register_boot_set(spapr_boot_set, spapr);
> > > >
> > > > if (kvm_enabled()) {
> > > > @@ -3238,6 +3242,103 @@ out:
> > > > error_propagate(errp, local_err);
> > > > }
> > > >
> > > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > + Error **errp)
> > > > +{
> > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > +
> > > > + if (sphb->drc_id == (uint32_t)-1) {
> > > > + sphb->drc_id = sphb->index;
> > > > + }
> > > > +
> > > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > > + }
> > >
> > >
> > > sphb->index in considered 16bits in the existing code (even though it is
> > > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > > same limit for both, either 256 or 65536 is fine for me.
> > >
> > > It is actually a bit weird - it is possible to completely configure few
> > > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > > this and just does (sphb->index << 16).
> >
> > You're right and this looks like a bug... I'll try to come up with a fix.
>
> Yup, that's a bug, and we can trigger it currently by adding 2
> additional PHBs that don't have an index specified. QOM catches
> and reports them as "attempt to add duplicate property", but it's
> just reported by spapr_dr_connector_new() and doesn't seem to be
> treated as fatal (and probably should be).
>
Yeah, spapr_dr_connector_new() doesn't care for errors at all and
happily returns a pointer to an unrealized DRC. This causes weird
behavior. For example, when using mandatory props instead of index:
-device spapr-pci-host-bridge,id=pci1,\
buid=0x800000020000001,\
liobn=0x80000100,\
liobn64=0x80000101,\
mem_win_addr=0x200100000000,\
mem64_win_addr=0x220000000000,\
io_win_addr=0x200000010000 \
-device spapr-pci-host-bridge,id=pci2,\
buid=0x800000020000002,\
liobn=0x80000200,\
liobn64=0x80000201,\
mem_win_addr=0x200180000000,\
mem64_win_addr=0x230000000000,\
io_win_addr=0x200000020000 \
-device virtio-net,id=hp1,bus=pci1.0 \
-device virtio-net,id=hp2,bus=pci2.0
QEMU complains and exits:
qemu-system-ppc64: -device virtio-net,id=hp2,bus=pci2.0: an attached device
is still awaiting release
> Might also see this more in practice now with the multi-phb support
> in libvirt, though I'd imagine those would tend to rely on phb->index
> being set.
>
I had confirmation from Shiva that libvirt passes the following to QEMU:
spapr-pci-host-bridge,index=X,id=pci.X<,numa_node=Z>
> Now that phb->drc_id is available though we can just use that instead.
> I agree it should be limited to 16-bit or smaller to avoid any
> possibility of overlap.
>
Well, David changed his mind and now suggests we rather make index mandatory
instead of adding drc_id (even if it breaks existing setups that don't rely
on index).
> >
> > > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> > >
> >
> > This was how Mike did it in the original patchset but David suggested
> > to introduce drc_id (to preserve existing setups I guess):
> >
> > http://patchwork.ozlabs.org/patch/466262/
>
> Althrough IIRC what David proposed was to handle it like the other
> properties which are filled in automatically when 'index' is specified,
> i.e. 'drc_id' would be set automatically by 'index' if index is
> specified, or we can set it explicitly if 'index' not specified, but
> if we try to set both 'index' and 'drc_id' we trigger the "Either "index"
> or other parameters must be specified for PAPR PHB, not both" error.
>
> And it looks like 'index' is limited to 30, so it fits within our
> 16-bit limit for drc_id.
>
> >
> > >
> > >
> > > > +}
> > > > +
> > > > +static void spapr_phb_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > + Error **errp)
> > > > +{
> > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > + void *fdt = NULL;
> > > > + int fdt_start_offset;
> > > > + int fdt_size;
> > > > + Error *local_err = NULL;
> > > > + sPAPRDRConnector *drc;
> > > > + uint32_t phandle;
> > > > + int ret;
> > > > + bool hotplugged = spapr_drc_hotplugged(dev);
> > > > +
> > > > + if (!spapr->dr_phb_enabled) {
> > > > + return;
> > > > + }
> > > > +
> > > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > > > + /* hotplug hooks should check it's enabled before getting this far */
> > > > + g_assert(drc);
> > > > +
> > > > + if (hotplugged) {
> > > > + if (spapr->xics_phandle == UINT32_MAX) {
> > > > + error_setg(&local_err,
> > > > + "SLOF didn't update the XICS phandle. PHB hotplug cancelled");
> > > > + goto out;
> > > > + }
> > > > + phandle = spapr->xics_phandle;
> > > > +
> > > > + spapr_phb_reset(dev);
> > >
> > >
> > > It could be DEVICE_GET_CLASS(dev)->reset(dev) without exporting
> > > spapr_phb_reset. Not sure how this fits into the current QEMU coding style
> > > though.
> > >
> > >
> > >
> > > > + } else {
> > > > + phandle = PHANDLE_XICP;
> > > > + }
> > > > +
> > > > + fdt = create_device_tree(&fdt_size);
> > > > + ret = spapr_populate_pci_dt(sphb, phandle, fdt, &fdt_start_offset);
> > > > + if (ret < 0) {
> > > > + error_setg(&local_err, "unable to create FDT for %sPHB",
> > > > + dev->hotplugged ? "hotplugged " : "");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (hotplugged) {
> > > > + /* generally SLOF creates these, for hotplug it's up to QEMU */
> > > > + _FDT(fdt_setprop_string(fdt, fdt_start_offset, "name", "pci"));
> > > > + }
> > > > +
> > > > + spapr_drc_attach(drc, DEVICE(dev), fdt, fdt_start_offset, &local_err);
> > > > +out:
> > > > + if (local_err) {
> > > > + error_propagate(errp, local_err);
> > > > + g_free(fdt);
> > > > + return;
> > > > + }
> > > > +
> > > > + if (hotplugged) {
> > > > + spapr_hotplug_req_add_by_index(drc);
> > > > + } else if (drc) {
> > > > + spapr_drc_reset(drc);
> > > > + }
> > > > +}
> > > > +
> > > > +void spapr_phb_release(DeviceState *dev)
> > > > +{
> > > > + object_unparent(OBJECT(dev));
> > > > +}
> > > > +
> > > > +static void spapr_phb_unplug_request(HotplugHandler *hotplug_dev,
> > > > + DeviceState *dev, Error **errp)
> > > > +{
> > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > + sPAPRDRConnector *drc;
> > > > +
> > > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PHB, sphb->drc_id);
> > > > + g_assert(drc);
> > > > +
> > > > + if (!spapr_drc_unplug_requested(drc)) {
> > > > + spapr_drc_detach(drc);
> > > > + spapr_hotplug_req_remove_by_index(drc);
> > > > + }
> > > > +}
> > > > +
> > > > static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > > DeviceState *dev, Error **errp)
> > > > {
> > > > @@ -3284,6 +3385,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> > > > spapr_memory_plug(hotplug_dev, dev, node, errp);
> > > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > spapr_core_plug(hotplug_dev, dev, errp);
> > > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > > + spapr_phb_plug(hotplug_dev, dev, errp);
> > > > }
> > > > }
> > > >
> > > > @@ -3311,6 +3414,12 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> > > > return;
> > > > }
> > > > spapr_core_unplug_request(hotplug_dev, dev, errp);
> > > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > > + if (sms->dr_phb_enabled) {
> > > > + spapr_phb_unplug_request(hotplug_dev, dev, errp);
> > > > + } else {
> > > > + error_setg(errp, "PHB hot unplug not supported on this machine");
> > > > + }
> > > > }
> > > > }
> > > >
> > > > @@ -3321,6 +3430,8 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > > > spapr_memory_pre_plug(hotplug_dev, dev, errp);
> > > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > spapr_core_pre_plug(hotplug_dev, dev, errp);
> > > > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > > + spapr_phb_pre_plug(hotplug_dev, dev, errp);
> > > > }
> > > > }
> > > >
> > > > @@ -3328,7 +3439,8 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
> > > > DeviceState *dev)
> > > > {
> > > > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> > > > - object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > > > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE) ||
> > > > + object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_PCI_HOST_BRIDGE)) {
> > > > return HOTPLUG_HANDLER(machine);
> > > > }
> > > > return NULL;
> > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > index 2e1049ce61c7..845fcf70b932 100644
> > > > --- a/hw/ppc/spapr_drc.c
> > > > +++ b/hw/ppc/spapr_drc.c
> > > > @@ -704,6 +704,7 @@ static void spapr_drc_phb_class_init(ObjectClass *k, void *data)
> > > > drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PHB;
> > > > drck->typename = "PHB";
> > > > drck->drc_name_prefix = "PHB ";
> > > > + drck->release = spapr_phb_release;
> > > > }
> > > >
> > > > static const TypeInfo spapr_dr_connector_info = {
> > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > > index 157867af8178..c12f71ae3e2d 100644
> > > > --- a/hw/ppc/spapr_pci.c
> > > > +++ b/hw/ppc/spapr_pci.c
> > > > @@ -1833,7 +1833,7 @@ void spapr_phb_dma_reset(sPAPRPHBState *sphb)
> > > > sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
> > > > }
> > > >
> > > > -static void spapr_phb_reset(DeviceState *qdev)
> > > > +void spapr_phb_reset(DeviceState *qdev)
> > > > {
> > > > sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
> > > >
> > > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > > > index 7837fb0b1110..15799cee4280 100644
> > > > --- a/include/hw/pci-host/spapr.h
> > > > +++ b/include/hw/pci-host/spapr.h
> > > > @@ -120,6 +120,8 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
> > > >
> > > > void spapr_pci_rtas_init(void);
> > > >
> > > > +void spapr_phb_reset(DeviceState *qdev);
> > > > +
> > > > sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> > > > PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> > > > uint32_t config_addr);
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index f09c54d5bb94..a2f6782bdbbf 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -673,6 +673,7 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
> > > > /* CPU and LMB DRC release callbacks. */
> > > > void spapr_core_release(DeviceState *dev);
> > > > void spapr_lmb_release(DeviceState *dev);
> > > > +void spapr_phb_release(DeviceState *dev);
> > > >
> > > > void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
> > > > int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
> > > >
> > > >
> > >
> > >
> >
>
On Thu, Jul 27, 2017 at 07:09:55PM +0200, Greg Kurz wrote:
> On Thu, 27 Jul 2017 14:41:31 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> > On 26/07/17 18:40, Greg Kurz 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.
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > - rebased against ppc-for-2.10
> > > - converted to unplug_request
> > > - handle drc_id at pre-plug
> > > - reset hotplugged PHB at plug
> > > - compatibility with older machine types
> > > ---
> > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > hw/ppc/spapr_drc.c | 1
> > > hw/ppc/spapr_pci.c | 2 -
> > > include/hw/pci-host/spapr.h | 2 +
> > > include/hw/ppc/spapr.h | 1
> > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 90485054c2e7..589f76ef9fb8 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > &savevm_htab_handlers, spapr);
> > >
> > > + if (spapr->dr_phb_enabled) {
> > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > + }
> > > +
> > > qemu_register_boot_set(spapr_boot_set, spapr);
> > >
> > > if (kvm_enabled()) {
> > > @@ -3238,6 +3242,103 @@ out:
> > > error_propagate(errp, local_err);
> > > }
> > >
> > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > + Error **errp)
> > > +{
> > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > +
> > > + if (sphb->drc_id == (uint32_t)-1) {
> > > + sphb->drc_id = sphb->index;
> > > + }
> > > +
> > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > + }
> >
> >
> > sphb->index in considered 16bits in the existing code (even though it is
> > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > same limit for both, either 256 or 65536 is fine for me.
> >
> > It is actually a bit weird - it is possible to completely configure few
> > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > this and just does (sphb->index << 16).
>
> You're right and this looks like a bug... I'll try to come up with a fix.
>
> > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> >
>
> This was how Mike did it in the original patchset but David suggested
> to introduce drc_id (to preserve existing setups I guess):
>
> http://patchwork.ozlabs.org/patch/466262/
Huh. So I did. But.. sorry, I've changed my mind.
The fact that needing a DRC forces us to have a reasonable small id
for each PHB seems like a good excuse to make index mandatory - I'm
not convinced anyone was actually creating PHBs without index, and
this does allow us to simplify a bunch of things.
I'd like to see that done as a preliminary cleanup patch, though.
--
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
On Fri, 28 Jul 2017 14:24:03 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, Jul 27, 2017 at 07:09:55PM +0200, Greg Kurz wrote:
> > On Thu, 27 Jul 2017 14:41:31 +1000
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > > On 26/07/17 18:40, Greg Kurz 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.
> > > >
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > - rebased against ppc-for-2.10
> > > > - converted to unplug_request
> > > > - handle drc_id at pre-plug
> > > > - reset hotplugged PHB at plug
> > > > - compatibility with older machine types
> > > > ---
> > > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > > hw/ppc/spapr_drc.c | 1
> > > > hw/ppc/spapr_pci.c | 2 -
> > > > include/hw/pci-host/spapr.h | 2 +
> > > > include/hw/ppc/spapr.h | 1
> > > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 90485054c2e7..589f76ef9fb8 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > > &savevm_htab_handlers, spapr);
> > > >
> > > > + if (spapr->dr_phb_enabled) {
> > > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > > + }
> > > > +
> > > > qemu_register_boot_set(spapr_boot_set, spapr);
> > > >
> > > > if (kvm_enabled()) {
> > > > @@ -3238,6 +3242,103 @@ out:
> > > > error_propagate(errp, local_err);
> > > > }
> > > >
> > > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > + Error **errp)
> > > > +{
> > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > +
> > > > + if (sphb->drc_id == (uint32_t)-1) {
> > > > + sphb->drc_id = sphb->index;
> > > > + }
> > > > +
> > > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > > + }
> > >
> > >
> > > sphb->index in considered 16bits in the existing code (even though it is
> > > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > > same limit for both, either 256 or 65536 is fine for me.
> > >
> > > It is actually a bit weird - it is possible to completely configure few
> > > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > > this and just does (sphb->index << 16).
> >
> > You're right and this looks like a bug... I'll try to come up with a fix.
> >
> > > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> > >
> >
> > This was how Mike did it in the original patchset but David suggested
> > to introduce drc_id (to preserve existing setups I guess):
> >
> > http://patchwork.ozlabs.org/patch/466262/
>
> Huh. So I did. But.. sorry, I've changed my mind.
>
> The fact that needing a DRC forces us to have a reasonable small id
> for each PHB seems like a good excuse to make index mandatory - I'm
> not convinced anyone was actually creating PHBs without index, and
> this does allow us to simplify a bunch of things.
>
> I'd like to see that done as a preliminary cleanup patch, though.
>
Just to be sure. I could verify that the weirdness reported by Alexey
causes QEMU to misbehave. Only the first "index-less" PHB has realized
DRCs:
=> subsequent "index-less" PHBs silently ignore hotplugging of PCI devices
=> QEMU won't even start with coldplugged devices in these "index-less"
PHBs
This preliminary cleanup for hotpluggable PHBs is hence also a bug fix
for current PHBs.
Do we want to fix this long-standing bug in 2.10 ?
Do we want to preserve the current buggy behavior for older machine types ?
On Tue, Aug 01, 2017 at 05:30:46PM +0200, Greg Kurz wrote:
> On Fri, 28 Jul 2017 14:24:03 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Thu, Jul 27, 2017 at 07:09:55PM +0200, Greg Kurz wrote:
> > > On Thu, 27 Jul 2017 14:41:31 +1000
> > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >
> > > > On 26/07/17 18:40, Greg Kurz 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.
> > > > >
> > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > - rebased against ppc-for-2.10
> > > > > - converted to unplug_request
> > > > > - handle drc_id at pre-plug
> > > > > - reset hotplugged PHB at plug
> > > > > - compatibility with older machine types
> > > > > ---
> > > > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > > > hw/ppc/spapr_drc.c | 1
> > > > > hw/ppc/spapr_pci.c | 2 -
> > > > > include/hw/pci-host/spapr.h | 2 +
> > > > > include/hw/ppc/spapr.h | 1
> > > > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 90485054c2e7..589f76ef9fb8 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > > > &savevm_htab_handlers, spapr);
> > > > >
> > > > > + if (spapr->dr_phb_enabled) {
> > > > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > > > + }
> > > > > +
> > > > > qemu_register_boot_set(spapr_boot_set, spapr);
> > > > >
> > > > > if (kvm_enabled()) {
> > > > > @@ -3238,6 +3242,103 @@ out:
> > > > > error_propagate(errp, local_err);
> > > > > }
> > > > >
> > > > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > + Error **errp)
> > > > > +{
> > > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > > +
> > > > > + if (sphb->drc_id == (uint32_t)-1) {
> > > > > + sphb->drc_id = sphb->index;
> > > > > + }
> > > > > +
> > > > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > > > + }
> > > >
> > > >
> > > > sphb->index in considered 16bits in the existing code (even though it is
> > > > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > > > same limit for both, either 256 or 65536 is fine for me.
> > > >
> > > > It is actually a bit weird - it is possible to completely configure few
> > > > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > > > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > > > this and just does (sphb->index << 16).
> > >
> > > You're right and this looks like a bug... I'll try to come up with a fix.
> > >
> > > > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> > > >
> > >
> > > This was how Mike did it in the original patchset but David suggested
> > > to introduce drc_id (to preserve existing setups I guess):
> > >
> > > http://patchwork.ozlabs.org/patch/466262/
> >
> > Huh. So I did. But.. sorry, I've changed my mind.
> >
> > The fact that needing a DRC forces us to have a reasonable small id
> > for each PHB seems like a good excuse to make index mandatory - I'm
> > not convinced anyone was actually creating PHBs without index, and
> > this does allow us to simplify a bunch of things.
> >
> > I'd like to see that done as a preliminary cleanup patch, though.
> >
>
> Just to be sure. I could verify that the weirdness reported by Alexey
> causes QEMU to misbehave. Only the first "index-less" PHB has realized
> DRCs:
>
> => subsequent "index-less" PHBs silently ignore hotplugging of PCI devices
>
> => QEMU won't even start with coldplugged devices in these "index-less"
> PHBs
>
> This preliminary cleanup for hotpluggable PHBs is hence also a bug fix
> for current PHBs.
Ok.
> Do we want to fix this long-standing bug in 2.10 ?
No, not worth pushing in this late.
> Do we want to preserve the current buggy behavior for older machine
> types ?
No, I don't think so. I think the reasonable course here is to push
the new behaviour out. Only if someone complains that they were
actually relying on the old behaviour in the wild do we try to
preserve it for the older machine types.
--
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
On Wed, 2 Aug 2017 12:39:12 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Aug 01, 2017 at 05:30:46PM +0200, Greg Kurz wrote:
> > On Fri, 28 Jul 2017 14:24:03 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Thu, Jul 27, 2017 at 07:09:55PM +0200, Greg Kurz wrote:
> > > > On Thu, 27 Jul 2017 14:41:31 +1000
> > > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > >
> > > > > On 26/07/17 18:40, Greg Kurz 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.
> > > > > >
> > > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > ---
> > > > > > - rebased against ppc-for-2.10
> > > > > > - converted to unplug_request
> > > > > > - handle drc_id at pre-plug
> > > > > > - reset hotplugged PHB at plug
> > > > > > - compatibility with older machine types
> > > > > > ---
> > > > > > hw/ppc/spapr.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> > > > > > hw/ppc/spapr_drc.c | 1
> > > > > > hw/ppc/spapr_pci.c | 2 -
> > > > > > include/hw/pci-host/spapr.h | 2 +
> > > > > > include/hw/ppc/spapr.h | 1
> > > > > > 5 files changed, 118 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 90485054c2e7..589f76ef9fb8 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -2540,6 +2540,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > > > > register_savevm_live(NULL, "spapr/htab", -1, 1,
> > > > > > &savevm_htab_handlers, spapr);
> > > > > >
> > > > > > + if (spapr->dr_phb_enabled) {
> > > > > > + qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine), NULL);
> > > > > > + }
> > > > > > +
> > > > > > qemu_register_boot_set(spapr_boot_set, spapr);
> > > > > >
> > > > > > if (kvm_enabled()) {
> > > > > > @@ -3238,6 +3242,103 @@ out:
> > > > > > error_propagate(errp, local_err);
> > > > > > }
> > > > > >
> > > > > > +static void spapr_phb_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > > > + Error **errp)
> > > > > > +{
> > > > > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(dev);
> > > > > > +
> > > > > > + if (sphb->drc_id == (uint32_t)-1) {
> > > > > > + sphb->drc_id = sphb->index;
> > > > > > + }
> > > > > > +
> > > > > > + if (sphb->drc_id >= SPAPR_DRC_MAX_PHB) {
> > > > > > + error_setg(errp, "PHB id %d out of range", sphb->drc_id);
> > > > > > + }
> > > > >
> > > > >
> > > > > sphb->index in considered 16bits in the existing code (even though it is
> > > > > defined as 32bit) and SPAPR_DRC_MAX_PHB is just 256. I'd suggest using the
> > > > > same limit for both, either 256 or 65536 is fine for me.
> > > > >
> > > > > It is actually a bit weird - it is possible to completely configure few
> > > > > PHBs in the command line so they will have index==-1 but PCI hotplug code -
> > > > > spapr_phb_get_pci_func_drc() and spapr_phb_realize() - does not check for
> > > > > this and just does (sphb->index << 16).
> > > >
> > > > You're right and this looks like a bug... I'll try to come up with a fix.
> > > >
> > > > > May be just ditch drc_id, enforce index not to be -1 and use it as drc_id?
> > > > >
> > > >
> > > > This was how Mike did it in the original patchset but David suggested
> > > > to introduce drc_id (to preserve existing setups I guess):
> > > >
> > > > http://patchwork.ozlabs.org/patch/466262/
> > >
> > > Huh. So I did. But.. sorry, I've changed my mind.
> > >
> > > The fact that needing a DRC forces us to have a reasonable small id
> > > for each PHB seems like a good excuse to make index mandatory - I'm
> > > not convinced anyone was actually creating PHBs without index, and
> > > this does allow us to simplify a bunch of things.
> > >
> > > I'd like to see that done as a preliminary cleanup patch, though.
> > >
> >
> > Just to be sure. I could verify that the weirdness reported by Alexey
> > causes QEMU to misbehave. Only the first "index-less" PHB has realized
> > DRCs:
> >
> > => subsequent "index-less" PHBs silently ignore hotplugging of PCI devices
> >
> > => QEMU won't even start with coldplugged devices in these "index-less"
> > PHBs
> >
> > This preliminary cleanup for hotpluggable PHBs is hence also a bug fix
> > for current PHBs.
>
> Ok.
>
> > Do we want to fix this long-standing bug in 2.10 ?
>
> No, not worth pushing in this late.
>
Cool. This will give us enough time to do it right.
> > Do we want to preserve the current buggy behavior for older machine
> > types ?
>
> No, I don't think so. I think the reasonable course here is to push
> the new behaviour out. Only if someone complains that they were
> actually relying on the old behaviour in the wild do we try to
> preserve it for the older machine types.
>
Perfect ! I was hoping you'd say that. :)
Cheers,
--
Greg
© 2016 - 2025 Red Hat, Inc.