Re: [Qemu-devel] fw cfg files cross-version migration races

[Michael S. Tsirkin]

On Mon, Jun 01, 2015 at 11:32:37AM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 01, 2015 at 03:13:43PM +0100, Daniel P. Berrange wrote:
> > On Mon, Jun 01, 2015 at 04:10:54PM +0200, Michael S. Tsirkin wrote:
> > > Hi all,
> > > At the moment we have devices adding and removing fw cfg file entries.
> > > One problem is that this makes the contents of fw cfg
> > > depend on order of device initialization.
> > > Since the fw cfg file list is not migrated, this means that
> > > guest will break if it is migrated between qemu versions
> > > which generate the list diferently, and if migration triggers
> > > while guest happens to read fw cfg.
> > > 
> > > As there are plans to extend the use of fw cfg, I think it's
> > > important to fix this issue sooner rather than later.
> > > 
> > > Ideas:
> > >   - sort fw cfg files by name before exposing them to guest
> > >   - keep doing what we did for compat machine types,
> > >     hope that things don't break too often
> > > 
> > > More ideas? Comments? Anyone wants to try implementing this?
> > 
> > Shouldn't we migrate the fw cfg data that the source host generates
> > originally,  rather than trying to play games make sure the way it
> > is re-generated on dest doesn't change.
> 
> Right now, in hw/nvram/fw_cfg.c, we have:
> 
> struct FWCfgState {
>     /*< private >*/
>     SysBusDevice parent_obj; 
>     /*< public >*/
>     
>     FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>     FWCfgFiles *files;
>     uint16_t cur_entry;
>     uint32_t cur_offset;
>     Notifier machine_ready;
> };
> 
> and, later:
> 
> static const VMStateDescription vmstate_fw_cfg = {
>     .name = "fw_cfg",
>     .version_id = 2,
>     .minimum_version_id = 1,
>     .fields = (VMStateField[]) {
>         VMSTATE_UINT16(cur_entry, FWCfgState),
>         VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1),
>         VMSTATE_UINT32_V(cur_offset, FWCfgState, 2),
>         VMSTATE_END_OF_LIST() 
>     }
> };  
> 
> Would this be as simple as adding a VMSTATE_ARRAY* for 'entries'
> and something like a VMSTATE_VBUFFER_ALLOC_UINT32 for 'files', which
> is dynamically allocated the first time a fwcfg "file" is inserted ?
> 
> The one catch is that the value of the "files" pointer is itself a
> fw_cfg entry (FW_CFG_FILE_DIR), so that would need to be "patched"
> on the destination side...
> 
> I do like the idea of simply migrating the full content of the fw_cfg
> device though, seems like the safest solution.
> 
> Thanks much,
> --Gabriel

OK but you need to do a bunch of work on load, e.g. some fw cfg
entries trigger callbacks on access, etc.