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

[Eduardo Habkost]

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

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

[Alexander Graf]

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

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

[Thomas Huth]

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

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

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Eduardo Habkost]

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

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Apr 03, 2017 at 08:49:16PM +0100, Peter Maydell wrote:
> On 1 April 2017 at 01:46, Eduardo Habkost  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.

-- 
Eduardo

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

[Peter Maydell]

On 1 April 2017 at 01:46, Eduardo Habkost  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

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

[Eduardo Habkost]

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 
Cc: Alexander Graf 
Cc: John Snow 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Paolo Bonzini 
Cc: Richard Henderson 
Cc: Eduardo Habkost 
Cc: "Michael S. Tsirkin" 
Cc: Scott Wood 
Cc: Jason Wang 
Cc: David Gibson 
Cc: Gerd Hoffmann 
Cc: Alex Williamson 
Cc: qemu-block@nongnu.org
Cc: qemu-...@nongnu.org
Cc: Alistair Francis 
Cc: Beniamino Galvani 
Cc: "Edgar E. Iglesias" 
Cc: Gabriel L. Somlo 
Cc: Igor Mammedov 
Cc: Laszlo Ersek 
Cc: Marcel Apfelbaum 
Cc: Prasad J Pandit 
Cc: qemu-...@nongnu.org
Cc: Shannon Zhao 
Cc: Thomas Huth 
Signed-off-by: Eduardo Habkost 
---
 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(+)

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 =