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

[Eduardo Habkost]

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.

> Commit 9e3f9733 shows how this
> should be done (that's where spapr got has_dynamic_sysbus).
> 
> Maybe we should just fix the q35 bug first?

This fixes the q35 bug by setting user_creatable correctly on
sys-bus-devices, and makes a q35-specific whitelist unnecessary.

> 
> >>               [...] there's never been a requirement to
> >> mark them as such separately (because it would require
> >> touching the files for a huge number of devices for no
> >> particularly good reason when you can default the whole
> >> set of devices subclassing SysBusDevice).
> >
> > Yes, and this is the whole point of this series. At the end of
> > this series, only the few devices that are actually usable with
> > some machine will have an explicit user_creatable=true
> > assignment. See cover letter for details.
> 
> ...OK, but in that case why not just set that where it should
> be set, rather than having a big long patchset that touches
> a lot of devices that in the end are right back where
> they started? I think a lot of why I'm confused by
> this patchset is that it seems like it's changing
> behaviour on devices like this one which don't need
> any changes...

The devices don't get back right where they started: they start
with cannot_instantiate_with_device_add_yet=false (meaning they
are user-creatable in q35), and end up with user_creatable=false
(meaning they become non-user-creatable in all machines).

I could squash patches 04/19 to 19/19 into patch 03/19, it's
true. But then I would probably not get confirmation from you
(and other developers) that unimplemented-device (and other
devices) is really not supposed to be user-creatable. I wouldn't
know if I am breaking a valid q35 use case, or not.

-- 
Eduardo