[Qemu-devel] [PATCH v2 00/18] fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy

[Qemu-devel] [PATCH v2 00/18] fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy

[Philippe Mathieu-Daudé]

Hi,

This series consists of:
- trivial cleanups, add trace events (was in v1)
- add QMP/HMP commands to display the list of fw_cfg entries (reworked from v1)
- add unrealize() method and deallocate various buffers (new in v2)
- add fw_cfg_add_file_from_host (was in v1)
- add edk2_add_host_crypto_policy (new in v2)

Since v1:
- Addressed Michael and Laszlo comments.

Please review,

Phil.

v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html

Philippe Mathieu-Daudé (18):
  hw/arm/virt: Remove null-check in virt_build_smbios()
  hw/i386: Remove unused include
  cutils: Add qemu_strdup_hexlify() and qemu_strdup_unhexlify()
  hw/nvram/fw_cfg: Add trace events
  hw/nvram/fw_cfg: Use the ldst API
  hw/nvram/fw_cfg: Remove the unnecessary boot_splash_filedata_size
  hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()
  hw/nvram/fw_cfg: Move fw_cfg_file_slots_allocate() to common_realize()
  hw/nvram/fw_cfg: Free file_slots in common_unrealize()
  hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState
  hw/nvram/fw_cfg: Add boot_splash.time_le16 to FWCfgState
  hw/nvram/fw_cfg: Keep reference of file_data in FWCfgState
  hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command
  hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command
  hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()
  hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()
  hw/i386: Use edk2_add_host_crypto_policy()
  hw/arm/virt: Use edk2_add_host_crypto_policy()

 MAINTAINERS                             |   9 +
 hmp-commands-info.hx                    |  17 ++
 hw/Makefile.objs                        |   1 +
 hw/acpi/piix4.c                         |   1 -
 hw/arm/virt.c                           |  11 +-
 hw/firmware/Makefile.objs               |   1 +
 hw/firmware/uefi_edk2_crypto_policies.c | 166 ++++++++++++++
 hw/i386/pc.c                            |  28 +++
 hw/nvram/fw_cfg.c                       | 284 ++++++++++++++++++++++--
 hw/nvram/trace-events                   |   7 +-
 hw/ppc/Makefile.objs                    |   2 +-
 hw/ppc/fw_cfg.c                         |  31 +++
 hw/sparc/sun4m.c                        |  19 ++
 hw/sparc64/sun4u.c                      |  19 ++
 include/hw/firmware/uefi_edk2.h         |  28 +++
 include/hw/nvram/fw_cfg.h               |  42 ++++
 include/qemu/cutils.h                   |  33 +++
 include/sysemu/sysemu.h                 |   2 -
 qapi/misc.json                          |  44 ++++
 stubs/Makefile.objs                     |   1 +
 stubs/fw_cfg.c                          |  19 ++
 util/cutils.c                           |  55 +++++
 vl.c                                    |  10 -
 23 files changed, 792 insertions(+), 38 deletions(-)
 create mode 100644 hw/firmware/Makefile.objs
 create mode 100644 hw/firmware/uefi_edk2_crypto_policies.c
 create mode 100644 hw/ppc/fw_cfg.c
 create mode 100644 include/hw/firmware/uefi_edk2.h
 create mode 100644 stubs/fw_cfg.c

-- 
2.20.1

[Qemu-devel] [PATCH v2 01/18] hw/arm/virt: Remove null-check in virt_build_smbios()

[Philippe Mathieu-Daudé]

Since commit 578f3c7b0835 ("arm: add fw_cfg to "virt" board",
2014-12-22), the machvirt_init() unconditionally creates the
fw_cfg object.  Later, commit c30e15658b1b ("smbios: implement
smbios support for mach-virt", 2015-09-07) added a superfluous
null-check on it.
Remove this superfluous check.

Fixes: c30e15658b1b
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Corrected commit reference (Laszlo)
---
 hw/arm/virt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c7fb5348ae..bb7255a080 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1282,10 +1282,6 @@ static void virt_build_smbios(VirtMachineState *vms)
     size_t smbios_tables_len, smbios_anchor_len;
     const char *product = "QEMU Virtual Machine";
 
-    if (!vms->fw_cfg) {
-        return;
-    }
-
     if (kvm_enabled()) {
         product = "KVM Virtual Machine";
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 01/18] hw/arm/virt: Remove null-check in virt_build_smbios()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Since commit 578f3c7b0835 ("arm: add fw_cfg to "virt" board",
> 2014-12-22), the machvirt_init() unconditionally creates the
> fw_cfg object.  Later, commit c30e15658b1b ("smbios: implement
> smbios support for mach-virt", 2015-09-07) added a superfluous
> null-check on it.
> Remove this superfluous check.
>
> Fixes: c30e15658b1b

I'd use 'Fixes:' only for bug fixes.  This is a cleanup

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v2 02/18] hw/i386: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Drop files that do use fw_cfg (Michael):
- hw/i386/acpi-build.c
- hw/i386/pc.c
---
 hw/acpi/piix4.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8fd25a5926..7b98121070 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -28,7 +28,6 @@
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qemu/range.h"
-#include "hw/nvram/fw_cfg.h"
 #include "exec/address-spaces.h"
 #include "hw/acpi/piix4.h"
 #include "hw/acpi/pcihp.h"
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 02/18] hw/i386: Remove unused include

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop files that do use fw_cfg (Michael):
> - hw/i386/acpi-build.c
> - hw/i386/pc.c
> ---
>  hw/acpi/piix4.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 8fd25a5926..7b98121070 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -28,7 +28,6 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/error.h"
>  #include "qemu/range.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "exec/address-spaces.h"
>  #include "hw/acpi/piix4.h"
>  #include "hw/acpi/pcihp.h"
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 02/18] hw/i386: Remove unused include

[Thomas Huth]

On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop files that do use fw_cfg (Michael):
> - hw/i386/acpi-build.c
> - hw/i386/pc.c
> ---
>  hw/acpi/piix4.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 8fd25a5926..7b98121070 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -28,7 +28,6 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/error.h"
>  #include "qemu/range.h"
> -#include "hw/nvram/fw_cfg.h"
>  #include "exec/address-spaces.h"
>  #include "hw/acpi/piix4.h"
>  #include "hw/acpi/pcihp.h"
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [Qemu-trivial] [Qemu-ppc] [PATCH v2 02/18] hw/i386: Remove unused include

[Laurent Vivier]

On 08/03/2019 12:32, Thomas Huth wrote:
> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v2: Drop files that do use fw_cfg (Michael):
>> - hw/i386/acpi-build.c
>> - hw/i386/pc.c
>> ---
>>  hw/acpi/piix4.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 8fd25a5926..7b98121070 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -28,7 +28,6 @@
>>  #include "sysemu/sysemu.h"
>>  #include "qapi/error.h"
>>  #include "qemu/range.h"
>> -#include "hw/nvram/fw_cfg.h"
>>  #include "exec/address-spaces.h"
>>  #include "hw/acpi/piix4.h"
>>  #include "hw/acpi/pcihp.h"
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

[Qemu-devel] [PATCH v2 03/18] cutils: Add qemu_strdup_hexlify() and qemu_strdup_unhexlify()

[Philippe Mathieu-Daudé]

Add two helpers: one to represent a binary data as a string of
hexadecimal values, and one to restore a such string into its
original binary data.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/cutils.h | 33 ++++++++++++++++++++++++++
 util/cutils.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index d2dad3057c..375a5508b0 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -171,6 +171,39 @@ bool test_buffer_is_zero_next_accel(void);
 int uleb128_encode_small(uint8_t *out, uint32_t n);
 int uleb128_decode_small(const uint8_t *in, uint32_t *n);
 
+/**
+ * qemu_strdup_hexlify:
+ *
+ * Encode a sequence of binary data into its hexadecimal stringified
+ * representation.
+ *
+ * @ptr: Buffer to hexlify
+ * @size: Length of the buffer
+ *
+ * Use qemu_strdup_unhexlify() to convert the hex string to original data.
+ *
+ * Returns: A newly allocated, zero-terminated hex encoded string representing
+ * the data. The returned string must be freed with g_free().
+ */
+gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize size);
+
+/**
+ * qemu_strdup_unhexlify:
+ *
+ * Decode a sequence of hexadecimal encoded text into binary data.
+ *
+ * @hex_string: String to unhexlify
+ * @out_size: if not NULL: gsize to be written with the data length
+ *
+ * This function is the opposite of qemu_strdup_hexlify().
+ *
+ * Returns: A newly allocated buffer containing the binary data that text
+ * represents. The returned buffer must be freed with g_free().
+ * Note that the returned binary data is not necessarily zero-terminated,
+ * so it should not be used as a character string.
+ */
+gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size);
+
 /**
  * qemu_pstrcmp0:
  * @str1: a non-NULL pointer to a C string (*str1 can be NULL)
diff --git a/util/cutils.c b/util/cutils.c
index e098debdc0..bf324c0d8b 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -779,6 +779,61 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n)
     }
 }
 
+static guchar hexval(const gchar v)
+{
+    switch (v) {
+    case '0' ... '9':
+        return v - '0';
+    case 'A' ... 'F':
+        return v - 'A' + 10;
+    case 'a' ... 'f':
+        return v - 'a' + 10;
+    default:
+        return 0;
+    }
+}
+
+gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize len)
+{
+    guchar *data = (guchar *)ptr;
+    gchar *hex_string;
+
+    if (!ptr || !len) {
+        return g_strdup("");
+    }
+
+    hex_string = g_malloc(2 * len + 1);
+    for (gsize i = 0; i < len; i++) {
+        g_snprintf(&hex_string[2 * i], 3, "%02x", data[i]);
+    }
+
+    return hex_string;
+}
+
+gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size)
+{
+    size_t size = 0;
+    guchar *data = NULL;
+
+    if (hex_string) {
+        size = strlen(hex_string) / 2;
+        if (size) {
+            size_t i;
+
+            data = g_new(guchar, size + 1);
+            for (i = 0; i < size; i++) {
+                data[i]  = hexval(*hex_string++) << 4;
+                data[i] |= hexval(*hex_string++);
+            }
+            data[i] = '\0';
+        }
+    }
+    if (out_size) {
+        *out_size = size;
+    }
+    return data;
+}
+
 /*
  * helper to parse debug environment variables
  */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 03/18] cutils: Add qemu_strdup_hexlify() and qemu_strdup_unhexlify()

[Laszlo Ersek]

Hi Phil,

most important comment at the bottom.

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Add two helpers: one to represent a binary data as a string of
> hexadecimal values, and one to restore a such string into its
> original binary data.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/cutils.h | 33 ++++++++++++++++++++++++++
>  util/cutils.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index d2dad3057c..375a5508b0 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -171,6 +171,39 @@ bool test_buffer_is_zero_next_accel(void);
>  int uleb128_encode_small(uint8_t *out, uint32_t n);
>  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>  
> +/**
> + * qemu_strdup_hexlify:

(1) I think the name "hexlify" is unusual. I think we should use
encode/decode terminology, or hex/unhex, or, if we want to stick with
the "stringify" pattern, hexify/unhexify. (No "l".)

> + *
> + * Encode a sequence of binary data into its hexadecimal stringified
> + * representation.
> + *
> + * @ptr: Buffer to hexlify
> + * @size: Length of the buffer
> + *
> + * Use qemu_strdup_unhexlify() to convert the hex string to original data.
> + *
> + * Returns: A newly allocated, zero-terminated hex encoded string representing
> + * the data. The returned string must be freed with g_free().
> + */
> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize size);
> +
> +/**
> + * qemu_strdup_unhexlify:
> + *
> + * Decode a sequence of hexadecimal encoded text into binary data.
> + *
> + * @hex_string: String to unhexlify
> + * @out_size: if not NULL: gsize to be written with the data length
> + *
> + * This function is the opposite of qemu_strdup_hexlify().
> + *
> + * Returns: A newly allocated buffer containing the binary data that text
> + * represents. The returned buffer must be freed with g_free().
> + * Note that the returned binary data is not necessarily zero-terminated,
> + * so it should not be used as a character string.
> + */
> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size);
> +
>  /**
>   * qemu_pstrcmp0:
>   * @str1: a non-NULL pointer to a C string (*str1 can be NULL)
> diff --git a/util/cutils.c b/util/cutils.c
> index e098debdc0..bf324c0d8b 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -779,6 +779,61 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n)
>      }
>  }
>  
> +static guchar hexval(const gchar v)
> +{
> +    switch (v) {
> +    case '0' ... '9':
> +        return v - '0';
> +    case 'A' ... 'F':
> +        return v - 'A' + 10;
> +    case 'a' ... 'f':
> +        return v - 'a' + 10;
> +    default:
> +        return 0;
> +    }
> +}

(2) I don't think that we should silently translate invalid characters
to zero, in any hexadecimal decoder.

> +
> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize len)
> +{
> +    guchar *data = (guchar *)ptr;
> +    gchar *hex_string;
> +
> +    if (!ptr || !len) {
> +        return g_strdup("");
> +    }
> +
> +    hex_string = g_malloc(2 * len + 1);

(3) Should check against integer overflow in the g_malloc() argument
(multiplication and addition).

> +    for (gsize i = 0; i < len; i++) {
> +        g_snprintf(&hex_string[2 * i], 3, "%02x", data[i]);
> +    }
> +
> +    return hex_string;
> +}
> +
> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size)
> +{
> +    size_t size = 0;
> +    guchar *data = NULL;
> +
> +    if (hex_string) {
> +        size = strlen(hex_string) / 2;

(4) Should likely check that the length of the string is an even integer.

> +        if (size) {
> +            size_t i;
> +
> +            data = g_new(guchar, size + 1);
> +            for (i = 0; i < size; i++) {
> +                data[i]  = hexval(*hex_string++) << 4;
> +                data[i] |= hexval(*hex_string++);
> +            }
> +            data[i] = '\0';
> +        }
> +    }
> +    if (out_size) {
> +        *out_size = size;
> +    }
> +    return data;
> +}
> +
>  /*
>   * helper to parse debug environment variables
>   */
> 

(5) Most importantly: I don't think we need this patch.

First, AFAICS, the unhex function is never used in the series, and no
unit test is being added for it. That makes it a bad candidate for
"include/qemu/cutils.h".

Second, while the hex function is used in PATCH v2 13/18
("hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command"), the documentation in
that patch and the logic in the patch are inconsistent. The
documentation -- i.e. both the commit message and the "misc.json" change
-- say that "FirmwareConfigurationItem.data" is unused (not populated).
However, that's exactly what create_qmp_fw_cfg_item() uses the hex
function for.

Third, if we do decide that the QMP command should output the fw_cfg
binary data, then the QMP tradition (to my knowledge) has been to use
base64 encoding. GLib provides helpers for base64:

https://developer.gnome.org/glib/stable/glib-Base64-Encoding.html

and you can see examples of it being used in e.g.

(a) qmp_ringbuf_read() [chardev/char-ringbuf.c] -- the  @ringbuf-read
command is defined in "qapi/char.json"

(b) qmp_guest_exec_status() [qga/commands.c] -- the @guest-exec-status
command is defined in "qga/qapi-schema.json".

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 03/18] cutils: Add qemu_strdup_hexlify() and qemu_strdup_unhexlify()

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Hi Phil,
>
> most important comment at the bottom.
>
> On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
>> Add two helpers: one to represent a binary data as a string of
>> hexadecimal values, and one to restore a such string into its
>> original binary data.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/qemu/cutils.h | 33 ++++++++++++++++++++++++++
>>  util/cutils.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 88 insertions(+)
>> 
>> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
>> index d2dad3057c..375a5508b0 100644
>> --- a/include/qemu/cutils.h
>> +++ b/include/qemu/cutils.h
>> @@ -171,6 +171,39 @@ bool test_buffer_is_zero_next_accel(void);
>>  int uleb128_encode_small(uint8_t *out, uint32_t n);
>>  int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>>  
>> +/**
>> + * qemu_strdup_hexlify:
>
> (1) I think the name "hexlify" is unusual.

hexlify-buffer is an interactive autoloaded Lisp function in
‘hexl.el’.

(hexlify-buffer)

Convert a binary buffer to hexl format.
This discards the buffer’s undo information.

;-P

>                                            I think we should use
> encode/decode terminology, or hex/unhex, or, if we want to stick with
> the "stringify" pattern, hexify/unhexify. (No "l".)
>
>> + *
>> + * Encode a sequence of binary data into its hexadecimal stringified
>> + * representation.
>> + *
>> + * @ptr: Buffer to hexlify

Similar parameters elsewhere in this header are called @buf.

>> + * @size: Length of the buffer
>> + *
>> + * Use qemu_strdup_unhexlify() to convert the hex string to original data.
>> + *
>> + * Returns: A newly allocated, zero-terminated hex encoded string representing
>> + * the data. The returned string must be freed with g_free().
>> + */
>> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize size);

Avoid the silly GLib types, please.

>> +
>> +/**
>> + * qemu_strdup_unhexlify:
>> + *
>> + * Decode a sequence of hexadecimal encoded text into binary data.
>> + *
>> + * @hex_string: String to unhexlify
>> + * @out_size: if not NULL: gsize to be written with the data length
>> + *
>> + * This function is the opposite of qemu_strdup_hexlify().
>> + *
>> + * Returns: A newly allocated buffer containing the binary data that text
>> + * represents. The returned buffer must be freed with g_free().
>> + * Note that the returned binary data is not necessarily zero-terminated,
>> + * so it should not be used as a character string.
>> + */
>> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size);
>> +
>>  /**
>>   * qemu_pstrcmp0:
>>   * @str1: a non-NULL pointer to a C string (*str1 can be NULL)
>> diff --git a/util/cutils.c b/util/cutils.c
>> index e098debdc0..bf324c0d8b 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -779,6 +779,61 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n)
>>      }
>>  }
>>  
>> +static guchar hexval(const gchar v)

Naming the parameter @ch would be more idiomatic, I think.

>> +{
>> +    switch (v) {
>> +    case '0' ... '9':
>> +        return v - '0';
>> +    case 'A' ... 'F':
>> +        return v - 'A' + 10;
>> +    case 'a' ... 'f':
>> +        return v - 'a' + 10;
>> +    default:
>> +        return 0;
>> +    }
>> +}
>
> (2) I don't think that we should silently translate invalid characters
> to zero, in any hexadecimal decoder.

Yup.  Let's abort().

>> +
>> +gchar *qemu_strdup_hexlify(gconstpointer ptr, gsize len)
>> +{
>> +    guchar *data = (guchar *)ptr;
>> +    gchar *hex_string;
>> +
>> +    if (!ptr || !len) {
>> +        return g_strdup("");
>> +    }

A null pointer is not the same as the empty string.  Replace this by

        assert(ptr);

and ...

>> +
>> +    hex_string = g_malloc(2 * len + 1);
>
> (3) Should check against integer overflow in the g_malloc() argument
> (multiplication and addition).

E.g.
        assert(len <= (SIZE_MAX - 1) / 2);

>> +    for (gsize i = 0; i < len; i++) {
>> +        g_snprintf(&hex_string[2 * i], 3, "%02x", data[i]);
>> +    }
>> +

... esnure termination here

        hex_string[2 * i] = 0;

What does g_snprintf() buy us over plain snprintf()?  I count 400+ uses
of the latter, and none of the former.

>> +    return hex_string;
>> +}
>> +
>> +gpointer qemu_strdup_unhexlify(const gchar *hex_string, gsize *out_size)
>> +{
>> +    size_t size = 0;
>> +    guchar *data = NULL;
>> +
>> +    if (hex_string) {

A null pointer is not the same as the empty string.  assert(hex_string)
and make the conversion unconditional.


>> +        size = strlen(hex_string) / 2;
>
> (4) Should likely check that the length of the string is an even integer.
>
>> +        if (size) {
>> +            size_t i;
>> +
>> +            data = g_new(guchar, size + 1);
>> +            for (i = 0; i < size; i++) {
>> +                data[i]  = hexval(*hex_string++) << 4;
>> +                data[i] |= hexval(*hex_string++);
>> +            }
>> +            data[i] = '\0';
>> +        }
>> +    }
>> +    if (out_size) {
>> +        *out_size = size;
>> +    }
>> +    return data;

This maps "" to null.  I think it shold return "".  It naturally does if
you make the if (size) code unconditional.

>> +}
>> +
>>  /*
>>   * helper to parse debug environment variables
>>   */
>> 
>
> (5) Most importantly: I don't think we need this patch.
>
> First, AFAICS, the unhex function is never used in the series, and no
> unit test is being added for it. That makes it a bad candidate for
> "include/qemu/cutils.h".
>
> Second, while the hex function is used in PATCH v2 13/18
> ("hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command"), the documentation in
> that patch and the logic in the patch are inconsistent. The
> documentation -- i.e. both the commit message and the "misc.json" change
> -- say that "FirmwareConfigurationItem.data" is unused (not populated).
> However, that's exactly what create_qmp_fw_cfg_item() uses the hex
> function for.
>
> Third, if we do decide that the QMP command should output the fw_cfg
> binary data, then the QMP tradition (to my knowledge) has been to use
> base64 encoding. GLib provides helpers for base64:
>
> https://developer.gnome.org/glib/stable/glib-Base64-Encoding.html
>
> and you can see examples of it being used in e.g.
>
> (a) qmp_ringbuf_read() [chardev/char-ringbuf.c] -- the  @ringbuf-read
> command is defined in "qapi/char.json"
>
> (b) qmp_guest_exec_status() [qga/commands.c] -- the @guest-exec-status
> command is defined in "qga/qapi-schema.json".

