diff mbox series

[RFC] hw/ppc: Implement -dtb support for PowerNV

Message ID 20240731132235.887918-1-adityag@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series [RFC] hw/ppc: Implement -dtb support for PowerNV | expand

Commit Message

Aditya Gupta July 31, 2024, 1:22 p.m. UTC
Currently any device tree passed with -dtb option in QEMU, was ignored
by the PowerNV code.

Read and pass the passed -dtb to the kernel, thus enabling easier
debugging with custom DTBs.

The existing behaviour when -dtb is 'not' passed, is preserved as-is.

But when a '-dtb' is passed, it completely overrides any dtb nodes or
changes QEMU might have done, such as '-append' arguments to the kernel
(which are mentioned in /chosen/bootargs in the dtb), hence add warning
when -dtb is being used

Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>

---
This is an RFC patch, and hence might not be the final implementation,
though this current one is a solution which works
---
---
 hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

Comments

Daniel P. Berrangé July 31, 2024, 1:34 p.m. UTC | #1
On Wed, Jul 31, 2024 at 06:52:35PM +0530, Aditya Gupta wrote:
> Currently any device tree passed with -dtb option in QEMU, was ignored
> by the PowerNV code.
> 
> Read and pass the passed -dtb to the kernel, thus enabling easier
> debugging with custom DTBs.
> 
> The existing behaviour when -dtb is 'not' passed, is preserved as-is.
> 
> But when a '-dtb' is passed, it completely overrides any dtb nodes or
> changes QEMU might have done, such as '-append' arguments to the kernel
> (which are mentioned in /chosen/bootargs in the dtb), hence add warning
> when -dtb is being used
> 
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> 
> ---
> This is an RFC patch, and hence might not be the final implementation,
> though this current one is a solution which works
> ---
> ---
>  hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..12cc909b9e26 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>      PnvMachineState *pnv = PNV_MACHINE(machine);
>      IPMIBmc *bmc;
>      void *fdt;
> +    FILE *fdt_file;
> +    uint32_t fdt_size;
>  
>      qemu_devices_reset(reason);
>  
> @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>          }
>      }
>  
> -    fdt = pnv_dt_create(machine);
> +    if (machine->dtb) {
> +        warn_report("with manually passed dtb, some options like '-append'"
> +                " might ignored and the dtb passed will be used as-is");

Check whether append is actually set, and report an fatal error in
that case. 

>  
> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +        fdt = g_malloc0(FDT_MAX_SIZE);
> +
> +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> +        fdt_file = fopen(machine->dtb, "r");
> +        if (fdt_file != NULL) {
> +            fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file);
> +            if (ferror(fdt_file) != 0) {
> +                error_report("Could not load dtb '%s'",
> +                             machine->dtb);
> +                exit(1);
> +            }
> +
> +            /* mark end of the fdt buffer with '\0' */
> +            ((char *)fdt)[fdt_size] = '\0';
> +        }

Preferrable to use g_file_get_contents()

> +    } else {
> +        fdt = pnv_dt_create(machine);
> +
> +        /* Pack resulting tree */
> +        _FDT((fdt_pack(fdt)));
> +    }
>  
>      qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>      cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
> -- 
> 2.45.2
> 
> 

With regards,
Daniel
Aditya Gupta July 31, 2024, 1:51 p.m. UTC | #2
Hi Daniel,

Thank you for the review.

