diff mbox

[v2] Sort the fw_cfg file list

Message ID 1457974531-8768-1-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard March 14, 2016, 4:55 p.m. UTC
From: Gerd Hoffmann <kraxel@redhat.com>

Entries are inserted at the correct place instead of being
appended to the end in case sorting is enabled.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Added a machine type handling for compatibility.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---

Don't add a new machine type in this version, just use the 2.6 one.

 hw/i386/pc_piix.c   |  1 +
 hw/i386/pc_q35.c    |  1 +
 hw/nvram/fw_cfg.c   | 32 ++++++++++++++++++++++++++------
 include/hw/boards.h |  3 ++-
 4 files changed, 30 insertions(+), 7 deletions(-)

Comments

Michael S. Tsirkin March 15, 2016, 6:57 a.m. UTC | #1
On Mon, Mar 14, 2016 at 11:55:31PM +0700, minyard@acm.org wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Entries are inserted at the correct place instead of being
> appended to the end in case sorting is enabled.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Added a machine type handling for compatibility.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
> 
> Don't add a new machine type in this version, just use the 2.6 one.

Unfortunately this patch won't help any, as your next patch reorders fw
cfg files which will affect the old machine types.

What is needed is not dont_sort_fw_cfgs, instead we need to create a
list of existing fw cfg files in the order they appear currently, and
sort them according to this predefined order (at least for old machine
types).


