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

[Markus Armbruster]

Dinar Valeev <address@hidden> 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, address@hidden wrote:
>>>>>>
>>>>>>> From: Dinar Valeev <address@hidden>
>>>>>>>
>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>> got changed on reset.
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <address@hidden>
>>>>>>> ---
>>>>>>>  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 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().