On 24/07/31 02:34PM, Daniel P. Berrangé wrote:
> On Wed, Jul 31, 2024 at 06:52:35PM +0530, Aditya Gupta wrote:
> > Currently any device tree passed with -dtb option in QEMU, was ignored
> > by the PowerNV code.
> > 
> > Read and pass the passed -dtb to the kernel, thus enabling easier
> > debugging with custom DTBs.
> > 
> > The existing behaviour when -dtb is 'not' passed, is preserved as-is.
> > 
> > But when a '-dtb' is passed, it completely overrides any dtb nodes or
> > changes QEMU might have done, such as '-append' arguments to the kernel
> > (which are mentioned in /chosen/bootargs in the dtb), hence add warning
> > when -dtb is being used
> > 
> > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> > 
> > ---
> > This is an RFC patch, and hence might not be the final implementation,
> > though this current one is a solution which works
> > ---
> > ---
> >  hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 3526852685b4..12cc909b9e26 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
> >      PnvMachineState *pnv = PNV_MACHINE(machine);
> >      IPMIBmc *bmc;
> >      void *fdt;
> > +    FILE *fdt_file;
> > +    uint32_t fdt_size;
> >  
> >      qemu_devices_reset(reason);
> >  
> > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
> >          }
> >      }
> >  
> > -    fdt = pnv_dt_create(machine);
> > +    if (machine->dtb) {
> > +        warn_report("with manually passed dtb, some options like '-append'"
> > +                " might ignored and the dtb passed will be used as-is");
> 
> Check whether append is actually set, and report an fatal error in
> that case. 

Got it.

Though there might be more options which might get ignored, such as
maybe even -device options, as whatever QEMU adds to it's device tree,
that will get ignored, and only the device nodes present in passed DTB
will be used.

> 
> >  
> > -    /* Pack resulting tree */
> > -    _FDT((fdt_pack(fdt)));
> > +        fdt = g_malloc0(FDT_MAX_SIZE);
> > +
> > +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> > +        fdt_file = fopen(machine->dtb, "r");
> > +        if (fdt_file != NULL) {
> > +            fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file);
> > +            if (ferror(fdt_file) != 0) {
> > +                error_report("Could not load dtb '%s'",
> > +                             machine->dtb);
> > +                exit(1);
> > +            }
> > +
> > +            /* mark end of the fdt buffer with '\0' */
> > +            ((char *)fdt)[fdt_size] = '\0';
> > +        }
> 
> Preferrable to use g_file_get_contents()

Sure, thanks, didn't know that !

Thanks,
Aditya Gupta

> 
> > +    } else {
> > +        fdt = pnv_dt_create(machine);
> > +
> > +        /* Pack resulting tree */
> > +        _FDT((fdt_pack(fdt)));
> > +    }
> >  
> >      qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> >      cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
> > -- 
> > 2.45.2
> > 
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
Cédric Le Goater July 31, 2024, 2:43 p.m. UTC | #3
Hello Aditya,

On 7/31/24 15:22, Aditya Gupta wrote:
> Currently any device tree passed with -dtb option in QEMU, was ignored
> by the PowerNV code.
> 
> Read and pass the passed -dtb to the kernel, thus enabling easier
> debugging with custom DTBs.

I thought we had enough controls with the QEMU command line options to
generate a custom DTB. We should improve that first. Unless you want
to mimic a bogus DTB as generated by hostboot and check skiboot behavior.

Can you explain more the use case please ? Is it for skiboot testing ?

Thanks,

C.


