[Qemu-devel] [PATCH RESEND v2 00/21] qdev/sysbus: Set user_creatable=false by default on sysbus

Eduardo Habkost posted 21 patches 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170503203604.31462-1-ehabkost@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
include/hw/qdev-core.h              | 10 +++++-----
include/hw/qdev-properties.h        |  4 ++--
hw/acpi/piix4.c                     |  2 +-
hw/arm/spitz.c                      |  2 +-
hw/audio/marvell_88w8618.c          |  2 +-
hw/audio/pcspk.c                    |  2 +-
hw/core/or-irq.c                    |  2 +-
hw/core/qdev.c                      |  1 +
hw/core/register.c                  |  2 +-
hw/core/sysbus.c                    | 11 +++++++++++
hw/dma/i8257.c                      |  2 +-
hw/dma/sparc32_dma.c                |  2 +-
hw/gpio/omap_gpio.c                 |  4 ++--
hw/i2c/omap_i2c.c                   |  2 +-
hw/i2c/smbus_eeprom.c               |  2 +-
hw/i2c/smbus_ich9.c                 |  2 +-
hw/i386/amd_iommu.c                 |  2 ++
hw/i386/intel_iommu.c               |  2 ++
hw/i386/pc.c                        |  2 +-
hw/input/vmmouse.c                  |  2 +-
hw/intc/apic_common.c               |  2 +-
hw/intc/etraxfs_pic.c               |  2 +-
hw/intc/grlib_irqmp.c               |  2 +-
hw/intc/i8259_common.c              |  2 +-
hw/intc/nios2_iic.c                 |  2 +-
hw/intc/omap_intc.c                 |  4 ++--
hw/isa/lpc_ich9.c                   |  2 +-
hw/isa/piix4.c                      |  2 +-
hw/isa/vt82c686.c                   |  2 +-
hw/mips/gt64xxx_pci.c               |  2 +-
hw/misc/vmport.c                    |  2 +-
hw/net/dp8393x.c                    |  2 +-
hw/net/etraxfs_eth.c                |  2 +-
hw/net/fsl_etsec/etsec.c            |  2 ++
hw/net/lance.c                      |  2 +-
hw/pci-bridge/dec.c                 |  2 +-
hw/pci-bridge/pci_expander_bridge.c |  2 +-
hw/pci-host/apb.c                   |  2 +-
hw/pci-host/bonito.c                |  2 +-
hw/pci-host/gpex.c                  |  2 +-
hw/pci-host/grackle.c               |  2 +-
hw/pci-host/piix.c                  |  6 +++---
hw/pci-host/ppce500.c               |  2 +-
hw/pci-host/prep.c                  |  2 +-
hw/pci-host/q35.c                   |  4 ++--
hw/pci-host/uninorth.c              |  8 ++++----
hw/pci-host/versatile.c             |  2 +-
hw/pci-host/xilinx-pcie.c           |  2 +-
hw/ppc/ppc4xx_pci.c                 |  2 +-
hw/ppc/spapr_drc.c                  |  2 +-
hw/ppc/spapr_pci.c                  |  2 ++
hw/s390x/s390-pci-bus.c             |  1 -
hw/sd/milkymist-memcard.c           |  2 +-
hw/sd/pl181.c                       |  2 +-
hw/sh4/sh_pci.c                     |  2 +-
hw/timer/i8254_common.c             |  2 +-
hw/timer/mc146818rtc.c              |  2 +-
hw/vfio/amd-xgbe.c                  |  2 ++
hw/vfio/calxeda-xgmac.c             |  2 ++
hw/xen/xen_backend.c                |  2 ++
monitor.c                           |  2 +-
qdev-monitor.c                      |  6 +++---
qom/cpu.c                           |  2 +-
target/i386/cpu.c                   |  2 +-
64 files changed, 95 insertions(+), 70 deletions(-)
[Qemu-devel] [PATCH RESEND v2 00/21] qdev/sysbus: Set user_creatable=false by default on sysbus
Posted by Eduardo Habkost 6 years, 11 months ago
No code changes since v2, just a rebase to latest qemu.git
master, a trivial commit message fix at patch 1, and added
Acked-by and Reviewed-by tags.

Changes v1 -> v2
----------------