Yes.  I wish you had wrote that first, saving me the trouble of looking
at the patch.

[Qemu-devel] [PATCH v2 04/18] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

Add fw_cfg_arch_key_name() to be able to resolve architecture
specific keys. All architectures do have specific keys, thus
implement this function. Architectures that don't use the fw_cfg
device don't have to implement this function, however to ease
the Makefile rules and satisfy the linking, we provide a stub.

Now than we can resolve keys into "well know numeric", add
trace events to display more information when keys are added
(and dump the key content when possible).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Added fw_cfg_arch_key_name() -> reset R-b
---
 MAINTAINERS               |  1 +
 hw/i386/pc.c              | 21 +++++++++++++
 hw/nvram/fw_cfg.c         | 63 ++++++++++++++++++++++++++++++++++++++-
 hw/nvram/trace-events     |  7 ++++-
 hw/ppc/Makefile.objs      |  2 +-
 hw/ppc/fw_cfg.c           | 31 +++++++++++++++++++
 hw/sparc/sun4m.c          | 19 ++++++++++++
 hw/sparc64/sun4u.c        | 19 ++++++++++++
 include/hw/nvram/fw_cfg.h | 11 +++++++
 stubs/Makefile.objs       |  1 +
 stubs/fw_cfg.c            | 19 ++++++++++++
 11 files changed, 191 insertions(+), 3 deletions(-)
 create mode 100644 hw/ppc/fw_cfg.c
 create mode 100644 stubs/fw_cfg.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 074ad46d47..306fc2aefa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1659,6 +1659,7 @@ R: Gerd Hoffmann <kraxel@redhat.com>
 S: Supported
 F: docs/specs/fw_cfg.txt
 F: hw/nvram/fw_cfg.c
+F: stubs/fw_cfg.c
 F: include/hw/nvram/fw_cfg.h
 F: include/standard-headers/linux/qemu_fw_cfg.h
 F: tests/libqos/fw_cfg.c
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 42128183e9..0848cdc18f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -349,6 +349,27 @@ GlobalProperty pc_compat_1_4[] = {
 };
 const size_t pc_compat_1_4_len = G_N_ELEMENTS(pc_compat_1_4);
 
