* [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host()
@ 2018-12-07 17:03 Philippe Mathieu-Daudé
2018-12-07 17:03 ` [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() Philippe Mathieu-Daudé
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw)
To: Michael S . Tsirkin, Laszlo Ersek, Dr . David Alan Gilbert,
Peter Maydell
Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, Igor Mammedov,
Eduardo Habkost
Hi, various fw_cfg easy patches:
- First patches are trivial cleanups (and add trace events),
- patch 5 add a 'info fw_cfg' HMP command to display comprehensive list of
fw_cfg entries registered,
- patch 6 add fw_cfg_add_file_from_host(), a helper to map a file from the
host (using g_file_get_contents). This will be used by later UEFI series.
Philippe Mathieu-Daudé (6):
hw/arm/virt: Remove null-check in virt_build_smbios()
hw/arm: Remove unused include
hw/i386: Remove unused include
hw/nvram/fw_cfg: Add trace events
hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command
hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()
hmp-commands-info.hx | 14 ++++
hw/acpi/piix4.c | 1 -
hw/arm/virt-acpi-build.c | 1 -
hw/arm/virt.c | 4 --
hw/i386/acpi-build.c | 1 -
hw/i386/pc.c | 1 -
hw/nvram/fw_cfg.c | 142 ++++++++++++++++++++++++++++++++++++++
hw/nvram/trace-events | 5 ++
include/hw/nvram/fw_cfg.h | 24 +++++++
9 files changed, 185 insertions(+), 8 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 26+ messages in thread* [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé @ 2018-12-07 17:03 ` Philippe Mathieu-Daudé 2018-12-10 16:02 ` Laszlo Ersek 2018-12-07 17:03 ` [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include Philippe Mathieu-Daudé ` (5 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek, Peter Maydell Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). There is no need to null check it. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/arm/virt.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index f69e7eb399..36303ed59c 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) size_t smbios_tables_len, smbios_anchor_len; const char *product = "QEMU Virtual Machine"; - if (!vms->fw_cfg) { - return; - } - if (kvm_enabled()) { product = "KVM Virtual Machine"; } -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() 2018-12-07 17:03 ` [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() Philippe Mathieu-Daudé @ 2018-12-10 16:02 ` Laszlo Ersek 2019-03-05 21:01 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 26+ messages in thread From: Laszlo Ersek @ 2018-12-10 16:02 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Michael S . Tsirkin, Peter Maydell Cc: qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost, Drew Jones, Wei Huang On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: > Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). > There is no need to null check it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/arm/virt.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index f69e7eb399..36303ed59c 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) > size_t smbios_tables_len, smbios_anchor_len; > const char *product = "QEMU Virtual Machine"; > > - if (!vms->fw_cfg) { > - return; > - } > - > if (kvm_enabled()) { > product = "KVM Virtual Machine"; > } > This patch looks correct to me; however, the fact that fw_cfg is always present on the virt board does not seem related to commit af1f60a4022 af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said commit only flattened the contents of VirtGuestInfo into VirtMachineState; the conditions under which fw_cfg would be created were not changed. As far as I can see (from a number of git-blame / git-show commands), fw_cfg was added unconditionally to the virt board in commit 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the check that you are now (correctly) removing has always been superfluous -- it was superfluous already when it was first added in commit c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07). As far as I can tell, anyway. If you agree, I suggest mentioning commit c30e15658b1b in the commit message, rather than commit af1f60a4022. With such an update: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() 2018-12-10 16:02 ` Laszlo Ersek @ 2019-03-05 21:01 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2019-03-05 21:01 UTC (permalink / raw) To: Laszlo Ersek, Michael S . Tsirkin Cc: Peter Maydell, qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost, Drew Jones, Wei Huang Hi Laszlo, On 12/10/18 5:02 PM, Laszlo Ersek wrote: > On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: >> Since af1f60a4022, the fw_cfg field is always created in machvirt_init(). >> There is no need to null check it. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/arm/virt.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index f69e7eb399..36303ed59c 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1279,10 +1279,6 @@ static void virt_build_smbios(VirtMachineState *vms) >> size_t smbios_tables_len, smbios_anchor_len; >> const char *product = "QEMU Virtual Machine"; >> >> - if (!vms->fw_cfg) { >> - return; >> - } >> - >> if (kvm_enabled()) { >> product = "KVM Virtual Machine"; >> } >> > > This patch looks correct to me; however, the fact that fw_cfg is always > present on the virt board does not seem related to commit af1f60a4022 > af1f60a40226 ("hw/arm/virt: remove VirtGuestInfo", 2017-01-09). Said > commit only flattened the contents of VirtGuestInfo into > VirtMachineState; the conditions under which fw_cfg would be created > were not changed. This is certainly not related to commit af1f60a4022... This is not the commit I remember I wanted to refer to, I suppose I did a copy/paste mistake, thanks for carefully reviewing me and catching this! > As far as I can see (from a number of git-blame / git-show commands), > fw_cfg was added unconditionally to the virt board in commit > 578f3c7b0835 ("arm: add fw_cfg to "virt" board", 2014-12-22). And the > check that you are now (correctly) removing has always been superfluous > -- it was superfluous already when it was first added in commit > c30e15658b1b ("smbios: implement smbios support for mach-virt", 2015-09-07). > > As far as I can tell, anyway. > > If you agree, I suggest mentioning commit c30e15658b1b in the commit > message, rather than commit af1f60a4022. Yes, this one makes more sense ;) > > With such an update: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Phil. > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() Philippe Mathieu-Daudé @ 2018-12-07 17:03 ` Philippe Mathieu-Daudé 2018-12-07 17:57 ` Michael S. Tsirkin 2018-12-07 17:03 ` [Qemu-devel] [PATCH 3/6] hw/i386: " Philippe Mathieu-Daudé ` (4 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek, Peter Maydell Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost, Shannon Zhao Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/arm/virt-acpi-build.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 5785fb697c..98d7f7cf20 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -35,7 +35,6 @@ #include "target/arm/cpu.h" #include "hw/acpi/acpi-defs.h" #include "hw/acpi/acpi.h" -#include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #include "hw/loader.h" #include "hw/hw.h" -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include 2018-12-07 17:03 ` [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include Philippe Mathieu-Daudé @ 2018-12-07 17:57 ` Michael S. Tsirkin 2018-12-10 9:14 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 26+ messages in thread From: Michael S. Tsirkin @ 2018-12-07 17:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, Peter Maydell, qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost, Shannon Zhao On Fri, Dec 07, 2018 at 06:03:56PM +0100, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/arm/virt-acpi-build.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 5785fb697c..98d7f7cf20 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -35,7 +35,6 @@ > #include "target/arm/cpu.h" > #include "hw/acpi/acpi-defs.h" > #include "hw/acpi/acpi.h" > -#include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > #include "hw/loader.h" > #include "hw/hw.h" This file uses fw_cfg_add_file, looks wrong. > -- > 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include 2018-12-07 17:57 ` Michael S. Tsirkin @ 2018-12-10 9:14 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-10 9:14 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laszlo Ersek, Peter Maydell, qemu-devel, qemu-arm, Igor Mammedov, Eduardo Habkost, Shannon Zhao Hi Michael, Peter. On 12/7/18 6:57 PM, Michael S. Tsirkin wrote: > On Fri, Dec 07, 2018 at 06:03:56PM +0100, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/arm/virt-acpi-build.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 5785fb697c..98d7f7cf20 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -35,7 +35,6 @@ >> #include "target/arm/cpu.h" >> #include "hw/acpi/acpi-defs.h" >> #include "hw/acpi/acpi.h" >> -#include "hw/nvram/fw_cfg.h" >> #include "hw/acpi/bios-linker-loader.h" >> #include "hw/loader.h" >> #include "hw/hw.h" > > > This file uses fw_cfg_add_file, looks wrong. Previously I thought if another include (here hw/loader.h) already pulls this header, we can drop it, but Peter Maydell explained me this not safe for refactors: Generally I think that if a .c file directly uses function X declared in header Y it should #include Y, even if it happens that it already includes other header Z that includes Y. Otherwise if we refactor Z later such that it no longer needs to include Y, it will break compilation of the .c file. (That is, Z including Y is a detail of the implementation of Z, not a guarantee made by Z to its users.) https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02460.html I'm sorry I forgot to update this in the series :/ Regards, Phil. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 3/6] hw/i386: Remove unused include 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include Philippe Mathieu-Daudé @ 2018-12-07 17:03 ` Philippe Mathieu-Daudé 2018-12-07 17:43 ` Michael S. Tsirkin 2018-12-07 17:03 ` [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé ` (3 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek Cc: Philippe Mathieu-Daudé, qemu-devel, Igor Mammedov, Eduardo Habkost, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/acpi/piix4.c | 1 - hw/i386/acpi-build.c | 1 - hw/i386/pc.c | 1 - 3 files changed, 3 deletions(-) diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index e330f24c71..49a33bfcda 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -28,7 +28,6 @@ #include "sysemu/sysemu.h" #include "qapi/error.h" #include "qemu/range.h" -#include "hw/nvram/fw_cfg.h" #include "exec/address-spaces.h" #include "hw/acpi/piix4.h" #include "hw/acpi/pcihp.h" diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 236a20eaa8..1e5c371294 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -35,7 +35,6 @@ #include "hw/acpi/acpi-defs.h" #include "hw/acpi/acpi.h" #include "hw/acpi/cpu.h" -#include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #include "hw/loader.h" #include "hw/isa/isa.h" diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f095725dba..2221dcdb36 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -35,7 +35,6 @@ #include "hw/ide.h" #include "hw/pci/pci.h" #include "hw/pci/pci_bus.h" -#include "hw/nvram/fw_cfg.h" #include "hw/timer/hpet.h" #include "hw/smbios/smbios.h" #include "hw/loader.h" -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] hw/i386: Remove unused include 2018-12-07 17:03 ` [Qemu-devel] [PATCH 3/6] hw/i386: " Philippe Mathieu-Daudé @ 2018-12-07 17:43 ` Michael S. Tsirkin 0 siblings, 0 replies; 26+ messages in thread From: Michael S. Tsirkin @ 2018-12-07 17:43 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, qemu-devel, Igor Mammedov, Eduardo Habkost, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson On Fri, Dec 07, 2018 at 06:03:57PM +0100, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/acpi/piix4.c | 1 - > hw/i386/acpi-build.c | 1 - > hw/i386/pc.c | 1 - > 3 files changed, 3 deletions(-) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index e330f24c71..49a33bfcda 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -28,7 +28,6 @@ > #include "sysemu/sysemu.h" > #include "qapi/error.h" > #include "qemu/range.h" > -#include "hw/nvram/fw_cfg.h" > #include "exec/address-spaces.h" > #include "hw/acpi/piix4.h" > #include "hw/acpi/pcihp.h" Looks right. > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 236a20eaa8..1e5c371294 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -35,7 +35,6 @@ > #include "hw/acpi/acpi-defs.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/cpu.h" > -#include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > #include "hw/loader.h" > #include "hw/isa/isa.h" This one calls fw_cfg_add_file. > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index f095725dba..2221dcdb36 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -35,7 +35,6 @@ > #include "hw/ide.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > -#include "hw/nvram/fw_cfg.h" > #include "hw/timer/hpet.h" > #include "hw/smbios/smbios.h" > #include "hw/loader.h" And this calls fw_cfg_add_bytes > -- > 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2018-12-07 17:03 ` [Qemu-devel] [PATCH 3/6] hw/i386: " Philippe Mathieu-Daudé @ 2018-12-07 17:03 ` Philippe Mathieu-Daudé 2018-12-07 17:44 ` Michael S. Tsirkin 2018-12-10 16:10 ` Laszlo Ersek 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé ` (2 subsequent siblings) 6 siblings, 2 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek Cc: Philippe Mathieu-Daudé, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/nvram/fw_cfg.c | 5 +++++ hw/nvram/trace-events | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 3cb726ff68..582653f07e 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -627,6 +627,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, len); fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true); } @@ -634,6 +635,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, value); fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); } @@ -643,6 +645,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, value); fw_cfg_add_bytes(s, key, copy, sizeof(value)); } @@ -662,6 +665,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, value); fw_cfg_add_bytes(s, key, copy, sizeof(value)); } @@ -671,6 +675,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, value); fw_cfg_add_bytes(s, key, copy, sizeof(value)); } diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events index 6b55ba7a09..0ee0f8d04a 100644 --- a/hw/nvram/trace-events +++ b/hw/nvram/trace-events @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> # hw/nvram/fw_cfg.c fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64 +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%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, const char *value) "key 0x%04" PRIx16 ", value '%s'" +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64 -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events 2018-12-07 17:03 ` [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé @ 2018-12-07 17:44 ` Michael S. Tsirkin 2018-12-10 16:10 ` Laszlo Ersek 1 sibling, 0 replies; 26+ messages in thread From: Michael S. Tsirkin @ 2018-12-07 17:44 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On Fri, Dec 07, 2018 at 06:03:58PM +0100, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Well why not. Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/nvram/fw_cfg.c | 5 +++++ > hw/nvram/trace-events | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 3cb726ff68..582653f07e 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -627,6 +627,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, len); > fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true); > } > > @@ -634,6 +635,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, value); > fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); > } > > @@ -643,6 +645,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > @@ -662,6 +665,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > @@ -671,6 +675,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events > index 6b55ba7a09..0ee0f8d04a 100644 > --- a/hw/nvram/trace-events > +++ b/hw/nvram/trace-events > @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64 > +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%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, const char *value) "key 0x%04" PRIx16 ", value '%s'" > +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 > +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 > +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64 > -- > 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events 2018-12-07 17:03 ` [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé 2018-12-07 17:44 ` Michael S. Tsirkin @ 2018-12-10 16:10 ` Laszlo Ersek 2018-12-10 16:33 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 26+ messages in thread From: Laszlo Ersek @ 2018-12-10 16:10 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Michael S . Tsirkin Cc: qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/nvram/fw_cfg.c | 5 +++++ > hw/nvram/trace-events | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 3cb726ff68..582653f07e 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -627,6 +627,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, len); > fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true); > } > > @@ -634,6 +635,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, value); > fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); > } > > @@ -643,6 +645,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > @@ -662,6 +665,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > @@ -671,6 +675,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, value); > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events > index 6b55ba7a09..0ee0f8d04a 100644 > --- a/hw/nvram/trace-events > +++ b/hw/nvram/trace-events > @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64 > +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%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, const char *value) "key 0x%04" PRIx16 ", value '%s'" > +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 For the "value" parameter, you should use "uint16_t" here (and replace PRIx32 with PRIx16, as well). (In practice this will make no difference, but for consistency's sake, we should do this.) With the update: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 > +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events 2018-12-10 16:10 ` Laszlo Ersek @ 2018-12-10 16:33 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-10 16:33 UTC (permalink / raw) To: Laszlo Ersek, Michael S . Tsirkin Cc: qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On 12/10/18 5:10 PM, Laszlo Ersek wrote: > On 12/07/18 18:03, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/nvram/fw_cfg.c | 5 +++++ >> hw/nvram/trace-events | 5 +++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 3cb726ff68..582653f07e 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -627,6 +627,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, len); >> fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true); >> } >> >> @@ -634,6 +635,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, value); >> fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); >> } >> >> @@ -643,6 +645,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, value); >> fw_cfg_add_bytes(s, key, copy, sizeof(value)); >> } >> >> @@ -662,6 +665,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, value); >> fw_cfg_add_bytes(s, key, copy, sizeof(value)); >> } >> >> @@ -671,6 +675,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, value); >> fw_cfg_add_bytes(s, key, copy, sizeof(value)); >> } >> >> diff --git a/hw/nvram/trace-events b/hw/nvram/trace-events >> index 6b55ba7a09..0ee0f8d04a 100644 >> --- a/hw/nvram/trace-events >> +++ b/hw/nvram/trace-events >> @@ -7,4 +7,9 @@ nvram_write(uint32_t addr, uint32_t old, uint32_t val) "write addr %d: 0x%02x -> >> # hw/nvram/fw_cfg.c >> fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" >> fw_cfg_read(void *s, uint64_t ret) "%p = 0x%"PRIx64 >> +fw_cfg_add_bytes(uint16_t key, size_t len) "key 0x%04" PRIx16 " (%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, const char *value) "key 0x%04" PRIx16 ", value '%s'" >> +fw_cfg_add_i16(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 > > For the "value" parameter, you should use "uint16_t" here (and replace > PRIx32 with PRIx16, as well). > > (In practice this will make no difference, but for consistency's sake, > we should do this.) You are right :) > > With the update: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks > Laszlo > >> +fw_cfg_add_i32(uint16_t key, uint32_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx32 >> +fw_cfg_add_i64(uint16_t key, uint64_t value) "key 0x%04" PRIx16 ", value 0x%" PRIx64 >> > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé ` (3 preceding siblings ...) 2018-12-07 17:03 ` [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé @ 2018-12-07 17:03 ` Philippe Mathieu-Daudé 2018-12-07 17:26 ` Eric Blake ` (3 more replies) 2018-12-07 17:04 ` [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 22:48 ` [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() no-reply 6 siblings, 4 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:03 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek, Dr . David Alan Gilbert Cc: Philippe Mathieu-Daudé, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann $ 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é <philmd@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] = { + [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", +}; + +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); + } + } +} 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 -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé @ 2018-12-07 17:26 ` Eric Blake 2018-12-07 17:54 ` Michael S. Tsirkin ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Eric Blake @ 2018-12-07 17:26 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Michael S . Tsirkin, Laszlo Ersek, Dr . David Alan Gilbert Cc: Igor Mammedov, Gerd Hoffmann, qemu-devel, Eduardo Habkost On 12/7/18 11:03 AM, 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) > In general, when adding something to HMP but not QMP, it is worth a comment explaining WHY we did not want QMP. (Here, I think the reason is that this is a debugging aid likely to be useful to developers but unlikely to be needed by a management app, and matches similar other HMP-only info commands, and is therefore not worth the effort of turning it into QAPI). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé 2018-12-07 17:26 ` Eric Blake @ 2018-12-07 17:54 ` Michael S. Tsirkin 2018-12-10 9:18 ` Philippe Mathieu-Daudé 2018-12-10 11:42 ` Dr. David Alan Gilbert 2018-12-10 16:49 ` Laszlo Ersek 3 siblings, 1 reply; 26+ messages in thread From: Michael S. Tsirkin @ 2018-12-07 17:54 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, Dr . David Alan Gilbert, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote: > $ qemu-system-x86_64 -S -monitor stdio > (qemu) info fw_cfg > Type Perm Size Specific Order Info Can we do better than "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) Weird. Your code has arch_spec. > (qemu) > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Looks helpful but generally IMHO whatever is exposed through hmp should be in the qmp as well. > --- > 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", I think the help line should be a bit more verbose. Mention it's a paravirtualized interface, and why would one want to display it (debugging guest firmware?). > + .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] = { > + [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", > +}; > + > +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict) > +{ > + FWCfgState *s = fw_cfg_find(); > + int arch, key; Looks like this will crash on a machine without fw cfg. > + > + 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); > + } > + } > +} > 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 > -- > 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:54 ` Michael S. Tsirkin @ 2018-12-10 9:18 ` Philippe Mathieu-Daudé 2018-12-18 15:14 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-10 9:18 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laszlo Ersek, Dr . David Alan Gilbert, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On 12/7/18 6:54 PM, Michael S. Tsirkin wrote: > On Fri, Dec 07, 2018 at 06:03:59PM +0100, Philippe Mathieu-Daudé wrote: >> $ qemu-system-x86_64 -S -monitor stdio >> (qemu) info fw_cfg >> Type Perm Size Specific Order Info > > Can we do better than "Info"? For some entry this is the "content", but for the files this is the "path". Do you prefer "Content or path"? > >> 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) > > Weird. Your code has arch_spec. Hmmm I'll check that. > >> (qemu) >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > Looks helpful but generally IMHO whatever is exposed through hmp > should be in the qmp as well. OK. > > >> --- >> 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", > > I think the help line should be a bit more verbose. > Mention it's a paravirtualized interface, and why > would one want to display it (debugging guest firmware?). OK. > > >> + .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] = { >> + [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", >> +}; >> + >> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict) >> +{ >> + FWCfgState *s = fw_cfg_find(); >> + int arch, key; > > Looks like this will crash on a machine without fw cfg. Oops, good catch :) > > >> + >> + 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); >> + } >> + } >> +} >> 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 >> -- >> 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-10 9:18 ` Philippe Mathieu-Daudé @ 2018-12-18 15:14 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-18 15:14 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laszlo Ersek, Dr . David Alan Gilbert, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann Hi Michael, On 12/10/18 10:18 AM, Philippe Mathieu-Daudé wrote: > On 12/7/18 6:54 PM, Michael S. Tsirkin wrote: >> On Fri, Dec 07, 2018 at 06:03:59PM +0100, 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) >> >> Weird. Your code has arch_spec. > > Hmmm I'll check that. These are the entries used for Bochs: #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) static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) { [...] fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, acpi_tables, acpi_tables_len); fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override()); fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, &e820_reserve, sizeof(e820_reserve)); So what happened here is the 'type' name is incorrect (I'll fix), but the arch_spec flag is correct. Thanks for noticing this :) Regards, Phil. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé 2018-12-07 17:26 ` Eric Blake 2018-12-07 17:54 ` Michael S. Tsirkin @ 2018-12-10 11:42 ` Dr. David Alan Gilbert 2018-12-10 16:49 ` Laszlo Ersek 3 siblings, 0 replies; 26+ messages in thread From: Dr. David Alan Gilbert @ 2018-12-10 11:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S . Tsirkin, Laszlo Ersek, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann * Philippe Mathieu-Daudé (philmd@redhat.com) 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é <philmd@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] = { That magic 0x20 is a shame; we should probably have a #define for that max, or perhaps it can just be removed. > + [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", > +}; > + > +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; It seems odd to encode the type of each entry in this switch; I guess it's OK if no one else needs it, else perhaps there should be a type array. Other than that, it's fine from the HMP side, but I agree it should either have a QMP equivalent (in which case the structure is a bit different) or a good justification of if it's really debug only. Dave > + 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); > + } > + } > +} > 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 > -- > 2.17.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2018-12-10 11:42 ` Dr. David Alan Gilbert @ 2018-12-10 16:49 ` Laszlo Ersek 3 siblings, 0 replies; 26+ messages in thread From: Laszlo Ersek @ 2018-12-10 16:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Michael S . Tsirkin, Dr . David Alan Gilbert Cc: qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann 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é <philmd@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 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé ` (4 preceding siblings ...) 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé @ 2018-12-07 17:04 ` Philippe Mathieu-Daudé 2018-12-07 17:56 ` Michael S. Tsirkin 2018-12-10 17:11 ` Laszlo Ersek 2018-12-07 22:48 ` [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() no-reply 6 siblings, 2 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 17:04 UTC (permalink / raw) To: Michael S . Tsirkin, Laszlo Ersek Cc: Philippe Mathieu-Daudé, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann Add a function to read the full content of file on the host, and add a new 'file' name item to the fw_cfg device. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/nvram/fw_cfg.c | 22 ++++++++++++++++++++++ include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 50525ec1dd..f3232fcb16 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true); } +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, + const char *host_path, size_t *len) +{ + GError *gerr = NULL; + gchar *ptr = NULL; + gsize contents_len = 0; + + + if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) { + fw_cfg_add_file(s, filename, ptr, contents_len); + } else { + error_report("failed to read file %s, %s", host_path, gerr->message); + g_error_free(gerr); + return NULL; + } + if (len) { + *len = contents_len; + } + + return ptr; +} + void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data, size_t len) { diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index 28630b2f26..8de8d63433 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value); void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data, size_t len); +/** + * fw_cfg_add_file_from_host: + * @s: fw_cfg device being modified + * @filename: name of new fw_cfg file item + * @host_path: path of the host file to read the data from + * @len: pointer to hold the length of the host file (optional) + * + * Read the content of a host file as a raw "blob" then add a new NAMED + * fw_cfg item of the file size. If @len is provided, it will contains the + * total length read from the host file. The data referenced by the starting + * pointer is only linked, NOT copied, into the data structure of the fw_cfg + * device. + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST + * will be used; also, a new entry will be added to the file directory + * structure residing at key value FW_CFG_FILE_DIR, containing the item name, + * data size, and assigned selector key value. + * + * Returns: pointer to the file content, or NULL if an error occured. + */ +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, + const char *host_path, size_t *len); + /** * fw_cfg_add_file_callback: * @s: fw_cfg device being modified -- 2.17.2 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() 2018-12-07 17:04 ` [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé @ 2018-12-07 17:56 ` Michael S. Tsirkin 2018-12-07 19:17 ` Philippe Mathieu-Daudé 2018-12-10 17:11 ` Laszlo Ersek 1 sibling, 1 reply; 26+ messages in thread From: Michael S. Tsirkin @ 2018-12-07 17:56 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laszlo Ersek, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On Fri, Dec 07, 2018 at 06:04:00PM +0100, Philippe Mathieu-Daudé wrote: > Add a function to read the full content of file on the host, and add > a new 'file' name item to the fw_cfg device. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Need to be careful with options that let users play with firmware. Way too easy to create conflicts. Anyway, I'd rather have this included with whatever is using it. > --- > hw/nvram/fw_cfg.c | 22 ++++++++++++++++++++++ > include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 50525ec1dd..f3232fcb16 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, > fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true); > } > > +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, > + const char *host_path, size_t *len) > +{ > + GError *gerr = NULL; > + gchar *ptr = NULL; > + gsize contents_len = 0; > + > + > + if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) { > + fw_cfg_add_file(s, filename, ptr, contents_len); > + } else { > + error_report("failed to read file %s, %s", host_path, gerr->message); > + g_error_free(gerr); > + return NULL; > + } > + if (len) { > + *len = contents_len; > + } > + > + return ptr; > +} > + > void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > void *data, size_t len) > { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index 28630b2f26..8de8d63433 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value); > void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data, > size_t len); > > +/** > + * fw_cfg_add_file_from_host: > + * @s: fw_cfg device being modified > + * @filename: name of new fw_cfg file item > + * @host_path: path of the host file to read the data from > + * @len: pointer to hold the length of the host file (optional) > + * > + * Read the content of a host file as a raw "blob" then add a new NAMED > + * fw_cfg item of the file size. If @len is provided, it will contains the > + * total length read from the host file. The data referenced by the starting > + * pointer is only linked, NOT copied, into the data structure of the fw_cfg > + * device. > + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST > + * will be used; also, a new entry will be added to the file directory > + * structure residing at key value FW_CFG_FILE_DIR, containing the item name, > + * data size, and assigned selector key value. > + * > + * Returns: pointer to the file content, or NULL if an error occured. > + */ > +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, > + const char *host_path, size_t *len); > + > /** > * fw_cfg_add_file_callback: > * @s: fw_cfg device being modified > -- > 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() 2018-12-07 17:56 ` Michael S. Tsirkin @ 2018-12-07 19:17 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-07 19:17 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Laszlo Ersek, qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On 12/7/18 6:56 PM, Michael S. Tsirkin wrote: > On Fri, Dec 07, 2018 at 06:04:00PM +0100, Philippe Mathieu-Daudé wrote: >> Add a function to read the full content of file on the host, and add >> a new 'file' name item to the fw_cfg device. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Need to be careful with options that let users play with firmware. > Way too easy to create conflicts. > Anyway, I'd rather have this included with whatever is using it. Yes, sure. > >> --- >> hw/nvram/fw_cfg.c | 22 ++++++++++++++++++++++ >> include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 50525ec1dd..f3232fcb16 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, >> fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true); >> } >> >> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, >> + const char *host_path, size_t *len) >> +{ >> + GError *gerr = NULL; >> + gchar *ptr = NULL; >> + gsize contents_len = 0; >> + >> + >> + if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) { >> + fw_cfg_add_file(s, filename, ptr, contents_len); >> + } else { >> + error_report("failed to read file %s, %s", host_path, gerr->message); >> + g_error_free(gerr); >> + return NULL; >> + } >> + if (len) { >> + *len = contents_len; >> + } >> + >> + return ptr; >> +} >> + >> void *fw_cfg_modify_file(FWCfgState *s, const char *filename, >> void *data, size_t len) >> { >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index 28630b2f26..8de8d63433 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value); >> void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data, >> size_t len); >> >> +/** >> + * fw_cfg_add_file_from_host: >> + * @s: fw_cfg device being modified >> + * @filename: name of new fw_cfg file item >> + * @host_path: path of the host file to read the data from >> + * @len: pointer to hold the length of the host file (optional) >> + * >> + * Read the content of a host file as a raw "blob" then add a new NAMED >> + * fw_cfg item of the file size. If @len is provided, it will contains the >> + * total length read from the host file. The data referenced by the starting >> + * pointer is only linked, NOT copied, into the data structure of the fw_cfg >> + * device. >> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST >> + * will be used; also, a new entry will be added to the file directory >> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name, >> + * data size, and assigned selector key value. >> + * >> + * Returns: pointer to the file content, or NULL if an error occured. >> + */ >> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, >> + const char *host_path, size_t *len); >> + >> /** >> * fw_cfg_add_file_callback: >> * @s: fw_cfg device being modified >> -- >> 2.17.2 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() 2018-12-07 17:04 ` [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 17:56 ` Michael S. Tsirkin @ 2018-12-10 17:11 ` Laszlo Ersek 1 sibling, 0 replies; 26+ messages in thread From: Laszlo Ersek @ 2018-12-10 17:11 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Michael S . Tsirkin Cc: qemu-devel, Igor Mammedov, Eduardo Habkost, Gerd Hoffmann On 12/07/18 18:04, Philippe Mathieu-Daudé wrote: > Add a function to read the full content of file on the host, and add > a new 'file' name item to the fw_cfg device. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/nvram/fw_cfg.c | 22 ++++++++++++++++++++++ > include/hw/nvram/fw_cfg.h | 22 ++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 50525ec1dd..f3232fcb16 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -842,6 +842,28 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, > fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true); > } > > +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, > + const char *host_path, size_t *len) > +{ > + GError *gerr = NULL; > + gchar *ptr = NULL; > + gsize contents_len = 0; > + > + > + if (g_file_get_contents(host_path, &ptr, &contents_len, &gerr)) { > + fw_cfg_add_file(s, filename, ptr, contents_len); Can you rename "ptr" to "data" or "contents"? > + } else { > + error_report("failed to read file %s, %s", host_path, gerr->message); OK... most functions in this file that populate fw_cfg and can fail (from external data) still use error_report() + a NULL retval, so this looks consistent. > + g_error_free(gerr); > + return NULL; > + } > + if (len) { > + *len = contents_len; > + } > + > + return ptr; > +} > + > void *fw_cfg_modify_file(FWCfgState *s, const char *filename, > void *data, size_t len) > { > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index 28630b2f26..8de8d63433 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -166,6 +166,28 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value); > void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data, > size_t len); > > +/** > + * fw_cfg_add_file_from_host: > + * @s: fw_cfg device being modified > + * @filename: name of new fw_cfg file item > + * @host_path: path of the host file to read the data from > + * @len: pointer to hold the length of the host file (optional) > + * > + * Read the content of a host file as a raw "blob" then add a new NAMED > + * fw_cfg item of the file size. If @len is provided, it will contains the s/will contains/will contain/ > + * total length read from the host file. The data referenced by the starting > + * pointer is only linked, NOT copied, into the data structure of the fw_cfg > + * device. Please drop the last sentence; it does not apply here. There is no starting pointer passed to this function. ... Should we perhaps replace the last sentence by stating that the data read from the host filesystem is owned by the new fw_cfg entry? > + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST > + * will be used; also, a new entry will be added to the file directory > + * structure residing at key value FW_CFG_FILE_DIR, containing the item name, > + * data size, and assigned selector key value. > + * > + * Returns: pointer to the file content, or NULL if an error occured. > + */ > +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename, > + const char *host_path, size_t *len); > + > /** > * fw_cfg_add_file_callback: > * @s: fw_cfg device being modified > Hmmmm. If you do return the pointer to the data just read, directly exposing the allocation address to the caller, then we could in theory make this new interface consistent with the rest, and make the *caller* own the data. Then the currently proposed documentation would be sort-of accurate (the new fw_cfg entry would not own the data just read -- the caller would). It's hard to divine what's the best approach here, without seeing any call sites. ... Purely from a consistency POV, I think I'm slightly attracted to make the fw_cfg reference again a non-owning one, and force the caller of fw_cfg_add_file_from_host() to own the data read. Thanks Laszlo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé ` (5 preceding siblings ...) 2018-12-07 17:04 ` [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé @ 2018-12-07 22:48 ` no-reply 2018-12-10 9:22 ` Philippe Mathieu-Daudé 6 siblings, 1 reply; 26+ messages in thread From: no-reply @ 2018-12-07 22:48 UTC (permalink / raw) To: philmd Cc: famz, mst, lersek, dgilbert, peter.maydell, imammedo, qemu-arm, qemu-devel, ehabkost Patchew URL: https://patchew.org/QEMU/20181207170400.5129-1-philmd@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END === libpmem support no libudev no WARNING: Use of SDL 1.2 is deprecated and will be removed in WARNING: future releases. Please switch to using SDL 2.0 NOTE: cross-compilers enabled: 'cc' GEN x86_64-softmmu/config-devices.mak.tmp The full log is available at http://patchew.org/logs/20181207170400.5129-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() 2018-12-07 22:48 ` [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() no-reply @ 2018-12-10 9:22 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 26+ messages in thread From: Philippe Mathieu-Daudé @ 2018-12-10 9:22 UTC (permalink / raw) To: qemu-devel Cc: famz, mst, lersek, dgilbert, peter.maydell, imammedo, qemu-arm, ehabkost On 12/7/18 11:48 PM, no-reply@patchew.org wrote: > This series failed the docker-quick@centos7 build test. Please find the testing commands and > their output below. If you have Docker installed, you can probably reproduce it > locally. > ... > The full log is available at > http://patchew.org/logs/20181207170400.5129-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message. qemu-system-x86_64: Back to tcg accelerator Broken pipe /tmp/qemu-test/src/tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) GTester: last random seed: R02Sb6a4400485731f23217a8855e030b450 Broken pipe Probably the error Michael noticed: >> +void hmp_info_fw_cfg(Monitor *mon, const QDict *qdict) >> +{ >> + FWCfgState *s = fw_cfg_find(); >> + int arch, key; > > Looks like this will crash on a machine without fw cfg. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-03-05 21:01 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-07 17:03 [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 1/6] hw/arm/virt: Remove null-check in virt_build_smbios() Philippe Mathieu-Daudé 2018-12-10 16:02 ` Laszlo Ersek 2019-03-05 21:01 ` Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 2/6] hw/arm: Remove unused include Philippe Mathieu-Daudé 2018-12-07 17:57 ` Michael S. Tsirkin 2018-12-10 9:14 ` Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 3/6] hw/i386: " Philippe Mathieu-Daudé 2018-12-07 17:43 ` Michael S. Tsirkin 2018-12-07 17:03 ` [Qemu-devel] [PATCH 4/6] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé 2018-12-07 17:44 ` Michael S. Tsirkin 2018-12-10 16:10 ` Laszlo Ersek 2018-12-10 16:33 ` Philippe Mathieu-Daudé 2018-12-07 17:03 ` [Qemu-devel] [PATCH 5/6] hw/nvram/fw_cfg: Add HMP 'info fw_cfg' command Philippe Mathieu-Daudé 2018-12-07 17:26 ` Eric Blake 2018-12-07 17:54 ` Michael S. Tsirkin 2018-12-10 9:18 ` Philippe Mathieu-Daudé 2018-12-18 15:14 ` Philippe Mathieu-Daudé 2018-12-10 11:42 ` Dr. David Alan Gilbert 2018-12-10 16:49 ` Laszlo Ersek 2018-12-07 17:04 ` [Qemu-devel] [PATCH 6/6] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé 2018-12-07 17:56 ` Michael S. Tsirkin 2018-12-07 19:17 ` Philippe Mathieu-Daudé 2018-12-10 17:11 ` Laszlo Ersek 2018-12-07 22:48 ` [Qemu-devel] [PATCH 0/6] fw_cfg: add HMP 'info fw_cfg' and add_file_from_host() no-reply 2018-12-10 9:22 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).