7:32 a.m.
On 05/09/2017 07:53 AM, Roman Bogorodskiy wrote:
Add support for video driver configuration. In domain xml it looks
like
this:
<video>
<model type=''>
<driver .../>
</model>
</video>
At present, the only supported configuration is 'vgaconf' that looks this way:
<driver vgaconf='io|on|off'>
It was added with bhyve gop video in mind to allow users control how the
video device is exposed to the guest, specifically, how VGA I/O is
handled.
Signed-off-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com>
---
docs/schemas/domaincommon.rng | 13 +++++++++
src/conf/domain_conf.c | 63 +++++++++++++++++++++++++++++++++++++++++--
src/conf/domain_conf.h | 17 ++++++++++++
src/libvirt_private.syms | 2 ++
4 files changed, 93 insertions(+), 2 deletions(-)
Due to languishing on list and given jtomko's changes in a similar
place, this no longer 'git am -3' applies... Could you please rebase
and resend? You may also need to rethink the <driver> subelement too as
it seems jtomko's commit f5384fb4 added this as well.
The xml2xml test should also be added here w/ various options...
There's no adjustment to the formatdomain.html.in to describe this -
would it be strictly related to one specific "<model
type='?'.../>"
diff --git a/docs/schemas/domaincommon.rng
b/docs/schemas/domaincommon.rng
index 281309ec0..f45820383 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3278,6 +3278,19 @@
</optional>
</element>
</optional>
+ <optional>
+ <element name="driver">
+ <optional>
+ <attribute name="vgaconf">
+ <choice>
+ <value>io</value>
+ <value>on</value>
+ <value>off</value>
+ </choice>
+ </attribute>
+ </optional>
Seems strange to have an optional <driver> with one optional attribute
"vgaconf".
+ </element>
+ </optional>
</element>
</optional>
<optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0ff216e3a..2ab55b52f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -553,6 +553,11 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
"virtio",
"gop")
+VIR_ENUM_IMPL(virDomainVideoVgaconf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
+ "io",
+ "on",
+ "off")
+
A "nit", but should it be "VGA" and not "Vga"?
VIR_ENUM_IMPL(virDomainInput, VIR_DOMAIN_INPUT_TYPE_LAST,
"mouse",
"tablet",
@@ -2280,6 +2285,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
virDomainDeviceInfoClear(&def->info);
VIR_FREE(def->accel);
+ VIR_FREE(def->driver);
VIR_FREE(def);
}
@@ -13368,6 +13374,43 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
return def;
}
+static virDomainVideoDriverDefPtr
+virDomainVideoDriverDefParseXML(xmlNodePtr node)
+{
+ xmlNodePtr cur;
+ virDomainVideoDriverDefPtr def;
+ char *vgaconf = NULL;
+ int val;
+
+ cur = node->children;
+ while (cur != NULL) {
+ if (cur->type == XML_ELEMENT_NODE) {
+ if (!vgaconf &&
+ xmlStrEqual(cur->name, BAD_CAST "driver")) {
+ vgaconf = virXMLPropString(cur, "vgaconf");
+ }
+ }
+ cur = cur->next;
+ }
+
+ if (!vgaconf)
+ return NULL;
+
+ if (VIR_ALLOC(def) < 0)
+ goto cleanup;
+
+ if ((val = virDomainVideoVgaconfTypeFromString(vgaconf)) <= 0) {
<= ?
How does VIR_DOMAIN_VIDEO_VGACONF_IO get set? Perhaps a specific xml2xml
test...
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown vgaconf value '%s'"), vgaconf);
+ goto cleanup;
+ }
+ def->vgaconf = val;
+
+ cleanup:
+ VIR_FREE(vgaconf);
+ return def;
+}
+
static virDomainVideoDefPtr
virDomainVideoDefParseXML(xmlNodePtr node,
const virDomainDef *dom,
@@ -13405,6 +13448,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
}
def->accel = virDomainVideoAccelDefParseXML(cur);
+ def->driver = virDomainVideoDriverDefParseXML(cur);
}
}
cur = cur->next;
@@ -22998,6 +23042,18 @@ virDomainVideoAccelDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}
+static void
+virDomainVideoDriverDefFormat(virBufferPtr buf,
+ virDomainVideoDriverDefPtr def)
+{
+ virBufferAddLit(buf, "<driver");
+ if (def->vgaconf) {
+ virBufferAsprintf(buf, " vgaconf='%s'",
+ virDomainVideoVgaconfTypeToString(def->vgaconf));
+ }
Without def->driver, the def->vgaconf doesn't exist, true?
If !vgaconf, then all you're getting is <driver>, which while
technically legal according to the RNG added here does look a little
strange. Are there plans to "enhance" this in the future? Always a bit
difficult to take that crystal ball out, but wouldn't that type of
enhancement need some sort of "vgaconf" or would it be possible for
something else...
Given what I found w/ jtomko's changes and this, maybe the optional
subelement just becomes <vgaconf>, unless you see <driver> being
expanded. Still vgaconf would seem to be related to model type='vga' in
my mind at least...
John
+ virBufferAddLit(buf, "/>\n");
+}
+
static int
virDomainVideoDefFormat(virBufferPtr buf,
@@ -23028,10 +23084,13 @@ virDomainVideoDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " heads='%u'", def->heads);
if (def->primary)
virBufferAddLit(buf, " primary='yes'");
- if (def->accel) {
+ if (def->accel || def->driver) {
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
- virDomainVideoAccelDefFormat(buf, def->accel);
+ if (def->accel)
+ virDomainVideoAccelDefFormat(buf, def->accel);
+ if (def->driver)
+ virDomainVideoDriverDefFormat(buf, def->driver);
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</model>\n");
} else {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 09fb7aada..cbf25a67b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1350,6 +1350,16 @@ typedef enum {
} virDomainVideoType;
+typedef enum {
+ VIR_DOMAIN_VIDEO_VGACONF_IO = 0,
+ VIR_DOMAIN_VIDEO_VGACONF_ON,
+ VIR_DOMAIN_VIDEO_VGACONF_OFF,
+
+ VIR_DOMAIN_VIDEO_VGACONF_LAST
+} virDomainVideoVgaconf;
+
+VIR_ENUM_DECL(virDomainVideoVgaconf)
+
typedef struct _virDomainVideoAccelDef virDomainVideoAccelDef;
typedef virDomainVideoAccelDef *virDomainVideoAccelDefPtr;
struct _virDomainVideoAccelDef {
@@ -1358,6 +1368,12 @@ struct _virDomainVideoAccelDef {
};
+typedef struct _virDomainVideoDriverDef virDomainVideoDriverDef;
+typedef virDomainVideoDriverDef *virDomainVideoDriverDefPtr;
+struct _virDomainVideoDriverDef {
+ virDomainVideoVgaconf vgaconf;
+};
+
struct _virDomainVideoDef {
int type;
unsigned int ram; /* kibibytes (multiples of 1024) */
@@ -1367,6 +1383,7 @@ struct _virDomainVideoDef {
unsigned int heads;
bool primary;
virDomainVideoAccelDefPtr accel;
+ virDomainVideoDriverDefPtr driver;
virDomainDeviceInfo info;
};
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e6901a8f1..40995533c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -525,6 +525,8 @@ virDomainVideoDefaultType;
virDomainVideoDefFree;
virDomainVideoTypeFromString;
virDomainVideoTypeToString;
+virDomainVideoVgaconfTypeFromString;
+virDomainVideoVgaconfTypeToString;
virDomainVirtTypeFromString;
virDomainVirtTypeToString;
virDomainWatchdogActionTypeFromString;