[Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[dvaleev]

From: Dinar Valeev <dvaleev@suse.com>

In order to use -boot once=X option we need to have default list
 where restore to on reset.

Signed-off-by: Dinar Valeev <dvaleev@suse.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..3d2cfa3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
     mc->max_cpus = MAX_CPUS;
     mc->no_parallel = 1;
-    mc->default_boot_order = NULL;
+    mc->default_boot_order = "cdn";
     mc->kvm_type = spapr_kvm_type;
     mc->has_dynamic_sysbus = true;
 
-- 
2.1.2

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

[dvaleev]

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 (boot_menu) {
         _FDT((fdt_property_cell(fdt, "qemu,boot-menu", boot_menu)));
     }
-- 
2.1.2

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

[Alexander Graf]

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


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

[Dinar Valeev]

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

[Dinar Valeev]

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"

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

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


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

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

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

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

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

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

[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, 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).
>>
> 
> 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

[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, 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).
>>>
>>
>> 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

[Alexey Kardashevskiy]

[-- Attachment #1: Type: text/plain, Size: 3820 bytes --]

On Fri, Jan 30, 2015 at 12:00 AM, Gonglei <arei.gonglei@huawei.com> 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, 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).
> >>>
> >>
> >> 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

[-- Attachment #2: Type: text/html, Size: 6692 bytes --]

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

[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 1/2] hw/ppc/spapr.c Set default boot order

[Alexander Graf]

On 23.01.15 23:51, dvaleev@suse.de wrote:
> From: Dinar Valeev <dvaleev@suse.com>
> 
> In order to use -boot once=X option we need to have default list
>  where restore to on reset.
> 
> Signed-off-by: Dinar Valeev <dvaleev@suse.com>

Alexey, Nijunj, where is the default boot order stored usually? Is "cdn"
an accurate equivalent?


Alex

> ---
>  hw/ppc/spapr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b560459..3d2cfa3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1733,7 +1733,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_SCSI;
>      mc->max_cpus = MAX_CPUS;
>      mc->no_parallel = 1;
> -    mc->default_boot_order = NULL;
> +    mc->default_boot_order = "cdn";
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
>  
> 

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 23.01.15 23:51, dvaleev@suse.de wrote:
>> From: Dinar Valeev <dvaleev@suse.com>
>> 
>> In order to use -boot once=X option we need to have default list
>>  where restore to on reset.
>> 
>> Signed-off-by: Dinar Valeev <dvaleev@suse.com>
>
> Alexey, Nijunj, where is the default boot order stored usually?

When -boot argument is not present, the default boot order is NULL for
spapr, and SLOF decides to boot from NVRAM set variable or discovered
disk.

> Is "cdn" an accurate equivalent?

No, we have changed that to NULL.

http://comments.gmane.org/gmane.comp.emulators.qemu/187318

Regards,
Nikunj

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Markus Armbruster]

dvaleev@suse.de writes:

> From: Dinar Valeev <dvaleev@suse.com>
>
> In order to use -boot once=X option we need to have default list
>  where restore to on reset.

Really?  What happens without this patch?

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Dinar Valeev]

On 01/26/2015 10:11 AM, Markus Armbruster wrote:
> dvaleev@suse.de writes:
>
>> From: Dinar Valeev <dvaleev@suse.com>
>>
>> In order to use -boot once=X option we need to have default list
>>   where restore to on reset.
>
> Really?  What happens without this patch?
>
qemu segfaults on reset.
0 > reset-all  Segmentation fault

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Markus Armbruster]

Dinar Valeev <dvaleev@suse.de> writes:

> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>> dvaleev@suse.de writes:
>>
>>> From: Dinar Valeev <dvaleev@suse.com>
>>>
>>> In order to use -boot once=X option we need to have default list
>>>   where restore to on reset.
>>
>> Really?  What happens without this patch?
>>
> qemu segfaults on reset.
> 0 > reset-all  Segmentation fault

Next time, include a backtrace, please.

Here's what I think happens.

Boot order comes from --boot parameter once, order, or else the machine
type's .default_boot_order.  The latter is null for you.

It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
sets qemu,boot-device in the FDT to it, but only when it isn't null.

If it comes from parameter once, we additionally register a reset
handler to switch it to parameter order or else .default_boot_order on
reset.  If you specify once, but not order, this is null for you.

On reset, reset handler restore_boot_order() runs.  Unlike
spapr_create_fdt_skel(), it doesn't check for null, and crashes in
validate_bootdevices().

Correct?

For me, a null .default_boot_order means "machine type does not support
boot order" (this is how commit c165473 treats it).  Arguably, --boot
order and once should be rejected then.

