[Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing

[Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing

[Philippe Mathieu-Daudé]

Trivial series to improve fw_cfg tracing.

Regards,

Phil.

Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
- Split arch-specific code (1 patch per arch) (requested by Laszlo)

Since v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html
- Added arch-specific keys

Philippe Mathieu-Daudé (7):
  hw/nvram/fw_cfg: Add trace events
  hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()
  hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
  hw/i386: Implement fw_cfg_arch_key_name()
  hw/ppc: Implement fw_cfg_arch_key_name()
  hw/sparc: Implement fw_cfg_arch_key_name()
  hw/sparc64: Implement fw_cfg_arch_key_name()

 MAINTAINERS               |  1 +
 hw/i386/Makefile.objs     |  2 +-
 hw/i386/fw_cfg.c          | 38 +++++++++++++++++++++++
 hw/i386/fw_cfg.h          | 20 +++++++++++++
 hw/i386/pc.c              |  7 +----
 hw/nvram/fw_cfg.c         | 63 ++++++++++++++++++++++++++++++++++++++-
 hw/nvram/trace-events     |  7 ++++-
 hw/ppc/Makefile.objs      |  2 +-
 hw/ppc/fw_cfg.c           | 45 ++++++++++++++++++++++++++++
 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            | 21 +++++++++++++
 14 files changed, 246 insertions(+), 10 deletions(-)
 create mode 100644 hw/i386/fw_cfg.c
 create mode 100644 hw/i386/fw_cfg.h
 create mode 100644 hw/ppc/fw_cfg.c
 create mode 100644 stubs/fw_cfg.c

-- 
2.20.1

[Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing

[Philippe Mathieu-Daudé]

Trivial series to improve fw_cfg tracing.

Regards,

Phil.

Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
- Split arch-specific code (1 patch per arch) (requested by Laszlo)

Since v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html
- Added arch-specific keys

Philippe Mathieu-Daudé (7):
  hw/nvram/fw_cfg: Add trace events
  hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()
  hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
  hw/i386: Implement fw_cfg_arch_key_name()
  hw/ppc: Implement fw_cfg_arch_key_name()
  hw/sparc: Implement fw_cfg_arch_key_name()
  hw/sparc64: Implement fw_cfg_arch_key_name()

 MAINTAINERS               |  1 +
 hw/i386/Makefile.objs     |  2 +-
 hw/i386/fw_cfg.c          | 38 +++++++++++++++++++++++
 hw/i386/fw_cfg.h          | 20 +++++++++++++
 hw/i386/pc.c              |  7 +----
 hw/nvram/fw_cfg.c         | 63 ++++++++++++++++++++++++++++++++++++++-
 hw/nvram/trace-events     |  7 ++++-
 hw/ppc/Makefile.objs      |  2 +-
 hw/ppc/fw_cfg.c           | 45 ++++++++++++++++++++++++++++
 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            | 21 +++++++++++++
 14 files changed, 246 insertions(+), 10 deletions(-)
 create mode 100644 hw/i386/fw_cfg.c
 create mode 100644 hw/i386/fw_cfg.h
 create mode 100644 hw/ppc/fw_cfg.c
 create mode 100644 stubs/fw_cfg.c

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/7] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

Add trace events to dump the key content.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v1 -> v3:
- moved static fw_cfg_wellknown_keys[] in key_name()
- split trace_key_name() (can return "unknown")
  from key_name() (can return NULL)

Since changes from v1 are trivial, keep S-o-b tags.
---
 hw/nvram/fw_cfg.c     | 63 ++++++++++++++++++++++++++++++++++++++++++-
 hw/nvram/trace-events |  7 ++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5c3a46ce6f2..d374a970fea 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 NULL;
+    }
+    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
 
@@ -233,7 +289,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;
 }
 
@@ -616,6 +672,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);
 }
 
@@ -623,6 +680,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);
 }
 
@@ -632,6 +690,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));
 }
 
@@ -651,6 +710,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));
 }
 
@@ -660,6 +720,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 e191991e2a8..0dea9260ce1 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"
 
 # 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
-- 
2.20.1

[Qemu-devel] [PATCH v3 1/7] hw/nvram/fw_cfg: Add trace events

[Philippe Mathieu-Daudé]

Add trace events to dump the key content.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v1 -> v3:
- moved static fw_cfg_wellknown_keys[] in key_name()
- split trace_key_name() (can return "unknown")
  from key_name() (can return NULL)

Since changes from v1 are trivial, keep S-o-b tags.
---
 hw/nvram/fw_cfg.c     | 63 ++++++++++++++++++++++++++++++++++++++++++-
 hw/nvram/trace-events |  7 ++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 5c3a46ce6f2..d374a970fea 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 NULL;
+    }
+    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
 
@@ -233,7 +289,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;
 }
 
@@ -616,6 +672,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);
 }
 
@@ -623,6 +680,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);
 }
 
@@ -632,6 +690,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));
 }
 
@@ -651,6 +710,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));
 }
 
@@ -660,6 +720,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 e191991e2a8..0dea9260ce1 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"
 
 # 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
-- 
2.20.1

