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

2015-04-02 Thread Gonglei
On 2015/4/2 15:41, Alexey Kardashevskiy wrote:
>>
>> Just like what we said in other thread, remove the check about handler,
>> I will post a patch soon.
>>
> 
> 
> Have you posted the patch? Cannot find it in the lists so I assume you have
> not :-) Thanks!
> 
I had posted the patch which caused a regression about HMP command
hmp_boot_set(). By discussion again and again, we decide to remain the
original style. If you guys want to use this feature, please register the 
handler.
BTW I saw somebody had submitted patches with handler in the maillist. :)

Regards,
-Gonglei





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

2015-04-02 Thread Alexey Kardashevskiy
On Fri, Jan 30, 2015 at 12:00 AM, Gonglei  wrote:

> 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, dval...@suse.de wrote:
>  From: Dinar Valeev 
> 
>  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 
>  ---
>    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.
>


Have you posted the patch? Cannot find it in the lists so I assume you have
not :-) Thanks!



>
> Regards,
> -Gonglei
>
>
>


-- 
-- 
Alexey


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

2015-01-29 Thread 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, dval...@suse.de wrote:
 From: Dinar Valeev 

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




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

2015-01-29 Thread Alexander Graf


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, dval...@suse.de wrote:
>>> From: Dinar Valeev 
>>>
>>> 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 
>>> ---
>>>   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.


Alex



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

2015-01-28 Thread 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, dval...@suse.de wrote:
>> From: Dinar Valeev 
>>
>> 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 
>> ---
>>   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




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

2015-01-28 Thread Alexander Graf


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, dval...@suse.de wrote:
> From: Dinar Valeev 
>
> 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 
> ---
>   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



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

2015-01-28 Thread Gonglei
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, dval...@suse.de wrote:
 From: Dinar Valeev 

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

Regards,
-Gonglei

> 
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..3b750ff 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -50,6 +50,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void 
> *opaque)
>  void qemu_boot_set(const char *boot_order, Error **errp)
>  {
>  Error *local_err = NULL;
> +MachineState *machine = MACHINE(qdev_get_machine());
> 
>  if (!boot_set_handler) {
>  error_setg(errp, "no function defined to set boot device list for"
> @@ -63,6 +64,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>  return;
>  }
> 
> +machine->boot_order = boot_order;
>  boot_set_handler(boot_set_opaque, boot_order, errp);
>  }
> 
> 
> 
> Thanks,
> 
> Alex
> 
> 






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

2015-01-26 Thread Alexander Graf

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, dval...@suse.de wrote:

From: Dinar Valeev 

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



diff --git a/bootdevice.c b/bootdevice.c
index 5914417..3b750ff 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -50,6 +50,7 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, 
void *opaque)

 void qemu_boot_set(const char *boot_order, Error **errp)
 {
 Error *local_err = NULL;
+MachineState *machine = MACHINE(qdev_get_machine());

 if (!boot_set_handler) {
 error_setg(errp, "no function defined to set boot device list for"
@@ -63,6 +64,7 @@ void qemu_boot_set(const char *boot_order, Error **errp)
 return;
 }

+machine->boot_order = boot_order;
 boot_set_handler(boot_set_opaque, boot_order, errp);
 }



Thanks,

Alex




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

2015-01-26 Thread Dinar Valeev

On 01/24/2015 12:04 AM, Alexander Graf wrote:



On 23.01.15 23:51, dval...@suse.de wrote:

From: Dinar Valeev 

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

qemu,boot-device d
0 > reset-all
qemu,boot-device d


Here is what I tried:
@@ -411,9 +410,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
 _FDT((fdt_property(fdt, "qemu,boot-kernel-le", NULL, 0)));
 }
 }
-if (boot_device) {
-_FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
-}
 if (boot_menu) {
 _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
 }
@@ -730,6 +726,8 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
 char *bootlist;
 void *fdt;
 sPAPRPHBState *phb;
+MachineState *machine = MACHINE(qdev_get_machine());
+const char *boot_device = machine->boot_order;

 fdt = g_malloc(FDT_MAX_SIZE);

@@ -769,6 +767,14 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
 fprintf(stderr, "Couldn't finalize CPU device tree properties\n");
 }

+if (boot_device) {
+int offset = fdt_path_offset(fdt, "/chosen");
+ret = fdt_setprop_string(fdt, offset, "qemu,boot-device", 
boot_device);

+if (ret < 0) {
+fprintf(stderr, "Couldn't set up boot-device dt property\n");
+}
+}
+
 bootlist = get_boot_devices_list(&cb, true);
 if (cb && bootlist) {
 int offset = fdt_path_offset(fdt, "/chosen");

Dinar,


Alex


+
  if (boot_menu) {
  _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
  }






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

2015-01-24 Thread Dinar Valeev

On 01/24/2015 12:04 AM, Alexander Graf wrote:



On 23.01.15 23:51, dval...@suse.de wrote:

From: Dinar Valeev 

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 
---
  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 :).
Haven't tried it, but I suspect -boot once=d on reset will be still 
equal to -boot d behaviour.


And does it make sense to split boot_device from the rest?


Alex


+
  if (boot_menu) {
  _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
  }






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

2015-01-23 Thread Alexander Graf


On 23.01.15 23:51, dval...@suse.de wrote:
> From: Dinar Valeev 
> 
> 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 
> ---
>  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 :).


Alex

> +
>  if (boot_menu) {
>  _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
>  }
>