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

[Steven Newbury <steve@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> > ---
> >  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...