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

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

[Michael S. Tsirkin]

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?

-- 
MST

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

[Daniel P. Berrange]

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.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Gabriel L. Somlo]

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

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.

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

[Gabriel L. Somlo]

On Mon, Jun 01, 2015 at 05:44:47PM +0200, Michael S. Tsirkin wrote:
> > > 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.

Oh, you mean here:

  typedef struct FWCfgEntry {
    uint32_t len;
    uint8_t *data;
    void *callback_opaque;
    FWCfgReadCallback read_callback;
  } FWCfgEntry;

... I can't just assume that 'read_callback' is a valid function
pointer in the context of the destination host ?

Ouch, that could get painful really really quickly :)

Thanks,
--Gabriel

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

[Gabriel L. Somlo]

On Mon, Jun 01, 2015 at 02:00:22PM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 01, 2015 at 05:44:47PM +0200, Michael S. Tsirkin wrote:
> > > > 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.
> 
> Oh, you mean here:
> 
>   typedef struct FWCfgEntry {
>     uint32_t len;
>     uint8_t *data;
>     void *callback_opaque;
>     FWCfgReadCallback read_callback;
>   } FWCfgEntry;
> 
> ... I can't just assume that 'read_callback' is a valid function
> pointer in the context of the destination host ?
> 
> Ouch, that could get painful really really quickly :)

Actually, it's much worse than that. A lot of the data items stored in
fw_cfg are just pointers to somewhere in the qemu process address
space, and I have no confidence that these pointers are guaranteed to
make sense in the address space of the *destination* qemu process...

I guess the only reason this isn't a problem is that nobody currently
attempts to access fw_cfg after a migration ? :)

--Gabriel

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

[Laszlo Ersek]

(resending, with Paolo's address corrected)

On 06/01/15 22:31, Gabriel L. Somlo wrote:
> On Mon, Jun 01, 2015 at 02:00:22PM -0400, Gabriel L. Somlo wrote:
>> On Mon, Jun 01, 2015 at 05:44:47PM +0200, Michael S. Tsirkin wrote:
>>>>> 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.
>>
>> Oh, you mean here:
>>
>>   typedef struct FWCfgEntry {
>>     uint32_t len;
>>     uint8_t *data;
>>     void *callback_opaque;
>>     FWCfgReadCallback read_callback;
>>   } FWCfgEntry;
>>
>> ... I can't just assume that 'read_callback' is a valid function
>> pointer in the context of the destination host ?
>>
>> Ouch, that could get painful really really quickly :)
> 
> Actually, it's much worse than that. A lot of the data items stored in
> fw_cfg are just pointers to somewhere in the qemu process address
> space, and I have no confidence that these pointers are guaranteed to
> make sense in the address space of the *destination* qemu process...
> 
> I guess the only reason this isn't a problem is that nobody currently
> attempts to access fw_cfg after a migration ? :)

I thought once fw_cfg was rebased to an (anonymous?) RAMBlock footing,
migration became a non-issue. But, admittedly, I haven't thought much
about it.

Laszlo

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

[Gerd Hoffmann]

  Hi,

> I guess the only reason this isn't a problem is that nobody currently
> attempts to access fw_cfg after a migration ? :)

Accessing fw_cfg after migration is fine.  Problem is this ...

  (1) read directory
  (2) migrate
  (3) read file

... in case the file ordering happens to be different on the destination
host due to initialization order changes.

So, sorting entries (and the index assigned too) should fix this, right?
That looks easiest to me.

cheers,
  Gerd

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

[Paolo Bonzini]

On 02/06/2015 09:11, Gerd Hoffmann wrote:
> Accessing fw_cfg after migration is fine.  Problem is this ...
> 
>   (1) read directory
>   (2) migrate
>   (3) read file
> 
> ... in case the file ordering happens to be different on the destination
> host due to initialization order changes.
> 
> So, sorting entries (and the index assigned too) should fix this, right?
> That looks easiest to me.

Agreed.

Paolo

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

[Michael S. Tsirkin]

On Tue, Jun 02, 2015 at 09:11:14AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > I guess the only reason this isn't a problem is that nobody currently
> > attempts to access fw_cfg after a migration ? :)
> 
> Accessing fw_cfg after migration is fine.  Problem is this ...
> 
>   (1) read directory
>   (2) migrate
>   (3) read file
> 
> ... in case the file ordering happens to be different on the destination
> host due to initialization order changes.
> 
> So, sorting entries (and the index assigned too) should fix this, right?
> That looks easiest to me.
> 
> cheers,
>   Gerd

