Re: [PATCH v3 1/3] ACPI: RISC-V: Add support for RIMT

[Sunil V L <sunilvl@ventanamicro.com>]

Hi Drew,

Thank you very much for the review!.

On Mon, Jun 30, 2025 at 09:55:09AM +0200, Andrew Jones wrote:
> Hi Sunil,
> 
> I found a few nits while skimming this.
> 
> On Mon, Jun 30, 2025 at 09:18:01AM +0530, Sunil V L wrote:
> > RISC-V IO Mapping Table (RIMT) is a static ACPI table to communicate
> > IOMMU information to the OS. The spec is available at [1].
> > 
> > The changes at high level are,
> > 	a) Initialize data structures required for IOMMU/device
> > 	   configuration using the data from RIMT. Provide APIs required
> > 	   for device configuration.
> > 	b) Provide an API for IOMMU drivers to register the
> > 	   fwnode with RIMT data structures. This API will create a
> > 	   fwnode for PCIe IOMMU.
> > 
> > [1] - https://github.com/riscv-non-isa/riscv-acpi-rimt
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  MAINTAINERS                 |   1 +
> >  arch/riscv/Kconfig          |   1 +
> >  drivers/acpi/Kconfig        |   4 +
> >  drivers/acpi/riscv/Kconfig  |   7 +
> >  drivers/acpi/riscv/Makefile |   1 +
> >  drivers/acpi/riscv/init.c   |   2 +
> >  drivers/acpi/riscv/init.h   |   1 +
> >  drivers/acpi/riscv/rimt.c   | 523 ++++++++++++++++++++++++++++++++++++
> >  include/linux/acpi_rimt.h   |  26 ++
> >  9 files changed, 566 insertions(+)
> >  create mode 100644 drivers/acpi/riscv/Kconfig
> >  create mode 100644 drivers/acpi/riscv/rimt.c
> >  create mode 100644 include/linux/acpi_rimt.h
> > 
[...]
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 36061f4732b7..96d64e0a7b97 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -16,6 +16,7 @@ config RISCV
> >  	select ACPI_MCFG if (ACPI && PCI)
> >  	select ACPI_PPTT if ACPI
> >  	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> > +        select ACPI_RIMT if ACPI
> 
> Should use tab here.
> 
Okay.
> >  	select ACPI_SPCR_TABLE if ACPI
> >  	select ARCH_DMA_DEFAULT_COHERENT
> >  	select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
[...]
> > +static inline int rimt_set_fwnode(struct acpi_rimt_node *rimt_node,
> > +				  struct fwnode_handle *fwnode)
> 
> I see this is a faithful port of arm's iort functions, but using
> 'inline' in source files is pretty pointless, so we could drop
> that.
> 
Sure.

> > +{
> > +	struct rimt_fwnode *np;
> > +
> > +	np = kzalloc(sizeof(*np), GFP_ATOMIC);
>
[...]
> > +	map = ACPI_ADD_PTR(struct acpi_rimt_id_mapping, node,
> > +			   id_mapping_offset + index * sizeof(*map));
> > +
> > +	/* Firmware bug! */
> > +	if (!map->dest_offset) {
> > +		pr_err(FW_BUG "[node %p type %d] ID map has NULL parent reference\n",
> > +		       node, node->type);
> 
> Should we have a pr_fmt() definition at the top of this source file for
> this pr_err?
> 
Oh yeah. Will add.

> > +		return NULL;
> > +	}
> > +
>
[...]
> > +	return err;
> > +}
> > +
> > +#else
> > +int rimt_iommu_configure_id(struct device *dev, const u32 *id_in)
> > +{
> > +	return -ENODEV;
> > +}
> 
> This is unnecessary since we have the stub in the header file.
> 
Right.

> > +#endif
> > +
>
[...]
> > +int rimt_iommu_configure_id(struct device *dev, const u32 *id_in);
> > +
> 
> ubernit: no need for this blank line
> 
Sure.

Thanks!
Sunil