Re: [Qemu-devel] [PATCH] vl: move rom_load_all after machine init done

[Eric Auger <eric.auger@linaro.org>]

ping

Do you think that change is sensible? Since this takes place in vl.c I
am quite scared but with your experience you may know how much this can
be wrong.

Best Regards

Eric

On 06/16/2015 06:07 PM, Eric Auger wrote:
> On ARM, commit ac9d32e39664e060cd1b538ff190980d57ad69e4 postponed the
> memory preparation for boot until the machine init done notifier. This
> has for consequence to insert ROM at machine init done time.
> 
> However the rom_load_all function stayed called before the ROM are
> inserted. As a consequence the rom_load_all function does not do
> everything it is expected to do, on ARM.
> 
> It currently registers the ROM reset notifier but does not iterate through
> the registered ROM list. the isrom field is not set properly. This latter
> is used to report info in the monitor and also to decide whether the
> rom->data can be freed on ROM reset notifier.
> 
> To fix that regression the patch moves the rom_load_all call after
> machine init done. We also take the opportunity to rename the rom_load_all
> function into rom_check_and_resgister_reset() and integrate the
> rom_load_done in it.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> Reported-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>  hw/core/loader.c    |  8 ++------
>  include/hw/loader.h |  3 +--
>  vl.c                | 11 ++++-------
>  3 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7ee675c..216eeeb 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -933,7 +933,7 @@ static void rom_reset(void *unused)
>      }
>  }
>  
> -int rom_load_all(void)
> +int rom_check_and_register_reset(void)
>  {
>      hwaddr addr = 0;
>      MemoryRegionSection section;
> @@ -957,12 +957,8 @@ int rom_load_all(void)
>          memory_region_unref(section.mr);
>      }
>      qemu_register_reset(rom_reset, NULL);
> -    return 0;
> -}
> -
> -void rom_load_done(void)
> -{
>      roms_loaded = 1;
> +    return 0;
>  }
>  
>  void rom_set_fw(FWCfgState *f)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 485ff8f..f7b43ab 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -75,8 +75,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                             void *callback_opaque);
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
>                          size_t romsize, hwaddr addr);
> -int rom_load_all(void);
> -void rom_load_done(void);
> +int rom_check_and_register_reset(void);
>  void rom_set_fw(FWCfgState *f);
>  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>  void *rom_ptr(hwaddr addr);
> diff --git a/vl.c b/vl.c
> index 9542095..4f12957 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4419,18 +4419,15 @@ int main(int argc, char **argv, char **envp)
>  
>      qdev_machine_creation_done();
>  
> -    if (rom_load_all() != 0) {
> -        fprintf(stderr, "rom loading failed\n");
> -        exit(1);
> -    }
> -
>      /* TODO: once all bus devices are qdevified, this should be done
>       * when bus is created by qdev.c */
>      qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();
>  
> -    /* Done notifiers can load ROMs */
> -    rom_load_done();
> +    if (rom_check_and_register_reset() != 0) {
> +        fprintf(stderr, "rom check and register reset failed\n");
> +        exit(1);
> +    }
>  
>      qemu_system_reset(VMRESET_SILENT);
>      if (loadvm) {
>