11 a.m.
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>
>> ---
>> 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