+const char *fw_cfg_arch_key_name(uint16_t key)
+{
+    static const struct {
+        uint16_t key;
+        const char *name;
+    } fw_cfg_arch_wellknown_keys[] = {
+        {FW_CFG_ACPI_TABLES, "acpi_tables"},
+        {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
+        {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
+        {FW_CFG_E820_TABLE, "e820_tables"},
+        {FW_CFG_HPET, "hpet"},
+    };
+
+    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
+        if (fw_cfg_arch_wellknown_keys[i].key == key) {
+            return fw_cfg_arch_wellknown_keys[i].name;
+        }
+    }
+    return NULL;
+}
+
 void gsi_handler(void *opaque, int n, int level)
 {
     GSIState *s = opaque;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7fdf04adc9..684c2cf00a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -60,6 +60,62 @@ struct FWCfgEntry {
     FWCfgWriteCallback write_cb;
 };
 
+/**
+ * key_name:
+ *
+ * @key: The uint16 selector key.
+ *
+ * Returns: The stringified name if the selector refers to a well-known
+ *          numerically defined item, or NULL on key lookup failure.
+ */
+static const char *key_name(uint16_t key)
+{
+    static const char *fw_cfg_wellknown_keys[FW_CFG_FILE_FIRST] = {
+        [FW_CFG_SIGNATURE] = "signature",
+        [FW_CFG_ID] = "id",
+        [FW_CFG_UUID] = "uuid",
+        [FW_CFG_RAM_SIZE] = "ram_size",
+        [FW_CFG_NOGRAPHIC] = "nographic",
+        [FW_CFG_NB_CPUS] = "nb_cpus",
+        [FW_CFG_MACHINE_ID] = "machine_id",
+        [FW_CFG_KERNEL_ADDR] = "kernel_addr",
+        [FW_CFG_KERNEL_SIZE] = "kernel_size",
+        [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
+        [FW_CFG_INITRD_ADDR] = "initrd_addr",
+        [FW_CFG_INITRD_SIZE] = "initdr_size",
+        [FW_CFG_BOOT_DEVICE] = "boot_device",
+        [FW_CFG_NUMA] = "numa",
+        [FW_CFG_BOOT_MENU] = "boot_menu",
+        [FW_CFG_MAX_CPUS] = "max_cpus",
+        [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
+        [FW_CFG_KERNEL_DATA] = "kernel_data",
+        [FW_CFG_INITRD_DATA] = "initrd_data",
+        [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
+        [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
+        [FW_CFG_CMDLINE_DATA] = "cmdline_data",
+        [FW_CFG_SETUP_ADDR] = "setup_addr",
+        [FW_CFG_SETUP_SIZE] = "setup_size",
+        [FW_CFG_SETUP_DATA] = "setup_data",
+        [FW_CFG_FILE_DIR] = "file_dir",
+    };
+
+    if (key & FW_CFG_ARCH_LOCAL) {
+        return fw_cfg_arch_key_name(key);
+    }
+    if (key < FW_CFG_FILE_FIRST) {
+        return fw_cfg_wellknown_keys[key];
+    }
+
+    return NULL;
+}
+
+static inline const char *trace_key_name(uint16_t key)
+{
+    const char *name = key_name(key);
+
+    return name ? name : "unknown";
+}
+
 #define JPG_FILE 0
 #define BMP_FILE 1
 
@@ -234,7 +290,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
         }
     }
 
-    trace_fw_cfg_select(s, key, ret);
+    trace_fw_cfg_select(s, key, trace_key_name(key), ret);
     return ret;
 }
 
@@ -617,6 +673,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
 
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
+    trace_fw_cfg_add_bytes(key, trace_key_name(key), len);
     fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
 }
 
@@ -624,6 +681,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
 {
     size_t sz = strlen(value) + 1;
 
+    trace_fw_cfg_add_string(key, trace_key_name(key), value);
     fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
 }
 
@@ -633,6 +691,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le16(value);
+    trace_fw_cfg_add_i16(key, trace_key_name(key), value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
@@ -652,6 +711,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le32(value);
+    trace_fw_cfg_add_i32(key, trace_key_name(key), value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
@@ -661,6 +721,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le64(value);
+    trace_fw_cfg_add_i64(key, trace_key_name(key), value);
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
index 6b55ba7a09..4d8fa992fe 100644
--- a/hw/nvram/trace-events
+++ b/hw/nvram/trace-events
@@ -5,6 +5,11 @@ nvram_read(uint32_t addr, uint32_t ret) "read addr %d: 0x%02x"
 nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> 0x%02x"
 
 # hw/nvram/fw_cfg.c
-fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
+fw_cfg_select(void *s, uint16_t key_value, const char *key_name, int ret) "%p key 0x%04" PRIx16 " '%s', ret: %d"
 fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64
+fw_cfg_add_bytes(uint16_t key_value, const char *key_name, size_t len) "key 0x%04" PRIx16 " '%s', %zu bytes"
 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
+fw_cfg_add_string(uint16_t key_value, const char *key_name, const char *value) "key 0x%04" PRIx16 " '%s', value '%s'"
+fw_cfg_add_i16(uint16_t key_value, const char *key_name, uint16_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx16
+fw_cfg_add_i32(uint16_t key_value, const char *key_name, uint32_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx32
+fw_cfg_add_i64(uint16_t key_value, const char *key_name, uint64_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx64
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 1111b218a0..ae94098155 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -1,5 +1,5 @@
 # shared objects
-obj-y += ppc.o ppc_booke.o fdt.o
+obj-y += ppc.o ppc_booke.o fdt.o fw_cfg.o
 # IBM pSeries (sPAPR)
 obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
diff --git a/hw/ppc/fw_cfg.c b/hw/ppc/fw_cfg.c
new file mode 100644
index 0000000000..0e7616b78a
--- /dev/null
+++ b/hw/ppc/fw_cfg.c
@@ -0,0 +1,31 @@
+#include "qemu/osdep.h"
+#include "hw/ppc/ppc.h"
+#include "hw/nvram/fw_cfg.h"
+
+const char *fw_cfg_arch_key_name(uint16_t key)
+{
+    static const struct {
+        uint16_t key;
+        const char *name;
+    } fw_cfg_arch_wellknown_keys[] = {
+        {FW_CFG_PPC_WIDTH, "width"},
+        {FW_CFG_PPC_HEIGHT, "height"},
+        {FW_CFG_PPC_DEPTH, "depth"},
+        {FW_CFG_PPC_TBFREQ, "tbfreq"},
+        {FW_CFG_PPC_CLOCKFREQ, "clockfreq"},
+        {FW_CFG_PPC_IS_KVM, "is_kvm"},
+        {FW_CFG_PPC_KVM_HC, "kvm_hc"},
+        {FW_CFG_PPC_KVM_PID, "pid"},
+        {FW_CFG_PPC_NVRAM_ADDR, "nvram_addr"},
+        {FW_CFG_PPC_BUSFREQ, "busfreq"},
+        {FW_CFG_PPC_NVRAM_FLAT, "nvram_flat"},
+        {FW_CFG_PPC_VIACONFIG, "viaconfig"},
+    };
+
+    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
+        if (fw_cfg_arch_wellknown_keys[i].key == key) {
+            return fw_cfg_arch_wellknown_keys[i].name;
+        }
+    }
+    return NULL;
+}
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index ca1e3825d5..49251d62b3 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -97,6 +97,25 @@ struct sun4m_hwdef {
     uint8_t nvram_machine_id;
 };
 
+const char *fw_cfg_arch_key_name(uint16_t key)
+{
+    static const struct {
+        uint16_t key;
+        const char *name;
+    } fw_cfg_arch_wellknown_keys[] = {
+        {FW_CFG_SUN4M_DEPTH, "depth"},
+        {FW_CFG_SUN4M_WIDTH, "width"},
+        {FW_CFG_SUN4M_HEIGHT, "height"},
+    };
+
+    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
+        if (fw_cfg_arch_wellknown_keys[i].key == key) {
+            return fw_cfg_arch_wellknown_keys[i].name;
+        }
+    }
+    return NULL;
+}
+
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 399f2d73c8..4230b17b87 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -91,6 +91,25 @@ typedef struct EbusState {
 #define TYPE_EBUS "ebus"
 #define EBUS(obj) OBJECT_CHECK(EbusState, (obj), TYPE_EBUS)
 
+const char *fw_cfg_arch_key_name(uint16_t key)
+{
+    static const struct {
+        uint16_t key;
+        const char *name;
+    } fw_cfg_arch_wellknown_keys[] = {
+        {FW_CFG_SPARC64_WIDTH, "width"},
+        {FW_CFG_SPARC64_HEIGHT, "height"},
+        {FW_CFG_SPARC64_DEPTH, "depth"},
+    };
+
+    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
+        if (fw_cfg_arch_wellknown_keys[i].key == key) {
+            return fw_cfg_arch_wellknown_keys[i].name;
+        }
+    }
+    return NULL;
+}
+
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
 {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a74..828ad9dedc 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -226,4 +226,15 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
 FWCfgState *fw_cfg_find(void);
 bool fw_cfg_dma_enabled(void *opaque);
 
+/**
+ * fw_cfg_arch_key_name:
+ *
+ * @key: The uint16 selector key.
+ *
+ * Returns: The stringified architecture-specific name if the selector
+ *          refers to a well-known numerically defined item, or NULL on
+ *          key lookup failure.
+ */
+const char *fw_cfg_arch_key_name(uint16_t key);
+
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 269dfa5832..73452ad265 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,4 @@ stub-obj-y += xen-hvm.o
 stub-obj-y += pci-host-piix.o
 stub-obj-y += ram-block.o
 stub-obj-y += ramfb.o
+stub-obj-y += fw_cfg.o
diff --git a/stubs/fw_cfg.c b/stubs/fw_cfg.c
new file mode 100644
index 0000000000..2b886c94d6
--- /dev/null
+++ b/stubs/fw_cfg.c
@@ -0,0 +1,19 @@
+/*
+ * fw_cfg stubs
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/nvram/fw_cfg.h"
+
+const char *fw_cfg_arch_key_name(uint16_t key)
+{
+    return NULL;
+}
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 04/18] hw/nvram/fw_cfg: Add trace events

[Laszlo Ersek]

Hi Phil,

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Add fw_cfg_arch_key_name() to be able to resolve architecture
> specific keys. All architectures do have specific keys, thus
> implement this function. Architectures that don't use the fw_cfg
> device don't have to implement this function, however to ease
> the Makefile rules and satisfy the linking, we provide a stub.
> 
> Now than we can resolve keys into "well know numeric", add
> trace events to display more information when keys are added
> (and dump the key content when possible).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Added fw_cfg_arch_key_name() -> reset R-b
> ---

This is not a small patch, and Michael and I reviewed v1 three months ago:

https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01616.html
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01947.html

I'm not keen on re-reviewing the patch from zero (after three months)
just because of fw_cfg_arch_key_name().

Is it possible to split fw_cfg_arch_key_name() to a separate patch first
(which we could review in isolation -- it affects multiple arches), and
rebase the original (v1) patch to it, with minimal updates (keeping the
R-b's, or at least pointing out the minimal change for incremental review)?

Thanks
Laszlo

>  MAINTAINERS               |  1 +
>  hw/i386/pc.c              | 21 +++++++++++++
>  hw/nvram/fw_cfg.c         | 63 ++++++++++++++++++++++++++++++++++++++-
>  hw/nvram/trace-events     |  7 ++++-
>  hw/ppc/Makefile.objs      |  2 +-
>  hw/ppc/fw_cfg.c           | 31 +++++++++++++++++++
>  hw/sparc/sun4m.c          | 19 ++++++++++++
>  hw/sparc64/sun4u.c        | 19 ++++++++++++
>  include/hw/nvram/fw_cfg.h | 11 +++++++
>  stubs/Makefile.objs       |  1 +
>  stubs/fw_cfg.c            | 19 ++++++++++++
>  11 files changed, 191 insertions(+), 3 deletions(-)
>  create mode 100644 hw/ppc/fw_cfg.c
>  create mode 100644 stubs/fw_cfg.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 074ad46d47..306fc2aefa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1659,6 +1659,7 @@ R: Gerd Hoffmann <kraxel@redhat.com>
>  S: Supported
>  F: docs/specs/fw_cfg.txt
>  F: hw/nvram/fw_cfg.c
> +F: stubs/fw_cfg.c
>  F: include/hw/nvram/fw_cfg.h
>  F: include/standard-headers/linux/qemu_fw_cfg.h
>  F: tests/libqos/fw_cfg.c
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 42128183e9..0848cdc18f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -349,6 +349,27 @@ GlobalProperty pc_compat_1_4[] = {
>  };
>  const size_t pc_compat_1_4_len = G_N_ELEMENTS(pc_compat_1_4);
>  
> +const char *fw_cfg_arch_key_name(uint16_t key)
> +{
> +    static const struct {
> +        uint16_t key;
> +        const char *name;
> +    } fw_cfg_arch_wellknown_keys[] = {
> +        {FW_CFG_ACPI_TABLES, "acpi_tables"},
> +        {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
> +        {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
> +        {FW_CFG_E820_TABLE, "e820_tables"},
> +        {FW_CFG_HPET, "hpet"},
> +    };
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
> +            return fw_cfg_arch_wellknown_keys[i].name;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  void gsi_handler(void *opaque, int n, int level)
>  {
>      GSIState *s = opaque;
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7fdf04adc9..684c2cf00a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -60,6 +60,62 @@ struct FWCfgEntry {
>      FWCfgWriteCallback write_cb;
>  };
>  
> +/**
> + * key_name:
> + *
> + * @key: The uint16 selector key.
> + *
> + * Returns: The stringified name if the selector refers to a well-known
> + *          numerically defined item, or NULL on key lookup failure.
> + */
> +static const char *key_name(uint16_t key)
> +{
> +    static const char *fw_cfg_wellknown_keys[FW_CFG_FILE_FIRST] = {
> +        [FW_CFG_SIGNATURE] = "signature",
> +        [FW_CFG_ID] = "id",
> +        [FW_CFG_UUID] = "uuid",
> +        [FW_CFG_RAM_SIZE] = "ram_size",
> +        [FW_CFG_NOGRAPHIC] = "nographic",
> +        [FW_CFG_NB_CPUS] = "nb_cpus",
> +        [FW_CFG_MACHINE_ID] = "machine_id",
> +        [FW_CFG_KERNEL_ADDR] = "kernel_addr",
> +        [FW_CFG_KERNEL_SIZE] = "kernel_size",
> +        [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
> +        [FW_CFG_INITRD_ADDR] = "initrd_addr",
> +        [FW_CFG_INITRD_SIZE] = "initdr_size",
> +        [FW_CFG_BOOT_DEVICE] = "boot_device",
> +        [FW_CFG_NUMA] = "numa",
> +        [FW_CFG_BOOT_MENU] = "boot_menu",
> +        [FW_CFG_MAX_CPUS] = "max_cpus",
> +        [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
> +        [FW_CFG_KERNEL_DATA] = "kernel_data",
> +        [FW_CFG_INITRD_DATA] = "initrd_data",
> +        [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
> +        [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
> +        [FW_CFG_CMDLINE_DATA] = "cmdline_data",
> +        [FW_CFG_SETUP_ADDR] = "setup_addr",
> +        [FW_CFG_SETUP_SIZE] = "setup_size",
> +        [FW_CFG_SETUP_DATA] = "setup_data",
> +        [FW_CFG_FILE_DIR] = "file_dir",
> +    };
> +
> +    if (key & FW_CFG_ARCH_LOCAL) {
> +        return fw_cfg_arch_key_name(key);
> +    }
> +    if (key < FW_CFG_FILE_FIRST) {
> +        return fw_cfg_wellknown_keys[key];
> +    }
> +
> +    return NULL;
> +}
> +
> +static inline const char *trace_key_name(uint16_t key)
> +{
> +    const char *name = key_name(key);
> +
> +    return name ? name : "unknown";
> +}
> +
>  #define JPG_FILE 0
>  #define BMP_FILE 1
>  
> @@ -234,7 +290,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
>          }
>      }
>  
> -    trace_fw_cfg_select(s, key, ret);
> +    trace_fw_cfg_select(s, key, trace_key_name(key), ret);
>      return ret;
>  }
>  
> @@ -617,6 +673,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>  
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>  {
> +    trace_fw_cfg_add_bytes(key, trace_key_name(key), len);
>      fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
>  }
>  
> @@ -624,6 +681,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
>  {
>      size_t sz = strlen(value) + 1;
>  
> +    trace_fw_cfg_add_string(key, trace_key_name(key), value);
>      fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
>  }
>  
> @@ -633,6 +691,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le16(value);
> +    trace_fw_cfg_add_i16(key, trace_key_name(key), value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -652,6 +711,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le32(value);
> +    trace_fw_cfg_add_i32(key, trace_key_name(key), value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> @@ -661,6 +721,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>  
>      copy = g_malloc(sizeof(value));
>      *copy = cpu_to_le64(value);
> +    trace_fw_cfg_add_i64(key, trace_key_name(key), value);
>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>  }
>  
> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
> index 6b55ba7a09..4d8fa992fe 100644
> --- a/hw/nvram/trace-events
> +++ b/hw/nvram/trace-events
> @@ -5,6 +5,11 @@ nvram_read(uint32_t addr, uint32_t ret) "read addr %d: 0x%02x"
>  nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> 0x%02x"
>  
>  # hw/nvram/fw_cfg.c
> -fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
> +fw_cfg_select(void *s, uint16_t key_value, const char *key_name, int ret) "%p key 0x%04" PRIx16 " '%s', ret: %d"
>  fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64
> +fw_cfg_add_bytes(uint16_t key_value, const char *key_name, size_t len) "key 0x%04" PRIx16 " '%s', %zu bytes"
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
> +fw_cfg_add_string(uint16_t key_value, const char *key_name, const char *value) "key 0x%04" PRIx16 " '%s', value '%s'"
> +fw_cfg_add_i16(uint16_t key_value, const char *key_name, uint16_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx16
> +fw_cfg_add_i32(uint16_t key_value, const char *key_name, uint32_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx32
> +fw_cfg_add_i64(uint16_t key_value, const char *key_name, uint64_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx64
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 1111b218a0..ae94098155 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -1,5 +1,5 @@
>  # shared objects
> -obj-y += ppc.o ppc_booke.o fdt.o
> +obj-y += ppc.o ppc_booke.o fdt.o fw_cfg.o
>  # IBM pSeries (sPAPR)
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> diff --git a/hw/ppc/fw_cfg.c b/hw/ppc/fw_cfg.c
> new file mode 100644
> index 0000000000..0e7616b78a
> --- /dev/null
> +++ b/hw/ppc/fw_cfg.c
> @@ -0,0 +1,31 @@
> +#include "qemu/osdep.h"
> +#include "hw/ppc/ppc.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +const char *fw_cfg_arch_key_name(uint16_t key)
> +{
> +    static const struct {
> +        uint16_t key;
> +        const char *name;
> +    } fw_cfg_arch_wellknown_keys[] = {
> +        {FW_CFG_PPC_WIDTH, "width"},
> +        {FW_CFG_PPC_HEIGHT, "height"},
> +        {FW_CFG_PPC_DEPTH, "depth"},
> +        {FW_CFG_PPC_TBFREQ, "tbfreq"},
> +        {FW_CFG_PPC_CLOCKFREQ, "clockfreq"},
> +        {FW_CFG_PPC_IS_KVM, "is_kvm"},
> +        {FW_CFG_PPC_KVM_HC, "kvm_hc"},
> +        {FW_CFG_PPC_KVM_PID, "pid"},
> +        {FW_CFG_PPC_NVRAM_ADDR, "nvram_addr"},
> +        {FW_CFG_PPC_BUSFREQ, "busfreq"},
> +        {FW_CFG_PPC_NVRAM_FLAT, "nvram_flat"},
> +        {FW_CFG_PPC_VIACONFIG, "viaconfig"},
> +    };
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
> +            return fw_cfg_arch_wellknown_keys[i].name;
> +        }
> +    }
> +    return NULL;
> +}
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index ca1e3825d5..49251d62b3 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -97,6 +97,25 @@ struct sun4m_hwdef {
>      uint8_t nvram_machine_id;
>  };
>  
> +const char *fw_cfg_arch_key_name(uint16_t key)
> +{
> +    static const struct {
> +        uint16_t key;
> +        const char *name;
> +    } fw_cfg_arch_wellknown_keys[] = {
> +        {FW_CFG_SUN4M_DEPTH, "depth"},
> +        {FW_CFG_SUN4M_WIDTH, "width"},
> +        {FW_CFG_SUN4M_HEIGHT, "height"},
> +    };
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
> +            return fw_cfg_arch_wellknown_keys[i].name;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>                              Error **errp)
>  {
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 399f2d73c8..4230b17b87 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -91,6 +91,25 @@ typedef struct EbusState {
>  #define TYPE_EBUS "ebus"
>  #define EBUS(obj) OBJECT_CHECK(EbusState, (obj), TYPE_EBUS)
>  
> +const char *fw_cfg_arch_key_name(uint16_t key)
> +{
> +    static const struct {
> +        uint16_t key;
> +        const char *name;
> +    } fw_cfg_arch_wellknown_keys[] = {
> +        {FW_CFG_SPARC64_WIDTH, "width"},
> +        {FW_CFG_SPARC64_HEIGHT, "height"},
> +        {FW_CFG_SPARC64_DEPTH, "depth"},
> +    };
> +
> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
> +            return fw_cfg_arch_wellknown_keys[i].name;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>                              Error **errp)
>  {
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..828ad9dedc 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -226,4 +226,15 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>  FWCfgState *fw_cfg_find(void);
>  bool fw_cfg_dma_enabled(void *opaque);
>  
> +/**
> + * fw_cfg_arch_key_name:
> + *
> + * @key: The uint16 selector key.
> + *
> + * Returns: The stringified architecture-specific name if the selector
> + *          refers to a well-known numerically defined item, or NULL on
> + *          key lookup failure.
> + */
> +const char *fw_cfg_arch_key_name(uint16_t key);
> +
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 269dfa5832..73452ad265 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -39,3 +39,4 @@ stub-obj-y += xen-hvm.o
>  stub-obj-y += pci-host-piix.o
>  stub-obj-y += ram-block.o
>  stub-obj-y += ramfb.o
> +stub-obj-y += fw_cfg.o
> diff --git a/stubs/fw_cfg.c b/stubs/fw_cfg.c
> new file mode 100644
> index 0000000000..2b886c94d6
> --- /dev/null
> +++ b/stubs/fw_cfg.c
> @@ -0,0 +1,19 @@
> +/*
> + * fw_cfg stubs
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +const char *fw_cfg_arch_key_name(uint16_t key)
> +{
> +    return NULL;
> +}
> 

Re: [Qemu-devel] [PATCH v2 04/18] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 3/8/19 10:57 AM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
>> Add fw_cfg_arch_key_name() to be able to resolve architecture
>> specific keys. All architectures do have specific keys, thus
>> implement this function. Architectures that don't use the fw_cfg
>> device don't have to implement this function, however to ease
>> the Makefile rules and satisfy the linking, we provide a stub.
>>
>> Now than we can resolve keys into "well know numeric", add
>> trace events to display more information when keys are added
>> (and dump the key content when possible).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v2: Added fw_cfg_arch_key_name() -> reset R-b
>> ---
> 
> This is not a small patch, and Michael and I reviewed v1 three months ago:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01616.html
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01947.html
> 
> I'm not keen on re-reviewing the patch from zero (after three months)
> just because of fw_cfg_arch_key_name().
> 
> Is it possible to split fw_cfg_arch_key_name() to a separate patch first
> (which we could review in isolation -- it affects multiple arches), and
> rebase the original (v1) patch to it, with minimal updates (keeping the
> R-b's, or at least pointing out the minimal change for incremental review)?

Sure it is, this is what I first did, then expected a comment "this is
not used until the next patch, why not squash it?" and I squashed both.
The series respin will have this patch split as you suggested.

> 
> Thanks
> Laszlo
> 
>>  MAINTAINERS               |  1 +
>>  hw/i386/pc.c              | 21 +++++++++++++
>>  hw/nvram/fw_cfg.c         | 63 ++++++++++++++++++++++++++++++++++++++-
>>  hw/nvram/trace-events     |  7 ++++-
>>  hw/ppc/Makefile.objs      |  2 +-
>>  hw/ppc/fw_cfg.c           | 31 +++++++++++++++++++
>>  hw/sparc/sun4m.c          | 19 ++++++++++++
>>  hw/sparc64/sun4u.c        | 19 ++++++++++++
>>  include/hw/nvram/fw_cfg.h | 11 +++++++
>>  stubs/Makefile.objs       |  1 +
>>  stubs/fw_cfg.c            | 19 ++++++++++++
>>  11 files changed, 191 insertions(+), 3 deletions(-)
>>  create mode 100644 hw/ppc/fw_cfg.c
>>  create mode 100644 stubs/fw_cfg.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 074ad46d47..306fc2aefa 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1659,6 +1659,7 @@ R: Gerd Hoffmann <kraxel@redhat.com>
>>  S: Supported
>>  F: docs/specs/fw_cfg.txt
>>  F: hw/nvram/fw_cfg.c
>> +F: stubs/fw_cfg.c
>>  F: include/hw/nvram/fw_cfg.h
>>  F: include/standard-headers/linux/qemu_fw_cfg.h
>>  F: tests/libqos/fw_cfg.c
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 42128183e9..0848cdc18f 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -349,6 +349,27 @@ GlobalProperty pc_compat_1_4[] = {
>>  };
>>  const size_t pc_compat_1_4_len = G_N_ELEMENTS(pc_compat_1_4);
>>  
>> +const char *fw_cfg_arch_key_name(uint16_t key)
>> +{
>> +    static const struct {
>> +        uint16_t key;
>> +        const char *name;
>> +    } fw_cfg_arch_wellknown_keys[] = {
>> +        {FW_CFG_ACPI_TABLES, "acpi_tables"},
>> +        {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
>> +        {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
>> +        {FW_CFG_E820_TABLE, "e820_tables"},
>> +        {FW_CFG_HPET, "hpet"},
>> +    };
>> +
>> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
>> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
>> +            return fw_cfg_arch_wellknown_keys[i].name;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>>  void gsi_handler(void *opaque, int n, int level)
>>  {
>>      GSIState *s = opaque;
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 7fdf04adc9..684c2cf00a 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -60,6 +60,62 @@ struct FWCfgEntry {
>>      FWCfgWriteCallback write_cb;
>>  };
>>  
>> +/**
>> + * key_name:
>> + *
>> + * @key: The uint16 selector key.
>> + *
>> + * Returns: The stringified name if the selector refers to a well-known
>> + *          numerically defined item, or NULL on key lookup failure.
>> + */
>> +static const char *key_name(uint16_t key)
>> +{
>> +    static const char *fw_cfg_wellknown_keys[FW_CFG_FILE_FIRST] = {
>> +        [FW_CFG_SIGNATURE] = "signature",
>> +        [FW_CFG_ID] = "id",
>> +        [FW_CFG_UUID] = "uuid",
>> +        [FW_CFG_RAM_SIZE] = "ram_size",
>> +        [FW_CFG_NOGRAPHIC] = "nographic",
>> +        [FW_CFG_NB_CPUS] = "nb_cpus",
>> +        [FW_CFG_MACHINE_ID] = "machine_id",
>> +        [FW_CFG_KERNEL_ADDR] = "kernel_addr",
>> +        [FW_CFG_KERNEL_SIZE] = "kernel_size",
>> +        [FW_CFG_KERNEL_CMDLINE] = "kernel_cmdline",
>> +        [FW_CFG_INITRD_ADDR] = "initrd_addr",
>> +        [FW_CFG_INITRD_SIZE] = "initdr_size",
>> +        [FW_CFG_BOOT_DEVICE] = "boot_device",
>> +        [FW_CFG_NUMA] = "numa",
>> +        [FW_CFG_BOOT_MENU] = "boot_menu",
>> +        [FW_CFG_MAX_CPUS] = "max_cpus",
>> +        [FW_CFG_KERNEL_ENTRY] = "kernel_entry",
>> +        [FW_CFG_KERNEL_DATA] = "kernel_data",
>> +        [FW_CFG_INITRD_DATA] = "initrd_data",
>> +        [FW_CFG_CMDLINE_ADDR] = "cmdline_addr",
>> +        [FW_CFG_CMDLINE_SIZE] = "cmdline_size",
>> +        [FW_CFG_CMDLINE_DATA] = "cmdline_data",
>> +        [FW_CFG_SETUP_ADDR] = "setup_addr",
>> +        [FW_CFG_SETUP_SIZE] = "setup_size",
>> +        [FW_CFG_SETUP_DATA] = "setup_data",
>> +        [FW_CFG_FILE_DIR] = "file_dir",
>> +    };
>> +
>> +    if (key & FW_CFG_ARCH_LOCAL) {
>> +        return fw_cfg_arch_key_name(key);
>> +    }
>> +    if (key < FW_CFG_FILE_FIRST) {
>> +        return fw_cfg_wellknown_keys[key];
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static inline const char *trace_key_name(uint16_t key)
>> +{
>> +    const char *name = key_name(key);
>> +
>> +    return name ? name : "unknown";
>> +}
>> +
>>  #define JPG_FILE 0
>>  #define BMP_FILE 1
>>  
>> @@ -234,7 +290,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
>>          }
>>      }
>>  
>> -    trace_fw_cfg_select(s, key, ret);
>> +    trace_fw_cfg_select(s, key, trace_key_name(key), ret);
>>      return ret;
>>  }
>>  
>> @@ -617,6 +673,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>>  
>>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
>>  {
>> +    trace_fw_cfg_add_bytes(key, trace_key_name(key), len);
>>      fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
>>  }
>>  
>> @@ -624,6 +681,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
>>  {
>>      size_t sz = strlen(value) + 1;
>>  
>> +    trace_fw_cfg_add_string(key, trace_key_name(key), value);
>>      fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
>>  }
>>  
>> @@ -633,6 +691,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le16(value);
>> +    trace_fw_cfg_add_i16(key, trace_key_name(key), value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> @@ -652,6 +711,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le32(value);
>> +    trace_fw_cfg_add_i32(key, trace_key_name(key), value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> @@ -661,6 +721,7 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
>>  
>>      copy = g_malloc(sizeof(value));
>>      *copy = cpu_to_le64(value);
>> +    trace_fw_cfg_add_i64(key, trace_key_name(key), value);
>>      fw_cfg_add_bytes(s, key, copy, sizeof(value));
>>  }
>>  
>> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events
>> index 6b55ba7a09..4d8fa992fe 100644
>> --- a/hw/nvram/trace-events
>> +++ b/hw/nvram/trace-events
>> @@ -5,6 +5,11 @@ nvram_read(uint32_t addr, uint32_t ret) "read addr %d: 0x%02x"
>>  nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> 0x%02x"
>>  
>>  # hw/nvram/fw_cfg.c
>> -fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
>> +fw_cfg_select(void *s, uint16_t key_value, const char *key_name, int ret) "%p key 0x%04" PRIx16 " '%s', ret: %d"
>>  fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64
>> +fw_cfg_add_bytes(uint16_t key_value, const char *key_name, size_t len) "key 0x%04" PRIx16 " '%s', %zu bytes"
>>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
>> +fw_cfg_add_string(uint16_t key_value, const char *key_name, const char *value) "key 0x%04" PRIx16 " '%s', value '%s'"
>> +fw_cfg_add_i16(uint16_t key_value, const char *key_name, uint16_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx16
>> +fw_cfg_add_i32(uint16_t key_value, const char *key_name, uint32_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx32
>> +fw_cfg_add_i64(uint16_t key_value, const char *key_name, uint64_t value) "key 0x%04" PRIx16 " '%s', value 0x%" PRIx64
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 1111b218a0..ae94098155 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -1,5 +1,5 @@
>>  # shared objects
>> -obj-y += ppc.o ppc_booke.o fdt.o
>> +obj-y += ppc.o ppc_booke.o fdt.o fw_cfg.o
>>  # IBM pSeries (sPAPR)
>>  obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>> diff --git a/hw/ppc/fw_cfg.c b/hw/ppc/fw_cfg.c
>> new file mode 100644
>> index 0000000000..0e7616b78a
>> --- /dev/null
>> +++ b/hw/ppc/fw_cfg.c
>> @@ -0,0 +1,31 @@
>> +#include "qemu/osdep.h"
>> +#include "hw/ppc/ppc.h"
>> +#include "hw/nvram/fw_cfg.h"
>> +
>> +const char *fw_cfg_arch_key_name(uint16_t key)
>> +{
>> +    static const struct {
>> +        uint16_t key;
>> +        const char *name;
>> +    } fw_cfg_arch_wellknown_keys[] = {
>> +        {FW_CFG_PPC_WIDTH, "width"},
>> +        {FW_CFG_PPC_HEIGHT, "height"},
>> +        {FW_CFG_PPC_DEPTH, "depth"},
>> +        {FW_CFG_PPC_TBFREQ, "tbfreq"},
>> +        {FW_CFG_PPC_CLOCKFREQ, "clockfreq"},
>> +        {FW_CFG_PPC_IS_KVM, "is_kvm"},
>> +        {FW_CFG_PPC_KVM_HC, "kvm_hc"},
>> +        {FW_CFG_PPC_KVM_PID, "pid"},
>> +        {FW_CFG_PPC_NVRAM_ADDR, "nvram_addr"},
>> +        {FW_CFG_PPC_BUSFREQ, "busfreq"},
>> +        {FW_CFG_PPC_NVRAM_FLAT, "nvram_flat"},
>> +        {FW_CFG_PPC_VIACONFIG, "viaconfig"},
>> +    };
>> +
>> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
>> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
>> +            return fw_cfg_arch_wellknown_keys[i].name;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index ca1e3825d5..49251d62b3 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -97,6 +97,25 @@ struct sun4m_hwdef {
>>      uint8_t nvram_machine_id;
>>  };
>>  
>> +const char *fw_cfg_arch_key_name(uint16_t key)
>> +{
>> +    static const struct {
>> +        uint16_t key;
>> +        const char *name;
>> +    } fw_cfg_arch_wellknown_keys[] = {
>> +        {FW_CFG_SUN4M_DEPTH, "depth"},
>> +        {FW_CFG_SUN4M_WIDTH, "width"},
>> +        {FW_CFG_SUN4M_HEIGHT, "height"},
>> +    };
>> +
>> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
>> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
>> +            return fw_cfg_arch_wellknown_keys[i].name;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>>                              Error **errp)
>>  {
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 399f2d73c8..4230b17b87 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -91,6 +91,25 @@ typedef struct EbusState {
>>  #define TYPE_EBUS "ebus"
>>  #define EBUS(obj) OBJECT_CHECK(EbusState, (obj), TYPE_EBUS)
>>  
>> +const char *fw_cfg_arch_key_name(uint16_t key)
>> +{
>> +    static const struct {
>> +        uint16_t key;
>> +        const char *name;
>> +    } fw_cfg_arch_wellknown_keys[] = {
>> +        {FW_CFG_SPARC64_WIDTH, "width"},
>> +        {FW_CFG_SPARC64_HEIGHT, "height"},
>> +        {FW_CFG_SPARC64_DEPTH, "depth"},
>> +    };
>> +
>> +    for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
>> +        if (fw_cfg_arch_wellknown_keys[i].key == key) {
>> +            return fw_cfg_arch_wellknown_keys[i].name;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>>                              Error **errp)
>>  {
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index f5a6895a74..828ad9dedc 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -226,4 +226,15 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>  FWCfgState *fw_cfg_find(void);
>>  bool fw_cfg_dma_enabled(void *opaque);
>>  
>> +/**
>> + * fw_cfg_arch_key_name:
>> + *
>> + * @key: The uint16 selector key.
>> + *
>> + * Returns: The stringified architecture-specific name if the selector
>> + *          refers to a well-known numerically defined item, or NULL on
>> + *          key lookup failure.
>> + */
>> +const char *fw_cfg_arch_key_name(uint16_t key);
>> +
>>  #endif
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index 269dfa5832..73452ad265 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -39,3 +39,4 @@ stub-obj-y += xen-hvm.o
>>  stub-obj-y += pci-host-piix.o
>>  stub-obj-y += ram-block.o
>>  stub-obj-y += ramfb.o
>> +stub-obj-y += fw_cfg.o
>> diff --git a/stubs/fw_cfg.c b/stubs/fw_cfg.c
>> new file mode 100644
>> index 0000000000..2b886c94d6
>> --- /dev/null
>> +++ b/stubs/fw_cfg.c
>> @@ -0,0 +1,19 @@
>> +/*
>> + * fw_cfg stubs
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + *
>> + * Author:
>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/nvram/fw_cfg.h"
>> +
>> +const char *fw_cfg_arch_key_name(uint16_t key)
>> +{
>> +    return NULL;
>> +}
>>
> 

[Qemu-devel] [PATCH v2 05/18] hw/nvram/fw_cfg: Use the ldst API

[Philippe Mathieu-Daudé]

The load/store API eases code review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 684c2cf00a..8eb76a382c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -141,7 +141,7 @@ static char *read_splashfile(char *filename, gsize *file_sizep,
     }
 
     /* check magic ID */
-    filehead = ((content[0] & 0xff) + (content[1] << 8)) & 0xffff;
+    filehead = lduw_le_p(content);
     if (filehead == 0xd8ff) {
         file_type = JPG_FILE;
     } else if (filehead == 0x4d42) {
@@ -152,7 +152,7 @@ static char *read_splashfile(char *filename, gsize *file_sizep,
 
     /* check BMP bpp */
     if (file_type == BMP_FILE) {
-        bmp_bpp = (content[28] + (content[29] << 8)) & 0xffff;
+        bmp_bpp = lduw_le_p(&content[28]);
         if (bmp_bpp != 24) {
             goto error;
         }
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 05/18] hw/nvram/fw_cfg: Use the ldst API

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> The load/store API eases code review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 684c2cf00a..8eb76a382c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -141,7 +141,7 @@ static char *read_splashfile(char *filename, gsize *file_sizep,
>      }
>  
>      /* check magic ID */
> -    filehead = ((content[0] & 0xff) + (content[1] << 8)) & 0xffff;
> +    filehead = lduw_le_p(content);
>      if (filehead == 0xd8ff) {
>          file_type = JPG_FILE;
>      } else if (filehead == 0x4d42) {
> @@ -152,7 +152,7 @@ static char *read_splashfile(char *filename, gsize *file_sizep,
>  
>      /* check BMP bpp */
>      if (file_type == BMP_FILE) {
> -        bmp_bpp = (content[28] + (content[29] << 8)) & 0xffff;
> +        bmp_bpp = lduw_le_p(&content[28]);
>          if (bmp_bpp != 24) {
>              goto error;
>          }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH v2 06/18] hw/nvram/fw_cfg: Remove the unnecessary boot_splash_filedata_size

[Philippe Mathieu-Daudé]

The 'boot_splash_filedata_size' was introduced as a global variable
in 3d3b8303c6f. This variable is used as a 'size' argument to the
fw_cfg_add_file(). This function has an interface contract with his
'data' argument, but there is no such contract for 'size' (this is
not a referenced pointer).  We can simply remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c       | 5 ++---
 include/sysemu/sysemu.h | 1 -
 vl.c                    | 1 -
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8eb76a382c..b2dc0a80cb 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -217,15 +217,14 @@ static void fw_cfg_bootsplash(FWCfgState *s)
         }
         g_free(boot_splash_filedata);
         boot_splash_filedata = (uint8_t *)file_data;
-        boot_splash_filedata_size = file_size;
 
         /* insert data */
         if (file_type == JPG_FILE) {
             fw_cfg_add_file(s, "bootsplash.jpg",
-                    boot_splash_filedata, boot_splash_filedata_size);
+                            boot_splash_filedata, file_size);
         } else {
             fw_cfg_add_file(s, "bootsplash.bmp",
-                    boot_splash_filedata, boot_splash_filedata_size);
+                            boot_splash_filedata, file_size);
         }
         g_free(filename);
     }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 89604a8328..6065d9e420 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -110,7 +110,6 @@ extern int old_param;
 extern int boot_menu;
 extern bool boot_strict;
 extern uint8_t *boot_splash_filedata;
-extern size_t boot_splash_filedata_size;
 extern bool enable_mlock;
 extern bool enable_cpu_pm;
 extern QEMUClockType rtc_clock;
diff --git a/vl.c b/vl.c
index 4c5cc0d8ad..fad6fec38c 100644
--- a/vl.c
+++ b/vl.c
@@ -188,7 +188,6 @@ const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
 bool boot_strict;
 uint8_t *boot_splash_filedata;
-size_t boot_splash_filedata_size;
 bool wakeup_suspend_enabled;
 
 int icount_align_option;
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 06/18] hw/nvram/fw_cfg: Remove the unnecessary boot_splash_filedata_size

[Thomas Huth]

On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> The 'boot_splash_filedata_size' was introduced as a global variable
> in 3d3b8303c6f. This variable is used as a 'size' argument to the
> fw_cfg_add_file(). This function has an interface contract with his
> 'data' argument, but there is no such contract for 'size' (this is
> not a referenced pointer).  We can simply remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c       | 5 ++---
>  include/sysemu/sysemu.h | 1 -
>  vl.c                    | 1 -
>  3 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 8eb76a382c..b2dc0a80cb 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -217,15 +217,14 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>          }
>          g_free(boot_splash_filedata);
>          boot_splash_filedata = (uint8_t *)file_data;
> -        boot_splash_filedata_size = file_size;
>  
>          /* insert data */
>          if (file_type == JPG_FILE) {
>              fw_cfg_add_file(s, "bootsplash.jpg",
> -                    boot_splash_filedata, boot_splash_filedata_size);
> +                            boot_splash_filedata, file_size);
>          } else {
>              fw_cfg_add_file(s, "bootsplash.bmp",
> -                    boot_splash_filedata, boot_splash_filedata_size);
> +                            boot_splash_filedata, file_size);
>          }
>          g_free(filename);
>      }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 89604a8328..6065d9e420 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -110,7 +110,6 @@ extern int old_param;
>  extern int boot_menu;
>  extern bool boot_strict;
>  extern uint8_t *boot_splash_filedata;
> -extern size_t boot_splash_filedata_size;
>  extern bool enable_mlock;
>  extern bool enable_cpu_pm;
>  extern QEMUClockType rtc_clock;
> diff --git a/vl.c b/vl.c
> index 4c5cc0d8ad..fad6fec38c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -188,7 +188,6 @@ const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
>  uint8_t *boot_splash_filedata;
> -size_t boot_splash_filedata_size;
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> 

Nice, one global variable less!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2 06/18] hw/nvram/fw_cfg: Remove the unnecessary boot_splash_filedata_size

[Laurent Vivier]

On 08/03/2019 07:49, Thomas Huth wrote:
> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>> The 'boot_splash_filedata_size' was introduced as a global variable
>> in 3d3b8303c6f. This variable is used as a 'size' argument to the
>> fw_cfg_add_file(). This function has an interface contract with his
>> 'data' argument, but there is no such contract for 'size' (this is
>> not a referenced pointer).  We can simply remove it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/nvram/fw_cfg.c       | 5 ++---
>>  include/sysemu/sysemu.h | 1 -
>>  vl.c                    | 1 -
>>  3 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 8eb76a382c..b2dc0a80cb 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -217,15 +217,14 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>>          }
>>          g_free(boot_splash_filedata);
>>          boot_splash_filedata = (uint8_t *)file_data;
>> -        boot_splash_filedata_size = file_size;
>>  
>>          /* insert data */
>>          if (file_type == JPG_FILE) {
>>              fw_cfg_add_file(s, "bootsplash.jpg",
>> -                    boot_splash_filedata, boot_splash_filedata_size);
>> +                            boot_splash_filedata, file_size);
>>          } else {
>>              fw_cfg_add_file(s, "bootsplash.bmp",
>> -                    boot_splash_filedata, boot_splash_filedata_size);
>> +                            boot_splash_filedata, file_size);
>>          }
>>          g_free(filename);
>>      }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 89604a8328..6065d9e420 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -110,7 +110,6 @@ extern int old_param;
>>  extern int boot_menu;
>>  extern bool boot_strict;
>>  extern uint8_t *boot_splash_filedata;
>> -extern size_t boot_splash_filedata_size;
>>  extern bool enable_mlock;
>>  extern bool enable_cpu_pm;
>>  extern QEMUClockType rtc_clock;
>> diff --git a/vl.c b/vl.c
>> index 4c5cc0d8ad..fad6fec38c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -188,7 +188,6 @@ const char *prom_envs[MAX_PROM_ENVS];
>>  int boot_menu;
>>  bool boot_strict;
>>  uint8_t *boot_splash_filedata;
>> -size_t boot_splash_filedata_size;
>>  bool wakeup_suspend_enabled;
>>  
>>  int icount_align_option;
>>
> 
> Nice, one global variable less!
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 06/18] hw/nvram/fw_cfg: Remove the unnecessary boot_splash_filedata_size

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> The 'boot_splash_filedata_size' was introduced as a global variable
> in 3d3b8303c6f. This variable is used as a 'size' argument to the
> fw_cfg_add_file(). This function has an interface contract with his

s/with his/on its/

With that update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 'data' argument, but there is no such contract for 'size' (this is
> not a referenced pointer).  We can simply remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c       | 5 ++---
>  include/sysemu/sysemu.h | 1 -
>  vl.c                    | 1 -
>  3 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 8eb76a382c..b2dc0a80cb 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -217,15 +217,14 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>          }
>          g_free(boot_splash_filedata);
>          boot_splash_filedata = (uint8_t *)file_data;
> -        boot_splash_filedata_size = file_size;
>  
>          /* insert data */
>          if (file_type == JPG_FILE) {
>              fw_cfg_add_file(s, "bootsplash.jpg",
> -                    boot_splash_filedata, boot_splash_filedata_size);
> +                            boot_splash_filedata, file_size);
>          } else {
>              fw_cfg_add_file(s, "bootsplash.bmp",
> -                    boot_splash_filedata, boot_splash_filedata_size);
> +                            boot_splash_filedata, file_size);
>          }
>          g_free(filename);
>      }
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 89604a8328..6065d9e420 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -110,7 +110,6 @@ extern int old_param;
>  extern int boot_menu;
>  extern bool boot_strict;
>  extern uint8_t *boot_splash_filedata;
> -extern size_t boot_splash_filedata_size;
>  extern bool enable_mlock;
>  extern bool enable_cpu_pm;
>  extern QEMUClockType rtc_clock;
> diff --git a/vl.c b/vl.c
> index 4c5cc0d8ad..fad6fec38c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -188,7 +188,6 @@ const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
>  uint8_t *boot_splash_filedata;
> -size_t boot_splash_filedata_size;
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> 

[Qemu-devel] [PATCH v2 07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

[Philippe Mathieu-Daudé]

Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
was no QOM design, object where not created and released at runtime.
Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
adding the fw_cfg_common_realize() method.
The time has come to add the equivalent destructor and release the
memory allocated for 'files'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b2dc0a80cb..0fb020edce 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -959,6 +959,13 @@ static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
     qemu_add_machine_init_done_notifier(&s->machine_ready);
 }
 
+static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
+{
+    FWCfgState *s = FW_CFG(dev);
+
+    g_free(s->files);
+}
+
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as)
 {
@@ -1127,6 +1134,7 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = fw_cfg_io_realize;
+    dc->unrealize = fw_cfg_common_unrealize;
     dc->props = fw_cfg_io_properties;
 }
 
@@ -1190,6 +1198,7 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = fw_cfg_mem_realize;
+    dc->unrealize = fw_cfg_common_unrealize;
     dc->props = fw_cfg_mem_properties;
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

[Thomas Huth]

On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
> was no QOM design, object where not created and released at runtime.
> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
> adding the fw_cfg_common_realize() method.
> The time has come to add the equivalent destructor and release the
> memory allocated for 'files'.

You should mention that the unrealize function is currently never called
since the object never gets destroyed (AFAIK). But I hope we can fix
that in the not so distant future, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

[Laszlo Ersek]

On 03/08/19 07:55, Thomas Huth wrote:
> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
>> was no QOM design, object where not created and released at runtime.

s/object where/objects were/

>> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
>> adding the fw_cfg_common_realize() method.

(I'm not sure if 38f3adc34d "finished the QOM conversion", but that's
just my lack of QOM knowledge. Hopefully someone can confirm whether
this statement is accurate.)

>> The time has come to add the equivalent destructor and release the
>> memory allocated for 'files'.
> 
> You should mention that the unrealize function is currently never called
> since the object never gets destroyed (AFAIK). But I hope we can fix
> that in the not so distant future, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

How about we delay this patch until such time the function is actually
called?

This series is already huge, and quite divergent considering its goals.
(Please refer to the blurb.)

I don't mind this patch, but I think it should either belong to a
separate fw_cfg cleanup series (or "wave" if you like).

Or else, we should have a bug reported somewhere public, ensuring that
we eventually call the function introduced here. And then the commit
message should spell out -- as you say -- that the function is not
called yet, but it will be, because of <ticket-reference>.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/08/19 07:55, Thomas Huth wrote:
>> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>>> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there

Suggest to say "commit abe147e0ce4".

>>> was no QOM design, object where not created and released at runtime.
>
> s/object where/objects were/
>
>>> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
>>> adding the fw_cfg_common_realize() method.
>
> (I'm not sure if 38f3adc34d "finished the QOM conversion", but that's
> just my lack of QOM knowledge. Hopefully someone can confirm whether
> this statement is accurate.)
>
>>> The time has come to add the equivalent destructor and release the
>>> memory allocated for 'files'.
>> 
>> You should mention that the unrealize function is currently never called
>> since the object never gets destroyed (AFAIK). But I hope we can fix
>> that in the not so distant future, so:
>> 
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> 
>
> How about we delay this patch until such time the function is actually
> called?
>
> This series is already huge, and quite divergent considering its goals.
> (Please refer to the blurb.)
>
> I don't mind this patch, but I think it should either belong to a
> separate fw_cfg cleanup series (or "wave" if you like).
>
> Or else, we should have a bug reported somewhere public, ensuring that
> we eventually call the function introduced here. And then the commit
> message should spell out -- as you say -- that the function is not
> called yet, but it will be, because of <ticket-reference>.

I suspect filing a bug "somewhere" and making use of it would be much
more work than simply applying the fix.

I'm fine with including trivial patches for stuff you spot "on the way".
I'd be also fine with some patches spun off into a separate cleanup
series that László doesn't have to review, if you can do that safely,
and with little effort.

Re: [Qemu-devel] [PATCH v2 07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
> was no QOM design, object where not created and released at runtime.
> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
> adding the fw_cfg_common_realize() method.
> The time has come to add the equivalent destructor and release the
> memory allocated for 'files'.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b2dc0a80cb..0fb020edce 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -959,6 +959,13 @@ static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
>  }
>  
> +static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
> +{
> +    FWCfgState *s = FW_CFG(dev);
> +
> +    g_free(s->files);
> +}
> +

Still leaks at least s->entries[0], s->entries[1], s->entry_order,
doesn't it?

>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
> @@ -1127,6 +1134,7 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = fw_cfg_io_realize;
> +    dc->unrealize = fw_cfg_common_unrealize;
>      dc->props = fw_cfg_io_properties;
>  }
>  
> @@ -1190,6 +1198,7 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = fw_cfg_mem_realize;
> +    dc->unrealize = fw_cfg_common_unrealize;
>      dc->props = fw_cfg_mem_properties;
>  }

[Qemu-devel] [PATCH v2 08/18] hw/nvram/fw_cfg: Move fw_cfg_file_slots_allocate() to common_realize()

[Philippe Mathieu-Daudé]

Each implementation (I/O and MEM) calls fw_cfg_file_slots_allocate()
then fw_cfg_common_realize().
Simplify by moving the fw_cfg_file_slots_allocate() call into
fw_cfg_common_realize() where it belongs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0fb020edce..ca58d279a4 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -929,19 +929,26 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
     qemu_register_reset(fw_cfg_machine_reset, s);
 }
 
