From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z6yDd-0007T6-H9 for qemu-devel@nongnu.org; Mon, 22 Jun 2015 05:40:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z6yDZ-0008KC-GK for qemu-devel@nongnu.org; Mon, 22 Jun 2015 05:40:45 -0400 Received: from mail-wi0-f180.google.com ([209.85.212.180]:37356) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z6xzi-0008Vi-9s for qemu-devel@nongnu.org; Mon, 22 Jun 2015 05:26:22 -0400 Received: by wicgi11 with SMTP id gi11so69021638wic.0 for ; Mon, 22 Jun 2015 02:26:21 -0700 (PDT) Message-ID: <5587D4AC.1050203@linaro.org> Date: Mon, 22 Jun 2015 11:26:04 +0200 From: Eric Auger MIME-Version: 1.0 References: <1434470874-22573-1-git-send-email-eric.auger@linaro.org> In-Reply-To: <1434470874-22573-1-git-send-email-eric.auger@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] vl: move rom_load_all after machine init done List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: eric.auger@st.com, qemu-devel@nongnu.org, peter.maydell@linaro.org, peter.crosthwaite@xilinx.com, pbonzini@redhat.com Cc: christoffer.dall@linaro.org, patches@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 > Reported-by: Peter Crosthwaite > --- > 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) { >