Re: [Qemu-devel] vmbus bridge: machine property or device?

[Eduardo Habkost]

On Thu, Apr 13, 2017 at 06:15:34PM +0300, Roman Kagan wrote:
> On Wed, Apr 12, 2017 at 05:07:20PM -0300, Eduardo Habkost wrote:
> > On Wed, Apr 12, 2017 at 05:18:51PM +0200, Markus Armbruster wrote:
> > > Roman Kagan <address@hidden> writes:
> > > > VMBus is provided by a vmbus bridge; it appears the most natural to have
> > > > it subclassed from SysBusDevice.  There can only be one VMBus in the
> > > > VM.
> > > 
> > > TYPE_DEVICE unless you actually need something TYPE_SYS_BUS_DEVICE
> > > provides.
> > > 
> > > > Now the question is how to add it to the system:
> > > >
> > > > 1) with a boolean machine property "vmbus" that would trigger the
> > > >    creation of the VMBus bridge; its class would have
> > > >    ->cannot_instantiate_with_device_add_yet = true
> > > 
> > > This makes it an optional onboard device.  Similar ones exist already,
> > > e.g. various optional onboard USB host controllers controlled by machine
> > > property "usb".
> > > 
> > > > 2) with a regular -device option; this would require setting
> > > >    ->has_dynamic_sysbus = true for i440fx machines (q35 already have it)
> > > 
> > > This makes it a pluggable sysbus device.
> > > 
> > > I'd be tempted to leave old i400FX rot in peace, but your use case may
> > > not allow that.
> > 
> > I have sent a RFC some time ago that replaces the all-or-nothing
> > has_dynamic_sysbus flag with an explicit sysbus device whitelist,
> > so i440fx wouldn't be a big problem.
> 
> I looked at the patchset, and it seems to adress this very nicely,
> indeed.
> 
> > But as you noted above, if
> > you don't need TYPE_SYS_BUS_DEVIC, you can just use TYPE_DEVICE.
> 
> Can you (or anybody else) please help me decide if I need
> TYPE_SYS_BUS_DEVICE?  Logically the VMBus bridge is "attached directly
> to the main system bus" as written at the top of include/hw/sysbus.h.

I think that documentation was written before we supported
bus-less devices.

> OTOH we use neither mmio nor pio members of SysBusDevice; nor do we
> currently use any of its *_irq helpers, but we may eventually need to
> (the guests require VMBus to announce two IRQs in its ACPI description
> but nothing seems to use them so we just hardcode two (almost) random
> numbers).

I'm not sure about the consequences of simply connecting IRQs
inside ->realize() without using the sysbus *_irq helpers. I hope
others can clarify this.

> 
> > > > 3) anything else
> > > >
> > > >
> > > > So far we went with 1) but since it's essentially the API the management
> > > > layer would have to use we'd like to get it right from the beginning.
> > > 
> > > Asking for advice here is a good idea.
> > > 
> > > Anyone?
> > > 
> > 
> > I would go with (2) instead of (1): it allows more flexibility in
> > case the device needs additional arguments, and will
> > automatically benefit from (present and future) mechanisms for
> > reporting available device-types and buses. Asking QEMU if
> > "-device FOO" is supported is easy and reliable; the mechanisms
> > for asking QEMU about supported "-machine" options are obscure
> > and probably not well-tested.
> 
> Good point in favor of -device, thanks.
> 
> Is there an idiom to express that no more than a single vmbus-bridge can
> be present in the system?  Or the only way to ensure that is with a
> static or a class variable and checking / setting it in ->realize?

I wouldn't use a static or class variable. You have some
alternatives:

You could check if a TYPE_VMBUS device already exists anywhere in
the device tree, or always create it at a specific QOM path.

Or, if your device is 100% specific for PC machines, you could
add a PCMachineState struct field pointing to the existing vmbus
device.

-- 
Eduardo