[PATCH v2 0/4] [for-10.0] hw/uefi: some bugfixes

[PATCH v2 0/4] [for-10.0] hw/uefi: some bugfixes

[Gerd Hoffmann]

Gerd Hoffmann (4):
  hw/uefi: flush variable store to disk in post load
  hw/uefi: fix error handling in uefi_vars_json_save
  hw/uefi: fix error handling in uefi_vars_json_load
  docs/firmware: add feature flag for qemu variable store

 hw/uefi/var-service-core.c |  1 +
 hw/uefi/var-service-json.c | 24 +++++++++++++++++++-----
 docs/interop/firmware.json |  5 ++++-
 3 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH v2 1/4] hw/uefi: flush variable store to disk in post load

[Gerd Hoffmann]

Makes live migration more robust.  Commit 4c0cfc72b31a ("pflash_cfi01:
write flash contents to bdrv on incoming migration") elaborates in
detail on the motivation.

Cc: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/uefi/var-service-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/uefi/var-service-core.c b/hw/uefi/var-service-core.c
index 8ed8378ab991..4836a0cb8116 100644
--- a/hw/uefi/var-service-core.c
+++ b/hw/uefi/var-service-core.c
@@ -29,6 +29,7 @@ static int uefi_vars_post_load(void *opaque, int version_id)
     uefi_vars_state *uv = opaque;
 
     uefi_vars_update_storage(uv);
+    uefi_vars_json_save(uv);
     uv->buffer = g_malloc(uv->buf_size);
     return 0;
 }
-- 
2.48.1

[PATCH v2 2/4] hw/uefi: fix error handling in uefi_vars_json_save

[Gerd Hoffmann]