-
+static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp);
 
 static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
+    Error *local_err = NULL;
 
     if (!fw_cfg_find()) {
         error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
         return;
     }
 
+    fw_cfg_file_slots_allocate(s, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
@@ -1108,7 +1115,7 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
     FWCfgIoState *s = FW_CFG_IO(dev);
     Error *local_err = NULL;
 
-    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
+    fw_cfg_common_realize(dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1125,8 +1132,6 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
-
-    fw_cfg_common_realize(dev, errp);
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1162,7 +1167,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
     Error *local_err = NULL;
 
-    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
+    fw_cfg_common_realize(dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1189,8 +1194,6 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
-
-    fw_cfg_common_realize(dev, errp);
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 08/18] hw/nvram/fw_cfg: Move fw_cfg_file_slots_allocate() to common_realize()

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Each implementation (I/O and MEM) calls fw_cfg_file_slots_allocate()
> then fw_cfg_common_realize().
> Simplify by moving the fw_cfg_file_slots_allocate() call into
> fw_cfg_common_realize() where it belongs.

The patch does more than that, namely:

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 0fb020edce..ca58d279a4 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -929,19 +929,26 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
>      qemu_register_reset(fw_cfg_machine_reset, s);
>  }
>  
> -
> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp);
>  
>  static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgState *s = FW_CFG(dev);
>      MachineState *machine = MACHINE(qdev_get_machine());
>      uint32_t version = FW_CFG_VERSION;
> +    Error *local_err = NULL;
>  
>      if (!fw_cfg_find()) {
>          error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
>          return;
>      }
>  
> +    fw_cfg_file_slots_allocate(s, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
>      fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> @@ -1108,7 +1115,7 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>      FWCfgIoState *s = FW_CFG_IO(dev);
>      Error *local_err = NULL;
>  
> -    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    fw_cfg_common_realize(dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1125,8 +1132,6 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
>      }
> -
> -    fw_cfg_common_realize(dev, errp);
>  }

it moves the fw_cfg_common_realize() call from the ends of the IO/MEM
realize functions to their middles; in particular to the point before
memory regions and (when appropriate) MMIO resources are initialized.

In other words, the patch reorders all of the current actions in
fw_cfg_common_realize() against said MemoryRegion & SysBusDevice
resource actions.

Why is this safe? (I'm not claiming it is unsafe, but I'd like to see it
explained in the commit message.)

Note: the fw_cfg_common_realize() you are moving around was introduced
in commit 38f3adc34de8 ("fw_cfg: move qdev_init_nofail() from
fw_cfg_init1() to callers", 2017-07-17), by Mark Cave-Ayland. I see Mark
is CC'd already, so I hope he can comment.

Thanks,
Laszlo

>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1162,7 +1167,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
>      Error *local_err = NULL;
>  
> -    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    fw_cfg_common_realize(dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1189,8 +1194,6 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> -
> -    fw_cfg_common_realize(dev, errp);
>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> 

[Qemu-devel] [PATCH v2 09/18] hw/nvram/fw_cfg: Free file_slots in common_unrealize()

[Philippe Mathieu-Daudé]

Called by fw_cfg_common_realize(), fw_cfg_file_slots_allocate()
allocates various buffers.
Free them in fw_cfg_common_unrealize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index ca58d279a4..b73a591eff 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -971,6 +971,10 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
     FWCfgState *s = FW_CFG(dev);
 
     g_free(s->files);
+
+    g_free(s->entries[0]);
+    g_free(s->entries[1]);
+    g_free(s->entry_order);
 }
 
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 09/18] hw/nvram/fw_cfg: Free file_slots in common_unrealize()

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Called by fw_cfg_common_realize(), fw_cfg_file_slots_allocate()
> allocates various buffers.
> Free them in fw_cfg_common_unrealize().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index ca58d279a4..b73a591eff 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -971,6 +971,10 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
>      FWCfgState *s = FW_CFG(dev);
>  
>      g_free(s->files);
> +
> +    g_free(s->entries[0]);
> +    g_free(s->entries[1]);
> +    g_free(s->entry_order);
>  }
>  
>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState

[Philippe Mathieu-Daudé]

Due to the contract interface of fw_cfg_add_file(), the
'reboot_timeout' data has to be valid for the lifetime of the
FwCfg object. For this reason it is copied on the heap with
memdup().

The object state, 'FWCfgState', is also meant to be valid during the
lifetime of the object.
Move the 'reboot_timeout' in FWCfgState to achieve the same purpose.
Doing so we avoid a memory leak.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c         | 4 +++-
 include/hw/nvram/fw_cfg.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b73a591eff..182d27f59a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s)
         }
     }
 
-    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
+    s->reboot_timeout = rt_val;
+    fw_cfg_add_file(s, "etc/boot-fail-wait",
+                    &s->reboot_timeout, sizeof(s->reboot_timeout));
 }
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 828ad9dedc..99f6fafcaa 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -53,6 +53,8 @@ struct FWCfgState {
     dma_addr_t dma_addr;
     AddressSpace *dma_as;
     MemoryRegion dma_iomem;
+
+    uint32_t reboot_timeout;
 };
 
 struct FWCfgIoState {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState

[Laszlo Ersek]

Hi Phil,

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Due to the contract interface of fw_cfg_add_file(), the
> 'reboot_timeout' data has to be valid for the lifetime of the
> FwCfg object. For this reason it is copied on the heap with
> memdup().
> 
> The object state, 'FWCfgState', is also meant to be valid during the
> lifetime of the object.
> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose.
> Doing so we avoid a memory leak.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 4 +++-
>  include/hw/nvram/fw_cfg.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)

