Re: [Qemu-devel] [RFC 13/19] unimplemented-device: Remove user_creatable flag

[Eduardo Habkost]

On Tue, Apr 04, 2017 at 09:12:59AM +0200, Alexander Graf wrote:
> 
> 
> > Am 04.04.2017 um 09:05 schrieb Thomas Huth <address@hidden>:
> > 
> >> On 03.04.2017 22:05, Eduardo Habkost wrote:
> >>> On Mon, Apr 03, 2017 at 08:42:12PM +0100, Peter Maydell wrote:
> >>>> On 3 April 2017 at 19:30, Eduardo Habkost <address@hidden> wrote:
> >>>>> On Mon, Apr 03, 2017 at 03:08:06PM +0100, Peter Maydell wrote:
> >>>>>> On 3 April 2017 at 14:54, Eduardo Habkost <address@hidden> wrote:
> >>>>>> This, on the other hand, currently works:
> >>>>>>  $ qemu-system-x86_64 -M q35 -device 
> >>>>>> unimplemented-device,size=1024,name=foo
> >>>>> 
> >>>>> That's a bug in the q35 machine or the handling of -device
> >>>>> on non-platform-bus systems, though, isn't it? The default
> >>>>> for all sysbus devices has always been "you can't use
> >>>>> -device with this", [...]
> >>>> 
> >>>> This was true until 2014, only. commit
> >>>> 33cd52b5d7b9adfd009e95f07e6c64dd88ae2a31 changed sys-bus-device
> >>>> to have cannot_instantiate_with_device_add_yet=false. See patch
> >>>> 03/19 for more detailed info.
> >>> 
> >>> That commit shouldn't cause this problem -- unless the
> >>> machine sets has_dynamic_sysbus then the machine_init_notify()
> >>> that commit adds will cause it to complain.
> >>> 
> >>> I think the bug was introduced much later, in bf8d4924 in 2016,
> >>> when q35 had the has_dynamic_sysbus flag added but without
> >>> any code to restruct this to a whitelist of working
> >>> devices. (In fact you can see in that commit a very
> >>> minimal attempt to blacklist a few devices.)
> >> 
> >> It's true that the problem happened when q35 set
> >> has_dynamic_sysbus=1 without a whitelist. The point of this
> >> series is to make a per-machine-type whitelist unnecessary.
> >> 
> >>> 
> >>> The commit message says only intel-iommu was supposed to
> >>> be -device creatable, so it should have been the only
> >>> thing on the whitelist.
> >> 
> >> There was a thread about it (Subject: "q35 and sysbus devices"),
> >> and nobody knew for sure if the q35 whitelist was supposed to
> >> have *-iommu only, or not.
> >> 
> >> The conclusion of that thread is that we can simply use
> >> cannot_instantiate_with_device_add_yet to build the whitelist, if
> >> the field was set consistently. This series renames
> >> cannot_instantiate_with_device_add_yet to user_creatable, and
> >> sets it consistently.
> > 
> > I don't think that this will always work. While you can clearly mark
> > those devices that can never be added dynamically, there might also be
> > some devices that only work on certain machines. For example, there
> > might be devices that only work on the "ppce500" machine, but do not
> > work on the "pseries" machine. So we always need some kind of
> > whitelisting for the machines that have a dynamic sysbus.
> 
> It's even worse. Sysbus needs explicit logic to wire up memory
> regions and irqs. That logic is board specific (or at least was
> last time I checked).
> 
> So your board *needs* explicit logic for every sysbus device it
> spawns. And that's why you also get a board whitelist.

You're correct. My suggestion is that we (humans, not the machine
code) can build the whitelist inside q35 code by looking at the
remaining user-creatable devices (after this series is reviewed).

If I suggested that fixing cannot_instantiate_with_device_add_yet
is sufficient to fix the q35 bug, that was a mistake.

-- 
Eduardo