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.
