Re: [PATCH 7/7] iommu/riscv: Add NAPOT range invalidation support

From: Jason Gunthorpe <jgg@nvidia.com>
To: Andrew Jones <andrew.jones@oss.qualcomm.com>
Cc: Alexandre Ghiti <alex@ghiti.fr>,
	Albert Ou <aou@eecs.berkeley.edu>,
	iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-riscv@lists.infradead.org,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <pjw@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Tomasz Jeznach <tjeznach@rivosinc.com>,
	Will Deacon <will@kernel.org>,
	Fangyu Yu <fangyu.yu@linux.alibaba.com>,
	patches@lists.linux.dev
Subject: Re: [PATCH 7/7] iommu/riscv: Add NAPOT range invalidation support
Date: Wed, 6 May 2026 06:13:06 -0300	[thread overview]
Message-ID: <afsGItRkLibstQpa@nvidia.com> (raw)
In-Reply-To: <oe75yk2fx2c7wg73xrikppw2gxxe2e6bsvkktowbo64ftldy7p@53gjjzhu6cy5>

On Tue, May 05, 2026 at 05:47:10PM -0500, Andrew Jones wrote:
> On Fri, Apr 10, 2026 at 12:57:08PM -0300, Jason Gunthorpe wrote:
> > Use the RISC-V IOMMU Address Range Invalidation extension
> > (capabilities.S, spec section 9.3) to invalidate an IOVA range with
> > a single IOTINVAL.VMA command using NAPOT-encoded addressing.
> > 
> > One iommu_iotlb_gather maps to one NAPOT invalidation command. The
> > smallest power-of-two aligned range covering the gather is used since
> > over-invalidation is always safe.
> > 
> > S and NL seem to be orthogonal in the spec, so if NL is not
> > supported then global invalidation is probably always going to happen
> > as wiping a large range without a table change is not common.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/riscv/iommu-bits.h | 17 +++++++++++++
> >  drivers/iommu/riscv/iommu.c      | 43 +++++++++++++++++++++++++++-----
> >  2 files changed, 54 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iommu/riscv/iommu-bits.h b/drivers/iommu/riscv/iommu-bits.h
> > index f01b49ac815586..32b3ad3ac9ae59 100644
> > --- a/drivers/iommu/riscv/iommu-bits.h
> > +++ b/drivers/iommu/riscv/iommu-bits.h
> > @@ -64,6 +64,7 @@
> >  #define RISCV_IOMMU_CAPABILITIES_PD17		BIT_ULL(39)
> >  #define RISCV_IOMMU_CAPABILITIES_PD20		BIT_ULL(40)
> >  #define RISCV_IOMMU_CAPABILITIES_NL		BIT_ULL(42)
> > +#define RISCV_IOMMU_CAPABILITIES_S		BIT_ULL(43)
> >  
> >  /**
> >   * enum riscv_iommu_igs_settings - Interrupt Generation Support Settings
> > @@ -475,6 +476,7 @@ struct riscv_iommu_command {
> >  #define RISCV_IOMMU_CMD_IOTINVAL_GV		BIT_ULL(33)
> >  #define RISCV_IOMMU_CMD_IOTINVAL_GSCID		GENMASK_ULL(59, 44)
> >  #define RISCV_IOMMU_CMD_IOTINVAL_NL		BIT_ULL(34)
> > +#define RISCV_IOMMU_CMD_IOTINVAL_S		BIT_ULL(9)
> 
> We should add a comment here that this is actually bit 73, i.e.
> 
> #define RISCV_IOMMU_CMD_IOTINVAL_S           BIT_ULL(9)	/* bit 73 (dword1 bit 9) */
> 
> because...

!! That's a big mistake, the common thing in other drivers is
something like CMD0 CMD1 to designate the word.

> > +static inline void riscv_iommu_cmd_inval_set_napot(
> > +	struct riscv_iommu_command *cmd, u64 addr, unsigned int sz_lg2)
> > +{
> > +	u64 pfn = addr >> 12;
> > +
> > +	pfn |= BIT_U64(sz_lg2 - 13) - 1;
> > +	cmd->dword1 = FIELD_PREP(RISCV_IOMMU_CMD_IOTINVAL_ADDR, pfn);
> > +	cmd->dword0 |= RISCV_IOMMU_CMD_IOTINVAL_AV | RISCV_IOMMU_CMD_IOTINVAL_S;
> 
> ...here we're setting the wrong dword. This should be
> 
> 	cmd->dword1 = FIELD_PREP(RISCV_IOMMU_CMD_IOTINVAL_ADDR, pfn) |
> 		      RISCV_IOMMU_CMD_IOTINVAL_S;
> 	cmd->dword0 |= RISCV_IOMMU_CMD_IOTINVAL_AV;

Right, I will fix it up

Thanks,
Jason