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

Fri Jun 8 16:00:50 UTC 2018

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

John

>>
>>>                  break;
>>>              case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>>>                  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW)) {
>>> @@ -5335,6 +5350,13 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>>>                  return -1;
>>>              virCommandAddArg(cmd, devstr);
>>>              VIR_FREE(devstr);
>>> +
>>> +            /* we need to add '-display egl-headless' if 'display=on' for
>>> +             * vfio-pci and OpenGL is disabled (either not supported or user
>>> +             * forgot to add 'gl=on' to SPICE or simply wants to use VNC...)
>>> +             */
>>> +            if (mdevsrc->display && !virDomainDefHasSpiceGL(def))
>>
>> Doesn't this only matter if display == SWITCH_ON  ?
>>
>> Hence the OCD on the "if (mdevsrc->display)" type checks. ;-)
> 
> Will fix this.
> 
> Thanks,
> Erik
> 