[libvirt] [PATCH 2/2] hw/vfio/display: add ramfb support

[Erik Skultety]

On Fri, Sep 14, 2018 at 09:16:33AM -0600, Alex Williamson wrote:
> On Fri, 14 Sep 2018 16:19:07 +0200
> Erik Skultety <eskultet at redhat.com> 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

Okay.

> 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

although possible, I'd reserve the model attribute to describe the backend API
used: vfio-pci, vfio-ap, etc.

> via attribute, ie. nohotplug="on", or perhaps if enabling a property

Hmm, I'm afraid there would be a significant amount of confusion around such an
attribute, because this would essentially become a feature "sink" + you'd still
need to expose the individual properties too, IOW if someone wanted to enable
one of x,y,z features they'd need to use 'nohotplug="on"', but in order for
libvirt to format the correct cmdline we still need to know that we have to use
-device vfio-pci-nohotplug with property ramfb="on" (okay this one can be
determined by the existing "display" attribute, but let's look at this in
general). I'd go for something more specific, like 'vgaBIOS="on"' so that it
immediately does tell more about what it is related to. However, I'm starting
to get the feeling that it's not the best idea to stuff all the
graphics-related attributes to the master <hostdev> element, but then again, we
already kinda started that trend with "display". I'm also wondering whether it
wouldn't have been a better idea to have had a video model "vfio-pci" instead
of the "none" model, as it would have been more extensible, but then again,
<video> describes the device within the guest, not the host...Truth to be told,
I'm not really sure what the best approach in libvirt would be, but regardless
of what we do in libvirt, it doesn't really matter for QEMU which way you decide
to go here as long as QEMU is able to "inform" libvirt about that.

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

At first I was playing with the idea that we actually could do that based on
the presence of the 'display' attribute, but I guess that would be a very
dangerous road to take even though right now I somehow can't see a potential
issue, did you think about something specific which we can discuss and
therefore find a better solution?

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

Erik