[Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[Cédric Le Goater]

Devices such as the BT or serial devices require a valid
"interrupt-parent" phandle in the device tree and it is currently
empty (0x0). It was not a problem until now but since OpenFirmare
started using a recent libdft (>= 1.4.7), petitboot fails to boot the
system image with error :

   dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC

Provide a phandle for the LPC bus.

Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 36f57479a1f5..2deceecdccb5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -431,9 +431,14 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
         .fdt = fdt,
         .offset = isa_offset,
     };
+    uint32_t phandle;
 
     _FDT((fdt_setprop(fdt, isa_offset, "primary", NULL, 0)));
 
+    phandle = qemu_fdt_alloc_phandle(fdt);
+    assert(phandle > 0);
+    _FDT((fdt_setprop_cell(fdt, isa_offset, "phandle", phandle)));
+
     /* ISA devices are not necessarily parented to the ISA bus so we
      * can not use object_child_foreach() */
     qbus_walk_children(BUS(pnv->isa_bus), pnv_dt_isa_device, NULL, NULL, NULL,
-- 
2.21.0

Re: [Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[David Gibson]

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

On Tue, Jul 23, 2019 at 11:01:38AM +0200, Cédric Le Goater wrote:
> Devices such as the BT or serial devices require a valid
> "interrupt-parent" phandle in the device tree and it is currently
> empty (0x0). It was not a problem until now but since OpenFirmare
> started using a recent libdft (>= 1.4.7), petitboot fails to boot the
> system image with error :
> 
>    dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC
> 
> Provide a phandle for the LPC bus.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

I've applied this, since it looks to be correct.

But.. can you connect the dots for me in how this being missing
results in a BADMAGIC error??

> ---
>  hw/ppc/pnv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 36f57479a1f5..2deceecdccb5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -431,9 +431,14 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
>          .fdt = fdt,
>          .offset = isa_offset,
>      };
> +    uint32_t phandle;
>  
>      _FDT((fdt_setprop(fdt, isa_offset, "primary", NULL, 0)));
>  
> +    phandle = qemu_fdt_alloc_phandle(fdt);
> +    assert(phandle > 0);
> +    _FDT((fdt_setprop_cell(fdt, isa_offset, "phandle", phandle)));
> +
>      /* ISA devices are not necessarily parented to the ISA bus so we
>       * can not use object_child_foreach() */
>      qbus_walk_children(BUS(pnv->isa_bus), pnv_dt_isa_device, NULL, NULL, NULL,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[Cédric Le Goater]

On 24/07/2019 05:23, David Gibson wrote:
> On Tue, Jul 23, 2019 at 11:01:38AM +0200, Cédric Le Goater wrote:
>> Devices such as the BT or serial devices require a valid
>> "interrupt-parent" phandle in the device tree and it is currently
>> empty (0x0). It was not a problem until now but since OpenFirmare
>> started using a recent libdft (>= 1.4.7), petitboot fails to boot the
>> system image with error :
>>
>>    dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC
>>
>> Provide a phandle for the LPC bus.
>>
>> Suggested-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> I've applied this, since it looks to be correct.
> 
> But.. can you connect the dots for me in how this being missing
> results in a BADMAGIC error??

Some binary called by petitboot segfaults when trying to kexec an image on 
a system with a bogus DT (generated by QEMU). I don't know exactly which one 
as I only see the error message above and the segv message in dmesg

I didn't notice before because I was using an old open-power-v2.1 image. I have
switched to more recent ones now :

  https://openpower.xyz/job/openpower/job/openpower-op-build/label=slave,target=witherspoon/lastSuccessfulBuild/
  https://openpower.xyz/job/openpower/job/openpower-op-build/label=slave,target=palmetto/lastSuccessfulBuild/

Btw, where could we keep some images of reference ? I have a couple of patches 
which enables the QEMU PowerNV machine to boot directly from the PNOR using :
 
  -drive file=./witherspoon.pnor,format=raw,if=mtd

Thanks,

C. 

> 
>> ---
>>  hw/ppc/pnv.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 36f57479a1f5..2deceecdccb5 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -431,9 +431,14 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
>>          .fdt = fdt,
>>          .offset = isa_offset,
>>      };
>> +    uint32_t phandle;
>>  
>>      _FDT((fdt_setprop(fdt, isa_offset, "primary", NULL, 0)));
>>  
>> +    phandle = qemu_fdt_alloc_phandle(fdt);
>> +    assert(phandle > 0);
>> +    _FDT((fdt_setprop_cell(fdt, isa_offset, "phandle", phandle)));
>> +
>>      /* ISA devices are not necessarily parented to the ISA bus so we
>>       * can not use object_child_foreach() */
>>      qbus_walk_children(BUS(pnv->isa_bus), pnv_dt_isa_device, NULL, NULL, NULL,
> 

Re: [Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[David Gibson]

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

On Wed, Jul 24, 2019 at 09:11:54AM +0200, Cédric Le Goater wrote:
> On 24/07/2019 05:23, David Gibson wrote:
> > On Tue, Jul 23, 2019 at 11:01:38AM +0200, Cédric Le Goater wrote:
> >> Devices such as the BT or serial devices require a valid
> >> "interrupt-parent" phandle in the device tree and it is currently
> >> empty (0x0). It was not a problem until now but since OpenFirmare
> >> started using a recent libdft (>= 1.4.7), petitboot fails to boot the
> >> system image with error :
> >>
> >>    dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC
> >>
> >> Provide a phandle for the LPC bus.
> >>
> >> Suggested-by: Greg Kurz <groug@kaod.org>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > 
> > I've applied this, since it looks to be correct.
> > 
> > But.. can you connect the dots for me in how this being missing
> > results in a BADMAGIC error??
> 
> Some binary called by petitboot segfaults when trying to kexec an image on 
> a system with a bogus DT (generated by QEMU). I don't know exactly which one 
> as I only see the error message above and the segv message in dmesg

Ok, I'm still not seeing how that gets you to a BADMAGIC error.

> 
> I didn't notice before because I was using an old open-power-v2.1 image. I have
> switched to more recent ones now :
> 
>   https://openpower.xyz/job/openpower/job/openpower-op-build/label=slave,target=witherspoon/lastSuccessfulBuild/
>   https://openpower.xyz/job/openpower/job/openpower-op-build/label=slave,target=palmetto/lastSuccessfulBuild/
> 
> Btw, where could we keep some images of reference ? I have a couple of patches 
> which enables the QEMU PowerNV machine to boot directly from the PNOR using :
>  
>   -drive file=./witherspoon.pnor,format=raw,if=mtd
> 
> Thanks,
> 
> C. 
> 
> > 
> >> ---
> >>  hw/ppc/pnv.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 36f57479a1f5..2deceecdccb5 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -431,9 +431,14 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
> >>          .fdt = fdt,
> >>          .offset = isa_offset,
> >>      };
> >> +    uint32_t phandle;
> >>  
> >>      _FDT((fdt_setprop(fdt, isa_offset, "primary", NULL, 0)));
> >>  
> >> +    phandle = qemu_fdt_alloc_phandle(fdt);
> >> +    assert(phandle > 0);
> >> +    _FDT((fdt_setprop_cell(fdt, isa_offset, "phandle", phandle)));
> >> +
> >>      /* ISA devices are not necessarily parented to the ISA bus so we
> >>       * can not use object_child_foreach() */
> >>      qbus_walk_children(BUS(pnv->isa_bus), pnv_dt_isa_device, NULL, NULL, NULL,
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[Cédric Le Goater]

>>> But.. can you connect the dots for me in how this being missing
>>> results in a BADMAGIC error??
>>
>> Some binary called by petitboot segfaults when trying to kexec an image on 
>> a system with a bogus DT (generated by QEMU). I don't know exactly which one 
>> as I only see the error message above and the segv message in dmesg
> 
> Ok, I'm still not seeing how that gets you to a BADMAGIC error.

It should be that guy :

  https://github.com/open-power/petitboot/blob/master/utils/hooks/30-dtb-updates.c

C. 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[Amol Surati]

On Wed, Jul 24, 2019 at 06:57:30PM +1000, David Gibson wrote:
> On Wed, Jul 24, 2019 at 09:11:54AM +0200, Cédric Le Goater wrote:
> > On 24/07/2019 05:23, David Gibson wrote:
> > > On Tue, Jul 23, 2019 at 11:01:38AM +0200, Cédric Le Goater wrote:
> > >> Devices such as the BT or serial devices require a valid
> > >> "interrupt-parent" phandle in the device tree and it is currently
> > >> empty (0x0). It was not a problem until now but since OpenFirmare
> > >> started using a recent libdft (>= 1.4.7), petitboot fails to boot the
> > >> system image with error :
> > >>
> > >>    dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC
> > >>
> > >> Provide a phandle for the LPC bus.
> > >>
> > >> Suggested-by: Greg Kurz <groug@kaod.org>
> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > 
> > > I've applied this, since it looks to be correct.
> > > 
> > > But.. can you connect the dots for me in how this being missing
> > > results in a BADMAGIC error??
> > 
> > Some binary called by petitboot segfaults when trying to kexec an image on 
> > a system with a bogus DT (generated by QEMU). I don't know exactly which one 
> > as I only see the error message above and the segv message in dmesg
> 
> Ok, I'm still not seeing how that gets you to a BADMAGIC error.

If I may interject, as this patch is related to the qemu bug:
https://bugs.launchpad.net/qemu/+bug/1826827.

The error is printed by dtc_resize in kexec.c from kexec-lite
(antonblanchard/kexec-lite).

There are two places where dtc_resize is called -
(1) initialize_fdt, when kexec is passed a dtb file.
(2) fdt_from_fs, when kexec must make dtc read /proc/device-tree to form
    a dtb.

If initialize_fdt is called with a file which is an invalid dtb, the
dtc_resize prints the FDT_ERR_BADMAGIC error.

Bug# 1826827 shows that dtc is one application that does
crash, although through the firing of an assertion, in the absence of
the mentioned properties. (fix to avoid the crash already checked into
dtc upstream, commit 8f69567622; to be released with dtc-v1.5.1).

Assuming that the crashing app (it is not known here what it is) is
supposed to create a dtb for kexec, and its crash leaves behind an
incomplete/invalid dtb file, the initialize_fdt might receive an invalid
dtb.


Another possibility for that error exists within the fdt_from_fs function,
but that needs a version of kexec-lite at least 5 years old, which is
unlikely to be used here I guess.



If this patch fixes both the crash and the error "dtc_resize: ....",
it is likely that dtc (or anything else which depends on libfdt) was the
cause of the crash, with dtc/libfdt version being < g8f69567622.


Thanks,
-amol

Re: [Qemu-devel] [Qemu-ppc] [PATCH] ppc/pnv: Generate phandle for the "interrupt-parent" property

[David Gibson]

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

On Wed, Jul 24, 2019 at 08:25:04PM +0530, Amol Surati wrote:
> On Wed, Jul 24, 2019 at 06:57:30PM +1000, David Gibson wrote:
> > On Wed, Jul 24, 2019 at 09:11:54AM +0200, Cédric Le Goater wrote:
> > > On 24/07/2019 05:23, David Gibson wrote:
> > > > On Tue, Jul 23, 2019 at 11:01:38AM +0200, Cédric Le Goater wrote:
> > > >> Devices such as the BT or serial devices require a valid
> > > >> "interrupt-parent" phandle in the device tree and it is currently
> > > >> empty (0x0). It was not a problem until now but since OpenFirmare
> > > >> started using a recent libdft (>= 1.4.7), petitboot fails to boot the
> > > >> system image with error :
> > > >>
> > > >>    dtc_resize: fdt_open_into returned FDT_ERR_BADMAGIC
> > > >>
> > > >> Provide a phandle for the LPC bus.
> > > >>
> > > >> Suggested-by: Greg Kurz <groug@kaod.org>
> > > >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > 
> > > > I've applied this, since it looks to be correct.
> > > > 
> > > > But.. can you connect the dots for me in how this being missing
> > > > results in a BADMAGIC error??
> > > 
> > > Some binary called by petitboot segfaults when trying to kexec an image on 
> > > a system with a bogus DT (generated by QEMU). I don't know exactly which one 
> > > as I only see the error message above and the segv message in dmesg
> > 
> > Ok, I'm still not seeing how that gets you to a BADMAGIC error.
> 
> If I may interject, as this patch is related to the qemu bug:
> https://bugs.launchpad.net/qemu/+bug/1826827.
> 
> The error is printed by dtc_resize in kexec.c from kexec-lite
> (antonblanchard/kexec-lite).
> 
> There are two places where dtc_resize is called -
> (1) initialize_fdt, when kexec is passed a dtb file.
> (2) fdt_from_fs, when kexec must make dtc read /proc/device-tree to form
>     a dtb.
> 
> If initialize_fdt is called with a file which is an invalid dtb, the
> dtc_resize prints the FDT_ERR_BADMAGIC error.
> 
> Bug# 1826827 shows that dtc is one application that does
> crash, although through the firing of an assertion, in the absence of
> the mentioned properties. (fix to avoid the crash already checked into
> dtc upstream, commit 8f69567622; to be released with dtc-v1.5.1).
> 
> Assuming that the crashing app (it is not known here what it is) is
> supposed to create a dtb for kexec, and its crash leaves behind an
> incomplete/invalid dtb file, the initialize_fdt might receive an invalid
> dtb.

Ok, thanks.  That's what I was after.

> 
> 
> Another possibility for that error exists within the fdt_from_fs function,
> but that needs a version of kexec-lite at least 5 years old, which is
> unlikely to be used here I guess.
> 
> 
> 
> If this patch fixes both the crash and the error "dtc_resize: ....",
> it is likely that dtc (or anything else which depends on libfdt) was the
> cause of the crash, with dtc/libfdt version being < g8f69567622.
> 
> 
> Thanks,
> -amol
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]