[Qemu-devel] Update boot_order on reset for sPAPR

[Qemu-devel] Update boot_order on reset for sPAPR

[dvaleev]

This is cleaner attempt to fix -boot once=X behaviour on sPAPR

qemu-system-ppc64 -m 1024 -enable-kvm -boot once=d -vga none -nographic
Ready! 
0 > dev /chosen   ok
0 > .properties  
stdout                           3e512100  
....
ibm,architecture-vec-5           00000000  0080
qemu,boot-device                 d
                                 6400
linux,stdout-path                /vdevice/vty@71000000
....
0 > reset-all
....
Ready! 
0 > dev /chosen   ok
0 > .properties  
stdout                           3e512100  
....
ibm,architecture-vec-5           00000000  0080
linux,stdout-path                /vdevice/vty@71000000

Compared to previous patch set, this one doesn't implement register_boot_set for sPAPR,
but instead updates boot_order at MachineState and read it on each guest's reset.

[Qemu-devel] [PATCH 1/2] sPAPR: reread boot_device on reset

[dvaleev]

From: Dinar Valeev <dvaleev@suse.com>

Update boot_device on reset. This is required to have -boot once=X
working.

This patch is moving creating fdt qemu,boot-device entry from
spapr_create_fdt_skel to spapr_finalize_fdt which is used on guest
reset.

Signed-off-by: Dinar Valeev <dvaleev@suse.com>
---
 hw/ppc/spapr.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..36b0bdb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -318,7 +318,6 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
                                    hwaddr initrd_size,
                                    hwaddr kernel_size,
                                    bool little_endian,
-                                   const char *boot_device,
                                    const char *kernel_cmdline,
                                    uint32_t epow_irq)
 {
@@ -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");
@@ -1347,7 +1353,6 @@ static void ppc_spapr_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
-    const char *boot_device = machine->boot_order;
     PowerPCCPU *cpu;
     CPUPPCState *env;
     PCIHostState *phb;
@@ -1601,8 +1606,7 @@ static void ppc_spapr_init(MachineState *machine)
     /* Prepare the device tree */
     spapr->fdt_skel = spapr_create_fdt_skel(initrd_base, initrd_size,
                                             kernel_size, kernel_le,
-                                            boot_device, kernel_cmdline,
-                                            spapr->epow_irq);
+                                            kernel_cmdline, spapr->epow_irq);
     assert(spapr->fdt_skel != NULL);
 }
 
-- 
2.1.2

[Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[dvaleev]

From: Dinar Valeev <dvaleev@suse.com>

on sPAPR we need to update boot_order in MachineState in case it
got changed on reset.

Signed-off-by: Dinar Valeev <dvaleev@suse.com>
---
 bootdevice.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/bootdevice.c b/bootdevice.c
index 5914417..4f11a06 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -26,6 +26,7 @@
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/hw.h"
+#include "hw/boards.h"
 
 typedef struct FWBootEntry FWBootEntry;
 
@@ -50,6 +51,8 @@ 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());
+    machine->boot_order = boot_order;
 
     if (!boot_set_handler) {
         error_setg(errp, "no function defined to set boot device list for"
-- 
2.1.2

Re: [Qemu-devel] Update boot_order on reset for sPAPR

[Alexey Kardashevskiy]

On 01/27/2015 10:52 AM, dvaleev@suse.de wrote:
> This is cleaner attempt to fix -boot once=X behaviour on sPAPR
> 
> qemu-system-ppc64 -m 1024 -enable-kvm -boot once=d -vga none -nographic
> Ready! 
> 0 > dev /chosen   ok
> 0 > .properties  
> stdout                           3e512100  
> ....
> ibm,architecture-vec-5           00000000  0080
> qemu,boot-device                 d
>                                  6400
> linux,stdout-path                /vdevice/vty@71000000
> ....
> 0 > reset-all
> ....
> Ready! 
> 0 > dev /chosen   ok
> 0 > .properties  
> stdout                           3e512100  
> ....
> ibm,architecture-vec-5           00000000  0080
> linux,stdout-path                /vdevice/vty@71000000
> 
> Compared to previous patch set, this one doesn't implement register_boot_set for sPAPR,
> but instead updates boot_order at MachineState and read it on each guest's reset.

Looks good to me.
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Gonglei]

On 2015/1/27 7:52, dvaleev@suse.de wrote:

> From: Dinar Valeev <dvaleev@suse.com>
> 
> on sPAPR we need to update boot_order in MachineState in case it
> got changed on reset.
> 
> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
> ---
>  bootdevice.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..4f11a06 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -26,6 +26,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
> +#include "hw/boards.h"
>  
>  typedef struct FWBootEntry FWBootEntry;
>  
> @@ -50,6 +51,8 @@ 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());
> +    machine->boot_order = boot_order;
>  
>      if (!boot_set_handler) {
>          error_setg(errp, "no function defined to set boot device list for"

Have you registered boot set handler on ppc/sPAPR platform by calling
qemu_register_boot_set()? Otherwise qemu_boot_set function
 will return error.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Dinar Valeev]

On 01/27/2015 03:51 AM, Gonglei wrote:
> On 2015/1/27 7:52, dvaleev@suse.de wrote:
> 
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> on sPAPR we need to update boot_order in MachineState in case it
>> got changed on reset.
>>
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>> ---
>>  bootdevice.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index 5914417..4f11a06 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -26,6 +26,7 @@
>>  #include "qapi/visitor.h"
>>  #include "qemu/error-report.h"
>>  #include "hw/hw.h"
>> +#include "hw/boards.h"
>>  
>>  typedef struct FWBootEntry FWBootEntry;
>>  
>> @@ -50,6 +51,8 @@ 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());
>> +    machine->boot_order = boot_order;
>>  
>>      if (!boot_set_handler) {
>>          error_setg(errp, "no function defined to set boot device list for"
> 
> Have you registered boot set handler on ppc/sPAPR platform by calling
> qemu_register_boot_set()? Otherwise qemu_boot_set function
>  will return error.
No, I set boot_order on each machine reset. My tests are showing it works without an error.
Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset.

> 
> Regards,
> -Gonglei
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Gonglei]

On 2015/1/27 16:57, Dinar Valeev wrote:

> On 01/27/2015 03:51 AM, Gonglei wrote:
>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>
>>> From: Dinar Valeev <dvaleev@suse.com>
>>>
>>> on sPAPR we need to update boot_order in MachineState in case it
>>> got changed on reset.
>>>
>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>> ---
>>>  bootdevice.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/bootdevice.c b/bootdevice.c
>>> index 5914417..4f11a06 100644
>>> --- a/bootdevice.c
>>> +++ b/bootdevice.c
>>> @@ -26,6 +26,7 @@
>>>  #include "qapi/visitor.h"
>>>  #include "qemu/error-report.h"
>>>  #include "hw/hw.h"
>>> +#include "hw/boards.h"
>>>  
>>>  typedef struct FWBootEntry FWBootEntry;
>>>  
>>> @@ -50,6 +51,8 @@ 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());
>>> +    machine->boot_order = boot_order;
>>>  
>>>      if (!boot_set_handler) {
>>>          error_setg(errp, "no function defined to set boot device list for"
>>
>> Have you registered boot set handler on ppc/sPAPR platform by calling
>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>  will return error.
> No, I set boot_order on each machine reset. My tests are showing it works without an error.

That's interesting. Does this function be called?
Would you debug it by setting a breakpoint ?

Regards,
-Gonglei

> Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset.
> 
>>
>> Regards,
>> -Gonglei
>>
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Dinar Valeev]

On 01/27/2015 10:18 AM, Gonglei wrote:
> On 2015/1/27 16:57, Dinar Valeev wrote:
> 
>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>
>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>
>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>> got changed on reset.
>>>>
>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>> ---
>>>>  bootdevice.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>> index 5914417..4f11a06 100644
>>>> --- a/bootdevice.c
>>>> +++ b/bootdevice.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include "qapi/visitor.h"
>>>>  #include "qemu/error-report.h"
>>>>  #include "hw/hw.h"
>>>> +#include "hw/boards.h"
>>>>  
>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>  
>>>> @@ -50,6 +51,8 @@ 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());
>>>> +    machine->boot_order = boot_order;
>>>>  
>>>>      if (!boot_set_handler) {
>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>
>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>  will return error.
>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
> 
> That's interesting. Does this function be called?
I'll see what I can do here. I recall once I debugged once option, I haven't seen
error message either.

> Would you debug it by setting a breakpoint ?
> 
> Regards,
> -Gonglei
> 
>> Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset.
>>
>>>
>>> Regards,
>>> -Gonglei
>>>
>>
> 
> 
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Dinar Valeev]

