diff mbox series

hw/riscv/virt.c: add address-cells in create_fdt_one_aplic()

Message ID 20240530084949.761034-1-dbarboza@ventanamicro.com
State New
Headers show
Series hw/riscv/virt.c: add address-cells in create_fdt_one_aplic() | expand

Commit Message

Daniel Henrique Barboza May 30, 2024, 8:49 a.m. UTC 
We need #address-cells properties in all interrupt controllers that are
referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
APLIC controllers must have this property.

PLIC already sets it in create_fdt_socket_plic(). Set the property for
APLIC in create_fdt_one_aplic().

[1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/

Suggested-by: Anup Patel <apatel@ventanamicro.com>
Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c         | 2 ++
 include/hw/riscv/virt.h | 1 +
 2 files changed, 3 insertions(+)

Comments

Andrew Jones May 30, 2024, 11:05 a.m. UTC | #1 
On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza wrote:
> We need #address-cells properties in all interrupt controllers that are
> referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
> APLIC controllers must have this property.
> 
> PLIC already sets it in create_fdt_socket_plic(). Set the property for
> APLIC in create_fdt_one_aplic().
> 
> [1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/

There are other issues[2] with the DT nodes that we should address at the
same time.

[2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/

Thanks,
drew

> 
> Suggested-by: Anup Patel <apatel@ventanamicro.com>
> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/virt.c         | 2 ++
>  include/hw/riscv/virt.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4fdb660525..1a7e1e73c5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -609,6 +609,8 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>      aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
>      qemu_fdt_add_subnode(ms->fdt, aplic_name);
>      qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
> +    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
> +                          FDT_APLIC_ADDR_CELLS);
>      qemu_fdt_setprop_cell(ms->fdt, aplic_name,
>                            "#interrupt-cells", FDT_APLIC_INT_CELLS);
>      qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 3db839160f..c0dc41ff9a 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -118,6 +118,7 @@ enum {
>  #define FDT_PLIC_ADDR_CELLS   0
>  #define FDT_PLIC_INT_CELLS    1
>  #define FDT_APLIC_INT_CELLS   2
> +#define FDT_APLIC_ADDR_CELLS  0
>  #define FDT_IMSIC_INT_CELLS   0
>  #define FDT_MAX_INT_CELLS     2
>  #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
> -- 
> 2.45.1
> 
>
Andrew Jones May 30, 2024, 11:06 a.m. UTC | #2 
On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote:
> On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza wrote:
> > We need #address-cells properties in all interrupt controllers that are
> > referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
> > APLIC controllers must have this property.
> > 
> > PLIC already sets it in create_fdt_socket_plic(). Set the property for
> > APLIC in create_fdt_one_aplic().
> > 
> > [1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/
> 
> There are other issues[2] with the DT nodes that we should address at the
> same time.
> 
> [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/

I meant to CC Conor. Doing that now.

> 
> Thanks,
> drew
> 
> > 
> > Suggested-by: Anup Patel <apatel@ventanamicro.com>
> > Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >  hw/riscv/virt.c         | 2 ++
> >  include/hw/riscv/virt.h | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 4fdb660525..1a7e1e73c5 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -609,6 +609,8 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
> >      aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
> >      qemu_fdt_add_subnode(ms->fdt, aplic_name);
> >      qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
> > +    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
> > +                          FDT_APLIC_ADDR_CELLS);
> >      qemu_fdt_setprop_cell(ms->fdt, aplic_name,
> >                            "#interrupt-cells", FDT_APLIC_INT_CELLS);
> >      qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 3db839160f..c0dc41ff9a 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -118,6 +118,7 @@ enum {
> >  #define FDT_PLIC_ADDR_CELLS   0
> >  #define FDT_PLIC_INT_CELLS    1
> >  #define FDT_APLIC_INT_CELLS   2
> > +#define FDT_APLIC_ADDR_CELLS  0
> >  #define FDT_IMSIC_INT_CELLS   0
> >  #define FDT_MAX_INT_CELLS     2
> >  #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
> > -- 
> > 2.45.1
> > 
> >
Daniel Henrique Barboza May 30, 2024, 12:26 p.m. UTC | #3 
On 5/30/24 08:06, Andrew Jones wrote:
> On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote:
>> On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza wrote:
>>> We need #address-cells properties in all interrupt controllers that are
>>> referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
>>> APLIC controllers must have this property.
>>>
>>> PLIC already sets it in create_fdt_socket_plic(). Set the property for
>>> APLIC in create_fdt_one_aplic().
>>>
>>> [1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/
>>
>> There are other issues[2] with the DT nodes that we should address at the
>> same time.
>>
>> [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/
> 
> I meant to CC Conor. Doing that now.

I'll take a look at these other DT nodes issues.

Conor, mind give me pointers on how do I reproduce the validation you did
in [2]? Using upstream 'dtc' I have stuff like:

../qemu/qemu_dts.dts:261.4-68: Warning (interrupts_extended_property): /soc/aplic@d000000:interrupts-extended: cell 0 is not a phandle reference

Which seems to also be an error but it's not what you reported. Are you
using 'dt-validate' from dt-schema?


Thanks,

Daniel

> 
>>
>> Thanks,
>> drew
>>
>>>
>>> Suggested-by: Anup Patel <apatel@ventanamicro.com>
>>> Fixes: e6faee65855b ("hw/riscv: virt: Add optional AIA APLIC support to virt machine")
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/virt.c         | 2 ++
>>>   include/hw/riscv/virt.h | 1 +
>>>   2 files changed, 3 insertions(+)
>>>
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index 4fdb660525..1a7e1e73c5 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -609,6 +609,8 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
>>>       aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
>>>       qemu_fdt_add_subnode(ms->fdt, aplic_name);
>>>       qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
>>> +    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
>>> +                          FDT_APLIC_ADDR_CELLS);
>>>       qemu_fdt_setprop_cell(ms->fdt, aplic_name,
>>>                             "#interrupt-cells", FDT_APLIC_INT_CELLS);
>>>       qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
>>> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>>> index 3db839160f..c0dc41ff9a 100644
>>> --- a/include/hw/riscv/virt.h
>>> +++ b/include/hw/riscv/virt.h
>>> @@ -118,6 +118,7 @@ enum {
>>>   #define FDT_PLIC_ADDR_CELLS   0
>>>   #define FDT_PLIC_INT_CELLS    1
>>>   #define FDT_APLIC_INT_CELLS   2
>>> +#define FDT_APLIC_ADDR_CELLS  0
>>>   #define FDT_IMSIC_INT_CELLS   0
>>>   #define FDT_MAX_INT_CELLS     2
>>>   #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
>>> -- 
>>> 2.45.1
>>>
>>>
Conor Dooley May 30, 2024, 3:35 p.m. UTC | #4 
On Thu, May 30, 2024 at 09:26:58AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 5/30/24 08:06, Andrew Jones wrote:
> > On Thu, May 30, 2024 at 01:05:41PM GMT, Andrew Jones wrote:
> > > On Thu, May 30, 2024 at 05:49:49AM GMT, Daniel Henrique Barboza wrote:
> > > > We need #address-cells properties in all interrupt controllers that are
> > > > referred by an interrupt-map [1]. For the RISC-V machine, both PLIC and
> > > > APLIC controllers must have this property.
> > > > 
> > > > PLIC already sets it in create_fdt_socket_plic(). Set the property for
> > > > APLIC in create_fdt_one_aplic().
> > > > 
> > > > [1] https://lore.kernel.org/linux-arm-kernel/CAL_JsqJE15D-xXxmELsmuD+JQHZzxGzdXvikChn6KFWqk6NzPw@mail.gmail.com/
> > > 
> > > There are other issues[2] with the DT nodes that we should address at the
> > > same time.
> > > 
> > > [2] https://lore.kernel.org/all/20240529-rust-tile-a05517a6260f@spud/
> > 
> > I meant to CC Conor. Doing that now.
> 
> I'll take a look at these other DT nodes issues.
> 
> Conor, mind give me pointers on how do I reproduce the validation you did
> in [2]? Using upstream 'dtc' I have stuff like:
> 
> ../qemu/qemu_dts.dts:261.4-68: Warning (interrupts_extended_property): /soc/aplic@d000000:interrupts-extended: cell 0 is not a phandle reference
> 
> Which seems to also be an error but it's not what you reported. Are you
> using 'dt-validate' from dt-schema?

Yeah, dt-validate. There's probably some stuff that I could add to my
machine to make it more interesting, but I ran:
	$(qemu) -smp 4 -M virt,aia=aplic-imsic,dumpdtb=$(qemu_dtb) -cpu max -m 1G -nographic
	dt-validate --schema $(processed_schema) $(qemu_dtb) 2>&1 | tee logs/dtbdump.log

A processed schema is a pre-requisite, and I usually have one sitting
around from running dtbs_check or dt_binding_check in Linux, but I think
you can use dt-rebasing to generate one either:
https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/tree/Bindings/Makefile

You'll see a bunch of noise from undocumented isa extensions, but at the
end of the output should be the aplic complaints.

I forgot I had it disabled to test something when I did that test the
other day, there's also complaints about the imsics:
qemu.dtb: imsics@28000000: $nodename:0: 'imsics@28000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@28000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics']
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@28000000: compatible: ['riscv,imsics'] is too short
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@28000000: '#msi-cells' is a required property
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@28000000: Unevaluated properties are not allowed ('compatible' was unexpected)
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@24000000: $nodename:0: 'imsics@24000000' does not match '^interrupt-controller(@[0-9a-f,]+)*$'
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@24000000: compatible:0: 'riscv,imsics' is not one of ['qemu,imsics']
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@24000000: compatible: ['riscv,imsics'] is too short
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@24000000: '#msi-cells' is a required property
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
qemu.dtb: imsics@24000000: Unevaluated properties are not allowed ('compatible' was unexpected)
        from schema $id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#

Cheers,
Conor.
diff mbox series

Patch

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4fdb660525..1a7e1e73c5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -609,6 +609,8 @@  static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
     aplic_name = g_strdup_printf("/soc/aplic@%lx", aplic_addr);
     qemu_fdt_add_subnode(ms->fdt, aplic_name);
     qemu_fdt_setprop_string(ms->fdt, aplic_name, "compatible", "riscv,aplic");
+    qemu_fdt_setprop_cell(ms->fdt, aplic_name, "#address-cells",
+                          FDT_APLIC_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, aplic_name,
                           "#interrupt-cells", FDT_APLIC_INT_CELLS);
     qemu_fdt_setprop(ms->fdt, aplic_name, "interrupt-controller", NULL, 0);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 3db839160f..c0dc41ff9a 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -118,6 +118,7 @@  enum {
 #define FDT_PLIC_ADDR_CELLS   0
 #define FDT_PLIC_INT_CELLS    1
 #define FDT_APLIC_INT_CELLS   2
+#define FDT_APLIC_ADDR_CELLS  0
 #define FDT_IMSIC_INT_CELLS   0
 #define FDT_MAX_INT_CELLS     2
 #define FDT_MAX_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \