[libvirt] [PATCH v2 06/15] conf: Introduce firmware attribute to <os/>

Michal Privoznik posted 15 patches 6 years, 11 months ago
[libvirt] [PATCH v2 06/15] conf: Introduce firmware attribute to <os/>
Posted by Michal Privoznik 6 years, 11 months ago
The idea is that using this attribute users enable libvirt to
automagically select firmware image for their domain. For
instance:

  <os firmware='efi'>
    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
    <loader secure='no'/>
  </os>

  <os firmware='bios'>
    <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
  </os>

(The automagic of selecting firmware image will be described in
later commits.)

Accepted values are 'bios' and 'efi' to let libvirt select
corresponding type of firmware.

I know it is a good sign to introduce xml2xml test case when
changing XML config parser but that will have to come later.
Firmware auto selection is not enabled for any driver just yet so
any xml2xml test would fail right away.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/formatdomain.html.in     | 22 +++++++++-
 docs/schemas/domaincommon.rng |  8 ++++
 src/conf/domain_conf.c        | 75 ++++++++++++++++++++++++++++-------
 src/conf/domain_conf.h        | 12 ++++++
 src/libvirt_private.syms      |  2 +
 5 files changed, 103 insertions(+), 16 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index eb00c01d96..fedca0eedc 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -128,7 +128,7 @@
 
 <pre>
 ...
-&lt;os&gt;
+&lt;os firmware='uefi'&gt;
   &lt;type&gt;hvm&lt;/type&gt;
   &lt;loader readonly='yes' secure='no' type='rom'&gt;/usr/lib/xen/boot/hvmloader&lt;/loader&gt;
   &lt;nvram template='/usr/share/OVMF/OVMF_VARS.fd'&gt;/var/lib/libvirt/nvram/guest_VARS.fd&lt;/nvram&gt;
@@ -141,6 +141,26 @@
 ...</pre>
 
     <dl>
+      <dt><code>firmware</code></dt>
+      <dd>The <code>firmware</code> attribute allows management
+        applications to automatically fill <code>&lt;loader/&gt;</code>
+        and <code>&lt;nvram/&gt;</code> elements and possibly enable
+        some features required by selected firmware. Accepted values are
+        <code>bios</code> and <code>efi</code>.<br/>
+        The selection process scans for files describing installed
+        firmware images in specified location and uses the most specific
+        one which fulfils domain requirements. The locations in order of
+        preference (from generic to most specific one) are:
+        <ul>
+          <li><code>/usr/share/qemu/firmware</code></li>
+          <li><code>/etc/qemu/firmware</code></li>
+          <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li>
+        </ul>
+        For more information refer to firmware metadata specification as
+        described in <code>docs/interop/firmware.json</code> in QEMU
+        repository. Regular users do not need to bother.
+        <span class="since">Since 5.2.0 (QEMU and KVM only)</span>
+      </dd>
       <dt><code>type</code></dt>
       <dd>The content of the <code>type</code> element specifies the
         type of operating system to be booted in the virtual machine.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index c8d63c4912..87ba9daeda 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -256,6 +256,14 @@
     </optional>
     <element name="os">
       <interleave>
+        <optional>
+          <attribute name="firmware">
+            <choice>
+              <value>bios</value>
+              <value>efi</value>
+            </choice>
+          </attribute>
+        </optional>
         <ref name="ostypehvm"/>
         <optional>
           <element name="loader">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fbfdd57224..f92ebeb892 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1079,6 +1079,13 @@ VIR_ENUM_IMPL(virDomainHPTResizing,
               "required",
 );
 
+VIR_ENUM_IMPL(virDomainOsDefFirmware,
+              VIR_DOMAIN_OS_DEF_FIRMWARE_LAST,
+              "none",
+              "bios",
+              "efi",
+);
+
 /* Internal mapping: subset of block job types that can be present in
  * <mirror> XML (remaining types are not two-phase). */
 VIR_ENUM_DECL(virDomainBlockJob);
@@ -6608,14 +6615,23 @@ virDomainDefMemtuneValidate(const virDomainDef *def)
 
 
 static int
-virDomainDefOSValidate(const virDomainDef *def)
+virDomainDefOSValidate(const virDomainDef *def,
+                       virDomainXMLOptionPtr xmlopt)
 {
     if (!def->os.loader)
         return 0;
 
-    if (!def->os.loader->path) {
+    if (def->os.firmware &&
+        !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT)) {
         virReportError(VIR_ERR_XML_DETAIL, "%s",
-                       _("no loader path specified"));
+                       _("firmware auto selection not implemented for this driver"));
+        return -1;
+    }
+
+    if (!def->os.loader->path &&
+        def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE) {
+        virReportError(VIR_ERR_XML_DETAIL, "%s",
+                       _("no loader path specified and firmware auto selection disabled"));
         return -1;
     }
 
@@ -6624,7 +6640,8 @@ virDomainDefOSValidate(const virDomainDef *def)
 
 
 static int
-virDomainDefValidateInternal(const virDomainDef *def)
+virDomainDefValidateInternal(const virDomainDef *def,
+                             virDomainXMLOptionPtr xmlopt)
 {
     if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
         return -1;
@@ -6661,7 +6678,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
     if (virDomainDefMemtuneValidate(def) < 0)
         return -1;
 
-    if (virDomainDefOSValidate(def) < 0)
+    if (virDomainDefOSValidate(def, xmlopt) < 0)
         return -1;
 
     return 0;
@@ -6712,7 +6729,7 @@ virDomainDefValidate(virDomainDefPtr def,
                                            &data) < 0)
         return -1;
 
-    if (virDomainDefValidateInternal(def) < 0)
+    if (virDomainDefValidateInternal(def, xmlopt) < 0)
         return -1;
 
     return 0;
@@ -18271,19 +18288,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
 
 static int
 virDomainLoaderDefParseXML(xmlNodePtr node,
-                           virDomainLoaderDefPtr loader)
+                           virDomainLoaderDefPtr loader,
+                           bool fwAutoSelect)
 {
     VIR_AUTOFREE(char *) readonly_str = NULL;
     VIR_AUTOFREE(char *) secure_str = NULL;
     VIR_AUTOFREE(char *) type_str = NULL;
 
-    readonly_str = virXMLPropString(node, "readonly");
     secure_str = virXMLPropString(node, "secure");
-    type_str = virXMLPropString(node, "type");
-    loader->path = (char *) xmlNodeGetContent(node);
 
-    if (STREQ_NULLABLE(loader->path, ""))
-        VIR_FREE(loader->path);
+    if (!fwAutoSelect) {
+        readonly_str = virXMLPropString(node, "readonly");
+        type_str = virXMLPropString(node, "type");
+        loader->path = (char *) xmlNodeGetContent(node);
+        if (STREQ_NULLABLE(loader->path, ""))
+            VIR_FREE(loader->path);
+    }
 
     if (readonly_str &&
         (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
@@ -18678,6 +18698,7 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
         def->os.type == VIR_DOMAIN_OSTYPE_XENPVH ||
         def->os.type == VIR_DOMAIN_OSTYPE_HVM ||
         def->os.type == VIR_DOMAIN_OSTYPE_UML) {
+        VIR_AUTOFREE(char *) firmware = NULL;
         xmlNodePtr loader_node;
 
         def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt);
@@ -18685,15 +18706,35 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
         def->os.cmdline = virXPathString("string(./os/cmdline[1])", ctxt);
         def->os.dtb = virXPathString("string(./os/dtb[1])", ctxt);
         def->os.root = virXPathString("string(./os/root[1])", ctxt);
+
+        if (def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
+            (firmware = virXPathString("string(./os/@firmware)", ctxt))) {
+            int fw = virDomainOsDefFirmwareTypeFromString(firmware);
+
+            if (fw <= 0) {
+                virReportError(VIR_ERR_XML_ERROR,
+                               _("unknown firmware value %s"),
+                               firmware);
+                return -1;
+            }
+
+            def->os.firmware = fw;
+        }
+
         if ((loader_node = virXPathNode("./os/loader[1]", ctxt))) {
+            const bool fwAutoSelect = def->os.firmware != VIR_DOMAIN_OS_DEF_FIRMWARE_NONE;
+
             if (VIR_ALLOC(def->os.loader) < 0)
                 return -1;
 
-            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
+            if (virDomainLoaderDefParseXML(loader_node,
+                                           def->os.loader,
+                                           fwAutoSelect) < 0)
                 return -1;
 
             def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
-            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+            if (!fwAutoSelect)
+                def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
         }
     }
 
@@ -28082,7 +28123,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
                               def->os.bootloaderArgs);
     }
 
