Re: [PATCH 1/2] amd-vi: set the size flag in the IOTLB page invalidation command

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/10/2026 2:51 pm, Jason Andryuk wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
>
> The S bit of an INVALIDATE_IOTLB_PAGES command selects between the two
> sizes the command can name: cleared, the invalidation covers 4 Kbytes;
> set, it covers the range encoded by the first zero bit of the address,
> counting from bit 12. invalidate_iotlb_pages() builds that encoding
> whenever it is given a non-zero order or the all-pages address, and then
> passes the field's mask where set_field_in_reg_u32() expects the shift.
> The correct mask is 1 and the shift is 0.  When mask (1) is passed for
> both, the value is shifted out of the field it was to occupy and the
> bit is left clear.  i.e. (x << 1) & 1 is always 0.
>
> An endpoint holding translations of its own is therefore told to drop one
> page where it should have dropped a range.  The rest of the range is
> kept in its cache after the page tables backing it have changed.
>
> Pass the shift, as the sibling INVALIDATE_IOMMU_PAGES builder does.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Reviewed-by: Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>
> [ Reword commit message. ]
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This was submitted to the security list, but it is out of scope.  Xenia
> reviewed internally and Jan reviewed from the security list.
>
>  xen/drivers/passthrough/amd/iommu_cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c 
> b/xen/drivers/passthrough/amd/iommu_cmd.c
> index 665e882a2d..abfbdf2b28 100644
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -212,7 +212,7 @@ static void invalidate_iotlb_pages(struct amd_iommu 
> *iommu,
>  
>      set_field_in_reg_u32(sflag, 0,
>                           IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK,
> -                         IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK, &entry);
> +                         IOMMU_INV_IOTLB_PAGES_S_FLAG_SHIFT, &entry);
>  
>      set_field_in_reg_u32((u32)addr_lo >> PAGE_SHIFT, entry,
>                           IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK,

As said on the security list, this is probably fine for a fix wanting
backporting, but can we please purge the use of set_field_in_reg_u32()
entirely.

It is horrible for code legibility and generated code, and very fragile
for mistakes such as this.

~Andrew