Replace has_dynamic_sysbus with device type whitelist

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Cc Juergen Gross <[email protected]>

Cc Stefano Stabellini <[email protected]>

Signed-off-by Eduardo Habkost <[email protected]>

---
 hw/xen/xen_backend.c | 11 -----------
 1 file changed, 11 deletions(-)

hw/xen/xen_backend.c +0 -11

index 6c21c37d68..4607d6d3ee 100644
@@ -550,15 +550,6 @@ err:
     return -1;
 }
 
-static void xen_set_dynamic_sysbus(void)
-{
-    Object *machine = qdev_get_machine();
-    ObjectClass *oc = object_get_class(machine);
-    MachineClass *mc = MACHINE_CLASS(oc);
-
-    mc->has_dynamic_sysbus = true;
-}
-
 int xen_be_register(const char *type, struct XenDevOps *ops)
 {
     char path[50];

@@ -580,8 +571,6 @@ int xen_be_register(const char *type, struct XenDevOps *ops)
 
 void xen_be_register_common(void)
 {
-    xen_set_dynamic_sysbus();
-
     xen_be_register("console", &xen_console_ops);
     xen_be_register("vkbd", &xen_kbdmouse_ops);
     xen_be_register("qdisk", &xen_blkdev_ops);

Quoted 9 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

Quoted 3 lines

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

Quoted 15 lines · Peter Maydell

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 8 lines

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

When writing the patches I couldn't find a way to do it differently. OTOH I'm not so deep in qemu internals I'd be able to add the needed support.

I'd be happy to test any patch, though.

Quoted 25 lines · Juergen Gross

Quoted 15 lines · Peter Maydell

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 8 lines

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

Quoted 5 lines

When writing the patches I couldn't find a way to do it differently. OTOH I'm not so deep in qemu internals I'd be able to add the needed support.

I'd be happy to test any patch, though.

If xen-backend devices are created via QMP commands, then has_dynamic_sysbus is (currently) really needed, although I would have preferred to set it on all x86 machines instead of changing MachineClass::has_dynamic_sysbus outside class_init.

But with the new whitelist implemented by this series, we could simply include xen-backend in the whitelist for the machines that can be used with Xen, and get rid of xen_set_dynamic_sysbus().

I assume plugging/unplugging xen-backend devices apply to both xen{pv,fv} and pc,accel=xen, right? Do we need to make it work with "-machine none,accel=xen" and "-machine isapc" too?

Quoted 41 lines · Eduardo Habkost

Quoted 25 lines · Juergen Gross

Quoted 15 lines · Peter Maydell

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 8 lines

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

Quoted 5 lines

When writing the patches I couldn't find a way to do it differently. OTOH I'm not so deep in qemu internals I'd be able to add the needed support.

I'd be happy to test any patch, though.

If xen-backend devices are created via QMP commands, then has_dynamic_sysbus is (currently) really needed, although I would have preferred to set it on all x86 machines instead of changing MachineClass::has_dynamic_sysbus outside class_init.

But with the new whitelist implemented by this series, we could simply include xen-backend in the whitelist for the machines that can be used with Xen, and get rid of xen_set_dynamic_sysbus().

Quoted 3 lines

I assume plugging/unplugging xen-backend devices apply to both xen{pv,fv} and pc,accel=xen, right? Do we need to make it work with "-machine none,accel=xen" and "-machine isapc" too?

AFAIK -xenpv, -xenfv and -pc,accel=xen are the only machine types to support. Wouldn't it make sense to do the whitelisting in xen_be_register_common() in spite of setting has_dynamic_sysbus?

Quoted 42 lines · Juergen Gross

Quoted 41 lines · Eduardo Habkost

Quoted 25 lines · Juergen Gross

Quoted 15 lines · Peter Maydell

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 8 lines

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

Quoted 5 lines

When writing the patches I couldn't find a way to do it differently. OTOH I'm not so deep in qemu internals I'd be able to add the needed support.

I'd be happy to test any patch, though.

If xen-backend devices are created via QMP commands, then has_dynamic_sysbus is (currently) really needed, although I would have preferred to set it on all x86 machines instead of changing MachineClass::has_dynamic_sysbus outside class_init.

But with the new whitelist implemented by this series, we could simply include xen-backend in the whitelist for the machines that can be used with Xen, and get rid of xen_set_dynamic_sysbus().

Quoted 7 lines

Quoted 3 lines

I assume plugging/unplugging xen-backend devices apply to both xen{pv,fv} and pc,accel=xen, right? Do we need to make it work with "-machine none,accel=xen" and "-machine isapc" too?

AFAIK -xenpv, -xenfv and -pc,accel=xen are the only machine types to support. Wouldn't it make sense to do the whitelisting in xen_be_register_common() in spite of setting has_dynamic_sysbus?

It would, but that would mean we would make the whitelisting mechanism more complex: in addition to the static per-machine-class whitelist, we would need a runtime whitelist.

This would make the interface for querying available/supported device types more complex and messier, and I would like to avoid that.

Quoted 14 lines · Juergen Gross

Quoted 12 lines · Eduardo Habkost

The xen-backend devices created by the Xen code are not supposed to be treated as dynamic sysbus devices. This is an attempt to change that and see what happens, but I couldn't test it because I don't have a Xen host set up.

If this patch breaks anything, this means we have a bug in foreach_dynamic_sysbus_device(), which is supposed to return only devices created using -device.

The original code that sets has_dynamic_sysbus was added by commit 3a6c9172ac5951e6dac2b3f6cbce3cfccdec5894, but I don't see any comment explaining why it was necessary.

Quoted 3 lines

xen-backend devices are created via qmp commands when attaching new pv-devices to a domain. They can be dynamically removed, too. Setting has_dynamic_sysbus was necessary to support this feature.

This seems like it ought to be handled by marking the xen-backend devices as being ok-to-dynamically-create somehow, not by marking the machine as supporting dynamic-sysbus (which it doesn't). Maybe we don't have the necessary support code to do that though?

Summary
-------

This series replaces the existing has_dynamic_sysbus flag (that makes the machine accept every single sysbus device type on the command-line) with a short whitelist.

This will be helpful when implementing the new query-device-slots command, because each machine type will include only the sysbus devices it really supports, instead of including a catch-all TYPE_SYS_BUS_DEVICE "slot".

Most of the machines already have an internal whitelist implemented using foreach_dynamic_sysbus_device(), so we just encode the existing behavior inside the new MachineClass field.

Caveat: q35
-----------

The only problematic case is q35, that accepts all sysbus device types. In this case, instead of adding TYPE_SYS_BUS_DEVICE to the whitelist, I'm adding the existing list of existing sysbus device types to keep compatibility. After that, we can gradually and carefully remove unnecessary entries from the q35 whitelist.

Issue: xen_set_dynamic_sysbus() hack
------------------------------------

We still have an obstacle to get around: the callers of qdev_set_id() outside qdev_device_add() are breaking foreach_dynamic_sysbus_device(), and xen_set_dynamic_sysbus() is a workaround for that bug. We now need to fix that bug to be able to remove the xen_set_dynamic_sysbus() hack. This means the first patch of this series will probably break Xen, until we fix that bug.

Eduardo Habkost (4):
[UNTESTED] xen: Don't force has_dynamic_sysbus on machine class
machine: Replace has_dynamic_sysbus with a whitelist
q35: Remove ioapic devices from sysbus whitelist
q35: Remove fw_cfg* from sysbus whitelist

 include/hw/arm/sysbus-fdt.h |  7 +++++++
 include/hw/boards.h         |  3 ++-
 hw/arm/sysbus-fdt.c         | 10 ++++++++++
 hw/arm/virt.c               |  4 +++-
 hw/core/machine.c           | 43 +++++++++++++++++++++++++++++--------------
 hw/i386/pc_q35.c            | 25 ++++++++++++++++++++++++-
 hw/ppc/e500plat.c           |  4 +++-
 hw/ppc/spapr.c              |  2 +-
 hw/xen/xen_backend.c        | 11 -----------
 9 files changed, 79 insertions(+), 30 deletions(-)