-    virBufferAddLit(buf, "<os>\n");
+    virBufferAddLit(buf, "<os");
+    if (def->os.firmware)
+        virBufferAsprintf(buf, " firmware='%s'",
+                          virDomainOsDefFirmwareTypeToString(def->os.firmware));
+    virBufferAddLit(buf, ">\n");
     virBufferAdjustIndent(buf, 2);
     virBufferAddLit(buf, "<type");
     if (def->os.arch)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ed5d00c399..fe9d4fb81a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2005,10 +2005,21 @@ struct _virDomainOSEnv {
     char *value;
 };
 
+typedef enum {
+    VIR_DOMAIN_OS_DEF_FIRMWARE_NONE = 0,
+    VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS,
+    VIR_DOMAIN_OS_DEF_FIRMWARE_EFI,
+
+    VIR_DOMAIN_OS_DEF_FIRMWARE_LAST
+} virDomainOsDefFirmware;
+
+VIR_ENUM_DECL(virDomainOsDefFirmware);
+
 typedef struct _virDomainOSDef virDomainOSDef;
 typedef virDomainOSDef *virDomainOSDefPtr;
 struct _virDomainOSDef {
     int type;
+    virDomainOsDefFirmware firmware;
     virArch arch;
     char *machine;
     size_t nBootDevs;
@@ -2729,6 +2740,7 @@ typedef enum {
     VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4),
     VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
     VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER = (1 << 6),
+    VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT = (1 << 7),
 } virDomainDefFeatures;
 
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2a240fc7a..e8bb0d8bc4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -509,6 +509,8 @@ virDomainObjTaint;
 virDomainObjUpdateModificationImpact;
 virDomainObjWait;
 virDomainObjWaitUntil;
+virDomainOsDefFirmwareTypeFromString;
+virDomainOsDefFirmwareTypeToString;
 virDomainOSTypeFromString;
 virDomainOSTypeToString;
 virDomainParseMemory;
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] conf: Introduce firmware attribute to <os/>
Posted by Daniel P. Berrangé 6 years, 11 months ago
On Thu, Mar 07, 2019 at 10:29:16AM +0100, Michal Privoznik wrote:
> The idea is that using this attribute users enable libvirt to
> automagically select firmware image for their domain. For
> instance:
> 
>   <os firmware='efi'>
>     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
>     <loader secure='no'/>

Ah, now I see why you need <loader> without a path
specified. We need to be able to set the secure
attribute. 

>   </os>
> 
>   <os firmware='bios'>
>     <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
>   </os>
> 
> (The automagic of selecting firmware image will be described in
> later commits.)
> 
> Accepted values are 'bios' and 'efi' to let libvirt select
> corresponding type of firmware.
> 
> I know it is a good sign to introduce xml2xml test case when
> changing XML config parser but that will have to come later.
> Firmware auto selection is not enabled for any driver just yet so
> any xml2xml test would fail right away.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/formatdomain.html.in     | 22 +++++++++-
>  docs/schemas/domaincommon.rng |  8 ++++
>  src/conf/domain_conf.c        | 75 ++++++++++++++++++++++++++++-------
>  src/conf/domain_conf.h        | 12 ++++++
>  src/libvirt_private.syms      |  2 +
>  5 files changed, 103 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index eb00c01d96..fedca0eedc 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -128,7 +128,7 @@
>  
>  <pre>
>  ...
> -&lt;os&gt;
> +&lt;os firmware='uefi'&gt;
>    &lt;type&gt;hvm&lt;/type&gt;
>    &lt;loader readonly='yes' secure='no' type='rom'&gt;/usr/lib/xen/boot/hvmloader&lt;/loader&gt;
>    &lt;nvram template='/usr/share/OVMF/OVMF_VARS.fd'&gt;/var/lib/libvirt/nvram/guest_VARS.fd&lt;/nvram&gt;
> @@ -141,6 +141,26 @@
>  ...</pre>
>  
>      <dl>
> +      <dt><code>firmware</code></dt>
> +      <dd>The <code>firmware</code> attribute allows management
> +        applications to automatically fill <code>&lt;loader/&gt;</code>
> +        and <code>&lt;nvram/&gt;</code> elements and possibly enable
> +        some features required by selected firmware. Accepted values are
> +        <code>bios</code> and <code>efi</code>.<br/>

NB, this firmware attribute is something we'll want in the VMWare
driver too.  So the text from this point downwards should be explicitly
noted as being specific to QEMU. Or maybe this text about search locations
should just be in the drvqemu.html.in file, and linked from here ?

