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, dval...@suse.de wrote:
>>>>>
>>>>>> From: Dinar Valeev <dval...@suse.com>
>>>>>>
>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>> got changed on reset.
>>>>>>
>>>>>> Signed-off-by: Dinar Valeev <dval...@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.
Why is it not a good idea? The check is only checking whether there are
callbacks. The boot order changes nevertheless.
Alex
