[PATCH for-6.0 0/4] Don't treat all sysbus devices as hotpluggable

[PATCH for-6.0 0/4] Don't treat all sysbus devices as hotpluggable

[Peter Maydell]

On the two machines which have the "platform bus" (ppc e500 and arm
virt) we currently treat all TYPE_SYS_BUS_DEVICE devices as being
hotpluggable in the device callbacks, and try to plug those devices
into the platform bus.  This is far too broad, because only a handful
of devices are actually valid to plug into the platform bus. 
Moreover, if a device which is pluggable for some other reason (like
a PCI device) happens to use a sysbus device internally as part of
its implementation, the hotplug callback will incorrectly grab that
sysbus device, probably resulting in an assertion failure.

Mostly PCI devices don't use sysbus devices internally, so the only
case we've encountered so far is the not-valid-anyway
 qemu-system-ppc64 -M ppce500 -device macio-oldworld
but we might create more in future.

This series restricts hotpluggability of sysbus devices on these
platforms to those devices which are on the dynamic sysbus whitelist
(which we were maintaining anyway).  With it, the above ppc
commandline stops asserting and instead fails cleanly with
  qemu-system-ppc64: Device heathrow is not supported by this machine yet.

Patch 1 is an API doc improvement while I was in the header file
anyway.

thanks
-- PMM

Peter Maydell (4):
  include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()
  machine: Provide a function to check the dynamic sysbus whitelist
  hw/arm/virt: Only try to add valid dynamic sysbus devices to platform
    bus
  hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to
    platform bus

 include/hw/boards.h | 38 ++++++++++++++++++++++++++++++++++++++
 hw/arm/virt.c       |  8 ++++++--
 hw/core/machine.c   | 21 ++++++++++++++++-----
 hw/ppc/e500plat.c   |  8 ++++++--
 4 files changed, 66 insertions(+), 9 deletions(-)

-- 
2.20.1

[PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Peter Maydell]

The function machine_class_allow_dynamic_sysbus_dev() is currently
undocumented; add a doc comment.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/boards.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 4a90549ad85..27106abc11d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props,
                                Error **errp);
 
+/**
+ * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
+ * @mc: Machine class
+ * @type: type to whitelist (should be a subtype of TYPE_SYS_BUS_DEVICE)
+ *
+ * Add the QOM type @type to the list of devices of which are subtypes
+ * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically
+ * created (eg by the user on the command line with -device).
+ * By default if the user tries to create any devices on the command line
+ * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message;
+ * for the special cases which are permitted for this machine model, the
+ * machine model class init code must call this function to whitelist them.
+ */
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
+
 /*
  * Checks that backend isn't used, preps it for exclusive usage and
  * returns migratable MemoryRegion provided by backend.
-- 
2.20.1

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The function machine_class_allow_dynamic_sysbus_dev() is currently
> undocumented; add a doc comment.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/boards.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 4a90549ad85..27106abc11d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
>                                 const CpuInstanceProperties *props,
>                                 Error **errp);
>  
> +/**
> + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
nit: s/of valid devices/of dynamically instantiable sysbus devices ?
> + * @mc: Machine class
> + * @type: type to whitelist (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + *
> + * Add the QOM type @type to the list of devices of which are subtypes
> + * of TYPE_SYS_BUS_DEVICE but which are still permitted to be dynamically
> + * created (eg by the user on the command line with -device).
> + * By default if the user tries to create any devices on the command line
> + * that are subtypes of TYPE_SYS_BUS_DEVICE they will get an error message;
> + * for the special cases which are permitted for this machine model, the
> + * machine model class init code must call this function to whitelist them.
> + */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
> +
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
>   * returns migratable MemoryRegion provided by backend.
> 
Besides

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Peter Maydell]

