Re: [Qemu-devel] [PATCH 2/2] hw/ppc/spapr Add qemu_register_boot_set for SPAPR

[Alexander Graf <agraf@suse.de>]

On 29.01.15 01:40, Gonglei wrote:
> On 2015/1/26 17:35, Alexander Graf wrote:
> 
>> On 01/26/2015 11:49 AM, Dinar Valeev wrote:
>>> On 01/24/2015 12:04 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 23.01.15 23:51, dvaleev@suse.de wrote:
>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>
>>>>> In order to have -boot once=d functioning, it is required to have
>>>>> qemu_register_boot_set
>>>>>
>>>>> qemu-system-ppc64 -enable-kvm -boot once=d
>>>>>
>>>>> Ready!
>>>>> 0 > dev /chosen   ok
>>>>> 0 > .properties
>>>>> ...
>>>>> qemu,boot-device                 d
>>>>> ...
>>>>> 0 > reset-all
>>>>>
>>>>> Ready!
>>>>> 0 > dev /chosen   ok
>>>>> 0 > .properties
>>>>> ...
>>>>> qemu,boot-device                 cdn
>>>>> ...
>>>>>
>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>> ---
>>>>>   hw/ppc/spapr.c | 12 ++++++++++++
>>>>>   1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 3d2cfa3..38b03fc 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -314,6 +314,16 @@ static void add_str(GString *s, const gchar *s1)
>>>>>       g_string_append_len(s, s1, strlen(s1) + 1);
>>>>>   }
>>>>>
>>>>> +static void spapr_boot_set(void *opaque, const char *boot_device,
>>>>> +                           Error **errp)
>>>>> +{
>>>>> +    int offset;
>>>>> +    offset = fdt_path_offset(opaque, "/chosen");
>>>>> +    fdt_setprop_string(opaque, offset, "qemu,boot-device", boot_device);
>>>>> +
>>>>> +}
>>>>> +
>>>>> +
>>>>>   static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>                                      hwaddr initrd_size,
>>>>>                                      hwaddr kernel_size,
>>>>> @@ -414,6 +424,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>>>>>       if (boot_device) {
>>>>>           _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>>>>>       }
>>>>> +    qemu_register_boot_set(spapr_boot_set, fdt);
>>>>
>>>> If you simply move the code above (the _FDT() one) from create_fdt_skel
>>>> to spapr_finalize_fdt() you should have the same net effect and much
>>>> cleaner code :).
>>> I've tried your proposal, on reset boot-device property stays "d"
>>
>> Ugh, the machine field doesn't change on reset. I think it'd be a lot more intuitive if it did. Can you try with the patch below applied as well?
>>
> 
> This approach is not good because boot_set_handler is NULL and return error directly.
> Please using qemu_register_boot_set for this purpose.

I'd personally prefer if we get rid of qemu_register_boot_set
completely. It duplicates the reset logic as well as information holder
locality (machine struct vs parameter).


Alex