On Wed, 2020-10-07 at 15:07 +0200, Peter Krempa wrote: > 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. > Okay, so should I bug qemu to promote the feature to stable? It's been like that forever, it's certainly not a new feature:
https://github.com/qemu/qemu/commit/f15689c7e4422d5453ae45628df5b83a53e518ed So it's been that way for 8 years! > > 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 <st...@snewbury.org.uk> > > --- > > 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. > Yes, I should have marked this [RFC] since I obviously didn't put enough time into this patch (see below). I partly blame the fact it worked first time and I thought, good enough... Then I thought I really should send it upstream! > > 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. > Like I just did in the chunks below! > > > 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. Yes, sorry. I didn't read the context properly, I'm sure it's quite clear I grepped for multifunction...
