| Message ID | 1471442498-8357-1-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 17, 2016 at 07:31:38PM +0530, Bharata B Rao wrote: > sPAPR supports only Core level CPU plug and unplug, but nothing > prevents user from issuing a device_del on the underlying thread > device by using its qom path directly. This hits g_assert(hotplug_ctrl) > in qdev_unplug(). > > Gracefully reject such unplug requests from ->unplug() handler > > Reported-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Why isn't there a graceful failure if we return NULL from the hotplug_handler()? Doesn't that indicate a bug in the generic code? Couldn't the same error be triggered by attempting to unplug some other random device - say the RTC on x86, or the NVRAM on POWER? Also I only just noticed that we've had a misspelling here for ages: "hotpug_handler". > --- > hw/ppc/spapr.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 30d6800..0e89d7d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2344,6 +2344,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > return; > } > spapr_core_unplug(hotplug_dev, dev, errp); > + } else { > + error_setg(errp, "Unplug not supported for device type: %s", > + object_get_typename(OBJECT(dev))); > } > } > > @@ -2359,6 +2362,7 @@ static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, > DeviceState *dev) > { > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || > + object_dynamic_cast(OBJECT(dev), TYPE_CPU) || > object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > return HOTPLUG_HANDLER(machine); > }
On Thu, Aug 18, 2016 at 10:57:04AM +1000, David Gibson wrote: > On Wed, Aug 17, 2016 at 07:31:38PM +0530, Bharata B Rao wrote: > > sPAPR supports only Core level CPU plug and unplug, but nothing > > prevents user from issuing a device_del on the underlying thread > > device by using its qom path directly. This hits g_assert(hotplug_ctrl) > > in qdev_unplug(). > > > > Gracefully reject such unplug requests from ->unplug() handler > > > > Reported-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Why isn't there a graceful failure if we return NULL from the > hotplug_handler()? Doesn't that indicate a bug in the generic code? > Couldn't the same error be triggered by attempting to unplug some > other random device - say the RTC on x86, or the NVRAM on POWER? True, realized that this error can be triggered for other devices as well like dr-connector or icp devices. So it definitely doesn't make sense to explicitly return a non-NULL hotplug_handler for all of these and then fail the unplug gracefully from ->unplug() like I am doing here for CPU thread devices. Hence, this particular fix isn't needed right now. May be we can gracefully error out for all such cases from qdev_unplug(). Regards, Bharata.
On Thu, 18 Aug 2016 15:50:45 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Thu, Aug 18, 2016 at 10:57:04AM +1000, David Gibson wrote: > > On Wed, Aug 17, 2016 at 07:31:38PM +0530, Bharata B Rao wrote: > > > sPAPR supports only Core level CPU plug and unplug, but nothing > > > prevents user from issuing a device_del on the underlying thread > > > device by using its qom path directly. This hits g_assert(hotplug_ctrl) > > > in qdev_unplug(). > > > > > > Gracefully reject such unplug requests from ->unplug() handler > > > > > > Reported-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Why isn't there a graceful failure if we return NULL from the > > hotplug_handler()? Doesn't that indicate a bug in the generic code? > > Couldn't the same error be triggered by attempting to unplug some > > other random device - say the RTC on x86, or the NVRAM on POWER? > > True, realized that this error can be triggered for other devices as well > like dr-connector or icp devices. So it definitely doesn't make sense > to explicitly return a non-NULL hotplug_handler for all of these > and then fail the unplug gracefully from ->unplug() like I am doing here > for CPU thread devices. > > Hence, this particular fix isn't needed right now. May be we can gracefully > error out for all such cases from qdev_unplug(). device_del by QOM path was discussed here: https://patchwork.ozlabs.org/patch/516781/ but it only takes Device::hotpluggable into account, perhaps we should add something like DeviceClass::cannot_delete_device_del in addition to above? > > Regards, > Bharata. >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 30d6800..0e89d7d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2344,6 +2344,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, return; } spapr_core_unplug(hotplug_dev, dev, errp); + } else { + error_setg(errp, "Unplug not supported for device type: %s", + object_get_typename(OBJECT(dev))); } } @@ -2359,6 +2362,7 @@ static HotplugHandler *spapr_get_hotpug_handler(MachineState *machine, DeviceState *dev) { if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) || + object_dynamic_cast(OBJECT(dev), TYPE_CPU) || object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { return HOTPLUG_HANDLER(machine); }
sPAPR supports only Core level CPU plug and unplug, but nothing prevents user from issuing a device_del on the underlying thread device by using its qom path directly. This hits g_assert(hotplug_ctrl) in qdev_unplug(). Gracefully reject such unplug requests from ->unplug() handler Reported-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 4 ++++ 1 file changed, 4 insertions(+)