Catch lseek errors.  Return on errors.
Use autoptr for the GString to simplify cleanup.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/uefi/var-service-json.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/uefi/var-service-json.c b/hw/uefi/var-service-json.c
index 761082c11fc1..f1c20a6b8c1e 100644
--- a/hw/uefi/var-service-json.c
+++ b/hw/uefi/var-service-json.c
@@ -178,7 +178,7 @@ void uefi_vars_json_init(uefi_vars_state *uv, Error **errp)
 
 void uefi_vars_json_save(uefi_vars_state *uv)
 {
-    GString *gstr;
+    g_autoptr(GString) gstr = NULL;
     int rc;
 
     if (uv->jsonfd == -1) {
@@ -187,18 +187,25 @@ void uefi_vars_json_save(uefi_vars_state *uv)
 
     gstr = uefi_vars_to_json(uv);
 
-    lseek(uv->jsonfd, 0, SEEK_SET);
+    rc = lseek(uv->jsonfd, 0, SEEK_SET);
+    if (rc < 0) {
+        warn_report("%s: lseek error", __func__);
+        return;
+    }
+
     rc = ftruncate(uv->jsonfd, 0);
     if (rc != 0) {
         warn_report("%s: ftruncate error", __func__);
+        return;
     }
+
     rc = write(uv->jsonfd, gstr->str, gstr->len);
     if (rc != gstr->len) {
         warn_report("%s: write error", __func__);
+        return;
     }
+
     fsync(uv->jsonfd);
-
-    g_string_free(gstr, true);
 }
 
 void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
-- 
2.48.1

[PATCH v2 3/4] hw/uefi: fix error handling in uefi_vars_json_load

[Gerd Hoffmann]

Catch lseek errors.  Return on read errors.

Fixes: CID 1593154
Fixes: CID 1593157
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/uefi/var-service-json.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/uefi/var-service-json.c b/hw/uefi/var-service-json.c
index f1c20a6b8c1e..ad3462cd1557 100644
--- a/hw/uefi/var-service-json.c
+++ b/hw/uefi/var-service-json.c
@@ -214,7 +214,7 @@ void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
     QObject *qobj;
     Visitor *v;
     char *str;
-    size_t len;
+    ssize_t len;
     int rc;
 
     if (uv->jsonfd == -1) {
@@ -222,7 +222,12 @@ void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
     }
 
     len = lseek(uv->jsonfd, 0, SEEK_END);
+    if (len < 0) {
+        warn_report("%s: lseek error", __func__);
+        return;
+    }
     if (len == 0) {
+        /* empty file */
         return;
     }
 
@@ -231,6 +236,8 @@ void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
     rc = read(uv->jsonfd, str, len);
     if (rc != len) {
         warn_report("%s: read error", __func__);
+        g_free(str);
+        return;
     }
     str[len] = 0;
 
-- 
2.48.1

[PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/interop/firmware.json | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 57f55f6c5455..76df1043dae9 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -214,13 +214,16 @@
 #                  PL011 UART. @verbose-static is mutually exclusive
 #                  with @verbose-dynamic.
 #
+# @qemu-vars: The firmware expects qemu to provide an efi variable
+#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.
+#
 # Since: 3.0
 ##
 { 'enum' : 'FirmwareFeature',
   'data' : [ 'acpi-s3', 'acpi-s4',
              'amd-sev', 'amd-sev-es', 'amd-sev-snp',
              'intel-tdx',
-             'enrolled-keys', 'requires-smm', 'secure-boot',
+             'enrolled-keys', 'requires-smm', 'secure-boot', 'qemu-vars',
              'verbose-dynamic', 'verbose-static' ] }
 
 ##
-- 
2.48.1

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Daniel P. Berrangé]

On Wed, Mar 19, 2025 at 12:01:51PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/interop/firmware.json | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> index 57f55f6c5455..76df1043dae9 100644
> --- a/docs/interop/firmware.json
> +++ b/docs/interop/firmware.json
> @@ -214,13 +214,16 @@
>  #                  PL011 UART. @verbose-static is mutually exclusive
>  #                  with @verbose-dynamic.
>  #
> +# @qemu-vars: The firmware expects qemu to provide an efi variable
> +#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.

It seems like this would imply mapping.device == memory, as if we had
mapping.device == flash, then we would need to extend FirmwareFlashMode
with an extra option ? If so, lets document this expectation.

> +#
>  # Since: 3.0
>  ##
>  { 'enum' : 'FirmwareFeature',
>    'data' : [ 'acpi-s3', 'acpi-s4',
>               'amd-sev', 'amd-sev-es', 'amd-sev-snp',
>               'intel-tdx',
> -             'enrolled-keys', 'requires-smm', 'secure-boot',
> +             'enrolled-keys', 'requires-smm', 'secure-boot', 'qemu-vars',
>               'verbose-dynamic', 'verbose-static' ] }
>  
>  ##
> -- 
> 2.48.1
> 

With 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 :|

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Gerd Hoffmann]

On Wed, Mar 19, 2025 at 11:07:05AM +0000, Daniel P. Berrangé wrote:
> On Wed, Mar 19, 2025 at 12:01:51PM +0100, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  docs/interop/firmware.json | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> > index 57f55f6c5455..76df1043dae9 100644
> > --- a/docs/interop/firmware.json
> > +++ b/docs/interop/firmware.json
> > @@ -214,13 +214,16 @@
> >  #                  PL011 UART. @verbose-static is mutually exclusive
> >  #                  with @verbose-dynamic.
> >  #
> > +# @qemu-vars: The firmware expects qemu to provide an efi variable
> > +#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.
> 
> It seems like this would imply mapping.device == memory,

edk2 doesn't care if you load it into flash or memory, both cases will
work fine.  Using flash if we don't actually need it makes things more
complicated for no good reason, so yes, I'd go write config files with
mapping.device == memory.

> as if we had
> mapping.device == flash, then we would need to extend FirmwareFlashMode
> with an extra option ?

There is 'stateless' already for 'firmware image in r/o flash'.

take care,
  Gerd

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Daniel P. Berrangé]

