Re: [Qemu-devel] [RFC 03/19] sysbus: Set user_creatable=false by default on TYPE_SYS_BUS_DEVICE

[Eduardo Habkost]

On Tue, Apr 04, 2017 at 08:53:43AM +0200, Alexander Graf wrote:
> 
> 
> On 03.04.17 23:00, Eduardo Habkost wrote:
> > On Mon, Apr 03, 2017 at 10:15:44PM +0200, Alexander Graf wrote:
> > > 
> > > 
> > > On 03.04.17 22:10, Eduardo Habkost wrote:
> > > > On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> > > > > On 1 April 2017 at 01:46, Eduardo Habkost <address@hidden> wrote:
> > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS, making
> > > > > > all kinds of untested devices available to -device and
> > > > > > device_add.
> > > > > > 
> > > > > > The problem with that is: setting has_dynamic_sysbus on a
> > > > > > machine-type lets it accept all the 288 sysbus device types we
> > > > > > have in QEMU, and most of them were never meant to be used with
> > > > > > -device. That's a lot of untested code.
> > > > > > 
> > > > > > Fortunately today we have just a few has_dynamic_sysbus=1
> > > > > > machines: virt, pc-q35-*, ppce500, and spapr.
> > > > > > 
> > > > > > virt, ppce500, and spapr have extra checks to ensure just a few
> > > > > > device types can be instantiated:
> > > > > > 
> > > > > > * virt supports only TYPE_VFIO_CALXEDA_XGMAC, TYPE_VFIO_AMD_XGBE.
> > > > > > * ppce500 supports only TYPE_ETSEC_COMMON.
> > > > > > * spapr supports only TYPE_SPAPR_PCI_HOST_BRIDGE.
> > > > > > 
> > > > > > q35 has no code to block unsupported sysbus devices, however, and
> > > > > > accepts all device types. Fortunately, only the following 20
> > > > > > device types are compiled into the qemu-system-x86_64 and
> > > > > > qemu-system-i386 binaries:
> > > > > > 
> > > > > > * allwinner-ahci
> > > > > > * amd-iommu
> > > > > > * cfi.pflash01
> > > > > > * esp
> > > > > > * fw_cfg_io
> > > > > > * fw_cfg_mem
> > > > > > * generic-sdhci
> > > > > > * hpet
> > > > > > * intel-iommu
> > > > > > * ioapic
> > > > > > * isabus-bridge
> > > > > > * kvmclock
> > > > > > * kvm-ioapic
> > > > > > * kvmvapic
> > > > > > * SUNW,fdtwo
> > > > > > * sysbus-ahci
> > > > > > * sysbus-fdc
> > > > > > * sysbus-ohci
> > > > > > * unimplemented-device
> > > > > > * virtio-mmio
> > > > > > 
> > > > > > Instead of requiring each machine-type with has_dynamic_sysbus=1
> > > > > > to implement its own mechanism to block unsupported devices, we
> > > > > > can use the user_creatable flag to ensure we won't let the user
> > > > > > plug anything that will never work.
> > > > > 
> > > > > How does this work? Which devices can be dynamically
> > > > > plugged is machine dependent. You can't dynamically-plug
> > > > > an intel-iommu on the ARM virt board, and you can't
> > > > > dynamically-plug the vfio-calxeda-xgmac on the spapr
> > > > > board, and so on. So I don't see how we can just have
> > > > > a flag on the device itself that controls whether
> > > > > it can be dynamically plugged.
> > > > > 
> > > > > So I'm definitely coming around to the opinion that
> > > > > it's just a bug in the q35 board that it doesn't have
> > > > > any device whitelisting, and we should fix that.
> > > > 
> > > > OK, let's assume q35 must implement a whitelist:
> > > > 
> > > > To build that whitelist, we need to be able to know what should
> > > > be in the whitelist, or not. And nobody knew for sure what was
> > > > user-creatable in q35 by accident, and what was really supposed
> > > > to be user-creatable in q35. See the "q35 and sysbus devices"
> > > > thread I started ~2 weeks ago.
> > > > 
> > > > Building a q35 whitelist will be much easier if make
> > > > sys-bus-devices non-user-creatable by default.
> > > 
> > > So why are they user creatable in the first place?
> > > 
> > > We used to have boards that were dynamic sysbus aware (ppce500, virt) that
> > > allowed dynamic creation and every other board did not. I don't remember 
> > > the
> > > exact mechanism behind it though.
> > > 
> > > When did that behavior change? It sounds like a regression somewhere.
> > 
> > See patch description:
> > 
> > > > > > commit 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 unset
> > > > > > cannot_instantiate_with_device_add_yet in TYPE_SYSBUS,
> > 
> > Note that the commit above is not a regression[1] (because q35
> > didn't have has_dynamic_sysbus=1 yet), but having sysbus devices
> > have cannot_instantiate_with_device_add_yet=false/user_creatable=true
> > by default makes it harder to build the whitelist for q35 (or
> > other machines that will have has_dynamic_sysbus=1 in the
> > future).
> 
> I seem to miss the bigger picture.
> 
> Why would anyone set has_dynamic_sysbus=1 in a board file without an
> explicit whitelist?

I guess it was because this was not documented anywhere. Note
that q35 is not the only case today: see
xen_set_dynamic_sysbus().

>                     The whitelist is *not* device specific. It's board
> specific, because your board needs to know how to wire up a device and how
> to expose the fact that it exists to the OS.

That part is true.

> 
> So the real bug is that someone set has_dynamic_sysbus=1 in q35 without
> implementing all of the dynamic sysbus logic, no?

This part I disagree: we don't have "a real bug", we have two
different bugs that we are mixing them up:

1) q35 and the xen_set_dynamic_sysbus() hack have no whitelist.
2) cannot_instantiate_with_device_add_yet is not set correctly on
   sysbus device classes (breaking "info qdm", and maybe other
   code that depends on cannot_instantiate_with_device_add_yet).

It's my fault that we're mixing them up, because I am using bug
#1 as justification for the #2 fix (this seris). But fixing #2 is
not necessary for fixing #1, it just helps a lot.

-- 
Eduardo