[PATCH] Support x-vga=on for PCI host passthrough devices
Steven Newbury
steve at snewbury.org.uk
Wed Oct 7 13:20:19 UTC 2020
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 <steve at 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...
More information about the libvir-list
mailing list