Add iopf-capable hw page table attach/detach/replace helpers. The pointer
to iommufd_device is stored in the domain attachment handle, so that it
can be echo'ed back in the iopf_group.
The iopf-capable hw page tables can only be attached to devices that
support the IOMMU_DEV_FEAT_IOPF feature. On the first attachment of an
iopf-capable hw_pagetable to the device, the IOPF feature is enabled on
the device. Similarly, after the last iopf-capable hwpt is detached from
the device, the IOPF feature is disabled on the device.
The current implementation allows a replacement between iopf-capable and
non-iopf-capable hw page tables. This matches the nested translation use
case, where a parent domain is attached by default and can then be
replaced with a nested user domain with iopf support.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
drivers/iommu/iommufd/iommufd_private.h | 41 +++++
drivers/iommu/iommufd/device.c | 7 +-
drivers/iommu/iommufd/fault.c | 190 ++++++++++++++++++++++++
3 files changed, 235 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index c8a4519f1405..aa4c26c87cb9 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -11,6 +11,7 @@
#include <linux/iommu.h>
#include <linux/iova_bitmap.h>
#include <uapi/linux/iommufd.h>
+#include "../iommu-priv.h"
struct iommu_domain;
struct iommu_group;
@@ -293,6 +294,7 @@ int iommufd_check_iova_range(struct io_pagetable *iopt,
struct iommufd_hw_pagetable {
struct iommufd_object obj;
struct iommu_domain *domain;
+ struct iommufd_fault *fault;
};
struct iommufd_hwpt_paging {
@@ -396,6 +398,9 @@ struct iommufd_device {
/* always the physical device */
struct device *dev;
bool enforce_cache_coherency;
+ /* protect iopf_enabled counter */
+ struct mutex iopf_lock;
+ unsigned int iopf_enabled;
};
static inline struct iommufd_device *
@@ -456,6 +461,42 @@ struct iommufd_attach_handle {
int iommufd_fault_alloc(struct iommufd_ucmd *ucmd);
void iommufd_fault_destroy(struct iommufd_object *obj);
+int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev);
+void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev);
+int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
+ struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_hw_pagetable *old);
+
+static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev)
+{
+ if (hwpt->fault)
+ return iommufd_fault_domain_attach_dev(hwpt, idev);
+
+ return iommu_attach_group(hwpt->domain, idev->igroup->group);
+}
+
+static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev)
+{
+ if (hwpt->fault)
+ iommufd_fault_domain_detach_dev(hwpt, idev);
+
+ iommu_detach_group(hwpt->domain, idev->igroup->group);
+}
+
+static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
+ struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_hw_pagetable *old)
+{
+ if (old->fault || hwpt->fault)
+ return iommufd_fault_domain_replace_dev(idev, hwpt, old);
+
+ return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
+}
+
#ifdef CONFIG_IOMMUFD_TEST
int iommufd_test(struct iommufd_ucmd *ucmd);
void iommufd_selftest_destroy(struct iommufd_object *obj);
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 873630c111c1..9a7ec5997c61 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -215,6 +215,7 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
refcount_inc(&idev->obj.users);
/* igroup refcount moves into iommufd_device */
idev->igroup = igroup;
+ mutex_init(&idev->iopf_lock);
/*
* If the caller fails after this success it must call
@@ -376,7 +377,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
* attachment.
*/
if (list_empty(&idev->igroup->device_list)) {
- rc = iommu_attach_group(hwpt->domain, idev->igroup->group);
+ rc = iommufd_hwpt_attach_device(hwpt, idev);
if (rc)
goto err_unresv;
idev->igroup->hwpt = hwpt;
@@ -402,7 +403,7 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev)
mutex_lock(&idev->igroup->lock);
list_del(&idev->group_item);
if (list_empty(&idev->igroup->device_list)) {
- iommu_detach_group(hwpt->domain, idev->igroup->group);
+ iommufd_hwpt_detach_device(hwpt, idev);
idev->igroup->hwpt = NULL;
}
if (hwpt_is_paging(hwpt))
@@ -497,7 +498,7 @@ iommufd_device_do_replace(struct iommufd_device *idev,
goto err_unlock;
}
- rc = iommu_group_replace_domain(igroup->group, hwpt->domain);
+ rc = iommufd_hwpt_replace_device(idev, hwpt, old_hwpt);
if (rc)
goto err_unresv;
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 68ff94671d48..4934ae572638 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -8,6 +8,7 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/iommufd.h>
+#include <linux/pci.h>
#include <linux/poll.h>
#include <linux/anon_inodes.h>
#include <uapi/linux/iommufd.h>
@@ -15,6 +16,195 @@
#include "../iommu-priv.h"
#include "iommufd_private.h"
+static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
+{
+ struct device *dev = idev->dev;
+ int ret;
+
+ /*
+ * Once we turn on PCI/PRI support for VF, the response failure code
+ * should not be forwarded to the hardware due to PRI being a shared
+ * resource between PF and VFs. There is no coordination for this
+ * shared capability. This waits for a vPRI reset to recover.
+ */
+ if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
+ return -EINVAL;
+
+ mutex_lock(&idev->iopf_lock);
+ /* Device iopf has already been on. */
+ if (++idev->iopf_enabled > 1) {
+ mutex_unlock(&idev->iopf_lock);
+ return 0;
+ }
+
+ ret = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_IOPF);
+ if (ret)
+ --idev->iopf_enabled;
+ mutex_unlock(&idev->iopf_lock);
+
+ return ret;
+}
+
+static void iommufd_fault_iopf_disable(struct iommufd_device *idev)
+{
+ mutex_lock(&idev->iopf_lock);
+ if (!WARN_ON(idev->iopf_enabled == 0)) {
+ if (--idev->iopf_enabled == 0)
+ iommu_dev_disable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
+ }
+ mutex_unlock(&idev->iopf_lock);
+}
+
+static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev)
+{
+ struct iommufd_attach_handle *handle;
+ int ret;
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return -ENOMEM;
+
+ handle->idev = idev;
+ ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,
+ &handle->handle);
+ if (ret)
+ kfree(handle);
+
+ return ret;
+}
+
+int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev)
+{
+ int ret;
+
+ if (!hwpt->fault)
+ return -EINVAL;
+
+ ret = iommufd_fault_iopf_enable(idev);
+ if (ret)
+ return ret;
+
+ ret = __fault_domain_attach_dev(hwpt, idev);
+ if (ret)
+ iommufd_fault_iopf_disable(idev);
+
+ return ret;
+}
+
+static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_attach_handle *handle)
+{
+ struct iommufd_fault *fault = hwpt->fault;
+ struct iopf_group *group, *next;
+ unsigned long index;
+
+ if (!fault)
+ return;
+
+ mutex_lock(&fault->mutex);
+ list_for_each_entry_safe(group, next, &fault->deliver, node) {
+ if (group->attach_handle != &handle->handle)
+ continue;
+ list_del(&group->node);
+ iopf_group_response(group, IOMMU_PAGE_RESP_INVALID);
+ iopf_free_group(group);
+ }
+
+ xa_for_each(&fault->response, index, group) {
+ if (group->attach_handle != &handle->handle)
+ continue;
+ xa_erase(&fault->response, index);
+ iopf_group_response(group, IOMMU_PAGE_RESP_INVALID);
+ iopf_free_group(group);
+ }
+ mutex_unlock(&fault->mutex);
+}
+
+static struct iommufd_attach_handle *
+iommufd_device_get_attach_handle(struct iommufd_device *idev)
+{
+ struct iommu_attach_handle *handle;
+
+ handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
+ if (!handle)
+ return NULL;
+
+ return to_iommufd_handle(handle);
+}
+
+void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_device *idev)
+{
+ struct iommufd_attach_handle *handle;
+
+ handle = iommufd_device_get_attach_handle(idev);
+ iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
+ iommufd_auto_response_faults(hwpt, handle);
+ iommufd_fault_iopf_disable(idev);
+ kfree(handle);
+}
+
+static int __fault_domain_replace_dev(struct iommufd_device *idev,
+ struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_hw_pagetable *old)
+{
+ struct iommufd_attach_handle *handle, *curr = NULL;
+ int ret;
+
+ if (old->fault)
+ curr = iommufd_device_get_attach_handle(idev);
+
+ if (hwpt->fault) {
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return -ENOMEM;
+
+ handle->handle.domain = hwpt->domain;
+ handle->idev = idev;
+ ret = iommu_replace_group_handle(idev->igroup->group,
+ hwpt->domain, &handle->handle);
+ } else {
+ ret = iommu_replace_group_handle(idev->igroup->group,
+ hwpt->domain, NULL);
+ }
+
+ if (!ret && curr) {
+ iommufd_auto_response_faults(old, curr);
+ kfree(curr);
+ }
+
+ return ret;
+}
+
+int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
+ struct iommufd_hw_pagetable *hwpt,
+ struct iommufd_hw_pagetable *old)
+{
+ bool iopf_off = !hwpt->fault && old->fault;
+ bool iopf_on = hwpt->fault && !old->fault;
+ int ret;
+
+ if (iopf_on) {
+ ret = iommufd_fault_iopf_enable(idev);
+ if (ret)
+ return ret;
+ }
+
+ ret = __fault_domain_replace_dev(idev, hwpt, old);
+ if (ret) {
+ if (iopf_on)
+ iommufd_fault_iopf_disable(idev);
+ return ret;
+ }
+
+ if (iopf_off)
+ iommufd_fault_iopf_disable(idev);
+
+ return 0;
+}
+
void iommufd_fault_destroy(struct iommufd_object *obj)
{
struct iommufd_fault *fault = container_of(obj, struct iommufd_fault, obj);
--
2.34.1Hi, Baolu
On Tue, 2 Jul 2024 at 14:39, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Add iopf-capable hw page table attach/detach/replace helpers. The pointer
> to iommufd_device is stored in the domain attachment handle, so that it
> can be echo'ed back in the iopf_group.
>
> The iopf-capable hw page tables can only be attached to devices that
> support the IOMMU_DEV_FEAT_IOPF feature. On the first attachment of an
> iopf-capable hw_pagetable to the device, the IOPF feature is enabled on
> the device. Similarly, after the last iopf-capable hwpt is detached from
> the device, the IOPF feature is disabled on the device.
>
> The current implementation allows a replacement between iopf-capable and
> non-iopf-capable hw page tables. This matches the nested translation use
> case, where a parent domain is attached by default and can then be
> replaced with a nested user domain with iopf support.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 41 +++++
> drivers/iommu/iommufd/device.c | 7 +-
> drivers/iommu/iommufd/fault.c | 190 ++++++++++++++++++++++++
> 3 files changed, 235 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 68ff94671d48..4934ae572638 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -8,6 +8,7 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/iommufd.h>
> +#include <linux/pci.h>
> #include <linux/poll.h>
> #include <linux/anon_inodes.h>
> #include <uapi/linux/iommufd.h>
> @@ -15,6 +16,195 @@
> #include "../iommu-priv.h"
> #include "iommufd_private.h"
>
> +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> +{
> + struct device *dev = idev->dev;
> + int ret;
> +
> + /*
> + * Once we turn on PCI/PRI support for VF, the response failure code
> + * should not be forwarded to the hardware due to PRI being a shared
> + * resource between PF and VFs. There is no coordination for this
> + * shared capability. This waits for a vPRI reset to recover.
> + */
> + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> + return -EINVAL;
I am using the SMMUv3 stall feature, and need to forward this to hardware,
And now I am hacking to comment this check.
Any suggestions?
> +
> + mutex_lock(&idev->iopf_lock);
> + /* Device iopf has already been on. */
> + if (++idev->iopf_enabled > 1) {
> + mutex_unlock(&idev->iopf_lock);
> + return 0;
> + }
> +
> + ret = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_IOPF);
> + if (ret)
> + --idev->iopf_enabled;
> + mutex_unlock(&idev->iopf_lock);
Also iommu_dev_enable_feature(idev->dev, IOMMU_DEV_FEAT_SVA); is required
In thinking how to add it properly.
Thanks
On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > +{
> > + struct device *dev = idev->dev;
> > + int ret;
> > +
> > + /*
> > + * Once we turn on PCI/PRI support for VF, the response failure code
> > + * should not be forwarded to the hardware due to PRI being a shared
> > + * resource between PF and VFs. There is no coordination for this
> > + * shared capability. This waits for a vPRI reset to recover.
> > + */
> > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > + return -EINVAL;
>
> I am using the SMMUv3 stall feature, and need to forward this to hardware,
> And now I am hacking to comment this check.
> Any suggestions?
Are you using PCI SRIOV and stall together?
> > + mutex_lock(&idev->iopf_lock);
> > + /* Device iopf has already been on. */
> > + if (++idev->iopf_enabled > 1) {
> > + mutex_unlock(&idev->iopf_lock);
> > + return 0;
> > + }
> > +
> > + ret = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_IOPF);
> > + if (ret)
> > + --idev->iopf_enabled;
> > + mutex_unlock(&idev->iopf_lock);
>
> Also iommu_dev_enable_feature(idev->dev, IOMMU_DEV_FEAT_SVA); is required
> In thinking how to add it properly.
FEAT_SVA needs to be deleted, not added too.
smmu-v3 needs some more fixing to move that
arm_smmu_master_enable_sva() logic into domain attachment.
Jason
On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > > +{
> > > + struct device *dev = idev->dev;
> > > + int ret;
> > > +
> > > + /*
> > > + * Once we turn on PCI/PRI support for VF, the response failure code
> > > + * should not be forwarded to the hardware due to PRI being a shared
> > > + * resource between PF and VFs. There is no coordination for this
> > > + * shared capability. This waits for a vPRI reset to recover.
> > > + */
> > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > + return -EINVAL;
> >
> > I am using the SMMUv3 stall feature, and need to forward this to hardware,
> > And now I am hacking to comment this check.
> > Any suggestions?
>
> Are you using PCI SRIOV and stall together?
Only use smmuv3 stall feature.
>
> > > + mutex_lock(&idev->iopf_lock);
> > > + /* Device iopf has already been on. */
> > > + if (++idev->iopf_enabled > 1) {
> > > + mutex_unlock(&idev->iopf_lock);
> > > + return 0;
> > > + }
> > > +
> > > + ret = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_IOPF);
> > > + if (ret)
> > > + --idev->iopf_enabled;
> > > + mutex_unlock(&idev->iopf_lock);
> >
> > Also iommu_dev_enable_feature(idev->dev, IOMMU_DEV_FEAT_SVA); is required
> > In thinking how to add it properly.
>
> FEAT_SVA needs to be deleted, not added too.
>
> smmu-v3 needs some more fixing to move that
> arm_smmu_master_enable_sva() logic into domain attachment.
Will think about this, Thanks Jason
>
> Jason
On Wed, Oct 16, 2024 at 09:58:36AM +0800, Zhangfei Gao wrote:
> On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > > > +{
> > > > + struct device *dev = idev->dev;
> > > > + int ret;
> > > > +
> > > > + /*
> > > > + * Once we turn on PCI/PRI support for VF, the response failure code
> > > > + * should not be forwarded to the hardware due to PRI being a shared
> > > > + * resource between PF and VFs. There is no coordination for this
> > > > + * shared capability. This waits for a vPRI reset to recover.
> > > > + */
> > > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > > + return -EINVAL;
> > >
> > > I am using the SMMUv3 stall feature, and need to forward this to hardware,
> > > And now I am hacking to comment this check.
> > > Any suggestions?
> >
> > Are you using PCI SRIOV and stall together?
>
> Only use smmuv3 stall feature.
Then isn't to_pci_dev(dev)->is_virtfn == false?
That should only be true with SRIOV
> > FEAT_SVA needs to be deleted, not added too.
> >
> > smmu-v3 needs some more fixing to move that
> > arm_smmu_master_enable_sva() logic into domain attachment.
>
> Will think about this, Thanks Jason
Can you test it if a patch is made?
Thanks,
Jason
On Wed, Oct 16, 2024 at 12:25:03PM -0300, Jason Gunthorpe wrote: > > > smmu-v3 needs some more fixing to move that > > > arm_smmu_master_enable_sva() logic into domain attachment. > > > > Will think about this, Thanks Jason > > Can you test it if a patch is made? Here it is: https://github.com/jgunthorpe/linux/commits/smmuv3_nesting/ fa1528253d2210 iommu: Remove IOMMU_DEV_FEAT_SVA 5675560a272cf5 iommu/vt-d: Check if SVA is supported when attaching the SVA domain 94bc2b9525b508 iommu/arm-smmu-v3: Put iopf enablement in the domain attach path Let me know.. Jason
On Fri, 18 Oct 2024 at 21:53, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, Oct 16, 2024 at 12:25:03PM -0300, Jason Gunthorpe wrote:
> > > > smmu-v3 needs some more fixing to move that
> > > > arm_smmu_master_enable_sva() logic into domain attachment.
> > >
> > > Will think about this, Thanks Jason
> >
> > Can you test it if a patch is made?
>
> Here it is:
>
> https://github.com/jgunthorpe/linux/commits/smmuv3_nesting/
>
> fa1528253d2210 iommu: Remove IOMMU_DEV_FEAT_SVA
> 5675560a272cf5 iommu/vt-d: Check if SVA is supported when attaching the SVA domain
> 94bc2b9525b508 iommu/arm-smmu-v3: Put iopf enablement in the domain attach path
>
> Let me know..
With these patches, do we still need ops->user_pasid_table?
if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) {
attach_handle = iommu_attach_handle_get(dev->iommu_group,
fault->prm.pasid, 0);
// is attach_handle expected effect value here?
if (IS_ERR(attach_handle)) {
const struct iommu_ops *ops = dev_iommu_ops(dev);
if (!ops->user_pasid_table)
return NULL;
/*
* The iommu driver for this device supports user-
* managed PASID table. Therefore page faults for
* any PASID should go through the NESTING domain
* attached to the device RID.
*/
attach_handle = iommu_attach_handle_get(
dev->iommu_group, IOMMU_NO_PASID,
IOMMU_DOMAIN_NESTED);
Now I still need set ops->user_pasid_table, since attach_handle can not
return from the first iommu_attach_handle_get with fault->prm.pasid = 1,
but the second iommu_attach_handle_get with IOMMU_NO_PASID,
suppose it is not expected?
Thanks
On Tue, Oct 22, 2024 at 10:30:10PM +0800, Zhangfei Gao wrote:
> On Fri, 18 Oct 2024 at 21:53, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Wed, Oct 16, 2024 at 12:25:03PM -0300, Jason Gunthorpe wrote:
> > > > > smmu-v3 needs some more fixing to move that
> > > > > arm_smmu_master_enable_sva() logic into domain attachment.
> > > >
> > > > Will think about this, Thanks Jason
> > >
> > > Can you test it if a patch is made?
> >
> > Here it is:
> >
> > https://github.com/jgunthorpe/linux/commits/smmuv3_nesting/
> >
> > fa1528253d2210 iommu: Remove IOMMU_DEV_FEAT_SVA
> > 5675560a272cf5 iommu/vt-d: Check if SVA is supported when attaching the SVA domain
> > 94bc2b9525b508 iommu/arm-smmu-v3: Put iopf enablement in the domain attach path
> >
> > Let me know..
>
> With these patches, do we still need ops->user_pasid_table?
It makes no change - you need user_pasid_table to make
IOMMU_DOMAIN_NESTED work.
If you aren't using IOMMU_DOMAIN_NESTED you shouldn't need it.
> if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) {
> attach_handle = iommu_attach_handle_get(dev->iommu_group,
> fault->prm.pasid, 0);
>
> // is attach_handle expected effect value here?
> if (IS_ERR(attach_handle)) {
> const struct iommu_ops *ops = dev_iommu_ops(dev);
>
> if (!ops->user_pasid_table)
> return NULL;
> /*
> * The iommu driver for this device supports user-
> * managed PASID table. Therefore page faults for
> * any PASID should go through the NESTING domain
> * attached to the device RID.
> */
> attach_handle = iommu_attach_handle_get(
> dev->iommu_group, IOMMU_NO_PASID,
> IOMMU_DOMAIN_NESTED);
>
> Now I still need set ops->user_pasid_table, since attach_handle can not
> return from the first iommu_attach_handle_get with fault->prm.pasid = 1,
> but the second iommu_attach_handle_get with IOMMU_NO_PASID,
> suppose it is not expected?
The second handle_get will always fail unless you are using
IOMMU_DOMAIN_NESTED in userspace with iommufd.
What testing are you doing exactly?
Jason
On Tue, 22 Oct 2024 at 22:53, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Oct 22, 2024 at 10:30:10PM +0800, Zhangfei Gao wrote:
> > On Fri, 18 Oct 2024 at 21:53, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Wed, Oct 16, 2024 at 12:25:03PM -0300, Jason Gunthorpe wrote:
> > > > > > smmu-v3 needs some more fixing to move that
> > > > > > arm_smmu_master_enable_sva() logic into domain attachment.
> > > > >
> > > > > Will think about this, Thanks Jason
> > > >
> > > > Can you test it if a patch is made?
> > >
> > > Here it is:
> > >
> > > https://github.com/jgunthorpe/linux/commits/smmuv3_nesting/
> > >
> > > fa1528253d2210 iommu: Remove IOMMU_DEV_FEAT_SVA
> > > 5675560a272cf5 iommu/vt-d: Check if SVA is supported when attaching the SVA domain
> > > 94bc2b9525b508 iommu/arm-smmu-v3: Put iopf enablement in the domain attach path
> > >
> > > Let me know..
> >
> > With these patches, do we still need ops->user_pasid_table?
>
> It makes no change - you need user_pasid_table to make
> IOMMU_DOMAIN_NESTED work.
>
> If you aren't using IOMMU_DOMAIN_NESTED you shouldn't need it.
OK, I misunderstood.
Then with user_pasid_table=1
both with these patches and without patches, user page fault is OK.
>
> > if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) {
> > attach_handle = iommu_attach_handle_get(dev->iommu_group,
> > fault->prm.pasid, 0);
> >
> > // is attach_handle expected effect value here?
> > if (IS_ERR(attach_handle)) {
> > const struct iommu_ops *ops = dev_iommu_ops(dev);
> >
> > if (!ops->user_pasid_table)
> > return NULL;
> > /*
> > * The iommu driver for this device supports user-
> > * managed PASID table. Therefore page faults for
> > * any PASID should go through the NESTING domain
> > * attached to the device RID.
> > */
> > attach_handle = iommu_attach_handle_get(
> > dev->iommu_group, IOMMU_NO_PASID,
> > IOMMU_DOMAIN_NESTED);
> >
> > Now I still need set ops->user_pasid_table, since attach_handle can not
> > return from the first iommu_attach_handle_get with fault->prm.pasid = 1,
> > but the second iommu_attach_handle_get with IOMMU_NO_PASID,
> > suppose it is not expected?
>
> The second handle_get will always fail unless you are using
> IOMMU_DOMAIN_NESTED in userspace with iommufd.
>
> What testing are you doing exactly?
I am testing vsva based on Nico's iommufd_viommu_p2-v3 branch,
which requires IOMMU_DOMAIN_NESTED in user space with iommufd.
About the three patches
1. Tested host sva, OK
2. Simply tested vsva on guests, OK, will do more tests.
Thanks
On Wed, 16 Oct 2024 at 23:25, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Oct 16, 2024 at 09:58:36AM +0800, Zhangfei Gao wrote:
> > On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > > > > +{
> > > > > + struct device *dev = idev->dev;
> > > > > + int ret;
> > > > > +
> > > > > + /*
> > > > > + * Once we turn on PCI/PRI support for VF, the response failure code
> > > > > + * should not be forwarded to the hardware due to PRI being a shared
> > > > > + * resource between PF and VFs. There is no coordination for this
> > > > > + * shared capability. This waits for a vPRI reset to recover.
> > > > > + */
> > > > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > > > + return -EINVAL;
> > > >
> > > > I am using the SMMUv3 stall feature, and need to forward this to hardware,
> > > > And now I am hacking to comment this check.
> > > > Any suggestions?
> > >
> > > Are you using PCI SRIOV and stall together?
> >
> > Only use smmuv3 stall feature.
>
> Then isn't to_pci_dev(dev)->is_virtfn == false?
>
> That should only be true with SRIOV
Do you mean
if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn == false)
return -EINVAL;
This is fine
>
> > > FEAT_SVA needs to be deleted, not added too.
> > >
> > > smmu-v3 needs some more fixing to move that
> > > arm_smmu_master_enable_sva() logic into domain attachment.
> >
> > Will think about this, Thanks Jason
>
> Can you test it if a patch is made?
Yes, sure.
Thanks
On Thu, Oct 17, 2024 at 09:44:18AM +0800, Zhangfei Gao wrote:
> On Wed, 16 Oct 2024 at 23:25, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Wed, Oct 16, 2024 at 09:58:36AM +0800, Zhangfei Gao wrote:
> > > On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > > > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > > > > > +{
> > > > > > + struct device *dev = idev->dev;
> > > > > > + int ret;
> > > > > > +
> > > > > > + /*
> > > > > > + * Once we turn on PCI/PRI support for VF, the response failure code
> > > > > > + * should not be forwarded to the hardware due to PRI being a shared
> > > > > > + * resource between PF and VFs. There is no coordination for this
> > > > > > + * shared capability. This waits for a vPRI reset to recover.
> > > > > > + */
> > > > > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > > > > + return -EINVAL;
> > > > >
> > > > > I am using the SMMUv3 stall feature, and need to forward this to hardware,
> > > > > And now I am hacking to comment this check.
> > > > > Any suggestions?
> > > >
> > > > Are you using PCI SRIOV and stall together?
> > >
> > > Only use smmuv3 stall feature.
> >
> > Then isn't to_pci_dev(dev)->is_virtfn == false?
> >
> > That should only be true with SRIOV
>
> Do you mean
> if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn == false)
> return -EINVAL;
>
> This is fine
No, I mean on your test system you are not using SRIOV so all your PCI
devices will have is_virtfn == false and the above if shouldn't be a
problem. is_virtfn indicates the PCI device is a SRIOV VF.
Your explanation for your problem doesn't really make sense, or there
is something wrong someplace else to get a bogus is_virtfn..
If you are doing SRIOV with stall, then that is understandable.
Jason
On Thu, 17 Oct 2024 at 20:05, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Oct 17, 2024 at 09:44:18AM +0800, Zhangfei Gao wrote:
> > On Wed, 16 Oct 2024 at 23:25, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Wed, Oct 16, 2024 at 09:58:36AM +0800, Zhangfei Gao wrote:
> > > > On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > >
> > > > > On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > > > > > +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> > > > > > > +{
> > > > > > > + struct device *dev = idev->dev;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Once we turn on PCI/PRI support for VF, the response failure code
> > > > > > > + * should not be forwarded to the hardware due to PRI being a shared
> > > > > > > + * resource between PF and VFs. There is no coordination for this
> > > > > > > + * shared capability. This waits for a vPRI reset to recover.
> > > > > > > + */
> > > > > > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > > > > > + return -EINVAL;
> > > > > >
> > > > > > I am using the SMMUv3 stall feature, and need to forward this to hardware,
> > > > > > And now I am hacking to comment this check.
> > > > > > Any suggestions?
> > > > >
> > > > > Are you using PCI SRIOV and stall together?
> > > >
> > > > Only use smmuv3 stall feature.
Sorry, this is not correct
> > >
> > > Then isn't to_pci_dev(dev)->is_virtfn == false?
> > >
> > > That should only be true with SRIOV
> >
> > Do you mean
> > if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn == false)
> > return -EINVAL;
> >
> > This is fine
>
> No, I mean on your test system you are not using SRIOV so all your PCI
> devices will have is_virtfn == false and the above if shouldn't be a
> problem. is_virtfn indicates the PCI device is a SRIOV VF.
>
> Your explanation for your problem doesn't really make sense, or there
> is something wrong someplace else to get a bogus is_virtfn..
>
> If you are doing SRIOV with stall, then that is understandable.
Yes, you are right
I am using SRIOV vf and stall feature, so is_virtfn == true
Our ACC devices are fake pci endpoint devices which supports stall,
And they also supports sriov
So I have to ignore the limitation.
Thanks
On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote: > Yes, you are right > I am using SRIOV vf and stall feature, so is_virtfn == true > > Our ACC devices are fake pci endpoint devices which supports stall, > And they also supports sriov > > So I have to ignore the limitation. I see, so that is more complicated. Lu, what do you think about also checking if the PCI function has PRI ? If not PRI assume the fault is special and doesn't follow PRI rules? Or maybe we can have the iommu driver tag the event as a PRI/not-PRI fault? Jason
On 2024/10/17 21:08, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote:
>
>> Yes, you are right
>> I am using SRIOV vf and stall feature, so is_virtfn == true
>>
>> Our ACC devices are fake pci endpoint devices which supports stall,
>> And they also supports sriov
>>
>> So I have to ignore the limitation.
> I see, so that is more complicated.
>
> Lu, what do you think about also checking if the PCI function has PRI
> ? If not PRI assume the fault is special and doesn't follow PRI rules?
>
> Or maybe we can have the iommu driver tag the event as a PRI/not-PRI
> fault?
This limitation applies to PRI on PCI/SRIOV VFs because the PRI might be
a shared resource and current iommu subsystem is not ready to support
enabling/disabling PRI on a VF without any impact on others.
In my understanding, it's fine to remove this limitation from the use
case of non-PRI on SRIOV VFs. Perhaps something like below?
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
if (pdev->is_virtfn && pci_pri_supported(pdev))
return -EINVAL;
}
Thanks,
baolu
On Fri, Oct 18, 2024 at 09:58:37AM +0800, Baolu Lu wrote:
> On 2024/10/17 21:08, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote:
> >
> > > Yes, you are right
> > > I am using SRIOV vf and stall feature, so is_virtfn == true
> > >
> > > Our ACC devices are fake pci endpoint devices which supports stall,
> > > And they also supports sriov
> > >
> > > So I have to ignore the limitation.
> > I see, so that is more complicated.
> >
> > Lu, what do you think about also checking if the PCI function has PRI
> > ? If not PRI assume the fault is special and doesn't follow PRI rules?
> >
> > Or maybe we can have the iommu driver tag the event as a PRI/not-PRI
> > fault?
>
> This limitation applies to PRI on PCI/SRIOV VFs because the PRI might be
> a shared resource and current iommu subsystem is not ready to support
> enabling/disabling PRI on a VF without any impact on others.
>
> In my understanding, it's fine to remove this limitation from the use
> case of non-PRI on SRIOV VFs. Perhaps something like below?
>
> if (dev_is_pci(dev)) {
> struct pci_dev *pdev = to_pci_dev(dev);
> if (pdev->is_virtfn && pci_pri_supported(pdev))
> return -EINVAL;
> }
Makes sense to me
Thanks,
Jason
Hi, Baolu
On Fri, 18 Oct 2024 at 09:58, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2024/10/17 21:08, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote:
> >
> >> Yes, you are right
> >> I am using SRIOV vf and stall feature, so is_virtfn == true
> >>
> >> Our ACC devices are fake pci endpoint devices which supports stall,
> >> And they also supports sriov
> >>
> >> So I have to ignore the limitation.
> > I see, so that is more complicated.
> >
> > Lu, what do you think about also checking if the PCI function has PRI
> > ? If not PRI assume the fault is special and doesn't follow PRI rules?
> >
> > Or maybe we can have the iommu driver tag the event as a PRI/not-PRI
> > fault?
>
> This limitation applies to PRI on PCI/SRIOV VFs because the PRI might be
> a shared resource and current iommu subsystem is not ready to support
> enabling/disabling PRI on a VF without any impact on others.
>
> In my understanding, it's fine to remove this limitation from the use
> case of non-PRI on SRIOV VFs. Perhaps something like below?
>
#include <linux/pci-ats.h>
> if (dev_is_pci(dev)) {
> struct pci_dev *pdev = to_pci_dev(dev);
> if (pdev->is_virtfn && pci_pri_supported(pdev))
> return -EINVAL;
> }
Yes, this works on our platform.
Thanks Baolu.
Hi, Baolu
On Fri, 18 Oct 2024 at 10:45, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
> Hi, Baolu
>
> On Fri, 18 Oct 2024 at 09:58, Baolu Lu <baolu.lu@linux.intel.com> wrote:
> >
> > On 2024/10/17 21:08, Jason Gunthorpe wrote:
> > > On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote:
> > >
> > >> Yes, you are right
> > >> I am using SRIOV vf and stall feature, so is_virtfn == true
> > >>
> > >> Our ACC devices are fake pci endpoint devices which supports stall,
> > >> And they also supports sriov
> > >>
> > >> So I have to ignore the limitation.
> > > I see, so that is more complicated.
> > >
> > > Lu, what do you think about also checking if the PCI function has PRI
> > > ? If not PRI assume the fault is special and doesn't follow PRI rules?
> > >
> > > Or maybe we can have the iommu driver tag the event as a PRI/not-PRI
> > > fault?
> >
> > This limitation applies to PRI on PCI/SRIOV VFs because the PRI might be
> > a shared resource and current iommu subsystem is not ready to support
> > enabling/disabling PRI on a VF without any impact on others.
> >
> > In my understanding, it's fine to remove this limitation from the use
> > case of non-PRI on SRIOV VFs. Perhaps something like below?
> >
> #include <linux/pci-ats.h>
> > if (dev_is_pci(dev)) {
> > struct pci_dev *pdev = to_pci_dev(dev);
> > if (pdev->is_virtfn && pci_pri_supported(pdev))
> > return -EINVAL;
> > }
>
> Yes, this works on our platform.
Will you send this patch?
Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Thanks
On 2024/10/27 22:12, Zhangfei Gao wrote:
> On Fri, 18 Oct 2024 at 10:45, Zhangfei Gao<zhangfei.gao@linaro.org> wrote:
>> Hi, Baolu
>>
>> On Fri, 18 Oct 2024 at 09:58, Baolu Lu<baolu.lu@linux.intel.com> wrote:
>>> On 2024/10/17 21:08, Jason Gunthorpe wrote:
>>>> On Thu, Oct 17, 2024 at 08:35:24PM +0800, Zhangfei Gao wrote:
>>>>
>>>>> Yes, you are right
>>>>> I am using SRIOV vf and stall feature, so is_virtfn == true
>>>>>
>>>>> Our ACC devices are fake pci endpoint devices which supports stall,
>>>>> And they also supports sriov
>>>>>
>>>>> So I have to ignore the limitation.
>>>> I see, so that is more complicated.
>>>>
>>>> Lu, what do you think about also checking if the PCI function has PRI
>>>> ? If not PRI assume the fault is special and doesn't follow PRI rules?
>>>>
>>>> Or maybe we can have the iommu driver tag the event as a PRI/not-PRI
>>>> fault?
>>> This limitation applies to PRI on PCI/SRIOV VFs because the PRI might be
>>> a shared resource and current iommu subsystem is not ready to support
>>> enabling/disabling PRI on a VF without any impact on others.
>>>
>>> In my understanding, it's fine to remove this limitation from the use
>>> case of non-PRI on SRIOV VFs. Perhaps something like below?
>>>
>> #include <linux/pci-ats.h>
>>> if (dev_is_pci(dev)) {
>>> struct pci_dev *pdev = to_pci_dev(dev);
>>> if (pdev->is_virtfn && pci_pri_supported(pdev))
>>> return -EINVAL;
>>> }
>> Yes, this works on our platform.
> Will you send this patch?
>
> Tested-by: Zhangfei Gao<zhangfei.gao@linaro.org>
Can you please make this change a formal patch by yourself? As I don't
have hardware in hand, I'm not confident to accurately describe the
requirement or verify the new version during the upstream process.
Thanks,
baolu
On Sun, 27 Oct 2024 at 22:26, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> Can you please make this change a formal patch by yourself? As I don't
> have hardware in hand, I'm not confident to accurately describe the
> requirement or verify the new version during the upstream process.
>
OK, how about this one
Subject: [PATCH] iommufd: modify iommufd_fault_iopf_enable limitation
iommufd_fault_iopf_enable has limitation to PRI on PCI/SRIOV VFs
because the PRI might be a shared resource and current iommu
subsystem is not ready to support enabling/disabling PRI on a VF
without any impact on others.
However, we have devices that appear as PCI but are actually on the
AMBA bus. These fake PCI devices have PASID capability, support
stall as well as SRIOV, so remove the limitation for these devices.
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommufd/fault.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index bca956d496bd..8b3e34250dae 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/pci.h>
+#include <linux/pci-ats.h>
#include <linux/poll.h>
#include <uapi/linux/iommufd.h>
@@ -27,8 +28,12 @@ static int iommufd_fault_iopf_enable(struct
iommufd_device *idev)
* resource between PF and VFs. There is no coordination for this
* shared capability. This waits for a vPRI reset to recover.
*/
- if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
- return -EINVAL;
+ if (dev_is_pci(dev)) {
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (pdev->is_virtfn && pci_pri_supported(pdev))
+ return -EINVAL;
+ }
mutex_lock(&idev->iopf_lock);
/* Device iopf has already been on. */
--
2.25.1
On 2024/10/28 17:56, Zhangfei Gao wrote:
> On Sun, 27 Oct 2024 at 22:26, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
>>
>> Can you please make this change a formal patch by yourself? As I don't
>> have hardware in hand, I'm not confident to accurately describe the
>> requirement or verify the new version during the upstream process.
>>
>
> OK, how about this one
>
> Subject: [PATCH] iommufd: modify iommufd_fault_iopf_enable limitation
>
> iommufd_fault_iopf_enable has limitation to PRI on PCI/SRIOV VFs
> because the PRI might be a shared resource and current iommu
> subsystem is not ready to support enabling/disabling PRI on a VF
> without any impact on others.
>
> However, we have devices that appear as PCI but are actually on the
> AMBA bus. These fake PCI devices have PASID capability, support
> stall as well as SRIOV, so remove the limitation for these devices.
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/iommufd/fault.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index bca956d496bd..8b3e34250dae 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/pci.h>
> +#include <linux/pci-ats.h>
> #include <linux/poll.h>
> #include <uapi/linux/iommufd.h>
>
> @@ -27,8 +28,12 @@ static int iommufd_fault_iopf_enable(struct
> iommufd_device *idev)
> * resource between PF and VFs. There is no coordination for this
> * shared capability. This waits for a vPRI reset to recover.
> */
> - if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> - return -EINVAL;
> + if (dev_is_pci(dev)) {
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + if (pdev->is_virtfn && pci_pri_supported(pdev))
> + return -EINVAL;
> + }
>
> mutex_lock(&idev->iopf_lock);
> /* Device iopf has already been on. */
Looks fine to me, but you need to use the "git sendemail" command to
post the patch. Otherwise, the maintainer has no means to pick your
patch with tools like b4.
> -----Original Message-----
> From: Zhangfei Gao <zhangfei.gao@linaro.org>
> Sent: Thursday, October 17, 2024 1:35 PM
> To: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>; Kevin Tian
> <kevin.tian@intel.com>; Joerg Roedel <joro@8bytes.org>; Will Deacon
> <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; Jean-
> Philippe Brucker <jean-philippe@linaro.org>; Nicolin Chen
> <nicolinc@nvidia.com>; Yi Liu <yi.l.liu@intel.com>; Jacob Pan
> <jacob.jun.pan@linux.intel.com>; Joel Granados
> <j.granados@samsung.com>; iommu@lists.linux.dev;
> virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org;
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Subject: Re: [PATCH v8 07/10] iommufd: Fault-capable hwpt
> attach/detach/replace
>
> On Thu, 17 Oct 2024 at 20:05, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Thu, Oct 17, 2024 at 09:44:18AM +0800, Zhangfei Gao wrote:
> > > On Wed, 16 Oct 2024 at 23:25, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Wed, Oct 16, 2024 at 09:58:36AM +0800, Zhangfei Gao wrote:
> > > > > On Tue, 15 Oct 2024 at 20:54, Jason Gunthorpe <jgg@ziepe.ca>
> wrote:
> > > > > >
> > > > > > On Tue, Oct 15, 2024 at 11:19:33AM +0800, Zhangfei Gao wrote:
> > > > > > > > +static int iommufd_fault_iopf_enable(struct
> > > > > > > > +iommufd_device *idev) {
> > > > > > > > + struct device *dev = idev->dev;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * Once we turn on PCI/PRI support for VF, the response
> failure code
> > > > > > > > + * should not be forwarded to the hardware due to PRI
> being a shared
> > > > > > > > + * resource between PF and VFs. There is no coordination
> for this
> > > > > > > > + * shared capability. This waits for a vPRI reset to recover.
> > > > > > > > + */
> > > > > > > > + if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn)
> > > > > > > > + return -EINVAL;
> > > > > > >
> > > > > > > I am using the SMMUv3 stall feature, and need to forward
> > > > > > > this to hardware, And now I am hacking to comment this check.
> > > > > > > Any suggestions?
> > > > > >
> > > > > > Are you using PCI SRIOV and stall together?
> > > > >
> > > > > Only use smmuv3 stall feature.
> Sorry, this is not correct
>
> > > >
> > > > Then isn't to_pci_dev(dev)->is_virtfn == false?
> > > >
> > > > That should only be true with SRIOV
> > >
> > > Do you mean
> > > if (dev_is_pci(dev) && to_pci_dev(dev)->is_virtfn == false)
> > > return -EINVAL;
> > >
> > > This is fine
> >
> > No, I mean on your test system you are not using SRIOV so all your PCI
> > devices will have is_virtfn == false and the above if shouldn't be a
> > problem. is_virtfn indicates the PCI device is a SRIOV VF.
> >
> > Your explanation for your problem doesn't really make sense, or there
> > is something wrong someplace else to get a bogus is_virtfn..
> >
> > If you are doing SRIOV with stall, then that is understandable.
>
> Yes, you are right
> I am using SRIOV vf and stall feature, so is_virtfn == true
>
> Our ACC devices are fake pci endpoint devices which supports stall, And
> they also supports sriov
May be this will help to get the background:
https://lore.kernel.org/all/1626144876-11352-1-git-send-email-zhangfei.gao@linaro.org/
Shameer
© 2016 - 2025 Red Hat, Inc.