[libvirt] [RFC PATCH 7/7] qemu: command: Enable formatting vfio-pci.display option onto cmdline

[Erik Skultety]

On Fri, Jun 08, 2018 at 12:00:50PM -0400, John Ferlan wrote:
>
>
> On 06/08/2018 07:09 AM, Erik Skultety wrote:
> > On Mon, Jun 04, 2018 at 07:40:42PM -0400, John Ferlan wrote:
> >>
> >>
> >> On 05/30/2018 09:43 AM, Erik Skultety wrote:
> >>> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> >>> ---
> >>>  src/qemu/qemu_command.c                            | 24 +++++++++++++++-
> >>>  .../hostdev-mdev-display-spice-no-opengl.args      | 32 ++++++++++++++++++++++
> >>>  .../hostdev-mdev-display-spice-opengl.args         | 31 +++++++++++++++++++++
> >>>  .../qemuxml2argvdata/hostdev-mdev-display-vnc.args | 32 ++++++++++++++++++++++
> >>>  tests/qemuxml2argvtest.c                           | 23 ++++++++++++++++
> >>>  5 files changed, 141 insertions(+), 1 deletion(-)
> >>>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-no-opengl.args
> >>>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-spice-opengl.args
> >>>  create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-vnc.args
> >>>
> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >>> index 1667b09a8b..8a1a4dc72b 100644
> >>> --- a/src/qemu/qemu_command.c
> >>> +++ b/src/qemu/qemu_command.c
> >>> @@ -5088,6 +5088,10 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
> >>>      virBufferAdd(&buf, dev_str, -1);
> >>>      virBufferAsprintf(&buf, ",id=%s,sysfsdev=%s", dev->info->alias, mdevPath);
> >>>
> >>> +    if (mdevsrc->display)
> >>
> >>> VIR_TRISTATE_SWITCH_ABSENT
> >>
> >>> +        virBufferAsprintf(&buf, ",display=%s",
> >>> +                          virTristateSwitchTypeToString(mdevsrc->display));
> >>> +
> >>>      if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
> >>>          goto cleanup;
> >>>
> >>> @@ -5305,7 +5309,9 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> >>>
> >>>          /* MDEV */
> >>>          if (virHostdevIsMdevDevice(hostdev)) {
> >>> -            switch ((virMediatedDeviceModelType) subsys->u.mdev.model) {
> >>> +            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev;
> >>> +
> >>> +            switch ((virMediatedDeviceModelType) mdevsrc->model) {
> >>>              case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> >>>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
> >>>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>> @@ -5313,6 +5319,15 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> >>>                                       "supported by this version of QEMU"));
> >>>                      return -1;
> >>>                  }
> >>> +
> >>> +                if (mdevsrc->display &&
> >>
> >> != SWITCH_ABSENT
> >>
> >>> +                    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) {
> >>> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>> +                                   _("display property of device vfio-pci is "
> >>> +                                     "not supported by this version of QEMU"));
> >>> +                        return -1;
> >>> +                }
> >>> +
> >>
> >> After reading this, I thought perhaps patch 6 wasn't necessary at least
> >> w/r/t that by setting off we write out to the config file which may not
> >> be desired.
> >>
> >> So as a way to avoid this, can we set a "local" @mdev_display value and
> >> pass that qemuBuildHostdevMediatedDevStr which then would then either
> >> ignore or write on/off on the command line.
> >>
> >> Thus:
> >>
> >>     if (mdevsrc->display > SWITCH_ABSENT && !VFIO_PCI_DISPLAY)
> >>         error as is
> >>     mdev_display = mdevsrc->display
> >>     if (mdev_display == SWITCH_ABSENT && VFIO_PCI_DISPLAY)
> >>         mdev_display == SWITCH_OFF;
> >>
> >> And passing mdev_display to building the command line will print "off"
> >> when mdevsrc->display == ABSENT, but we don't write "off" to the config
> >> file.
> >>
> >> ??? thoughts
> >
> > So, w/r/t comments in patch 6, I understand that we'd keep an open door to
> > changing the default (no display attr means use 'auto' as it would in most
> > cases in libvirt). However, I don't believe that we'd want to make 'auto' a
> > default ever because you can't predict whether the user wants to use the device
> > as GPGPU or graphics, if they leave the display attr unset and we'd default to
> > auto in the future, we might risk that they know they can't use it for graphics
> > and they didn't even intend to, but we tried and failed because of some
> > underlying reasons. And I'm not even sure we'd ever introduce 'auto' to libvirt
> > for reasons I just mentioned + the cover letter and the discussion that
> > happened upstream regarding the VNC console. What I know though, is that
> > libvirt is never going to use QEMU's 'auto', since it doesn't make sense,
> > either we take care of everything or the user does, regardless of QEMU.
> >
> > Anyway, the way I imagine 'auto' could work in libvirt is that user references
> > an mdev, but doesn't care (or doesn't know) what technology it uses underneath
> > and explicitly says "libvirt please take care of that on my behalf, I want to
> > use the display, but have 0 clue on what to enable". That's a different kind of
> > 'auto' compared to when we'd try to read user's mind. I tried to play it very
> > safe here, as the vgpu display thing can be very tricky, not even thinking about
> > future migrations. But if there's a strong disagreement with this kind of
> > approach, then I'll of course need to incorporate your comments in patch 6 and
> > this one to get this successfully merged.
> >
> > Erik
> >
>
> I didn't think I was advocating for adding AUTO - just blabbering how
> insane the code is ;-).  IIRC, if "off" isn't written then auto is
> assumed. What I thought I was suggesting here though was to not write
> the "off" into the libvirt XML, but rather only into the .args. IOW: If
> the option the option is available in qemu, but not supplied in libvirt
> XML, then write "off". If someone changes the XML to have "off" or "on",
> then that's fine - go with what they have and keep it. I just was trying
> to be cautious about writing "off" into the output XML (especially the
> *config* one).

Alright, we could skip the "offline" XML that's a fair argument, but we should
format something into the "live" one, as that one serves as the base for the
"migration" XML, I'd rather us not sending a missing display argument to a newer
libvirt on the destination for safety reasons, I believe that turning something
'off' rather than 'on' by default is good.

Erik