> The existing behaviour when -dtb is 'not' passed, is preserved as-is.
> 
> But when a '-dtb' is passed, it completely overrides any dtb nodes or
> changes QEMU might have done, such as '-append' arguments to the kernel
> (which are mentioned in /chosen/bootargs in the dtb), hence add warning
> when -dtb is being used
> 
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> 
> ---
> This is an RFC patch, and hence might not be the final implementation,
> though this current one is a solution which works
>
> ---
>   hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3526852685b4..12cc909b9e26 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>       PnvMachineState *pnv = PNV_MACHINE(machine);
>       IPMIBmc *bmc;
>       void *fdt;
> +    FILE *fdt_file;
> +    uint32_t fdt_size;
>   
>       qemu_devices_reset(reason);
>   
> @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>           }
>       }
>   
> -    fdt = pnv_dt_create(machine);
> +    if (machine->dtb) {
> +        warn_report("with manually passed dtb, some options like '-append'"
> +                " might ignored and the dtb passed will be used as-is");
>   
> -    /* Pack resulting tree */
> -    _FDT((fdt_pack(fdt)));
> +        fdt = g_malloc0(FDT_MAX_SIZE);
> +
> +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> +        fdt_file = fopen(machine->dtb, "r");
> +        if (fdt_file != NULL) {
> +            fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file);
> +            if (ferror(fdt_file) != 0) {
> +                error_report("Could not load dtb '%s'",
> +                             machine->dtb);
> +                exit(1);
> +            }
> +
> +            /* mark end of the fdt buffer with '\0' */
> +            ((char *)fdt)[fdt_size] = '\0';
> +        }
> +    } else {
> +        fdt = pnv_dt_create(machine);
> +
> +        /* Pack resulting tree */
> +        _FDT((fdt_pack(fdt)));
> +    }
>   
>       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
Cédric Le Goater July 31, 2024, 2:49 p.m. UTC | #4
On 7/31/24 15:51, Aditya Gupta wrote:
> Hi Daniel,
> 
> Thank you for the review.
> 
> On 24/07/31 02:34PM, Daniel P. Berrangé wrote:
>> On Wed, Jul 31, 2024 at 06:52:35PM +0530, Aditya Gupta wrote:
>>> Currently any device tree passed with -dtb option in QEMU, was ignored
>>> by the PowerNV code.
>>>
>>> Read and pass the passed -dtb to the kernel, thus enabling easier
>>> debugging with custom DTBs.
>>>
>>> The existing behaviour when -dtb is 'not' passed, is preserved as-is.
>>>
>>> But when a '-dtb' is passed, it completely overrides any dtb nodes or
>>> changes QEMU might have done, such as '-append' arguments to the kernel
>>> (which are mentioned in /chosen/bootargs in the dtb), hence add warning
>>> when -dtb is being used
>>>
>>> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
>>>
>>> ---
>>> This is an RFC patch, and hence might not be the final implementation,
>>> though this current one is a solution which works
>>> ---
>>> ---
>>>   hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
>>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 3526852685b4..12cc909b9e26 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>>>       PnvMachineState *pnv = PNV_MACHINE(machine);
>>>       IPMIBmc *bmc;
>>>       void *fdt;
>>> +    FILE *fdt_file;
>>> +    uint32_t fdt_size;
>>>   
>>>       qemu_devices_reset(reason);
>>>   
>>> @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
>>>           }
>>>       }
>>>   
>>> -    fdt = pnv_dt_create(machine);
>>> +    if (machine->dtb) {
>>> +        warn_report("with manually passed dtb, some options like '-append'"
>>> +                " might ignored and the dtb passed will be used as-is");
>>
>> Check whether append is actually set, and report an fatal error in
>> that case.
> 
> Got it.

and this check should be done preferably when the machine is initialized,
not in the reset handler.

Thanks,

C.
Aditya Gupta July 31, 2024, 7:08 p.m. UTC | #5
Hi Cedric,

On 24/07/31 04:43PM, Cédric Le Goater wrote:
> Hello Aditya,
> 
> On 7/31/24 15:22, Aditya Gupta wrote:
> > Currently any device tree passed with -dtb option in QEMU, was ignored
> > by the PowerNV code.
> > 
> > Read and pass the passed -dtb to the kernel, thus enabling easier
> > debugging with custom DTBs.
> 
> I thought we had enough controls with the QEMU command line options to
> generate a custom DTB. We should improve that first. Unless you want
> to mimic a bogus DTB as generated by hostboot and check skiboot behavior.
> 
> Can you explain more the use case please ? Is it for skiboot testing ?

My usecase is mostly experimental, where I am changing skiboot's
relocation, trying to boot with almost no/minimal parts of skiboot, and
thereby I am passing a custom DTB.

Though the DTB I pass is basically the DTB QEMU passes to skiboot, and
edited it to remove things, add an 'opal' node, and basically have more
control on what device nodes QEMU passes to the firmware/kernel.

The usecase you told seems interesting though, I have not encountered
bogus DTBs by hostboot yet (still new), but might help test skiboot when
that happens.

'-dtb' option will not be for the usual usecase, but can help try these
experimental things with a quick and dirty dtb.

Thanks,
Aditya Gupta

> 
> Thanks,
> 
> C.
> 
> 
> > The existing behaviour when -dtb is 'not' passed, is preserved as-is.
> > 
> > But when a '-dtb' is passed, it completely overrides any dtb nodes or
> > changes QEMU might have done, such as '-append' arguments to the kernel
> > (which are mentioned in /chosen/bootargs in the dtb), hence add warning
> > when -dtb is being used
> > 
> > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> > 
> > ---
> > This is an RFC patch, and hence might not be the final implementation,
> > though this current one is a solution which works
> > 
> > ---
> >   hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++---
> >   1 file changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 3526852685b4..12cc909b9e26 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -714,6 +714,8 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
> >       PnvMachineState *pnv = PNV_MACHINE(machine);
> >       IPMIBmc *bmc;
> >       void *fdt;
> > +    FILE *fdt_file;
> > +    uint32_t fdt_size;
> >       qemu_devices_reset(reason);
> > @@ -736,10 +738,31 @@ static void pnv_reset(MachineState *machine, ShutdownCause reason)
> >           }
> >       }
> > -    fdt = pnv_dt_create(machine);
> > +    if (machine->dtb) {
> > +        warn_report("with manually passed dtb, some options like '-append'"
> > +                " might ignored and the dtb passed will be used as-is");
> > -    /* Pack resulting tree */
> > -    _FDT((fdt_pack(fdt)));
> > +        fdt = g_malloc0(FDT_MAX_SIZE);
> > +
> > +        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
> > +        fdt_file = fopen(machine->dtb, "r");
> > +        if (fdt_file != NULL) {
> > +            fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file);
> > +            if (ferror(fdt_file) != 0) {
> > +                error_report("Could not load dtb '%s'",
> > +                             machine->dtb);
> > +                exit(1);
> > +            }
> > +
> > +            /* mark end of the fdt buffer with '\0' */
> > +            ((char *)fdt)[fdt_size] = '\0';
> > +        }
> > +    } else {
> > +        fdt = pnv_dt_create(machine);
> > +
> > +        /* Pack resulting tree */
> > +        _FDT((fdt_pack(fdt)));
> > +    }
> >       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
> >       cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>
diff mbox series