* Rewrote series name and cover letter completely to not pretend
  we're fixing the q35 lack-of-sysbus-whitelist bug, and explain
  the motivation for the series.
  * Previous series name was:
    "sysbus: Don't allow -device/device_add by default"
  * Rewrote description of patch 02/21, too
  * (I really hope people read this cover letter before
    commenting on individual patches.)
* Rewrote FIXME comments to make it clear that we just set
  user_creatable=true temporarily because we don't know yet if
  the device should be in the q35 whitelist.
* Set user_creatable=true on xen-backend also
  (I didn't notice it was missing because I was building QEMU
  without xen support)
  * New patches:
    * "xen-backend: Remove FIXME comment about user_creatable flag"
    * "xen-sysdev: Remove user_creatable flag"
* Patch:
    "s390: Add FIXME for unexplained user_creatable=false line"
  replaced with:
    "s390-pcibus: No need to set user_creatable=false explicitly"

Description
-----------

This series refactor the cannot_instantiate_with_device_add code
for sysbus. First, the cannot_instantiate_with_device_add field
is replaced by !user_creatable.

Then, we change TYPE_SYS_BUS_DEVICE to set user_creatable=false
by default, and we set user_creatable=true explicitly only on the
devices that are really supposed to be user-creatable on some
machines.

Motivation
----------

First of all, this makes the code less fragile: setting
user_creatable=false or cannot_instantiate_with_device_add=true
on all sysbus devices is incorrect, and makes code that looks at
cannot_instantiate_with_device_add/user_creatable easy to break.

This also fixes a regression introduced by commit
33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31, that makes all sysbus
devices appear on "-device help" and lack the "no-user" flag on
"info qdm"[1].

This will also make it possible for automatic test code (like the
device-crash-test.py script I sent a while ago[2]) skip devices
that are not supposed to be user-creatable on any machine.

A note about the lack of sysbus whitelist on q35
------------------------------------------------

This series won't make the per-machine whitelist of sysbus
devices unnecessary, but just makes the user_creatable field
consistent on the sys-bus-device classes. This means q35 and xen
still need to be fixed to implemented a sysbus device whitelist.

However, despite not being strictly necessary for fixing the q35
bug, reducing the list of user_creatable=true devices will help
us be more confident when building the q35 whitelist.

Full list of user_creatable=true sysbus devices
-----------------------------------------------

In the end of this series, the only remaining sysbus devices with
user_creatable=true will be:

* vfio-amd-xgbe (arm)
* vfio-calxeda-xgmac (arm)
* amd-iommu (x86)
* intel-iommu (x86)
* xen-backend (x86)
* spapr-pci-host-bridge (ppc)
* spapr-pci-vfio-host-bridge (ppc)
* eTSEC (ppc)

References/Notes
----------------

[1] For example, before this series, we had 174 sysbus devices
    listed on qemu-system-arm -device help:
      $ qemu-system-arm -machine none -device help 2>&1 | grep 'bus System' | wc -l
      174
      $
    after this series, we now have:
      $ ./arm-softmmu/qemu-system-arm -machine none -device help 2>&1 | grep 'bus System'
      name "vfio-amd-xgbe", bus System, desc "VFIO AMD XGBE"
      name "vfio-calxeda-xgmac", bus System, desc "VFIO Calxeda XGMAC"
      $

[2] Subject: [PATCH 0/3] script for crash-testing -device

---
Cc: Alexander Graf <agraf@suse.de>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marcel Apfelbaum <marcel@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>