> +        The selection process scans for files describing installed
> +        firmware images in specified location and uses the most specific
> +        one which fulfils domain requirements. The locations in order of
> +        preference (from generic to most specific one) are:
> +        <ul>
> +          <li><code>/usr/share/qemu/firmware</code></li>
> +          <li><code>/etc/qemu/firmware</code></li>
> +          <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li>
> +        </ul>
> +        For more information refer to firmware metadata specification as
> +        described in <code>docs/interop/firmware.json</code> in QEMU
> +        repository. Regular users do not need to bother.
> +        <span class="since">Since 5.2.0 (QEMU and KVM only)</span>
> +      </dd>
>        <dt><code>type</code></dt>
>        <dd>The content of the <code>type</code> element specifies the
>          type of operating system to be booted in the virtual machine.


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/15] conf: Introduce firmware attribute to <os/>
Posted by Michal Privoznik 6 years, 11 months ago
On 3/11/19 4:04 PM, Daniel P. Berrangé wrote:
> On Thu, Mar 07, 2019 at 10:29:16AM +0100, Michal Privoznik wrote:
>> The idea is that using this attribute users enable libvirt to
>> automagically select firmware image for their domain. For
>> instance:
>>
>>    <os firmware='efi'>
>>      <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
>>      <loader secure='no'/>
> 
> Ah, now I see why you need <loader> without a path
> specified. We need to be able to set the secure
> attribute.
> 
>>    </os>
>>
>>    <os firmware='bios'>
>>      <type arch='x86_64' machine='pc-q35-4.0'>hvm</type>
>>    </os>
>>
>> (The automagic of selecting firmware image will be described in
>> later commits.)
>>
>> Accepted values are 'bios' and 'efi' to let libvirt select
>> corresponding type of firmware.
>>
>> I know it is a good sign to introduce xml2xml test case when
>> changing XML config parser but that will have to come later.
>> Firmware auto selection is not enabled for any driver just yet so
>> any xml2xml test would fail right away.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   docs/formatdomain.html.in     | 22 +++++++++-
>>   docs/schemas/domaincommon.rng |  8 ++++
>>   src/conf/domain_conf.c        | 75 ++++++++++++++++++++++++++++-------
>>   src/conf/domain_conf.h        | 12 ++++++
>>   src/libvirt_private.syms      |  2 +
>>   5 files changed, 103 insertions(+), 16 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index eb00c01d96..fedca0eedc 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -128,7 +128,7 @@
>>   
>>   <pre>
>>   ...
>> -&lt;os&gt;
>> +&lt;os firmware='uefi'&gt;
>>     &lt;type&gt;hvm&lt;/type&gt;
>>     &lt;loader readonly='yes' secure='no' type='rom'&gt;/usr/lib/xen/boot/hvmloader&lt;/loader&gt;
>>     &lt;nvram template='/usr/share/OVMF/OVMF_VARS.fd'&gt;/var/lib/libvirt/nvram/guest_VARS.fd&lt;/nvram&gt;
>> @@ -141,6 +141,26 @@
>>   ...</pre>
>>   
>>       <dl>
>> +      <dt><code>firmware</code></dt>
>> +      <dd>The <code>firmware</code> attribute allows management
>> +        applications to automatically fill <code>&lt;loader/&gt;</code>
>> +        and <code>&lt;nvram/&gt;</code> elements and possibly enable
>> +        some features required by selected firmware. Accepted values are
>> +        <code>bios</code> and <code>efi</code>.<br/>
> 
> NB, this firmware attribute is something we'll want in the VMWare
> driver too.  So the text from this point downwards should be explicitly
> noted as being specific to QEMU. Or maybe this text about search locations
> should just be in the drvqemu.html.in file, and linked from here ?

I'll go with the former. When implementing this for VMWare it can be 
moved to drvqemu. BTW why do we need this for VMWare?

> 
>> +        The selection process scans for files describing installed
>> +        firmware images in specified location and uses the most specific
>> +        one which fulfils domain requirements. The locations in order of
>> +        preference (from generic to most specific one) are:
>> +        <ul>
>> +          <li><code>/usr/share/qemu/firmware</code></li>
>> +          <li><code>/etc/qemu/firmware</code></li>
>> +          <li><code>$XDG_CONFIG_HOME/qemu/firmware</code></li>
>> +        </ul>
>> +        For more information refer to firmware metadata specification as
>> +        described in <code>docs/interop/firmware.json</code> in QEMU
>> +        repository. Regular users do not need to bother.
>> +        <span class="since">Since 5.2.0 (QEMU and KVM only)</span>
>> +      </dd>
>>         <dt><code>type</code></dt>
>>         <dd>The content of the <code>type</code> element specifies the
>>           type of operating system to be booted in the virtual machine.
> 
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list