8:33 a.m.
John Ferlan wrote:
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.
Thanks, I'll rebase the change.
Also, looking at f5384fb4, it looks like I can still reuse the <driver>
subelement because it seems it goes in line with the vga-related
configuration I'm adding.
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='?'.../>"
Yeah, I was planning to update the doc as a separate commit after this
one gets merged because I wasn't sure this schema change will go in as
is, so I don't have to re-roll the doc patches as well (yeah, I'm
lazy...)
> 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"?
Will fix this one.
> 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) {
<= ?
Hm, 0 seems to correspond to the first element in the enum, so "<="
should be valid. I'll double check.
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?
Yeah, that'll need to be fixed if it stays relevant after rebase.
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...
There's at least one thing I'd like to make controllable using this
<driver> element: screen resolution (width and height in pixels).
There's one more thing in bhyve that I'd like add support for: it
supports the 'wait' argument for video device so VM will start booting
only after clients connect to it via VNC. I'm not if this one fits the
video <driver> element very well though (probably should be part of
<graphics>?).
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
Actually, bhyve only supports 'gop', so I'm targeting this one
specifically. Though I guess it could be used for type='vga' as well
because there's nothing bhyve specific in 'vgaconf' as I can see.
> + 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;
>
Roman Bogorodskiy