Re: [PATCH 08/18] amd_iommu: Helper to decode size of page invalidation command

[Sairaj Kodilkar <sarunkod@amd.com>]

On 4/14/2025 7:32 AM, Alejandro Jimenez wrote:
> The size of the region to invalidate depends on the S bit and address
> encoded in the command. Add a helper to extract this information, which
> will be used to sync shadow page tables in upcoming changes.
> 
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>   hw/i386/amd_iommu.c | 34 ++++++++++++++++++++++++++++++++++
>   hw/i386/amd_iommu.h |  4 ++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 5f55be1f4d36..0af873b66a31 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -481,6 +481,40 @@ static gboolean amdvi_iotlb_remove_by_domid(gpointer key, gpointer value,
>       return entry->domid == domid;
>   }
>   
> +/*
> + * Helper to decode the size of the range to invalidate encoded in the
> + * INVALIDATE_IOMMU_PAGES Command format.
> + * The size of the region to invalidate depends on the S bit and address.
> + * S bit value:
> + * 0 :  Invalidation size is 4 Kbytes.
> + * 1 :  Invalidation size is determined by first zero bit in the address
> + *      starting from Address[12].
> + *
> + * In the AMD IOMMU Linux driver, an invalidation command with address
> + * ((1 << 63) - 1) is sent when intending to clear the entire cache.
> + * However, Table 14: Example Page Size Encodings shows that an address of
> + * ((1ULL << 51) - 1) encodes the entire cache, so effectively any address with
> + * first zero at bit 51 or larger is a request to invalidate the entire address
> + * space.
> + */
> +static uint64_t __attribute__((unused))
> +amdvi_decode_invalidation_size(hwaddr addr, uint16_t flags)
> +{
> +    uint64_t size = AMDVI_PAGE_SIZE;
> +    uint8_t fzbit = 0;
> +
> +    if (flags & AMDVI_CMD_INVAL_IOMMU_PAGES_S) {
> +        fzbit = cto64(addr | 0xFFF);
> +
> +        if (fzbit >= 51 || !addr) {

I am skeptical about the condition addr == 0 (!addr)

Consider the case where user wants to invalidate 8K size page, starting
from address 0. It'll cause address field to be 0, right ? If so, then
we should invalidate only 8K page not the entire address range.

Am I missing something here ?

Regards
Sairaj
> +            size = AMDVI_INV_ALL_PAGES;
> +        } else {> +            size = 1ULL << (fzbit + 1);
> +        }
> +    }
> +    return size;
> +}
> +
>   /* we don't have devid - we can't remove pages by address */
>   static void amdvi_inval_pages(AMDVIState *s, uint64_t *cmd)
>   {
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index e12ecade4baa..c89e7dc9947d 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -123,6 +123,10 @@
>   #define AMDVI_CMD_COMPLETE_PPR_REQUEST    0x07
>   #define AMDVI_CMD_INVAL_AMDVI_ALL         0x08
>   
> +
> +#define AMDVI_CMD_INVAL_IOMMU_PAGES_S   (1ULL << 0)
> +#define AMDVI_INV_ALL_PAGES             (1ULL << 52)
> +
>   #define AMDVI_DEVTAB_ENTRY_SIZE           32
>   
>   /* Device table entry bits 0:63 */