Right. The only issue is we are introducing a breakage even if
there was otherwise no reason for it otherwise.
I'll let you make the decision.

-- 
MST

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

[Gabriel L. Somlo]

On Tue, Jun 02, 2015 at 09:11:14AM +0200, Gerd Hoffmann wrote:
> > I guess the only reason this isn't a problem is that nobody currently
> > attempts to access fw_cfg after a migration ? :)
> 
> Accessing fw_cfg after migration is fine.  Problem is this ...
> 
>   (1) read directory
>   (2) migrate
>   (3) read file
> 
> ... in case the file ordering happens to be different on the destination
> host due to initialization order changes.

I get this part :) Right now, most of the fw_cfg files get added
before command line options are processed, but some (like etc/tpm/log)
get added after. When all is said and done, *at least* the
user-provided blobs may be ordered differently when a saved guest
state is loaded.
 
> So, sorting entries (and the index assigned too) should fix this, right?
> That looks easiest to me.

Presumably, anything happening before (and after) user-provided blobs
are inserted will continue happening in the same order everywhere.
So we really only have to sort the user provided blobs, so that if
the same *set* is provided on both ends of the migration, things would
work out OK.

If a *different* set of blobs is specified on the migration origin vs.
the migration destination, we lose by default and worrying about it is
pointless -- did I get that right ? If yes, figuring out there's a
mismatch and declaring the migration failed would be a nice extra
feature, wonder if that's possible...

Thanks,
--Gabriel

PS.
At some point earlier in the thread I *thought* adding the actual blob
data to the state being migrated would help, but (and I would like to
have that assertion sanity checked, please) that would be crazy, since
a lot (most?) of the FwCfgEntry 'data' pointers are actually locations
within the qemu process on the host machine, and therefore subject to
the local host memory allocator, and wouldn't "translate" between the
migration source and destination.

Having both the source and destination dereference their own local
pointers based on 'cur_entry' and 'cur_offset' (the only state being
migrated, and rightfully so) works only if 'entries' and 'files'
have the same order.

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

[Gerd Hoffmann]

  Hi,

> > So, sorting entries (and the index assigned too) should fix this, right?
> > That looks easiest to me.
> 
> Presumably, anything happening before (and after) user-provided blobs
> are inserted will continue happening in the same order everywhere.

Which might change in the future though, in case the initialization
order changes for some reason.  It's not a problem we have at the
moment, so this stuff isn't urgent.  But we might face it in the future.

> So we really only have to sort the user provided blobs, so that if
> the same *set* is provided on both ends of the migration, things would
> work out OK.

I would simply sort everything (and do it for new machine types only,
for backward compatibility reasons).  Sorting user-provided blobs only
adds complexity for no reason.

> If a *different* set of blobs is specified on the migration origin vs.
> the migration destination, we lose by default and worrying about it is
> pointless -- did I get that right ?

Yes.  For migration to succeed you have to supply the same configuration
on both ends.  That includes user-provided fw_cfg blobs of course.

cheers,
  Gerd

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

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 09:21:45AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > So, sorting entries (and the index assigned too) should fix this, right?
> > > That looks easiest to me.
> > 
> > Presumably, anything happening before (and after) user-provided blobs
> > are inserted will continue happening in the same order everywhere.
> 
> Which might change in the future though, in case the initialization
> order changes for some reason.  It's not a problem we have at the
> moment, so this stuff isn't urgent.  But we might face it in the future.
> 
> > So we really only have to sort the user provided blobs, so that if
> > the same *set* is provided on both ends of the migration, things would
> > work out OK.
> 
> I would simply sort everything (and do it for new machine types only,
> for backward compatibility reasons).  Sorting user-provided blobs only
> adds complexity for no reason.
> 
> > If a *different* set of blobs is specified on the migration origin vs.
> > the migration destination, we lose by default and worrying about it is
> > pointless -- did I get that right ?
> 
> Yes.  For migration to succeed you have to supply the same configuration
> on both ends.  That includes user-provided fw_cfg blobs of course.
> 
> cheers,
>   Gerd
> 

Do we want this for all machine types, or only for new ones?

-- 
MST

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

[Gerd Hoffmann]

> > I would simply sort everything (and do it for new machine types only,
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > for backward compatibility reasons).  Sorting user-provided blobs only
> > adds complexity for no reason.

> Do we want this for all machine types, or only for new ones?

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

[Paolo Bonzini]

