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

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(a)redhat.com&gt; >>> --- >>> 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).