Re: [Qemu-devel] q35 and sysbus devices

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Mar 24, 2017 at 05:58:43PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Fri, Mar 24, 2017 at 01:49:16PM +0300, Marcel Apfelbaum wrote:
> >> On 03/22/2017 10:46 PM, Laszlo Ersek wrote:
> >> > On 03/22/17 21:31, Eduardo Habkost wrote:
> >> > > Hi,
> >> > > 
> >> > > I am investigating the current status of has_dynamic_sysbus and
> >> > > sysbus device support on each of QEMU's machine types. The good
> >> > > news is that almost all has_dynamic_sysbus=1 machines have their
> >> > > own internal (often short) whitelist of supported sysbus device
> >> > > types, and automatically reject unsupported devices.
> >> > > 
> >> > > ...except for q35.
> >> > > 
> >> > > q35 currently accepts all sys-bus-device subtypes on "-device",
> >> > > and today this includes the following 23 devices:
> >> > > 
> >> > > * 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
> >> > > * xen-backend
> >> > > * xen-sysdev
> >> > > 
> >> > > My question is: do all those devices really make sense to be used
> >> > > with "-device" on q35?
> >> > 
> >> > I think fw_cfg_io and fw_cfg_mem should be board-only devices (no
> >> > -device switch).
> >> > 
> >> > Regarding cfi.pflash01, I think originally it would have been nice to
> >> > specify pflash chips with the modern (non-legacy) syntax, that is,
> >> > separate -drive if=none,file=... backend options combined with -device
> >> > cfi.pflash01,drive=... frontend options. However, that ship has sailed,
> >> > even libvirt uses -drive if=pflash for these, and given the purpose we
> >> > use pflash chips for, on Q35, I don't see much benefit in exposing
> >> > cfi.pflash01 with a naked -device *now*.
> >> > 
> >> > Re: virtio-mmio, I don't think that should be available on Q35 at all.
> >> > 
> >> > I can't comment on the rest.
> >> > 
> >> 
> >> Hi Eduardo,
> >> Thanks for finding these problems.
> >> 
> >> We should ping all maintainers of the above devices, the best way to do it
> >> is to add the "cannot_instantiate_with_device_add_yet = true" and ask maintainers
> >> to agree (or not) on that.
> >
> > If I understand it correctly,
> > cannot_instantiate_with_device_add_yet is supposed to be
> > temporary.
> 
> A couple of years ago, we had a -device regression: devices marked
> no_user were no longer rejected.  To get my fix for that in, I had to
> rename it to cannot_instantiate_with_device_add_yet and add some solemn
> protestations that it's temporary.  It's been temporary ever since.

Interesting story. I will look for it in the qemu-devel archives.

> 
> Without doubt getting rid of it would be nice.  But I'm not holding my
> breath.

This sounds like a good demonstration that
cannot_instantiate_with_device_add_yet is not going to be
temporary. I suggest renaming it back to "no_user", or
"user_creatable", and living with the fact that the ability to
create a device using -device or device_add needs to be reported
by our APIs, instead of pretending otherwise.

> 
> >            And it applies to all machines, with no exceptions.
> 
> Correct.
> 
> > The problem with today's mechanism is that we have no way to make
> > a machine accept one type of sysbus device without making it
> > start accepting every other sysbus devices. If we thought all
> > !cannot_instantiate_with_device_add_yet sysbus devices were
> > already safe, we wouldn't have has_dynamic_sysbus in the first
> > place, would we?
> 
> In my relatively ignorant opinion, "dynamic sysbus" has to die even
> harder than "sysbus".
> 
> "Sysbus" isn't a bus.  In qdev's original design, every device had to
> plug into a bus, period.  The ones that really didn't were made to plug
> into "sysbus".
> 
> Pretty much the only thing "sysbus" devices had in common was that they
> couldn't be used with device_add and device_del.
> 
> We fixed the design to permit bus-less devices, but we didn't get rid of
> "sysbus".
> 
> We got a "platform bus", which is really not the same as "sysbus", but
> we shoehorned it into "sysbus" anyway.
> 
> The result is a mess, and you're sitting right in it.
> 
> One hack we could perhaps pile on top of the others: have sysbus devices
> again set cannot_instantiate_with_device_add_yet, then unset it for the
> ones in the whitelist.

This makes a lot of sense, to me. Maybe it would make the
existing per-machine-type whitelists unnecessary.

(And while doing that, let's rename
cannot_instantiate_with_device_add_yet to no_user or
user_creatable, and stop lying to ourselves.)

-- 
Eduardo