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

[Gonglei]

On 2015/1/29 6:22, Dinar Valeev wrote:

> 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?
> 

I have replied your previous thread. :)

Regards,
-Gonglei

> 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.
> 
> Thanks,
> Dinar
>>
>> Regards,
>> -Gonglei
>>
>>
>