[Qemu-devel] [FIX PATCH] spapr: Gracefully fail CPU thread unplug

[Qemu-devel] [FIX PATCH] spapr: Gracefully fail CPU thread unplug

[Bharata B Rao]

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(+)

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);
     }
-- 
2.7.4

Re: [Qemu-devel] [FIX PATCH] spapr: Gracefully fail CPU thread unplug

[David Gibson]

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

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);
>      }

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [FIX PATCH] spapr: Gracefully fail CPU thread unplug

[Bharata B Rao]

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.

Re: [Qemu-devel] Change the condition that permits to use device_del QOM-path on device? was (spapr: Gracefully fail CPU thread unplug)

[Igor Mammedov]

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