On 08/06/2015 13:19, Gerd Hoffmann wrote:
>>> I would simply sort everything (and do it for new machine types only,
>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> for backward compatibility reasons).  Sorting user-provided blobs only
>>> adds complexity for no reason.
> 
>> Do we want this for all machine types, or only for new ones?

I think it is not backwards compatible: sorting names changes the fw_cfg
ids of the files, and hence the guest ABI.

Paolo

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

[Gabriel L. Somlo]

On Mon, Jun 08, 2015 at 01:44:18PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 13:19, Gerd Hoffmann wrote:
> >>> I would simply sort everything (and do it for new machine types only,
> >                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>> for backward compatibility reasons).  Sorting user-provided blobs only
> >>> adds complexity for no reason.
> > 
> >> Do we want this for all machine types, or only for new ones?
> 
> I think it is not backwards compatible: sorting names changes the fw_cfg
> ids of the files, and hence the guest ABI.

I'm wonderingjust when exactly to start sorting items in fw_cfg ? 

Different machine types insert different blobs at various points
during their initialization (and possibly on-demand during hot-plug
events, I'm not 100% sure).

So when exactly can we know that "we now have everything, it's time to
start sorting" ?

I'm also assuming this applies to named (file) entries only, and the
unnamed entries (with "indices" in the "table" < FW_CFG_FILE_DIR) will
not have to be sorted.

Thanks,
--Gabriel

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

[Paolo Bonzini]

On 08/06/2015 14:23, Gabriel L. Somlo wrote:
> I'm wonderingjust when exactly to start sorting items in fw_cfg ? 
> 
> Different machine types insert different blobs at various points
> during their initialization (and possibly on-demand during hot-plug
> events, I'm not 100% sure).

Hotplug may modify contents (I don't think it does), but it shouldn't
add files.

> So when exactly can we know that "we now have everything, it's time to
> start sorting" ?

There is a machine_ready callback.

> I'm also assuming this applies to named (file) entries only, and the
> unnamed entries (with "indices" in the "table" < FW_CFG_FILE_DIR) will
> not have to be sorted.

Yes.

Paolo

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

[Gerd Hoffmann]

 Hi,

> I'm wonderingjust when exactly to start sorting items in fw_cfg ? 

I'd suggest to sort as entries are added, i.e. just insert the new entry
at the correct place instead of tacking it to the end.  So the list is
always sorted.

> I'm also assuming this applies to named (file) entries only,

Yes, the other ones have a fixed index anyway.

cheers,
  Gerd

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

[Gabriel L. Somlo]

On Mon, Jun 08, 2015 at 02:33:45PM +0200, Gerd Hoffmann wrote:
>  Hi,
> 
> > I'm wonderingjust when exactly to start sorting items in fw_cfg ? 
> 
> I'd suggest to sort as entries are added, i.e. just insert the new entry
> at the correct place instead of tacking it to the end.  So the list is
> always sorted.

I was thinking that too, but since these are all arrays (not linked
lists), I'm a bit put off by the idea of having to shift everything
that's already in there by one position upon each "sorted insert".
Not to mention updating the "select" field of each FWCfgFile entry in
the directory (and maybe sorting/shifting the FWCfgFiles directory
itself) :(

Combined with "only do this on machine versions 2.4 or later", maybe
just sorting everything during the machine_ready callback would be
less hairy ?

I'll be on vacation for one week, so meditating on this in the back
of my mind might help come up with something... :)

Thanks,
--Gabriel

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

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 09:32:53AM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 08, 2015 at 02:33:45PM +0200, Gerd Hoffmann wrote:
> >  Hi,
> > 
> > > I'm wonderingjust when exactly to start sorting items in fw_cfg ? 
> > 
> > I'd suggest to sort as entries are added, i.e. just insert the new entry
> > at the correct place instead of tacking it to the end.  So the list is
> > always sorted.
> 
> I was thinking that too, but since these are all arrays (not linked
> lists), I'm a bit put off by the idea of having to shift everything
> that's already in there by one position upon each "sorted insert".
> Not to mention updating the "select" field of each FWCfgFile entry in
> the directory (and maybe sorting/shifting the FWCfgFiles directory
> itself) :(
> 
> Combined with "only do this on machine versions 2.4 or later", maybe
> just sorting everything during the machine_ready callback would be
> less hairy ?

Unfortunately we have a bunch of code adding fw cfg entries
after machine ready.

> I'll be on vacation for one week, so meditating on this in the back
> of my mind might help come up with something... :)
> 
> Thanks,
> --Gabriel