Patch

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3526852685b4..12cc909b9e26 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -714,6 +714,8 @@  static void pnv_reset(MachineState *machine, ShutdownCause reason)
     PnvMachineState *pnv = PNV_MACHINE(machine);
     IPMIBmc *bmc;
     void *fdt;
+    FILE *fdt_file;
+    uint32_t fdt_size;
 
     qemu_devices_reset(reason);
 
@@ -736,10 +738,31 @@  static void pnv_reset(MachineState *machine, ShutdownCause reason)
         }
     }
 
-    fdt = pnv_dt_create(machine);
+    if (machine->dtb) {
+        warn_report("with manually passed dtb, some options like '-append'"
+                " might ignored and the dtb passed will be used as-is");
 
-    /* Pack resulting tree */
-    _FDT((fdt_pack(fdt)));
+        fdt = g_malloc0(FDT_MAX_SIZE);
+
+        /* read the file 'machine->dtb', and load it into 'fdt' buffer */
+        fdt_file = fopen(machine->dtb, "r");
+        if (fdt_file != NULL) {
+            fdt_size = fread(fdt, sizeof(char), FDT_MAX_SIZE, fdt_file);
+            if (ferror(fdt_file) != 0) {
+                error_report("Could not load dtb '%s'",
+                             machine->dtb);
+                exit(1);
+            }
+
+            /* mark end of the fdt buffer with '\0' */
+            ((char *)fdt)[fdt_size] = '\0';
+        }
+    } else {
+        fdt = pnv_dt_create(machine);
+
+        /* Pack resulting tree */
+        _FDT((fdt_pack(fdt)));
+    }
 
     qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));