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

[Alexander Graf]

On 29.01.15 12:08, Gonglei wrote:
> On 2015/1/29 18:55, Alexander Graf wrote:
> 
>>
>>
>> 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.
>>
> 
> I mean we can't simply ignore this error. If you don't use boot_set_handler
> but machine->boot_order then the check should not report error.
> 
> Something like this:

Oh, I see. Yes, I agree there :).


Alex