Re: [libvirt] [PATCH v4 1/3] conf: Extend <loader/> and introduce <nvram/>

On 08/21/2014 02:50 AM, Michal Privoznik wrote: > Up to now, users can configure BIOS via the <loader/> element. With > the upcoming implementation of UEFI this is not enough as BIOS and > UEFI are conceptually different. For instance, while BIOS is ROM, UEFI > is programmable flash (although all writes to code section are > denied). Therefore we need new attribute @type which will > differentiate the two. Then, new attribute @readonly is introduced to > reflect the fact that some images are RO. > > Moreover, the OVMF (which is going to be used mostly), works in two > modes: > 1) Code and UEFI variable store is mixed in one file. > 2) Code and UEFI variable store is separated in two files > > The latter has advantage of updating the UEFI code without losing the > configuration. However, in order to represent the latter case we need > yet another XML element: <nvram/>. Currently, it has no additional > attributes, it's just a bare element containing path to the variable > store file. > > +++ b/docs/formatdomain.html.in > @@ -102,7 +102,8 @@ > ... > &lt;os&gt; > &lt;type&gt;hvm&lt;/type&gt; > - &lt;loader&gt;/usr/lib/xen/boot/hvmloader&lt;/loader&gt; > + &lt;loader readonly='on' type='rom'&gt;/usr/lib/xen/boot/hvmloader&lt;/loader&gt; readonly='yes' is a bit more typical of other XML constructs.

> + &lt;nvram&gt;/var/lib/libvirt/nvram/guest_VARS.fd&lt;/nvram&gt; You chose <nvram> to be a sibling, rather than a child, of <loader>. Is it legal to have <nvram> in isolation, or can it only appear when <loader> is present? If the former, then you are okay; if the latter, then I'd rather see it as a child than a sibling. > &lt;boot dev='hd'/&gt; > &lt;boot dev='cdrom'/&gt; > &lt;bootmenu enable='yes'/&gt; > @@ -129,7 +130,21 @@ > used to assist the domain creation process. It is used by Xen > fully virtualized domains as well as setting the QEMU BIOS file > path for QEMU/KVM domains. <span class="since">Xen since 0.1.0, > - QEMU/KVM since 0.9.12</span></dd> > + QEMU/KVM since 0.9.12</span> Then, <span class="since">Since s/Since/since/, because you are using it in the middle of a sentence > + 1.2.8</span> it's possible for the element to have two > + optional attributes: <code>readonly</code> (accepted values are > + <code>on</code> and <code>off</code>) to reflect the fact that the > + image should be writable or read-only. Again, yes/no might be more consistent than on/off > The second attribute > + <code>type</code> accepts values <code>rom</code> and > + <code>pflash</code>. It tells the hypervisor where in the guest > + memory the file should be mapped. For instance, if the loader > + path points to an UEFI image, <code>type</code> should be > + <code>pflash</code>.</dd> > + <dt><code>nvram</code></dt> > + <dd>Some UEFI firmwares may want to use a non-volatile memory to store > + some variables. In the host, this is represented as a file and the > + path to the file is stored in this element. <span class="since">Since > + 1.2.8</span></dd> Is this going to bite us in the future? What if we want to store the file on a networked device, such as via gluster or nbd? I'm wondering if: <nvram type='file'> <source file='/path/to/storage'/> </nvram> is a better representation, because that way, we could also do: <nvram type='network'> <source protocol='gluster'> ... <!-- all the stuff we do for <disk type='network'> --> </source> </nvram>

Also, by reusing a virStorageSourcePtr to store the nvram location, rather than limiting to just a file name, we can ensure we do proper SELinux labeling of the file, and allow the user the ability to overwrite what label we would otherwise generate.

> +++ b/docs/schemas/domaincommon.rng > @@ -242,6 +242,27 @@ > <interleave> > <optional> > <element name="loader"> > + <optional> > + <attribute name="readonly"> > + <choice> > + <value>on</value> > + <value>off</value> > + </choice> > + </attribute> > + </optional> > + <optional> > + <attribute name="type"> > + <choice> > + <value>rom</value> > + <value>pflash</value> > + </choice> > + </attribute> > + </optional> > + <ref name="absFilePath"/> > + </element> > + </optional> > + <optional> > + <element name="nvram"> > <ref name="absFilePath"/> This matches your docs, whether or not we decide to change the design. And at any rate, the existing <loader> element is just as hard-coded to a local filename as your new nvram, so if we add type='file' support, we'd want it on both places at once, so probably fine to defer that to a later day if someone actually needs it. So for now, I'm okay living with the design of just a string (but would still like to see the on/off changed to yes/no), since it's better to get OVMF usage enabled than to wait for a more complicated virStorageSource solution. > > +static int > +virDomainLoaderDefParseXML(xmlNodePtr node, > + virDomainLoaderDefPtr loader) > +{ > + int ret = -1; > + char *readonly_str = NULL; > + char *type_str = NULL; > + > + readonly_str = virXMLPropString(node, "readonly"); > + type_str = virXMLPropString(node, "type"); > + loader->path = (char *) xmlNodeGetContent(node); > + > + if (readonly_str && > + (loader->readonly = virTristateSwitchTypeFromString(readonly_str)) <= 0) { > + virReportError(VIR_ERR_XML_DETAIL, > + _("unknown readonly value: %s"), readonly_str); Oh, so the on/off decision was because we reused an existing common function! In that case, in the .rng, I think it would be nice to replace: <attribute name="readonly"> <choice> <value>on</value> <value>off</value> </choice> </attribute> with <attribute name="readonly"> <ref name="tristateSwitch"/> </attribute> I didn't check if we already have a define for on/off, and I'm fairly certain that if we do, it's not named tristateSwitch. But my point is that it would be nice to have ALL on/off clients go through the same define, instead of open-coding it. That way, if we ever change our minds and allow virTristateSwitchTypeFromString to handle synonyms, we only have to update one spot in the rng to accept both spelling variants of "on". On the other hand, doing that cleanup is a separate patch, if we are going to convert all existing tri-state users to reuse a common define.

> - def->os.loader = virXPathString("string(./os/loader[1])", ctxt); > + if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) { > + if (VIR_ALLOC(def->os.loader) < 0) > + goto error; > + > + if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0) > + goto error; > + > + def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); Ah - evidence that you only parse <nvram> if <loader> was present; and an argument in favor of my suggestion to make nvram a child rather than sibling.