On Wed, Mar 19, 2025 at 12:30:34PM +0100, Gerd Hoffmann wrote:
> On Wed, Mar 19, 2025 at 11:07:05AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Mar 19, 2025 at 12:01:51PM +0100, Gerd Hoffmann wrote:
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  docs/interop/firmware.json | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> > > index 57f55f6c5455..76df1043dae9 100644
> > > --- a/docs/interop/firmware.json
> > > +++ b/docs/interop/firmware.json
> > > @@ -214,13 +214,16 @@
> > >  #                  PL011 UART. @verbose-static is mutually exclusive
> > >  #                  with @verbose-dynamic.
> > >  #
> > > +# @qemu-vars: The firmware expects qemu to provide an efi variable
> > > +#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.

I wonder if 'qemu-vars' is the right name here ? It feels like the specification
for such device is effectively defined by UEFI, with any hypervisor providing a
impl. Perhaps just call it 'uefi-vars-dev' or some name that's relevant for
what EDK2 calls it ?

> > 
> > It seems like this would imply mapping.device == memory,
> 
> edk2 doesn't care if you load it into flash or memory, both cases will
> work fine.  Using flash if we don't actually need it makes things more
> complicated for no good reason, so yes, I'd go write config files with
> mapping.device == memory.
> 
> > as if we had
> > mapping.device == flash, then we would need to extend FirmwareFlashMode
> > with an extra option ?
> 
> There is 'stateless' already for 'firmware image in r/o flash'.

What's the behaviour of UEFI if build with JSON vars support, but without
QEMU providing any JSON vars backend ?  If that happily runs stateless
in that case, then we could reuse the existing 'stateless' mode, without
having compat trouble with older libvirt that don't know about the
'qemu-vars' feature.

We would want to expand the 'stateless' docs to mention that this feature
flag indicates optional support for persistence in that case.

With 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 :|

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Gerd Hoffmann]

On Wed, Mar 19, 2025 at 11:37:40AM +0000, Daniel P. Berrangé wrote:

> > > > +# @qemu-vars: The firmware expects qemu to provide an efi variable
> > > > +#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.
> 
> I wonder if 'qemu-vars' is the right name here ? It feels like the specification
> for such device is effectively defined by UEFI, with any hypervisor providing a
> impl. Perhaps just call it 'uefi-vars-dev' or some name that's relevant for
> what EDK2 calls it ?

'host-uefi-vars' maybe?  Or 'vmm-uefi-vars'?

> > There is 'stateless' already for 'firmware image in r/o flash'.
> 
> What's the behaviour of UEFI if build with JSON vars support, but without
> QEMU providing any JSON vars backend ?

It will panic.

> We would want to expand the 'stateless' docs to mention that this feature
> flag indicates optional support for persistence in that case.

Optional is not possible do due to the way variable store support is
organized in edk2.  Firmware can't switch between smm and non-smm
configuration at runtime for similar reasons.

take care,
  Gerd

Re: [PATCH v2 1/4] hw/uefi: flush variable store to disk in post load

[Philippe Mathieu-Daudé]

On 19/3/25 12:01, Gerd Hoffmann wrote:
> Makes live migration more robust.

Typo "Make"

>  Commit 4c0cfc72b31a ("pflash_cfi01:
> write flash contents to bdrv on incoming migration") elaborates in
> detail on the motivation.
> 
> Cc: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/uefi/var-service-core.c | 1 +
>   1 file changed, 1 insertion(+)

Re: [PATCH v2 2/4] hw/uefi: fix error handling in uefi_vars_json_save

[Philippe Mathieu-Daudé]

On 19/3/25 12:01, Gerd Hoffmann wrote:
> Catch lseek errors.  Return on errors.
> Use autoptr for the GString to simplify cleanup.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/uefi/var-service-json.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 3/4] hw/uefi: fix error handling in uefi_vars_json_load

[Philippe Mathieu-Daudé]

