Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Alexander Graf <agraf@suse.de>]

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?
>>
>