Re: [RFC PATCH v2 1/6] iommu: Evolve to support more scenarios of using IOPF
Hi Baolu,
On 2021/3/10 10:09, Lu Baolu wrote:
> Hi Shenming,
>
> On 3/9/21 2:22 PM, Shenming Lu wrote:
>> This patch follows the discussion here:
>>
>> https://lore.kernel.org/linux-acpi/YAaxjmJW+ZMvrhac@myrica/
>>
>> In order to support more scenarios of using IOPF (mainly consider
>> the nested extension), besides keeping IOMMU_DEV_FEAT_IOPF as a
>> general capability for whether delivering faults through the IOMMU,
>> we extend iommu_register_fault_handler() with flags and introduce
>> IOPF_REPORT_FLAT and IOPF_REPORT_NESTED to describe the page fault
>> reporting capability under a specific configuration.
>> IOPF_REPORT_NESTED needs additional info to indicate which level/stage
>> is concerned since the fault client may be interested in only one
>> level.
>>
>> Signed-off-by: Shenming Lu
>> ---
>> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +-
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++--
>> drivers/iommu/io-pgfault.c | 4 --
>> drivers/iommu/iommu.c | 56 ++-
>> include/linux/iommu.h | 21 ++-
>> include/uapi/linux/iommu.h | 3 +
>> 6 files changed, 85 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> index ee66d1f4cb81..5de9432349d4 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> @@ -482,7 +482,8 @@ static int arm_smmu_master_sva_enable_iopf(struct
>> arm_smmu_master *master)
>> if (ret)
>> return ret;
>> - ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
>> + ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
>> + IOPF_REPORT_FLAT, dev);
>> if (ret) {
>> iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
>> return ret;
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 363744df8d51..f40529d0075d 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1447,10 +1447,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device
>> *smmu, u64 *evt)
>> return -EOPNOTSUPP;
>> }
>> - /* Stage-2 is always pinned at the moment */
>> - if (evt[1] & EVTQ_1_S2)
>> - return -EFAULT;
>> -
>> if (evt[1] & EVTQ_1_RnW)
>> perm |= IOMMU_FAULT_PERM_READ;
>> else
>> @@ -1468,13 +1464,18 @@ static int arm_smmu_handle_evt(struct
>> arm_smmu_device *smmu, u64 *evt)
>> .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
>> .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
>> .perm = perm,
>> - .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
>> };
>> if (ssid_valid) {
>> flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
>> flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
>> }
>> +
>> + if (evt[1] & EVTQ_1_S2) {
>> + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_L2;
>> + flt->prm.addr = FIELD_GET(EVTQ_3_IPA, evt[3]);
>> + } else
>> + flt->prm.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]);
>> } else {
>> flt->type = IOMMU_FAULT_DMA_UNRECOV;
>> flt->event = (struct iommu_fault_unrecoverable) {
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 1df8c1dcae77..abf16e06bcf5 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -195,10 +195,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, void
>> *cookie)
>> lockdep_assert_held(>lock);
>> - if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> - /* Not a recoverable page fault */
>> - return -EOPNOTSUPP;
>> -
>
> Any reasons why do you want to remove this check?
My thought was to make the reporting cap more detailed: IOPF_REPORT_ is only
for recoverable
page faults (IOMMU_FAULT_PAGE_REQ), and we may add UNRECOV_FAULT_REPORT_ later
for unrecoverable
faults (IOMMU_FAULT_DMA_UNRECOV)...
>
>> /*
>> * As long as we're holding param->lock, the queue can't be unlinked
>> * from the device and therefore cannot disappear.
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index d0b0a15dba84..cb1d93b00f7d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1056,6 +1056,40 @@ int iommu_group_unregister_notifier(struct
>> iommu_group *group,
>> }
>> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>> +/*
>> + * iommu_update_device_fault_handler - Update the device fault handler via
>> flags
>> + * @dev: the device
>> + * @mask: bits(not set) to clear
>> + * @set: bits to set
>> + *
>> + * Update the device fault handler installed by
>> + *
[PATCH v2] iommu/tegra-smmu: Add pagetable mappings to debugfs
This patch dumps all active mapping entries from pagetable
to a debugfs directory named "mappings".
Ataching an example:
SWGROUP: hc
ASID: 0
reg: 0x250
PTB_ASID: 0xe0080004
as->pd_dma: 0x80004000
{
[1023] 0xf0080013 (1)
{
PTE RANGE PHYS IOVASIZEATTR
#1023 - #1023 0x122e5e0000xf000 0x1000 0x5
}
}
Total PDE count: 1
Total PTE count: 1
Signed-off-by: Nicolin Chen
---
Changelog
v2:
* Expanded mutex range to the entire function
* Added as->lock to protect pagetable walkthrough
* Replaced devm_kzalloc with devm_kcalloc for group_debug
* Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
* Dropped as->count check; added WARN_ON when as->count mismatches pd[pd_index]
v1: https://lkml.org/lkml/2020/9/26/70
drivers/iommu/tegra-smmu.c | 172 +++--
1 file changed, 167 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 97eb62f667d2..155735f6323f 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -19,6 +19,11 @@
#include
#include
+struct tegra_smmu_group_debug {
+ const struct tegra_smmu_swgroup *group;
+ void *priv;
+};
+
struct tegra_smmu_group {
struct list_head list;
struct tegra_smmu *smmu;
@@ -47,6 +52,8 @@ struct tegra_smmu {
struct dentry *debugfs;
struct iommu_device iommu; /* IOMMU Core code handle */
+
+ struct tegra_smmu_group_debug *group_debug;
};
struct tegra_smmu_as {
@@ -152,6 +159,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu,
unsigned long offset)
#define SMMU_PDE_ATTR (SMMU_PDE_READABLE | SMMU_PDE_WRITABLE | \
SMMU_PDE_NONSECURE)
+#define SMMU_PTE_ATTR (SMMU_PTE_READABLE | SMMU_PTE_WRITABLE | \
+SMMU_PTE_NONSECURE)
+#define SMMU_PTE_ATTR_SHIFT(29)
static unsigned int iova_pd_index(unsigned long iova)
{
@@ -163,6 +173,12 @@ static unsigned int iova_pt_index(unsigned long iova)
return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
}
+static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int
pt_index)
+{
+ return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
+ ((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
+}
+
static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
{
addr >>= 12;
@@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
*domain)
}
static const struct tegra_smmu_swgroup *
-tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
+tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
*index)
{
const struct tegra_smmu_swgroup *group = NULL;
unsigned int i;
@@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
int swgroup)
for (i = 0; i < smmu->soc->num_swgroups; i++) {
if (smmu->soc->swgroups[i].swgroup == swgroup) {
group = >soc->swgroups[i];
+ if (index)
+ *index = i;
break;
}
}
@@ -350,19 +368,22 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
int swgroup)
}
static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
- unsigned int asid)
+ struct tegra_smmu_as *as)
{
const struct tegra_smmu_swgroup *group;
+ unsigned int asid = as->id;
unsigned int i;
u32 value;
- group = tegra_smmu_find_swgroup(smmu, swgroup);
+ group = tegra_smmu_find_swgroup(smmu, swgroup, );
if (group) {
value = smmu_readl(smmu, group->reg);
value &= ~SMMU_ASID_MASK;
value |= SMMU_ASID_VALUE(asid);
value |= SMMU_ASID_ENABLE;
smmu_writel(smmu, value, group->reg);
+ if (smmu->group_debug)
+ smmu->group_debug[i].priv = as;
} else {
pr_warn("%s group from swgroup %u not found\n", __func__,
swgroup);
@@ -389,13 +410,15 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu,
unsigned int swgroup,
unsigned int i;
u32 value;
- group = tegra_smmu_find_swgroup(smmu, swgroup);
+ group = tegra_smmu_find_swgroup(smmu, swgroup, );
if (group) {
value = smmu_readl(smmu, group->reg);
value &= ~SMMU_ASID_MASK;
value |= SMMU_ASID_VALUE(asid);
value &= ~SMMU_ASID_ENABLE;
smmu_writel(smmu, value, group->reg);
+ if (smmu->group_debug)
+ smmu->group_debug[i].priv = NULL;
}
for (i = 0; i <
Re: [RFC PATCH v2 1/6] iommu: Evolve to support more scenarios of using IOPF
Hi Shenming,
On 3/9/21 2:22 PM, Shenming Lu wrote:
This patch follows the discussion here:
https://lore.kernel.org/linux-acpi/YAaxjmJW+ZMvrhac@myrica/
In order to support more scenarios of using IOPF (mainly consider
the nested extension), besides keeping IOMMU_DEV_FEAT_IOPF as a
general capability for whether delivering faults through the IOMMU,
we extend iommu_register_fault_handler() with flags and introduce
IOPF_REPORT_FLAT and IOPF_REPORT_NESTED to describe the page fault
reporting capability under a specific configuration.
IOPF_REPORT_NESTED needs additional info to indicate which level/stage
is concerned since the fault client may be interested in only one
level.
Signed-off-by: Shenming Lu
---
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++--
drivers/iommu/io-pgfault.c| 4 --
drivers/iommu/iommu.c | 56 ++-
include/linux/iommu.h | 21 ++-
include/uapi/linux/iommu.h| 3 +
6 files changed, 85 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index ee66d1f4cb81..5de9432349d4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -482,7 +482,8 @@ static int arm_smmu_master_sva_enable_iopf(struct
arm_smmu_master *master)
if (ret)
return ret;
- ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
+ ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
+ IOPF_REPORT_FLAT, dev);
if (ret) {
iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
return ret;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 363744df8d51..f40529d0075d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1447,10 +1447,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device
*smmu, u64 *evt)
return -EOPNOTSUPP;
}
- /* Stage-2 is always pinned at the moment */
- if (evt[1] & EVTQ_1_S2)
- return -EFAULT;
-
if (evt[1] & EVTQ_1_RnW)
perm |= IOMMU_FAULT_PERM_READ;
else
@@ -1468,13 +1464,18 @@ static int arm_smmu_handle_evt(struct arm_smmu_device
*smmu, u64 *evt)
.flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
.grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
.perm = perm,
- .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
};
if (ssid_valid) {
flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
}
+
+ if (evt[1] & EVTQ_1_S2) {
+ flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_L2;
+ flt->prm.addr = FIELD_GET(EVTQ_3_IPA, evt[3]);
+ } else
+ flt->prm.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]);
} else {
flt->type = IOMMU_FAULT_DMA_UNRECOV;
flt->event = (struct iommu_fault_unrecoverable) {
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1df8c1dcae77..abf16e06bcf5 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -195,10 +195,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, void
*cookie)
lockdep_assert_held(>lock);
- if (fault->type != IOMMU_FAULT_PAGE_REQ)
- /* Not a recoverable page fault */
- return -EOPNOTSUPP;
-
Any reasons why do you want to remove this check?
/*
* As long as we're holding param->lock, the queue can't be unlinked
* from the device and therefore cannot disappear.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..cb1d93b00f7d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1056,6 +1056,40 @@ int iommu_group_unregister_notifier(struct iommu_group
*group,
}
EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
+/*
+ * iommu_update_device_fault_handler - Update the device fault handler via
flags
+ * @dev: the device
+ * @mask: bits(not set) to clear
+ * @set: bits to set
+ *
+ * Update the device fault handler installed by
+ * iommu_register_device_fault_handler().
+ *
+ * Return 0 on success, or an error.
+ */
+int iommu_update_device_fault_handler(struct device *dev, u32 mask, u32 set)
+{
+ struct dev_iommu *param = dev->iommu;
+ int ret = 0;
+
+ if (!param)
+ return -EINVAL;
+
+ mutex_lock(>lock);
+
+ if (param->fault_param) {
+ ret = -EINVAL;
+ goto
Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m
Am 09.03.21 um 18:59 schrieb Alex Deucher: On Tue, Mar 9, 2021 at 12:55 PM Jean-Philippe Brucker wrote: Hi Felix, On Tue, Mar 09, 2021 at 11:30:19AM -0500, Felix Kuehling wrote: I think the proper fix would be to not rely on custom hooks into a particular IOMMU driver, but to instead ensure that the amdgpu driver can do everything it needs through the regular linux/iommu.h interfaces. I realize this is more work, but I wonder if you've tried that, and why it didn't work out. As far as I know this hasn't been tried. I see that intel-iommu has its own SVM thing, which seems to be similar to what our IOMMUv2 does. I guess we'd have to abstract that into a common API. The common API was added in 26b25a2b98e4 and implemented by the Intel driver in 064a57d7ddfc. To support it an IOMMU driver implements new IOMMU ops: .dev_has_feat() .dev_feat_enabled() .dev_enable_feat() .dev_disable_feat() .sva_bind() .sva_unbind() .sva_get_pasid() And a device driver calls iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA) followed by iommu_sva_bind_device(). If I remember correctly the biggest obstacle for KFD is the PASID allocation, done by the GPU driver instead of the IOMMU driver, but there may be others. IIRC, we tried to make the original IOMMUv2 functionality generic but other vendors were not interested at the time, so it ended up being AMD specific and since nothing else was using the pasid allocations we put them in the GPU driver. I guess if this is generic now, it could be moved to a common API and taken out of the driver. There has been quite some effort for this already for generic PASID interface etc.. But it looks like that effort is stalled by now. Anyway at least I'm perfectly fine to have the IOMMUv2 || !IOMMUv2 dependency on the core amdgpu driver for x86. That should solve the build problem at hand quite nicely. Regards, Christian. Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m
On Tue, Mar 9, 2021 at 12:55 PM Jean-Philippe Brucker wrote: > > Hi Felix, > > On Tue, Mar 09, 2021 at 11:30:19AM -0500, Felix Kuehling wrote: > > > I think the proper fix would be to not rely on custom hooks into a > > > particular > > > IOMMU driver, but to instead ensure that the amdgpu driver can do > > > everything > > > it needs through the regular linux/iommu.h interfaces. I realize this > > > is more work, > > > but I wonder if you've tried that, and why it didn't work out. > > > > As far as I know this hasn't been tried. I see that intel-iommu has its > > own SVM thing, which seems to be similar to what our IOMMUv2 does. I > > guess we'd have to abstract that into a common API. > > The common API was added in 26b25a2b98e4 and implemented by the Intel > driver in 064a57d7ddfc. To support it an IOMMU driver implements new IOMMU > ops: > .dev_has_feat() > .dev_feat_enabled() > .dev_enable_feat() > .dev_disable_feat() > .sva_bind() > .sva_unbind() > .sva_get_pasid() > > And a device driver calls iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA) > followed by iommu_sva_bind_device(). > > If I remember correctly the biggest obstacle for KFD is the PASID > allocation, done by the GPU driver instead of the IOMMU driver, but there > may be others. IIRC, we tried to make the original IOMMUv2 functionality generic but other vendors were not interested at the time, so it ended up being AMD specific and since nothing else was using the pasid allocations we put them in the GPU driver. I guess if this is generic now, it could be moved to a common API and taken out of the driver. Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m
Hi Felix, On Tue, Mar 09, 2021 at 11:30:19AM -0500, Felix Kuehling wrote: > > I think the proper fix would be to not rely on custom hooks into a > > particular > > IOMMU driver, but to instead ensure that the amdgpu driver can do everything > > it needs through the regular linux/iommu.h interfaces. I realize this > > is more work, > > but I wonder if you've tried that, and why it didn't work out. > > As far as I know this hasn't been tried. I see that intel-iommu has its > own SVM thing, which seems to be similar to what our IOMMUv2 does. I > guess we'd have to abstract that into a common API. The common API was added in 26b25a2b98e4 and implemented by the Intel driver in 064a57d7ddfc. To support it an IOMMU driver implements new IOMMU ops: .dev_has_feat() .dev_feat_enabled() .dev_enable_feat() .dev_disable_feat() .sva_bind() .sva_unbind() .sva_get_pasid() And a device driver calls iommu_dev_enable_feature(IOMMU_DEV_FEAT_SVA) followed by iommu_sva_bind_device(). If I remember correctly the biggest obstacle for KFD is the PASID allocation, done by the GPU driver instead of the IOMMU driver, but there may be others. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m
Am 2021-03-09 um 3:58 a.m. schrieb Arnd Bergmann: > On Tue, Mar 9, 2021 at 4:23 AM Felix Kuehling wrote: >> Using 'imply AMD_IOMMU_V2' does not guarantee that the driver can link >> against the exported functions. If the GPU driver is built-in but the >> IOMMU driver is a loadable module, the kfd_iommu.c file is indeed >> built but does not work: >> >> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function >> `kfd_iommu_bind_process_to_device': >> kfd_iommu.c:(.text+0x516): undefined reference to `amd_iommu_bind_pasid' >> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function >> `kfd_iommu_unbind_process': >> kfd_iommu.c:(.text+0x691): undefined reference to `amd_iommu_unbind_pasid' >> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function >> `kfd_iommu_suspend': >> kfd_iommu.c:(.text+0x966): undefined reference to >> `amd_iommu_set_invalidate_ctx_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0x97f): undefined reference to >> `amd_iommu_set_invalid_ppr_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0x9a4): undefined reference to >> `amd_iommu_free_device' >> x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function >> `kfd_iommu_resume': >> kfd_iommu.c:(.text+0xa9a): undefined reference to `amd_iommu_init_device' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xadc): undefined reference to >> `amd_iommu_set_invalidate_ctx_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xaff): undefined reference to >> `amd_iommu_set_invalid_ppr_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xc72): undefined reference to >> `amd_iommu_bind_pasid' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xe08): undefined reference to >> `amd_iommu_set_invalidate_ctx_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xe26): undefined reference to >> `amd_iommu_set_invalid_ppr_cb' >> x86_64-linux-ld: kfd_iommu.c:(.text+0xe42): undefined reference to >> `amd_iommu_free_device' >> >> Use IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols >> are reachable by the amdkfd driver. Output a warning if they are not, >> because that may not be what the user was expecting. > This would fix the compile-time failure, but it still fails my CI > because you introduce > a compile-time warning. Can you change it into a runtime warning instead? Sure. > > Neither type of warning is likely to actually reach the person trying > to debug the > problem, so you still end up with machines that don't do what they should, > but I can live with the runtime warning as long as the build doesn't break. OK. > > I think the proper fix would be to not rely on custom hooks into a particular > IOMMU driver, but to instead ensure that the amdgpu driver can do everything > it needs through the regular linux/iommu.h interfaces. I realize this > is more work, > but I wonder if you've tried that, and why it didn't work out. As far as I know this hasn't been tried. I see that intel-iommu has its own SVM thing, which seems to be similar to what our IOMMUv2 does. I guess we'd have to abstract that into a common API. Regards, Felix > >Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu/iova: Improve restart logic
On 05/03/2021 16:35, Robin Murphy wrote:
Hi Robin,
When restarting after searching below the cached node fails, resetting
the start point to the anchor node is often overly pessimistic. If
allocations are made with mixed limits - particularly in the case of the
opportunistic 32-bit allocation for PCI devices - this could mean
significant time wasted walking through the whole populated upper range
just to reach the initial limit.
Right
We can improve on that by implementing
a proper tree traversal to find the first node above the relevant limit,
and set the exact start point.
Thanks for this. However, as mentioned in the other thread, this does
not help our performance regression Re: commit 4e89dce72521.
And mentioning this "retry" approach again, in case it was missed, from
my experiment on the affected HW, it also has generally a dreadfully low
success rate - less than 1% for the 32b range retry. Retry rate is about
20%. So I am saying from this 20%, less than 1% of those succeed.
Failing faster sounds key.
Thanks,
John
Signed-off-by: Robin Murphy
---
drivers/iommu/iova.c | 39 ++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index c28003e1d2ee..471c48dd71e7 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -154,6 +154,43 @@ __cached_rbnode_delete_update(struct iova_domain *iovad,
struct iova *free)
iovad->cached_node = rb_next(>node);
}
+static struct rb_node *iova_find_limit(struct iova_domain *iovad, unsigned long limit_pfn)
+{
+ struct rb_node *node, *next;
+ /*
+* Ideally what we'd like to judge here is whether limit_pfn is close
+* enough to the highest-allocated IOVA that starting the allocation
+* walk from the anchor node will be quicker than this initial work to
+* find an exact starting point (especially if that ends up being the
+* anchor node anyway). This is an incredibly crude approximation which
+* only really helps the most likely case, but is at least trivially
easy.
+*/
+ if (limit_pfn > iovad->dma_32bit_pfn)
+ return >anchor.node;
+
+ node = iovad->rbroot.rb_node;
+ while (to_iova(node)->pfn_hi < limit_pfn)
+ node = node->rb_right;
+
+search_left:
+ while (node->rb_left && to_iova(node->rb_left)->pfn_lo >= limit_pfn)
+ node = node->rb_left;
+
+ if (!node->rb_left)
+ return node;
+
+ next = node->rb_left;
+ while (next->rb_right) {
+ next = next->rb_right;
+ if (to_iova(next)->pfn_lo >= limit_pfn) {
+ node = next;
+ goto search_left;
+ }
+ }
+
+ return node;
+}
+
/* Insert the iova into domain rbtree by holding writer lock */
static void
iova_insert_rbtree(struct rb_root *root, struct iova *iova,
@@ -219,7 +256,7 @@ static int __alloc_and_insert_iova_range(struct iova_domain
*iovad,
if (low_pfn == iovad->start_pfn && retry_pfn < limit_pfn) {
high_pfn = limit_pfn;
low_pfn = retry_pfn;
- curr = >anchor.node;
+ curr = iova_find_limit(iovad, limit_pfn);
curr_iova = to_iova(curr);
goto retry;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu: Check dev->iommu in iommu_dev_xxx functions
On 2021-03-03 17:36, Shameer Kolothum wrote:
The device iommu probe/attach might have failed leaving dev->iommu
to NULL and device drivers may still invoke these functions resulting
in a crash in iommu vendor driver code.
Hence make sure we check that.
Reviewed-by: Robin Murphy
Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device")
Signed-off-by: Shameer Kolothum
---
v2 --> v3
-Removed iommu_ops from struct dev_iommu.
v1 --> v2:
-Added iommu_ops to struct dev_iommu based on the discussion with Robin.
-Rebased against iommu-tree core branch.
---
drivers/iommu/iommu.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..e10cfa99057c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2878,10 +2878,12 @@ EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
*/
int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
{
- const struct iommu_ops *ops = dev->bus->iommu_ops;
+ if (dev->iommu && dev->iommu->iommu_dev) {
+ const struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
- if (ops && ops->dev_enable_feat)
- return ops->dev_enable_feat(dev, feat);
+ if (ops->dev_enable_feat)
+ return ops->dev_enable_feat(dev, feat);
+ }
return -ENODEV;
}
@@ -2894,10 +2896,12 @@ EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
*/
int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features
feat)
{
- const struct iommu_ops *ops = dev->bus->iommu_ops;
+ if (dev->iommu && dev->iommu->iommu_dev) {
+ const struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
- if (ops && ops->dev_disable_feat)
- return ops->dev_disable_feat(dev, feat);
+ if (ops->dev_disable_feat)
+ return ops->dev_disable_feat(dev, feat);
+ }
return -EBUSY;
}
@@ -2905,10 +2909,12 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat)
{
- const struct iommu_ops *ops = dev->bus->iommu_ops;
+ if (dev->iommu && dev->iommu->iommu_dev) {
+ const struct iommu_ops *ops = dev->iommu->iommu_dev->ops;
- if (ops && ops->dev_feat_enabled)
- return ops->dev_feat_enabled(dev, feat);
+ if (ops->dev_feat_enabled)
+ return ops->dev_feat_enabled(dev, feat);
+ }
return false;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC PATCH 3/5] KVM: ARM64: Add support for pinned VMIDs
Hi Marc, > -Original Message- > From: Marc Zyngier [mailto:m...@kernel.org] > Sent: 09 March 2021 10:33 > To: Shameerali Kolothum Thodi > Cc: linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org; > kvm...@lists.cs.columbia.edu; alex.william...@redhat.com; > jean-phili...@linaro.org; eric.au...@redhat.com; zhangfei@linaro.org; > Jonathan Cameron ; Zengtao (B) > ; linux...@openeuler.org; Will Deacon > > Subject: Re: [RFC PATCH 3/5] KVM: ARM64: Add support for pinned VMIDs > > Hi Shameer, > > [+Will] > > On Mon, 22 Feb 2021 15:53:36 +, > Shameer Kolothum wrote: > > > > On an ARM64 system with a SMMUv3 implementation that fully supports > > Broadcast TLB Maintenance(BTM) feature, the CPU TLB invalidate > > instructions are received by SMMU. This is very useful when the > > SMMU shares the page tables with the CPU(eg: Guest SVA use case). > > For this to work, the SMMU must use the same VMID that is allocated > > by KVM to configure the stage 2 translations. > > > > At present KVM VMID allocations are recycled on rollover and may > > change as a result. This will create issues if we have to share > > the KVM VMID with SMMU. Hence, we spilt the KVM VMID space into > > two, the first half follows the normal recycle on rollover policy > > while the second half of the VMID pace is used to allocate pinned > > VMIDs. This feature is enabled based on a command line option > > "kvm-arm.pinned_vmid_enable". > > I think this is the wrong approach. Instead of shoving the notion of > pinned VMID into the current allocator, which really isn't designed > for this, it'd be a lot better if we aligned the KVM VMID allocator > with the ASID allocator, which already has support for pinning and is > in general much more efficient. Ok. Agree that this is not efficient, but was easy to prototype something :) > Julien Grall worked on such a series[1] a long while ago, which got > stalled because of the 32bit KVM port. Since we don't have this burden > anymore, I'd rather you look in that direction instead of wasting half > of the VMID space on potentially pinned VMIDs. Sure. I will check that and work on it. Thanks, Shameer > Thanks, > > M. > > [1] > https://patchwork.kernel.org/project/linux-arm-kernel/cover/20190724162534 > .7390-1-julien.gr...@arm.com/ > > > -- > Without deviation from the norm, progress is not possible. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 3/5] KVM: ARM64: Add support for pinned VMIDs
Hi Shameer, [+Will] On Mon, 22 Feb 2021 15:53:36 +, Shameer Kolothum wrote: > > On an ARM64 system with a SMMUv3 implementation that fully supports > Broadcast TLB Maintenance(BTM) feature, the CPU TLB invalidate > instructions are received by SMMU. This is very useful when the > SMMU shares the page tables with the CPU(eg: Guest SVA use case). > For this to work, the SMMU must use the same VMID that is allocated > by KVM to configure the stage 2 translations. > > At present KVM VMID allocations are recycled on rollover and may > change as a result. This will create issues if we have to share > the KVM VMID with SMMU. Hence, we spilt the KVM VMID space into > two, the first half follows the normal recycle on rollover policy > while the second half of the VMID pace is used to allocate pinned > VMIDs. This feature is enabled based on a command line option > "kvm-arm.pinned_vmid_enable". I think this is the wrong approach. Instead of shoving the notion of pinned VMID into the current allocator, which really isn't designed for this, it'd be a lot better if we aligned the KVM VMID allocator with the ASID allocator, which already has support for pinning and is in general much more efficient. Julien Grall worked on such a series[1] a long while ago, which got stalled because of the 32bit KVM port. Since we don't have this burden anymore, I'd rather you look in that direction instead of wasting half of the VMID space on potentially pinned VMIDs. Thanks, M. [1] https://patchwork.kernel.org/project/linux-arm-kernel/cover/20190724162534.7390-1-julien.gr...@arm.com/ -- Without deviation from the norm, progress is not possible. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] drm/amdkfd: fix build error with AMD_IOMMU_V2=m
On Tue, Mar 9, 2021 at 4:23 AM Felix Kuehling wrote: > > Using 'imply AMD_IOMMU_V2' does not guarantee that the driver can link > against the exported functions. If the GPU driver is built-in but the > IOMMU driver is a loadable module, the kfd_iommu.c file is indeed > built but does not work: > > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function > `kfd_iommu_bind_process_to_device': > kfd_iommu.c:(.text+0x516): undefined reference to `amd_iommu_bind_pasid' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function > `kfd_iommu_unbind_process': > kfd_iommu.c:(.text+0x691): undefined reference to `amd_iommu_unbind_pasid' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function > `kfd_iommu_suspend': > kfd_iommu.c:(.text+0x966): undefined reference to > `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0x97f): undefined reference to > `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0x9a4): undefined reference to > `amd_iommu_free_device' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function > `kfd_iommu_resume': > kfd_iommu.c:(.text+0xa9a): undefined reference to `amd_iommu_init_device' > x86_64-linux-ld: kfd_iommu.c:(.text+0xadc): undefined reference to > `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xaff): undefined reference to > `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xc72): undefined reference to > `amd_iommu_bind_pasid' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe08): undefined reference to > `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe26): undefined reference to > `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe42): undefined reference to > `amd_iommu_free_device' > > Use IS_REACHABLE to only build IOMMU-V2 support if the amd_iommu symbols > are reachable by the amdkfd driver. Output a warning if they are not, > because that may not be what the user was expecting. This would fix the compile-time failure, but it still fails my CI because you introduce a compile-time warning. Can you change it into a runtime warning instead? Neither type of warning is likely to actually reach the person trying to debug the problem, so you still end up with machines that don't do what they should, but I can live with the runtime warning as long as the build doesn't break. I think the proper fix would be to not rely on custom hooks into a particular IOMMU driver, but to instead ensure that the amdgpu driver can do everything it needs through the regular linux/iommu.h interfaces. I realize this is more work, but I wonder if you've tried that, and why it didn't work out. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
