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@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
drivers/iommu/amd/amd_iommu_types.h | 13 +++-
drivers/iommu/amd/iommu.c | 114 ++++++++++++++++++++++++++++
2 files changed, 126 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 601fb4ee6900..91f802be7898 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -425,9 +425,16 @@
#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)
+
+/* DTE[180:181] */
+#define DTE_DATA2_RESV_MASK GENMASK_ULL(53, 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)
@@ -832,6 +839,7 @@ struct devid_map {
struct iommu_dev_data {
/*Protect against attach/detach races */
spinlock_t lock;
+ 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 */
@@ -882,7 +890,10 @@ extern struct amd_iommu *amd_iommus[MAX_IOMMUS];
* 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 8364cd6fa47d..deb19af48a3e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -77,12 +77,116 @@ static void detach_device(struct device *dev);
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 void write_dte_upper128(struct dev_table_entry *ptr, struct dev_table_entry *new)
+{
+ struct dev_table_entry old = {};
+
+ do {
+ old.data128[1] = ptr->data128[1];
+ new->data[2] &= ~DTE_DATA2_INTR_MASK;
+ new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK | DTE_DATA2_RESV_MASK);
+ } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1], new->data128[1]));
+}
+
+static void write_dte_lower128(struct dev_table_entry *ptr, struct dev_table_entry *new)
+{
+ struct dev_table_entry old = {};
+
+ /*
+ * Need to preserve DTE[96:106], which can be set by information in IVRS table.
+ * See set_dev_entry_from_acpi().
+ */
+ new->data[1] |= ptr->data[1] & DTE_FLAG_MASK;
+
+ do {
+ old.data128[0] = ptr->data128[0];
+ } while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0]));
+}
+
+/*
+ * Note:
+ * IOMMU reads the entire Device Table entry in a single 256-bit transaction
+ * but the driver is programming DTE using 2 128-bit cmpxchg. So, the driver
+ * need to ensure the following:
+ * - DTE[V|GV] bit is being written last when setting.
+ * - DTE[V|GV] bit is being written first when clearing.
+ *
+ * This function is used only by code, which updates DMA translation part of the DTE.
+ * So, only consider control bits related to DMA when updating the entry.
+ */
+static void update_dte256(struct amd_iommu *iommu, struct iommu_dev_data *dev_data,
+ struct dev_table_entry *new)
+{
+ struct dev_table_entry *dev_table = get_dev_table(iommu);
+ struct dev_table_entry *ptr = &dev_table[dev_data->devid];
+
+ spin_lock(&dev_data->dte_lock);
+
+ if (!(ptr->data[0] & DTE_FLAG_V)) {
+ /* Existing DTE is not valid. */
+ write_dte_upper128(ptr, new);
+ write_dte_lower128(ptr, new);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+ } else if (!(new->data[0] & DTE_FLAG_V)) {
+ /* Existing DTE is valid. New DTE is not valid. */
+ write_dte_lower128(ptr, new);
+ write_dte_upper128(ptr, new);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+ } else if (!FIELD_GET(DTE_FLAG_GV, ptr->data[0])) {
+ /*
+ * Both DTEs are valid.
+ * Existing DTE has no guest page table.
+ */
+ write_dte_upper128(ptr, new);
+ write_dte_lower128(ptr, new);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+ } else if (!FIELD_GET(DTE_FLAG_GV, new->data[0])) {
+ /*
+ * Both DTEs are valid.
+ * Existing DTE has guest page table,
+ * new DTE has no guest page table,
+ */
+ write_dte_lower128(ptr, new);
+ write_dte_upper128(ptr, new);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+ } else if (FIELD_GET(DTE_GPT_LEVEL_MASK, ptr->data[2]) !=
+ FIELD_GET(DTE_GPT_LEVEL_MASK, new->data[2])) {
+ /*
+ * Both DTEs are valid and have guest page table,
+ * but have different number of levels. So, we need
+ * to upadte both upper and lower 128-bit value, which
+ * require disabling and flushing.
+ */
+ struct dev_table_entry clear = {};
+
+ /* First disable DTE */
+ write_dte_lower128(ptr, &clear);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+
+ /* Then update DTE */
+ write_dte_upper128(ptr, new);
+ write_dte_lower128(ptr, new);
+ iommu_flush_dte_sync(iommu, dev_data->devid);
+ } else {
+ /*
+ * Both DTEs are valid and have guest page table,
+ * and same number of levels. We just need to only
+ * update the lower 128-bit. So no need to disable DTE.
+ */
+ write_dte_lower128(ptr, new);
+ }
+
+ spin_unlock(&dev_data->dte_lock);
+}
+
static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
{
return (pdom && (pdom->pd_mode == PD_MODE_V2));
@@ -203,6 +307,7 @@ static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
return NULL;
spin_lock_init(&dev_data->lock);
+ spin_lock_init(&dev_data->dte_lock);
dev_data->devid = devid;
ratelimit_default_init(&dev_data->rs);
@@ -1272,6 +1377,15 @@ static int iommu_flush_dte(struct amd_iommu *iommu, u16 devid)
return iommu_queue_command(iommu, &cmd);
}
+static void iommu_flush_dte_sync(struct amd_iommu *iommu, u16 devid)
+{
+ int ret;
+
+ ret = iommu_flush_dte(iommu, devid);
+ if (!ret)
+ iommu_completion_wait(iommu);
+}
+
static void amd_iommu_flush_dte_all(struct amd_iommu *iommu)
{
u32 devid;
--
2.34.1
On 7. 10. 24 06:13, Suravee Suthikulpanit wrote:
> +
> /****************************************************************************
> *
> * Helper functions
> *
> ****************************************************************************/
>
> +static void write_dte_upper128(struct dev_table_entry *ptr, struct dev_table_entry *new)
> +{
> + struct dev_table_entry old = {};
> +
> + do {
> + old.data128[1] = ptr->data128[1];
> + new->data[2] &= ~DTE_DATA2_INTR_MASK;
> + new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK | DTE_DATA2_RESV_MASK);
> + } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1], new->data128[1]));
Please note that try_cmpxchg inherently updates &old.data128[1] above on
failure. There is no need to update value again in the loop.
Please also note that the value from ptr->data128[1] should be read
using READ_ONCE() to prevent compiler from merging, refetching or
reordering the read. Currently, there is no READ_ONCE() implemented for
__int128, so something like the attached patch should be used.
Based on the above, the loop should be rewritten as:
old.data128[1] = READ_ONCE(ptr->data128[1]);
do {
new->data[2] &= ~DTE_DATA2_INTR_MASK;
new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK | DTE_DATA2_RESV_MASK);
} while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
new->data128[1]));
> +}
> +
> +static void write_dte_lower128(struct dev_table_entry *ptr, struct dev_table_entry *new)
> +{
> + struct dev_table_entry old = {};
> +
> + /*
> + * Need to preserve DTE[96:106], which can be set by information in IVRS table.
> + * See set_dev_entry_from_acpi().
> + */
> + new->data[1] |= ptr->data[1] & DTE_FLAG_MASK;
> +
> + do {
> + old.data128[0] = ptr->data128[0];
> + } while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0], new->data128[0]));
And this one as:
old.data128[0] = READ_ONCE(ptr->data128[0]);
do {
} while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
new->data128[0]));
Best regards,
Uros.diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
index 8d0a6280e982..8bf942ad5ef3 100644
--- a/include/asm-generic/rwonce.h
+++ b/include/asm-generic/rwonce.h
@@ -33,7 +33,7 @@
* (e.g. a virtual address) and a strong prevailing wind.
*/
#define compiletime_assert_rwonce_type(t) \
- compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long), \
+ compiletime_assert(__native_word(t) || sizeof(t) == sizeof(__dword_type), \
"Unsupported access size for {READ,WRITE}_ONCE().")
/*
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 94b8fedfb077..8615e91f48fd 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -469,6 +469,12 @@ struct ftrace_likely_data {
unsigned type: (unsigned type)0, \
signed type: (signed type)0
+#ifdef __SIZEOF_INT128__
+#define __dword_type __int128
+#else
+#define __dword_type long long
+#endif
+
#define __unqual_scalar_typeof(x) typeof( \
_Generic((x), \
char: (char)0, \
@@ -476,7 +482,7 @@ struct ftrace_likely_data {
__scalar_type_to_expr_cases(short), \
__scalar_type_to_expr_cases(int), \
__scalar_type_to_expr_cases(long), \
- __scalar_type_to_expr_cases(long long), \
+ __scalar_type_to_expr_cases(__dword_type), \
default: (x)))
/* Is this type a native word size -- useful for atomic operations */
On 10/7/2024 9:42 PM, Uros Bizjak wrote:
>
>
> On 7. 10. 24 06:13, Suravee Suthikulpanit wrote:
>
>> +
>>
>> /****************************************************************************
>> *
>> * Helper functions
>> *
>>
>> ****************************************************************************/
>> +static void write_dte_upper128(struct dev_table_entry *ptr, struct
>> dev_table_entry *new)
>> +{
>> + struct dev_table_entry old = {};
>> +
>> + do {
>> + old.data128[1] = ptr->data128[1];
>> + new->data[2] &= ~DTE_DATA2_INTR_MASK;
>> + new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK |
>> DTE_DATA2_RESV_MASK);
>> + } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
>> new->data128[1]));
>
> Please note that try_cmpxchg inherently updates &old.data128[1] above on
> failure. There is no need to update value again in the loop.
>
> Please also note that the value from ptr->data128[1] should be read
> using READ_ONCE() to prevent compiler from merging, refetching or
> reordering the read. Currently, there is no READ_ONCE() implemented for
> __int128, so something like the attached patch should be used.
Thanks for pointing this out. I will introduce the attached patch
separately in this series on your behalf as author/sign-off, and review
the current code to properly use the READ_ONCE().
Thanks,
Suravee
> Based on the above, the loop should be rewritten as:
>
> old.data128[1] = READ_ONCE(ptr->data128[1]);
> do {
> new->data[2] &= ~DTE_DATA2_INTR_MASK;
> new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK |
> DTE_DATA2_RESV_MASK);
> } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
> new->data128[1]));
>
>> +}
>> +
>> +static void write_dte_lower128(struct dev_table_entry *ptr, struct
>> dev_table_entry *new)
>> +{
>> + struct dev_table_entry old = {};
>> +
>> + /*
>> + * Need to preserve DTE[96:106], which can be set by information
>> in IVRS table.
>> + * See set_dev_entry_from_acpi().
>> + */
>> + new->data[1] |= ptr->data[1] & DTE_FLAG_MASK;
>> +
>> + do {
>> + old.data128[0] = ptr->data128[0];
>> + } while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
>> new->data128[0]));
>
> And this one as:
>
> old.data128[0] = READ_ONCE(ptr->data128[0]);
> do {
> } while (!try_cmpxchg128(&ptr->data128[0], &old.data128[0],
> new->data128[0]));
>
> Best regards,
> Uros.
On Fri, Oct 11, 2024 at 12:22 PM Suthikulpanit, Suravee
<suravee.suthikulpanit@amd.com> wrote:
>
> On 10/7/2024 9:42 PM, Uros Bizjak wrote:
> >
> >
> > On 7. 10. 24 06:13, Suravee Suthikulpanit wrote:
> >
> >> +
> >>
> >> /****************************************************************************
> >> *
> >> * Helper functions
> >> *
> >>
> >> ****************************************************************************/
> >> +static void write_dte_upper128(struct dev_table_entry *ptr, struct
> >> dev_table_entry *new)
> >> +{
> >> + struct dev_table_entry old = {};
> >> +
> >> + do {
> >> + old.data128[1] = ptr->data128[1];
> >> + new->data[2] &= ~DTE_DATA2_INTR_MASK;
> >> + new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK |
> >> DTE_DATA2_RESV_MASK);
> >> + } while (!try_cmpxchg128(&ptr->data128[1], &old.data128[1],
> >> new->data128[1]));
> >
> > Please note that try_cmpxchg inherently updates &old.data128[1] above on
> > failure. There is no need to update value again in the loop.
> >
> > Please also note that the value from ptr->data128[1] should be read
> > using READ_ONCE() to prevent compiler from merging, refetching or
> > reordering the read. Currently, there is no READ_ONCE() implemented for
> > __int128, so something like the attached patch should be used.
>
> Thanks for pointing this out. I will introduce the attached patch
> separately in this series on your behalf as author/sign-off, and review
> the current code to properly use the READ_ONCE().
FTR, for the mentioned patch:
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
for Co-authored-by: tag.
Thanks,
Uros.
On Mon, Oct 07, 2024 at 04:13:49AM +0000, Suravee Suthikulpanit wrote:
> +static void write_dte_upper128(struct dev_table_entry *ptr, struct dev_table_entry *new)
> +{
> + struct dev_table_entry old = {};
> +
> + do {
> + old.data128[1] = ptr->data128[1];
> + new->data[2] &= ~DTE_DATA2_INTR_MASK;
> + new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK | DTE_DATA2_RESV_MASK);
Why preserve the reserved bits? Shouldn't they be reserved by forced
to 0? Should have a comment explaining this
> +static void iommu_flush_dte_sync(struct amd_iommu *iommu, u16 devid)
> +{
You might consider re-ordering to avoid the function forward
declaration.
Looks fine otherwise
Jason
On 10/7/2024 9:06 PM, Jason Gunthorpe wrote:
> On Mon, Oct 07, 2024 at 04:13:49AM +0000, Suravee Suthikulpanit wrote:
>> +static void write_dte_upper128(struct dev_table_entry *ptr, struct dev_table_entry *new)
>> +{
>> + struct dev_table_entry old = {};
>> +
>> + do {
>> + old.data128[1] = ptr->data128[1];
>> + new->data[2] &= ~DTE_DATA2_INTR_MASK;
>> + new->data[2] |= old.data[2] & (DTE_DATA2_INTR_MASK | DTE_DATA2_RESV_MASK);
>
> Why preserve the reserved bits? Shouldn't they be reserved by forced
> to 0? Should have a comment explaining this
You are correct.
>> +static void iommu_flush_dte_sync(struct amd_iommu *iommu, u16 devid)
>> +{
>
> You might consider re-ordering to avoid the function forward
> declaration.
This will require moving a lot of other functions as well. We will
consider this in overall clean up later.
Thanks,
Suravee
> Looks fine otherwise
>
> Jason
© 2016 - 2024 Red Hat, Inc.