hw/ppc/pnv.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
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");
- /* 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));
--
2.45.2Hello 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));
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));
>
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
--
|: 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 :|
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 :|
>
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.
© 2016 - 2024 Red Hat, Inc.