Re: [PATCH] Support x-vga=on for PCI host passthrough devices

On Wed, Oct 07, 2020 at 13:59:35 +0100, Steven Newbury wrote: > When using a passthrough GPU with libvirt there is no option to > pass "x-vga=on" to the device specification. This means legacy Please note that we don't add support for experimental qemu features (prefixed with "x-") until they are deemed stable and the x- is removed, so this patch can't be accepted in this form.

> VGA support isn't available which prevents any non-UEFI cards from > POSTing and prevents some drivers from initialising for example > Windows 10 NVIDIA driver for GeForce 8800. > > Signed-off-by: Steven Newbury <steve(a)snewbury.org.uk&gt; > --- > src/conf/device_conf.c | 9 +++++++++ > src/conf/domain_conf.c | 4 ++++ > src/qemu/qemu_capabilities.c | 1 + > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 4 ++++ > src/util/virpci.h | 1 + > tools/virsh-domain.c | 6 ++++++ We require that patches are split into logical chunks, thus e.g. the XML bits and (missing) documentation should be separate, then the qemu capabilities stuff needs to be separate, implementation in qemu needs to be separate, and virsh changes need to be separate. Also it's missing tests for the XML.

> 7 files changed, 26 insertions(+) > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c > index 87bf32bbc6..02d226747e 100644 > --- a/src/conf/device_conf.c > +++ b/src/conf/device_conf.c > @@ -215,6 +215,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, > g_autofree char *slot = virXMLPropString(node, "slot"); > g_autofree char *function = virXMLPropString(node, > "function"); > g_autofree char *multi = virXMLPropString(node, > "multifunction"); > + g_autofree char *vga = virXMLPropString(node, "vga"); > > memset(addr, 0, sizeof(*addr)); > > @@ -253,6 +254,14 @@ virPCIDeviceAddressParseXML(xmlNodePtr node, > multi); > return -1; > > + } > + if (vga && > + ((addr->vga = virTristateSwitchTypeFromString(vga)) <= 0)) > { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Unknown value '%s' for <address> 'vga' > attribute"), > + vga); > + return -1; > + > } > if (!virPCIDeviceAddressIsEmpty(addr) && > !virPCIDeviceAddressIsValid(addr, true)) > return -1; > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index c003b5c030..048b0f4028 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -7587,6 +7587,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf, > virBufferAsprintf(&attrBuf, " multifunction='%s'", > virTristateSwitchTypeToString(info- > >addr.pci.multi)); > } > + if (info->addr.pci.vga) { > + virBufferAsprintf(&attrBuf, " vga='%s'", > + virTristateSwitchTypeToString(info- > >addr.pci.vga)); > + } > > if (virZPCIDeviceAddressIsPresent(&info->addr.pci.zpci)) { > virBufferAsprintf(&childBuf, We also need validation that this isn't used with devices where it doesn't make sense ... such as disk, or network card.

> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index 8f11393197..587efbdb2a 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -294,6 +294,10 @@ static const vshCmdOptDef opts_attach_disk[] = > { > .type = VSH_OT_BOOL, > .help = N_("use multifunction pci under specified address") > }, > + {.name = "vga", > + .type = VSH_OT_BOOL, > + .help = N_("enable legacy VGA") See below. > + }, > {.name = "print-xml", > .type = VSH_OT_BOOL, > .help = N_("print XML document rather than attach the disk") > @@ -694,6 +698,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd > *cmd) > diskAddr.addr.pci.slot, > diskAddr.addr.pci.function); > if (vshCommandOptBool(cmd, "multifunction")) > virBufferAddLit(&buf, " multifunction='on'"); > + if (vshCommandOptBool(cmd, "vga")) > + virBufferAddLit(&buf, " vga='on'"); This doesn't make any sense. This function is formatting an <disk> definition, where VGA doesn't make any sense.