| Message ID | 1422053517-23781-1-git-send-email-dvaleev@suse.de |
|---|---|
| State | New |
| Headers | show |
On 23.01.15 23:51, dvaleev@suse.de wrote: > From: Dinar Valeev <dvaleev@suse.com> > > In order to use -boot once=X option we need to have default list > where restore to on reset. > > Signed-off-by: Dinar Valeev <dvaleev@suse.com> Alexey, Nijunj, where is the default boot order stored usually? Is "cdn" an accurate equivalent? Alex > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b560459..3d2cfa3 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > mc->block_default_type = IF_SCSI; > mc->max_cpus = MAX_CPUS; > mc->no_parallel = 1; > - mc->default_boot_order = NULL; > + mc->default_boot_order = "cdn"; > mc->kvm_type = spapr_kvm_type; > mc->has_dynamic_sysbus = true; > >
dvaleev@suse.de writes: > From: Dinar Valeev <dvaleev@suse.com> > > In order to use -boot once=X option we need to have default list > where restore to on reset. Really? What happens without this patch?
On 01/26/2015 01:57 PM, Dinar Valeev wrote: > On 01/26/2015 01:37 PM, Markus Armbruster wrote: >> Dinar Valeev <dvaleev@suse.de> writes: >> >>> On 01/26/2015 10:11 AM, Markus Armbruster wrote: >>>> dvaleev@suse.de writes: >>>> >>>>> From: Dinar Valeev <dvaleev@suse.com> >>>>> >>>>> In order to use -boot once=X option we need to have default list >>>>> where restore to on reset. >>>> >>>> Really? What happens without this patch? >>>> >>> qemu segfaults on reset. >>> 0 > reset-all Segmentation fault >> >> Next time, include a backtrace, please. > Ok, sorry for that. >> >> Here's what I think happens. >> >> Boot order comes from --boot parameter once, order, or else the machine >> type's .default_boot_order. The latter is null for you. >> >> It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which >> sets qemu,boot-device in the FDT to it, but only when it isn't null. >> >> If it comes from parameter once, we additionally register a reset >> handler to switch it to parameter order or else .default_boot_order on >> reset. If you specify once, but not order, this is null for you. >> >> On reset, reset handler restore_boot_order() runs. Unlike >> spapr_create_fdt_skel(), it doesn't check for null, and crashes in >> validate_bootdevices(). >> >> Correct? > Yes > > qemu_register_boot_set is implemented in PATCH 2/2. on reset > boot_device is restored to NULL >> >> For me, a null .default_boot_order means "machine type does not support >> boot order" (this is how commit c165473 treats it). Arguably, --boot >> order and once should be rejected then. > AFICS SLOF handles qemu,boot-device as boot device, if nothing passed > then it goes disk, cdrom, network. Which is the same as "cdn" list. That's not entirely true. If SLOF doesn't see qemu,boot-device, it first goes via its NVRAM configured boot list and only if nothing is there it falls back to "cdn". Maybe the actual appropriate thing would be "mcdn" with m meaning "NVRAM". Alex > >> If I understand you correctly, your machine type does support boot >> order. Giving it a non-null .default_boot_order makes sense then. The >> appropriate value depends on firmware. It could even be "". >> >> The null check in spapr_create_fdt_skel() looks superfluous then. >> Consider dropping it. >> >> Makes sense? >> >
On 01/26/2015 10:11 AM, Markus Armbruster wrote: > dvaleev@suse.de writes: > >> From: Dinar Valeev <dvaleev@suse.com> >> >> In order to use -boot once=X option we need to have default list >> where restore to on reset. > > Really? What happens without this patch? > qemu segfaults on reset. 0 > reset-all Segmentation fault
Dinar Valeev <dvaleev@suse.de> writes: > On 01/26/2015 10:11 AM, Markus Armbruster wrote: >> dvaleev@suse.de writes: >> >>> From: Dinar Valeev <dvaleev@suse.com> >>> >>> In order to use -boot once=X option we need to have default list >>> where restore to on reset. >> >> Really? What happens without this patch? >> > qemu segfaults on reset. > 0 > reset-all Segmentation fault Next time, include a backtrace, please. Here's what I think happens. Boot order comes from --boot parameter once, order, or else the machine type's .default_boot_order. The latter is null for you. It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which sets qemu,boot-device in the FDT to it, but only when it isn't null. If it comes from parameter once, we additionally register a reset handler to switch it to parameter order or else .default_boot_order on reset. If you specify once, but not order, this is null for you. On reset, reset handler restore_boot_order() runs. Unlike spapr_create_fdt_skel(), it doesn't check for null, and crashes in validate_bootdevices(). Correct? For me, a null .default_boot_order means "machine type does not support boot order" (this is how commit c165473 treats it). Arguably, --boot order and once should be rejected then. If I understand you correctly, your machine type does support boot order. Giving it a non-null .default_boot_order makes sense then. The appropriate value depends on firmware. It could even be "". The null check in spapr_create_fdt_skel() looks superfluous then. Consider dropping it. Makes sense?
On 01/26/2015 01:37 PM, Markus Armbruster wrote: > Dinar Valeev <dvaleev@suse.de> writes: > >> On 01/26/2015 10:11 AM, Markus Armbruster wrote: >>> dvaleev@suse.de writes: >>> >>>> From: Dinar Valeev <dvaleev@suse.com> >>>> >>>> In order to use -boot once=X option we need to have default list >>>> where restore to on reset. >>> >>> Really? What happens without this patch? >>> >> qemu segfaults on reset. >> 0 > reset-all Segmentation fault > > Next time, include a backtrace, please. Ok, sorry for that. > > Here's what I think happens. > > Boot order comes from --boot parameter once, order, or else the machine > type's .default_boot_order. The latter is null for you. > > It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which > sets qemu,boot-device in the FDT to it, but only when it isn't null. > > If it comes from parameter once, we additionally register a reset > handler to switch it to parameter order or else .default_boot_order on > reset. If you specify once, but not order, this is null for you. > > On reset, reset handler restore_boot_order() runs. Unlike > spapr_create_fdt_skel(), it doesn't check for null, and crashes in > validate_bootdevices(). > > Correct? Yes qemu_register_boot_set is implemented in PATCH 2/2. on reset boot_device is restored to NULL > > For me, a null .default_boot_order means "machine type does not support > boot order" (this is how commit c165473 treats it). Arguably, --boot > order and once should be rejected then. AFICS SLOF handles qemu,boot-device as boot device, if nothing passed then it goes disk, cdrom, network. Which is the same as "cdn" list. > If I understand you correctly, your machine type does support boot > order. Giving it a non-null .default_boot_order makes sense then. The > appropriate value depends on firmware. It could even be "". > > The null check in spapr_create_fdt_skel() looks superfluous then. > Consider dropping it. > > Makes sense? >
Alexander Graf <agraf@suse.de> writes: > On 23.01.15 23:51, dvaleev@suse.de wrote: >> From: Dinar Valeev <dvaleev@suse.com> >> >> In order to use -boot once=X option we need to have default list >> where restore to on reset. >> >> Signed-off-by: Dinar Valeev <dvaleev@suse.com> > > Alexey, Nijunj, where is the default boot order stored usually? When -boot argument is not present, the default boot order is NULL for spapr, and SLOF decides to boot from NVRAM set variable or discovered disk. > Is "cdn" an accurate equivalent? No, we have changed that to NULL. http://comments.gmane.org/gmane.comp.emulators.qemu/187318 Regards, Nikunj
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b560459..3d2cfa3 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_SCSI; mc->max_cpus = MAX_CPUS; mc->no_parallel = 1; - mc->default_boot_order = NULL; + mc->default_boot_order = "cdn"; mc->kvm_type = spapr_kvm_type; mc->has_dynamic_sysbus = true;