The cells of 'initrd-start/end' should follow the '#address-cell'.
QEMU API could support 1 and 2 cells.
Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
hw/riscv/boot.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index ad45bd7a6a..76b099c696 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
void *fdt = machine->fdt;
hwaddr start, end;
ssize_t size;
+ uint32_t acells;
g_assert(filename != NULL);
@@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
/* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
if (fdt) {
+ acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+ NULL, NULL);
+ if (acells == 0) {
+ error_report("dtb file invalid (#address-cells 0)");
+ exit(1);
+ }
+
end = start + size;
- qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
- qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
+ qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
+ acells, start);
+ qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
+ acells, end);
}
}
--
2.17.1
On 10/21/24 1:09 AM, Jim Shu wrote:
> The cells of 'initrd-start/end' should follow the '#address-cell'.
> QEMU API could support 1 and 2 cells.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
> hw/riscv/boot.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index ad45bd7a6a..76b099c696 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> void *fdt = machine->fdt;
> hwaddr start, end;
> ssize_t size;
> + uint32_t acells;
>
> g_assert(filename != NULL);
>
> @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>
> /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> if (fdt) {
> + acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> + NULL, NULL);
> + if (acells == 0) {
> + error_report("dtb file invalid (#address-cells 0)");
> + exit(1);
> + }
> +
> end = start + size;
> - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> + acells, start);
> + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> + acells, end);
> }
Is this a legal format for linux,initrd-start and linux,initrd-end?
This link:
https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
Defines both attributes as:
"These properties hold the physical start and end address of an initrd that's
loaded by the bootloader."
So I'm not sure if this format you're using here is valid.
Conor, care to weight in? Thanks,
Daniel
> }
>
On Mon, Oct 21, 2024 at 04:30:11PM -0300, Daniel Henrique Barboza wrote:
>
>
> On 10/21/24 1:09 AM, Jim Shu wrote:
> > The cells of 'initrd-start/end' should follow the '#address-cell'.
> > QEMU API could support 1 and 2 cells.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> > hw/riscv/boot.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index ad45bd7a6a..76b099c696 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> > void *fdt = machine->fdt;
> > hwaddr start, end;
> > ssize_t size;
> > + uint32_t acells;
> > g_assert(filename != NULL);
> > @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> > /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> > if (fdt) {
> > + acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> > + NULL, NULL);
> > + if (acells == 0) {
> > + error_report("dtb file invalid (#address-cells 0)");
> > + exit(1);
> > + }
> > +
> > end = start + size;
> > - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> > - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> > + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> > + acells, start);
> > + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> > + acells, end);
> > }
>
> Is this a legal format for linux,initrd-start and linux,initrd-end?
>
> This link:
>
> https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
>
> Defines both attributes as:
>
> "These properties hold the physical start and end address of an initrd that's
> loaded by the bootloader."
>
> So I'm not sure if this format you're using here is valid.
Looks like my input isn't really required here anymore, but I think
these two are actually identical in how they appear in the blob. Don't
see much reason to change it.
Compare Linux code to handle "linux,initrd-start" [1] and
"linux,usable-memory-range" [2], I think the initrd code doesn't check
root #address-cells.
Using the origin code of "u64" is okay. I can remove this commit in
the next version.
[1] https://elixir.bootlin.com/linux/v6.11.4/source/drivers/of/fdt.c#L785
[2] https://elixir.bootlin.com/linux/v6.11.4/source/drivers/of/fdt.c#L857
Thanks,
Jim Shu
On Tue, Oct 22, 2024 at 3:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/21/24 1:09 AM, Jim Shu wrote:
> > The cells of 'initrd-start/end' should follow the '#address-cell'.
> > QEMU API could support 1 and 2 cells.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> > hw/riscv/boot.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index ad45bd7a6a..76b099c696 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> > void *fdt = machine->fdt;
> > hwaddr start, end;
> > ssize_t size;
> > + uint32_t acells;
> >
> > g_assert(filename != NULL);
> >
> > @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> >
> > /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> > if (fdt) {
> > + acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> > + NULL, NULL);
> > + if (acells == 0) {
> > + error_report("dtb file invalid (#address-cells 0)");
> > + exit(1);
> > + }
> > +
> > end = start + size;
> > - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> > - qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> > + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> > + acells, start);
> > + qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> > + acells, end);
> > }
>
> Is this a legal format for linux,initrd-start and linux,initrd-end?
>
> This link:
>
> https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
>
> Defines both attributes as:
>
> "These properties hold the physical start and end address of an initrd that's
> loaded by the bootloader."
>
> So I'm not sure if this format you're using here is valid.
>
>
> Conor, care to weight in? Thanks,
>
> Daniel
>
> > }
> >
© 2016 - 2026 Red Hat, Inc.