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 ++++++ 7 files changed, 26 insertions(+)
When using a passthrough GPU with libvirt there is no option to
pass "x-vga=on" to the device specification. This means legacy
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@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 ++++++
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,
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 38b901a6c4..b2864c1e9b 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -600,6 +600,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 380 */
"usb-host.hostdevice",
+ "pci-vga",
);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 107056ba17..d2d456fc43 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -580,6 +580,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
/* 380 */
QEMU_CAPS_USB_HOST_HOSTDEVICE, /* -device usb-host.hostdevice */
+ X_QEMU_CAPS_PCI_VGA, /* -device x-vga=on|off */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 476cf6972e..b4285425ed 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -350,6 +350,10 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
virBufferAddLit(buf, ",multifunction=on");
else if (info->addr.pci.multi == VIR_TRISTATE_SWITCH_OFF)
virBufferAddLit(buf, ",multifunction=off");
+ if (info->addr.pci.vga == VIR_TRISTATE_SWITCH_ON)
+ virBufferAddLit(buf, ",x-vga=on");
+ else if (info->addr.pci.vga == VIR_TRISTATE_SWITCH_OFF)
+ virBufferAddLit(buf, ",x-vga=off");
virBufferAsprintf(buf, ",addr=0x%x", info->addr.pci.slot);
if (info->addr.pci.function != 0)
virBufferAsprintf(buf, ".0x%x", info->addr.pci.function);
diff --git a/src/util/virpci.h b/src/util/virpci.h
index b3322ba61b..1ec2e3ba34 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -61,6 +61,7 @@ struct _virPCIDeviceAddress {
unsigned int slot;
unsigned int function;
int multi; /* virTristateSwitch */
+ int vga; /* virTristateSwitch */
int extFlags; /* enum virPCIDeviceAddressExtensionFlags */
virZPCIDeviceAddress zpci;
/* Don't forget to update virPCIDeviceAddressCopy if needed. */
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")
+ },
{.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'");
virBufferAddLit(&buf, "/>\n");
} else if (diskAddr.type == DISK_ADDR_TYPE_CCW) {
virBufferAsprintf(&buf,
--
2.28.0
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@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.
> 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.
> virBufferAddLit(&buf, "/>\n");
> } else if (diskAddr.type == DISK_ADDR_TYPE_CCW) {
> virBufferAsprintf(&buf,
> --
> 2.28.0
>
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@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...
On Wed, 2020-10-07 at 14:20 +0100, Steven Newbury wrote:
> 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:
> > >
> > > 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.
As I see it there are two distinct validation requirements:
1) It should only apply to hostdev PCI vfio devices
2) It is only relevant to PCI Class 3 (Display Controller), but libvirt
doesn't know about the PCI device class, does it?
1 is the most important probably. Should it just be hard error if it's
defined for each devices type except for hostdev in domain_conf.c or
check for device type hostdev before setting the parameter and ignore
it otherwise?
On Wed, 2020-10-07 at 15:56 +0100, Steven Newbury wrote: > > As I see it there are two distinct validation requirements: > > 1) It should only apply to hostdev PCI vfio devices > I think I've come up with a solution: Add a new enum to extFlags to allow vga, and set the flag for vfio pci devices in domain_conf.c. Then throw an error if vga is set when the flag is not. Doesn't help with below though, unless there's some way to know the device class. > 2) It is only relevant to PCI Class 3 (Display Controller), but > libvirt > doesn't know about the PCI device class, does it? > > > 1 is the most important probably. Should it just be hard error if > it's > defined for each devices type except for hostdev in domain_conf.c or > check for device type hostdev before setting the parameter and ignore > it otherwise? > > >
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@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...
On Wed, 07 Oct 2020 14:20:21 +0100 Steven Newbury <steve@snewbury.org.uk> wrote: > 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! It's that way upstream because VGA routing is a nightmare, it's essentially broken on any system with Intel graphics because the device always has VGA regions routed to itself. That problem is not getting better, but the demand for VGA is getting less, so there's very little incentive to work on the problem rather than just letting it die out once nobody cares about VGA. Any 600 series or newer GeForce card should have a UEFI ROM available. Also note that VGA support can be configured both as a module option of vfio-pci and a build option, so there's no guarantee that a display class device will have the VGA regions available for QEMU to use this option. Thanks, Alex
-------- Original message --------From: Alex Williamson <alex.williamson@redhat.com> Date: 07/10/2020 18:08 (GMT+00:00) To: Steven Newbury <steve@snewbury.org.uk> Cc: Peter Krempa <pkrempa@redhat.com>, libvir-list@redhat.com Subject: Re: [PATCH] Support x-vga=on for PCI host passthrough devices On Wed, 07 Oct 2020 14:20:21 +0100Steven Newbury <steve@snewbury.org.uk> wrote:> 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!It's that way upstream because VGA routing is a nightmare, it'sessentially broken on any system with Intel graphics because the devicealways has VGA regions routed to itself. That problem is not gettingbetter, but the demand for VGA is getting less, so there's very littleincentive to work on the problem rather than just letting it die outonce nobody cares about VGA. Any 600 series or newer GeForce cardshould have a UEFI ROM available. Also note that VGA support can beconfigured both as a module option of vfio-pci and a build option, sothere's no guarantee that a display class device will have the VGAregions available for QEMU to use this option. Thanks,I'm still going to fix up my patch even if it's not going to get committed. It's very useful for me, my old nvidia card doesn't work without it, and I also need it for seabios/dos compatibility with my ATIs. In fairness, I haven't tried it with my Intel, it's too old to support the new vGPU stuff and I need to see what I'm typing! ;-)For what it's worth, I had no trouble with GPU passthrough once I realised it needed x-vga=on. VGA arbitration was automatically switched. It's been rock solid stability wise, although the 8800 has horrible VGA/DOS performance. Windows10 runs like on bare metal.
On Wed, 07 Oct 2020 19:59:36 +0100
steve <steve@snewbury.org.uk> wrote:
> -------- Original message --------
> From: Alex Williamson <alex.williamson@redhat.com>
> Date: 07/10/2020 18:08 (GMT+00:00)
> To: Steven Newbury <steve@snewbury.org.uk>
> Cc: Peter Krempa <pkrempa@redhat.com>, libvir-list@redhat.com
> Subject: Re: [PATCH] Support x-vga=on for PCI host passthrough devices
>
> On Wed, 07 Oct 2020 14:20:21 +0100
> Steven Newbury <steve@snewbury.org.uk> wrote:
>
> > 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!
>
> It's that way upstream because VGA routing is a nightmare, it's
> essentially broken on any system with Intel graphics because the device
> always has VGA regions routed to itself. That problem is not getting
> better, but the demand for VGA is getting less, so there's very little
> incentive to work on the problem rather than just letting it die out
> once nobody cares about VGA. Any 600 series or newer GeForce card
> should have a UEFI ROM available. Also note that VGA support can be
> configured both as a module option of vfio-pci and a build option, so
> there's no guarantee that a display class device will have the VGA
> regions available for QEMU to use this option. Thanks,
>
> I'm still going to fix up my patch even if it's not going to get
> committed. It's very useful for me, my old nvidia card doesn't work
> without it, and I also need it for seabios/dos compatibility with my
> ATIs. In fairness, I haven't tried it with my Intel, it's too old to
> support the new vGPU stuff and I need to see what I'm typing! ;-)
>
> For what it's worth, I had no trouble with GPU passthrough once I
> realised it needed x-vga=on. VGA arbitration was automatically
> switched. It's been rock solid stability wise, although the 8800 has
> horrible VGA/DOS performance. Windows10 runs like on bare metal.
Are you aware that you can add arbitrary options to devices using the
<qemu:commandline> support in libvirt? For example:
<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
...
<devices>
...
<hostdev mode='subsystem' type='pci' managed='yes'>
<source>
<address domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
</source>
<address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
<alias name='ua-geforce8800'/>
</hostdev>
...
</devices>
<qemu:commandline>
<qemu:arg value='-set'/>
<qemu:arg value='device.ua-geforce8800.x-vga=on'/>
</qemu:commandline>
</domain>
This is generally how users make use of unsupportable or
not-yet-supported QEMU features while still using libvirt to manage the
VM. Thanks,
Alex
On Wed, 2020-10-07 at 13:17 -0600, Alex Williamson wrote: > Are you aware that you can add arbitrary options to devices using the > <qemu:commandline> support in libvirt? For example: > > <domain type='kvm' xmlns:qemu=' > http://libvirt.org/schemas/domain/qemu/1.0'> > ... > <devices> > ... > <hostdev mode='subsystem' type='pci' managed='yes'> > <source> > <address domain='0x0000' bus='0x01' slot='0x00' > function='0x0'/> > </source> > <address type='pci' domain='0x0000' bus='0x01' slot='0x00' > function='0x0'/> > <alias name='ua-geforce8800'/> > </hostdev> > ... > </devices> > <qemu:commandline> > <qemu:arg value='-set'/> > <qemu:arg value='device.ua-geforce8800.x-vga=on'/> > </qemu:commandline> > </domain> > > This is generally how users make use of unsupportable or > not-yet-supported QEMU features while still using libvirt to manage > the > VM. I am aware of it, but I never managed to get virt-manager to accept arbitary command line arguments, but never persisted in trying to get it to work since it felt hackier than patching libvirt to accept the parameter.
On 10/7/20 5:44 PM, Steven Newbury wrote: > On Wed, 2020-10-07 at 13:17 -0600, Alex Williamson wrote: >> Are you aware that you can add arbitrary options to devices using the >> <qemu:commandline> support in libvirt? For example: >> >> <domain type='kvm' xmlns:qemu=' >> http://libvirt.org/schemas/domain/qemu/1.0'> >> ... >> <devices> >> ... >> <hostdev mode='subsystem' type='pci' managed='yes'> >> <source> >> <address domain='0x0000' bus='0x01' slot='0x00' >> function='0x0'/> >> </source> >> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' >> function='0x0'/> >> <alias name='ua-geforce8800'/> >> </hostdev> >> ... >> </devices> >> <qemu:commandline> >> <qemu:arg value='-set'/> >> <qemu:arg value='device.ua-geforce8800.x-vga=on'/> >> </qemu:commandline> >> </domain> >> >> This is generally how users make use of unsupportable or >> not-yet-supported QEMU features while still using libvirt to manage >> the >> VM. > I am aware of it, but I never managed to get virt-manager to accept > arbitary command line arguments, That should work with no problems, as long as you know where to put it. If you go into the configuration for a guest (either during guest creation by clicking on "Customize configuration before install", or after creation by double clicking on the guest name then clicking on the "light bulb" icon at the upper left), click on "Overview" in the list on the left, then click on the "XML" tab above the right-hand side of the window (the default tab is "Details"), then you should be presented with the full XML for the guest. You'll need to do three things: 1) change the very top line of XML that currently says <domain type='kvm'> to <domain xmlns:qemu="http://libvirt.org/schemas/domain/qemu/1.0" type="kvm"> 2) In the middle of the XML, where you will find the <hostdev> element for your assigned GPU (similar to this example), add the line "<alias name="ua-video"/> as shown here: <hostdev mode="subsystem" type="pci" managed="no"> <source> <address domain="0x0000" bus="0x05" slot="0x00" function="0x0"/> </source> <alias name="ua-video"/> <address type="pci" domain="0x0000" bus="0x05" slot="0x00" function="0x0" multifunction="on"/> </hostdev> 3) down at the bottom of the XML, just above the "</domain>" line, add this: <qemu:commandline> <qemu:arg value="-set"/> <qemu:arg value="device.ua-video.x-vga=on"/> </qemu:commandline> Once you've made these 3 small changes, click on the "Apply" button. > but never persisted in trying to get > it to work since it felt hackier than patching libvirt to accept the > parameter. It is *definitely* less hacky to use <qemu:commandline> than to carry your own local patch on top of the libvirt source, which would force you to always build your own libvirt binaries, and rebase your patch every time upstream libvirt changed code that touched the same place. Although <qemu:commandline> isn't "Supported" (Capital "S" - in the sense that a vendor providing paid technical support for a system using libvirt won't officially commit themselves to solving any problem you may have associated with use of <qemu:commandline>, and the options you provide there *could* disappear from qemu), it is "supported" (small "s" - in the sense that libvirt has no plans to remove <qemu:commandline>, and it is used by other people so if it becomes broken it will surely get fixed). Using <qemu:commandline> will take 10 minutes of your time right now, and then you'll never have to think about it ever again (until/unless QEMU removes the x-vga option). Using your own custom build of libvirt that carries a locally written patch will continue to be a burden every time you upgrade libvirt until the end of time.
On Wed, 2020-10-07 at 21:45 -0400, Laine Stump wrote: > > > It is *definitely* less hacky to use <qemu:commandline> than to carry > your own local patch on top of the libvirt source, which would force > you to always build your own libvirt binaries, and rebase your patch > every time upstream libvirt changed code that touched the same place. > Although <qemu:commandline> isn't "Supported" (Capital "S" - in the > sense that a vendor providing paid technical support for a system > using libvirt won't officially commit themselves to solving any > problem you may have associated with use of <qemu:commandline>, and > the options you provide there *could* disappear from qemu), it is > "supported" (small "s" - in the sense that libvirt has no plans to > remove <qemu:commandline>, and it is used by other people so if it > becomes broken it will surely get fixed). > > > > Using <qemu:commandline> will take 10 minutes of your time right now, > and then you'll never have to think about it ever again (until/unless > QEMU removes the x-vga option). Using your own custom build of > libvirt that carries a locally written patch will continue to be a > burden every time you upgrade libvirt until the end of time. > Thanks for the detailed instructions, I had missed the alias part before, I don't know if that was where I was going wrong? The <qemu::commandline> part would just disappear when I hit apply. The original patch only took me 10mins at most, yeah it shows, but worked straight away. I've been running Gentoo unstable for the last 20 years, patch rebasing isn't really a problem! :-) I'm very used to creating patches for new features and bug fixes as I need to, it's part of my usual update process. I must have written thousands over the years. I've been trying to make a bit of effort to get some pushed upstream, rather than having them sit in an overlay or "portage/patches". It is taking some time and effort to get the validation working, but I don't think it's a waste of time. I'm getting better familarised with the code which isn't a bad thing, and might allow me to make more appropriate contributions in the future. Maybe I'll try fixing the "isapc" machine type?! ;-)
On 10/8/20 4:54 AM, Steven Newbury wrote: > On Wed, 2020-10-07 at 21:45 -0400, Laine Stump wrote: >> >> >> It is *definitely* less hacky to use <qemu:commandline> than to carry >> your own local patch on top of the libvirt source, which would force >> you to always build your own libvirt binaries, and rebase your patch >> every time upstream libvirt changed code that touched the same place. >> Although <qemu:commandline> isn't "Supported" (Capital "S" - in the >> sense that a vendor providing paid technical support for a system >> using libvirt won't officially commit themselves to solving any >> problem you may have associated with use of <qemu:commandline>, and >> the options you provide there *could* disappear from qemu), it is >> "supported" (small "s" - in the sense that libvirt has no plans to >> remove <qemu:commandline>, and it is used by other people so if it >> becomes broken it will surely get fixed). >> >> >> >> Using <qemu:commandline> will take 10 minutes of your time right now, >> and then you'll never have to think about it ever again (until/unless >> QEMU removes the x-vga option). Using your own custom build of >> libvirt that carries a locally written patch will continue to be a >> burden every time you upgrade libvirt until the end of time. >> > Thanks for the detailed instructions, I had missed the alias part > before, I don't know if that was where I was going wrong? Nah, that would just lead to an error when you started the guest. > The > <qemu::commandline> part would just disappear when I hit apply. The most likely reason for that would be a) if you left out step (1), or b) if you put <qemu:commandline> after </domain>, or maybe put it inside some other element (e.g. <devices>). > The original patch only took me 10mins at most, yeah it shows, but > worked straight away. I've been running Gentoo unstable for the last > 20 years, patch rebasing isn't really a problem! :-) I'm very used to > creating patches for new features and bug fixes as I need to, it's part > of my usual update process. I must have written thousands over the > years. I've been trying to make a bit of effort to get some pushed > upstream, rather than having them sit in an overlay or > "portage/patches". > > It is taking some time and effort to get the validation working, but I > don't think it's a waste of time. I'm getting better familarised with > the code which isn't a bad thing, and might allow me to make more > appropriate contributions in the future. Now *that* is a worthwhile reason! :-) > Maybe I'll try fixing the > "isapc" machine type?! ;-) Hmm. Trying to think of why you would need that (other than just the standard "because it's there"), and coming up empty. I *think* even MSDOS will boot on an i440fx guest...
© 2016 - 2024 Red Hat, Inc.