[Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist

[Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist

[Eduardo Habkost]

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

-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

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

Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/xen/xen_backend.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 6c21c37d68..4607d6d3ee 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -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);
-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 2/4] machine: Replace has_dynamic_sysbus with a whitelist

[Eduardo Habkost]

Replace the existing has_dynamic_sysbus flag, that makes the
machine accept every single sysbus device type on the
command-line, with a short whitelist.

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.

The only problematic case is q35, that accepts all sysbus device
types. In this case, instad of adding TYPE_SYS_BUS_DEVICE to the
whitelist, add 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.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 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            | 29 ++++++++++++++++++++++++++++-
 hw/ppc/e500plat.c           |  4 +++-
 hw/ppc/spapr.c              |  2 +-
 8 files changed, 83 insertions(+), 19 deletions(-)

diff --git a/include/hw/arm/sysbus-fdt.h b/include/hw/arm/sysbus-fdt.h
index e15bb81807..071124e13d 100644
--- a/include/hw/arm/sysbus-fdt.h
+++ b/include/hw/arm/sysbus-fdt.h
@@ -57,4 +57,11 @@ typedef struct {
  */
 void arm_register_platform_bus_fdt_creator(ARMPlatformBusFDTParams *fdt_params);
 
+/**
+ * arm_register_fdt_sysbus_whitelist - register supported sysbus device types
+ *
+ * Add supported dynamic sysbus device types to machine class.
+ */
+void arm_register_fdt_sysbus_whitelist(MachineClass *mc);
+
 #endif
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 269d0ba399..6576f6a5b0 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -42,6 +42,7 @@ bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
 void machine_register_compat_props(MachineState *machine);
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
+void machine_class_add_sysbus_whitelist(MachineClass *mc, const char *type);
 
 /**
  * CPUArchId:
@@ -121,7 +122,6 @@ struct MachineClass {
         no_floppy:1,
         no_cdrom:1,
         no_sdcard:1,
-        has_dynamic_sysbus:1,
         pci_allow_0_address:1,
         legacy_fw_cfg_order:1;
     int is_default;
@@ -135,6 +135,7 @@ struct MachineClass {
     bool rom_file_has_mr;
     int minimum_page_bits;
     bool has_hotpluggable_cpus;
+    strList *dynamic_sysbus_whitelist;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dcdbd..29573a4412 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -29,6 +29,7 @@
 #include <linux/vfio.h>
 #endif
 #include "hw/arm/sysbus-fdt.h"
+#include "hw/boards.h"
 #include "qemu/error-report.h"
 #include "sysemu/device_tree.h"
 #include "hw/platform-bus.h"
@@ -424,6 +425,15 @@ static const NodeCreationPair add_fdt_node_functions[] = {
     {"", NULL}, /* last element */
 };
 
+void arm_register_fdt_sysbus_whitelist(MachineClass *mc)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(add_fdt_node_functions); i++) {
+        machine_class_add_sysbus_whitelist(mc, add_fdt_node_functions[i].typename);
+    }
+}
+
 /* Generic Code */
 
 /**
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 5f62a0321e..293710a6ea 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -52,6 +52,8 @@
 #include "hw/arm/fdt.h"
 #include "hw/intc/arm_gic.h"
 #include "hw/intc/arm_gicv3_common.h"
+#include "hw/vfio/vfio-calxeda-xgmac.h"
+#include "hw/vfio/vfio-amd-xgbe.h"
 #include "kvm_arm.h"
 #include "hw/smbios/smbios.h"
 #include "qapi/visitor.h"
@@ -1528,7 +1530,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
      * configuration of the particular instance.
      */
     mc->max_cpus = 255;
-    mc->has_dynamic_sysbus = true;
+    arm_register_fdt_sysbus_whitelist(mc);
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->pci_allow_0_address = true;
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0d92672203..606edffc38 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -332,29 +332,44 @@ static bool machine_get_enforce_config_section(Object *obj, Error **errp)
     return ms->enforce_config_section;
 }
 
-static void error_on_sysbus_device(SysBusDevice *sbdev, void *opaque)
+void machine_class_add_sysbus_whitelist(MachineClass *mc, const char *type)
 {
-    error_report("Option '-device %s' cannot be handled by this machine",
-                 object_class_get_name(object_get_class(OBJECT(sbdev))));
-    exit(1);
+    strList *item = g_new0(strList, 1);
+
+    item->value = g_strdup(type);
+    item->next = mc->dynamic_sysbus_whitelist;
+    mc->dynamic_sysbus_whitelist = item;
 }
 
