| Message ID | 1422316341-28983-3-git-send-email-dvaleev@suse.de |
|---|---|
| State | New |
| Headers | show |
On 2015/1/27 7:52, dvaleev@suse.de wrote: > From: Dinar Valeev <dvaleev@suse.com> > > on sPAPR we need to update boot_order in MachineState in case it > got changed on reset. > > Signed-off-by: Dinar Valeev <dvaleev@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. Regards, -Gonglei
On 01/27/2015 03:51 AM, Gonglei wrote: > On 2015/1/27 7:52, dvaleev@suse.de wrote: > >> From: Dinar Valeev <dvaleev@suse.com> >> >> on sPAPR we need to update boot_order in MachineState in case it >> got changed on reset. >> >> Signed-off-by: Dinar Valeev <dvaleev@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. Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset. > > Regards, > -Gonglei >
On 2015/1/27 16:57, Dinar Valeev wrote: > On 01/27/2015 03:51 AM, Gonglei wrote: >> On 2015/1/27 7:52, dvaleev@suse.de wrote: >> >>> From: Dinar Valeev <dvaleev@suse.com> >>> >>> on sPAPR we need to update boot_order in MachineState in case it >>> got changed on reset. >>> >>> Signed-off-by: Dinar Valeev <dvaleev@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? Would you debug it by setting a breakpoint ? Regards, -Gonglei > Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset. > >> >> Regards, >> -Gonglei >> >
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, dvaleev@suse.de wrote: >>> >>>> From: Dinar Valeev <dvaleev@suse.com> >>>> >>>> on sPAPR we need to update boot_order in MachineState in case it >>>> got changed on reset. >>>> >>>> Signed-off-by: Dinar Valeev <dvaleev@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? I'll see what I can do here. I recall once I debugged once option, I haven't seen error message either. > Would you debug it by setting a breakpoint ? > > Regards, > -Gonglei > >> Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset. >> >>> >>> Regards, >>> -Gonglei >>> >> > > >
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, dvaleev@suse.de wrote: >>> >>>> From: Dinar Valeev <dvaleev@suse.com> >>>> >>>> on sPAPR we need to update boot_order in MachineState in case it >>>> got changed on reset. >>>> >>>> Signed-off-by: Dinar Valeev <dvaleev@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. > > Regards, > -Gonglei > >> Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset. >> >>> >>> Regards, >>> -Gonglei >>> >> > > >
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, dvaleev@suse.de wrote: >>>> >>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>> >>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>> got changed on reset. >>>>> >>>>> Signed-off-by: Dinar Valeev <dvaleev@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. Regards, -Gonglei
On 01/28/2015 02:48 AM, 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, dvaleev@suse.de wrote: >>>>> >>>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>>> >>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>> got changed on reset. >>>>>> >>>>>> Signed-off-by: Dinar Valeev <dvaleev@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. Yes Any proposal how this can be done differently? It seems I'm almost alone who wants -boot once=X option to get fixed for sPAPR. We use it in test automation, and we need to be sure that we boot from hard disk once installation is done. Thanks, Dinar > > Regards, > -Gonglei > >
On 2015/1/29 6:22, Dinar Valeev wrote: > On 01/28/2015 02:48 AM, 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, dvaleev@suse.de wrote: >>>>>> >>>>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>>>> >>>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>>> got changed on reset. >>>>>>> >>>>>>> Signed-off-by: Dinar Valeev <dvaleev@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. > Yes > > Any proposal how this can be done differently? > I have replied your previous thread. :) Regards, -Gonglei > It seems I'm almost alone who wants -boot once=X option to get fixed for sPAPR. We use it in test automation, and we need to be sure that we boot from hard disk once installation is done. > > Thanks, > Dinar >> >> Regards, >> -Gonglei >> >> >
Dinar Valeev <dvaleev@suse.de> writes: > On 01/28/2015 02:48 AM, 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, dvaleev@suse.de wrote: >>>>>> >>>>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>>>> >>>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>>> got changed on reset. >>>>>>> >>>>>>> Signed-off-by: Dinar Valeev <dvaleev@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. > Yes > > Any proposal how this can be done differently? > > It seems I'm almost alone who wants -boot once=X option to get fixed > for sPAPR. We use it in test automation, and we need to be sure that > we boot from hard disk once installation is done. The crash is a bug, and bugs need fixing. You first patch looked okay to me. I suggested dropping the null check in spapr_create_fdt_skel(), because I feel it's papering over the bug you fix. This isn't a review of your second attempt, it's encouragement. I haven't looked at this version, nor have I thought through Alex's idea to patch machine->boot_order in qemu_boot_set().
On 29.01.15 08:48, Markus Armbruster wrote: > Dinar Valeev <dvaleev@suse.de> writes: > >> On 01/28/2015 02:48 AM, 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, dvaleev@suse.de wrote: >>>>>>> >>>>>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>>>>> >>>>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>>>> got changed on reset. >>>>>>>> >>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@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. >> Yes >> >> Any proposal how this can be done differently? >> >> It seems I'm almost alone who wants -boot once=X option to get fixed >> for sPAPR. We use it in test automation, and we need to be sure that >> we boot from hard disk once installation is done. > > The crash is a bug, and bugs need fixing. > > You first patch looked okay to me. I'd rather not modify the "base fdt" ;). Today we have a pretty simple layout that does base fdt -> init runtime changes -> reset I'd prefer to keep it that way, so over the lifetime of a VM, the base fdt shouldn't change. > I suggested dropping the null check > in spapr_create_fdt_skel(), because I feel it's papering over the bug > you fix. > > This isn't a review of your second attempt, it's encouragement. I > haven't looked at this version, nor have I thought through Alex's idea > to patch machine->boot_order in qemu_boot_set(). If people are against it (though I can't see why), we can also move the logic to spapr.c and change the machine->boot_order in a local callback to set_boot_handler(). Alex
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, dvaleev@suse.de wrote: >>>>> >>>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>>> >>>>>> on sPAPR we need to update boot_order in MachineState in case it >>>>>> got changed on reset. >>>>>> >>>>>> Signed-off-by: Dinar Valeev <dvaleev@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
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"