[Qemu-devel] q35 and sysbus devices

[Qemu-devel] q35 and sysbus devices

[Eduardo Habkost]

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? Should we make q35 validate dynamic sysbus
devices against a whitelist, like the other has_dynamic_sysbus
machines?

I'm asking this because I will resume work on the
"query-device-slots" command, which will report supported sysbus
devices too. And I don't want the new command to report any
devices that it shouldn't.

-- 
Eduardo

Re: [Qemu-devel] q35 and sysbus devices

[Laszlo Ersek]

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.

Thanks
Laszlo

> Should we make q35 validate dynamic sysbus
> devices against a whitelist, like the other has_dynamic_sysbus
> machines?
> 
> I'm asking this because I will resume work on the
> "query-device-slots" command, which will report supported sysbus
> devices too. And I don't want the new command to report any
> devices that it shouldn't.
> 

Re: [Qemu-devel] q35 and sysbus devices

[Marcel Apfelbaum]

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.

Thanks,
Marcel

> Thanks
> Laszlo
>
>> Should we make q35 validate dynamic sysbus
>> devices against a whitelist, like the other has_dynamic_sysbus
>> machines?
>>
>> I'm asking this because I will resume work on the
>> "query-device-slots" command, which will report supported sysbus
>> devices too. And I don't want the new command to report any
>> devices that it shouldn't.
>>
>

Re: [Qemu-devel] q35 and sysbus devices

[Thomas Huth]

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

I think that SUNW,fdtwo device only makes sense on the sun4m machine, so
the code should likely be extracted from fdc.c into a separate file,
which should then only be compiled if CONFIG_SUN4M is set.

 Thomas

> * 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? Should we make q35 validate dynamic sysbus
> devices against a whitelist, like the other has_dynamic_sysbus
> machines?
> 
> I'm asking this because I will resume work on the
> "query-device-slots" command, which will report supported sysbus
> devices too. And I don't want the new command to report any
> devices that it shouldn't.
> 

Re: [Qemu-devel] q35 and sysbus devices

[Eduardo Habkost]

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. And it applies to all machines, with no exceptions.

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?

-- 
Eduardo

Re: [Qemu-devel] q35 and sysbus devices

[Peter Maydell]

On 24 March 2017 at 13:48, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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?

Almost all sysbus devices are not dynamically instantiable
and never will be because they need to map mmio and wire
up IRQs and so on, and we have no mechanism for doing that.
So the default position is "no, you can't do that (but maybe
a subclass which knows better might override)". has_dynamic_sysbus
is a dodgy hack for boards which support the "platform bus"
which is a special case where the user can use -device
for certain sysbus devices that the platform bus knows about
and can wire up itself (in a range of irqs and memory that
the board has set aside for that purpose). The default
is still "no, you can't dynamically instantiate this", but
instead of this (and the handful of exceptions) being imposed
at compile time it's imposed (with exceptions) at runtime.

So for instance the ARM 'virt' board sets has_dynamic_sysbus,
which allows a few devices to be added with -device, but
most still do not permit it (they'll end up hitting the
error_report() in hw/arm/sysbus-fdt.c:add_fdt_node()).

thanks
-- PMM

Re: [Qemu-devel] q35 and sysbus devices

[Markus Armbruster]

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.

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

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

Re: [Qemu-devel] q35 and sysbus devices

[Peter Maydell]

On 24 March 2017 at 16:58, Markus Armbruster <armbru@redhat.com> wrote:
> "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.

This isn't really true. Sysbus devices support having MMIO regions
and IRQ lines and GPIO lines. If you need those you're a
sysbus device; otherwise you can probably just be a plain old Device.

> We fixed the design to permit bus-less devices, but we didn't get rid of
> "sysbus".

Call it what you want, but we should have some common code support
for "I want to have MMIOs and IRQs and GPIO lines". You could
argue for moving all that into Device I suppose.

> We got a "platform bus", which is really not the same as "sysbus", but
> we shoehorned it into "sysbus" anyway.

