On 04.04.2017 08:53, 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 <ehabk...@redhat.com> 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? 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. > > So the real bug is that someone set has_dynamic_sysbus=1 in q35 without > implementing all of the dynamic sysbus logic, no?
According to commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f this was just introduced for allowing the "intel-iommu" device, so I guess this is the device that we want to have in a whitelist there? Thomas