On Fri, 26 Mar 2021 at 09:27, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 3/25/21 4:33 PM, Peter Maydell wrote:
> > The function machine_class_allow_dynamic_sysbus_dev() is currently
> > undocumented; add a doc comment.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  include/hw/boards.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 4a90549ad85..27106abc11d 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
> >                                 const CpuInstanceProperties *props,
> >                                 Error **errp);
> >
> > +/**
> > + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
> nit: s/of valid devices/of dynamically instantiable sysbus devices ?

I was trying to keep the summary line to be one line, which
doesn't give much space for nuance with a function name this long...


-- PMM

Re: [PATCH for-6.0 1/4] include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()

[Auger Eric]

Hi Peter,

On 3/26/21 11:20 AM, Peter Maydell wrote:
> On Fri, 26 Mar 2021 at 09:27, Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 3/25/21 4:33 PM, Peter Maydell wrote:
>>> The function machine_class_allow_dynamic_sysbus_dev() is currently
>>> undocumented; add a doc comment.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  include/hw/boards.h | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 4a90549ad85..27106abc11d 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -36,7 +36,21 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>>                                 const CpuInstanceProperties *props,
>>>                                 Error **errp);
>>>
>>> +/**
>>> + * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
>> nit: s/of valid devices/of dynamically instantiable sysbus devices ?
> 
> I was trying to keep the summary line to be one line, which
> doesn't give much space for nuance with a function name this long...

OK no worries

Thanks

Eric
> 
> 
> -- PMM
> 

[PATCH for-6.0 2/4] machine: Provide a function to check the dynamic sysbus whitelist

[Peter Maydell]

Provide a new function dynamic_sysbus_dev_allowed() which checks
the per-machine whitelist of dynamic sysbus devices and returns
a boolean result indicating whether the device is whitelisted.
We can use this in the implementation of validate_sysbus_device(),
but we will also need it so that machine hotplug callbacks can
validate devices rather than assuming that any sysbus device
might be hotpluggable into the platform bus.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/boards.h | 24 ++++++++++++++++++++++++
 hw/core/machine.c   | 21 ++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 27106abc11d..609112a4e1a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -51,6 +51,30 @@ void machine_set_cpu_numa_node(MachineState *machine,
  */
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
 
