I can only muster some random thoughts for this one, presently: On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: > $ qemu-system-x86_64 -S -monitor stdio > (qemu) info fw_cfg > Type Perm Size Specific Order Info > signature RO 4 QEMU > id RO 4 0x00000003 > uuid RO 16 > 00000000-0000-0000-0000-000000000000 > ram_size RO 8 0x0000000008000000 > nographic RO 2 0x0000 > nb_cpus RO 2 0x0001 > numa RO 16 > boot_menu RO 2 0x0000 > max_cpus RO 2 0x0001 > file_dir RO 2052 > file (id 1) RO 36 160 etc/acpi/rsdp > file (id 2) RO 131072 130 etc/acpi/tables > file (id 3) RO 4 15 etc/boot-fail-wait > file (id 4) RO 20 40 etc/e820 > file (id 5) RO 31 30 etc/smbios/smbios-anchor > file (id 6) RO 320 20 etc/smbios/smbios-tables > file (id 7) RO 6 90 etc/system-states > file (id 8) RO 4096 140 etc/table-loader > file (id 10) RO 9216 55 genroms/kvmvapic.bin > uuid RO 4 (arch spec) > 01000000-0000-0000-0000-000000000000 > ram_size RO 324 (arch spec) > nographic RO 121 (arch spec) > (qemu) > > Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> > --- > hmp-commands-info.hx | 14 +++++ > hw/nvram/fw_cfg.c | 115 ++++++++++++++++++++++++++++++++++++++ > include/hw/nvram/fw_cfg.h | 2 + > 3 files changed, 131 insertions(+) > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > index cbee8b944d..9e86dceeb4 100644 > --- a/hmp-commands-info.hx > +++ b/hmp-commands-info.hx > @@ -916,6 +916,20 @@ STEXI > @item info sev > @findex info sev > Show SEV information. > +ETEXI > + > + { > + .name = "fw_cfg", > + .args_type = "", > + .params = "", > + .help = "Display the table of the fw_cfg entries registered", > + .cmd = hmp_info_fw_cfg, > + }, > + > +STEXI > +@item info fw_cfg > +@findex info fw_cfg > +Show roms. > ETEXI > > STEXI > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 582653f07e..50525ec1dd 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -36,6 +36,7 @@ > #include "qemu/config-file.h" > #include "qemu/cutils.h" > #include "qapi/error.h" > +#include "monitor/monitor.h" > > #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > @@ -1164,3 +1165,117 @@ static void fw_cfg_register_types(void) > } > > type_init(fw_cfg_register_types) > + > +static const char *fw_cfg_wellknown_entries[0x20] = {
(1) Using 0x20 is not wrong here (it is a well-known magic constant), but we should refer to it by name: please refer to FW_CFG_FILE_FIRST. (See "include/standard-headers/linux/qemu_fw_cfg.h") > + [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", > +}; (2) I suggest introducing a macro that generates the initializer, using the stringify() macro from "include/qemu/compiler.h". It should go something like #define FW_CFG_KEY_STRINGIFY(key) [key] = stringify(key), Feel free to disagree though; this set of predefined, fixed numeric keys will never change (nor will it ever be extened), so we might as well live with the currently proposed verbosity. > + > +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict) > +{ > + FWCfgState *s = fw_cfg_find(); > + int arch, key; > + > + monitor_printf(mon, "%12s %5s %7s %9s %6s %-24s\n", > + "Type", "Perm", "Size", "Specific", "Order", "Info"); > + for (arch = 0; arch < ARRAY_SIZE(s->entries); ++arch) { > + for (key = 0; key < fw_cfg_max_entry(s); ++key) { > + FWCfgEntry *e = &s->entries[arch][key]; > + const char *perm = e->allow_write ? "RW" : "RO"; > + const char *arch_spec = arch ? "arch_spec" : ""; > + char *type, *info = NULL; > + > + if (!e->len) { > + continue; > + } > + if (key >= FW_CFG_FILE_FIRST) { > + int id = key - FW_CFG_FILE_FIRST; > + const char *path = s->files->f[id].name; > + > + type = g_strdup_printf("file (id %d)", id); > + monitor_printf(mon, "%12s %5s %7d %10s %5d %-24s\n", > + type, perm, e->len, arch_spec, > + get_fw_cfg_order(s, path), path); > + g_free(type); > + continue; > + } > + type = g_strdup(fw_cfg_wellknown_entries[key]); > + if (!type) { > + type = g_strdup_printf("entry_%d", key); > + } > + > + switch (key) { > + case FW_CFG_SIGNATURE: > + info = g_strndup((const gchar *)e->data, e->len); > + break; > + case FW_CFG_UUID: > + info = qemu_uuid_unparse_strdup((const QemuUUID *)e->data); > + break; > + case FW_CFG_ID: > + case FW_CFG_NOGRAPHIC: > + case FW_CFG_NB_CPUS: > + case FW_CFG_BOOT_MENU: > + case FW_CFG_MAX_CPUS: > + case FW_CFG_RAM_SIZE: > + case FW_CFG_KERNEL_ADDR: > + case FW_CFG_KERNEL_SIZE: > + case FW_CFG_KERNEL_CMDLINE: > + case FW_CFG_KERNEL_ENTRY: > + case FW_CFG_KERNEL_DATA: > + case FW_CFG_INITRD_ADDR: > + case FW_CFG_INITRD_SIZE: > + case FW_CFG_INITRD_DATA: > + case FW_CFG_CMDLINE_ADDR: > + case FW_CFG_CMDLINE_SIZE: > + case FW_CFG_CMDLINE_DATA: > + case FW_CFG_SETUP_ADDR: > + case FW_CFG_SETUP_SIZE: > + case FW_CFG_SETUP_DATA: > + switch (e->len) { > + case 2: > + info = g_strdup_printf("0x%04x", lduw_le_p(e->data)); > + break; > + case 4: > + info = g_strdup_printf("0x%08x", ldl_le_p(e->data)); > + break; > + case 8: > + info = g_strdup_printf("0x%016" PRIx64, > ldq_le_p(e->data)); > + break; > + default: > + break; > + } > + break; > + default: > + break; > + } > + monitor_printf(mon, "%12s %5s %7d %10s %5s %-24s\n", > + type, perm, e->len, arch_spec, "", info ? info : > ""); > + g_free(type); > + g_free(info); > + } > + } > +} (3) In general I wouldn't try to be smart here, regarding the contents. I suggest simply dumping arrays of uint8_t values, in hex. (4) Also I wouldn't reuse the same column for different purposes; I think it's easier to read if a column always means the same thing, and if it doesn't apply, it's just left blank. Something like this: Selector Well-Known Numeric Pathname Perm Size Arch-Spec Order Data - "Selector" is the uint16 selector key - "Well-Known Numeric" is the stringified name if the selector refers to a well-known numerically defined item. - "Pathname" is the pathname for named files. Mutually exclusive with "Well-Known Numeric", but that's OK, IMO. - "Arch-Spec" should be left blank, or marked as "set" with an asterisk. - "Data" should be a hexdump of uint8_t values (normally limited to 8 bytes, if the array is larger). (5) I think this interface would benefit from being exposed over QMP / JSON, and then the tabular presentation would be a separate question in HMP. (6) If we want bells and whistles, two optional parameters could be considered: one for identifying one specific key to request info about (identify by selector? well-known macro name? file pathname?), and another param for placing a limit, different from 8, on the individual hexdump size. Important: these are not "requirements", just random ideas, food for thought. I'm fine if you reject any subset of them, after consideration. I don't intend this to spiral into feature creep; take whatever you like. Thanks! Laszlo > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index f5a6895a74..28630b2f26 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -226,4 +226,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, > FWCfgState *fw_cfg_find(void); > bool fw_cfg_dma_enabled(void *opaque); > > +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict); > + > #endif >