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