Re: [Qemu-devel] [PATCH v5 3/8] fw_cfg: add vmcoreinfo file

[Michael S. Tsirkin]

On Mon, Aug 07, 2017 at 08:16:13PM +0200, Marc-André Lureau wrote:
> See docs/specs/fw_cfg.txt for details.
> 
> The "etc/vmcoreinfo" is added when using "-global
> fw_cfg.vmcoreinfo=on" qemu option.
> 
> Disabled by default for machine types v2.9 and older.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/hw/compat.h       |  8 ++++++++
>  include/hw/nvram/fw_cfg.h |  9 +++++++++
>  hw/nvram/fw_cfg.c         | 20 ++++++++++++++++++++
>  docs/specs/fw_cfg.txt     | 16 ++++++++++++++++
>  4 files changed, 53 insertions(+)
> 
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 08f36004da..317fd2e2e3 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -18,6 +18,14 @@
>          .driver   = "pcie-root-port",\
>          .property = "x-migrate-msix",\
>          .value    = "false",\
> +    },{\
> +        .driver   = "fw_cfg_mem",\
> +        .property = "vmcoreinfo",\
> +        .value    = "off",\
> +    },{\
> +        .driver   = "fw_cfg_io",\
> +        .property = "vmcoreinfo",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_8 \
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 3527cd51d8..a35f47405d 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -30,6 +30,11 @@ typedef struct FWCfgFile {
>  void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
>  void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
>  
> +typedef struct FWCfgVMCoreInfo {
> +    uint64_t paddr;
> +    uint32_t size;

Pls add padding to align structure size to multiple of 8 bytes.

> +} QEMU_PACKED FWCfgVMCoreInfo;
> +
>  typedef struct FWCfgFiles {
>      uint32_t  count;
>      FWCfgFile f[];
> @@ -65,6 +70,10 @@ struct FWCfgState {
>      dma_addr_t dma_addr;
>      AddressSpace *dma_as;
>      MemoryRegion dma_iomem;
> +
> +    bool vmcoreinfo_enabled;
> +    bool has_vmcoreinfo;
> +    FWCfgVMCoreInfo vmcoreinfo;
>  };
>  
>  struct FWCfgIoState {
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 28780088b9..342afc4ed2 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -504,6 +504,7 @@ static void fw_cfg_reset(DeviceState *d)
>  
>      /* we never register a read callback for FW_CFG_SIGNATURE */
>      fw_cfg_select(s, FW_CFG_SIGNATURE);
> +    s->has_vmcoreinfo = false;

I do not think this is enough.  If guest only writes the last couple of
bytes you leak some info from before to after reset.
You want to zero the whole structure.



>  }
>  
>  /* Save restore 32 bit int as uint16_t
> @@ -869,7 +870,12 @@ static void fw_cfg_machine_ready(struct Notifier *n, 
> void *data)
>      qemu_register_reset(fw_cfg_machine_reset, s);
>  }
>  
> +static void fw_cfg_vmci_written(void *dev)
> +{
> +    FWCfgState *s = FW_CFG(dev);
>  
> +    s->has_vmcoreinfo = true;
> +}
>  
>  static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
>  {
> @@ -895,6 +901,16 @@ static void fw_cfg_common_realize(DeviceState *dev, 
> Error **errp)
>  
>      fw_cfg_add_i32(s, FW_CFG_ID, version);
>  
> +    if (s->vmcoreinfo_enabled) {
> +        if (!s->dma_enabled) {
> +            error_setg(errp, "vmcoreinfo requires dma_enabled");
> +            return;
> +        }
> +        fw_cfg_add_file_callback(s, "etc/vmcoreinfo",
> +                                 NULL, fw_cfg_vmci_written, s,
> +                                 &s->vmcoreinfo, sizeof(s->vmcoreinfo), 
> false);
> +    }
> +
>      s->machine_ready.notify = fw_cfg_machine_ready;
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
>  }
> @@ -1031,6 +1047,8 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s, 
> Error **errp)
>  static Property fw_cfg_io_properties[] = {
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_BOOL("vmcoreinfo", FWCfgIoState, 
> parent_obj.vmcoreinfo_enabled,
> +                     true),
>      DEFINE_PROP_UINT16("x-file-slots", FWCfgIoState, parent_obj.file_slots,
>                         FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -1082,6 +1100,8 @@ static Property fw_cfg_mem_properties[] = {
>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_BOOL("vmcoreinfo", FWCfgMemState, 
> parent_obj.vmcoreinfo_enabled,
> +                     true),
>      DEFINE_PROP_UINT16("x-file-slots", FWCfgMemState, parent_obj.file_slots,
>                         FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 08c00bdf44..37d0f9f40a 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -136,6 +136,22 @@ struct FWCfgFile {               /* an individual file 
> entry, 64 bytes total */
>      char name[56];           /* fw_cfg item name, NUL-terminated ascii */
>  };
>  
> +=== etc/vmcoreinfo ===
> +
> +A guest may use this entry to add information details to qemu
> +dumps. The entry gives location and size of an ELF note that is
> +appended in qemu dumps.
> +
> +The entry is of 12 bytes with this format:
> +
> +struct FWCfgVMCoreInfo {
> +    uint64_t paddr;             /* physical address of ELF note, LE */
> +    uint32_t size;              /* size of ELF note region, LE */
> +};
> +
> +The note format/class must be of the target bitness and the size must
> +be less than 1Mb.
> +
>  === All Other Data Items ===
>  
>  Please consult the QEMU source for the most up-to-date and authoritative list
> -- 
> 2.14.0.1.geff633fa0
>