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, 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.
>>
>
> 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