Eduardo Habkost (21):
  qdev: Replace cannot_instantiate_with_device_add_yet with
    !user_creatable
  sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE
  xen-backend: Remove FIXME comment about user_creatable flag
  iommu: Remove FIXME comment about user_creatable=true
  fdc: Remove user_creatable flag from sysbus-fdc & SUNW,fdtwo
  pflash_cfi01: Remove user_creatable flag
  kvmclock: Remove user_creatable flag
  ioapic: Remove user_creatable flag
  kvmvapic: Remove user_creatable flag
  sysbus-ahci: Remove user_creatable flag
  allwinner-ahci: Remove user_creatable flag
  isabus-bridge: Remove user_creatable flag
  unimplemented-device: Remove user_creatable flag
  fw_cfg: Remove user_creatable flag
  esp: Remove user_creatable flag
  generic-sdhci: Remove user_creatable flag
  hpet: Remove user_creatable flag
  sysbus-ohci: Remove user_creatable flag
  virtio-mmio: Remove user_creatable flag
  xen-sysdev: Remove user_creatable flag
  s390-pcibus: No need to set user_creatable=false explicitly

 include/hw/qdev-core.h              | 10 +++++-----
 include/hw/qdev-properties.h        |  4 ++--
 hw/acpi/piix4.c                     |  2 +-
 hw/arm/spitz.c                      |  2 +-
 hw/audio/marvell_88w8618.c          |  2 +-
 hw/audio/pcspk.c                    |  2 +-
 hw/core/or-irq.c                    |  2 +-
 hw/core/qdev.c                      |  1 +
 hw/core/register.c                  |  2 +-
 hw/core/sysbus.c                    | 11 +++++++++++
 hw/dma/i8257.c                      |  2 +-
 hw/dma/sparc32_dma.c                |  2 +-
 hw/gpio/omap_gpio.c                 |  4 ++--
 hw/i2c/omap_i2c.c                   |  2 +-
 hw/i2c/smbus_eeprom.c               |  2 +-
 hw/i2c/smbus_ich9.c                 |  2 +-
 hw/i386/amd_iommu.c                 |  2 ++
 hw/i386/intel_iommu.c               |  2 ++
 hw/i386/pc.c                        |  2 +-
 hw/input/vmmouse.c                  |  2 +-
 hw/intc/apic_common.c               |  2 +-
 hw/intc/etraxfs_pic.c               |  2 +-
 hw/intc/grlib_irqmp.c               |  2 +-
 hw/intc/i8259_common.c              |  2 +-
 hw/intc/nios2_iic.c                 |  2 +-
 hw/intc/omap_intc.c                 |  4 ++--
 hw/isa/lpc_ich9.c                   |  2 +-
 hw/isa/piix4.c                      |  2 +-
 hw/isa/vt82c686.c                   |  2 +-
 hw/mips/gt64xxx_pci.c               |  2 +-
 hw/misc/vmport.c                    |  2 +-
 hw/net/dp8393x.c                    |  2 +-
 hw/net/etraxfs_eth.c                |  2 +-
 hw/net/fsl_etsec/etsec.c            |  2 ++
 hw/net/lance.c                      |  2 +-
 hw/pci-bridge/dec.c                 |  2 +-
 hw/pci-bridge/pci_expander_bridge.c |  2 +-
 hw/pci-host/apb.c                   |  2 +-
 hw/pci-host/bonito.c                |  2 +-
 hw/pci-host/gpex.c                  |  2 +-
 hw/pci-host/grackle.c               |  2 +-
 hw/pci-host/piix.c                  |  6 +++---
 hw/pci-host/ppce500.c               |  2 +-
 hw/pci-host/prep.c                  |  2 +-
 hw/pci-host/q35.c                   |  4 ++--
 hw/pci-host/uninorth.c              |  8 ++++----
 hw/pci-host/versatile.c             |  2 +-
 hw/pci-host/xilinx-pcie.c           |  2 +-
 hw/ppc/ppc4xx_pci.c                 |  2 +-
 hw/ppc/spapr_drc.c                  |  2 +-
 hw/ppc/spapr_pci.c                  |  2 ++
 hw/s390x/s390-pci-bus.c             |  1 -
 hw/sd/milkymist-memcard.c           |  2 +-
 hw/sd/pl181.c                       |  2 +-
 hw/sh4/sh_pci.c                     |  2 +-
 hw/timer/i8254_common.c             |  2 +-
 hw/timer/mc146818rtc.c              |  2 +-
 hw/vfio/amd-xgbe.c                  |  2 ++
 hw/vfio/calxeda-xgmac.c             |  2 ++
 hw/xen/xen_backend.c                |  2 ++
 monitor.c                           |  2 +-
 qdev-monitor.c                      |  6 +++---
 qom/cpu.c                           |  2 +-
 target/i386/cpu.c                   |  2 +-
 64 files changed, 95 insertions(+), 70 deletions(-)

-- 
2.11.0.259.g40922b1


Re: [Qemu-devel] [PATCH RESEND v2 00/21] qdev/sysbus: Set user_creatable=false by default on sysbus
Posted by Eduardo Habkost 6 years, 11 months ago
Ping? If there are no objections to this series, I plan to merge
it through the Machine Core tree.

If anybody is interested, below are the results of squashing
patches 2-20 together:

---
 hw/core/sysbus.c         | 11 +++++++++++
 hw/i386/amd_iommu.c      |  2 ++
 hw/i386/intel_iommu.c    |  2 ++
 hw/net/fsl_etsec/etsec.c |  2 ++
 hw/ppc/spapr_pci.c       |  2 ++
 hw/vfio/amd-xgbe.c       |  2 ++
 hw/vfio/calxeda-xgmac.c  |  2 ++
 hw/xen/xen_backend.c     |  2 ++
 8 files changed, 25 insertions(+)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index c0f560b289..5d0887f499 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -326,6 +326,17 @@ static void sysbus_device_class_init(ObjectClass *klass, void *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = sysbus_device_init;
     k->bus_type = TYPE_SYSTEM_BUS;
+    /*
+     * device_add plugs devices into a suitable bus.  For "real" buses,
+     * that actually connects the device.  For sysbus, the connections
+     * need to be made separately, and device_add can't do that.  The
+     * device would be left unconnected, and will probably not work
+     *
+     * However, a few machines can handle device_add/-device with
+     * a few specific sysbus devices. In those cases, the device
+     * subclass needs to override it and set user_creatable=true.
+     */
+    k->user_creatable = false;
 }
 
 static const TypeInfo sysbus_device_type_info = {
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index f86a40aa30..efcc93cbfd 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1186,6 +1186,8 @@ static void amdvi_class_init(ObjectClass *klass, void* data)
     dc->vmsd = &vmstate_amdvi;
     dc->hotpluggable = false;
     dc_class->realize = amdvi_realize;
+    /* Supported by the pc-q35-* machine types */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo amdvi = {
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 02f047c8e3..327a46cd19 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3009,6 +3009,8 @@ static void vtd_class_init(ObjectClass *klass, void *data)
     dc->hotpluggable = false;
     x86_class->realize = vtd_realize;
     x86_class->int_remap = vtd_int_remap;
+    /* Supported by the pc-q35-* machine types */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo vtd_info = {
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index aa2b0d5a85..9da1932970 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -416,6 +416,8 @@ static void etsec_class_init(ObjectClass *klass, void *data)
     dc->realize = etsec_realize;
     dc->reset = etsec_reset;
     dc->props = etsec_properties;
+    /* Supported by ppce500 machine */
+    dc->user_creatable = true;
 }
 
 static TypeInfo etsec_info = {
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index e7567e2e8f..a7cff32bbf 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1994,6 +1994,8 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->props = spapr_phb_properties;
     dc->reset = spapr_phb_reset;
     dc->vmsd = &vmstate_spapr_pci;
+    /* Supported by TYPE_SPAPR_MACHINE */
+    dc->user_creatable = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hp->plug = spapr_phb_hot_plug_child;
     hp->unplug = spapr_phb_hot_unplug_child;
diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c
index 2c60310cf9..fab196cebf 100644
--- a/hw/vfio/amd-xgbe.c
+++ b/hw/vfio/amd-xgbe.c
@@ -38,6 +38,8 @@ static void vfio_amd_xgbe_class_init(ObjectClass *klass, void *data)
     dc->realize = amd_xgbe_realize;
     dc->desc = "VFIO AMD XGBE";
     dc->vmsd = &vfio_platform_amd_xgbe_vmstate;
+    /* Supported by TYPE_VIRT_MACHINE */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo vfio_amd_xgbe_dev_info = {
diff --git a/hw/vfio/calxeda-xgmac.c b/hw/vfio/calxeda-xgmac.c
index bb15d588e5..7bb17af7ad 100644
--- a/hw/vfio/calxeda-xgmac.c
+++ b/hw/vfio/calxeda-xgmac.c
@@ -38,6 +38,8 @@ static void vfio_calxeda_xgmac_class_init(ObjectClass *klass, void *data)
     dc->realize = calxeda_xgmac_realize;
     dc->desc = "VFIO Calxeda XGMAC";
     dc->vmsd = &vfio_platform_calxeda_xgmac_vmstate;
+    /* Supported by TYPE_VIRT_MACHINE */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo vfio_calxeda_xgmac_dev_info = {
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index c85f1637e4..f29b2b027b 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -619,6 +619,8 @@ static void xendev_class_init(ObjectClass *klass, void *data)
 
     dc->props = xendev_properties;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    /* xen-backend devices can be plugged/unplugged dynamically */
+    dc->user_creatable = true;
 }
 
 static const TypeInfo xendev_type_info = {
-- 
2.11.0.259.g40922b1

-- 
Eduardo