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@xxxxxxxxxx>
>>> ---
>>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
