Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Alexander Graf <agraf@suse.de>]

On 29.01.15 08:48, Markus Armbruster wrote:
> Dinar Valeev <dvaleev@suse.de> writes:
> 
>> On 01/28/2015 02:48 AM, Gonglei wrote:
>>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>>
>>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>>
>>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>>
>>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>>
>>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>>> got changed on reset.
>>>>>>>>
>>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>>> ---
>>>>>>>>  bootdevice.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>>> index 5914417..4f11a06 100644
>>>>>>>> --- a/bootdevice.c
>>>>>>>> +++ b/bootdevice.c
>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>  #include "qapi/visitor.h"
>>>>>>>>  #include "qemu/error-report.h"
>>>>>>>>  #include "hw/hw.h"
>>>>>>>> +#include "hw/boards.h"
>>>>>>>>  
>>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>>  
>>>>>>>> @@ -50,6 +51,8 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
>>>>>>>>  void qemu_boot_set(const char *boot_order, Error **errp)
>>>>>>>>  {
>>>>>>>>      Error *local_err = NULL;
>>>>>>>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>>  
>>>>>>>>      if (!boot_set_handler) {
>>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>>
>>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>>  will return error.
>>>>>> No, I set boot_order on each machine reset. My tests are showing
>>>>>> it works without an error.
>>>>>
>>>>> That's interesting. Does this function be called?
>>>> Yes, then simply returns.
>>>>> Would you debug it by setting a breakpoint ?
>>>> I added a trace event.
>>>>      if (!boot_set_handler) {
>>>> +        trace_qemu_boot_set(boot_order);
>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>                           " this architecture");
>>>>          return;
>>>>
>>>> And I see this now in qemu's monitor. Still I don't see error message.
>>>
>>> That's because NULL is passed to this function in restore_boot_order()
>>> the error is ignored (commit f183993). I have seen the previous conversation
>>> about your patch serials. And I think this is the reason which
>>> you moved machine->boot_order = boot_order before
>>> checking boot_set_handler variable based on Alexander's
>>> suggestion, right? But I think this is not a good idea.
>> Yes
>>
>> Any proposal how this can be done differently?
>>
>> It seems I'm almost alone who wants -boot once=X option to get fixed
>> for sPAPR. We use it in test automation, and we need to be sure that
>> we boot from hard disk once installation is done.
> 
> The crash is a bug, and bugs need fixing.
> 
> You first patch looked okay to me.  

I'd rather not modify the "base fdt" ;). Today we have a pretty simple
layout that does

base fdt -> init
runtime changes -> reset

I'd prefer to keep it that way, so over the lifetime of a VM, the base
fdt shouldn't change.

> I suggested dropping the null check
> in spapr_create_fdt_skel(), because I feel it's papering over the bug
> you fix.
> 
> This isn't a review of your second attempt, it's encouragement.  I
> haven't looked at this version, nor have I thought through Alex's idea
> to patch machine->boot_order in qemu_boot_set().

If people are against it (though I can't see why), we can also move the
logic to spapr.c and change the machine->boot_order in a local callback
to set_boot_handler().


Alex