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

Wei Wang
2026-03-30T07:39:25Z

On 3/27/26 11:23 PM, Jason Gunthorpe wrote:
> This function is doing more work than it needs to:
>
> - iommu_num_pages() is pointless, the fls() is going to compute the
> required page size already.
>
> - It is easier to understand as sz_lg2, which is 12 if size is 4K,
> than msb_diff which is 11 if size is 4K.
>
> - Simplify the control flow to early exit on the out of range cases.
>
> - Use the usual last instead of end to signify an inclusive last
> address.
>
> - Use GENMASK to compute the 1's mask.
>
> - Use GENMASK to compute the address mask for the command layout,
> not PAGE_MASK.
>
> - Directly reference the spec language that defines the 52 bit
> limit.
>
> No functional change intended.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/amd/iommu.c | 50 +++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 528ebdd26d71c9..28308759bb3e4d 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -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?

>
> - pages = iommu_num_pages(address, size, PAGE_SIZE);
> -
> - if (pages == 1)
> - return address & PAGE_MASK;
> -
> - 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.

> + 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)

> + * 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"

>
> /*
> - * Bits 63:52 are sign extended. If for some reason bit 51 is different
> - * between the start and the end, invalidate everything.
> + * The sz_lg2 calculation with fls() ensures that:
> + * address & BIT(sz_lg2 - 1) == 0
> + * Therefore only the 1's need to be added. 8KB requires no 1's
> */
> - if (unlikely(msb_diff > 51)) {
> - address = CMD_INV_IOMMU_ALL_PAGES_ADDRESS;
> - } else {
> - /*
> - * The msb-bit must be clear on the address. Just set all the
> - * lower bits.
> - */
> - address |= (1ull << msb_diff) - 1;
> - }
> -
> - /* Clear bits 11:0 */
> - address &= PAGE_MASK;
> -
> - /* Set the size bit - we flush more than one 4kb page */
> + if (sz_lg2 > 13)
> + address |= GENMASK_U64(sz_lg2 - 2, 12);
> return address | CMD_INV_IOMMU_PAGES_SIZE_MASK;
> }
>