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

[Alexander Graf]

On 28.01.15 02:48, 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.

Why is it not a good idea? The check is only checking whether there are
callbacks. The boot order changes nevertheless.


Alex