From: Alexey Kardashevskiy <aik@ozlabs.ru>

We are going to use spapr_build_fdt() for the boot time FDT and as an
update for SLOF during handling of H_CAS. SLOF will apply all properties
from the QEMU's FDT which is usually ok unless there are properties
changed by grub or guest kernel. The properties are:
bootargs, linux,initrd-start, linux,initrd-end, linux,stdout-path,
linux,rtas-base, linux,rtas-entry. Resetting those during CAS will most
likely cause grub failure.

This only creates such properties if we are booting with "-kernel" and
"-initrd" so they won't get included into the DT update blob and
therefore the guest is more likely to boot successfully.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d072c2aa3d..d18744268f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1177,11 +1177,16 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
 
     _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
 
-    _FDT(fdt_setprop_string(fdt, chosen, "bootargs", machine->kernel_cmdline));
-    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
-                          spapr->initrd_base));
-    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
-                          spapr->initrd_base + spapr->initrd_size));
+    if (machine->kernel_cmdline && machine->kernel_cmdline[0]) {
+        _FDT(fdt_setprop_string(fdt, chosen, "bootargs",
+                                machine->kernel_cmdline));
+    }
+    if (spapr->initrd_size) {
+        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
+                              spapr->initrd_base));
+        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
+                              spapr->initrd_base + spapr->initrd_size));
+    }
 
     if (spapr->kernel_size) {
         uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
-- 
2.21.0

On Wed, 11 Sep 2019 14:04:50 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> We are going to use spapr_build_fdt() for the boot time FDT and as an
> update for SLOF during handling of H_CAS. SLOF will apply all properties
> from the QEMU's FDT which is usually ok unless there are properties
> changed by grub or guest kernel. The properties are:
> bootargs, linux,initrd-start, linux,initrd-end, linux,stdout-path,
> linux,rtas-base, linux,rtas-entry. Resetting those during CAS will most
> likely cause grub failure.
> 

s/Resetting/Clearing ? They still get reset to the initial setup if "-kernel"
and "-initrd" were passed, but it is okay since neither grub, nor the guest
kernel is supposed to change them in this case, correct ?

> This only creates such properties if we are booting with "-kernel" and
> "-initrd" so they won't get included into the DT update blob and

so they won't get included {if we're not booting with "-kernel" ...}

> therefore the guest is more likely to boot successfully.
> 

Maybe rephrase like:

Don't create such properties if we're booting without "-kernel" and
"-initrd" ...

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d072c2aa3d..d18744268f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1177,11 +1177,16 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
>  
>      _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>  
> -    _FDT(fdt_setprop_string(fdt, chosen, "bootargs", machine->kernel_cmdline));
> -    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
> -                          spapr->initrd_base));
> -    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
> -                          spapr->initrd_base + spapr->initrd_size));
> +    if (machine->kernel_cmdline && machine->kernel_cmdline[0]) {

machine->kernel_cmdline cannot be NULL.

From vl.c:

    if (!kernel_cmdline) {
        kernel_cmdline = "";
        current_machine->kernel_cmdline = (char *)kernel_cmdline;
    }

Also this doesn't check if we're booting with -kernel but rather
that we're booting with -append ${some_not_empty_string}... what
about checking spapr->kernel_size, pretty much like you do for
the initrd ?

> +        _FDT(fdt_setprop_string(fdt, chosen, "bootargs",
> +                                machine->kernel_cmdline));
> +    }
> +    if (spapr->initrd_size) {
> +        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
> +                              spapr->initrd_base));
> +        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
> +                              spapr->initrd_base + spapr->initrd_size));
> +    }
>  
>      if (spapr->kernel_size) {
>          uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),

On 11/09/2019 18:46, Greg Kurz wrote:
> On Wed, 11 Sep 2019 14:04:50 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> From: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> We are going to use spapr_build_fdt() for the boot time FDT and as an
>> update for SLOF during handling of H_CAS. SLOF will apply all properties
>> from the QEMU's FDT which is usually ok unless there are properties
>> changed by grub or guest kernel. The properties are:
>> bootargs, linux,initrd-start, linux,initrd-end, linux,stdout-path,
>> linux,rtas-base, linux,rtas-entry. Resetting those during CAS will most
>> likely cause grub failure.
>>
> 
> s/Resetting/Clearing ? They still get reset to the initial setup if "-kernel"
> and "-initrd" were passed, but it is okay since neither grub, nor the guest
> kernel is supposed to change them in this case, correct ?

Correct.



>> This only creates such properties if we are booting with "-kernel" and
>> "-initrd" so they won't get included into the DT update blob and
> 
> so they won't get included {if we're not booting with "-kernel" ...}
> 
>> therefore the guest is more likely to boot successfully.
>>
> 
> Maybe rephrase like:
> 
> Don't create such properties if we're booting without "-kernel" and
> "-initrd" ...
> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/spapr.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d072c2aa3d..d18744268f 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1177,11 +1177,16 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
>>  
>>      _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
>>  
>> -    _FDT(fdt_setprop_string(fdt, chosen, "bootargs", machine->kernel_cmdline));
>> -    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
>> -                          spapr->initrd_base));
>> -    _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
>> -                          spapr->initrd_base + spapr->initrd_size));
>> +    if (machine->kernel_cmdline && machine->kernel_cmdline[0]) {
> 
> machine->kernel_cmdline cannot be NULL.
> 
> From vl.c:
> 
>     if (!kernel_cmdline) {
>         kernel_cmdline = "";
>         current_machine->kernel_cmdline = (char *)kernel_cmdline;
>     }

I do not see the point in having an empty string instead of NULL really
and probably one day somebody else will think the same so I prepared :)

> 
> Also this doesn't check if we're booting with -kernel but rather
> that we're booting with -append ${some_not_empty_string}... what
> about checking spapr->kernel_size, pretty much like you do for
> the initrd ?


We are preserving here "bootargs" which either comes from grub or
"-append" so I do just this. Having -kernel usually (always?) means
there is no grub which we are fixing here but this is just a consequence
of a weird command line.



>> +        _FDT(fdt_setprop_string(fdt, chosen, "bootargs",
>> +                                machine->kernel_cmdline));
>> +    }
>> +    if (spapr->initrd_size) {
>> +        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-start",
>> +                              spapr->initrd_base));
>> +        _FDT(fdt_setprop_cell(fdt, chosen, "linux,initrd-end",
>> +                              spapr->initrd_base + spapr->initrd_size));
>> +    }
>>  
>>      if (spapr->kernel_size) {
>>          uint64_t kprop[2] = { cpu_to_be64(KERNEL_LOAD_ADDR),
> 

-- 
Alexey