Currently, there is no memory leak. Right now, the leak is theoretical,
and it would depend on the fw_cfg object being actually destroyed.

I think armoring the fw_cfg implementation for such lifetime actions is
valuable. But, that definitely belongs to its own series, in my opinion.

In the "hw/nvram/fw_cfg.c" file, I count:

(a) two "specific purpose" g_memdup() calls, namely in
fw_cfg_bootsplash() and in fw_cfg_reboot();

(b) one "generic purpose" g_memdup() call, namely in fw_cfg_add_string();

(c) two "generic purpose" g_malloc() calls, namely in fw_cfg_add_i16(),
fw_cfg_add_i32(), and fw_cfg_add_i64(). (The one in fw_cfg_modify_i16()
does not matter here because the previous blob is freed in that function.)

Your series deals with (a), namely with fw_cfg_reboot() in this patch,
and with fw_cfg_bootsplash() in the next one.

Your series deals with neither (b) nor (c). The
fw_cfg_add_(string|i16|i32|i64) functions are called from a bunch of
places however, so if we really intend *not* to leak those copies upon
fw_cfg destruction, then we'll have to track all of them dynamically, in
a list for example.

(And that necessitates a separate series for this topic even more.)

In turn, once we add dynamic tracking, for those blobs that the
fw_cfg_add_(string|i16|i32|i64) functions allocate internally -- as they
are advertized to do --, then we might as well use the same tracking
infrastructure for (a). In other words, it should not be necessary to
add the specific fields "reboot_timeout" and "boot_splash" to FWCfgState.

Thanks,
Laszlo

> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b73a591eff..182d27f59a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>          }
>      }
>  
> -    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
> +    s->reboot_timeout = rt_val;
> +    fw_cfg_add_file(s, "etc/boot-fail-wait",
> +                    &s->reboot_timeout, sizeof(s->reboot_timeout));
>  }
>  
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 828ad9dedc..99f6fafcaa 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -53,6 +53,8 @@ struct FWCfgState {
>      dma_addr_t dma_addr;
>      AddressSpace *dma_as;
>      MemoryRegion dma_iomem;
> +
> +    uint32_t reboot_timeout;
>  };
>  
>  struct FWCfgIoState {
> 

Re: [Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState

[Philippe Mathieu-Daudé]

On 3/8/19 12:04 PM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
>> Due to the contract interface of fw_cfg_add_file(), the
>> 'reboot_timeout' data has to be valid for the lifetime of the
>> FwCfg object. For this reason it is copied on the heap with
>> memdup().
>>
>> The object state, 'FWCfgState', is also meant to be valid during the
>> lifetime of the object.
>> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose.
>> Doing so we avoid a memory leak.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/nvram/fw_cfg.c         | 4 +++-
>>  include/hw/nvram/fw_cfg.h | 2 ++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> Currently, there is no memory leak. Right now, the leak is theoretical,
> and it would depend on the fw_cfg object being actually destroyed.

Actually my first motivation came while using valgrind, there are a
bunch of warnings related to the fw_cfg device.
This device is not hotpluggable however, and we don't test it in the
device-introspect-test.

> I think armoring the fw_cfg implementation for such lifetime actions is
> valuable. But, that definitely belongs to its own series, in my opinion.
> 
> In the "hw/nvram/fw_cfg.c" file, I count:
> 
> (a) two "specific purpose" g_memdup() calls, namely in
> fw_cfg_bootsplash() and in fw_cfg_reboot();
> 
> (b) one "generic purpose" g_memdup() call, namely in fw_cfg_add_string();
> 
> (c) two "generic purpose" g_malloc() calls, namely in fw_cfg_add_i16(),
> fw_cfg_add_i32(), and fw_cfg_add_i64(). (The one in fw_cfg_modify_i16()
> does not matter here because the previous blob is freed in that function.)
> 
> Your series deals with (a), namely with fw_cfg_reboot() in this patch,
> and with fw_cfg_bootsplash() in the next one.
> 
> Your series deals with neither (b) nor (c). The

I did a PoC of (b) and (c) but it is a more invasive patchset indeed.

> fw_cfg_add_(string|i16|i32|i64) functions are called from a bunch of
> places however, so if we really intend *not* to leak those copies upon
> fw_cfg destruction, then we'll have to track all of them dynamically, in
> a list for example.

I haven't think of using a list.

> (And that necessitates a separate series for this topic even more.)

OK.

> In turn, once we add dynamic tracking, for those blobs that the
> fw_cfg_add_(string|i16|i32|i64) functions allocate internally -- as they
> are advertized to do --, then we might as well use the same tracking
> infrastructure for (a). In other words, it should not be necessary to
> add the specific fields "reboot_timeout" and "boot_splash" to FWCfgState.

OK, I'll drop these patches from this series.

> 
> Thanks,
> Laszlo
> 
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index b73a591eff..182d27f59a 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>>          }
>>      }
>>  
>> -    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
>> +    s->reboot_timeout = rt_val;
>> +    fw_cfg_add_file(s, "etc/boot-fail-wait",
>> +                    &s->reboot_timeout, sizeof(s->reboot_timeout));
>>  }
>>  
>>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index 828ad9dedc..99f6fafcaa 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -53,6 +53,8 @@ struct FWCfgState {
>>      dma_addr_t dma_addr;
>>      AddressSpace *dma_as;
>>      MemoryRegion dma_iomem;
>> +
>> +    uint32_t reboot_timeout;
>>  };
>>  
>>  struct FWCfgIoState {
>>
> 

Re: [Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState

[Philippe Mathieu-Daudé]

On 3/8/19 12:22 PM, Philippe Mathieu-Daudé wrote:
> On 3/8/19 12:04 PM, Laszlo Ersek wrote:
>> Hi Phil,
>>
>> On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
>>> Due to the contract interface of fw_cfg_add_file(), the
>>> 'reboot_timeout' data has to be valid for the lifetime of the
>>> FwCfg object. For this reason it is copied on the heap with
>>> memdup().
>>>
>>> The object state, 'FWCfgState', is also meant to be valid during the
>>> lifetime of the object.
>>> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose.
>>> Doing so we avoid a memory leak.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/nvram/fw_cfg.c         | 4 +++-
>>>  include/hw/nvram/fw_cfg.h | 2 ++
>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> Currently, there is no memory leak. Right now, the leak is theoretical,
>> and it would depend on the fw_cfg object being actually destroyed.
> 
> Actually my first motivation came while using valgrind, there are a
> bunch of warnings related to the fw_cfg device.
> This device is not hotpluggable however, and we don't test it in the
> device-introspect-test.

IOW this device makes finding memory leaks in other introspectable
devices harder.

>> I think armoring the fw_cfg implementation for such lifetime actions is
>> valuable. But, that definitely belongs to its own series, in my opinion.
>>
>> In the "hw/nvram/fw_cfg.c" file, I count:
>>
>> (a) two "specific purpose" g_memdup() calls, namely in
>> fw_cfg_bootsplash() and in fw_cfg_reboot();
>>
>> (b) one "generic purpose" g_memdup() call, namely in fw_cfg_add_string();
>>
>> (c) two "generic purpose" g_malloc() calls, namely in fw_cfg_add_i16(),
>> fw_cfg_add_i32(), and fw_cfg_add_i64(). (The one in fw_cfg_modify_i16()
>> does not matter here because the previous blob is freed in that function.)
>>
>> Your series deals with (a), namely with fw_cfg_reboot() in this patch,
>> and with fw_cfg_bootsplash() in the next one.
>>
>> Your series deals with neither (b) nor (c). The
> 
> I did a PoC of (b) and (c) but it is a more invasive patchset indeed.
> 
>> fw_cfg_add_(string|i16|i32|i64) functions are called from a bunch of
>> places however, so if we really intend *not* to leak those copies upon
>> fw_cfg destruction, then we'll have to track all of them dynamically, in
>> a list for example.
> 
> I haven't think of using a list.
> 
>> (And that necessitates a separate series for this topic even more.)
> 
> OK.
> 
>> In turn, once we add dynamic tracking, for those blobs that the
>> fw_cfg_add_(string|i16|i32|i64) functions allocate internally -- as they
>> are advertized to do --, then we might as well use the same tracking
>> infrastructure for (a). In other words, it should not be necessary to
>> add the specific fields "reboot_timeout" and "boot_splash" to FWCfgState.
> 
> OK, I'll drop these patches from this series.
> 
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index b73a591eff..182d27f59a 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>>>          }
>>>      }
>>>  
>>> -    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
>>> +    s->reboot_timeout = rt_val;
>>> +    fw_cfg_add_file(s, "etc/boot-fail-wait",
>>> +                    &s->reboot_timeout, sizeof(s->reboot_timeout));
>>>  }
>>>  
>>>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>>> index 828ad9dedc..99f6fafcaa 100644
>>> --- a/include/hw/nvram/fw_cfg.h
>>> +++ b/include/hw/nvram/fw_cfg.h
>>> @@ -53,6 +53,8 @@ struct FWCfgState {
>>>      dma_addr_t dma_addr;
>>>      AddressSpace *dma_as;
>>>      MemoryRegion dma_iomem;
>>> +
>>> +    uint32_t reboot_timeout;
>>>  };
>>>  
>>>  struct FWCfgIoState {
>>>
>>

Re: [Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState

[Michael S. Tsirkin]

On Fri, Mar 08, 2019 at 02:32:14AM +0100, Philippe Mathieu-Daudé wrote:
> Due to the contract interface of fw_cfg_add_file(), the
> 'reboot_timeout' data has to be valid for the lifetime of the
> FwCfg object. For this reason it is copied on the heap with
> memdup().
> 
> The object state, 'FWCfgState', is also meant to be valid during the
> lifetime of the object.
> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose.
> Doing so we avoid a memory leak.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I don't like adding random per-file state to fw cfg at all.

I don't see a leak. Please add more explanation about this.
And maybe split from this series.

> ---
>  hw/nvram/fw_cfg.c         | 4 +++-
>  include/hw/nvram/fw_cfg.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b73a591eff..182d27f59a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>          }
>      }
>  
> -    fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4);
> +    s->reboot_timeout = rt_val;
> +    fw_cfg_add_file(s, "etc/boot-fail-wait",
> +                    &s->reboot_timeout, sizeof(s->reboot_timeout));
>  }
>  
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 828ad9dedc..99f6fafcaa 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -53,6 +53,8 @@ struct FWCfgState {
>      dma_addr_t dma_addr;
>      AddressSpace *dma_as;
>      MemoryRegion dma_iomem;
> +
> +    uint32_t reboot_timeout;
>  };
>  
>  struct FWCfgIoState {
> -- 
> 2.20.1

[Qemu-devel] [PATCH v2 11/18] hw/nvram/fw_cfg: Add boot_splash.time_le16 to FWCfgState

[Philippe Mathieu-Daudé]

Similar to the previous commit, use the FWCfgState lifetime state
to hold the 'bst_le16' variable content (renaned as time_le16).
Doing so we avoid a memory leak.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c         | 6 +++---
 include/hw/nvram/fw_cfg.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 182d27f59a..3ac6687a04 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -187,7 +187,6 @@ static void fw_cfg_bootsplash(FWCfgState *s)
     /* insert splash time if user configurated */
     if (boot_splash_time) {
         int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
-        uint16_t bst_le16;
 
         /* validate the input */
         if (bst_val < 0 || bst_val > 0xffff) {
@@ -196,9 +195,10 @@ static void fw_cfg_bootsplash(FWCfgState *s)
             exit(1);
         }
         /* use little endian format */
-        bst_le16 = cpu_to_le16(bst_val);
+        s->boot_splash.time_le16 = cpu_to_le16(bst_val);
         fw_cfg_add_file(s, "etc/boot-menu-wait",
-                        g_memdup(&bst_le16, sizeof bst_le16), sizeof bst_le16);
+                        &s->boot_splash.time_le16,
+                        sizeof(s->boot_splash.time_le16));
     }
 
     /* insert splash file if user configurated */
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 99f6fafcaa..fcb771186c 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -55,6 +55,9 @@ struct FWCfgState {
     MemoryRegion dma_iomem;
 
     uint32_t reboot_timeout;
+    struct {
+        uint16_t time_le16;
+    } boot_splash;
 };
 
 struct FWCfgIoState {
-- 
2.20.1

[Qemu-devel] [PATCH v2 12/18] hw/nvram/fw_cfg: Keep reference of file_data in FWCfgState

[Philippe Mathieu-Daudé]

The 'file_data' is allocated by read_splashfile() (introduced in
commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
to the contract interface of fw_cfg_add_file(), it has to be valid
for the lifetime of the FwCfg object.

Keep a reference of 'file_data' in FWCfgState to be able to
free this memory in fw_cfg_common_unrealize().
We can now remove the res_free() from the main() loop.
The global boot_splash_filedata is now unused, remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c         | 10 ++++++----
 include/hw/nvram/fw_cfg.h |  1 +
 include/sysemu/sysemu.h   |  1 -
 vl.c                      |  9 ---------
 4 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 3ac6687a04..fc392cb7e0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -215,16 +215,16 @@ static void fw_cfg_bootsplash(FWCfgState *s)
             g_free(filename);
             return;
         }
-        g_free(boot_splash_filedata);
-        boot_splash_filedata = (uint8_t *)file_data;
+        g_free(s->boot_splash.file_data);
+        s->boot_splash.file_data = file_data;
 
         /* insert data */
         if (file_type == JPG_FILE) {
             fw_cfg_add_file(s, "bootsplash.jpg",
-                            boot_splash_filedata, file_size);
+                            s->boot_splash.file_data, file_size);
         } else {
             fw_cfg_add_file(s, "bootsplash.bmp",
-                            boot_splash_filedata, file_size);
+                            s->boot_splash.file_data, file_size);
         }
         g_free(filename);
     }
@@ -974,6 +974,8 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
 
     g_free(s->files);
 
+    g_free(s->boot_splash.file_data);
+
     g_free(s->entries[0]);
     g_free(s->entries[1]);
     g_free(s->entry_order);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fcb771186c..83a0540b6c 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -56,6 +56,7 @@ struct FWCfgState {
 
     uint32_t reboot_timeout;
     struct {
+        char *file_data;
         uint16_t time_le16;
     } boot_splash;
 };
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6065d9e420..3cd856b015 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -109,7 +109,6 @@ extern int no_shutdown;
 extern int old_param;
 extern int boot_menu;
 extern bool boot_strict;
-extern uint8_t *boot_splash_filedata;
 extern bool enable_mlock;
 extern bool enable_cpu_pm;
 extern QEMUClockType rtc_clock;
diff --git a/vl.c b/vl.c
index fad6fec38c..47dd63a309 100644
--- a/vl.c
+++ b/vl.c
@@ -187,7 +187,6 @@ unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
 bool boot_strict;
-uint8_t *boot_splash_filedata;
 bool wakeup_suspend_enabled;
 
 int icount_align_option;
@@ -558,12 +557,6 @@ const char *qemu_get_vm_name(void)
     return qemu_name;
 }
 
-static void res_free(void)
-{
-    g_free(boot_splash_filedata);
-    boot_splash_filedata = NULL;
-}
-
 static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *driver = qemu_opt_get(opts, "driver");
@@ -4591,8 +4584,6 @@ int main(int argc, char **argv, char **envp)
     job_cancel_sync_all();
     bdrv_close_all();
 
-    res_free();
-
     /* vhost-user must be cleaned up before chardevs.  */
     tpm_cleanup();
     net_cleanup();
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 12/18] hw/nvram/fw_cfg: Keep reference of file_data in FWCfgState

[Thomas Huth]

On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> The 'file_data' is allocated by read_splashfile() (introduced in
> commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
> to the contract interface of fw_cfg_add_file(), it has to be valid
> for the lifetime of the FwCfg object.
> 
> Keep a reference of 'file_data' in FWCfgState to be able to
> free this memory in fw_cfg_common_unrealize().
> We can now remove the res_free() from the main() loop.
> The global boot_splash_filedata is now unused, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 10 ++++++----
>  include/hw/nvram/fw_cfg.h |  1 +
>  include/sysemu/sysemu.h   |  1 -
>  vl.c                      |  9 ---------
>  4 files changed, 7 insertions(+), 14 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 12/18] hw/nvram/fw_cfg: Keep reference of file_data in FWCfgState

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> The 'file_data' is allocated by read_splashfile() (introduced in
> commit 3d3b8303c6f8).  It is then used by fw_cfg_add_file(). Due
> to the contract interface of fw_cfg_add_file(), it has to be valid
> for the lifetime of the FwCfg object.
> 
> Keep a reference of 'file_data' in FWCfgState to be able to
> free this memory in fw_cfg_common_unrealize().
> We can now remove the res_free() from the main() loop.
> The global boot_splash_filedata is now unused, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c         | 10 ++++++----
>  include/hw/nvram/fw_cfg.h |  1 +
>  include/sysemu/sysemu.h   |  1 -
>  vl.c                      |  9 ---------
>  4 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3ac6687a04..fc392cb7e0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -215,16 +215,16 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>              g_free(filename);
>              return;
>          }
> -        g_free(boot_splash_filedata);
> -        boot_splash_filedata = (uint8_t *)file_data;
> +        g_free(s->boot_splash.file_data);
> +        s->boot_splash.file_data = file_data;
>  
>          /* insert data */
>          if (file_type == JPG_FILE) {
>              fw_cfg_add_file(s, "bootsplash.jpg",
> -                            boot_splash_filedata, file_size);
> +                            s->boot_splash.file_data, file_size);
>          } else {
>              fw_cfg_add_file(s, "bootsplash.bmp",
> -                            boot_splash_filedata, file_size);
> +                            s->boot_splash.file_data, file_size);
>          }
>          g_free(filename);
>      }
> @@ -974,6 +974,8 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
>  
>      g_free(s->files);
>  
> +    g_free(s->boot_splash.file_data);
> +
>      g_free(s->entries[0]);
>      g_free(s->entries[1]);
>      g_free(s->entry_order);
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index fcb771186c..83a0540b6c 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -56,6 +56,7 @@ struct FWCfgState {
>  
>      uint32_t reboot_timeout;
>      struct {
> +        char *file_data;
>          uint16_t time_le16;
>      } boot_splash;
>  };
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6065d9e420..3cd856b015 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -109,7 +109,6 @@ extern int no_shutdown;
>  extern int old_param;
>  extern int boot_menu;
>  extern bool boot_strict;
> -extern uint8_t *boot_splash_filedata;
>  extern bool enable_mlock;
>  extern bool enable_cpu_pm;
>  extern QEMUClockType rtc_clock;
> diff --git a/vl.c b/vl.c
> index fad6fec38c..47dd63a309 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -187,7 +187,6 @@ unsigned int nb_prom_envs = 0;
>  const char *prom_envs[MAX_PROM_ENVS];
>  int boot_menu;
>  bool boot_strict;
> -uint8_t *boot_splash_filedata;
>  bool wakeup_suspend_enabled;
>  
>  int icount_align_option;
> @@ -558,12 +557,6 @@ const char *qemu_get_vm_name(void)
>      return qemu_name;
>  }
>  
> -static void res_free(void)
> -{
> -    g_free(boot_splash_filedata);
> -    boot_splash_filedata = NULL;
> -}
> -
>  static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      const char *driver = qemu_opt_get(opts, "driver");
> @@ -4591,8 +4584,6 @@ int main(int argc, char **argv, char **envp)
>      job_cancel_sync_all();
>      bdrv_close_all();
>  
> -    res_free();
> -
>      /* vhost-user must be cleaned up before chardevs.  */
>      tpm_cleanup();
>      net_cleanup();
> 

Referring to the earlier thread

  [Qemu-devel] [PATCH] hw/nvram/fw_cfg: Move boot_splash_filedata
                       variables into fw_cfg.c
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg599282.html

my opinion is that *all* of the g_free() calls touched in this patch are
presently -- that is, pre-patch -- bogus:

- As I wrote earlier, res_free() may be reached, but the freeing it does
is useless.

