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

[Gonglei]

On 2015/1/29 8:42, Alexander Graf wrote:

> 
> 
> 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, address@hidden wrote:
>>>>>> From: Dinar Valeev <address@hidden>
>>>>>>
>>>>>> 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 <address@hidden>
>>>>>> ---
>>>>>>   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).
> 

Maybe yes. But lots of other machines do not  register
reset callback. So those machines using qemu_register_boot_set()
register a handler callback achieve this purpose.

Regards,
-Gonglei