Re: [PATCH 1/5] iommu/amd: Simplify build_inv_address()

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Mar 30, 2026 at 03:39:16PM +0800, Wei Wang wrote:

> > @@ -1267,39 +1267,37 @@ static void build_inv_dte(struct iommu_cmd *cmd, u16 devid)
> >    */
> >   static inline u64 build_inv_address(u64 address, size_t size)
> >   {
> > -	u64 pages, end, msb_diff;
> > +	u64 last = address + size - 1;
> > +	unsigned int sz_lg2;
> 
> Would naming it shift (or order) be more conventional?

I find that confusing, shift of what exactly? It is size of the
invalidation encoded in log 2.

> > -	end = address + size - 1;
> > +	address &= GENMASK_U64(63, 12);
> > +	sz_lg2 = fls64(address ^ last);
> > +	if (sz_lg2 <= 12)
> 
> then could use "if (shift < PAGE_SHIFT)" here.

PAGE_SHIFT defines the kernel mm configuration, "12" is a hardware
constant from the iommu spec. They should not be intermixed.
 
> > +		return address;
> >   	/*
> > -	 * msb_diff would hold the index of the most significant bit that
> > -	 * flipped between the start and end.
> > +	 * Encode sz_lg2 according to Table 14: Example Page Size Encodings
> > +	 *
> > +	 * See "Note *":
> > +	 *   Address bits 51:32 can be used to encode page sizes greater
> > +	 *   that 4 Gbytes.
> 
> "than" (seems a spec typo)

Yeah, I'll leave it as is.
 
> > +	 * Which we take to mean that the highest page size has bit
> > +	 *  [51]=0, [50:12]=1
> > +	 * and that coding happens when sz_lg2 is 52. Fall back to full
> > +	 * invalidation if the size is too big.
> > +	 *
> >   	 */
> > -	msb_diff = fls64(end ^ address) - 1;
> > +	if (unlikely(sz_lg2 > 52))
> > +		return (CMD_INV_IOMMU_ALL_PAGES_ADDRESS & PAGE_MASK) |
> > +		       CMD_INV_IOMMU_PAGES_SIZE_MASK;
> 
> 
> The spec mentions "Address bits 63:52 are zero-extended.", should we enforce
> it:
> 
> -#define CMD_INV_IOMMU_ALL_PAGES_ADDRESS        0x7ffffffffffff000ULL
> +#define CMD_INV_IOMMU_ALL_PAGES_ADDRESS        GENMASK_ULL(51, 12)
> 
> also for the returned "address"

2.4.3 specifically calls out 

 The IOMMU invalidates all translation
 information associated with the DomainID for both nested and guest levels when S=1, PDE=1,
 GN=0, and Address[63:12]=7_FFFF_FFFF_FFFFh.

I wouldn't change it.

Jason