| Message ID | 1457974531-8768-1-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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 >
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.
> 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
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?
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? >
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? > >
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? >>>
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
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.
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? > >>>
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? >>>
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? > >>>
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
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
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 --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;