Re: [Qemu-devel] [PATCH 5/7] spapr: Do not put empty properties for -kernel/-initrd/-append

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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