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

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~

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.

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