I agree 'platform bus' is a mess, and I'd rather it didn't exist.
Unfortunately people really really want to be able to do device
pass through of random memory-mapped devices :-(

thanks
-- PMM

Re: [Qemu-devel] q35 and sysbus devices

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 24 March 2017 at 16:58, Markus Armbruster <armbru@redhat.com> wrote:
>> "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.
>
> This isn't really true. Sysbus devices support having MMIO regions
> and IRQ lines and GPIO lines. If you need those you're a
> sysbus device; otherwise you can probably just be a plain old Device.

Well, "device has MMIO regions, IRQ lines and GPIO lines" is about as
"device contains virtual silicon".  What would a device without any of
these *do*?

Devices plugging into a bus have to expose their MMIO regions, IRQ
lines, etc. in a certain way dictated by the bus.  In return, you don't
have to wire up each resource manually, you simply plug into the bus and
are done.  That's what makes a bus a bus for me.

"Sysbus" does nothing of the sort.

>> We fixed the design to permit bus-less devices, but we didn't get rid of
>> "sysbus".
>
> Call it what you want, but we should have some common code support
> for "I want to have MMIOs and IRQs and GPIO lines".

Of course.

>                                                     You could
> argue for moving all that into Device I suppose.

Yup.

>> We got a "platform bus", which is really not the same as "sysbus", but
>> we shoehorned it into "sysbus" anyway.
>
> I agree 'platform bus' is a mess, and I'd rather it didn't exist.
> Unfortunately people really really want to be able to do device
> pass through of random memory-mapped devices :-(

The "platform" bus adds certain constraints over "sysbus", precisely to
make these uses possible.  And that's precisely why it should be its own
thing instead of complicating "sysbus".

Re: [Qemu-devel] q35 and sysbus devices

[Peter Maydell]

On 24 March 2017 at 17:59, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> This isn't really true. Sysbus devices support having MMIO regions
>> and IRQ lines and GPIO lines. If you need those you're a
>> sysbus device; otherwise you can probably just be a plain old Device.
>
> Well, "device has MMIO regions, IRQ lines and GPIO lines" is about as
> "device contains virtual silicon".  What would a device without any of
> these *do*?

A PCI device doesn't have them, for instance -- it exposes only
a PCI-bus connector.

> Devices plugging into a bus have to expose their MMIO regions, IRQ
> lines, etc. in a certain way dictated by the bus.  In return, you don't
> have to wire up each resource manually, you simply plug into the bus and
> are done.  That's what makes a bus a bus for me.

I think of these as various kinds of connection. A connection
can be a single wire (like a GPIO line or an interrupt line),
or it can be a well-defined bundle of signals (like a PCI bus
slot's multiple connections), or (for emulation) it can be a
more abstracted connection like what we call an MMIO region
(which is an abstraction of a data bus and address bus
set of signals which we don't want to modify at that level).
A PCI device in QEMU is *not* exposing an MMIO region, because
the only correct way to access it is via the PCI bus APIs.
(We just implement our PCI bus in terms of MMIO regions.)

A "bus" is just a well defined set of signals that it's convenient
to manipulate all at once. It isn't necessarily every signal
the device has.

Not all hardware is defined like that -- quite a lot (especially
in the embedded world) is defined with much more ad-hoc
connection specifications because it's going to be "wired up"
as part of an SoC, with some verilog/RTL which says "this GPIO
line goes to here" and so on. In fact even at this level there's
usually some sort of 'bus' abstraction for some of these signals:
what we represent as an individual MMIO region is usually
a connection on an AXI or AHB bus or other architectural equivalent.

The way that we privilege some of these connections above others
(eg that PCI devices all inherit from a particular parent class)
is a bit ugly in an abstract sense (really a device should just
say "I have a PCI connection here" in the same way it can say
"I have a bare GPIO wire here", rather than PCI devices being
subclasses of a common PCI device base class), but it's a
bit simpler in practice. Similarly having a base class that's
providing some useful functionality for devices that deal
mostly in "bare wires" is convenient in practice.

thanks
-- PMM

Re: [Qemu-devel] q35 and sysbus devices

[Eduardo Habkost]

On Fri, Mar 24, 2017 at 02:13:48PM +0000, Peter Maydell wrote:
> On 24 March 2017 at 13:48, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 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?
> 
> Almost all sysbus devices are not dynamically instantiable
> and never will be [...]

Exactly. But most of them do _not_ have
cannot_instantiate_with_device_add_yet set. What I don't know is:
should they?

>              [...] because they need to map mmio and wire
> up IRQs and so on, and we have no mechanism for doing that.
> So the default position is "no, you can't do that (but maybe
> a subclass which knows better might override)". has_dynamic_sysbus
> is a dodgy hack for boards which support the "platform bus"
> which is a special case where the user can use -device
> for certain sysbus devices that the platform bus knows about
> and can wire up itself (in a range of irqs and memory that
> the board has set aside for that purpose). The default
> is still "no, you can't dynamically instantiate this", but
> instead of this (and the handful of exceptions) being imposed
> at compile time it's imposed (with exceptions) at runtime.
> 
> So for instance the ARM 'virt' board sets has_dynamic_sysbus,
> which allows a few devices to be added with -device, but
> most still do not permit it (they'll end up hitting the
> error_report() in hw/arm/sysbus-fdt.c:add_fdt_node()).

I sent a series yesterday that implements a generic mechanism for
boards to set the list of accepted device types, to replace
has_dynamic_sysbus. That part was easy. But I still don't know
what to do about q35.

-- 
Eduardo

Re: [Qemu-devel] q35 and sysbus devices

[Eduardo Habkost]

On Fri, Mar 24, 2017 at 05:08:56PM +0000, Peter Maydell wrote:
> On 24 March 2017 at 16:58, Markus Armbruster <armbru@redhat.com> wrote:
> > "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.
> 
> This isn't really true. Sysbus devices support having MMIO regions
> and IRQ lines and GPIO lines. If you need those you're a
> sysbus device; otherwise you can probably just be a plain old Device.
> 
> > We fixed the design to permit bus-less devices, but we didn't get rid of
> > "sysbus".
> 
> Call it what you want, but we should have some common code support
> for "I want to have MMIOs and IRQs and GPIO lines". You could
> argue for moving all that into Device I suppose.

Even if we don't move all that into Device, this sounds like an
argument for the existence of struct SysBusDevice. But I still
don't understand the reason TYPE_SYSTEM_BUS still exists.

-- 
Eduardo

Re: [Qemu-devel] q35 and sysbus devices

[Thomas Huth]

On 24.03.2017 18:59, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 24 March 2017 at 16:58, Markus Armbruster <armbru@redhat.com> wrote:
>>> "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.
>>
>> This isn't really true. Sysbus devices support having MMIO regions
>> and IRQ lines and GPIO lines. If you need those you're a
>> sysbus device; otherwise you can probably just be a plain old Device.
> 
> Well, "device has MMIO regions, IRQ lines and GPIO lines" is about as
> "device contains virtual silicon".  What would a device without any of
> these *do*?

On ppc64, we've got for a example pseudo-devices that takes care of
certain hypercalls, e.g. the spapr-rng or the spapr-rtc devices (well,
the latter is still a TYPE_SYS_BUS_DEVICE ... we should convert it to
TYPE_DEVICE instead one day...). These devices do not have any MMIO, IRQ
or GPIO lines at all, so I think the differentiation between a very
generic TYPE_DEVICE class and a "SYS_BUS_DEVICE" (with MMIO/IRQ/GPIO)
class still makes sense.

 Thomas

Re: [Qemu-devel] q35 and sysbus devices

[Cornelia Huck]

On Fri, 24 Mar 2017 16:23:18 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Mar 24, 2017 at 05:08:56PM +0000, Peter Maydell wrote:
> > On 24 March 2017 at 16:58, Markus Armbruster <armbru@redhat.com> wrote:
> > > "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.
> > 
> > This isn't really true. Sysbus devices support having MMIO regions
> > and IRQ lines and GPIO lines. If you need those you're a
> > sysbus device; otherwise you can probably just be a plain old Device.
> > 
> > > We fixed the design to permit bus-less devices, but we didn't get rid of
> > > "sysbus".
> > 
> > Call it what you want, but we should have some common code support
> > for "I want to have MMIOs and IRQs and GPIO lines". You could
> > argue for moving all that into Device I suppose.
> 
> Even if we don't move all that into Device, this sounds like an
> argument for the existence of struct SysBusDevice. But I still
> don't understand the reason TYPE_SYSTEM_BUS still exists.

ISTR some surprises that reset (or some other) callbacks were not
called as expected if there wasn't a sysbus device among the ancestors.
Don't know if that's still true.

Re: [Qemu-devel] q35 and sysbus devices

[Peter Maydell]

On 27 March 2017 at 09:44, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> ISTR some surprises that reset (or some other) callbacks were not
> called as expected if there wasn't a sysbus device among the ancestors.
> Don't know if that's still true.

Yes -- if you're a sysbus device then your reset method is
called. If you're only a subclass of Device then your reset
method is not called.

I think this is because buses like PCI have specific
reset behaviour (order important between host and
devices, maybe?), so we defer reset to the subclass.

(Reset is a mess anyway.)

thanks
-- PMM

Re: [Qemu-devel] q35 and sysbus devices

[Eduardo Habkost]

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