If I understand you correctly, your machine type does support boot
order.  Giving it a non-null .default_boot_order makes sense then.  The
appropriate value depends on firmware.  It could even be "".

The null check in spapr_create_fdt_skel() looks superfluous then.
Consider dropping it.

Makes sense?

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Dinar Valeev]

On 01/26/2015 01:37 PM, Markus Armbruster wrote:
> Dinar Valeev <dvaleev@suse.de> writes:
>
>> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>>> dvaleev@suse.de writes:
>>>
>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>
>>>> In order to use -boot once=X option we need to have default list
>>>>    where restore to on reset.
>>>
>>> Really?  What happens without this patch?
>>>
>> qemu segfaults on reset.
>> 0 > reset-all  Segmentation fault
>
> Next time, include a backtrace, please.
Ok, sorry for that.
>
> Here's what I think happens.
>
> Boot order comes from --boot parameter once, order, or else the machine
> type's .default_boot_order.  The latter is null for you.
>
> It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
> sets qemu,boot-device in the FDT to it, but only when it isn't null.
>
> If it comes from parameter once, we additionally register a reset
> handler to switch it to parameter order or else .default_boot_order on
> reset.  If you specify once, but not order, this is null for you.
>
> On reset, reset handler restore_boot_order() runs.  Unlike
> spapr_create_fdt_skel(), it doesn't check for null, and crashes in
> validate_bootdevices().
>
> Correct?
Yes

qemu_register_boot_set is implemented in PATCH 2/2. on reset boot_device 
is restored to NULL
>
> For me, a null .default_boot_order means "machine type does not support
> boot order" (this is how commit c165473 treats it).  Arguably, --boot
> order and once should be rejected then.
AFICS SLOF handles qemu,boot-device as boot device, if nothing passed 
then it goes disk, cdrom, network. Which is the same as "cdn" list.

> If I understand you correctly, your machine type does support boot
> order.  Giving it a non-null .default_boot_order makes sense then.  The
> appropriate value depends on firmware.  It could even be "".
>
> The null check in spapr_create_fdt_skel() looks superfluous then.
> Consider dropping it.
>
> Makes sense?
>

Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr.c Set default boot order

[Alexander Graf]

On 01/26/2015 01:57 PM, Dinar Valeev wrote:
> On 01/26/2015 01:37 PM, Markus Armbruster wrote:
>> Dinar Valeev <dvaleev@suse.de> writes:
>>
>>> On 01/26/2015 10:11 AM, Markus Armbruster wrote:
>>>> dvaleev@suse.de writes:
>>>>
>>>>> From: Dinar Valeev <dvaleev@suse.com>
>>>>>
>>>>> In order to use -boot once=X option we need to have default list
>>>>>    where restore to on reset.
>>>>
>>>> Really?  What happens without this patch?
>>>>
>>> qemu segfaults on reset.
>>> 0 > reset-all  Segmentation fault
>>
>> Next time, include a backtrace, please.
> Ok, sorry for that.
>>
>> Here's what I think happens.
>>
>> Boot order comes from --boot parameter once, order, or else the machine
>> type's .default_boot_order.  The latter is null for you.
>>
>> It gets passed via ppc_spapr_init() to spapr_create_fdt_skel(), which
>> sets qemu,boot-device in the FDT to it, but only when it isn't null.
>>
>> If it comes from parameter once, we additionally register a reset
>> handler to switch it to parameter order or else .default_boot_order on
>> reset.  If you specify once, but not order, this is null for you.
>>
>> On reset, reset handler restore_boot_order() runs.  Unlike
>> spapr_create_fdt_skel(), it doesn't check for null, and crashes in
>> validate_bootdevices().
>>
>> Correct?
> Yes
>
> qemu_register_boot_set is implemented in PATCH 2/2. on reset 
> boot_device is restored to NULL
>>
>> For me, a null .default_boot_order means "machine type does not support
>> boot order" (this is how commit c165473 treats it).  Arguably, --boot
>> order and once should be rejected then.
> AFICS SLOF handles qemu,boot-device as boot device, if nothing passed 
> then it goes disk, cdrom, network. Which is the same as "cdn" list.

That's not entirely true. If SLOF doesn't see qemu,boot-device, it first 
goes via its NVRAM configured boot list and only if nothing is there it 
falls back to "cdn".

Maybe the actual appropriate thing would be "mcdn" with m meaning "NVRAM".


Alex

>
>> If I understand you correctly, your machine type does support boot
>> order.  Giving it a non-null .default_boot_order makes sense then.  The
>> appropriate value depends on firmware.  It could even be "".
>>
>> The null check in spapr_create_fdt_skel() looks superfluous then.
>> Consider dropping it.
>>
>> Makes sense?
>>
>