On 01/27/2015 10:18 AM, Gonglei wrote:
> On 2015/1/27 16:57, Dinar Valeev wrote:
> 
>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>
>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>
>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>> got changed on reset.
>>>>
>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>> ---
>>>>  bootdevice.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>> index 5914417..4f11a06 100644
>>>> --- a/bootdevice.c
>>>> +++ b/bootdevice.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include "qapi/visitor.h"
>>>>  #include "qemu/error-report.h"
>>>>  #include "hw/hw.h"
>>>> +#include "hw/boards.h"
>>>>  
>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>  
>>>> @@ -50,6 +51,8 @@ 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());
>>>> +    machine->boot_order = boot_order;
>>>>  
>>>>      if (!boot_set_handler) {
>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>
>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>  will return error.
>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
> 
> That's interesting. Does this function be called?
Yes, then simply returns.
> Would you debug it by setting a breakpoint ?
I added a trace event.
     if (!boot_set_handler) {
+        trace_qemu_boot_set(boot_order);
         error_setg(errp, "no function defined to set boot device list for"
                          " this architecture");
         return;

And I see this now in qemu's monitor. Still I don't see error message.
> 
> Regards,
> -Gonglei
> 
>> Previous patch was using qemu_register_boot_set, but Alexender Graf, suggested me to use MachineState and simply update on each guest reset.
>>
>>>
>>> Regards,
>>> -Gonglei
>>>
>>
> 
> 
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Gonglei]

On 2015/1/27 18:49, Dinar Valeev wrote:

> On 01/27/2015 10:18 AM, Gonglei wrote:
>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>
>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>
>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>
>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>> got changed on reset.
>>>>>
>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>> ---
>>>>>  bootdevice.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>> index 5914417..4f11a06 100644
>>>>> --- a/bootdevice.c
>>>>> +++ b/bootdevice.c
>>>>> @@ -26,6 +26,7 @@
>>>>>  #include "qapi/visitor.h"
>>>>>  #include "qemu/error-report.h"
>>>>>  #include "hw/hw.h"
>>>>> +#include "hw/boards.h"
>>>>>  
>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>  
>>>>> @@ -50,6 +51,8 @@ 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());
>>>>> +    machine->boot_order = boot_order;
>>>>>  
>>>>>      if (!boot_set_handler) {
>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>
>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>  will return error.
>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>
>> That's interesting. Does this function be called?
> Yes, then simply returns.
>> Would you debug it by setting a breakpoint ?
> I added a trace event.
>      if (!boot_set_handler) {
> +        trace_qemu_boot_set(boot_order);
>          error_setg(errp, "no function defined to set boot device list for"
>                           " this architecture");
>          return;
> 
> And I see this now in qemu's monitor. Still I don't see error message.

That's because NULL is passed to this function in restore_boot_order()
the error is ignored (commit f183993). I have seen the previous conversation
about your patch serials. And I think this is the reason which
you moved machine->boot_order = boot_order before
checking boot_set_handler variable based on Alexander's
suggestion, right? But I think this is not a good idea.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Dinar Valeev]

On 01/28/2015 02:48 AM, Gonglei wrote:
> On 2015/1/27 18:49, Dinar Valeev wrote:
> 
>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>
>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>
>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>
>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>> got changed on reset.
>>>>>>
>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>> ---
>>>>>>  bootdevice.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>> index 5914417..4f11a06 100644
>>>>>> --- a/bootdevice.c
>>>>>> +++ b/bootdevice.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include "qapi/visitor.h"
>>>>>>  #include "qemu/error-report.h"
>>>>>>  #include "hw/hw.h"
>>>>>> +#include "hw/boards.h"
>>>>>>  
>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>  
>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>> +    machine->boot_order = boot_order;
>>>>>>  
>>>>>>      if (!boot_set_handler) {
>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>
>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>  will return error.
>>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>>
>>> That's interesting. Does this function be called?
>> Yes, then simply returns.
>>> Would you debug it by setting a breakpoint ?
>> I added a trace event.
>>      if (!boot_set_handler) {
>> +        trace_qemu_boot_set(boot_order);
>>          error_setg(errp, "no function defined to set boot device list for"
>>                           " this architecture");
>>          return;
>>
>> And I see this now in qemu's monitor. Still I don't see error message.
> 
> That's because NULL is passed to this function in restore_boot_order()
> the error is ignored (commit f183993). I have seen the previous conversation
> about your patch serials. And I think this is the reason which
> you moved machine->boot_order = boot_order before
> checking boot_set_handler variable based on Alexander's
> suggestion, right? But I think this is not a good idea.
Yes

Any proposal how this can be done differently?

It seems I'm almost alone who wants -boot once=X option to get fixed for sPAPR. We use it in test automation, and we need to be sure that we boot from hard disk once installation is done.

Thanks,
Dinar
> 
> Regards,
> -Gonglei
> 
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Gonglei]

On 2015/1/29 6:22, Dinar Valeev wrote:

> On 01/28/2015 02:48 AM, Gonglei wrote:
>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>
>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>
>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>
>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>
>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>> got changed on reset.
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>> ---
>>>>>>>  bootdevice.c | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>> index 5914417..4f11a06 100644
>>>>>>> --- a/bootdevice.c
>>>>>>> +++ b/bootdevice.c
>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>  #include "qapi/visitor.h"
>>>>>>>  #include "qemu/error-report.h"
>>>>>>>  #include "hw/hw.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>>  
>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>  
>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>  
>>>>>>>      if (!boot_set_handler) {
>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>
>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>  will return error.
>>>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>>>
>>>> That's interesting. Does this function be called?
>>> Yes, then simply returns.
>>>> Would you debug it by setting a breakpoint ?
>>> I added a trace event.
>>>      if (!boot_set_handler) {
>>> +        trace_qemu_boot_set(boot_order);
>>>          error_setg(errp, "no function defined to set boot device list for"
>>>                           " this architecture");
>>>          return;
>>>
>>> And I see this now in qemu's monitor. Still I don't see error message.
>>
>> That's because NULL is passed to this function in restore_boot_order()
>> the error is ignored (commit f183993). I have seen the previous conversation
>> about your patch serials. And I think this is the reason which
>> you moved machine->boot_order = boot_order before
>> checking boot_set_handler variable based on Alexander's
>> suggestion, right? But I think this is not a good idea.
> Yes
> 
> Any proposal how this can be done differently?
> 

I have replied your previous thread. :)

Regards,
-Gonglei

> It seems I'm almost alone who wants -boot once=X option to get fixed for sPAPR. We use it in test automation, and we need to be sure that we boot from hard disk once installation is done.
> 
> Thanks,
> Dinar
>>
>> Regards,
>> -Gonglei
>>
>>
> 

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Markus Armbruster]

Dinar Valeev <dvaleev@suse.de> writes:

> On 01/28/2015 02:48 AM, Gonglei wrote:
>> On 2015/1/27 18:49, Dinar Valeev wrote:
>> 
>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>
>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>
>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>
>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>> got changed on reset.
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>> ---
>>>>>>>  bootdevice.c | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>> index 5914417..4f11a06 100644
>>>>>>> --- a/bootdevice.c
>>>>>>> +++ b/bootdevice.c
>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>  #include "qapi/visitor.h"
>>>>>>>  #include "qemu/error-report.h"
>>>>>>>  #include "hw/hw.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>>  
>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>  
>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>  
>>>>>>>      if (!boot_set_handler) {
>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>
>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>  will return error.
>>>>> No, I set boot_order on each machine reset. My tests are showing
>>>>> it works without an error.
>>>>
>>>> That's interesting. Does this function be called?
>>> Yes, then simply returns.
>>>> Would you debug it by setting a breakpoint ?
>>> I added a trace event.
>>>      if (!boot_set_handler) {
>>> +        trace_qemu_boot_set(boot_order);
>>>          error_setg(errp, "no function defined to set boot device list for"
>>>                           " this architecture");
>>>          return;
>>>
>>> And I see this now in qemu's monitor. Still I don't see error message.
>> 
>> That's because NULL is passed to this function in restore_boot_order()
>> the error is ignored (commit f183993). I have seen the previous conversation
>> about your patch serials. And I think this is the reason which
>> you moved machine->boot_order = boot_order before
>> checking boot_set_handler variable based on Alexander's
>> suggestion, right? But I think this is not a good idea.
> Yes
>
> Any proposal how this can be done differently?
>
> It seems I'm almost alone who wants -boot once=X option to get fixed
> for sPAPR. We use it in test automation, and we need to be sure that
> we boot from hard disk once installation is done.

The crash is a bug, and bugs need fixing.

You first patch looked okay to me.  I suggested dropping the null check
in spapr_create_fdt_skel(), because I feel it's papering over the bug
you fix.

This isn't a review of your second attempt, it's encouragement.  I
haven't looked at this version, nor have I thought through Alex's idea
to patch machine->boot_order in qemu_boot_set().

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Alexander Graf]

On 29.01.15 08:48, Markus Armbruster wrote:
> Dinar Valeev <dvaleev@suse.de> writes:
> 
>> On 01/28/2015 02:48 AM, Gonglei wrote:
>>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>>
>>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>>
>>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>>
>>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>>
>>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>>> got changed on reset.
>>>>>>>>
>>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>>> ---
>>>>>>>>  bootdevice.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>>> index 5914417..4f11a06 100644
>>>>>>>> --- a/bootdevice.c
>>>>>>>> +++ b/bootdevice.c
>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>  #include "qapi/visitor.h"
>>>>>>>>  #include "qemu/error-report.h"
>>>>>>>>  #include "hw/hw.h"
>>>>>>>> +#include "hw/boards.h"
>>>>>>>>  
>>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>>  
>>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>>  
>>>>>>>>      if (!boot_set_handler) {
>>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>>
>>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>>  will return error.
>>>>>> No, I set boot_order on each machine reset. My tests are showing
>>>>>> it works without an error.
>>>>>
>>>>> That's interesting. Does this function be called?
>>>> Yes, then simply returns.
>>>>> Would you debug it by setting a breakpoint ?
>>>> I added a trace event.
>>>>      if (!boot_set_handler) {
>>>> +        trace_qemu_boot_set(boot_order);
>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>                           " this architecture");
>>>>          return;
>>>>
>>>> And I see this now in qemu's monitor. Still I don't see error message.
>>>
>>> That's because NULL is passed to this function in restore_boot_order()
>>> the error is ignored (commit f183993). I have seen the previous conversation
>>> about your patch serials. And I think this is the reason which
>>> you moved machine->boot_order = boot_order before
>>> checking boot_set_handler variable based on Alexander's
>>> suggestion, right? But I think this is not a good idea.
>> Yes
>>
>> Any proposal how this can be done differently?
>>
>> It seems I'm almost alone who wants -boot once=X option to get fixed
>> for sPAPR. We use it in test automation, and we need to be sure that
>> we boot from hard disk once installation is done.
> 
> The crash is a bug, and bugs need fixing.
> 
> You first patch looked okay to me.  

I'd rather not modify the "base fdt" ;). Today we have a pretty simple
layout that does

base fdt -> init
runtime changes -> reset

I'd prefer to keep it that way, so over the lifetime of a VM, the base
fdt shouldn't change.

> I suggested dropping the null check
> in spapr_create_fdt_skel(), because I feel it's papering over the bug
> you fix.
> 
> This isn't a review of your second attempt, it's encouragement.  I
> haven't looked at this version, nor have I thought through Alex's idea
> to patch machine->boot_order in qemu_boot_set().

If people are against it (though I can't see why), we can also move the
logic to spapr.c and change the machine->boot_order in a local callback
to set_boot_handler().


Alex

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Alexander Graf]

On 28.01.15 02:48, Gonglei wrote:
> On 2015/1/27 18:49, Dinar Valeev wrote:
> 
>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>
>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>
>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>
>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>> got changed on reset.
>>>>>>
>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>> ---
>>>>>>  bootdevice.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>> index 5914417..4f11a06 100644
>>>>>> --- a/bootdevice.c
>>>>>> +++ b/bootdevice.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include "qapi/visitor.h"
>>>>>>  #include "qemu/error-report.h"
>>>>>>  #include "hw/hw.h"
>>>>>> +#include "hw/boards.h"
>>>>>>  
>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>  
>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>> +    machine->boot_order = boot_order;
>>>>>>  
>>>>>>      if (!boot_set_handler) {
>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>
>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>  will return error.
>>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>>
>>> That's interesting. Does this function be called?
>> Yes, then simply returns.
>>> Would you debug it by setting a breakpoint ?
>> I added a trace event.
>>      if (!boot_set_handler) {
>> +        trace_qemu_boot_set(boot_order);
>>          error_setg(errp, "no function defined to set boot device list for"
>>                           " this architecture");
>>          return;
>>
>> And I see this now in qemu's monitor. Still I don't see error message.
> 
> That's because NULL is passed to this function in restore_boot_order()
> the error is ignored (commit f183993). I have seen the previous conversation
> about your patch serials. And I think this is the reason which
> you moved machine->boot_order = boot_order before
> checking boot_set_handler variable based on Alexander's
> suggestion, right? But I think this is not a good idea.

Why is it not a good idea? The check is only checking whether there are
callbacks. The boot order changes nevertheless.


Alex

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Gonglei]

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

> 
> 
> On 28.01.15 02:48, Gonglei wrote:
>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>
>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>
>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>
>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>
>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>> got changed on reset.
>>>>>>>
>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>> ---
>>>>>>>  bootdevice.c | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>> index 5914417..4f11a06 100644
>>>>>>> --- a/bootdevice.c
>>>>>>> +++ b/bootdevice.c
>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>  #include "qapi/visitor.h"
>>>>>>>  #include "qemu/error-report.h"
>>>>>>>  #include "hw/hw.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>>  
>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>  
>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>  
>>>>>>>      if (!boot_set_handler) {
>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>
>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>  will return error.
>>>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>>>
>>>> That's interesting. Does this function be called?
>>> Yes, then simply returns.
>>>> Would you debug it by setting a breakpoint ?
>>> I added a trace event.
>>>      if (!boot_set_handler) {
>>> +        trace_qemu_boot_set(boot_order);
>>>          error_setg(errp, "no function defined to set boot device list for"
>>>                           " this architecture");
>>>          return;
>>>
>>> And I see this now in qemu's monitor. Still I don't see error message.
>>
>> That's because NULL is passed to this function in restore_boot_order()
>> the error is ignored (commit f183993). I have seen the previous conversation
>> about your patch serials. And I think this is the reason which
>> you moved machine->boot_order = boot_order before
>> checking boot_set_handler variable based on Alexander's
>> suggestion, right? But I think this is not a good idea.
> 
> Why is it not a good idea? The check is only checking whether there are
> callbacks. The boot order changes nevertheless.
> 

I mean we can't simply ignore this error. If you don't use boot_set_handler
but machine->boot_order then the check should not report error.

Something like this:

diff --git a/bootdevice.c b/bootdevice.c
index c3a010c..98ed2d2 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp)
 {
     Error *local_err = NULL;

-    if (!boot_set_handler) {
-        error_setg(errp, "no function defined to set boot device list for"
-                         " this architecture");
-        return;
-    }
-
     validate_bootdevices(boot_order, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }

-    boot_set_handler(boot_set_opaque, boot_order, errp);
+    if (boot_set_handler) {
+        boot_set_handler(boot_set_opaque, boot_order, errp);
+    }
 }

 void validate_bootdevices(const char *devices, Error **errp)

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] bootdevice: update boot_order in MachineState

[Alexander Graf]

On 29.01.15 12:08, Gonglei wrote:
> On 2015/1/29 18:55, Alexander Graf wrote:
> 
>>
>>
>> On 28.01.15 02:48, Gonglei wrote:
>>> On 2015/1/27 18:49, Dinar Valeev wrote:
>>>
>>>> On 01/27/2015 10:18 AM, Gonglei wrote:
>>>>> On 2015/1/27 16:57, Dinar Valeev wrote:
>>>>>
>>>>>> On 01/27/2015 03:51 AM, Gonglei wrote:
>>>>>>> On 2015/1/27 7:52, dvaleev@suse.de wrote:
>>>>>>>
>>>>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>>>>
>>>>>>>> on sPAPR we need to update boot_order in MachineState in case it
>>>>>>>> got changed on reset.
>>>>>>>>
>>>>>>>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>>>>>>>> ---
>>>>>>>>  bootdevice.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/bootdevice.c b/bootdevice.c
>>>>>>>> index 5914417..4f11a06 100644
>>>>>>>> --- a/bootdevice.c
>>>>>>>> +++ b/bootdevice.c
>>>>>>>> @@ -26,6 +26,7 @@
>>>>>>>>  #include "qapi/visitor.h"
>>>>>>>>  #include "qemu/error-report.h"
>>>>>>>>  #include "hw/hw.h"
>>>>>>>> +#include "hw/boards.h"
>>>>>>>>  
>>>>>>>>  typedef struct FWBootEntry FWBootEntry;
>>>>>>>>  
>>>>>>>> @@ -50,6 +51,8 @@ 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());
>>>>>>>> +    machine->boot_order = boot_order;
>>>>>>>>  
>>>>>>>>      if (!boot_set_handler) {
>>>>>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>>>>
>>>>>>> Have you registered boot set handler on ppc/sPAPR platform by calling
>>>>>>> qemu_register_boot_set()? Otherwise qemu_boot_set function
>>>>>>>  will return error.
>>>>>> No, I set boot_order on each machine reset. My tests are showing it works without an error.
>>>>>
>>>>> That's interesting. Does this function be called?
>>>> Yes, then simply returns.
>>>>> Would you debug it by setting a breakpoint ?
>>>> I added a trace event.
>>>>      if (!boot_set_handler) {
>>>> +        trace_qemu_boot_set(boot_order);
>>>>          error_setg(errp, "no function defined to set boot device list for"
>>>>                           " this architecture");
>>>>          return;
>>>>
>>>> And I see this now in qemu's monitor. Still I don't see error message.
>>>
>>> That's because NULL is passed to this function in restore_boot_order()
>>> the error is ignored (commit f183993). I have seen the previous conversation
>>> about your patch serials. And I think this is the reason which
>>> you moved machine->boot_order = boot_order before
>>> checking boot_set_handler variable based on Alexander's
>>> suggestion, right? But I think this is not a good idea.
>>
>> Why is it not a good idea? The check is only checking whether there are
>> callbacks. The boot order changes nevertheless.
>>
> 
> I mean we can't simply ignore this error. If you don't use boot_set_handler
> but machine->boot_order then the check should not report error.
> 
> Something like this:

Oh, I see. Yes, I agree there :).


Alex