On 22.08.2014 18:41, Eric Blake wrote:
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 @@
...
<os>
<type>hvm</type>
- <loader>/usr/lib/xen/boot/hvmloader</loader>
+ <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader>
readonly='yes' is a bit more typical of other XML constructs.
I didn't know that. Everything I found was just a bare <readonly/>
element. But okay, I'm fine with 'yes|no' too.
+ <nvram>/var/lib/libvirt/nvram/guest_VARS.fd</nvram>
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.
<boot dev='hd'/>
<boot dev='cdrom'/>
<bootmenu enable='yes'/>
@@ -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>
Who would ever want to store UEFI image/varstore on gluster? It's 2M in
total after all from which the nvram is 128KiB.
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.
It doesn't make much sense to me to have different security label on the
nvram file than the qemu process is going to have. Thus I don't think we
should even allow arbitrary labels here.
+++ 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.
Sure, but that would be a much greater cleanup. I think if this concrete
patch shows something, then it is that we should start treating "on|off"
and "yes|no" as synonyms. Yet again, that's different story that can be
patched later.
- 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.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list