[Qemu-devel] [PATCH v3 2/7] hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Add fw_cfg_arch_key_name() which returns the name of
an architecture-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS               |  1 +
 hw/nvram/fw_cfg.c         |  2 +-
 include/hw/nvram/fw_cfg.h | 11 +++++++++++
 stubs/Makefile.objs       |  1 +
 stubs/fw_cfg.c            | 21 +++++++++++++++++++++
 5 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 stubs/fw_cfg.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 56139ac8ab0..444783bb652 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1675,6 +1675,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/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d374a970fea..b2dc0a80cbc 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -100,7 +100,7 @@ static const char *key_name(uint16_t key)
     };
 
     if (key & FW_CFG_ARCH_LOCAL) {
-        return NULL;
+        return fw_cfg_arch_key_name(key);
     }
     if (key < FW_CFG_FILE_FIRST) {
         return fw_cfg_wellknown_keys[key];
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a740..828ad9dedc6 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 269dfa58326..73452ad2657 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 00000000000..bb1e3c8aa95
--- /dev/null
+++ b/stubs/fw_cfg.c
@@ -0,0 +1,21 @@
+/*
+ * fw_cfg stubs
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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

[Qemu-devel] [PATCH v3 2/7] hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Add fw_cfg_arch_key_name() which returns the name of
an architecture-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS               |  1 +
 hw/nvram/fw_cfg.c         |  2 +-
 include/hw/nvram/fw_cfg.h | 11 +++++++++++
 stubs/Makefile.objs       |  1 +
 stubs/fw_cfg.c            | 21 +++++++++++++++++++++
 5 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 stubs/fw_cfg.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 56139ac8ab0..444783bb652 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1675,6 +1675,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/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d374a970fea..b2dc0a80cbc 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -100,7 +100,7 @@ static const char *key_name(uint16_t key)
     };
 
     if (key & FW_CFG_ARCH_LOCAL) {
-        return NULL;
+        return fw_cfg_arch_key_name(key);
     }
     if (key < FW_CFG_FILE_FIRST) {
         return fw_cfg_wellknown_keys[key];
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a740..828ad9dedc6 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 269dfa58326..73452ad2657 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 00000000000..bb1e3c8aa95
--- /dev/null
+++ b/stubs/fw_cfg.c
@@ -0,0 +1,21 @@
+/*
+ * fw_cfg stubs
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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

[Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Philippe Mathieu-Daudé]

Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
 hw/i386/pc.c     |  7 +------
 2 files changed, 21 insertions(+), 6 deletions(-)
 create mode 100644 hw/i386/fw_cfg.h

diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
new file mode 100644
index 00000000000..17a4bc32f22
--- /dev/null
+++ b/hw/i386/fw_cfg.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU fw_cfg helpers (X86 specific)
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#ifndef HW_I386_FW_CFG_H
+#define HW_I386_FW_CFG_H
+
+#include "hw/nvram/fw_cfg.h"
+
+#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
+#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
+#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
+#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
+#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
+
+#endif
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf1f2c..acb8fd9667d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -30,6 +30,7 @@
 #include "hw/char/parallel.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/topology.h"
+#include "hw/i386/fw_cfg.h"
 #include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
@@ -88,12 +89,6 @@
 #define DPRINTF(fmt, ...)
 #endif
 
-#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
-#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
-#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
-#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
-#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Philippe Mathieu-Daudé]

Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
 hw/i386/pc.c     |  7 +------
 2 files changed, 21 insertions(+), 6 deletions(-)
 create mode 100644 hw/i386/fw_cfg.h

diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
new file mode 100644
index 00000000000..17a4bc32f22
--- /dev/null
+++ b/hw/i386/fw_cfg.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU fw_cfg helpers (X86 specific)
+ *
+ * Copyright (c) 2003-2004 Fabrice Bellard
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#ifndef HW_I386_FW_CFG_H
+#define HW_I386_FW_CFG_H
+
+#include "hw/nvram/fw_cfg.h"
+
+#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
+#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
+#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
+#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
+#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
+
+#endif
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf1f2c..acb8fd9667d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -30,6 +30,7 @@
 #include "hw/char/parallel.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/topology.h"
+#include "hw/i386/fw_cfg.h"
 #include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
@@ -88,12 +89,6 @@
 #define DPRINTF(fmt, ...)
 #endif
 
-#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
-#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
-#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
-#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
-#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/7] hw/i386: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
i386-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/Makefile.objs |  2 +-
 hw/i386/fw_cfg.c      | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 hw/i386/fw_cfg.c

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 27248a0777c..5d9c9efd5fa 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -3,7 +3,7 @@ obj-y += multiboot.o
 obj-y += pc.o
 obj-$(CONFIG_I440FX) += pc_piix.o
 obj-$(CONFIG_Q35) += pc_q35.o
-obj-y += pc_sysfw.o
+obj-y += fw_cfg.o pc_sysfw.o
 obj-y += x86-iommu.o
 obj-$(CONFIG_VTD) += intel_iommu.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
new file mode 100644
index 00000000000..c5e8b4ead0b
--- /dev/null
+++ b/hw/i386/fw_cfg.c
@@ -0,0 +1,38 @@
+/*
+ * QEMU fw_cfg helpers (X86 specific)
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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/i386/fw_cfg.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_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;
+}
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/7] hw/i386: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
i386-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/Makefile.objs |  2 +-
 hw/i386/fw_cfg.c      | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 hw/i386/fw_cfg.c

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index 27248a0777c..5d9c9efd5fa 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -3,7 +3,7 @@ obj-y += multiboot.o
 obj-y += pc.o
 obj-$(CONFIG_I440FX) += pc_piix.o
 obj-$(CONFIG_Q35) += pc_q35.o
-obj-y += pc_sysfw.o
+obj-y += fw_cfg.o pc_sysfw.o
 obj-y += x86-iommu.o
 obj-$(CONFIG_VTD) += intel_iommu.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
new file mode 100644
index 00000000000..c5e8b4ead0b
--- /dev/null
+++ b/hw/i386/fw_cfg.c
@@ -0,0 +1,38 @@
+/*
+ * QEMU fw_cfg helpers (X86 specific)
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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/i386/fw_cfg.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_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;
+}
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
ppc-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ppc/Makefile.objs |  2 +-
 hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 hw/ppc/fw_cfg.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
--- /dev/null
+++ b/hw/ppc/fw_cfg.c
@@ -0,0 +1,45 @@
+/*
+ * fw_cfg helpers (PPC specific)
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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/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;
+}
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
ppc-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ppc/Makefile.objs |  2 +-
 hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 hw/ppc/fw_cfg.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
--- /dev/null
+++ b/hw/ppc/fw_cfg.c
@@ -0,0 +1,45 @@
+/*
+ * fw_cfg helpers (PPC specific)
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * 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/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;
+}
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/7] hw/sparc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
sparc32-specific key.

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

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index ca1e3825d58..49251d62b35 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)
 {
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/7] hw/sparc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
sparc32-specific key.

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

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index ca1e3825d58..49251d62b35 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)
 {
-- 
2.20.1

[Qemu-devel] [PATCH v3 7/7] hw/sparc64: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
sparc64-specific key.

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

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 399f2d73c81..4230b17b873 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)
 {
-- 
2.20.1

[Qemu-devel] [PATCH v3 7/7] hw/sparc64: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Implement fw_cfg_arch_key_name(), which returns the name of a
sparc64-specific key.

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

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 399f2d73c81..4230b17b873 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)
 {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

On Mon, Apr 22, 2019 at 09:50:18PM +0200, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> ppc-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

What ppc machines actually use fw_cfg?  I know pseries and powernv
don't.

> ---
>  hw/ppc/Makefile.objs |  2 +-
>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/fw_cfg.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
> --- /dev/null
> +++ b/hw/ppc/fw_cfg.c
> @@ -0,0 +1,45 @@
> +/*
> + * fw_cfg helpers (PPC specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/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;
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

On Mon, Apr 22, 2019 at 09:50:18PM +0200, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> ppc-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

What ppc machines actually use fw_cfg?  I know pseries and powernv
don't.

> ---
>  hw/ppc/Makefile.objs |  2 +-
>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/fw_cfg.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
> --- /dev/null
> +++ b/hw/ppc/fw_cfg.c
> @@ -0,0 +1,45 @@
> +/*
> + * fw_cfg helpers (PPC specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/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;
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Hi David,

On 4/23/19 3:20 AM, David Gibson wrote:
> On Mon, Apr 22, 2019 at 09:50:18PM +0200, Philippe Mathieu-Daudé wrote:
>> Implement fw_cfg_arch_key_name(), which returns the name of a
>> ppc-specific key.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> What ppc machines actually use fw_cfg?  I know pseries and powernv
> don't.

I see:

- 40p
- g3beige (newworld)
- mac99 (oldworld)

I guess it is because they use OpenBIOS.

>> ---
>>  hw/ppc/Makefile.objs |  2 +-
>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/fw_cfg.c
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>> --- /dev/null
>> +++ b/hw/ppc/fw_cfg.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * fw_cfg helpers (PPC specific)
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + *
>> + * Author:
>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * 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/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;
>> +}
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 2757 bytes --]

Hi David,

On 4/23/19 3:20 AM, David Gibson wrote:
> On Mon, Apr 22, 2019 at 09:50:18PM +0200, Philippe Mathieu-Daudé wrote:
>> Implement fw_cfg_arch_key_name(), which returns the name of a
>> ppc-specific key.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> What ppc machines actually use fw_cfg?  I know pseries and powernv
> don't.

I see:

- 40p
- g3beige (newworld)
- mac99 (oldworld)

I guess it is because they use OpenBIOS.

>> ---
>>  hw/ppc/Makefile.objs |  2 +-
>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/fw_cfg.c
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>> --- /dev/null
>> +++ b/hw/ppc/fw_cfg.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * fw_cfg helpers (PPC specific)
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + *
>> + * Author:
>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * 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/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;
>> +}
> 

Re: [Qemu-devel] [PATCH v3 2/7] hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Add fw_cfg_arch_key_name() which returns the name of
> an architecture-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS               |  1 +
>  hw/nvram/fw_cfg.c         |  2 +-
>  include/hw/nvram/fw_cfg.h | 11 +++++++++++
>  stubs/Makefile.objs       |  1 +
>  stubs/fw_cfg.c            | 21 +++++++++++++++++++++
>  5 files changed, 35 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/fw_cfg.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 56139ac8ab0..444783bb652 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1675,6 +1675,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/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d374a970fea..b2dc0a80cbc 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -100,7 +100,7 @@ static const char *key_name(uint16_t key)
>      };
>  
>      if (key & FW_CFG_ARCH_LOCAL) {
> -        return NULL;
> +        return fw_cfg_arch_key_name(key);
>      }
>      if (key < FW_CFG_FILE_FIRST) {
>          return fw_cfg_wellknown_keys[key];
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a740..828ad9dedc6 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.
> + */

We might want to document that we expect FW_CFG_ARCH_LOCAL to be set in
"key", but I really don't insist.

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

Thanks
Laszlo

> +const char *fw_cfg_arch_key_name(uint16_t key);
> +
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 269dfa58326..73452ad2657 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 00000000000..bb1e3c8aa95
> --- /dev/null
> +++ b/stubs/fw_cfg.c
> @@ -0,0 +1,21 @@
> +/*
> + * fw_cfg stubs
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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 v3 2/7] hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Add fw_cfg_arch_key_name() which returns the name of
> an architecture-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS               |  1 +
>  hw/nvram/fw_cfg.c         |  2 +-
>  include/hw/nvram/fw_cfg.h | 11 +++++++++++
>  stubs/Makefile.objs       |  1 +
>  stubs/fw_cfg.c            | 21 +++++++++++++++++++++
>  5 files changed, 35 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/fw_cfg.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 56139ac8ab0..444783bb652 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1675,6 +1675,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/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d374a970fea..b2dc0a80cbc 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -100,7 +100,7 @@ static const char *key_name(uint16_t key)
>      };
>  
>      if (key & FW_CFG_ARCH_LOCAL) {
> -        return NULL;
> +        return fw_cfg_arch_key_name(key);
>      }
>      if (key < FW_CFG_FILE_FIRST) {
>          return fw_cfg_wellknown_keys[key];
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a740..828ad9dedc6 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.
> + */

We might want to document that we expect FW_CFG_ARCH_LOCAL to be set in
"key", but I really don't insist.

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

Thanks
Laszlo

> +const char *fw_cfg_arch_key_name(uint16_t key);
> +
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 269dfa58326..73452ad2657 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 00000000000..bb1e3c8aa95
> --- /dev/null
> +++ b/stubs/fw_cfg.c
> @@ -0,0 +1,21 @@
> +/*
> + * fw_cfg stubs
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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 v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
>  hw/i386/pc.c     |  7 +------
>  2 files changed, 21 insertions(+), 6 deletions(-)
>  create mode 100644 hw/i386/fw_cfg.h
> 
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> new file mode 100644
> index 00000000000..17a4bc32f22
> --- /dev/null
> +++ b/hw/i386/fw_cfg.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU fw_cfg helpers (X86 specific)
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: MIT
> + */

(1) This is a new file -- I understand it could be plain code movement,
but shouldn't you add your (= RH's) copyright notice too (beyond Fabrice's)?

(2) I admit I'm confused by the difference between:
- include/hw/i386/*.h
- hw/i386/*.h

One could say that the latter is "internal" (compare e.g.
"intel_iommu.h" from the former and "intel_iommu_internal.h" from the
latter) -- but then, as a counter-example, we have *both* "ioapic.h" and
"ioapic_internal.h" under the former!

> +
> +#ifndef HW_I386_FW_CFG_H
> +#define HW_I386_FW_CFG_H
> +
> +#include "hw/nvram/fw_cfg.h"
> +
> +#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
> +#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
> +#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
> +#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
> +#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
> +
> +#endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2c..acb8fd9667d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -30,6 +30,7 @@
>  #include "hw/char/parallel.h"
>  #include "hw/i386/apic.h"
>  #include "hw/i386/topology.h"
> +#include "hw/i386/fw_cfg.h"
>  #include "sysemu/cpus.h"
>  #include "hw/block/fdc.h"
>  #include "hw/ide.h"
> @@ -88,12 +89,6 @@
>  #define DPRINTF(fmt, ...)
>  #endif
>  
> -#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> -#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> -#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> -#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> -#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> -
>  #define E820_NR_ENTRIES		16
>  
>  struct e820_entry {
> 

I'm not insisting on any particular code changes here, just please
consider (1) and (2) above in some way. (Stating why the code is fine
as-is is OK by me too.)

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
>  hw/i386/pc.c     |  7 +------
>  2 files changed, 21 insertions(+), 6 deletions(-)
>  create mode 100644 hw/i386/fw_cfg.h
> 
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> new file mode 100644
> index 00000000000..17a4bc32f22
> --- /dev/null
> +++ b/hw/i386/fw_cfg.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU fw_cfg helpers (X86 specific)
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: MIT
> + */

(1) This is a new file -- I understand it could be plain code movement,
but shouldn't you add your (= RH's) copyright notice too (beyond Fabrice's)?

(2) I admit I'm confused by the difference between:
- include/hw/i386/*.h
- hw/i386/*.h

One could say that the latter is "internal" (compare e.g.
"intel_iommu.h" from the former and "intel_iommu_internal.h" from the
latter) -- but then, as a counter-example, we have *both* "ioapic.h" and
"ioapic_internal.h" under the former!

> +
> +#ifndef HW_I386_FW_CFG_H
> +#define HW_I386_FW_CFG_H
> +
> +#include "hw/nvram/fw_cfg.h"
> +
> +#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
> +#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
> +#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
> +#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
> +#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
> +
> +#endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2c..acb8fd9667d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -30,6 +30,7 @@
>  #include "hw/char/parallel.h"
>  #include "hw/i386/apic.h"
>  #include "hw/i386/topology.h"
> +#include "hw/i386/fw_cfg.h"
>  #include "sysemu/cpus.h"
>  #include "hw/block/fdc.h"
>  #include "hw/ide.h"
> @@ -88,12 +89,6 @@
>  #define DPRINTF(fmt, ...)
>  #endif
>  
> -#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> -#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> -#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> -#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> -#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> -
>  #define E820_NR_ENTRIES		16
>  
>  struct e820_entry {
> 

I'm not insisting on any particular code changes here, just please
consider (1) and (2) above in some way. (Stating why the code is fine
as-is is OK by me too.)

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 4/7] hw/i386: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> i386-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/Makefile.objs |  2 +-
>  hw/i386/fw_cfg.c      | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 hw/i386/fw_cfg.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 27248a0777c..5d9c9efd5fa 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -3,7 +3,7 @@ obj-y += multiboot.o
>  obj-y += pc.o
>  obj-$(CONFIG_I440FX) += pc_piix.o
>  obj-$(CONFIG_Q35) += pc_q35.o
> -obj-y += pc_sysfw.o
> +obj-y += fw_cfg.o pc_sysfw.o
>  obj-y += x86-iommu.o
>  obj-$(CONFIG_VTD) += intel_iommu.o
>  obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> new file mode 100644
> index 00000000000..c5e8b4ead0b
> --- /dev/null
> +++ b/hw/i386/fw_cfg.c
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU fw_cfg helpers (X86 specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/i386/fw_cfg.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_ACPI_TABLES, "acpi_tables"},
> +        {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
> +        {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
> +        {FW_CFG_E820_TABLE, "e820_tables"},

s/e820_tables/e820_table/

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

Thanks
Laszlo

> +        {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;
> +}
> 

Re: [Qemu-devel] [PATCH v3 4/7] hw/i386: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> i386-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/i386/Makefile.objs |  2 +-
>  hw/i386/fw_cfg.c      | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 hw/i386/fw_cfg.c
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index 27248a0777c..5d9c9efd5fa 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -3,7 +3,7 @@ obj-y += multiboot.o
>  obj-y += pc.o
>  obj-$(CONFIG_I440FX) += pc_piix.o
>  obj-$(CONFIG_Q35) += pc_q35.o
> -obj-y += pc_sysfw.o
> +obj-y += fw_cfg.o pc_sysfw.o
>  obj-y += x86-iommu.o
>  obj-$(CONFIG_VTD) += intel_iommu.o
>  obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> new file mode 100644
> index 00000000000..c5e8b4ead0b
> --- /dev/null
> +++ b/hw/i386/fw_cfg.c
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU fw_cfg helpers (X86 specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/i386/fw_cfg.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_ACPI_TABLES, "acpi_tables"},
> +        {FW_CFG_SMBIOS_ENTRIES, "smbios_entries"},
> +        {FW_CFG_IRQ0_OVERRIDE, "irq0_override"},
> +        {FW_CFG_E820_TABLE, "e820_tables"},

s/e820_tables/e820_table/

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

Thanks
Laszlo

> +        {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;
> +}
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> ppc-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/ppc/Makefile.objs |  2 +-
>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/fw_cfg.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
> --- /dev/null
> +++ b/hw/ppc/fw_cfg.c
> @@ -0,0 +1,45 @@
> +/*
> + * fw_cfg helpers (PPC specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/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;
> +}
> 

(1) Have you considered extracting the struct type, and the linear
search, to code that's shared between the arches?

It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
target-specific.

(It's not complex code so I don't mind if you opt for the code duplication.)

(2) PPC highlights my question#2 from under "v3 3/7". Namely, we
extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
constants already exist in "include/hw/ppc/ppc.h". (My point being the
difference in the "include/" pathname prefix.) Should we be consistent?

If you decide to stick with this variant:

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> ppc-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/ppc/Makefile.objs |  2 +-
>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/fw_cfg.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
> --- /dev/null
> +++ b/hw/ppc/fw_cfg.c
> @@ -0,0 +1,45 @@
> +/*
> + * fw_cfg helpers (PPC specific)
> + *
> + * Copyright (c) 2019 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * 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/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;
> +}
> 

(1) Have you considered extracting the struct type, and the linear
search, to code that's shared between the arches?

It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
target-specific.

(It's not complex code so I don't mind if you opt for the code duplication.)

(2) PPC highlights my question#2 from under "v3 3/7". Namely, we
extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
constants already exist in "include/hw/ppc/ppc.h". (My point being the
difference in the "include/" pathname prefix.) Should we be consistent?

If you decide to stick with this variant:

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 6/7] hw/sparc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> sparc32-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sparc/sun4m.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index ca1e3825d58..49251d62b35 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)
>  {
> 

My previous questions apply. From my POV,

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

Re: [Qemu-devel] [PATCH v3 6/7] hw/sparc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> sparc32-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sparc/sun4m.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index ca1e3825d58..49251d62b35 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)
>  {
> 

My previous questions apply. From my POV,

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

Re: [Qemu-devel] [PATCH v3 7/7] hw/sparc64: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> sparc64-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sparc64/sun4u.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 399f2d73c81..4230b17b873 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)
>  {
> 

Same questions. From my POV:

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 7/7] hw/sparc64: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Implement fw_cfg_arch_key_name(), which returns the name of a
> sparc64-specific key.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sparc64/sun4u.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 399f2d73c81..4230b17b873 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)
>  {
> 

Same questions. From my POV:

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

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 4/23/19 8:38 PM, Laszlo Ersek wrote:
> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>> Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
>>  hw/i386/pc.c     |  7 +------
>>  2 files changed, 21 insertions(+), 6 deletions(-)
>>  create mode 100644 hw/i386/fw_cfg.h
>>
>> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
>> new file mode 100644
>> index 00000000000..17a4bc32f22
>> --- /dev/null
>> +++ b/hw/i386/fw_cfg.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU fw_cfg helpers (X86 specific)
>> + *
>> + * Copyright (c) 2003-2004 Fabrice Bellard
>> + *
>> + * SPDX-License-Identifier: MIT
>> + */
> 
> (1) This is a new file -- I understand it could be plain code movement,
> but shouldn't you add your (= RH's) copyright notice too (beyond Fabrice's)?

I asked few people on IRC, than googled and finally kept this link
(understable enough for me):
https://en.wikipedia.org/wiki/Derivative_work#When_does_derivative-work_copyright_apply?

  US Copyright Office Circular 14: Derivative Works notes that:

   [...] To be copyrightable, a derivative work must be different
   enough from the original to be regarded as a "new work" or
   must contain a substantial amount of new material. Making minor
   changes or additions of little substance to a preexisting work
   will not qualify the work as a new version for copyright
   purposes. [...]

Since I'm simply moving lines of code with no logical modification, I
understood it is not sufficient to add a new copyright entry...

> (2) I admit I'm confused by the difference between:
> - include/hw/i386/*.h
> - hw/i386/*.h
> 
> One could say that the latter is "internal" (compare e.g.
> "intel_iommu.h" from the former and "intel_iommu_internal.h" from the
> latter) -- but then, as a counter-example, we have *both* "ioapic.h" and
> "ioapic_internal.h" under the former!

There is a slow effort to keep API namespaces as simple/strict as
possible, but the cleaning is taking time :)

- hw/i386/*.h contains declarations used by
  hw/i386/{.,kvm,xen,../hyperv}*.c

- include/hw/i386/*.h contains declaration of X86-specific devices which
  are not located in hw/i386:

  - hw/acpi (this will be cleaned with merging NEMU patches)
  - hw/intc (apic, ioapic)
  - hw/timer (hpet)
  - hw/isa (southbridge, superio)
  - hw/pci-host (northbridge)

I am spending my personal time cleaning this, since it is not a project
priority, so it is taking me a lot.

>> +
>> +#ifndef HW_I386_FW_CFG_H
>> +#define HW_I386_FW_CFG_H
>> +
>> +#include "hw/nvram/fw_cfg.h"
>> +
>> +#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
>> +#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
>> +#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
>> +#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
>> +#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
>> +
>> +#endif
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf1f2c..acb8fd9667d 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -30,6 +30,7 @@
>>  #include "hw/char/parallel.h"
>>  #include "hw/i386/apic.h"
>>  #include "hw/i386/topology.h"
>> +#include "hw/i386/fw_cfg.h"
>>  #include "sysemu/cpus.h"
>>  #include "hw/block/fdc.h"
>>  #include "hw/ide.h"
>> @@ -88,12 +89,6 @@
>>  #define DPRINTF(fmt, ...)
>>  #endif
>>  
>> -#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
>> -#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
>> -#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
>> -#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
>> -#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>> -
>>  #define E820_NR_ENTRIES		16
>>  
>>  struct e820_entry {
>>
> 
> I'm not insisting on any particular code changes here, just please
> consider (1) and (2) above in some way. (Stating why the code is fine
> as-is is OK by me too.)
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 4/23/19 8:38 PM, Laszlo Ersek wrote:
> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>> Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
>>  hw/i386/pc.c     |  7 +------
>>  2 files changed, 21 insertions(+), 6 deletions(-)
>>  create mode 100644 hw/i386/fw_cfg.h
>>
>> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
>> new file mode 100644
>> index 00000000000..17a4bc32f22
>> --- /dev/null
>> +++ b/hw/i386/fw_cfg.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU fw_cfg helpers (X86 specific)
>> + *
>> + * Copyright (c) 2003-2004 Fabrice Bellard
>> + *
>> + * SPDX-License-Identifier: MIT
>> + */
> 
> (1) This is a new file -- I understand it could be plain code movement,
> but shouldn't you add your (= RH's) copyright notice too (beyond Fabrice's)?

I asked few people on IRC, than googled and finally kept this link
(understable enough for me):
https://en.wikipedia.org/wiki/Derivative_work#When_does_derivative-work_copyright_apply?

  US Copyright Office Circular 14: Derivative Works notes that:

   [...] To be copyrightable, a derivative work must be different
   enough from the original to be regarded as a "new work" or
   must contain a substantial amount of new material. Making minor
   changes or additions of little substance to a preexisting work
   will not qualify the work as a new version for copyright
   purposes. [...]

Since I'm simply moving lines of code with no logical modification, I
understood it is not sufficient to add a new copyright entry...

> (2) I admit I'm confused by the difference between:
> - include/hw/i386/*.h
> - hw/i386/*.h
> 
> One could say that the latter is "internal" (compare e.g.
> "intel_iommu.h" from the former and "intel_iommu_internal.h" from the
> latter) -- but then, as a counter-example, we have *both* "ioapic.h" and
> "ioapic_internal.h" under the former!

There is a slow effort to keep API namespaces as simple/strict as
possible, but the cleaning is taking time :)

- hw/i386/*.h contains declarations used by
  hw/i386/{.,kvm,xen,../hyperv}*.c

- include/hw/i386/*.h contains declaration of X86-specific devices which
  are not located in hw/i386:

  - hw/acpi (this will be cleaned with merging NEMU patches)
  - hw/intc (apic, ioapic)
  - hw/timer (hpet)
  - hw/isa (southbridge, superio)
  - hw/pci-host (northbridge)

I am spending my personal time cleaning this, since it is not a project
priority, so it is taking me a lot.

>> +
>> +#ifndef HW_I386_FW_CFG_H
>> +#define HW_I386_FW_CFG_H
>> +
>> +#include "hw/nvram/fw_cfg.h"
>> +
>> +#define FW_CFG_ACPI_TABLES      (FW_CFG_ARCH_LOCAL + 0)
>> +#define FW_CFG_SMBIOS_ENTRIES   (FW_CFG_ARCH_LOCAL + 1)
>> +#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
>> +#define FW_CFG_E820_TABLE       (FW_CFG_ARCH_LOCAL + 3)
>> +#define FW_CFG_HPET             (FW_CFG_ARCH_LOCAL + 4)
>> +
>> +#endif
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf1f2c..acb8fd9667d 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -30,6 +30,7 @@
>>  #include "hw/char/parallel.h"
>>  #include "hw/i386/apic.h"
>>  #include "hw/i386/topology.h"
>> +#include "hw/i386/fw_cfg.h"
>>  #include "sysemu/cpus.h"
>>  #include "hw/block/fdc.h"
>>  #include "hw/ide.h"
>> @@ -88,12 +89,6 @@
>>  #define DPRINTF(fmt, ...)
>>  #endif
>>  
>> -#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
>> -#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
>> -#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
>> -#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
>> -#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>> -
>>  #define E820_NR_ENTRIES		16
>>  
>>  struct e820_entry {
>>
> 
> I'm not insisting on any particular code changes here, just please
> consider (1) and (2) above in some way. (Stating why the code is fine
> as-is is OK by me too.)
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 4/23/19 9:02 PM, Laszlo Ersek wrote:
> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>> Implement fw_cfg_arch_key_name(), which returns the name of a
>> ppc-specific key.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/ppc/Makefile.objs |  2 +-
>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/fw_cfg.c
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>> --- /dev/null
>> +++ b/hw/ppc/fw_cfg.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * fw_cfg helpers (PPC specific)
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + *
>> + * Author:
>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * 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/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;
>> +}
>>
> 
> (1) Have you considered extracting the struct type, and the linear
> search, to code that's shared between the arches?
> 
> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
> target-specific.

Yes, I tried different ways:

1/ Declare as extern

If we declare the array as 'extern const', we can no more use the
ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
(No need to use a getter() since the array is *const*).

I personally try to avoid extern variables when possible, I find them
bug prone.

2/ Add a macro in the header, use it in each source

The macro is ugly to read, the result looked worse to me.

3/ I don't expect new keys to be added often, and adding them will be
trivial 1-line patch each key.

I might be unaware of better ways to deduplicate this, so if you have
suggestions I'm happy to learn :)

> (It's not complex code so I don't mind if you opt for the code duplication.)
> 
> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
> constants already exist in "include/hw/ppc/ppc.h". (My point being the
> difference in the "include/" pathname prefix.) Should we be consistent?

I'd like to be consistent :)

So far only machines set fw_cfg keys.

I don't see arch-specific devices accessing arch-specific fw_cfg keys,
so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
include/hw/$ARCH/fw_cfg.h).

> If you decide to stick with this variant:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 4/23/19 9:02 PM, Laszlo Ersek wrote:
> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>> Implement fw_cfg_arch_key_name(), which returns the name of a
>> ppc-specific key.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/ppc/Makefile.objs |  2 +-
>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/fw_cfg.c
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>> --- /dev/null
>> +++ b/hw/ppc/fw_cfg.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * fw_cfg helpers (PPC specific)
>> + *
>> + * Copyright (c) 2019 Red Hat, Inc.
>> + *
>> + * Author:
>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * 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/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;
>> +}
>>
> 
> (1) Have you considered extracting the struct type, and the linear
> search, to code that's shared between the arches?
> 
> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
> target-specific.

Yes, I tried different ways:

1/ Declare as extern

If we declare the array as 'extern const', we can no more use the
ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
(No need to use a getter() since the array is *const*).

I personally try to avoid extern variables when possible, I find them
bug prone.

2/ Add a macro in the header, use it in each source

The macro is ugly to read, the result looked worse to me.

3/ I don't expect new keys to be added often, and adding them will be
trivial 1-line patch each key.

I might be unaware of better ways to deduplicate this, so if you have
suggestions I'm happy to learn :)

> (It's not complex code so I don't mind if you opt for the code duplication.)
> 
> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
> constants already exist in "include/hw/ppc/ppc.h". (My point being the
> difference in the "include/" pathname prefix.) Should we be consistent?

I'd like to be consistent :)

So far only machines set fw_cfg keys.

I don't see arch-specific devices accessing arch-specific fw_cfg keys,
so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
include/hw/$ARCH/fw_cfg.h).

> If you decide to stick with this variant:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/29/19 18:01, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 4/23/19 9:02 PM, Laszlo Ersek wrote:
>> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>>> Implement fw_cfg_arch_key_name(), which returns the name of a
>>> ppc-specific key.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/ppc/Makefile.objs |  2 +-
>>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>>  create mode 100644 hw/ppc/fw_cfg.c
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>>> --- /dev/null
>>> +++ b/hw/ppc/fw_cfg.c
>>> @@ -0,0 +1,45 @@
>>> +/*
>>> + * fw_cfg helpers (PPC specific)
>>> + *
>>> + * Copyright (c) 2019 Red Hat, Inc.
>>> + *
>>> + * Author:
>>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * 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/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;
>>> +}
>>>
>>
>> (1) Have you considered extracting the struct type, and the linear
>> search, to code that's shared between the arches?
>>
>> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
>> target-specific.
> 
> Yes, I tried different ways:
> 
> 1/ Declare as extern
> 
> If we declare the array as 'extern const', we can no more use the
> ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
> (No need to use a getter() since the array is *const*).
> 
> I personally try to avoid extern variables when possible, I find them
> bug prone.
> 
> 2/ Add a macro in the header, use it in each source
> 
> The macro is ugly to read, the result looked worse to me.
> 
> 3/ I don't expect new keys to be added often, and adding them will be
> trivial 1-line patch each key.
> 
> I might be unaware of better ways to deduplicate this, so if you have
> suggestions I'm happy to learn :)

In the loop condition, you could replace ARRAY_SIZE with a terminator
element check, and you could terminate the arrays with an

  { FW_CFG_INVALID, NULL }

element. Then the loop could be extracted, and you wouldn't need further
size_t globals, for replacing ARRAY_SIZE.

But, again, it's not that important.

Thanks,
Laszlo

>> (It's not complex code so I don't mind if you opt for the code duplication.)
>>
>> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
>> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
>> constants already exist in "include/hw/ppc/ppc.h". (My point being the
>> difference in the "include/" pathname prefix.) Should we be consistent?
> 
> I'd like to be consistent :)
> 
> So far only machines set fw_cfg keys.
> 
> I don't see arch-specific devices accessing arch-specific fw_cfg keys,
> so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
> include/hw/$ARCH/fw_cfg.h).
> 
>> If you decide to stick with this variant:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> 
>>
>> Thanks
>> Laszlo
>>

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Laszlo Ersek]

On 04/29/19 18:01, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 4/23/19 9:02 PM, Laszlo Ersek wrote:
>> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>>> Implement fw_cfg_arch_key_name(), which returns the name of a
>>> ppc-specific key.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/ppc/Makefile.objs |  2 +-
>>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>>  create mode 100644 hw/ppc/fw_cfg.c
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>>> --- /dev/null
>>> +++ b/hw/ppc/fw_cfg.c
>>> @@ -0,0 +1,45 @@
>>> +/*
>>> + * fw_cfg helpers (PPC specific)
>>> + *
>>> + * Copyright (c) 2019 Red Hat, Inc.
>>> + *
>>> + * Author:
>>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * 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/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;
>>> +}
>>>
>>
>> (1) Have you considered extracting the struct type, and the linear
>> search, to code that's shared between the arches?
>>
>> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
>> target-specific.
> 
> Yes, I tried different ways:
> 
> 1/ Declare as extern
> 
> If we declare the array as 'extern const', we can no more use the
> ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
> (No need to use a getter() since the array is *const*).
> 
> I personally try to avoid extern variables when possible, I find them
> bug prone.
> 
> 2/ Add a macro in the header, use it in each source
> 
> The macro is ugly to read, the result looked worse to me.
> 
> 3/ I don't expect new keys to be added often, and adding them will be
> trivial 1-line patch each key.
> 
> I might be unaware of better ways to deduplicate this, so if you have
> suggestions I'm happy to learn :)

In the loop condition, you could replace ARRAY_SIZE with a terminator
element check, and you could terminate the arrays with an

  { FW_CFG_INVALID, NULL }

element. Then the loop could be extracted, and you wouldn't need further
size_t globals, for replacing ARRAY_SIZE.

But, again, it's not that important.

Thanks,
Laszlo

>> (It's not complex code so I don't mind if you opt for the code duplication.)
>>
>> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
>> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
>> constants already exist in "include/hw/ppc/ppc.h". (My point being the
>> difference in the "include/" pathname prefix.) Should we be consistent?
> 
> I'd like to be consistent :)
> 
> So far only machines set fw_cfg keys.
> 
> I don't see arch-specific devices accessing arch-specific fw_cfg keys,
> so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
> include/hw/$ARCH/fw_cfg.h).
> 
>> If you decide to stick with this variant:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> 
>>
>> Thanks
>> Laszlo
>>

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

On 4/30/19 11:41 AM, Laszlo Ersek wrote:
> On 04/29/19 18:01, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 4/23/19 9:02 PM, Laszlo Ersek wrote:
>>> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>>>> Implement fw_cfg_arch_key_name(), which returns the name of a
>>>> ppc-specific key.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/ppc/Makefile.objs |  2 +-
>>>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>>>  create mode 100644 hw/ppc/fw_cfg.c
>>>>
>>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>>>> --- /dev/null
>>>> +++ b/hw/ppc/fw_cfg.c
>>>> @@ -0,0 +1,45 @@
>>>> +/*
>>>> + * fw_cfg helpers (PPC specific)
>>>> + *
>>>> + * Copyright (c) 2019 Red Hat, Inc.
>>>> + *
>>>> + * Author:
>>>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + *
>>>> + * 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/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;
>>>> +}
>>>>
>>>
>>> (1) Have you considered extracting the struct type, and the linear
>>> search, to code that's shared between the arches?
>>>
>>> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
>>> target-specific.
>>
>> Yes, I tried different ways:
>>
>> 1/ Declare as extern
>>
>> If we declare the array as 'extern const', we can no more use the
>> ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
>> (No need to use a getter() since the array is *const*).
>>
>> I personally try to avoid extern variables when possible, I find them
>> bug prone.
>>
>> 2/ Add a macro in the header, use it in each source
>>
>> The macro is ugly to read, the result looked worse to me.
>>
>> 3/ I don't expect new keys to be added often, and adding them will be
>> trivial 1-line patch each key.
>>
>> I might be unaware of better ways to deduplicate this, so if you have
>> suggestions I'm happy to learn :)
> 
> In the loop condition, you could replace ARRAY_SIZE with a terminator
> element check, and you could terminate the arrays with an
> 
>   { FW_CFG_INVALID, NULL }
> 
> element. Then the loop could be extracted, and you wouldn't need further
> size_t globals, for replacing ARRAY_SIZE.

Clever, I forgot this way :>

> 
> But, again, it's not that important.
> 
> Thanks,
> Laszlo
> 
>>> (It's not complex code so I don't mind if you opt for the code duplication.)
>>>
>>> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
>>> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
>>> constants already exist in "include/hw/ppc/ppc.h". (My point being the
>>> difference in the "include/" pathname prefix.) Should we be consistent?
>>
>> I'd like to be consistent :)
>>
>> So far only machines set fw_cfg keys.
>>
>> I don't see arch-specific devices accessing arch-specific fw_cfg keys,
>> so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
>> include/hw/$ARCH/fw_cfg.h).
>>
>>> If you decide to stick with this variant:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks!
>>
>>>
>>> Thanks
>>> Laszlo
>>>
> 

Re: [Qemu-devel] [PATCH v3 5/7] hw/ppc: Implement fw_cfg_arch_key_name()

[Philippe Mathieu-Daudé]

On 4/30/19 11:41 AM, Laszlo Ersek wrote:
> On 04/29/19 18:01, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 4/23/19 9:02 PM, Laszlo Ersek wrote:
>>> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>>>> Implement fw_cfg_arch_key_name(), which returns the name of a
>>>> ppc-specific key.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/ppc/Makefile.objs |  2 +-
>>>>  hw/ppc/fw_cfg.c      | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 46 insertions(+), 1 deletion(-)
>>>>  create mode 100644 hw/ppc/fw_cfg.c
>>>>
>>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>>> index 1111b218a04..ae940981553 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 00000000000..a88b5c4bde2
>>>> --- /dev/null
>>>> +++ b/hw/ppc/fw_cfg.c
>>>> @@ -0,0 +1,45 @@
>>>> +/*
>>>> + * fw_cfg helpers (PPC specific)
>>>> + *
>>>> + * Copyright (c) 2019 Red Hat, Inc.
>>>> + *
>>>> + * Author:
>>>> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + *
>>>> + * 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/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;
>>>> +}
>>>>
>>>
>>> (1) Have you considered extracting the struct type, and the linear
>>> search, to code that's shared between the arches?
>>>
>>> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
>>> target-specific.
>>
>> Yes, I tried different ways:
>>
>> 1/ Declare as extern
>>
>> If we declare the array as 'extern const', we can no more use the
>> ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
>> (No need to use a getter() since the array is *const*).
>>
>> I personally try to avoid extern variables when possible, I find them
>> bug prone.
>>
>> 2/ Add a macro in the header, use it in each source
>>
>> The macro is ugly to read, the result looked worse to me.
>>
>> 3/ I don't expect new keys to be added often, and adding them will be
>> trivial 1-line patch each key.
>>
>> I might be unaware of better ways to deduplicate this, so if you have
>> suggestions I'm happy to learn :)
> 
> In the loop condition, you could replace ARRAY_SIZE with a terminator
> element check, and you could terminate the arrays with an
> 
>   { FW_CFG_INVALID, NULL }
> 
> element. Then the loop could be extracted, and you wouldn't need further
> size_t globals, for replacing ARRAY_SIZE.

Clever, I forgot this way :>

> 
> But, again, it's not that important.
> 
> Thanks,
> Laszlo
> 
>>> (It's not complex code so I don't mind if you opt for the code duplication.)
>>>
>>> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
>>> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
>>> constants already exist in "include/hw/ppc/ppc.h". (My point being the
>>> difference in the "include/" pathname prefix.) Should we be consistent?
>>
>> I'd like to be consistent :)
>>
>> So far only machines set fw_cfg keys.
>>
>> I don't see arch-specific devices accessing arch-specific fw_cfg keys,
>> so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
>> include/hw/$ARCH/fw_cfg.h).
>>
>>> If you decide to stick with this variant:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks!
>>
>>>
>>> Thanks
>>> Laszlo
>>>
> 

Re: [Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing

[Philippe Mathieu-Daudé]

On 4/22/19 9:50 PM, Philippe Mathieu-Daudé wrote:
> Trivial series to improve fw_cfg tracing.
> 
> Regards,
> 
> Phil.
> 
> Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
> - Split arch-specific code (1 patch per arch) (requested by Laszlo)
> 
> Since v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html
> - Added arch-specific keys
> 
> Philippe Mathieu-Daudé (7):
>   hw/nvram/fw_cfg: Add trace events
>   hw/nvram/fw_cfg: Add fw_cfg_arch_key_name()
>   hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
>   hw/i386: Implement fw_cfg_arch_key_name()
>   hw/ppc: Implement fw_cfg_arch_key_name()
>   hw/sparc: Implement fw_cfg_arch_key_name()
>   hw/sparc64: Implement fw_cfg_arch_key_name()

Queued, fixing the typo reported by Laszlo, documenting the PPC uses.