Re: [Qemu-devel] [PATCH v2 1/2] pc: avoid duplicate names for ROM MRs

[Paolo Bonzini]

Il 06/03/2014 17:23, Michael S. Tsirkin ha scritto:
> Changes from v1:
>     correctly set option flag to false for fixed files
>
>  include/hw/loader.h |  6 ++++--
>  hw/core/loader.c    | 10 ++++++----
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 91b0122..f63e455 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -44,9 +44,11 @@ void pstrcpy_targphys(const char *name,
>                        const char *source);
>
>  extern bool rom_file_in_ram;
> +extern bool option_rom_in_ram;

I'd rather change these to

fixed_rom_has_mr
option_rom_has_mr

and then you can use below

option ? option_rom_has_mr : fixed_rom_has_mr

For 1.7 you have both of them to true.  For 2.0 only fixed_rom_has_mr.

>  int rom_add_file(const char *file, const char *fw_dir,
> -                 hwaddr addr, int32_t bootindex);
> +                 hwaddr addr, int32_t bootindex,
> +                 bool option);
>  void *rom_add_blob(const char *name, const void *blob, size_t len,
>                     hwaddr addr, const char *fw_file_name,
>                     FWCfgReadCallback fw_callback, void *callback_opaque);
> @@ -60,7 +62,7 @@ void *rom_ptr(hwaddr addr);
>  void do_info_roms(Monitor *mon, const QDict *qdict);
>
>  #define rom_add_file_fixed(_f, _a, _i)          \
> -    rom_add_file(_f, NULL, _a, _i)
> +    rom_add_file(_f, NULL, _a, _i, false)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>      rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0634bee..a19b158 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -54,6 +54,7 @@
>
>  #include <zlib.h>
>
> +bool option_rom_in_ram = false;
>  bool rom_file_in_ram = true;
>
>  static int roms_loaded;
> @@ -624,7 +625,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char 
> *name)
>  }
>
>  int rom_add_file(const char *file, const char *fw_dir,
> -                 hwaddr addr, int32_t bootindex)
> +                 hwaddr addr, int32_t bootindex,
> +                 bool option)
>  {
>      Rom *rom;
>      int rc, fd = -1;
> @@ -676,7 +678,7 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   basename);
>          snprintf(devpath, sizeof(devpath), "/address@hidden", fw_file_name);
>
> -        if (rom_file_in_ram) {
> +        if ((!option || option_rom_in_ram) && rom_file_in_ram) {
>              data = rom_set_mr(rom, OBJECT(fw_cfg), devpath);
>          } else {
>              data = rom->data;
> @@ -755,12 +757,12 @@ int rom_add_elf_program(const char *name, void *data, 
> size_t datasize,
>
>  int rom_add_vga(const char *file)
>  {
> -    return rom_add_file(file, "vgaroms", 0, -1);
> +    return rom_add_file(file, "vgaroms", 0, -1, true);
>  }
>
>  int rom_add_option(const char *file, int32_t bootindex)
>  {
> -    return rom_add_file(file, "genroms", 0, bootindex);
> +    return rom_add_file(file, "genroms", 0, bootindex, true);
>  }
>
>  static void rom_reset(void *unused)

I don't really like the difference between option ROM and fixed ROM; I 
think what really matters is more the difference between user file (user 
is responsible for keeping it the same across migration) and system file 
(whose old contents are migrated automatically).

But you convinced me that, because the mechanism was introduced for 
fixed ROMs (kvmvapic and the ISA VGABIOS), this patch doesn't really 
lose anything interesting.

Paolo