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

[Michael S. Tsirkin]

On Fri, Sep 08, 2017 at 11:49:46AM -0400, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > On Fri, Sep 08, 2017 at 06:39:01PM +0300, Michael S. Tsirkin wrote:
> > > On Mon, Aug 07, 2017 at 08:16:13PM +0200, Marc-André Lureau wrote:
> > > > 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.
> > > > +
> > > 
> > > I would say adding a format bitmap would make sense for future
> > > compatibility.
> > > How about:
> > > 
> > > struct FWCfgVMCoreInfo {
> > > uint16_t host_format;   /* Formats host supports. 0x1 LE - ELF
> > > note. Other bits - ignored. */
> 
> Could this be added later?
>
> For ex, extend the entry to 16 bytes, with those 2 values appended?

I think aligned size is better overall so you can write
bindings in any language without issues.
I'm ok with just keeping 0s there for now if you prefer.


> If the size is 12, assume it is ELF note only, and that the host supports it.

Seems more like a hack that will make code ugly down the road.

> > > uint16_t guest_format;  /* Formats guest supplies. Must be 0x1 LE
> > > */
> > 
> > .. to set the ELF note info, or 0x0 to reset the device.
> > 
> > > uint32_t size;  /* size of ELF note region, LE */
> > > uint64_t paddr; /* physical address of ELF note, LE */
> > > };
> > > 
> > > 
> > > >  === All Other Data Items ===
> > > >  
> > > >  Please consult the QEMU source for the most up-to-date and 
> > > > authoritative
> > > >  list
> > > > --
> > > > 2.14.0.1.geff633fa0
> > 

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

[Marc-André Lureau]

Hi

- Original Message -
> On Fri, Sep 08, 2017 at 06:39:01PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Aug 07, 2017 at 08:16:13PM +0200, Marc-André Lureau wrote:
> > > 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.
> > > +
> > 
> > I would say adding a format bitmap would make sense for future
> > compatibility.
> > How about:
> > 
> > struct FWCfgVMCoreInfo {
> > uint16_t host_format;   /* Formats host supports. 0x1 LE - ELF
> > note. Other bits - ignored. */

Could this be added later?

For ex, extend the entry to 16 bytes, with those 2 values appended?

If the size is 12, assume it is ELF note only, and that the host supports it.

> > uint16_t guest_format;  /* Formats guest supplies. Must be 0x1 LE
> > */
> 
> .. to set the ELF note info, or 0x0 to reset the device.
> 
> > uint32_t size;  /* size of ELF note region, LE */
> > uint64_t paddr; /* physical address of ELF note, LE */
> > };
> > 
> > 
> > >  === All Other Data Items ===
> > >  
> > >  Please consult the QEMU source for the most up-to-date and authoritative
> > >  list
> > > --
> > > 2.14.0.1.geff633fa0
> 

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

[Michael S. Tsirkin]

On Fri, Sep 08, 2017 at 06:39:01PM +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 07, 2017 at 08:16:13PM +0200, Marc-André Lureau wrote:
> > 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.
> > +
> 
> I would say adding a format bitmap would make sense for future compatibility.
> How about:
> 
> struct FWCfgVMCoreInfo {
> uint16_t host_format;   /* Formats host supports. 0x1 LE - ELF note. 
> Other bits - ignored. */
> uint16_t guest_format;  /* Formats guest supplies. Must be 0x1 LE */

.. to set the ELF note info, or 0x0 to reset the device.

> uint32_t size;  /* size of ELF note region, LE */
> uint64_t paddr; /* physical address of ELF note, LE */
> };
> 
> 
> >  === All Other Data Items ===
> >  
> >  Please consult the QEMU source for the most up-to-date and authoritative 
> > list
> > -- 
> > 2.14.0.1.geff633fa0

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:
> 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.
> +

I would say adding a format bitmap would make sense for future compatibility.
How about:

struct FWCfgVMCoreInfo {
uint16_t host_format;   /* Formats host supports. 0x1 LE - ELF note. 
Other bits - ignored. */
uint16_t guest_format;  /* Formats guest supplies. Must be 0x1 LE */
uint32_t size;  /* size of ELF note region, LE */
uint64_t paddr; /* physical address of ELF note, LE */
};


>  === All Other Data Items ===
>  
>  Please consult the QEMU source for the most up-to-date and authoritative list
> -- 
> 2.14.0.1.geff633fa0

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 
> ---
>  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,
> + >vmcoreinfo, sizeof(s->vmcoreinfo), 
> false);
> +}
> +
>  s->machine_ready.notify = fw_cfg_machine_ready;
>  qemu_add_machine_init_done_notifier(>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 

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

I guess this means there's no value guest can write to clear it
without a reset. That's not good for e.g. kexec.
Let's specify all 0s means disabled too?


> @@ -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,
> + >vmcoreinfo, sizeof(s->vmcoreinfo), 
> false);
> +}
> +
>  s->machine_ready.notify = fw_cfg_machine_ready;
>  qemu_add_machine_init_done_notifier(>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 

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 

I don't like it that it's part of the fw cfg device.
Could we make this off by default and only add this with
some -device?

Thanks,
MST

> ---
>  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;
> +} 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;
>  }
>  
>  /* 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,
> + >vmcoreinfo, sizeof(s->vmcoreinfo), 
> false);
> +}
> +
>  s->machine_ready.notify = fw_cfg_machine_ready;
>  qemu_add_machine_init_done_notifier(>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 

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

[Marc-André Lureau]

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 
---
 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;
+} 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;
 }
 
 /* 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,
+ >vmcoreinfo, sizeof(s->vmcoreinfo), false);
+}
+
 s->machine_ready.notify = fw_cfg_machine_ready;
 qemu_add_machine_init_done_notifier(>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