Re: [PATCH v11 4/9] iommu/amd: Introduce helper function to update 256-bit DTE

[Uros Bizjak]

On Thu, Nov 14, 2024 at 8:45 PM Suravee Suthikulpanit
<suravee.suthikulpanit@xxxxxxx> wrote:
>
> The current implementation does not follow 128-bit write requirement
> to update DTE as specified in the AMD I/O Virtualization Techonology
> (IOMMU) Specification.
>
> Therefore, modify the struct dev_table_entry to contain union of u128 data
> array, and introduce a helper functions update_dte256() to update DTE using
> two 128-bit cmpxchg operations to update 256-bit DTE with the modified
> structure, and take into account the DTE[V, GV] bits when programming
> the DTE to ensure proper order of DTE programming and flushing.
>
> In addition, introduce a per-DTE spin_lock struct dev_data.dte_lock to
> provide synchronization when updating the DTE to prevent cmpxchg128
> failure.
>
> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Suggested-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  10 ++-
>  drivers/iommu/amd/iommu.c           | 123 ++++++++++++++++++++++++++++
>  2 files changed, 132 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index ae5f1e031722..ea7922b06325 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -427,9 +427,13 @@
>  #define DTE_GCR3_SHIFT_C       43
>
>  #define DTE_GPT_LEVEL_SHIFT    54
> +#define DTE_GPT_LEVEL_MASK     GENMASK_ULL(55, 54)
>
>  #define GCR3_VALID             0x01ULL
>
> +/* DTE[128:179] | DTE[184:191] */
> +#define DTE_DATA2_INTR_MASK    ~GENMASK_ULL(55, 52)
> +
>  #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
>  #define IOMMU_PTE_PRESENT(pte) ((pte) & IOMMU_PTE_PR)
>  #define IOMMU_PTE_DIRTY(pte) ((pte) & IOMMU_PTE_HD)
> @@ -842,6 +846,7 @@ struct devid_map {
>  struct iommu_dev_data {
>         /*Protect against attach/detach races */
>         struct mutex mutex;
> +       spinlock_t dte_lock;              /* DTE lock for 256-bit access */
>
>         struct list_head list;            /* For domain->dev_list */
>         struct llist_node dev_data_list;  /* For global dev_data_list */
> @@ -886,7 +891,10 @@ extern struct list_head amd_iommu_list;
>   * Structure defining one entry in the device table
>   */
>  struct dev_table_entry {
> -       u64 data[4];
> +       union {
> +               u64 data[4];
> +               u128 data128[2];
> +       };
>  };
>
>  /*
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 5ce8e6504ba7..7e0b62f2268a 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -83,12 +83,125 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>  static void set_dte_entry(struct amd_iommu *iommu,
>                           struct iommu_dev_data *dev_data);
>
> +static void iommu_flush_dte_sync(struct amd_iommu *iommu, u16 devid);
> +
>  /****************************************************************************
>   *
>   * Helper functions
>   *
>   ****************************************************************************/
>
> +static __always_inline void amd_iommu_atomic128_set(__int128 *ptr, __int128 val)
> +{
> +       /*
> +        * Note:
> +        * We use arch_try_cmpxchg128_local() because:
> +        * - Need cmpxchg16b instruction mainly for 128-bit store to DTE
> +        *   (not necessary for cmpxchg since this function is already
> +        *   protected by a spin_lock for this DTE).
> +        * - Neither need LOCK_PREFIX nor try loop because of the spin_lock.
> +        */
> +       arch_try_cmpxchg128_local(ptr, ptr, val);

Just use arch_cmpxchg128_local() here.

With that:

Acked-by: Uros Bizjak <ubizjak@xxxxxxxxx>.

Uros.