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

Wei Wang
2026-03-31T02:02:03Z

On 3/30/26 8:05 PM, Jason Gunthorpe wrote:
> 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.

Just the number of bits to shift the value 1 to get the the invalidate
size: 1 << shift = size.

I thought it's similar to PAGE_SHIFT - the number of bits to shift the
value 1 to get PAGE_SIZE (though it's also used to shift an address to
get a frame number).

It's not critical though, up to your preference.

>>
>> 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.
>

OK. The INVALIDATE_ALL case is specialized out. It seems the “Address
bits 63:52 are zero‑extended” requirement in the spec applies only to
the encoded addresses (i.e., the returned address), as stated in Table 14.