-static void machine_init_notify(Notifier *notifier, void *data)
+static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
 {
-    Object *machine = qdev_get_machine();
-    ObjectClass *oc = object_get_class(machine);
-    MachineClass *mc = MACHINE_CLASS(oc);
+    MachineState *machine = opaque;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    bool whitelisted = false;
+    strList *wl;
 
-    if (mc->has_dynamic_sysbus) {
-        /* Our machine can handle dynamic sysbus devices, we're all good */
-        return;
+    for (wl = mc->dynamic_sysbus_whitelist;
+         !whitelisted && wl;
+         wl = wl->next) {
+        whitelisted |= !!object_dynamic_cast(OBJECT(sbdev), wl->value);
     }
 
+    if (!whitelisted) {
+        error_report("Option '-device %s' cannot be handled by this machine",
+                     object_class_get_name(object_get_class(OBJECT(sbdev))));
+        exit(1);
+    }
+}
+
+static void machine_init_notify(Notifier *notifier, void *data)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+
     /*
-     * Loop through all dynamically created devices and check whether there
-     * are sysbus devices among them. If there are, error out.
+     * Loop through all dynamically created sysbus devices and check if they are
+     * all whitelisted. If there are non-whitelisted devices, error out.
      */
-    foreach_dynamic_sysbus_device(error_on_sysbus_device, NULL);
+    foreach_dynamic_sysbus_device(validate_sysbus_device, machine);
 }
 
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index dd792a8547..ea039540d8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -297,7 +297,34 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->no_floppy = 1;
-    m->has_dynamic_sysbus = true;
+    /* The following whitelist is based on the set of available
+     * sysbus devices when has_dynamic_sysbus was replaced with
+     * a whitelist. Not every single device in this list is really
+     * supposed to be supported, but they are kept here for compatibility.
+     * We can gradually remove unnecessary devices from the whitelist.
+     */
+    machine_class_add_sysbus_whitelist(m, "allwinner-ahci");
+    machine_class_add_sysbus_whitelist(m, "amd-iommu");
+    machine_class_add_sysbus_whitelist(m, "cfi.pflash01");
+    machine_class_add_sysbus_whitelist(m, "esp");
+    machine_class_add_sysbus_whitelist(m, "fw_cfg_io");
+    machine_class_add_sysbus_whitelist(m, "fw_cfg_mem");
+    machine_class_add_sysbus_whitelist(m, "generic-sdhci");
+    machine_class_add_sysbus_whitelist(m, "hpet");
+    machine_class_add_sysbus_whitelist(m, "intel-iommu");
+    machine_class_add_sysbus_whitelist(m, "ioapic");
+    machine_class_add_sysbus_whitelist(m, "isabus-bridge");
+    machine_class_add_sysbus_whitelist(m, "kvm-ioapic");
+    machine_class_add_sysbus_whitelist(m, "kvmclock");
+    machine_class_add_sysbus_whitelist(m, "kvmvapic");
+    machine_class_add_sysbus_whitelist(m, "SUNW,fdtwo");
+    machine_class_add_sysbus_whitelist(m, "sysbus-ahci");
+    machine_class_add_sysbus_whitelist(m, "sysbus-fdc");
+    machine_class_add_sysbus_whitelist(m, "sysbus-ohci");
+    machine_class_add_sysbus_whitelist(m, "unimplemented-device");
+    machine_class_add_sysbus_whitelist(m, "virtio-mmio");
+    machine_class_add_sysbus_whitelist(m, "xen-backend");
+    machine_class_add_sysbus_whitelist(m, "xen-sysdev");
     m->max_cpus = 288;
 }
 
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 94b454551f..3abed05add 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -12,7 +12,9 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "e500.h"
+#include "hw/net/fsl_etsec/etsec.h"
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 #include "sysemu/device_tree.h"
 #include "sysemu/kvm.h"
 #include "hw/pci/pci.h"
@@ -63,7 +65,7 @@ static void e500plat_machine_init(MachineClass *mc)
     mc->desc = "generic paravirt e500 platform";
     mc->init = e500plat_init;
     mc->max_cpus = 32;
-    mc->has_dynamic_sysbus = true;
+    machine_class_add_sysbus_whitelist(mc, TYPE_ETSEC_COMMON);
 }
 
 DEFINE_MACHINE("ppce500", e500plat_machine_init)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6ee566d658..db56d0a653 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3070,7 +3070,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = "";
     mc->default_ram_size = 512 * M_BYTE;
     mc->kvm_type = spapr_kvm_type;
-    mc->has_dynamic_sysbus = true;
+    machine_class_add_sysbus_whitelist(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
     mc->pci_allow_0_address = true;
     mc->get_hotplug_handler = spapr_get_hotplug_handler;
     hc->pre_plug = spapr_machine_device_pre_plug;
-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 3/4] q35: Remove ioapic devices from sysbus whitelist

[Eduardo Habkost]

An ioapic device is always created by the q35 machine, so
"-device ioapic" and "-device kvm-ioapic" never worked before.
Remove it from the sysbus whitelist.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_q35.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ea039540d8..94a3069e16 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -312,9 +312,7 @@ static void pc_q35_machine_options(MachineClass *m)
     machine_class_add_sysbus_whitelist(m, "generic-sdhci");
     machine_class_add_sysbus_whitelist(m, "hpet");
     machine_class_add_sysbus_whitelist(m, "intel-iommu");
