Re: [Qemu-devel] [libvirt] [PATCH 2/2] hw/vfio/display: add ramfb support

[Alex Williamson]

On Fri, 14 Sep 2018 16:19:07 +0200
Erik Skultety <address@hidden> wrote:

> On Fri, Sep 14, 2018 at 12:50:09PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >  
> > > > Also libvirt manages hotpluggability per device *class*, not per device
> > > > *instance*.  So a device being hotpluggable or not depending on some
> > > > device property is a problem for libvirt ...
> > > >
> > > > I'm open to suggestions how to handle this better, as long as the
> > > > libvirt people are on board with the approach.  
> > >
> > > Ok, so we need a new class to handle making a device non-hotpluggable,
> > > but I'm still not sure whether we should make:
> > >
> > >  -device vfio-pci-ramfb
> > >
> > > or
> > >
> > >  -device vfio-pci-nohotplug,ramfb=on
> > >
> > > Where ramfb would be a property only available on the nohotplug class
> > > variant.  
> >
> > I'm fine with the latter.
> >  
> > > The latter seems to provide a lot more flexibility, but which
> > > is more practical for libvirt?  
> >
> > Any comment from the libvirt camp?  
> 
> We had a discussion about this a few months ago [1] where we spoke about
> -device vfio-pci-ramfb.

Ah yes, probably my bad for not following up more thoroughly there.

> However, as Alex has pointed out, the latter proposal
> gives us more flexibility in terms of introduction of other device properties
> which are unrelated to ramfb but still might require non-hotpluggable device.
> Either way, libvirt needs a capability to test whether we should favour this
> new device over plain vfio-pci if an mdev with display='on' is required.
> What about new device properties (specifically mdev)? In the discussion below,
> Gerd noted that apart from the ramfb stuff and the fact that one can be
> hotplugged while the latter can not, these are identical (option-wise), is 
> that
> to stay, IOW are we going to keep these two device classes in sync when
> introducing new vfio-pci device options or are these going to divert more? Is
> it even possible? What I mean by that is that I'd like to avoid is a situation
> where there are 2 disjunct sets of options which could potentially lead to
> problems in decision making in libvirt and we don't like making decisions.

The vfio-pci device is the parent of this new device, so it should
automatically inherit any new properties of vfio-pci, it only modifies
the device class for non-hotpluggability and adds properties dependent
on non-hotpluggability.  I'm not sure if libvirt would expose this as a
new model, ie. model="vfio-pci-nohotplug", or if it would be selected
via attribute, ie. nohotplug="on", or perhaps if enabling a property
only found on the nohotplug variant would select it, ie. ramfb="on".
The latter option alone makes it difficult for a user to select it for
any random device, for instance if they're trying to setup a kiosk VM
where they want to prevent even the guest OS admin from changing the VM
configuration.  In any case, it seems that libvirt would never be
enabling this automatically.

> Anyhow, I don't feel like any of the proposals has a strong
> advantage/disadvantage in usage for libvirt, both will require a capability 
> and
> both would be special cased in our cmdline code depending on the 'display'
> attribute. Luckily, we don't have mdev migration yet, so it's good we don't
> have to worry about that at this point yet.

That's a good point that ramfb depends on display, it seems that
regardless of which route we take, using vfio-pci-ramfb or
vfio-pci-nohotplug,ramfb=on, it should fail without a display rather
than simply adding functionality if a display is present or in the
former case, being an obscure way to make a device non-hotpluggable.
Personally I prefer the non-hotplug variant of vfio-pci in hopes that
it provides more flexibility to users and we only need to tackle this
issue once rather than each device we invent with a non-hotplug
dependency.  Thanks,

Alex