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

[Gonglei]

On 2015/1/29 20:55, Alexander Graf wrote:

> 
> 
> On 29.01.15 04:46, Gonglei wrote:
>> 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.
> 
> I think we're better off just registering reset handlers then.
> 

Just like what we said in other thread, remove the check about handler,
I will post a patch soon.

Regards,
-Gonglei