- Furthermore, the g_free() call in fw_cfg_bootsplash() never frees
anything in reality. It is only called from fw_cfg_common_realize(), and
we only have one fw_cfg object (which is never destructed, for now).

So, first I would kill these bogus g_free()s altogether, in a separate
patch (in the separate series that I've recommended elsewhere). Then, in
a second patch (in the separate series), I would include the boot splash
image among the dynamically tracked allocations. Just add it to a linked
list, and when the fw_cfg object is destroyed, release it with the rest.

(Another note (and I should have made it earlier): fw_cfg_modify_i16()
will face a challenge; it will have to update the tracker data structure
too.)

Thanks
Laszlo

[Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

When debugging a paravirtualized guest firmware, it results very
useful to dump the fw_cfg table.
Add a QMP command which returns the most useful fields.
Since the QMP protocol is not designed for passing stream data,
we don't display a fw_cfg item data, only it's size:

{ "execute": "query-fw_cfg-items" }
{
    "return": [
        {
            "architecture_specific": false,
            "key": 0,
            "writeable": false,
            "size": 4,
            "keyname": "signature"
        },
        {
            "architecture_specific": false,
            "key": 1,
            "writeable": false,
            "size": 4,
            "keyname": "id"
        },
        {
            "architecture_specific": false,
            "key": 2,
            "writeable": false,
            "size": 16,
            "keyname": "uuid"
        },
        ...
        {
            "order": 40,
            "architecture_specific": false,
            "key": 36,
            "writeable": false,
            "path": "etc/e820",
            "size": 20,
            "keyname": "file"
        },
        {
            "order": 30,
            "architecture_specific": false,
            "key": 37,
            "writeable": false,
            "path": "etc/smbios/smbios-anchor",
            "size": 31,
            "keyname": "file"
        },
        ...
        {
            "architecture_specific": true,
            "key": 3,
            "writeable": false,
            "size": 324,
            "keyname": "e820_tables"
        },
        {
            "architecture_specific": true,
            "key": 4,
            "writeable": false,
            "size": 121,
            "keyname": "hpet"
        }
    ]
}

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: New commit, asked by Eric/Michael, using Laszlo suggestions
---
 hw/nvram/fw_cfg.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json    | 44 +++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index fc392cb7e0..2a8d69ba07 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -35,6 +35,7 @@
 #include "qemu/config-file.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-misc.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
@@ -1229,3 +1230,78 @@ static void fw_cfg_register_types(void)
 }
 
 type_init(fw_cfg_register_types)
+
+static FirmwareConfigurationItem *create_qmp_fw_cfg_item(FWCfgState *s,
+                                                         FWCfgEntry *e,
+                                                         bool is_arch_specific,
+                                                         uint16_t key,
+                                                         size_t hex_length)
+{
+    FirmwareConfigurationItem *item = g_malloc0(sizeof(*item));
+
+    item->key = key;
+    item->writeable = e->allow_write;
+    item->architecture_specific = is_arch_specific;
+    item->size = e->len;
+    if (hex_length) {
+        item->has_data = true;
+        item->data = qemu_strdup_hexlify(e->data, hex_length);
+    }
+
+    if (!is_arch_specific && key >= FW_CFG_FILE_FIRST) {
+        int id = key - FW_CFG_FILE_FIRST;
+        const char *path = s->files->f[id].name;
+
+        item->has_keyname = true;
+        item->keyname = g_strdup("file");
+        item->has_order = true;
+        item->order = get_fw_cfg_order(s, path);
+        item->has_path = true;
+        item->path = g_strdup(path);
+    } else {
+        const char *name;
+
+        if (is_arch_specific) {
+            key |= FW_CFG_ARCH_LOCAL;
+        }
+        name = key_name(key);
+        if (name) {
+            item->has_keyname = true;
+            item->keyname = g_strdup(name);
+        }
+    }
+    return item;
+}
+
+FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
+{
+    FirmwareConfigurationItemList *item_list = NULL;
+    uint32_t max_entries;
+    int arch, key;
+    FWCfgState *s = fw_cfg_find();
+
+    if (s == NULL) {
+        return NULL;
+    }
+
+    max_entries = fw_cfg_max_entry(s);
+    for (arch = ARRAY_SIZE(s->entries) - 1; arch >= 0 ; --arch) {
+        for (key = max_entries - 1; key >= 0; --key) {
+            FirmwareConfigurationItemList *info;
+            FWCfgEntry *e = &s->entries[arch][key];
+            size_t qmp_hex_length = 0;
+
+            if (!e->len) {
+                continue;
+            }
+
+            info = g_malloc0(sizeof(*info));
+            info->value = create_qmp_fw_cfg_item(s, e, arch, key,
+                                                 qmp_hex_length);
+            info->next = item_list;
+            item_list = info;
+        }
+    }
+
+    return item_list;
+}
diff --git a/qapi/misc.json b/qapi/misc.json
index 8b3ca4fdd3..9d1da7c766 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3051,3 +3051,47 @@
   'data': 'NumaOptions',
   'allow-preconfig': true
 }
+
+##
+# @FirmwareConfigurationItem:
+#
+# Firmware Configuration (fw_cfg) item.
+#
+# @key: The uint16 selector key.
+# @keyname: The stringified name if the selector refers to a well-known
+#           numerically defined item.
+# @architecture_specific: Indicates whether the configuration setting is
+#                         architecture specific.
+#                  false: The item is a generic configuration item.
+#                  true:  The item is specific to a particular architecture.
+# @writeable: Indicates whether the configuration setting is writeable by
+#             the guest.
+# @size: The length of @data associated with the item.
+# @data: A string representating the firmware configuration data.
+#        Note: This field is currently not used.
+# @path: If the key is a 'file', the named file path.
+# @order: If the key is a 'file', the named file order.
+#
+# Since 4.0
+##
+{ 'struct': 'FirmwareConfigurationItem',
+  'data': { 'key': 'uint16',
+            '*keyname': 'str',
+            'architecture_specific': 'bool',
+            'writeable': 'bool',
+            '*data': 'str',
+            'size': 'int',
+            '*path': 'str',
+            '*order': 'int' } }
+
+
+##
+# @query-fw_cfg-items:
+#
+# Returns the list of Firmware Configuration items.
+#
+# Returns: A list of @FirmwareConfigurationItem for each entry.
+#
+# Since 4.0
+##
+{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Eric Blake]

