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@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;