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
> >