On 19/3/25 12:01, Gerd Hoffmann wrote:
> Catch lseek errors.  Return on read errors.
> 
> Fixes: CID 1593154
> Fixes: CID 1593157
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/uefi/var-service-json.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Daniel P. Berrangé]

On Wed, Mar 19, 2025 at 12:51:16PM +0100, Gerd Hoffmann wrote:
> On Wed, Mar 19, 2025 at 11:37:40AM +0000, Daniel P. Berrangé wrote:
> 
> > > > > +# @qemu-vars: The firmware expects qemu to provide an efi variable
> > > > > +#             store, via "uefi-vars-sysbus" or "uefi-vars-x64" device.
> > 
> > I wonder if 'qemu-vars' is the right name here ? It feels like the specification
> > for such device is effectively defined by UEFI, with any hypervisor providing a
> > impl. Perhaps just call it 'uefi-vars-dev' or some name that's relevant for
> > what EDK2 calls it ?
> 
> 'host-uefi-vars' maybe?  Or 'vmm-uefi-vars'?
> 
> > > There is 'stateless' already for 'firmware image in r/o flash'.
> > 
> > What's the behaviour of UEFI if build with JSON vars support, but without
> > QEMU providing any JSON vars backend ?
> 
> It will panic.

In that case, we must not reuse 'stateless' with such builds, as that's
quite different semantics & incompatible with current usage.

We would need a new 'uefi-vars' mode, or just declare that we must
always use 'memory'  not 'flash' for such builds.

> 
> > We would want to expand the 'stateless' docs to mention that this feature
> > flag indicates optional support for persistence in that case.
> 
> Optional is not possible do due to the way variable store support is
> organized in edk2.  Firmware can't switch between smm and non-smm
> configuration at runtime for similar reasons.
> 
> take care,
>   Gerd
> 

With 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 :|

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Gerd Hoffmann]

  Hi,

> > > > There is 'stateless' already for 'firmware image in r/o flash'.
> > > 
> > > What's the behaviour of UEFI if build with JSON vars support, but without
> > > QEMU providing any JSON vars backend ?
> > 
> > It will panic.
> 
> In that case, we must not reuse 'stateless' with such builds, as that's
> quite different semantics & incompatible with current usage.
> 
> We would need a new 'uefi-vars' mode, or just declare that we must
> always use 'memory'  not 'flash' for such builds.

I don't see how 'flash.stateless' is any different than 'memory' (or
'kernel', or maybe 'igvm' some day).  If the 'host-uefi-vars' feature
is present '-device uefi-vars-$kind' is required, no matter how you
load the firmware.

What exactly will an old libvirt which does not know the
'host-uefi-vars' feature do in case it finds a firmware.json file with
that feature?  Play safe and ignore the file?

take care,
  Gerd

Re: [PATCH v2 4/4] docs/firmware: add feature flag for qemu variable store

[Daniel P. Berrangé]

On Wed, Mar 19, 2025 at 02:03:09PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > > > There is 'stateless' already for 'firmware image in r/o flash'.
> > > > 
> > > > What's the behaviour of UEFI if build with JSON vars support, but without
> > > > QEMU providing any JSON vars backend ?
> > > 
> > > It will panic.
> > 
> > In that case, we must not reuse 'stateless' with such builds, as that's
> > quite different semantics & incompatible with current usage.
> > 
> > We would need a new 'uefi-vars' mode, or just declare that we must
> > always use 'memory'  not 'flash' for such builds.
> 
> I don't see how 'flash.stateless' is any different than 'memory' (or
> 'kernel', or maybe 'igvm' some day).  If the 'host-uefi-vars' feature
> is present '-device uefi-vars-$kind' is required, no matter how you
> load the firmware.
> 
> What exactly will an old libvirt which does not know the
> 'host-uefi-vars' feature do in case it finds a firmware.json file with
> that feature?  Play safe and ignore the file?

No, unknown features are simply ignored. 

With 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 :|