On 10/03/2016 11:49 AM, Daniel P. Berrange wrote:
> Currently if you want to enable UEFI firmware for a guest
> you need to know about the hypervisor platform specific
> firmware path. This does not even work for all platforms,
> as hypervisors like VMWare don't expose UEFI as a path,
> they just have a direct config option for turning it on
> or off.
>
> This adds ability to use the much simpler:
>
> <loader firmware="uefi|bios"/>
>
> to choose the different firmware types.
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 9 +++++-
> docs/schemas/domaincommon.rng | 12 +++++++-
> src/conf/domain_conf.c | 70 ++++++++++++++++++++++++++++++++++++++-----
> src/conf/domain_conf.h | 11 +++++++
> 4 files changed, 93 insertions(+), 9 deletions(-)
>
The xml2xml tests from patch 3 could move here, but at least it exists..
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7008005..b8e9315 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -143,7 +143,14 @@
> <code>pflash</code>. Moreover, some firmwares may
> implement the Secure boot feature. Attribute
> <code>secure</code> can be used then to control it.
> - <span class="since">Since 2.1.0</span></dd>
> + <span class="since">Since 2.1.0</span>. The <code>firmware</code>
> + attribute can be used to request a specific type of firmware image
> + based on its common name, accepting the values <code>uefi</code>
> + and <code>bios</code>. <span class="since">Since 2.4.0</span>. When
> + <code>firmware</code> is set, it is not neccessary to provide any
necessary
> + path for the loader, nor set the <code>type</code> attribute or
> + <code>nvram</code> elements, as they will be automatically set
> + to the hypervisor specific default values.</dd>
So, if the firmware attribute isn't set, then is path is required? It
seems to only ever be defaulted in patch 3 in the post parse.
> <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
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 6eeb4e9..197b542 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -276,7 +276,17 @@
> </choice>
> </attribute>
> </optional>
> - <ref name="absFilePath"/>
> + <optional>
> + <attribute name="firmware">
> + <choice>
> + <value>bios</value>
> + <value>uefi</value>
> + </choice>
> + </attribute>
> + </optional>
> + <optional>
> + <ref name="absFilePath"/>
> + </optional>
> </element>
> </optional>
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7972a4e..054be94 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -835,6 +835,12 @@ VIR_ENUM_IMPL(virDomainLoader,
> "rom",
> "pflash")
>
> +VIR_ENUM_IMPL(virDomainLoaderFirmware,
> + VIR_DOMAIN_LOADER_FIRMWARE_LAST,
> + "default",
> + "bios",
> + "uefi")
> +
> /* Internal mapping: subset of block job types that can be present in
> * <mirror> XML (remaining types are not two-phase). */
> VIR_ENUM_DECL(virDomainBlockJob)
> @@ -15539,17 +15545,36 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> char *readonly_str = NULL;
> char *secure_str = NULL;
> char *type_str = NULL;
> + char *firmware_str = NULL;
>
> readonly_str = virXMLPropString(node, "readonly");
> secure_str = virXMLPropString(node, "secure");
> type_str = virXMLPropString(node, "type");
> + firmware_str = virXMLPropString(node, "firmware");
> loader->path = (char *) xmlNodeGetContent(node);
>
> - if (readonly_str &&
> - (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> - virReportError(VIR_ERR_XML_DETAIL,
> - _("unknown readonly value: %s"), readonly_str);
> - goto cleanup;
> + if (loader->path && STREQ(loader->path, ""))
> + VIR_FREE(loader->path);
STREQ_NULLABLE works too.
Is it 'undocumented' feature of having "" for path mean some sort of
default? It becomes more important in the next patch. But this does
follow the schema w/r/t path being optional I suppose.
ACK in principle - your call on the loader->path question.
John
> +
> + if (firmware_str) {
> + int firmware;
> + if ((firmware = virDomainLoaderFirmwareTypeFromString(firmware_str)) < 0) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("unknown firmware value: %s"), firmware_str);
> + goto cleanup;
> + }
> + loader->firmware = firmware;
> + }
> +
> + if (readonly_str) {
> + if ((loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("unknown readonly value: %s"), readonly_str);
> + goto cleanup;
> + }
> + } else {
> + if (loader->firmware == VIR_DOMAIN_LOADER_FIRMWARE_UEFI)
> + loader->readonly = VIR_TRISTATE_SWITCH_ON;
> }
>
> if (secure_str &&
> @@ -15567,6 +15592,29 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> goto cleanup;
> }
> loader->type = type;
> +
> + switch (loader->firmware) {
> + case VIR_DOMAIN_LOADER_FIRMWARE_UEFI:
> + if (loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("UEFI firmware must use pflash type"));
> + goto cleanup;
> + }
> + break;
> + case VIR_DOMAIN_LOADER_FIRMWARE_BIOS:
> + if (loader->type != VIR_DOMAIN_LOADER_TYPE_ROM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("BIOS firmware must use ROM type"));
> + goto cleanup;
> + }
> + break;
> + case VIR_DOMAIN_LOADER_FIRMWARE_DEFAULT:
> + case VIR_DOMAIN_LOADER_FIRMWARE_LAST:
> + break;
> + }
> + } else {
> + if (loader->firmware == VIR_DOMAIN_LOADER_FIRMWARE_UEFI)
> + loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> }
>
> ret = 0;
> @@ -15574,6 +15622,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> VIR_FREE(readonly_str);
> VIR_FREE(secure_str);
> VIR_FREE(type_str);
> + VIR_FREE(firmware_str);
> return ret;
> }
>
> @@ -22872,18 +22921,25 @@ virDomainLoaderDefFormat(virBufferPtr buf,
> const char *readonly = virTristateBoolTypeToString(loader->readonly);
> const char *secure = virTristateBoolTypeToString(loader->secure);
> const char *type = virDomainLoaderTypeToString(loader->type);
> + const char *firmware = virDomainLoaderFirmwareTypeToString(loader->firmware);
>
> virBufferAddLit(buf, "<loader");
>
> + if (loader->firmware)
> + virBufferAsprintf(buf, " firmware='%s'", firmware);
> +
> if (loader->readonly)
> virBufferAsprintf(buf, " readonly='%s'", readonly);
>
> if (loader->secure)
> virBufferAsprintf(buf, " secure='%s'", secure);
>
> - virBufferAsprintf(buf, " type='%s'>", type);
> + virBufferAsprintf(buf, " type='%s'", type);
>
> - virBufferEscapeString(buf, "%s</loader>\n", loader->path);
> + if (loader->path)
> + virBufferEscapeString(buf, ">%s</loader>\n", loader->path);
> + else
> + virBufferAddLit(buf, "/>\n");
> if (loader->nvram || loader->templt) {
> virBufferAddLit(buf, "<nvram");
> virBufferEscapeString(buf, " template='%s'", loader->templt);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 561a179..204c330 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1724,6 +1724,16 @@ struct _virDomainBIOSDef {
> };
>
> typedef enum {
> + VIR_DOMAIN_LOADER_FIRMWARE_DEFAULT = 0,
> + VIR_DOMAIN_LOADER_FIRMWARE_BIOS,
> + VIR_DOMAIN_LOADER_FIRMWARE_UEFI,
> +
> + VIR_DOMAIN_LOADER_FIRMWARE_LAST
> +} virDomainLoaderFirmware;
> +
> +VIR_ENUM_DECL(virDomainLoaderFirmware)
> +
> +typedef enum {
> VIR_DOMAIN_LOADER_TYPE_ROM = 0,
> VIR_DOMAIN_LOADER_TYPE_PFLASH,
>
> @@ -1735,6 +1745,7 @@ VIR_ENUM_DECL(virDomainLoader)
> typedef struct _virDomainLoaderDef virDomainLoaderDef;
> typedef virDomainLoaderDef *virDomainLoaderDefPtr;
> struct _virDomainLoaderDef {
> + virDomainLoaderFirmware firmware;
> char *path;
> int readonly; /* enum virTristateBool */
> virDomainLoader type;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list