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@xxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
