* [Qemu-devel] Update boot_order on reset for sPAPR @ 2015-01-26 23:52 dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset dvaleev ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: dvaleev @ 2015-01-26 23:52 UTC (permalink / raw) To: qemu-devel; +Cc: Gonglei, qemu-ppc, Alexander Graf, armbru This is cleaner attempt to fix -boot once=X behaviour on sPAPR qemu-system-ppc64 -m 1024 -enable-kvm -boot once=d -vga none -nographic Ready! 0 > dev /chosen ok 0 > .properties stdout 3e512100 .... ibm,architecture-vec-5 00000000 0080 qemu,boot-device d 6400 linux,stdout-path /vdevice/vty@71000000 .... 0 > reset-all .... Ready! 0 > dev /chosen ok 0 > .properties stdout 3e512100 .... ibm,architecture-vec-5 00000000 0080 linux,stdout-path /vdevice/vty@71000000 Compared to previous patch set, this one doesn't implement register_boot_set for sPAPR, but instead updates boot_order at MachineState and read it on each guest's reset. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset 2015-01-26 23:52 [Qemu-devel] Update boot_order on reset for sPAPR dvaleev @ 2015-01-26 23:52 ` dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState dvaleev 2015-01-27 2:31 ` [Qemu-devel] Update boot_order on reset for sPAPR Alexey Kardashevskiy 2 siblings, 0 replies; 17+ messages in thread From: dvaleev @ 2015-01-26 23:52 UTC (permalink / raw) To: qemu-devel; +Cc: Dinar Valeev, Gonglei, qemu-ppc, Alexander Graf, armbru From: Dinar Valeev <dvaleev@suse.com> Update boot_device on reset. This is required to have -boot once=X working. This patch is moving creating fdt qemu,boot-device entry from spapr_create_fdt_skel to spapr_finalize_fdt which is used on guest reset. Signed-off-by: Dinar Valeev <dvaleev@suse.com> --- hw/ppc/spapr.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b560459..36b0bdb 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -318,7 +318,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, hwaddr initrd_size, hwaddr kernel_size, bool little_endian, - const char *boot_device, const char *kernel_cmdline, uint32_t epow_irq) { @@ -411,9 +410,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0))); } } - if (boot_device) { - _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device))); - } if (boot_menu) { _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu))); } @@ -730,6 +726,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr, char *bootlist; void *fdt; sPAPRPHBState *phb; + MachineState *machine = MACHINE(qdev_get_machine()); + const char *boot_device = machine->boot_order; fdt = g_malloc(FDT_MAX_SIZE); @@ -769,6 +767,14 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr, fprintf(stderr, "Couldn't finalize CPU device tree properties\n"); } + if (boot_device) { + int offset = fdt_path_offset(fdt, "/chosen"); + ret = fdt_setprop_string(fdt, offset, "qemu,boot-device", boot_device); + if (ret < 0) { + fprintf(stderr, "Couldn't set up boot-device dt property\n"); + } + } + bootlist = get_boot_devices_list(&cb, true); if (cb && bootlist) { int offset = fdt_path_offset(fdt, "/chosen"); @@ -1347,7 +1353,6 @@ static void ppc_spapr_init(MachineState *machine) const char *kernel_filename = machine->kernel_filename; const char *kernel_cmdline = machine->kernel_cmdline; const char *initrd_filename = machine->initrd_filename; - const char *boot_device = machine->boot_order; PowerPCCPU *cpu; CPUPPCState *env; PCIHostState *phb; @@ -1601,8 +1606,7 @@ static void ppc_spapr_init(MachineState *machine) /* Prepare the device tree */ spapr->fdt_skel = spapr_create_fdt_skel(initrd_base, initrd_size, kernel_size, kernel_le, - boot_device, kernel_cmdline, - spapr->epow_irq); + kernel_cmdline, spapr->epow_irq); assert(spapr->fdt_skel != NULL); } -- 2.1.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-26 23:52 [Qemu-devel] Update boot_order on reset for sPAPR dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset dvaleev @ 2015-01-26 23:52 ` dvaleev 2015-01-27 2:51 ` Gonglei 2015-01-27 2:31 ` [Qemu-devel] Update boot_order on reset for sPAPR Alexey Kardashevskiy 2 siblings, 1 reply; 17+ messages in thread From: dvaleev @ 2015-01-26 23:52 UTC (permalink / raw) To: qemu-devel; +Cc: Dinar Valeev, Gonglei, qemu-ppc, Alexander Graf, armbru 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" -- 2.1.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-26 23:52 ` [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState dvaleev @ 2015-01-27 2:51 ` Gonglei 2015-01-27 8:57 ` Dinar Valeev 0 siblings, 1 reply; 17+ messages in thread From: Gonglei @ 2015-01-27 2:51 UTC (permalink / raw) To: dvaleev; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-27 2:51 ` Gonglei @ 2015-01-27 8:57 ` Dinar Valeev 2015-01-27 9:18 ` Gonglei 0 siblings, 1 reply; 17+ messages in thread From: Dinar Valeev @ 2015-01-27 8:57 UTC (permalink / raw) To: Gonglei; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-27 8:57 ` Dinar Valeev @ 2015-01-27 9:18 ` Gonglei 2015-01-27 9:56 ` Dinar Valeev 2015-01-27 10:49 ` Dinar Valeev 0 siblings, 2 replies; 17+ messages in thread From: Gonglei @ 2015-01-27 9:18 UTC (permalink / raw) To: Dinar Valeev; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-27 9:18 ` Gonglei @ 2015-01-27 9:56 ` Dinar Valeev 2015-01-27 10:49 ` Dinar Valeev 1 sibling, 0 replies; 17+ messages in thread From: Dinar Valeev @ 2015-01-27 9:56 UTC (permalink / raw) To: Gonglei; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 >>> >> > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-27 9:18 ` Gonglei 2015-01-27 9:56 ` Dinar Valeev @ 2015-01-27 10:49 ` Dinar Valeev 2015-01-28 1:48 ` Gonglei 1 sibling, 1 reply; 17+ messages in thread From: Dinar Valeev @ 2015-01-27 10:49 UTC (permalink / raw) To: Gonglei; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 >>> >> > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-27 10:49 ` Dinar Valeev @ 2015-01-28 1:48 ` Gonglei 2015-01-28 22:22 ` Dinar Valeev 2015-01-29 10:55 ` Alexander Graf 0 siblings, 2 replies; 17+ messages in thread From: Gonglei @ 2015-01-28 1:48 UTC (permalink / raw) To: Dinar Valeev; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-28 1:48 ` Gonglei @ 2015-01-28 22:22 ` Dinar Valeev 2015-01-29 0:41 ` Gonglei 2015-01-29 7:48 ` Markus Armbruster 2015-01-29 10:55 ` Alexander Graf 1 sibling, 2 replies; 17+ messages in thread From: Dinar Valeev @ 2015-01-28 22:22 UTC (permalink / raw) To: Gonglei; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-28 22:22 ` Dinar Valeev @ 2015-01-29 0:41 ` Gonglei 2015-01-29 7:48 ` Markus Armbruster 1 sibling, 0 replies; 17+ messages in thread From: Gonglei @ 2015-01-29 0:41 UTC (permalink / raw) To: Dinar Valeev; +Cc: armbru, Dinar Valeev, qemu-ppc, qemu-devel, Alexander Graf 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 >> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-28 22:22 ` Dinar Valeev 2015-01-29 0:41 ` Gonglei @ 2015-01-29 7:48 ` Markus Armbruster 2015-01-29 10:53 ` Alexander Graf 1 sibling, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2015-01-29 7:48 UTC (permalink / raw) To: Dinar Valeev; +Cc: qemu-ppc, Gonglei, Dinar Valeev, qemu-devel, Alexander Graf 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(). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-29 7:48 ` Markus Armbruster @ 2015-01-29 10:53 ` Alexander Graf 0 siblings, 0 replies; 17+ messages in thread From: Alexander Graf @ 2015-01-29 10:53 UTC (permalink / raw) To: Markus Armbruster, Dinar Valeev Cc: qemu-ppc, Gonglei, Dinar Valeev, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-28 1:48 ` Gonglei 2015-01-28 22:22 ` Dinar Valeev @ 2015-01-29 10:55 ` Alexander Graf 2015-01-29 11:08 ` Gonglei 1 sibling, 1 reply; 17+ messages in thread From: Alexander Graf @ 2015-01-29 10:55 UTC (permalink / raw) To: Gonglei, Dinar Valeev; +Cc: Dinar Valeev, qemu-ppc, qemu-devel, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-29 10:55 ` Alexander Graf @ 2015-01-29 11:08 ` Gonglei 2015-01-29 11:11 ` Alexander Graf 0 siblings, 1 reply; 17+ messages in thread From: Gonglei @ 2015-01-29 11:08 UTC (permalink / raw) To: Alexander Graf; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel, armbru 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, 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. > 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: diff --git a/bootdevice.c b/bootdevice.c index c3a010c..98ed2d2 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp) { Error *local_err = NULL; - if (!boot_set_handler) { - error_setg(errp, "no function defined to set boot device list for" - " this architecture"); - return; - } - validate_bootdevices(boot_order, &local_err); if (local_err) { error_propagate(errp, local_err); return; } - boot_set_handler(boot_set_opaque, boot_order, errp); + if (boot_set_handler) { + boot_set_handler(boot_set_opaque, boot_order, errp); + } } void validate_bootdevices(const char *devices, Error **errp) Regards, -Gonglei ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState 2015-01-29 11:08 ` Gonglei @ 2015-01-29 11:11 ` Alexander Graf 0 siblings, 0 replies; 17+ messages in thread From: Alexander Graf @ 2015-01-29 11:11 UTC (permalink / raw) To: Gonglei; +Cc: Dinar Valeev, Dinar Valeev, qemu-ppc, qemu-devel, armbru 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, 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. >> > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] Update boot_order on reset for sPAPR 2015-01-26 23:52 [Qemu-devel] Update boot_order on reset for sPAPR dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState dvaleev @ 2015-01-27 2:31 ` Alexey Kardashevskiy 2 siblings, 0 replies; 17+ messages in thread From: Alexey Kardashevskiy @ 2015-01-27 2:31 UTC (permalink / raw) To: dvaleev, qemu-devel; +Cc: Gonglei, qemu-ppc, Alexander Graf, armbru On 01/27/2015 10:52 AM, dvaleev@suse.de wrote: > This is cleaner attempt to fix -boot once=X behaviour on sPAPR > > qemu-system-ppc64 -m 1024 -enable-kvm -boot once=d -vga none -nographic > Ready! > 0 > dev /chosen ok > 0 > .properties > stdout 3e512100 > .... > ibm,architecture-vec-5 00000000 0080 > qemu,boot-device d > 6400 > linux,stdout-path /vdevice/vty@71000000 > .... > 0 > reset-all > .... > Ready! > 0 > dev /chosen ok > 0 > .properties > stdout 3e512100 > .... > ibm,architecture-vec-5 00000000 0080 > linux,stdout-path /vdevice/vty@71000000 > > Compared to previous patch set, this one doesn't implement register_boot_set for sPAPR, > but instead updates boot_order at MachineState and read it on each guest's reset. Looks good to me. Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> -- Alexey ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-01-29 11:11 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-26 23:52 [Qemu-devel] Update boot_order on reset for sPAPR dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset dvaleev 2015-01-26 23:52 ` [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState dvaleev 2015-01-27 2:51 ` Gonglei 2015-01-27 8:57 ` Dinar Valeev 2015-01-27 9:18 ` Gonglei 2015-01-27 9:56 ` Dinar Valeev 2015-01-27 10:49 ` Dinar Valeev 2015-01-28 1:48 ` Gonglei 2015-01-28 22:22 ` Dinar Valeev 2015-01-29 0:41 ` Gonglei 2015-01-29 7:48 ` Markus Armbruster 2015-01-29 10:53 ` Alexander Graf 2015-01-29 10:55 ` Alexander Graf 2015-01-29 11:08 ` Gonglei 2015-01-29 11:11 ` Alexander Graf 2015-01-27 2:31 ` [Qemu-devel] Update boot_order on reset for sPAPR Alexey Kardashevskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.