| Message ID | 20170401004624.30886-4-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@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. thanks -- PMM
On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote: > On 1 April 2017 at 01:46, Eduardo Habkost <ehabkost@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.
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 <ehabkost@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. Alex
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 <ehabkost@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). [1] Well, it was a regression on the "info qdm" HMP command output, which is a minor bug. But it's still a bug we still want to fix anyway.
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 <ehabkost@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? Alex
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 <ehabkost@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
On 04.04.17 08:58, Thomas Huth wrote: > 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 <ehabkost@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? If you want a dynamic intel-iommu, you also need to have dynamic ACPI generation to create DMAR tables the OS can use to drive the IOMMU, no? At last at that point, someone should have realized that a "whitelist" is mandatory. But yes, please just only add a whitelist for intel-iommu to the q35 board. That is the real bug. The basic idea behind dynamic sysbus is that you move the responsibility of sysbus spawnability checks from the sysbus layer to the board file. If the board file ignores to do those checks, it's at fault. Alex
On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote: > > > On 04.04.17 08:58, Thomas Huth wrote: > > 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 <ehabkost@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? > > If you want a dynamic intel-iommu, you also need to have dynamic ACPI > generation to create DMAR tables the OS can use to drive the IOMMU, no? At > last at that point, someone should have realized that a "whitelist" is > mandatory. > > But yes, please just only add a whitelist for intel-iommu to the q35 board. > That is the real bug. Look, I don't disagree that we need a whitelist on q35. But I don't know why you assume it is as simple as adding intel-iommu. We *don't know* what should be on q35 whitelist until we review each device that is accepted by q35 today, and make sure it really is not supposed to be supported on -device. Making has_dynamic_sysbus/user_creatable consistent on sysbus devices helps on that. It is not necessary nor sufficient to fix q35, that's true, but it helps *a lot*. See, after I start this series, I already found two exceptions to the "just add intel-iommu" rule: 1) amd-iommu 2) xen-backend I'm not sure yet if we have others, until people review the other "Remove user_creatable from <device>" patches in this series. > The basic idea behind dynamic sysbus is that you move the responsibility of > sysbus spawnability checks from the sysbus layer to the board file. If the > board file ignores to do those checks, it's at fault. I think I will do this: I will submit v2 of this thread pretending it is just going to fix the "info qdm" regression introduced by commit bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention of the q35 bug from the series and patch description. I hopt this will make us stop diverging the discussion to "you should add a whitelist to q35 first", and stop ignoring that: 1) cannot_instantiate_with_device_add_yet is being set incorrectly on the sysbus device classes and that's a bad idea; 2) cannot_instantiate_with_device_add_yet is an awful name.
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 <ehabkost@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? 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.
On 04/04/2017 02:59 PM, Eduardo Habkost wrote: > On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote: >> >> On 04.04.17 08:58, Thomas Huth wrote: >>> 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 <ehabkost@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? >> If you want a dynamic intel-iommu, you also need to have dynamic ACPI >> generation to create DMAR tables the OS can use to drive the IOMMU, no? At >> last at that point, someone should have realized that a "whitelist" is >> mandatory. >> >> But yes, please just only add a whitelist for intel-iommu to the q35 board. >> That is the real bug. > Look, I don't disagree that we need a whitelist on q35. But I > don't know why you assume it is as simple as adding intel-iommu. So why has intel-iommu worked in the first place? Who wired up its memory regions? Who wired up fault IRQs? Who added the DMAR table to ACPI when it was created? > We *don't know* what should be on q35 whitelist until we review > each device that is accepted by q35 today, and make sure it > really is not supposed to be supported on -device. If in doubt, not a single sysbus device should have ever worked without explicit code around it. Really :). > Making has_dynamic_sysbus/user_creatable consistent on sysbus > devices helps on that. It is not necessary nor sufficient to fix > q35, that's true, but it helps *a lot*. > > See, after I start this series, I already found two exceptions to > the "just add intel-iommu" rule: > > 1) amd-iommu > 2) xen-backend > > I'm not sure yet if we have others, until people review the other > "Remove user_creatable from <device>" patches in this > series. Same question as above there. How do they get mapped? How does the OS learn they exist? > >> The basic idea behind dynamic sysbus is that you move the responsibility of >> sysbus spawnability checks from the sysbus layer to the board file. If the >> board file ignores to do those checks, it's at fault. > I think I will do this: > > I will submit v2 of this thread pretending it is just going to > fix the "info qdm" regression introduced by commit > bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention > of the q35 bug from the series and patch description. > > I hopt this will make us stop diverging the discussion to "you > should add a whitelist to q35 first", and stop ignoring that: > 1) cannot_instantiate_with_device_add_yet is being set > incorrectly on the sysbus device classes and that's a bad > idea; > 2) cannot_instantiate_with_device_add_yet is an awful name. We need to make sure that the board has control over which devices are spawnable. I fail to see how this series helps or achieves that, but I'm happy to learn. Alex
On Tue, Apr 04, 2017 at 03:06:30PM +0200, Alexander Graf wrote: > On 04/04/2017 02:59 PM, Eduardo Habkost wrote: > > On Tue, Apr 04, 2017 at 09:02:28AM +0200, Alexander Graf wrote: > > > > > > On 04.04.17 08:58, Thomas Huth wrote: > > > > 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 <ehabkost@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? > > > If you want a dynamic intel-iommu, you also need to have dynamic ACPI > > > generation to create DMAR tables the OS can use to drive the IOMMU, no? At > > > last at that point, someone should have realized that a "whitelist" is > > > mandatory. > > > > > > But yes, please just only add a whitelist for intel-iommu to the q35 board. > > > That is the real bug. > > Look, I don't disagree that we need a whitelist on q35. But I > > don't know why you assume it is as simple as adding intel-iommu. > > So why has intel-iommu worked in the first place? Most of the code required to make it work is simply triggered by its realize() method (that only works with TYPE_PC_MACHINE machines). > Who wired up its memory regions? Its realize() method (vtd_realize()). > Who wired up fault IRQs? I don't know the answer for that, but probably it is at the apic_get_class()->send_msi() call at vtd_generate_interrupt(). > Who added the DMAR table to ACPI when it was created? acpi-build.c code calls calls x86_iommu_get_default(), which returns the value set by x86_iommu_set_default(), which is called by TYPE_X86_IOMMU_DEVICE's realize() method (x86_iommu_realize()). > > > We *don't know* what should be on q35 whitelist until we review > > each device that is accepted by q35 today, and make sure it > > really is not supposed to be supported on -device. > > If in doubt, not a single sysbus device should have ever worked without > explicit code around it. Really :). If by "code around it", you mean something as clear as a foreach_dynamic_sysbus_device() call somewhere that handles each type of sysbus device, then that's not true in practice. If by "code around it" you mean code buried inside board-specific code (like the x86_iommu_get_default() calls inside hw/i386/acpi-build.c), then you're right. (Don't ask me why it works that way, I didn't write that code, I am just trying to clean up the q35 mess. :) ) > > > Making has_dynamic_sysbus/user_creatable consistent on sysbus > > devices helps on that. It is not necessary nor sufficient to fix > > q35, that's true, but it helps *a lot*. > > > > See, after I start this series, I already found two exceptions to > > the "just add intel-iommu" rule: > > > > 1) amd-iommu > > 2) xen-backend > > > > I'm not sure yet if we have others, until people review the other > > "Remove user_creatable from <device>" patches in this > > series. > > Same question as above there. How do they get mapped? How does the OS learn > they exist? In the case of *-iommu, the realize() method seems to trigger everything we need. In the case of xen-backend, I guess the Xen magic is triggered by the xen_backend.c code. Some cases may be more subtle, like kvmclock: kvmclock _could_ work using -device already, because it simply provides a few MSRs to the guest. The only thing that let me conclude that -device kvmclock is not useful is that it doesn't set the kvmclock CPUID bits in the VCPU. If kvmclock added the CPUID bits from its realize() method, -device kvmclock would already working out of the box and I wouldn't be able to remove it from the whitelist. > > > > > The basic idea behind dynamic sysbus is that you move the responsibility of > > > sysbus spawnability checks from the sysbus layer to the board file. If the > > > board file ignores to do those checks, it's at fault. > > I think I will do this: > > > > I will submit v2 of this thread pretending it is just going to > > fix the "info qdm" regression introduced by commit > > bf8d492405feaee2c1685b3b9d5e03228ed3e47f, and remove any mention > > of the q35 bug from the series and patch description. > > > > I hopt this will make us stop diverging the discussion to "you > > should add a whitelist to q35 first", and stop ignoring that: > > 1) cannot_instantiate_with_device_add_yet is being set > > incorrectly on the sysbus device classes and that's a bad > > idea; > > 2) cannot_instantiate_with_device_add_yet is an awful name. > > > We need to make sure that the board has control over which devices are > spawnable. I fail to see how this series helps or achieves that, but I'm > happy to learn. The series doesn't help the board have control over which devices are spawnable. In addition to fixing (1) and (2) above, the series helps us (humans) reduce the list of candidates for the q35 whitelist we need to build.
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index a328693d15..a06c8e358c 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -2880,6 +2880,11 @@ static void sysbus_fdc_class_init(ObjectClass *klass, void *data) dc->props = sysbus_fdc_properties; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo sysbus_fdc_info = { @@ -2906,6 +2911,11 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void *data) dc->props = sun4m_fdc_properties; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo sun4m_fdc_info = { diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index 594d4cf6fe..f48dc20035 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -927,6 +927,11 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data) dc->props = pflash_cfi01_properties; dc->vmsd = &vmstate_pflash; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index c0f560b289..2d4121dd67 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 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 and a few devices can handle a few sysbus + * devices, if the device subclass overrides user_creatable=true and + * the machine has has_dynamic_sysbus=1. + */ + 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..1d139f3f6a 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1186,6 +1186,11 @@ static void amdvi_class_init(ObjectClass *klass, void* data) dc->vmsd = &vmstate_amdvi; dc->hotpluggable = false; dc_class->realize = amdvi_realize; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo amdvi = { diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 22d8226e43..5e5125f34b 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2601,6 +2601,11 @@ static void vtd_class_init(ObjectClass *klass, void *data) dc->hotpluggable = false; x86_class->realize = vtd_realize; x86_class->int_remap = vtd_int_remap; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo vtd_info = { diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c index 13eca374cd..0dbf083b17 100644 --- a/hw/i386/kvm/clock.c +++ b/hw/i386/kvm/clock.c @@ -286,6 +286,11 @@ static void kvmclock_class_init(ObjectClass *klass, void *data) dc->realize = kvmclock_realize; dc->vmsd = &kvmclock_vmsd; dc->props = kvmclock_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo kvmclock_info = { diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c index 98ca480792..cdc35de661 100644 --- a/hw/i386/kvm/ioapic.c +++ b/hw/i386/kvm/ioapic.c @@ -167,6 +167,11 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data) k->post_load = kvm_ioapic_put; dc->reset = kvm_ioapic_reset; dc->props = kvm_ioapic_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo kvm_ioapic_info = { diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c index 82a49556af..fbada19253 100644 --- a/hw/i386/kvmvapic.c +++ b/hw/i386/kvmvapic.c @@ -856,6 +856,11 @@ static void vapic_class_init(ObjectClass *klass, void *data) dc->reset = vapic_reset; dc->vmsd = &vmstate_vapic; dc->realize = vapic_realize; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo vapic_type = { diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index f60826d6e0..b1b7780d56 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1721,6 +1721,11 @@ static void sysbus_ahci_class_init(ObjectClass *klass, void *data) dc->props = sysbus_ahci_properties; dc->reset = sysbus_ahci_reset; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo sysbus_ahci_info = { @@ -1815,6 +1820,11 @@ static void allwinner_ahci_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); dc->vmsd = &vmstate_allwinner_ahci; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo allwinner_ahci_info = { diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index 37c4386ae3..38af393538 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -448,6 +448,11 @@ static void ioapic_class_init(ObjectClass *klass, void *data) k->post_load = ioapic_update_kvm_routes; dc->reset = ioapic_reset_common; dc->props = ioapic_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo ioapic_info = { diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c index 348e0eab9d..576dbf1763 100644 --- a/hw/isa/isa-bus.c +++ b/hw/isa/isa-bus.c @@ -221,6 +221,11 @@ static void isabus_bridge_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->fw_name = "isa"; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo isabus_bridge_info = { diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c index bcbb585888..284f096beb 100644 --- a/hw/misc/unimp.c +++ b/hw/misc/unimp.c @@ -90,6 +90,11 @@ static void unimp_class_init(ObjectClass *klass, void *data) dc->realize = unimp_realize; dc->props = unimp_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo unimp_info = { diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c index aa2b0d5a85..b230b88753 100644 --- a/hw/net/fsl_etsec/etsec.c +++ b/hw/net/fsl_etsec/etsec.c @@ -416,6 +416,7 @@ static void etsec_class_init(ObjectClass *klass, void *data) dc->realize = etsec_realize; dc->reset = etsec_reset; dc->props = etsec_properties; + dc->user_creatable = true; } static TypeInfo etsec_info = { diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 316fca9bc1..60bf4fdd2e 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -1101,6 +1101,11 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data) dc->realize = fw_cfg_io_realize; dc->props = fw_cfg_io_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo fw_cfg_io_info = { @@ -1167,6 +1172,11 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data) dc->realize = fw_cfg_mem_realize; dc->props = fw_cfg_mem_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo fw_cfg_mem_info = { diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 98c52e411f..360eeb2e40 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1990,6 +1990,7 @@ 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; + 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/scsi/esp.c b/hw/scsi/esp.c index eee831efeb..1087b8f14f 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -728,6 +728,11 @@ static void sysbus_esp_class_init(ObjectClass *klass, void *data) dc->reset = sysbus_esp_hard_reset; dc->vmsd = &vmstate_sysbus_esp_scsi; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo sysbus_esp_info = { diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 6d6a791ee9..186555188b 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1360,6 +1360,11 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data) dc->props = sdhci_sysbus_properties; dc->realize = sdhci_sysbus_realize; dc->reset = sdhci_poweron_reset; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo sdhci_sysbus_info = { diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index a2c18b30c3..b5dab1c8e2 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -771,6 +771,11 @@ static void hpet_device_class_init(ObjectClass *klass, void *data) dc->reset = hpet_reset; dc->vmsd = &vmstate_hpet; dc->props = hpet_device_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo hpet_device_info = { diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 3ada35e954..0e17fb1e60 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -2159,6 +2159,11 @@ static void ohci_sysbus_class_init(ObjectClass *klass, void *data) dc->desc = "OHCI USB Controller"; dc->props = ohci_sysbus_properties; dc->reset = usb_ohci_reset_sysbus; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo ohci_sysbus_info = { diff --git a/hw/vfio/amd-xgbe.c b/hw/vfio/amd-xgbe.c index 2c60310cf9..f0b6624530 100644 --- a/hw/vfio/amd-xgbe.c +++ b/hw/vfio/amd-xgbe.c @@ -38,6 +38,7 @@ 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; + 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..6fae3c5772 100644 --- a/hw/vfio/calxeda-xgmac.c +++ b/hw/vfio/calxeda-xgmac.c @@ -38,6 +38,7 @@ 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; + dc->user_creatable = true; } static const TypeInfo vfio_calxeda_xgmac_dev_info = { diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index 5807aa87fe..b595512a3d 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -450,6 +450,11 @@ static void virtio_mmio_class_init(ObjectClass *klass, void *data) dc->reset = virtio_mmio_reset; set_bit(DEVICE_CATEGORY_MISC, dc->categories); dc->props = virtio_mmio_properties; + /* + * FIXME: Set only for compatibility on q35 machine-type. + * Probably never meant to be user-creatable + */ + dc->user_creatable = true; } static const TypeInfo virtio_mmio_info = {
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. This patch adds user_creatable=true explicitly to the 24 device types mentioned above, to keep compatibility, and set user_creatable=false on TYPE_SYS_BUS_DEVICE. Subsequent patches will remove user_creatable=true from the devices that were not meant to be creatable using -device despite being currently accepted by q35. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Alexander Graf <agraf@suse.de> Cc: John Snow <jsnow@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Scott Wood <scottwood@freescale.com> Cc: Jason Wang <jasowang@redhat.com> Cc: David Gibson <david@gibson.dropbear.id.au> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: qemu-block@nongnu.org Cc: qemu-ppc@nongnu.org Cc: Alistair Francis <alistair.francis@xilinx.com> Cc: Beniamino Galvani <b.galvani@gmail.com> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com> Cc: Gabriel L. Somlo <somlo@cmu.edu> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Prasad J Pandit <pjp@fedoraproject.org> Cc: qemu-arm@nongnu.org Cc: Shannon Zhao <zhaoshenglong@huawei.com> Cc: Thomas Huth <thuth@redhat.com> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/block/fdc.c | 10 ++++++++++ hw/block/pflash_cfi01.c | 5 +++++ hw/core/sysbus.c | 11 +++++++++++ hw/i386/amd_iommu.c | 5 +++++ hw/i386/intel_iommu.c | 5 +++++ hw/i386/kvm/clock.c | 5 +++++ hw/i386/kvm/ioapic.c | 5 +++++ hw/i386/kvmvapic.c | 5 +++++ hw/ide/ahci.c | 10 ++++++++++ hw/intc/ioapic.c | 5 +++++ hw/isa/isa-bus.c | 5 +++++ hw/misc/unimp.c | 5 +++++ hw/net/fsl_etsec/etsec.c | 1 + hw/nvram/fw_cfg.c | 10 ++++++++++ hw/ppc/spapr_pci.c | 1 + hw/scsi/esp.c | 5 +++++ hw/sd/sdhci.c | 5 +++++ hw/timer/hpet.c | 5 +++++ hw/usb/hcd-ohci.c | 5 +++++ hw/vfio/amd-xgbe.c | 1 + hw/vfio/calxeda-xgmac.c | 1 + hw/virtio/virtio-mmio.c | 5 +++++ 22 files changed, 115 insertions(+)