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

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

[Eric Auger]

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) {
-- 
1.8.3.2

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

[Eric Auger]

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) {
> 

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

[Paolo Bonzini]

On 22/06/2015 11:26, Eric Auger wrote:
> 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.

It seems safe because rom_load_all really doesn't load anything, it only
does an overlap check.  Is this right?

Is the bug that some overlapping ROMs are not detected?  The commit
message is not clear.

Paolo

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

[Eric Auger]

Hi Paolo,
On 06/22/2015 11:43 AM, Paolo Bonzini wrote:
> 
> 
> On 22/06/2015 11:26, Eric Auger wrote:
>> 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.
> 
> It seems safe because rom_load_all really doesn't load anything, it only
> does an overlap check.  Is this right?
it does the check + isrom field setting
> 
> Is the bug that some overlapping ROMs are not detected?  The commit
> message is not clear.
The regression is that the both overlap check and isrom setting are not
done since ROM are inserted in the roms list afterwards, at machine init
done time. The bug was not really observed yet I think.

Best Regards

Eric
> 
> Paolo
> 

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

[Paolo Bonzini]

On 22/06/2015 11:49, Eric Auger wrote:
>> > It seems safe because rom_load_all really doesn't load anything, it only
>> > does an overlap check.  Is this right?
> it does the check + isrom field setting
>> > 
>> > Is the bug that some overlapping ROMs are not detected?  The commit
>> > message is not clear.
> The regression is that the both overlap check and isrom setting are not
> done since ROM are inserted in the roms list afterwards, at machine init
> done time. The bug was not really observed yet I think.

isrom is just an optimization though, right?  What is it useful for?

Paolo

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

[Eric Auger]

On 06/22/2015 11:53 AM, Paolo Bonzini wrote:
> 
> 
> On 22/06/2015 11:49, Eric Auger wrote:
>>>> It seems safe because rom_load_all really doesn't load anything, it only
>>>> does an overlap check.  Is this right?
>> it does the check + isrom field setting
>>>>
>>>> Is the bug that some overlapping ROMs are not detected?  The commit
>>>> message is not clear.
>> The regression is that the both overlap check and isrom setting are not
>> done since ROM are inserted in the roms list afterwards, at machine init
>> done time. The bug was not really observed yet I think.
> 
> isrom is just an optimization though, right?  What is it useful for?
My understanding is it serves 2 purposes:

- report info in the monitor (hmp_info_roms)
- decide whether the rom->data can be freed on ROM reset notifier
(rom_reset).

Hope I didn't miss anything else.

Eric
> 
> Paolo
> 

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

[Eric Auger]

Hi Paolo, Peter,
On 06/22/2015 11:58 AM, Eric Auger wrote:
> On 06/22/2015 11:53 AM, Paolo Bonzini wrote:
>>
>>
>> On 22/06/2015 11:49, Eric Auger wrote:
>>>>> It seems safe because rom_load_all really doesn't load anything, it only
>>>>> does an overlap check.  Is this right?
>>> it does the check + isrom field setting
>>>>>
>>>>> Is the bug that some overlapping ROMs are not detected?  The commit
>>>>> message is not clear.
>>> The regression is that the both overlap check and isrom setting are not
>>> done since ROM are inserted in the roms list afterwards, at machine init
>>> done time. The bug was not really observed yet I think.
>>
>> isrom is just an optimization though, right?  What is it useful for?
> My understanding is it serves 2 purposes:
> 
> - report info in the monitor (hmp_info_roms)
> - decide whether the rom->data can be freed on ROM reset notifier
> (rom_reset).
> 
> Hope I didn't miss anything else.
> 
> Eric

What do we decide then about this regression on arm. Do we fix it in 2.4
or later?

Best Regards

Eric
>>
>> Paolo
>>
> 

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

[Paolo Bonzini]

On 07/07/2015 11:00, Eric Auger wrote:
> Hi Paolo, Peter,
> On 06/22/2015 11:58 AM, Eric Auger wrote:
>> On 06/22/2015 11:53 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 22/06/2015 11:49, Eric Auger wrote:
>>>>>> It seems safe because rom_load_all really doesn't load anything, it only
>>>>>> does an overlap check.  Is this right?
>>>> it does the check + isrom field setting
>>>>>>
>>>>>> Is the bug that some overlapping ROMs are not detected?  The commit
>>>>>> message is not clear.
>>>> The regression is that the both overlap check and isrom setting are not
>>>> done since ROM are inserted in the roms list afterwards, at machine init
>>>> done time. The bug was not really observed yet I think.
>>>
>>> isrom is just an optimization though, right?  What is it useful for?
>> My understanding is it serves 2 purposes:
>>
>> - report info in the monitor (hmp_info_roms)
>> - decide whether the rom->data can be freed on ROM reset notifier
>> (rom_reset).
>>
>> Hope I didn't miss anything else.
>>
>> Eric
> 
> What do we decide then about this regression on arm. Do we fix it in 2.4
> or later?

Yes, it should be fixed in 2.4.

Paolo

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

[Eric Auger]

On 07/07/2015 11:02 AM, Paolo Bonzini wrote:
> 
> 
> On 07/07/2015 11:00, Eric Auger wrote:
>> Hi Paolo, Peter,
>> On 06/22/2015 11:58 AM, Eric Auger wrote:
>>> On 06/22/2015 11:53 AM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 22/06/2015 11:49, Eric Auger wrote:
>>>>>>> It seems safe because rom_load_all really doesn't load anything, it only
>>>>>>> does an overlap check.  Is this right?
>>>>> it does the check + isrom field setting
>>>>>>>
>>>>>>> Is the bug that some overlapping ROMs are not detected?  The commit
>>>>>>> message is not clear.
>>>>> The regression is that the both overlap check and isrom setting are not
>>>>> done since ROM are inserted in the roms list afterwards, at machine init
>>>>> done time. The bug was not really observed yet I think.
>>>>
>>>> isrom is just an optimization though, right?  What is it useful for?
>>> My understanding is it serves 2 purposes:
>>>
>>> - report info in the monitor (hmp_info_roms)
>>> - decide whether the rom->data can be freed on ROM reset notifier
>>> (rom_reset).
>>>
>>> Hope I didn't miss anything else.
>>>
>>> Eric
>>
>> What do we decide then about this regression on arm. Do we fix it in 2.4
>> or later?
> 
> Yes, it should be fixed in 2.4.
Do you want me to resend it with a new commit message or is the context
clearer now?

Thanks

Eric
> 
> Paolo
> 

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

[Paolo Bonzini]

On 07/07/2015 11:07, Eric Auger wrote:
> On 07/07/2015 11:02 AM, Paolo Bonzini wrote:
>>
>>
>> On 07/07/2015 11:00, Eric Auger wrote:
>>> Hi Paolo, Peter,
>>> On 06/22/2015 11:58 AM, Eric Auger wrote:
>>>> On 06/22/2015 11:53 AM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 22/06/2015 11:49, Eric Auger wrote:
>>>>>>>> It seems safe because rom_load_all really doesn't load anything, it only
>>>>>>>> does an overlap check.  Is this right?
>>>>>> it does the check + isrom field setting
>>>>>>>>
>>>>>>>> Is the bug that some overlapping ROMs are not detected?  The commit
>>>>>>>> message is not clear.
>>>>>> The regression is that the both overlap check and isrom setting are not
>>>>>> done since ROM are inserted in the roms list afterwards, at machine init
>>>>>> done time. The bug was not really observed yet I think.
>>>>>
>>>>> isrom is just an optimization though, right?  What is it useful for?
>>>> My understanding is it serves 2 purposes:
>>>>
>>>> - report info in the monitor (hmp_info_roms)
>>>> - decide whether the rom->data can be freed on ROM reset notifier
>>>> (rom_reset).
>>>>
>>>> Hope I didn't miss anything else.
>>>>
>>>> Eric
>>>
>>> What do we decide then about this regression on arm. Do we fix it in 2.4
>>> or later?
>>
>> Yes, it should be fixed in 2.4.
> Do you want me to resend it with a new commit message or is the context
> clearer now?

It's okay, thanks!

Paolo