-    machine_class_add_sysbus_whitelist(m, "ioapic");
     machine_class_add_sysbus_whitelist(m, "isabus-bridge");
-    machine_class_add_sysbus_whitelist(m, "kvm-ioapic");
     machine_class_add_sysbus_whitelist(m, "kvmclock");
     machine_class_add_sysbus_whitelist(m, "kvmvapic");
     machine_class_add_sysbus_whitelist(m, "SUNW,fdtwo");
-- 
2.11.0.259.g40922b1

[Qemu-devel] [RFC 4/4] q35: Remove fw_cfg* from sysbus whitelist

[Eduardo Habkost]

fw_cfg devices are supposed to be created by the q35 board code,
and not manually using -device. Remove them from the whitelist.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_q35.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 94a3069e16..91d7f67e48 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -307,8 +307,6 @@ static void pc_q35_machine_options(MachineClass *m)
     machine_class_add_sysbus_whitelist(m, "amd-iommu");
     machine_class_add_sysbus_whitelist(m, "cfi.pflash01");
     machine_class_add_sysbus_whitelist(m, "esp");
-    machine_class_add_sysbus_whitelist(m, "fw_cfg_io");
-    machine_class_add_sysbus_whitelist(m, "fw_cfg_mem");
     machine_class_add_sysbus_whitelist(m, "generic-sdhci");
     machine_class_add_sysbus_whitelist(m, "hpet");
     machine_class_add_sysbus_whitelist(m, "intel-iommu");
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Juergen Gross]

On 23/03/17 22:28, Eduardo Habkost wrote:
> 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.

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.

So just removing it will break Xen.

NAK as a standalone patch.


Juergen

> 
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/xen/xen_backend.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index 6c21c37d68..4607d6d3ee 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -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);
> 

Re: [Qemu-devel] [RFC 0/4] Replace has_dynamic_sysbus with device type whitelist

[Juergen Gross]

On 23/03/17 22:28, Eduardo Habkost wrote:
> 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(-)
> 

This seems to break Xen. I got:

qemu-system-i386: Option '-device xen-backend' cannot be handled by this
machine

when using qemu as Xen backend driver.


Juergen

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Peter Maydell]

On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> On 23/03/17 22:28, Eduardo Habkost wrote:
>> 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.
>
> 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?

thanks
-- PMM

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Juergen Gross]

On 24/03/17 11:09, Peter Maydell wrote:
> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>> 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.
>>
>> 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.


Juergen

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Thomas Huth]

On 24.03.2017 11:09, Peter Maydell wrote:
> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>> 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.
>>
>> 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?

Do the xen devices have to be sysbus devices? If no, I think you could
change them to be of type TYPE_DEVICE nowadays - TYPE_DEVICE can be
instantiated with "-device" nowadays, too. That's what we do with the
spapr-rng device for example (see hw/ppc/spapr_rng.c).

 Thomas

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Eduardo Habkost]

On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
> On 24/03/17 11:09, Peter Maydell wrote:
> > On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> >> On 23/03/17 22:28, Eduardo Habkost wrote:
> >>> 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.
> >>
> >> 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.

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?

-- 
Eduardo

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Juergen Gross]

On 24/03/17 12:10, Eduardo Habkost wrote:
> On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
>> On 24/03/17 11:09, Peter Maydell wrote:
>>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>>>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>>>> 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.
>>>>
>>>> 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.
> 
> 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?

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?


Juergen

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Eduardo Habkost]

On Fri, Mar 24, 2017 at 01:27:31PM +0100, Juergen Gross wrote:
> On 24/03/17 12:10, Eduardo Habkost wrote:
> > On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
> >> On 24/03/17 11:09, Peter Maydell wrote:
> >>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
> >>>> On 23/03/17 22:28, Eduardo Habkost wrote:
> >>>>> 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.
> >>>>
> >>>> 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.
> > 
> > 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?
> 
> 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.

-- 
Eduardo

Re: [Qemu-devel] [RFC 1/4] [UNTESTED] xen: Don't force has_dynamic_sysbus on machine class

[Juergen Gross]

On 24/03/17 14:50, Eduardo Habkost wrote:
> On Fri, Mar 24, 2017 at 01:27:31PM +0100, Juergen Gross wrote:
>> On 24/03/17 12:10, Eduardo Habkost wrote:
>>> On Fri, Mar 24, 2017 at 11:24:31AM +0100, Juergen Gross wrote:
>>>> On 24/03/17 11:09, Peter Maydell wrote:
>>>>> On 24 March 2017 at 08:23, Juergen Gross <jgross@suse.com> wrote:
>>>>>> On 23/03/17 22:28, Eduardo Habkost wrote:
>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>
>>> 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?
>>
>> 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.
> 

Okay, understood. And I suppose you don't want to add the Xen
devices to the per-machine-class whitelist (after all my patch
did the same with the has_dynamic_sysbus flag) on demand.

Either way is fine with me.


Juergen