>  hw/i386/pc_piix.c   |  1 +
>  hw/i386/pc_q35.c    |  1 +
>  hw/nvram/fw_cfg.c   | 32 ++++++++++++++++++++++++++------
>  include/hw/boards.h |  3 ++-
>  4 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 6f8c2cd..6c68e63 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -429,6 +429,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->save_tsc_khz = false;
> +    m->dont_sort_fw_cfgs = 1;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 208a224..417fb57 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -354,6 +354,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m)
>      pc_q35_2_6_machine_options(m);
>      m->alias = NULL;
>      pcmc->save_tsc_khz = false;
> +    m->dont_sort_fw_cfgs = 1;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
>  }
>  
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 79c5742..10dab77 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -28,6 +28,7 @@
>  #include "hw/isa/isa.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/sysbus.h"
> +#include "hw/boards.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
> @@ -669,8 +670,9 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
>                                void *data, size_t len)
>  {
> -    int i, index;
> +    int i, index, count;
>      size_t dsize;
> +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>  
>      if (!s->files) {
>          dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS;
> @@ -678,13 +680,31 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>      }
>  
> -    index = be32_to_cpu(s->files->count);
> -    assert(index < FW_CFG_FILE_SLOTS);
> +    count = be32_to_cpu(s->files->count);
> +    assert(count < FW_CFG_FILE_SLOTS);
> +
> +    index = count;
> +    if (!mc->dont_sort_fw_cfgs) {
> +        while (index > 0 && strcmp(filename, s->files->f[index-1].name) < 0) {
> +            s->files->f[index] =
> +                s->files->f[index - 1];
> +            s->files->f[index].select =
> +                cpu_to_be16(FW_CFG_FILE_FIRST + index);
> +            s->entries[0][FW_CFG_FILE_FIRST + index] =
> +                s->entries[0][FW_CFG_FILE_FIRST + index - 1];
> +            index--;
> +        }
> +        memset(&s->files->f[index],
> +               0, sizeof(FWCfgFile));
> +        memset(&s->entries[0][FW_CFG_FILE_FIRST + index],
> +               0, sizeof(FWCfgEntry));
> +    }
>  
>      pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
>              filename);
> -    for (i = 0; i < index; i++) {
> -        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
> +    for (i = 0; i <= count; i++) {
> +        if (i != index &&
> +            strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
>              error_report("duplicate fw_cfg file name: %s",
>                           s->files->f[index].name);
>              exit(1);
> @@ -698,7 +718,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
>      trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
>  
> -    s->files->count = cpu_to_be32(index+1);
> +    s->files->count = cpu_to_be32(count+1);
>  }
>  
>  void fw_cfg_add_file(FWCfgState *s,  const char *filename,
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..f8d99d2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -85,7 +85,8 @@ struct MachineClass {
>          no_sdcard:1,
>          has_dynamic_sysbus:1,
>          no_tco:1,
> -        pci_allow_0_address:1;
> +        pci_allow_0_address:1,
> +        dont_sort_fw_cfgs:1;
>      int is_default;
>      const char *default_machine_opts;
>      const char *default_boot_order;
> -- 
> 2.5.0
Gerd Hoffmann March 15, 2016, 7:04 a.m. UTC | #2
Hi,

> > Don't add a new machine type in this version, just use the 2.6 one.
> 
> Unfortunately this patch won't help any, as your next patch reorders fw
> cfg files which will affect the old machine types.

We can make that depend on dont_sort_fw_cfgs too, to keep things as-is
on old machine types.   Which probably implies only new machine types
will see the new smbios entries.

cheers,
  Gerd
Michael S. Tsirkin March 15, 2016, 7:17 a.m. UTC | #3
On Tue, Mar 15, 2016 at 08:04:48AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Don't add a new machine type in this version, just use the 2.6 one.
> > 
> > Unfortunately this patch won't help any, as your next patch reorders fw
> > cfg files which will affect the old machine types.
> 
> We can make that depend on dont_sort_fw_cfgs too, to keep things as-is
> on old machine types.

Yes, except the name is a bit off then. It's more "legacy_fw_cfg_order"
than "dont_sort_fw_cfgs".

> Which probably implies only new machine types
> will see the new smbios entries.
> 
> cheers,
>   Gerd

Not really I think.
If there's a new device triggering a new smbios entry, we don't care how
it's sorted since by then there's no question of cross-version migration
with old QEMU.
Gerd Hoffmann March 15, 2016, 7:34 a.m. UTC | #4
On Di, 2016-03-15 at 09:17 +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 08:04:48AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > > Don't add a new machine type in this version, just use the 2.6 one.
> > > 
> > > Unfortunately this patch won't help any, as your next patch reorders fw
> > > cfg files which will affect the old machine types.
> > 
> > We can make that depend on dont_sort_fw_cfgs too, to keep things as-is
> > on old machine types.
> 
> Yes, except the name is a bit off then. It's more "legacy_fw_cfg_order"
> than "dont_sort_fw_cfgs".
> 
> > Which probably implies only new machine types
> > will see the new smbios entries.
> > 
> > cheers,
> >   Gerd
> 
> Not really I think.
> If there's a new device triggering a new smbios entry, we don't care how
> it's sorted since by then there's no question of cross-version migration
> with old QEMU.

As I understand things the smbios fw_cfg file must be created later to
make new entries (created by devices) are actually visible.  So we can
make that initialization order change depend on dont_sort_fw_cfgs (or
legacy_fw_cfg_order) too.  That way we maintain the ordering for old
machine types.  But old machine types then will also not see the entries
added by devices.  Which isn't a problem I think, old qemu versions
don't have support for that device in the first place ...

Corey, can you please send all this in a patch series?  This should make
those dependencies more clear ...

thanks,
  Gerd

>
Michael S. Tsirkin March 15, 2016, 7:45 a.m. UTC | #5
On Tue, Mar 15, 2016 at 08:34:09AM +0100, Gerd Hoffmann wrote:
> On Di, 2016-03-15 at 09:17 +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 15, 2016 at 08:04:48AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > Don't add a new machine type in this version, just use the 2.6 one.
> > > > 
> > > > Unfortunately this patch won't help any, as your next patch reorders fw
> > > > cfg files which will affect the old machine types.
> > > 
> > > We can make that depend on dont_sort_fw_cfgs too, to keep things as-is
> > > on old machine types.
> > 
> > Yes, except the name is a bit off then. It's more "legacy_fw_cfg_order"
> > than "dont_sort_fw_cfgs".
> > 
> > > Which probably implies only new machine types
> > > will see the new smbios entries.
> > > 
> > > cheers,
> > >   Gerd
> > 
> > Not really I think.
> > If there's a new device triggering a new smbios entry, we don't care how
> > it's sorted since by then there's no question of cross-version migration
> > with old QEMU.
> 
> As I understand things the smbios fw_cfg file must be created later to
> make new entries (created by devices) are actually visible.  So we can
> make that initialization order change depend on dont_sort_fw_cfgs (or
> legacy_fw_cfg_order) too.  That way we maintain the ordering for old
> machine types. 

Yes.

> But old machine types then will also not see the entries
> added by devices.  Which isn't a problem I think, old qemu versions
> don't have support for that device in the first place ...

Depends on how you code it up.  We have a list, we look each file
there and sort accordingly.  Fine.
New devices will not be on this list, I guess you can just ignore them
and guests will not see them. OK but I think it is better to make old
machine types see them.  It's trivially easy and more user-friendly.
Simply sort anything that is not on the list alphabetically and
add it at the end.

> Corey, can you please send all this in a patch series?  This should make
> those dependencies more clear ...
> 
> thanks,
>   Gerd
> 
> > 

So like this:

fw_cfg_find(a) {
    for (index = 0; index < fw_cfg_legacy_array_size; ++index)
        if (!strcmp(a, ...))
            break;
    return index;
}

fw_cfg_cmp(a, b) {
    in cmp;
    if (legacy_fw_cfg_order) {
        int list1 = find(a);
        int list2 = find(b);

        if (list1 < list2)
            return -1;
        if (list1 > list2)
            return 1;
    }

    return strcmp(a, b);
}


This way anything not on list is sorted,
with !legacy_fw_cfg_order list is assumed empty.
Gerd Hoffmann March 15, 2016, 8:45 a.m. UTC | #6
> Depends on how you code it up.  We have a list, we look each file
> there and sort accordingly.  Fine.
> New devices will not be on this list, I guess you can just ignore them
> and guests will not see them. OK but I think it is better to make old
> machine types see them.

Not a new fw_cfg file.

It's existing smbios file which gets new records added by a new device.
So when initializing it early (old order) it doesn't (yet) contain the
new records.  When initializing it late it has them, but also has a
different place in the fw_cfg directory.

So old machine types initialize smbios early (for compatibility).

New machine types initialize smbios late (so guests see the new
records).

While mucking with the file ordering anyway:  Good opportunity to make
new machine types also sort the fw_cfg directory entries, so they get a
fixed order independent from the order they are created, and we will not
face this problem again.

cheers,
  Gerd
Michael S. Tsirkin March 15, 2016, 9:37 a.m. UTC | #7
On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
> > Depends on how you code it up.  We have a list, we look each file
> > there and sort accordingly.  Fine.
> > New devices will not be on this list, I guess you can just ignore them
> > and guests will not see them. OK but I think it is better to make old
> > machine types see them.
> 
> Not a new fw_cfg file.
> 
> It's existing smbios file which gets new records added by a new device.
> So when initializing it early (old order) it doesn't (yet) contain the
> new records.  When initializing it late it has them, but also has a
> different place in the fw_cfg directory.
> 
> So old machine types initialize smbios early (for compatibility).

I see. So in this model, we'd have to somehow keep track of
the old initialization order forever, and
add hacks whenever we change it.
IMHO That would just be too hard to maintain. I have an alternative
proposal.



> New machine types initialize smbios late (so guests see the new
> records).


So here is what I propose instead:

- always initialize it late
- sort late, a machine done, not when inserting entries
- figure out what the order of existing entries is currently,
  and fill an array listing them in this order.
  for old machine types, insert the existing entries
  in this specific order by using a sorting function:

qsort(....., fw_cfg_cmp);

where:

fw_cfg_find(a) {
    for (index = 0; index < fw_cfg_legacy_array_size; ++index)
        if (!strcmp(a, ...))
            break;
    return index;
}

fw_cfg_cmp(a, b) {
    in cmp;
    if (legacy_fw_cfg_order) {
        int list1 = find(a);
        int list2 = find(b);

        if (list1 < list2)
            return -1;
        if (list1 > list2)
            return 1;
    }

    return strcmp(a, b);
}







> While mucking with the file ordering anyway:  Good opportunity to make
> new machine types also sort the fw_cfg directory entries, so they get a
> fixed order independent from the order they are created, and we will not
> face this problem again.
> 
> cheers,
>   Gerd

What exactly do you mean by directory entries here?
Corey Minyard March 15, 2016, 12:38 p.m. UTC | #8
On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
>>> Depends on how you code it up.  We have a list, we look each file
>>> there and sort accordingly.  Fine.
>>> New devices will not be on this list, I guess you can just ignore them
>>> and guests will not see them. OK but I think it is better to make old
>>> machine types see them.
>> Not a new fw_cfg file.
>>
>> It's existing smbios file which gets new records added by a new device.
>> So when initializing it early (old order) it doesn't (yet) contain the
>> new records.  When initializing it late it has them, but also has a
>> different place in the fw_cfg directory.
>>
>> So old machine types initialize smbios early (for compatibility).
> I see. So in this model, we'd have to somehow keep track of
> the old initialization order forever, and
> add hacks whenever we change it.
> IMHO That would just be too hard to maintain. I have an alternative
> proposal.
>
>
>
>> New machine types initialize smbios late (so guests see the new
>> records).
>
> So here is what I propose instead:
>
> - always initialize it late
> - sort late, a machine done, not when inserting entries
> - figure out what the order of existing entries is currently,
>    and fill an array listing them in this order.
>    for old machine types, insert the existing entries
>    in this specific order by using a sorting function:
>
> qsort(....., fw_cfg_cmp);
>
> where:
>
> fw_cfg_find(a) {
>      for (index = 0; index < fw_cfg_legacy_array_size; ++index)
>          if (!strcmp(a, ...))
>              break;
>      return index;
> }
>
> fw_cfg_cmp(a, b) {
>      in cmp;
>      if (legacy_fw_cfg_order) {
>          int list1 = find(a);
>          int list2 = find(b);
>
>          if (list1 < list2)
>              return -1;
>          if (list1 > list2)
>              return 1;
>      }
>
>      return strcmp(a, b);
> }

Last night I had an idea something like this.  Sorting by filename
may not work because the user may pass in the file from the
command line and you wouldn't be able to track the file name that
way.

Instead, you could add a "legacy_order" parameter to the fw_cfg_add
functions.  Then figure out the current order add the numeric
order to each call.  Then sort by the numeric order.  As long as you
don't reorder things with the same numeric value I think that
would work and be fairly simple to implement.  New calls could
pass in NO_FW_CFG_LEGACY_ORDER or something like that and
be pasted onto the end in legacy mode.

-corey

>
>
>
>
>
>
>> While mucking with the file ordering anyway:  Good opportunity to make
>> new machine types also sort the fw_cfg directory entries, so they get a
>> fixed order independent from the order they are created, and we will not
>> face this problem again.
>>
>> cheers,
>>    Gerd
> What exactly do you mean by directory entries here?
>
Michael S. Tsirkin March 15, 2016, 12:45 p.m. UTC | #9
On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
> On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
> >On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
> >>>Depends on how you code it up.  We have a list, we look each file
> >>>there and sort accordingly.  Fine.
> >>>New devices will not be on this list, I guess you can just ignore them
> >>>and guests will not see them. OK but I think it is better to make old
> >>>machine types see them.
> >>Not a new fw_cfg file.
> >>
> >>It's existing smbios file which gets new records added by a new device.
> >>So when initializing it early (old order) it doesn't (yet) contain the
> >>new records.  When initializing it late it has them, but also has a
> >>different place in the fw_cfg directory.
> >>
> >>So old machine types initialize smbios early (for compatibility).
> >I see. So in this model, we'd have to somehow keep track of
> >the old initialization order forever, and
> >add hacks whenever we change it.
> >IMHO That would just be too hard to maintain. I have an alternative
> >proposal.
> >
> >
> >
> >>New machine types initialize smbios late (so guests see the new
> >>records).
> >
> >So here is what I propose instead:
> >
> >- always initialize it late
> >- sort late, a machine done, not when inserting entries
> >- figure out what the order of existing entries is currently,
> >   and fill an array listing them in this order.
> >   for old machine types, insert the existing entries
> >   in this specific order by using a sorting function:
> >
> >qsort(....., fw_cfg_cmp);
> >
> >where:
> >
> >fw_cfg_find(a) {
> >     for (index = 0; index < fw_cfg_legacy_array_size; ++index)
> >         if (!strcmp(a, ...))
> >             break;
> >     return index;
> >}
> >
> >fw_cfg_cmp(a, b) {
> >     in cmp;
> >     if (legacy_fw_cfg_order) {
> >         int list1 = find(a);
> >         int list2 = find(b);
> >
> >         if (list1 < list2)
> >             return -1;
> >         if (list1 > list2)
> >             return 1;
> >     }
> >
> >     return strcmp(a, b);
> >}
> 
> Last night I had an idea something like this.  Sorting by filename
> may not work because the user may pass in the file from the
> command line and you wouldn't be able to track the file name that
> way.

command line files must all have a consistent prefix,
so we can skip sorting them.
I'll need to look at the code - don't they already?
If not we IMHO absolutely must fix that before release
and give them consistent prefixes.

> Instead, you could add a "legacy_order" parameter to the fw_cfg_add
> functions.  Then figure out the current order add the numeric
> order to each call.  Then sort by the numeric order.  As long as you
> don't reorder things with the same numeric value I think that
> would work and be fairly simple to implement.  New calls could
> pass in NO_FW_CFG_LEGACY_ORDER or something like that and
> be pasted onto the end in legacy mode.
> 
> -corey

OK but it's a much larger change and less well contained.

> >
> >
> >
> >
> >
> >
> >>While mucking with the file ordering anyway:  Good opportunity to make
> >>new machine types also sort the fw_cfg directory entries, so they get a
> >>fixed order independent from the order they are created, and we will not
> >>face this problem again.
> >>
> >>cheers,
> >>   Gerd
> >What exactly do you mean by directory entries here?
> >
Corey Minyard March 15, 2016, 12:56 p.m. UTC | #10
On 03/15/2016 07:45 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
>> On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
>>> On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
>>>>> Depends on how you code it up.  We have a list, we look each file
>>>>> there and sort accordingly.  Fine.
>>>>> New devices will not be on this list, I guess you can just ignore them
>>>>> and guests will not see them. OK but I think it is better to make old
>>>>> machine types see them.
>>>> Not a new fw_cfg file.
>>>>
>>>> It's existing smbios file which gets new records added by a new device.
>>>> So when initializing it early (old order) it doesn't (yet) contain the
>>>> new records.  When initializing it late it has them, but also has a
>>>> different place in the fw_cfg directory.
>>>>
>>>> So old machine types initialize smbios early (for compatibility).
>>> I see. So in this model, we'd have to somehow keep track of
>>> the old initialization order forever, and
>>> add hacks whenever we change it.
>>> IMHO That would just be too hard to maintain. I have an alternative
>>> proposal.
>>>
>>>
>>>
>>>> New machine types initialize smbios late (so guests see the new
>>>> records).
>>> So here is what I propose instead:
>>>
>>> - always initialize it late
>>> - sort late, a machine done, not when inserting entries
>>> - figure out what the order of existing entries is currently,
>>>    and fill an array listing them in this order.
>>>    for old machine types, insert the existing entries
>>>    in this specific order by using a sorting function:
>>>
>>> qsort(....., fw_cfg_cmp);
>>>
>>> where:
>>>
>>> fw_cfg_find(a) {
>>>      for (index = 0; index < fw_cfg_legacy_array_size; ++index)
>>>          if (!strcmp(a, ...))
>>>              break;
>>>      return index;
>>> }
>>>
>>> fw_cfg_cmp(a, b) {
>>>      in cmp;
>>>      if (legacy_fw_cfg_order) {
>>>          int list1 = find(a);
>>>          int list2 = find(b);
>>>
>>>          if (list1 < list2)
>>>              return -1;
>>>          if (list1 > list2)
>>>              return 1;
>>>      }
>>>
>>>      return strcmp(a, b);
>>> }
>> Last night I had an idea something like this.  Sorting by filename
>> may not work because the user may pass in the file from the
>> command line and you wouldn't be able to track the file name that
>> way.
> command line files must all have a consistent prefix,
> so we can skip sorting them.
> I'll need to look at the code - don't they already?
> If not we IMHO absolutely must fix that before release
> and give them consistent prefixes.

You get a warning if it doesn't start with "opt/", but
that is not enforced.

>
>> Instead, you could add a "legacy_order" parameter to the fw_cfg_add
>> functions.  Then figure out the current order add the numeric
>> order to each call.  Then sort by the numeric order.  As long as you
>> don't reorder things with the same numeric value I think that
>> would work and be fairly simple to implement.  New calls could
>> pass in NO_FW_CFG_LEGACY_ORDER or something like that and
>> be pasted onto the end in legacy mode.
>>
>> -corey
> OK but it's a much larger change and less well contained.

True, it is less well contained.  If we want to assume the
command line entries always start with "opt/", then going
with a list of file names works and is simpler.  Otherwise
I don't see another way to preserve order.

-corey

>>>
>>>
>>>
>>>
>>>
>>>> While mucking with the file ordering anyway:  Good opportunity to make
>>>> new machine types also sort the fw_cfg directory entries, so they get a
>>>> fixed order independent from the order they are created, and we will not
>>>> face this problem again.
>>>>
>>>> cheers,
>>>>    Gerd
>>> What exactly do you mean by directory entries here?
>>>
Gerd Hoffmann March 15, 2016, 1:03 p.m. UTC | #11
Hi,

> So here is what I propose instead:
> 
> - always initialize it late
> - sort late, a machine done, not when inserting entries
> - figure out what the order of existing entries is currently,
>   and fill an array listing them in this order.
>   for old machine types, insert the existing entries
>   in this specific order by using a sorting function:

What is the point of using *two* ways to sort entries?

Sure, we can explicitly write down the current order of fw_cfg entries
and use that to sort them, so order stays as-is even if the
initialization order changes.  But when going that route I would do that
*instead* of the alphabetical ordering, for all machine types.

cheers,
  Gerd
Michael S. Tsirkin March 15, 2016, 1:19 p.m. UTC | #12
On Tue, Mar 15, 2016 at 02:03:47PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > So here is what I propose instead:
> > 
> > - always initialize it late
> > - sort late, a machine done, not when inserting entries
> > - figure out what the order of existing entries is currently,
> >   and fill an array listing them in this order.
> >   for old machine types, insert the existing entries
> >   in this specific order by using a sorting function:
> 
> What is the point of using *two* ways to sort entries?
> 
> Sure, we can explicitly write down the current order of fw_cfg entries
> and use that to sort them, so order stays as-is even if the
> initialization order changes.  But when going that route I would do that
> *instead* of the alphabetical ordering, for all machine types.
> 
> cheers,
>   Gerd

That's fine too.
The only question is for when we add new fw cfg entries:
do you want to add these to the list as well?

Sorting new entries after old ones will help
avoid future maintainance for this list:
we set it once and never change it.
Michael S. Tsirkin March 15, 2016, 2:43 p.m. UTC | #13
On Tue, Mar 15, 2016 at 07:56:36AM -0500, Corey Minyard wrote:
> On 03/15/2016 07:45 AM, Michael S. Tsirkin wrote:
> >On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
> >>On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
> >>>On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
> >>>>>Depends on how you code it up.  We have a list, we look each file
> >>>>>there and sort accordingly.  Fine.
> >>>>>New devices will not be on this list, I guess you can just ignore them
> >>>>>and guests will not see them. OK but I think it is better to make old
> >>>>>machine types see them.
> >>>>Not a new fw_cfg file.
> >>>>
> >>>>It's existing smbios file which gets new records added by a new device.
> >>>>So when initializing it early (old order) it doesn't (yet) contain the
> >>>>new records.  When initializing it late it has them, but also has a
> >>>>different place in the fw_cfg directory.
> >>>>
> >>>>So old machine types initialize smbios early (for compatibility).
> >>>I see. So in this model, we'd have to somehow keep track of
> >>>the old initialization order forever, and
> >>>add hacks whenever we change it.
> >>>IMHO That would just be too hard to maintain. I have an alternative
> >>>proposal.
> >>>
> >>>
> >>>
> >>>>New machine types initialize smbios late (so guests see the new
> >>>>records).
> >>>So here is what I propose instead:
> >>>
> >>>- always initialize it late
> >>>- sort late, a machine done, not when inserting entries
> >>>- figure out what the order of existing entries is currently,
> >>>   and fill an array listing them in this order.
> >>>   for old machine types, insert the existing entries
> >>>   in this specific order by using a sorting function:
> >>>
> >>>qsort(....., fw_cfg_cmp);
> >>>
> >>>where:
> >>>
> >>>fw_cfg_find(a) {
> >>>     for (index = 0; index < fw_cfg_legacy_array_size; ++index)
> >>>         if (!strcmp(a, ...))
> >>>             break;
> >>>     return index;
> >>>}
> >>>
> >>>fw_cfg_cmp(a, b) {
> >>>     in cmp;
> >>>     if (legacy_fw_cfg_order) {
> >>>         int list1 = find(a);
> >>>         int list2 = find(b);
> >>>
> >>>         if (list1 < list2)
> >>>             return -1;
> >>>         if (list1 > list2)
> >>>             return 1;
> >>>     }
> >>>
> >>>     return strcmp(a, b);
> >>>}
> >>Last night I had an idea something like this.  Sorting by filename
> >>may not work because the user may pass in the file from the
> >>command line and you wouldn't be able to track the file name that
> >>way.
> >command line files must all have a consistent prefix,
> >so we can skip sorting them.
> >I'll need to look at the code - don't they already?
> >If not we IMHO absolutely must fix that before release
> >and give them consistent prefixes.
> 
> You get a warning if it doesn't start with "opt/", but
> that is not enforced.
> 
> >
> >>Instead, you could add a "legacy_order" parameter to the fw_cfg_add
> >>functions.  Then figure out the current order add the numeric
> >>order to each call.  Then sort by the numeric order.  As long as you
> >>don't reorder things with the same numeric value I think that
> >>would work and be fairly simple to implement.  New calls could
> >>pass in NO_FW_CFG_LEGACY_ORDER or something like that and
> >>be pasted onto the end in legacy mode.
> >>
> >>-corey
> >OK but it's a much larger change and less well contained.
> 
> True, it is less well contained.  If we want to assume the
> command line entries always start with "opt/",

Basically what Gerd and Paolo say, yes it is safe to assume
this, even if QEMU does not exit, using a different prefix is user's
problem.


> then going
> with a list of file names works and is simpler.  Otherwise
> I don't see another way to preserve order.
> 
> -corey
> 
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>While mucking with the file ordering anyway:  Good opportunity to make
> >>>>new machine types also sort the fw_cfg directory entries, so they get a
> >>>>fixed order independent from the order they are created, and we will not
> >>>>face this problem again.
> >>>>
> >>>>cheers,
> >>>>   Gerd
> >>>What exactly do you mean by directory entries here?
> >>>
Corey Minyard March 15, 2016, 4:36 p.m. UTC | #14
On 03/15/2016 07:45 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
>> On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
>>> On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
>>>>> Depends on how you code it up.  We have a list, we look each file
>>>>> there and sort accordingly.  Fine.
>>>>> New devices will not be on this list, I guess you can just ignore them
>>>>> and guests will not see them. OK but I think it is better to make old
>>>>> machine types see them.
>>>> Not a new fw_cfg file.
>>>>
>>>> It's existing smbios file which gets new records added by a new device.
>>>> So when initializing it early (old order) it doesn't (yet) contain the
>>>> new records.  When initializing it late it has them, but also has a
>>>> different place in the fw_cfg directory.
>>>>
>>>> So old machine types initialize smbios early (for compatibility).
>>> I see. So in this model, we'd have to somehow keep track of
>>> the old initialization order forever, and
>>> add hacks whenever we change it.
>>> IMHO That would just be too hard to maintain. I have an alternative
>>> proposal.
>>>
>>>
>>>
>>>> New machine types initialize smbios late (so guests see the new
>>>> records).
>>> So here is what I propose instead:
>>>
>>> - always initialize it late
>>> - sort late, a machine done, not when inserting entries
>>> - figure out what the order of existing entries is currently,
>>>    and fill an array listing them in this order.
>>>    for old machine types, insert the existing entries
>>>    in this specific order by using a sorting function:
>>>
>>> qsort(....., fw_cfg_cmp);

I've hit a bit of a snag here.  For 0.11 and before, PCI option ROMs
were loaded via fw_cfg, not in the PCI ROM BAR.  This causes two
issues:

* The order depends on the device initialization order,
which I'm not sure is quantifiable.  I believe it depends on
how they are listed on the command line.

* Users can load their own romfile with their own name, which
means it can't be in the list.

Also, for the ISA VGA ROMs, their order will also depend on the
device list order.

Outside of that, I have an order of file names.

I think if I treat the device ROMs separately and handle them
in init order, and then stick that device list in the proper location,
that will work.  Does that sound reasonable?

Thanks,

-corey

>>> where:
>>>
>>> fw_cfg_find(a) {
>>>      for (index = 0; index < fw_cfg_legacy_array_size; ++index)
>>>          if (!strcmp(a, ...))
>>>              break;
>>>      return index;
>>> }
>>>
>>> fw_cfg_cmp(a, b) {
>>>      in cmp;
>>>      if (legacy_fw_cfg_order) {
>>>          int list1 = find(a);
>>>          int list2 = find(b);
>>>
>>>          if (list1 < list2)
>>>              return -1;
>>>          if (list1 > list2)
>>>              return 1;
>>>      }
>>>
>>>      return strcmp(a, b);
>>> }
>> Last night I had an idea something like this.  Sorting by filename
>> may not work because the user may pass in the file from the
>> command line and you wouldn't be able to track the file name that
>> way.
> command line files must all have a consistent prefix,
> so we can skip sorting them.
> I'll need to look at the code - don't they already?
> If not we IMHO absolutely must fix that before release
> and give them consistent prefixes.
>
>> Instead, you could add a "legacy_order" parameter to the fw_cfg_add
>> functions.  Then figure out the current order add the numeric
>> order to each call.  Then sort by the numeric order.  As long as you
>> don't reorder things with the same numeric value I think that
>> would work and be fairly simple to implement.  New calls could
>> pass in NO_FW_CFG_LEGACY_ORDER or something like that and
>> be pasted onto the end in legacy mode.
>>
>> -corey
> OK but it's a much larger change and less well contained.
>
>>>
>>>
>>>
>>>
>>>
>>>> While mucking with the file ordering anyway:  Good opportunity to make
>>>> new machine types also sort the fw_cfg directory entries, so they get a
>>>> fixed order independent from the order they are created, and we will not
>>>> face this problem again.
>>>>
>>>> cheers,
>>>>    Gerd
>>> What exactly do you mean by directory entries here?
>>>
Michael S. Tsirkin March 15, 2016, 5:01 p.m. UTC | #15
On Tue, Mar 15, 2016 at 11:36:39AM -0500, Corey Minyard wrote:
> On 03/15/2016 07:45 AM, Michael S. Tsirkin wrote:
> >On Tue, Mar 15, 2016 at 07:38:43AM -0500, Corey Minyard wrote:
> >>On 03/15/2016 04:37 AM, Michael S. Tsirkin wrote:
> >>>On Tue, Mar 15, 2016 at 09:45:22AM +0100, Gerd Hoffmann wrote:
> >>>>>Depends on how you code it up.  We have a list, we look each file
> >>>>>there and sort accordingly.  Fine.
> >>>>>New devices will not be on this list, I guess you can just ignore them
> >>>>>and guests will not see them. OK but I think it is better to make old
> >>>>>machine types see them.
> >>>>Not a new fw_cfg file.
> >>>>
> >>>>It's existing smbios file which gets new records added by a new device.
> >>>>So when initializing it early (old order) it doesn't (yet) contain the
> >>>>new records.  When initializing it late it has them, but also has a
> >>>>different place in the fw_cfg directory.
> >>>>
> >>>>So old machine types initialize smbios early (for compatibility).
> >>>I see. So in this model, we'd have to somehow keep track of
> >>>the old initialization order forever, and
> >>>add hacks whenever we change it.
> >>>IMHO That would just be too hard to maintain. I have an alternative
> >>>proposal.
> >>>
> >>>
> >>>
> >>>>New machine types initialize smbios late (so guests see the new
> >>>>records).
> >>>So here is what I propose instead:
> >>>
> >>>- always initialize it late
> >>>- sort late, a machine done, not when inserting entries
> >>>- figure out what the order of existing entries is currently,
> >>>   and fill an array listing them in this order.
> >>>   for old machine types, insert the existing entries
> >>>   in this specific order by using a sorting function:
> >>>
> >>>qsort(....., fw_cfg_cmp);
> 
> I've hit a bit of a snag here.  For 0.11 and before, PCI option ROMs
> were loaded via fw_cfg, not in the PCI ROM BAR.  This causes two
> issues:
> 
> * The order depends on the device initialization order,
> which I'm not sure is quantifiable.  I believe it depends on
> how they are listed on the command line.
> 
> * Users can load their own romfile with their own name, which
> means it can't be in the list.
> 
> Also, for the ISA VGA ROMs, their order will also depend on the
> device list order.
> 
> Outside of that, I have an order of file names.
> 
> I think if I treat the device ROMs separately and handle them
> in init order, and then stick that device list in the proper location,
> that will work.  Does that sound reasonable?
> 
> Thanks,
> 
> -corey

I think so. By the time this becomes an issue we might decide to
disable migration from 0.11

> >>>where:
> >>>
> >>>fw_cfg_find(a) {
> >>>     for (index = 0; index < fw_cfg_legacy_array_size; ++index)
> >>>         if (!strcmp(a, ...))
> >>>             break;
> >>>     return index;
> >>>}
> >>>
> >>>fw_cfg_cmp(a, b) {
> >>>     in cmp;
> >>>     if (legacy_fw_cfg_order) {
> >>>         int list1 = find(a);
> >>>         int list2 = find(b);
> >>>
> >>>         if (list1 < list2)
> >>>             return -1;
> >>>         if (list1 > list2)
> >>>             return 1;
> >>>     }
> >>>
> >>>     return strcmp(a, b);
> >>>}
> >>Last night I had an idea something like this.  Sorting by filename
> >>may not work because the user may pass in the file from the
> >>command line and you wouldn't be able to track the file name that
> >>way.
> >command line files must all have a consistent prefix,
> >so we can skip sorting them.
> >I'll need to look at the code - don't they already?
> >If not we IMHO absolutely must fix that before release
> >and give them consistent prefixes.
> >
> >>Instead, you could add a "legacy_order" parameter to the fw_cfg_add
> >>functions.  Then figure out the current order add the numeric
> >>order to each call.  Then sort by the numeric order.  As long as you
> >>don't reorder things with the same numeric value I think that
> >>would work and be fairly simple to implement.  New calls could
> >>pass in NO_FW_CFG_LEGACY_ORDER or something like that and
> >>be pasted onto the end in legacy mode.
> >>
> >>-corey
> >OK but it's a much larger change and less well contained.
> >
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>While mucking with the file ordering anyway:  Good opportunity to make
> >>>>new machine types also sort the fw_cfg directory entries, so they get a
> >>>>fixed order independent from the order they are created, and we will not
> >>>>face this problem again.
> >>>>
> >>>>cheers,
> >>>>   Gerd
> >>>What exactly do you mean by directory entries here?
> >>>
Corey Minyard March 16, 2016, 3:21 p.m. UTC | #16
On 03/15/2016 12:01 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 15, 2016 at 11:36:39AM -0500, Corey Minyard wrote:
>
>> I've hit a bit of a snag here.  For 0.11 and before, PCI option ROMs
>> were loaded via fw_cfg, not in the PCI ROM BAR.  This causes two
>> issues:
>>
>> * The order depends on the device initialization order,
>> which I'm not sure is quantifiable.  I believe it depends on
>> how they are listed on the command line.
>>
>> * Users can load their own romfile with their own name, which
>> means it can't be in the list.
>>
>> Also, for the ISA VGA ROMs, their order will also depend on the
>> device list order.
>>
>> Outside of that, I have an order of file names.
>>
>> I think if I treat the device ROMs separately and handle them
>> in init order, and then stick that device list in the proper location,
>> that will work.  Does that sound reasonable?
>>
>> Thanks,
>>
>> -corey
> I think so. By the time this becomes an issue we might decide to
> disable migration from 0.11
>

One more thing here.  I've realized that this is used for
non-x86, too, and I'm wondering how much those matter.
It's going to be really hard to figure out an order for these,
as different boards/arches use the same names and install
them in different orders.

For legacy mode, can I just put them in order for non-x86?
Is there an easy way to detect that you are running x86?

-corey
Paolo Bonzini March 16, 2016, 3:34 p.m. UTC | #17
On 16/03/2016 16:21, Corey Minyard wrote:
> One more thing here.  I've realized that this is used for
> non-x86, too, and I'm wondering how much those matter.
> It's going to be really hard to figure out an order for these,
> as different boards/arches use the same names and install
> them in different orders.
> 
> For legacy mode, can I just put them in order for non-x86?
> Is there an easy way to detect that you are running x86?

Of all boards that support fw-cfg, only x86 does versioning so they are
not a problem.

Paolo
Michael S. Tsirkin March 16, 2016, 3:37 p.m. UTC | #18
On Wed, Mar 16, 2016 at 10:21:07AM -0500, Corey Minyard wrote:
> On 03/15/2016 12:01 PM, Michael S. Tsirkin wrote:
> >On Tue, Mar 15, 2016 at 11:36:39AM -0500, Corey Minyard wrote:
> >
> >>I've hit a bit of a snag here.  For 0.11 and before, PCI option ROMs
> >>were loaded via fw_cfg, not in the PCI ROM BAR.  This causes two
> >>issues:
> >>
> >>* The order depends on the device initialization order,
> >>which I'm not sure is quantifiable.  I believe it depends on
> >>how they are listed on the command line.
> >>
> >>* Users can load their own romfile with their own name, which
> >>means it can't be in the list.
> >>
> >>Also, for the ISA VGA ROMs, their order will also depend on the
> >>device list order.
> >>
> >>Outside of that, I have an order of file names.
> >>
> >>I think if I treat the device ROMs separately and handle them
> >>in init order, and then stick that device list in the proper location,
> >>that will work.  Does that sound reasonable?
> >>
> >>Thanks,
> >>
> >>-corey
> >I think so. By the time this becomes an issue we might decide to
> >disable migration from 0.11
> >
> 
> One more thing here.  I've realized that this is used for
> non-x86, too, and I'm wondering how much those matter.
> It's going to be really hard to figure out an order for these,
> as different boards/arches use the same names and install
> them in different orders.
> 
> For legacy mode, can I just put them in order for non-x86?
> Is there an easy way to detect that you are running x86?
> 
> -corey

We only care about the order because of ability to migrate
guests between QEMU versions.
Only architectures that version their machine types support this.

AFAIK at this point this means x86 and s390.
diff mbox

Patch

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6f8c2cd..6c68e63 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -429,6 +429,7 @@  static void pc_i440fx_2_5_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->save_tsc_khz = false;
+    m->dont_sort_fw_cfgs = 1;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 208a224..417fb57 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -354,6 +354,7 @@  static void pc_q35_2_5_machine_options(MachineClass *m)
     pc_q35_2_6_machine_options(m);
     m->alias = NULL;
     pcmc->save_tsc_khz = false;
+    m->dont_sort_fw_cfgs = 1;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_5);
 }
 
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 79c5742..10dab77 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -28,6 +28,7 @@ 
 #include "hw/isa/isa.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
+#include "hw/boards.h"
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
@@ -669,8 +670,9 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len)
 {
-    int i, index;
+    int i, index, count;
     size_t dsize;
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
 
     if (!s->files) {
         dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS;
@@ -678,13 +680,31 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
         fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }
 
-    index = be32_to_cpu(s->files->count);
-    assert(index < FW_CFG_FILE_SLOTS);
+    count = be32_to_cpu(s->files->count);
+    assert(count < FW_CFG_FILE_SLOTS);
+
+    index = count;
+    if (!mc->dont_sort_fw_cfgs) {
+        while (index > 0 && strcmp(filename, s->files->f[index-1].name) < 0) {
+            s->files->f[index] =
+                s->files->f[index - 1];
+            s->files->f[index].select =
+                cpu_to_be16(FW_CFG_FILE_FIRST + index);
+            s->entries[0][FW_CFG_FILE_FIRST + index] =
+                s->entries[0][FW_CFG_FILE_FIRST + index - 1];
+            index--;
+        }
+        memset(&s->files->f[index],
+               0, sizeof(FWCfgFile));
+        memset(&s->entries[0][FW_CFG_FILE_FIRST + index],
+               0, sizeof(FWCfgEntry));
+    }
 
     pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
             filename);
-    for (i = 0; i < index; i++) {
-        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
+    for (i = 0; i <= count; i++) {
+        if (i != index &&
+            strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
             error_report("duplicate fw_cfg file name: %s",
                          s->files->f[index].name);
             exit(1);
@@ -698,7 +718,7 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
 
-    s->files->count = cpu_to_be32(index+1);
+    s->files->count = cpu_to_be32(count+1);
 }
 
 void fw_cfg_add_file(FWCfgState *s,  const char *filename,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..f8d99d2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -85,7 +85,8 @@  struct MachineClass {
         no_sdcard:1,
         has_dynamic_sysbus:1,
         no_tco:1,
-        pci_allow_0_address:1;
+        pci_allow_0_address:1,
+        dont_sort_fw_cfgs:1;
     int is_default;
     const char *default_machine_opts;
     const char *default_boot_order;