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

[Greg Kurz]

On Wed, 11 Sep 2019 14:04:50 +1000
David Gibson <address@hidden> wrote:

> From: Alexey Kardashevskiy <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: David Gibson <address@hidden>
> ---
>  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),