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