+/**
+ * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
+ * @mc: Machine class
+ * @dev: device to check
+ *
+ * Returns: true if @dev is a sysbus device on the machine's whitelist
+ * of dynamically pluggable sysbus devices; otherwise false.
+ *
+ * This function checks whether @dev is a valid dynamic sysbus device,
+ * by first confirming that it is a sysbus device and then checking it
+ * against the whitelist of permitted dynamic sysbus devices which has
+ * been set up by the machine using machine_class_allow_dynamic_sysbus_dev().
+ *
+ * It is valid to call this with something that is not a subclass of
+ * TYPE_SYS_BUS_DEVICE; the function will return false in this case.
+ * This allows hotplug callback functions to be written as:
+ *     if (device_is_dynamic_sysbus(mc, dev)) {
+ *         handle dynamic sysbus case;
+ *     } else if (some other kind of hotplug) {
+ *         handle that;
+ *     }
+ */
+bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev);
+
 /*
  * Checks that backend isn't used, preps it for exclusive usage and
  * returns migratable MemoryRegion provided by backend.
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9935c6ddd56..8d97094736a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -529,20 +529,31 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
     QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
 }
 
-static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
+bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
 {
-    MachineState *machine = opaque;
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
     bool allowed = false;
     strList *wl;
+    Object *obj = OBJECT(dev);
+
+    if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
+        return false;
+    }
 
     for (wl = mc->allowed_dynamic_sysbus_devices;
          !allowed && wl;
          wl = wl->next) {
-        allowed |= !!object_dynamic_cast(OBJECT(sbdev), wl->value);
+        allowed |= !!object_dynamic_cast(obj, wl->value);
     }
 
-    if (!allowed) {
+    return allowed;
+}
+
+static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
+{
+    MachineState *machine = opaque;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+    if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
         error_report("Option '-device %s' cannot be handled by this machine",
                      object_class_get_name(object_get_class(OBJECT(sbdev))));
         exit(1);
-- 
2.20.1

Re: [PATCH for-6.0 2/4] machine: Provide a function to check the dynamic sysbus whitelist

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> Provide a new function dynamic_sysbus_dev_allowed() which checks
> the per-machine whitelist of dynamic sysbus devices and returns
> a boolean result indicating whether the device is whitelisted.
> We can use this in the implementation of validate_sysbus_device(),
> but we will also need it so that machine hotplug callbacks can
> validate devices rather than assuming that any sysbus device
> might be hotpluggable into the platform bus.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
>  include/hw/boards.h | 24 ++++++++++++++++++++++++
>  hw/core/machine.c   | 21 ++++++++++++++++-----
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 27106abc11d..609112a4e1a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,6 +51,30 @@ void machine_set_cpu_numa_node(MachineState *machine,
>   */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>  
> +/**
> + * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
> + * @mc: Machine class
> + * @dev: device to check
> + *
> + * Returns: true if @dev is a sysbus device on the machine's whitelist
> + * of dynamically pluggable sysbus devices; otherwise false.
> + *
> + * This function checks whether @dev is a valid dynamic sysbus device,
> + * by first confirming that it is a sysbus device and then checking it
> + * against the whitelist of permitted dynamic sysbus devices which has
> + * been set up by the machine using machine_class_allow_dynamic_sysbus_dev().
> + *
> + * It is valid to call this with something that is not a subclass of
> + * TYPE_SYS_BUS_DEVICE; the function will return false in this case.
> + * This allows hotplug callback functions to be written as:
> + *     if (device_is_dynamic_sysbus(mc, dev)) {
> + *         handle dynamic sysbus case;
> + *     } else if (some other kind of hotplug) {
> + *         handle that;
> + *     }
> + */
> +bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev);
> +
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
>   * returns migratable MemoryRegion provided by backend.
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9935c6ddd56..8d97094736a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -529,20 +529,31 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>      QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
>  }
>  
> -static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
> +bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>  {
> -    MachineState *machine = opaque;
> -    MachineClass *mc = MACHINE_GET_CLASS(machine);
>      bool allowed = false;
>      strList *wl;
> +    Object *obj = OBJECT(dev);
> +
> +    if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
> +        return false;
> +    }
>  
>      for (wl = mc->allowed_dynamic_sysbus_devices;
>           !allowed && wl;
>           wl = wl->next) {
> -        allowed |= !!object_dynamic_cast(OBJECT(sbdev), wl->value);
> +        allowed |= !!object_dynamic_cast(obj, wl->value);
>      }
>  
> -    if (!allowed) {
> +    return allowed;
> +}
> +
> +static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
> +{
> +    MachineState *machine = opaque;
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +    if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
>          error_report("Option '-device %s' cannot be handled by this machine",
>                       object_class_get_name(object_get_class(OBJECT(sbdev))));
>          exit(1);
> 

[PATCH for-6.0 3/4] hw/arm/virt: Only try to add valid dynamic sysbus devices to platform bus

[Peter Maydell]

The virt machine device plug callback currently calls
platform_bus_link_device() for any sysbus device.  This is overly
broad, because platform_bus_link_device() will unconditionally grab
the IRQs and MMIOs of the device it is passed, whether it was
intended for the platform bus or not.  Restrict hotpluggability of
sysbus devices to only those devices on the dynamic sysbus whitelist.

We were mostly getting away with this because the board creates the
platform bus as the last device it creates, and so the hotplug
callback did not do anything for all the sysbus devices created by
the board itself.  However if the user plugged in a device which
itself uses a sysbus device internally we would have mishandled this
and probably asserted.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aa2bbd14e09..8625152a735 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2443,7 +2443,9 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
 
     if (vms->platform_bus_dev) {
-        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        MachineClass *mc = MACHINE_GET_CLASS(vms);
+
+        if (device_is_dynamic_sysbus(mc, dev)) {
             platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
                                      SYS_BUS_DEVICE(dev));
         }
@@ -2527,7 +2529,9 @@ static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
 static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
                                                         DeviceState *dev)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+    if (device_is_dynamic_sysbus(mc, dev) ||
        (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
         return HOTPLUG_HANDLER(machine);
     }
-- 
2.20.1

Re: [PATCH for-6.0 3/4] hw/arm/virt: Only try to add valid dynamic sysbus devices to platform bus

[Auger Eric]

Hi Peter,

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The virt machine device plug callback currently calls
> platform_bus_link_device() for any sysbus device.  This is overly
> broad, because platform_bus_link_device() will unconditionally grab
> the IRQs and MMIOs of the device it is passed, whether it was
> intended for the platform bus or not.  Restrict hotpluggability of
> sysbus devices to only those devices on the dynamic sysbus whitelist.
> 
> We were mostly getting away with this because the board creates the
> platform bus as the last device it creates, and so the hotplug
> callback did not do anything for all the sysbus devices created by
> the board itself.  However if the user plugged in a device which
> itself uses a sysbus device internally we would have mishandled this
> and probably asserted.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/arm/virt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index aa2bbd14e09..8625152a735 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2443,7 +2443,9 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  
>      if (vms->platform_bus_dev) {
> -        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        MachineClass *mc = MACHINE_GET_CLASS(vms);
> +
> +        if (device_is_dynamic_sysbus(mc, dev)) {
>              platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
>                                       SYS_BUS_DEVICE(dev));
>          }
> @@ -2527,7 +2529,9 @@ static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>                                                          DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +    if (device_is_dynamic_sysbus(mc, dev) ||
>         (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
>          return HOTPLUG_HANDLER(machine);
>      }
> 

[PATCH for-6.0 4/4] hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to platform bus

[Peter Maydell]

The e500plat machine device plug callback currently calls
platform_bus_link_device() for any sysbus device.  This is overly
broad, because platform_bus_link_device() will unconditionally grab
the IRQs and MMIOs of the device it is passed, whether it was
intended for the platform bus or not.  Restrict hotpluggability of
sysbus devices to only those devices on the dynamic sysbus whitelist.

We were mostly getting away with this because the board creates the
platform bus as the last device it creates, and so the hotplug
callback did not do anything for all the sysbus devices created by
the board itself.  However if the user plugged in a device which
itself uses a sysbus device internally we would have mishandled this
and probably asserted. An example of this is:
 qemu-system-ppc64 -M ppce500 -device macio-oldworld

This isn't a sensible command because the macio-oldworld device
is really specific to the 'g3beige' machine, but we now fail
with a reasonable error message rather than asserting:
qemu-system-ppc64: Device heathrow is not supported by this machine yet.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/e500plat.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index bddd5e7c48f..fc911bbb7bd 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -48,7 +48,9 @@ static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
 
     if (pms->pbus_dev) {
-        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+        MachineClass *mc = MACHINE_GET_CLASS(pms);
+
+        if (device_is_dynamic_sysbus(mc, dev)) {
             platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
         }
     }
@@ -58,7 +60,9 @@ static
 HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
                                                     DeviceState *dev)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+
+    if (device_is_dynamic_sysbus(mc, dev)) {
         return HOTPLUG_HANDLER(machine);
     }
 
-- 
2.20.1

Re: [PATCH for-6.0 4/4] hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to platform bus

[David Gibson]

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

On Thu, Mar 25, 2021 at 03:33:10PM +0000, Peter Maydell wrote:
> The e500plat machine device plug callback currently calls
> platform_bus_link_device() for any sysbus device.  This is overly
> broad, because platform_bus_link_device() will unconditionally grab
> the IRQs and MMIOs of the device it is passed, whether it was
> intended for the platform bus or not.  Restrict hotpluggability of
> sysbus devices to only those devices on the dynamic sysbus whitelist.
> 
> We were mostly getting away with this because the board creates the
> platform bus as the last device it creates, and so the hotplug
> callback did not do anything for all the sysbus devices created by
> the board itself.  However if the user plugged in a device which
> itself uses a sysbus device internally we would have mishandled this
> and probably asserted. An example of this is:
>  qemu-system-ppc64 -M ppce500 -device macio-oldworld
> 
> This isn't a sensible command because the macio-oldworld device
> is really specific to the 'g3beige' machine, but we now fail
> with a reasonable error message rather than asserting:
> qemu-system-ppc64: Device heathrow is not supported by this machine yet.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/ppc/e500plat.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index bddd5e7c48f..fc911bbb7bd 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -48,7 +48,9 @@ static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
>  
>      if (pms->pbus_dev) {
> -        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        MachineClass *mc = MACHINE_GET_CLASS(pms);
> +
> +        if (device_is_dynamic_sysbus(mc, dev)) {
>              platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
>          }
>      }
> @@ -58,7 +60,9 @@ static
>  HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
>                                                      DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +    if (device_is_dynamic_sysbus(mc, dev)) {
>          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: 833 bytes --]

Re: [PATCH for-6.0 4/4] hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to platform bus

[Auger Eric]

On 3/25/21 4:33 PM, Peter Maydell wrote:
> The e500plat machine device plug callback currently calls
> platform_bus_link_device() for any sysbus device.  This is overly
> broad, because platform_bus_link_device() will unconditionally grab
> the IRQs and MMIOs of the device it is passed, whether it was
> intended for the platform bus or not.  Restrict hotpluggability of
> sysbus devices to only those devices on the dynamic sysbus whitelist.
> 
> We were mostly getting away with this because the board creates the
> platform bus as the last device it creates, and so the hotplug
> callback did not do anything for all the sysbus devices created by
> the board itself.  However if the user plugged in a device which
> itself uses a sysbus device internally we would have mishandled this
> and probably asserted. An example of this is:
>  qemu-system-ppc64 -M ppce500 -device macio-oldworld
> 
> This isn't a sensible command because the macio-oldworld device
> is really specific to the 'g3beige' machine, but we now fail
> with a reasonable error message rather than asserting:
> qemu-system-ppc64: Device heathrow is not supported by this machine yet.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/ppc/e500plat.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index bddd5e7c48f..fc911bbb7bd 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -48,7 +48,9 @@ static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>      PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
>  
>      if (pms->pbus_dev) {
> -        if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +        MachineClass *mc = MACHINE_GET_CLASS(pms);
> +
> +        if (device_is_dynamic_sysbus(mc, dev)) {
>              platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
>          }
>      }
> @@ -58,7 +60,9 @@ static
>  HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
>                                                      DeviceState *dev)
>  {
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +    if (device_is_dynamic_sysbus(mc, dev)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>  
> 

Re: [PATCH for-6.0 0/4] Don't treat all sysbus devices as hotpluggable

[Richard Henderson]

On 3/25/21 9:33 AM, Peter Maydell wrote:
> On the two machines which have the "platform bus" (ppc e500 and arm
> virt) we currently treat all TYPE_SYS_BUS_DEVICE devices as being
> hotpluggable in the device callbacks, and try to plug those devices
> into the platform bus.  This is far too broad, because only a handful
> of devices are actually valid to plug into the platform bus.
> Moreover, if a device which is pluggable for some other reason (like
> a PCI device) happens to use a sysbus device internally as part of
> its implementation, the hotplug callback will incorrectly grab that
> sysbus device, probably resulting in an assertion failure.
> 
> Mostly PCI devices don't use sysbus devices internally, so the only
> case we've encountered so far is the not-valid-anyway
>   qemu-system-ppc64 -M ppce500 -device macio-oldworld
> but we might create more in future.
> 
> This series restricts hotpluggability of sysbus devices on these
> platforms to those devices which are on the dynamic sysbus whitelist

s/whitelist/allowlist/g ?

>    include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()
>    machine: Provide a function to check the dynamic sysbus whitelist
>    hw/arm/virt: Only try to add valid dynamic sysbus devices to platform
>      bus
>    hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to
>      platform bus

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-6.0 0/4] Don't treat all sysbus devices as hotpluggable

[Mark Cave-Ayland]

On 25/03/2021 15:33, Peter Maydell wrote:

> On the two machines which have the "platform bus" (ppc e500 and arm
> virt) we currently treat all TYPE_SYS_BUS_DEVICE devices as being
> hotpluggable in the device callbacks, and try to plug those devices
> into the platform bus.  This is far too broad, because only a handful
> of devices are actually valid to plug into the platform bus.
> Moreover, if a device which is pluggable for some other reason (like
> a PCI device) happens to use a sysbus device internally as part of
> its implementation, the hotplug callback will incorrectly grab that
> sysbus device, probably resulting in an assertion failure.
> 
> Mostly PCI devices don't use sysbus devices internally, so the only
> case we've encountered so far is the not-valid-anyway
>   qemu-system-ppc64 -M ppce500 -device macio-oldworld
> but we might create more in future.
> 
> This series restricts hotpluggability of sysbus devices on these
> platforms to those devices which are on the dynamic sysbus whitelist
> (which we were maintaining anyway).  With it, the above ppc
> commandline stops asserting and instead fails cleanly with
>    qemu-system-ppc64: Device heathrow is not supported by this machine yet.
> 
> Patch 1 is an API doc improvement while I was in the header file
> anyway.
> 
> thanks
> -- PMM
> 
> Peter Maydell (4):
>    include/hw/boards.h: Document machine_class_allow_dynamic_sysbus_dev()
>    machine: Provide a function to check the dynamic sysbus whitelist
>    hw/arm/virt: Only try to add valid dynamic sysbus devices to platform
>      bus
>    hw/ppc/e500plat: Only try to add valid dynamic sysbus devices to
>      platform bus
> 
>   include/hw/boards.h | 38 ++++++++++++++++++++++++++++++++++++++
>   hw/arm/virt.c       |  8 ++++++--
>   hw/core/machine.c   | 21 ++++++++++++++++-----
>   hw/ppc/e500plat.c   |  8 ++++++--
>   4 files changed, 66 insertions(+), 9 deletions(-)

Looks good to me having been poking around the code earlier today so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH for-6.0 0/4] Don't treat all sysbus devices as hotpluggable

[Peter Maydell]

On Thu, 25 Mar 2021 at 15:33, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On the two machines which have the "platform bus" (ppc e500 and arm
> virt) we currently treat all TYPE_SYS_BUS_DEVICE devices as being
> hotpluggable in the device callbacks, and try to plug those devices
> into the platform bus.  This is far too broad, because only a handful
> of devices are actually valid to plug into the platform bus.
> Moreover, if a device which is pluggable for some other reason (like
> a PCI device) happens to use a sysbus device internally as part of
> its implementation, the hotplug callback will incorrectly grab that
> sysbus device, probably resulting in an assertion failure.

I'm taking this into target-arm.next for 6.0 (with minor commit message
and comment text tweaks to use 'allowlist' as suggested by rth).

thanks
-- PMM