On 3/7/19 7:32 PM, Philippe Mathieu-Daudé wrote:
> When debugging a paravirtualized guest firmware, it results very
> useful to dump the fw_cfg table.
> Add a QMP command which returns the most useful fields.
> Since the QMP protocol is not designed for passing stream data,
> we don't display a fw_cfg item data, only it's size:
> 
> { "execute": "query-fw_cfg-items" }
> {
>     "return": [
>         {
>             "architecture_specific": false,
>             "key": 0,
>             "writeable": false,
>             "size": 4,
>             "keyname": "signature"

You could return a base64-encoded representation of the field (we do
that in other interfaces where raw binary can't be passed reliably over
JSON).  For 4 bytes, that makes sense,


>         {
>             "architecture_specific": true,
>             "key": 3,
>             "writeable": false,
>             "size": 324,
>             "keyname": "e820_tables"

for 324 bytes, it gets a bit long. Still, may be worth it for the added
debug visibility.


> +++ b/qapi/misc.json
> @@ -3051,3 +3051,47 @@
>    'data': 'NumaOptions',
>    'allow-preconfig': true
>  }
> +
> +##
> +# @FirmwareConfigurationItem:
> +#
> +# Firmware Configuration (fw_cfg) item.
> +#
> +# @key: The uint16 selector key.
> +# @keyname: The stringified name if the selector refers to a well-known
> +#           numerically defined item.
> +# @architecture_specific: Indicates whether the configuration setting is

Prefer '-' over '_' in new interfaces.

> +#                         architecture specific.
> +#                  false: The item is a generic configuration item.
> +#                  true:  The item is specific to a particular architecture.
> +# @writeable: Indicates whether the configuration setting is writeable by
> +#             the guest.

writable

> +# @size: The length of @data associated with the item.
> +# @data: A string representating the firmware configuration data.

representing

> +#        Note: This field is currently not used.

Either drop the field until it has a use, or let it be the base64
encoding and use it now.

> +# @path: If the key is a 'file', the named file path.
> +# @order: If the key is a 'file', the named file order.
> +#
> +# Since 4.0
> +##
> +{ 'struct': 'FirmwareConfigurationItem',
> +  'data': { 'key': 'uint16',
> +            '*keyname': 'str',
> +            'architecture_specific': 'bool',
> +            'writeable': 'bool',
> +            '*data': 'str',
> +            'size': 'int',
> +            '*path': 'str',
> +            '*order': 'int' } }

Is it worth making 'keyname' an enum type, and turning this into a flat
union where 'path' and 'order' appear on the branch associated with
'file', and where all other well-known keynames have smaller branches?


> +
> +
> +##
> +# @query-fw_cfg-items:

That looks weird to mix - and _. Any reason we can't just go with
query-firmware-config?

> +#
> +# Returns the list of Firmware Configuration items.
> +#
> +# Returns: A list of @FirmwareConfigurationItem for each entry.
> +#
> +# Since 4.0
> +##
> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

Hi Eric,

On 3/8/19 3:04 AM, Eric Blake wrote:
> On 3/7/19 7:32 PM, Philippe Mathieu-Daudé wrote:
>> When debugging a paravirtualized guest firmware, it results very
>> useful to dump the fw_cfg table.
>> Add a QMP command which returns the most useful fields.
>> Since the QMP protocol is not designed for passing stream data,
>> we don't display a fw_cfg item data, only it's size:
>>
>> { "execute": "query-fw_cfg-items" }
>> {
>>     "return": [
>>         {
>>             "architecture_specific": false,
>>             "key": 0,
>>             "writeable": false,
>>             "size": 4,
>>             "keyname": "signature"
> 
> You could return a base64-encoded representation of the field (we do
> that in other interfaces where raw binary can't be passed reliably over
> JSON).  For 4 bytes, that makes sense,
> 
> 
>>         {
>>             "architecture_specific": true,
>>             "key": 3,
>>             "writeable": false,
>>             "size": 324,
>>             "keyname": "e820_tables"
> 
> for 324 bytes, it gets a bit long. Still, may be worth it for the added
> debug visibility.

Until you see values in the next patch...:

$ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
[...]
 0x0019   file_dir                           RO    2052       0000000b..
 0x0022   file: etc/acpi/tables              RO  131072  130  46414353..
 0x0028   file: etc/table-loader             RO    4096  140  01000000..

What about using a 512B limit on this QMP answer?

>> +++ b/qapi/misc.json
>> @@ -3051,3 +3051,47 @@
>>    'data': 'NumaOptions',
>>    'allow-preconfig': true
>>  }
>> +
>> +##
>> +# @FirmwareConfigurationItem:
>> +#
>> +# Firmware Configuration (fw_cfg) item.
>> +#
>> +# @key: The uint16 selector key.
>> +# @keyname: The stringified name if the selector refers to a well-known
>> +#           numerically defined item.
>> +# @architecture_specific: Indicates whether the configuration setting is
> 
> Prefer '-' over '_' in new interfaces.

OK!

> 
>> +#                         architecture specific.
>> +#                  false: The item is a generic configuration item.
>> +#                  true:  The item is specific to a particular architecture.
>> +# @writeable: Indicates whether the configuration setting is writeable by
>> +#             the guest.
> 
> writable
> 
>> +# @size: The length of @data associated with the item.
>> +# @data: A string representating the firmware configuration data.
> 
> representing
> 
>> +#        Note: This field is currently not used.
> 
> Either drop the field until it has a use, or let it be the base64
> encoding and use it now.

Well, it is used by the HMP command, to pass the hexdump.
I'm rather fine using the base64 encoding.

>> +# @path: If the key is a 'file', the named file path.
>> +# @order: If the key is a 'file', the named file order.
>> +#
>> +# Since 4.0
>> +##
>> +{ 'struct': 'FirmwareConfigurationItem',
>> +  'data': { 'key': 'uint16',
>> +            '*keyname': 'str',
>> +            'architecture_specific': 'bool',
>> +            'writeable': 'bool',
>> +            '*data': 'str',
>> +            'size': 'int',
>> +            '*path': 'str',
>> +            '*order': 'int' } }
> 
> Is it worth making 'keyname' an enum type, and turning this into a flat
> union where 'path' and 'order' appear on the branch associated with
> 'file', and where all other well-known keynames have smaller branches?

I have no idea about that, I will have a look at QMP flat unions.

>> +
>> +
>> +##
>> +# @query-fw_cfg-items:
> 
> That looks weird to mix - and _. Any reason we can't just go with
> query-firmware-config?

Way better! I'll use query-firmware-config-items.

Thanks for the review,

Phil.

>> +#
>> +# Returns the list of Firmware Configuration items.
>> +#
>> +# Returns: A list of @FirmwareConfigurationItem for each entry.
>> +#
>> +# Since 4.0
>> +##
>> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
>>

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Eric Blake]

On 3/8/19 5:08 AM, Philippe Mathieu-Daudé wrote:

>>> {
>>>     "return": [
>>>         {
>>>             "architecture_specific": false,
>>>             "key": 0,
>>>             "writeable": false,
>>>             "size": 4,
>>>             "keyname": "signature"
>>
>> You could return a base64-encoded representation of the field (we do
>> that in other interfaces where raw binary can't be passed reliably over
>> JSON).  For 4 bytes, that makes sense,
>>
>>
>>>         {
>>>             "architecture_specific": true,
>>>             "key": 3,
>>>             "writeable": false,
>>>             "size": 324,
>>>             "keyname": "e820_tables"
>>
>> for 324 bytes, it gets a bit long. Still, may be worth it for the added
>> debug visibility.
> 
> Until you see values in the next patch...:
> 
> $ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
> [...]
>  0x0019   file_dir                           RO    2052       0000000b..
>  0x0022   file: etc/acpi/tables              RO  131072  130  46414353..

Yeah, that's a no-go.
> 
> What about using a 512B limit on this QMP answer?

Seems reasonable. Either omit the field when its size exceeds the limit,
or use the field to give a truncated version (where the size field still
shows the full length, either way).


>>> +# @path: If the key is a 'file', the named file path.
>>> +# @order: If the key is a 'file', the named file order.
>>> +#
>>> +# Since 4.0
>>> +##
>>> +{ 'struct': 'FirmwareConfigurationItem',
>>> +  'data': { 'key': 'uint16',
>>> +            '*keyname': 'str',
>>> +            'architecture_specific': 'bool',
>>> +            'writeable': 'bool',
>>> +            '*data': 'str',
>>> +            'size': 'int',
>>> +            '*path': 'str',
>>> +            '*order': 'int' } }
>>
>> Is it worth making 'keyname' an enum type, and turning this into a flat
>> union where 'path' and 'order' appear on the branch associated with
>> 'file', and where all other well-known keynames have smaller branches?
> 
> I have no idea about that, I will have a look at QMP flat unions.

Markus can help if you get stuck on what it might look like. But by now,
there are several examples in .qapi files to copy from (look for
'discriminator').

> 
>>> +
>>> +
>>> +##
>>> +# @query-fw_cfg-items:
>>
>> That looks weird to mix - and _. Any reason we can't just go with
>> query-firmware-config?
> 
> Way better! I'll use query-firmware-config-items.

Do we need the -items suffix? Also, is there a chance that we might ever
want to extend the command to return more information that is global to
firmware-config rather than per-item?

>>> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}

As written, this results in:

{ "return": [ { ... }, { ... } ] }

but if you add a layer of nesting, you could have easier extensions:

{ "return": { "global": {}, "items": [ { ... }, { ... } ] } }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

On 3/8/19 6:31 PM, Eric Blake wrote:
> On 3/8/19 5:08 AM, Philippe Mathieu-Daudé wrote:
> 
>>>> {
>>>>     "return": [
>>>>         {
>>>>             "architecture_specific": false,
>>>>             "key": 0,
>>>>             "writeable": false,
>>>>             "size": 4,
>>>>             "keyname": "signature"
>>>
>>> You could return a base64-encoded representation of the field (we do
>>> that in other interfaces where raw binary can't be passed reliably over
>>> JSON).  For 4 bytes, that makes sense,
>>>
>>>
>>>>         {
>>>>             "architecture_specific": true,
>>>>             "key": 3,
>>>>             "writeable": false,
>>>>             "size": 324,
>>>>             "keyname": "e820_tables"
>>>
>>> for 324 bytes, it gets a bit long. Still, may be worth it for the added
>>> debug visibility.
>>
>> Until you see values in the next patch...:
>>
>> $ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
>> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
>> [...]
>>  0x0019   file_dir                           RO    2052       0000000b..
>>  0x0022   file: etc/acpi/tables              RO  131072  130  46414353..
> 
> Yeah, that's a no-go.
>>
>> What about using a 512B limit on this QMP answer?
> 
> Seems reasonable. Either omit the field when its size exceeds the limit,
> or use the field to give a truncated version (where the size field still
> shows the full length, either way).

OK.

>>>> +# @path: If the key is a 'file', the named file path.
>>>> +# @order: If the key is a 'file', the named file order.
>>>> +#
>>>> +# Since 4.0
>>>> +##
>>>> +{ 'struct': 'FirmwareConfigurationItem',
>>>> +  'data': { 'key': 'uint16',
>>>> +            '*keyname': 'str',
>>>> +            'architecture_specific': 'bool',
>>>> +            'writeable': 'bool',
>>>> +            '*data': 'str',
>>>> +            'size': 'int',
>>>> +            '*path': 'str',
>>>> +            '*order': 'int' } }
>>>
>>> Is it worth making 'keyname' an enum type, and turning this into a flat
>>> union where 'path' and 'order' appear on the branch associated with
>>> 'file', and where all other well-known keynames have smaller branches?
>>
>> I have no idea about that, I will have a look at QMP flat unions.
> 
> Markus can help if you get stuck on what it might look like. But by now,
> there are several examples in .qapi files to copy from (look for
> 'discriminator').

OK.

>>
>>>> +
>>>> +
>>>> +##
>>>> +# @query-fw_cfg-items:
>>>
>>> That looks weird to mix - and _. Any reason we can't just go with
>>> query-firmware-config?
>>
>> Way better! I'll use query-firmware-config-items.
> 
> Do we need the -items suffix? Also, is there a chance that we might ever
> want to extend the command to return more information that is global to
> firmware-config rather than per-item?

Laszlo suggested it could be useful to ask for a specific item (with the
full data encoded?).

>>>> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
> 
> As written, this results in:
> 
> { "return": [ { ... }, { ... } ] }
> 
> but if you add a layer of nesting, you could have easier extensions:
> 
> { "return": { "global": {}, "items": [ { ... }, { ... } ] } }
> 

Oh I guess I got it, I'll try it.

Thanks!

Phil.

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Laszlo Ersek]

On 03/08/19 19:07, Philippe Mathieu-Daudé wrote:
> On 3/8/19 6:31 PM, Eric Blake wrote:
>> On 3/8/19 5:08 AM, Philippe Mathieu-Daudé wrote:
>>
>>>>> {
>>>>>     "return": [
>>>>>         {
>>>>>             "architecture_specific": false,
>>>>>             "key": 0,
>>>>>             "writeable": false,
>>>>>             "size": 4,
>>>>>             "keyname": "signature"
>>>>
>>>> You could return a base64-encoded representation of the field (we do
>>>> that in other interfaces where raw binary can't be passed reliably over
>>>> JSON).  For 4 bytes, that makes sense,
>>>>
>>>>
>>>>>         {
>>>>>             "architecture_specific": true,
>>>>>             "key": 3,
>>>>>             "writeable": false,
>>>>>             "size": 324,
>>>>>             "keyname": "e820_tables"
>>>>
>>>> for 324 bytes, it gets a bit long. Still, may be worth it for the added
>>>> debug visibility.
>>>
>>> Until you see values in the next patch...:
>>>
>>> $ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
>>> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
>>> [...]
>>>  0x0019   file_dir                           RO    2052       0000000b..
>>>  0x0022   file: etc/acpi/tables              RO  131072  130  46414353..
>>
>> Yeah, that's a no-go.
>>>
>>> What about using a 512B limit on this QMP answer?
>>
>> Seems reasonable. Either omit the field when its size exceeds the limit,
>> or use the field to give a truncated version (where the size field still
>> shows the full length, either way).
> 
> OK.
> 
>>>>> +# @path: If the key is a 'file', the named file path.
>>>>> +# @order: If the key is a 'file', the named file order.
>>>>> +#
>>>>> +# Since 4.0
>>>>> +##
>>>>> +{ 'struct': 'FirmwareConfigurationItem',
>>>>> +  'data': { 'key': 'uint16',
>>>>> +            '*keyname': 'str',
>>>>> +            'architecture_specific': 'bool',
>>>>> +            'writeable': 'bool',
>>>>> +            '*data': 'str',
>>>>> +            'size': 'int',
>>>>> +            '*path': 'str',
>>>>> +            '*order': 'int' } }
>>>>
>>>> Is it worth making 'keyname' an enum type, and turning this into a flat
>>>> union where 'path' and 'order' appear on the branch associated with
>>>> 'file', and where all other well-known keynames have smaller branches?
>>>
>>> I have no idea about that, I will have a look at QMP flat unions.
>>
>> Markus can help if you get stuck on what it might look like. But by now,
>> there are several examples in .qapi files to copy from (look for
>> 'discriminator').
> 
> OK.
> 
>>>
>>>>> +
>>>>> +
>>>>> +##
>>>>> +# @query-fw_cfg-items:
>>>>
>>>> That looks weird to mix - and _. Any reason we can't just go with
>>>> query-firmware-config?
>>>
>>> Way better! I'll use query-firmware-config-items.
>>
>> Do we need the -items suffix? Also, is there a chance that we might ever
>> want to extend the command to return more information that is global to
>> firmware-config rather than per-item?
> 
> Laszlo suggested it could be useful to ask for a specific item (with the
> full data encoded?).

It's possible I referred to that a long time ago, but most recently, I
think it has been raised by Dave.

Thanks
Laszlo

> 
>>>>> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
>>
>> As written, this results in:
>>
>> { "return": [ { ... }, { ... } ] }
>>
>> but if you add a layer of nesting, you could have easier extensions:
>>
>> { "return": { "global": {}, "items": [ { ... }, { ... } ] } }
>>
> 
> Oh I guess I got it, I'll try it.
> 
> Thanks!
> 
> Phil.
> 

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

On 3/8/19 9:00 PM, Laszlo Ersek wrote:
> On 03/08/19 19:07, Philippe Mathieu-Daudé wrote:
>> On 3/8/19 6:31 PM, Eric Blake wrote:
>>> On 3/8/19 5:08 AM, Philippe Mathieu-Daudé wrote:
>>>>>> +##
>>>>>> +# @query-fw_cfg-items:
>>>>>
>>>>> That looks weird to mix - and _. Any reason we can't just go with
>>>>> query-firmware-config?
>>>>
>>>> Way better! I'll use query-firmware-config-items.
>>>
>>> Do we need the -items suffix? Also, is there a chance that we might ever
>>> want to extend the command to return more information that is global to
>>> firmware-config rather than per-item?
>>
>> Laszlo suggested it could be useful to ask for a specific item (with the
>> full data encoded?).
> 
> It's possible I referred to that a long time ago, but most recently, I
> think it has been raised by Dave.

One of your 'random' ideas :)

https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01959.html

  (6) If we want bells and whistles, two optional parameters could be
  considered: one for identifying one specific key to request info about
  (identify by selector? well-known macro name? file pathname?), and
  another param for placing a limit, different from 8, on the individual
  hexdump size.

  Important: these are not "requirements", just random ideas, food for
  thought. I'm fine if you reject any subset of them, after
  consideration.

> 
> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH v2 13/18] hw/nvram/fw_cfg: Add QMP 'info fw_cfg' command

[Markus Armbruster]

The subject "Add QMP 'info fw_cfg' command" misspells query-fw_cfg-items
:)

Eric Blake <eblake@redhat.com> writes:

> On 3/7/19 7:32 PM, Philippe Mathieu-Daudé wrote:
>> When debugging a paravirtualized guest firmware, it results very
>> useful to dump the fw_cfg table.
>> Add a QMP command which returns the most useful fields.
>> Since the QMP protocol is not designed for passing stream data,
>> we don't display a fw_cfg item data, only it's size:
>> 
>> { "execute": "query-fw_cfg-items" }
>> {
>>     "return": [
>>         {
>>             "architecture_specific": false,
>>             "key": 0,
>>             "writeable": false,
>>             "size": 4,
>>             "keyname": "signature"
>
> You could return a base64-encoded representation of the field (we do
> that in other interfaces where raw binary can't be passed reliably over
> JSON).  For 4 bytes, that makes sense,
>
>
>>         {
>>             "architecture_specific": true,
>>             "key": 3,
>>             "writeable": false,
>>             "size": 324,
>>             "keyname": "e820_tables"
>
> for 324 bytes, it gets a bit long. Still, may be worth it for the added
> debug visibility.
>
>
>> +++ b/qapi/misc.json
>> @@ -3051,3 +3051,47 @@
>>    'data': 'NumaOptions',
>>    'allow-preconfig': true
>>  }
>> +
>> +##
>> +# @FirmwareConfigurationItem:
>> +#
>> +# Firmware Configuration (fw_cfg) item.
>> +#
>> +# @key: The uint16 selector key.
>> +# @keyname: The stringified name if the selector refers to a well-known
>> +#           numerically defined item.

Ignorant questions, I'm afraid...

What determines the possible values of @key?

What's the difference between a "well-known" and a "not well-known"
value?  Examples?

>> +# @architecture_specific: Indicates whether the configuration setting is
>
> Prefer '-' over '_' in new interfaces.
>
>> +#                         architecture specific.
>> +#                  false: The item is a generic configuration item.
>> +#                  true:  The item is specific to a particular architecture.
>> +# @writeable: Indicates whether the configuration setting is writeable by
>> +#             the guest.
>
> writable
>
>> +# @size: The length of @data associated with the item.
>> +# @data: A string representating the firmware configuration data.
>
> representing
>
>> +#        Note: This field is currently not used.
>
> Either drop the field until it has a use, or let it be the base64
> encoding and use it now.
>
>> +# @path: If the key is a 'file', the named file path.
>> +# @order: If the key is a 'file', the named file order.
>> +#
>> +# Since 4.0
>> +##
>> +{ 'struct': 'FirmwareConfigurationItem',
>> +  'data': { 'key': 'uint16',
>> +            '*keyname': 'str',
>> +            'architecture_specific': 'bool',
>> +            'writeable': 'bool',
>> +            '*data': 'str',
>> +            'size': 'int',
>> +            '*path': 'str',
>> +            '*order': 'int' } }
>
> Is it worth making 'keyname' an enum type, and turning this into a flat
> union where 'path' and 'order' appear on the branch associated with
> 'file', and where all other well-known keynames have smaller branches?

Discriminator can't be optional.  Obvious solution: add a suitabable
enum value for "other" key values.

Leads to something like this (untested):

    { 'union': 'FirmwareConfigurationItem',
      'base': { 'key': 'uint16',
                'keyname': 'FirmwareConfigurationKey',
                ... more members that make sense regardless of @key ... },
      'discriminator': 'key',
      'data': {
                'file': 'FirmwareConfigurationItemFile',
                ... more variants, if any ... } }

where 'FirmwareConfigurationItemFile' is a struct type containing the
members that make sense just for 'file'.

>> +
>> +
>> +##
>> +# @query-fw_cfg-items:
>
> That looks weird to mix - and _. Any reason we can't just go with
> query-firmware-config?
>
>> +#
>> +# Returns the list of Firmware Configuration items.
>> +#
>> +# Returns: A list of @FirmwareConfigurationItem for each entry.
>> +#
>> +# Since 4.0
>> +##
>> +{ 'command': 'query-fw_cfg-items', 'returns': ['FirmwareConfigurationItem']}
>> 

[Qemu-devel] [PATCH v2 14/18] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Philippe Mathieu-Daudé]

When debugging a paravirtualized guest firmware, it results very
useful to dump the fw_cfg table.
Add a HMP command which displays the most useful fields.
We display each fw_cfg item data in hexadecimal (only the first 8
bytes):

$ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
(qemu) info fw_cfg
Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
 0x0000   signature                          RO       4       51454d55
 0x0001   id                                 RO       4       03000000
 0x0002   uuid                               RO      16       0000000000000000..
 0x0003   ram_size                           RO       8       0000000800000000
 0x0004   nographic                          RO       2       0000
 0x0005   nb_cpus                            RO       2       0100
 0x000d   numa                               RO      16       0000000000000000..
 0x000e   boot_menu                          RO       2       0000
 0x000f   max_cpus                           RO       2       0100
 0x0019   file_dir                           RO    2052       0000000b00000000..
 0x0021   file: etc/acpi/rsdp                RO      20  160  5253442050545220..
 0x0022   file: etc/acpi/tables              RO  131072  130  4641435340000000..
 0x0023   file: etc/boot-fail-wait           RO       4   15  ffffffff
 0x0024   file: etc/e820                     RO      20   40  0000000000000000..
 0x0025   file: etc/smbios/smbios-anchor     RO      31   30  5f534d5f001f0208..
 0x0026   file: etc/smbios/smbios-tables     RO     321   20  011b000101020300..
 0x0027   file: etc/system-states            RO       6   90  800000818280
 0x0028   file: etc/table-loader             RO    4096  140  010000006574632f..
 0x002a   file: genroms/kvmvapic.bin         RO    9216   55  55aa12060e0731c0..
 0x0002   irq0_override                   *  RO       4       01000000
 0x0003   e820_tables                     *  RO     324       0000000000000000..
 0x0004   hpet                            *  RO     121       0101a286800000d0..
(qemu) q

$ (echo info fw_cfg; echo q) | qemu-system-mips -S -monitor stdio
(qemu) info fw_cfg
This machine does not use fw_cfg
(qemu) q

$ (echo info fw_cfg; echo q) | qemu-system-ppc -S -monitor stdio
(qemu) info fw_cfg
Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
 0x0000   signature                          RO       4       51454d55
 0x0001   id                                 RO       4       01000000
 0x0002   uuid                               RO      16       0000000000000000..
 0x0003   ram_size                           RO       8       0000000800000000
 0x0004   nographic                          RO       2       0000
 0x0005   nb_cpus                            RO       2       0100
 0x0006   machine_id                         RO       2       0200
 0x0007   kernel_addr                        RO       4       00000000
 0x0008   kernel_size                        RO       4       00000000
 0x0009   kernel_cmdline                     RO       4       00000000
 0x000a   initrd_addr                        RO       4       00000000
 0x000b   initdr_size                        RO       4       00000000
 0x000c   boot_device                        RO       2       6300
 0x000e   boot_menu                          RO       2       0000
 0x000f   max_cpus                           RO       2       0100
 0x0019   file_dir                           RO    2052       0000000200000000..
 0x0021   file: etc/boot-fail-wait           RO       4   15  ffffffff
 0x0000   width                           *  RO       2       2003
 0x0001   height                          *  RO       2       5802
 0x0002   depth                           *  RO       2       2000
 0x0003   tbfreq                          *  RO       4       c04bfd00
 0x0004   clockfreq                       *  RO       4       80d6da0f
 0x0005   is_kvm                          *  RO       4       00000000
 0x0009   busfreq                         *  RO       4       8014ef03
(qemu) q

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Check fw_cfg != NULL (Michael)
    Rename keys, display data in hexa (Laszlo)
---
 hmp-commands-info.hx      | 17 ++++++++++
 hw/nvram/fw_cfg.c         | 71 ++++++++++++++++++++++++++++++++++++++-
 include/hw/nvram/fw_cfg.h |  2 ++
 3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cbee8b944d..2c9538c8da 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -916,6 +916,23 @@ STEXI
 @item info sev
 @findex info sev
 Show SEV information.
+ETEXI
+
+    {
+        .name       = "fw_cfg",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Display the table firmware configuration entries "
+                      "registered by a paravirtualized machine. Helpful "
+                      "when debugging guest firmwares.",
+        .cmd        = hmp_info_fw_cfg,
+    },
+
+STEXI
+@item info fw_cfg
+@findex info fw_cfg
+Display the table firmware configuration entries registered by a paravirtualized
+machine. This information is useful when debugging guest firmwares.
 ETEXI
 
 STEXI
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 2a8d69ba07..4c82dcc125 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -35,6 +35,7 @@
 #include "qemu/config-file.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "monitor/monitor.h"
 #include "qapi/qapi-commands-misc.h"
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
@@ -1273,7 +1274,18 @@ static FirmwareConfigurationItem *create_qmp_fw_cfg_item(FWCfgState *s,
     return item;
 }
 
-FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
+/**
+ * query_fw_cfg_items:
+ *
+ * @use_hexdump: Whether to populate the @data field with the hexadecimal
+ *               representation of the item data.
+ * @errp: Pointer to a NULL initialized error object.
+ *
+ * Returns: A list of @FirmwareConfigurationItem, reverse sorted by the
+ *          item selector key.
+ */
+static FirmwareConfigurationItemList *query_fw_cfg_items(bool use_hexdump,
+                                                         Error **errp)
 {
     FirmwareConfigurationItemList *item_list = NULL;
     uint32_t max_entries;
@@ -1294,6 +1306,9 @@ FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
             if (!e->len) {
                 continue;
             }
+            if (use_hexdump) {
+                qmp_hex_length = MIN(e->len, 8);
+            }
 
             info = g_malloc0(sizeof(*info));
             info->value = create_qmp_fw_cfg_item(s, e, arch, key,
@@ -1305,3 +1320,57 @@ FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
 
     return item_list;
 }
+
+FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
+{
+    return query_fw_cfg_items(false, errp);
+}
+
+void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
+{
+    FirmwareConfigurationItemList *item_list, *method;
+    Error *err = NULL;
+
+    item_list = query_fw_cfg_items(true, &err);
+    if (!item_list) {
+        monitor_printf(mon, "This machine does not use fw_cfg\n");
+        return;
+    }
+    if (err) {
+        monitor_printf(mon, "Could not query fw_cfg entries: %s\n",
+                       error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+
+    monitor_printf(mon, "Selector  Well-Known Key  Pathname"
+                        " ArchSpec Perm   Size Order Hex Data\n");
+    for (method = item_list; method; method = method->next) {
+        if (method->value->has_path) {
+            monitor_printf(mon,
+                           " 0x%04x   file: %-28s %2s %7" PRId64
+                           "  %3" PRId64 "  %-16s%s\n",
+                           method->value->key,
+                           method->value->path,
+                           method->value->writeable ? "RW" : "RO",
+                           method->value->size,
+                           method->value->order,
+                           method->value->data,
+                           method->value->size > 8 ? ".." : "");
+        } else {
+            monitor_printf(mon,
+                           " 0x%04x   %-30s  %c  %2s %7" PRId64
+                           "       %-16s%s\n",
+                           method->value->key,
+                           method->value->has_keyname
+                                ? method->value->keyname : "",
+                           method->value->architecture_specific ? '*' : ' ',
+                           method->value->writeable ? "RW" : "RO",
+                           method->value->size,
+                           method->value->data,
+                           method->value->size > 8 ? ".." : "");
+       }
+    }
+
+    qapi_free_FirmwareConfigurationItemList(item_list);
+}
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 83a0540b6c..5ac9adfe1f 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -243,4 +243,6 @@ bool fw_cfg_dma_enabled(void *opaque);
  */
 const char *fw_cfg_arch_key_name(uint16_t key);
 
+void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
+
 #endif
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 14/18] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> When debugging a paravirtualized guest firmware, it results very
> useful to dump the fw_cfg table.
> Add a HMP command which displays the most useful fields.
> We display each fw_cfg item data in hexadecimal (only the first 8
> bytes):
> 
> $ (echo info fw_cfg; echo q) | qemu-system-x86_64 -S -monitor stdio
> (qemu) info fw_cfg
> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
>  0x0000   signature                          RO       4       51454d55
>  0x0001   id                                 RO       4       03000000
>  0x0002   uuid                               RO      16       0000000000000000..
>  0x0003   ram_size                           RO       8       0000000800000000
>  0x0004   nographic                          RO       2       0000
>  0x0005   nb_cpus                            RO       2       0100
>  0x000d   numa                               RO      16       0000000000000000..
>  0x000e   boot_menu                          RO       2       0000
>  0x000f   max_cpus                           RO       2       0100
>  0x0019   file_dir                           RO    2052       0000000b00000000..
>  0x0021   file: etc/acpi/rsdp                RO      20  160  5253442050545220..
>  0x0022   file: etc/acpi/tables              RO  131072  130  4641435340000000..
>  0x0023   file: etc/boot-fail-wait           RO       4   15  ffffffff
>  0x0024   file: etc/e820                     RO      20   40  0000000000000000..
>  0x0025   file: etc/smbios/smbios-anchor     RO      31   30  5f534d5f001f0208..
>  0x0026   file: etc/smbios/smbios-tables     RO     321   20  011b000101020300..
>  0x0027   file: etc/system-states            RO       6   90  800000818280
>  0x0028   file: etc/table-loader             RO    4096  140  010000006574632f..
>  0x002a   file: genroms/kvmvapic.bin         RO    9216   55  55aa12060e0731c0..
>  0x0002   irq0_override                   *  RO       4       01000000
>  0x0003   e820_tables                     *  RO     324       0000000000000000..
>  0x0004   hpet                            *  RO     121       0101a286800000d0..
> (qemu) q
> 
> $ (echo info fw_cfg; echo q) | qemu-system-mips -S -monitor stdio
> (qemu) info fw_cfg
> This machine does not use fw_cfg
> (qemu) q
> 
> $ (echo info fw_cfg; echo q) | qemu-system-ppc -S -monitor stdio
> (qemu) info fw_cfg
> Selector  Well-Known Key  Pathname ArchSpec Perm   Size Order Hex Data
>  0x0000   signature                          RO       4       51454d55
>  0x0001   id                                 RO       4       01000000
>  0x0002   uuid                               RO      16       0000000000000000..
>  0x0003   ram_size                           RO       8       0000000800000000
>  0x0004   nographic                          RO       2       0000
>  0x0005   nb_cpus                            RO       2       0100
>  0x0006   machine_id                         RO       2       0200
>  0x0007   kernel_addr                        RO       4       00000000
>  0x0008   kernel_size                        RO       4       00000000
>  0x0009   kernel_cmdline                     RO       4       00000000
>  0x000a   initrd_addr                        RO       4       00000000
>  0x000b   initdr_size                        RO       4       00000000
>  0x000c   boot_device                        RO       2       6300
>  0x000e   boot_menu                          RO       2       0000
>  0x000f   max_cpus                           RO       2       0100
>  0x0019   file_dir                           RO    2052       0000000200000000..
>  0x0021   file: etc/boot-fail-wait           RO       4   15  ffffffff
>  0x0000   width                           *  RO       2       2003
>  0x0001   height                          *  RO       2       5802
>  0x0002   depth                           *  RO       2       2000
>  0x0003   tbfreq                          *  RO       4       c04bfd00
>  0x0004   clockfreq                       *  RO       4       80d6da0f
>  0x0005   is_kvm                          *  RO       4       00000000
>  0x0009   busfreq                         *  RO       4       8014ef03
> (qemu) q
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I think this is fine from an HMP view; one thought of something that
might be useful is an optional parameter to specify a particular key
that dumps the whole object; then if you're particularly desperate to
look at an acpi table you could do something like:

info fw_cfg "file: etc/acpi/tables"

and get a fun block of hex back.

But that's just a suggestion, so:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> v2: Check fw_cfg != NULL (Michael)
>     Rename keys, display data in hexa (Laszlo)
> ---
>  hmp-commands-info.hx      | 17 ++++++++++
>  hw/nvram/fw_cfg.c         | 71 ++++++++++++++++++++++++++++++++++++++-
>  include/hw/nvram/fw_cfg.h |  2 ++
>  3 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..2c9538c8da 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -916,6 +916,23 @@ STEXI
>  @item info sev
>  @findex info sev
>  Show SEV information.
> +ETEXI
> +
> +    {
> +        .name       = "fw_cfg",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "Display the table firmware configuration entries "
> +                      "registered by a paravirtualized machine. Helpful "
> +                      "when debugging guest firmwares.",
> +        .cmd        = hmp_info_fw_cfg,
> +    },
> +
> +STEXI
> +@item info fw_cfg
> +@findex info fw_cfg
> +Display the table firmware configuration entries registered by a paravirtualized
> +machine. This information is useful when debugging guest firmwares.
>  ETEXI
>  
>  STEXI
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 2a8d69ba07..4c82dcc125 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -35,6 +35,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "monitor/monitor.h"
>  #include "qapi/qapi-commands-misc.h"
>  
>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
> @@ -1273,7 +1274,18 @@ static FirmwareConfigurationItem *create_qmp_fw_cfg_item(FWCfgState *s,
>      return item;
>  }
>  
> -FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
> +/**
> + * query_fw_cfg_items:
> + *
> + * @use_hexdump: Whether to populate the @data field with the hexadecimal
> + *               representation of the item data.
> + * @errp: Pointer to a NULL initialized error object.
> + *
> + * Returns: A list of @FirmwareConfigurationItem, reverse sorted by the
> + *          item selector key.
> + */
> +static FirmwareConfigurationItemList *query_fw_cfg_items(bool use_hexdump,
> +                                                         Error **errp)
>  {
>      FirmwareConfigurationItemList *item_list = NULL;
>      uint32_t max_entries;
> @@ -1294,6 +1306,9 @@ FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
>              if (!e->len) {
>                  continue;
>              }
> +            if (use_hexdump) {
> +                qmp_hex_length = MIN(e->len, 8);
> +            }
>  
>              info = g_malloc0(sizeof(*info));
>              info->value = create_qmp_fw_cfg_item(s, e, arch, key,
> @@ -1305,3 +1320,57 @@ FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
>  
>      return item_list;
>  }
> +
> +FirmwareConfigurationItemList *qmp_query_fw_cfg_items(Error **errp)
> +{
> +    return query_fw_cfg_items(false, errp);
> +}
> +
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict)
> +{
> +    FirmwareConfigurationItemList *item_list, *method;
> +    Error *err = NULL;
> +
> +    item_list = query_fw_cfg_items(true, &err);
> +    if (!item_list) {
> +        monitor_printf(mon, "This machine does not use fw_cfg\n");
> +        return;
> +    }
> +    if (err) {
> +        monitor_printf(mon, "Could not query fw_cfg entries: %s\n",
> +                       error_get_pretty(err));
> +        error_free(err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Selector  Well-Known Key  Pathname"
> +                        " ArchSpec Perm   Size Order Hex Data\n");
> +    for (method = item_list; method; method = method->next) {
> +        if (method->value->has_path) {
> +            monitor_printf(mon,
> +                           " 0x%04x   file: %-28s %2s %7" PRId64
> +                           "  %3" PRId64 "  %-16s%s\n",
> +                           method->value->key,
> +                           method->value->path,
> +                           method->value->writeable ? "RW" : "RO",
> +                           method->value->size,
> +                           method->value->order,
> +                           method->value->data,
> +                           method->value->size > 8 ? ".." : "");
> +        } else {
> +            monitor_printf(mon,
> +                           " 0x%04x   %-30s  %c  %2s %7" PRId64
> +                           "       %-16s%s\n",
> +                           method->value->key,
> +                           method->value->has_keyname
> +                                ? method->value->keyname : "",
> +                           method->value->architecture_specific ? '*' : ' ',
> +                           method->value->writeable ? "RW" : "RO",
> +                           method->value->size,
> +                           method->value->data,
> +                           method->value->size > 8 ? ".." : "");
> +       }
> +    }
> +
> +    qapi_free_FirmwareConfigurationItemList(item_list);
> +}
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 83a0540b6c..5ac9adfe1f 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -243,4 +243,6 @@ bool fw_cfg_dma_enabled(void *opaque);
>   */
>  const char *fw_cfg_arch_key_name(uint16_t key);
>  
> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict);
> +
>  #endif
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 15/18] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Philippe Mathieu-Daudé]

Add a function to read the full content of file on the host, and add
a new 'file' name item to the fw_cfg device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: s/ptr/data, corrected documentation (Laszlo)
---
 hw/nvram/fw_cfg.c         | 21 +++++++++++++++++++++
 include/hw/nvram/fw_cfg.h | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 4c82dcc125..a46a7c8f06 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -890,6 +890,27 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
     fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
 }
 
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len)
+{
+    GError *gerr = NULL;
+    gchar *data = NULL;
+    gsize contents_len = 0;
+
+    if (g_file_get_contents(host_path, &data, &contents_len, &gerr)) {
+        fw_cfg_add_file(s, filename, data, contents_len);
+    } else {
+        error_report("%s", gerr->message);
+        g_error_free(gerr);
+        return NULL;
+    }
+    if (len) {
+        *len = contents_len;
+    }
+
+    return data;
+}
+
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
                         void *data, size_t len)
 {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 5ac9adfe1f..75a29858dc 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -172,6 +172,29 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 
+/**
+ * fw_cfg_add_file_from_host:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @host_path: path of the host file to read the data from
+ * @len: pointer to hold the length of the host file (optional)
+ *
+ * Read the content of a host file as a raw "blob" then add a new NAMED
+ * fw_cfg item of the file size. If @len is provided, it will contain the
+ * total length read from the host file. The data read from the host
+ * filesystem is owned by the new fw_cfg entry, and is stored into the data
+ * structure of the fw_cfg device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ *
+ * Returns: pointer to the newly allocated file content, or NULL if an error
+ * occured. The returned pointer must be freed with g_free().
+ */
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len);
+
 /**
  * fw_cfg_add_file_callback:
  * @s: fw_cfg device being modified
-- 
2.20.1

[Qemu-devel] [PATCH v2 16/18] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

The Edk2Crypto object is used to hold configuration values specific
to EDK2.

The edk2_add_host_crypto_policy() function loads crypto policies
from the host, and register them as fw_cfg named file items.
So far only the 'https' policy is supported.

An usercase example is the 'HTTPS Boof' feature of OVMF [*].

Usage example:

  $ qemu-system-x86_64 \
      -object edk2_crypto,id=https,\
              ciphers=/etc/crypto-policies/back-ends/openssl.config,\
              cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin

(On Fedora these files are provided by the ca-certificates and
crypto-policies packages).

[*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS                             |   8 ++
 hw/Makefile.objs                        |   1 +
 hw/firmware/Makefile.objs               |   1 +
 hw/firmware/uefi_edk2_crypto_policies.c | 166 ++++++++++++++++++++++++
 include/hw/firmware/uefi_edk2.h         |  28 ++++
 5 files changed, 204 insertions(+)
 create mode 100644 hw/firmware/Makefile.objs
 create mode 100644 hw/firmware/uefi_edk2_crypto_policies.c
 create mode 100644 include/hw/firmware/uefi_edk2.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 306fc2aefa..3696b63249 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2205,6 +2205,14 @@ F: include/hw/i2c/smbus_master.h
 F: include/hw/i2c/smbus_slave.h
 F: include/hw/i2c/smbus_eeprom.h
 
+EDK2 Firmware
+M: Laszlo Ersek <lersek@redhat.com>
+M: Philippe Mathieu-Daudé <philmd@redhat.com>
+S: Maintained
+F: docs/interop/firmware.json
+F: hw/firmware/uefi_edk2_crypto_policies.c
+F: include/hw/firmware/uefi_edk2.h
+
 Usermode Emulation
 ------------------
 Overall
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index e2fcd6aafc..da4fb91285 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -8,6 +8,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += char/
 devices-dirs-$(CONFIG_SOFTMMU) += cpu/
 devices-dirs-$(CONFIG_SOFTMMU) += display/
 devices-dirs-$(CONFIG_SOFTMMU) += dma/
+devices-dirs-$(CONFIG_SOFTMMU) += firmware/
 devices-dirs-$(CONFIG_SOFTMMU) += gpio/
 devices-dirs-$(CONFIG_HYPERV) += hyperv/
 devices-dirs-$(CONFIG_SOFTMMU) += i2c/
diff --git a/hw/firmware/Makefile.objs b/hw/firmware/Makefile.objs
new file mode 100644
index 0000000000..ea1f6d44df
--- /dev/null
+++ b/hw/firmware/Makefile.objs
@@ -0,0 +1 @@
+common-obj-y += uefi_edk2_crypto_policies.o
diff --git a/hw/firmware/uefi_edk2_crypto_policies.c b/hw/firmware/uefi_edk2_crypto_policies.c
new file mode 100644
index 0000000000..660c7f3655
--- /dev/null
+++ b/hw/firmware/uefi_edk2_crypto_policies.c
@@ -0,0 +1,166 @@
+/*
+ * UEFI EDK2 Support
+ *
+ * Copyright (c) 2019 Red Hat Inc.
+ *
+ * Author:
+ *  Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "hw/firmware/uefi_edk2.h"
+
+
+#define TYPE_EDK2_CRYPTO "edk2_crypto"
+
+#define EDK2_CRYPTO_CLASS(klass) \
+     OBJECT_CLASS_CHECK(Edk2CryptoClass, (klass), \
+                        TYPE_EDK2_CRYPTO)
+#define EDK2_CRYPTO_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(Edk2CryptoClass, (obj), \
+                      TYPE_EDK2_CRYPTO)
+#define EDK2_CRYPTO(obj) \
+     INTERFACE_CHECK(Edk2Crypto, (obj), \
+                     TYPE_EDK2_CRYPTO)
+
+typedef struct Edk2Crypto {
+    Object parent_obj;
+
+    /*
+     * Path to the acceptable ciphersuites and the preferred order from
+     * the host-side crypto policy.
+     */
+    char *ciphers_path;
+
+    /* Path to the trusted CA certificates configured on the host side. */
+    char *cacerts_path;
+} Edk2Crypto;
+
+typedef struct Edk2CryptoClass {
+    ObjectClass parent_class;
+} Edk2CryptoClass;
+
+
+static void edk2_crypto_prop_set_ciphers(Object *obj, const char *value,
+                                         Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->ciphers_path);
+    s->ciphers_path = g_strdup(value);
+}
+
+static char *edk2_crypto_prop_get_ciphers(Object *obj,
+                                          Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    return g_strdup(s->ciphers_path);
+}
+
+static void edk2_crypto_prop_set_cacerts(Object *obj, const char *value,
+                                         Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->cacerts_path);
+    s->cacerts_path = g_strdup(value);
+}
+
+static char *edk2_crypto_prop_get_cacerts(Object *obj,
+                                          Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    return g_strdup(s->cacerts_path);
+}
+
+static void edk2_crypto_finalize(Object *obj)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->ciphers_path);
+    g_free(s->cacerts_path);
+}
+
+static void edk2_crypto_class_init(ObjectClass *oc, void *data)
+{
+    object_class_property_add_str(oc, "ciphers",
+                                  edk2_crypto_prop_get_ciphers,
+                                  edk2_crypto_prop_set_ciphers,
+                                  NULL);
+    object_class_property_add_str(oc, "cacerts",
+                                  edk2_crypto_prop_get_cacerts,
+                                  edk2_crypto_prop_set_cacerts,
+                                  NULL);
+}
+
+static const TypeInfo edk2_crypto_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_EDK2_CRYPTO,
+    .instance_size = sizeof(Edk2Crypto),
+    .instance_finalize = edk2_crypto_finalize,
+    .class_size = sizeof(Edk2CryptoClass),
+    .class_init = edk2_crypto_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void edk2_crypto_register_types(void)
+{
+    type_register_static(&edk2_crypto_info);
+}
+
+type_init(edk2_crypto_register_types);
+
+static Edk2Crypto *edk2_crypto_by_id(const char *edk_crypto_id, Error **errp)
+{
+    Object *obj;
+    Object *container;
+
+    container = object_get_objects_root();
+    obj = object_resolve_path_component(container,
+                                        edk_crypto_id);
+    if (!obj) {
+        error_setg(errp, "Cannot find EDK2 crypto object ID %s",
+                   edk_crypto_id);
+        return NULL;
+    }
+
+    if (!object_dynamic_cast(obj, TYPE_EDK2_CRYPTO)) {
+        error_setg(errp, "Object '%s' is not a EDK2 crypto subclass",
+                   edk_crypto_id);
+        return NULL;
+    }
+
+    return EDK2_CRYPTO(obj);
+}
+
+void edk2_add_host_crypto_policy(FWCfgState *fw_cfg)
+{
+    Edk2Crypto *s;
+
+    s = edk2_crypto_by_id("https", NULL);
+    if (!s) {
+        return;
+    }
+
+    if (s->ciphers_path) {
+        /* TODO g_free the returned pointer */
+        fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/ciphers",
+                                  s->ciphers_path, NULL);
+    }
+
+    if (s->cacerts_path) {
+        /* TODO g_free the returned pointer */
+        fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts",
+                                  s->cacerts_path, NULL);
+    }
+}
diff --git a/include/hw/firmware/uefi_edk2.h b/include/hw/firmware/uefi_edk2.h
new file mode 100644
index 0000000000..e0b2fb160a
--- /dev/null
+++ b/include/hw/firmware/uefi_edk2.h
@@ -0,0 +1,28 @@
+/*
+ * UEFI EDK2 Support
+ *
+ * Copyright (c) 2019 Red Hat Inc.
+ *
+ * Author:
+ *  Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_FIRMWARE_UEFI_EDK2_H
+#define HW_FIRMWARE_UEFI_EDK2_H
+
+#include "hw/nvram/fw_cfg.h"
+
+/**
+ * edk2_add_host_crypto_policy:
+ * @s: fw_cfg device being modified
+ *
+ * Add a new named file containing the host crypto policy.
+ *
+ * Currently only the 'https' policy is supported.
+ */
+void edk2_add_host_crypto_policy(FWCfgState *s);
+
+#endif /* HW_FIRMWARE_UEFI_EDK2_H */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 16/18] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Eric Blake]

On 3/7/19 7:32 PM, Philippe Mathieu-Daudé wrote:
> The Edk2Crypto object is used to hold configuration values specific
> to EDK2.
> 
> The edk2_add_host_crypto_policy() function loads crypto policies
> from the host, and register them as fw_cfg named file items.
> So far only the 'https' policy is supported.
> 
> An usercase example is the 'HTTPS Boof' feature of OVMF [*].

s/An/A/ since "user" is a pronounced or hard 'u' (English is funny, but
the rule of thumb is you add the consonant only before a soft u, and not
a pronounced one; as in "give an umbrella to a unicorn")

> 
> Usage example:
> 
>   $ qemu-system-x86_64 \
>       -object edk2_crypto,id=https,\

Might as well use --object (both spellings work for qemu, but since
--object is the only spelling for qemu-img/qemu-nbd, being consistent
between the lot is useful).

>               ciphers=/etc/crypto-policies/back-ends/openssl.config,\
>               cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin

(I really should follow through on my threat to teach QemuOpts to ignore
whitespace after ','; but for this commit message, it's obvious the
indentation has to be stripped for the command line to be valid)

> 
> (On Fedora these files are provided by the ca-certificates and
> crypto-policies packages).
> 
> [*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 16/18] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Hi Eric,

On 3/8/19 3:16 AM, Eric Blake wrote:
> On 3/7/19 7:32 PM, Philippe Mathieu-Daudé wrote:
>> The Edk2Crypto object is used to hold configuration values specific
>> to EDK2.
>>
>> The edk2_add_host_crypto_policy() function loads crypto policies
>> from the host, and register them as fw_cfg named file items.
>> So far only the 'https' policy is supported.
>>
>> An usercase example is the 'HTTPS Boof' feature of OVMF [*].
> 
> s/An/A/ since "user" is a pronounced or hard 'u' (English is funny, but
> the rule of thumb is you add the consonant only before a soft u, and not
> a pronounced one; as in "give an umbrella to a unicorn")

I appreciate the correction, thanks :)

>>
>> Usage example:
>>
>>   $ qemu-system-x86_64 \
>>       -object edk2_crypto,id=https,\
> 
> Might as well use --object (both spellings work for qemu, but since
> --object is the only spelling for qemu-img/qemu-nbd, being consistent
> between the lot is useful).

$ git grep -- ' -object ' | wc -l
83

^ cover various subsystems:

$ git grep -l -- ' -object '
docs/amd-memory-encryption.txt
docs/can.txt
docs/memory-hotplug.txt
docs/nvdimm.txt
docs/pr-manager.rst
docs/pvrdma.txt
docs/replay.txt
hw/virtio/vhost-user.c
include/authz/listfile.h
include/authz/pamacct.h
include/authz/simple.h
include/crypto/secret.h
include/crypto/tlscredsanon.h
include/crypto/tlscredsx509.h
qapi/misc.json
qemu-doc.texi
qemu-options.hx
target/i386/sev_i386.h
tests/bios-tables-test.c
tests/qemu-iotests/127
tests/qemu-iotests/200
tests/vhost-user-test.c


$ git grep -- ' --object ' | wc -l
252

^ mostly for the block subsystem:

$ git grep -l -- ' --object '
block/vxhs.c
include/crypto/tlscredspsk.h
qemu-doc.texi
qemu-img.texi
qemu-io.c
qemu-nbd.c
qemu-nbd.texi
tests/qemu-iotests/049
tests/qemu-iotests/049.out
tests/qemu-iotests/087
tests/qemu-iotests/134
tests/qemu-iotests/149.out
tests/qemu-iotests/158
tests/qemu-iotests/178
tests/qemu-iotests/188
tests/qemu-iotests/189
tests/qemu-iotests/198
tests/qemu-iotests/233

I'll change, but I'm not sure what is the default we should enforce...

> 
>>               ciphers=/etc/crypto-policies/back-ends/openssl.config,\
>>               cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin
> 
> (I really should follow through on my threat to teach QemuOpts to ignore
> whitespace after ','; but for this commit message, it's obvious the
> indentation has to be stripped for the command line to be valid)
> 
>>
>> (On Fedora these files are provided by the ca-certificates and
>> crypto-policies packages).
>>
>> [*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---

[Qemu-devel] [PATCH v2 17/18] hw/i386: Use edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Enable the EDK2 Crypto Policy features on the PC machine.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0848cdc18f..736211d623 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -38,6 +38,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
 #include "hw/firmware/smbios.h"
+#include "hw/firmware/uefi_edk2.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "multiboot.h"
@@ -1067,6 +1068,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
     return fw_cfg;
 }
 
+static void pc_uefi_setup(PCMachineState *pcms)
+{
+    edk2_add_host_crypto_policy(pcms->fw_cfg);
+}
+
 static long get_file_size(FILE *f)
 {
     long where, size;
@@ -1666,6 +1672,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     if (pcms->fw_cfg) {
         pc_build_smbios(pcms);
         pc_build_feature_control_file(pcms);
+        pc_uefi_setup(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }
-- 
2.20.1

[Qemu-devel] [PATCH v2 18/18] hw/arm/virt: Use edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Enable the EDK2 Crypto Policy features on the Virt machine.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index bb7255a080..bc2b14af48 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -56,6 +56,7 @@
 #include "hw/intc/arm_gicv3_common.h"
 #include "kvm_arm.h"
 #include "hw/firmware/smbios.h"
+#include "hw/firmware/uefi_edk2.h"
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
@@ -1301,6 +1302,11 @@ static void virt_build_smbios(VirtMachineState *vms)
     }
 }
 
+static void virt_uefi_setup(VirtMachineState *vms)
+{
+    edk2_add_host_crypto_policy(vms->fw_cfg);
+}
+
 static
 void virt_machine_done(Notifier *notifier, void *data)
 {
@@ -1329,6 +1335,7 @@ void virt_machine_done(Notifier *notifier, void *data)
 
     virt_acpi_setup(vms);
     virt_build_smbios(vms);
+    virt_uefi_setup(vms);
 }
 
 static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 00/18] fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy

[Laszlo Ersek]

On 03/08/19 02:32, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series consists of:
> - trivial cleanups, add trace events (was in v1)
> - add QMP/HMP commands to display the list of fw_cfg entries (reworked from v1)
> - add unrealize() method and deallocate various buffers (new in v2)
> - add fw_cfg_add_file_from_host (was in v1)
> - add edk2_add_host_crypto_policy (new in v2)
> 
> Since v1:
> - Addressed Michael and Laszlo comments.
> 
> Please review,
> 
> Phil.
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html

I'm completely exhausted after reviewing 13 QEMU patches this morning,
so I'll stop after "PATCH v2 12/18" in this series.

Please split the patch set into multiple, better focused waves (the
summary at the top is an indication of what each wave should concern
itself with).

Thanks,
Laszlo