From: Joel Granados <j.granados@samsung.com>
iommu_report_device_fault expects a pasid array to have an
iommu_attach_handle when a fault is detected. Add this handle when the
replacing hwpt has a valid iommufd fault object. Remove it when we
release ownership of the group.
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
drivers/iommu/iommu-priv.h | 3 +++
drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++++
drivers/iommu/iommufd/fault.c | 22 ++++++++++++++++++++++
3 files changed, 55 insertions(+)
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index de5b54eaa8bf..493e501badc7 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -38,6 +38,9 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
ioasid_t pasid,
unsigned int type);
+int iommu_init_pasid_array(struct iommu_domain *domain,
+ struct iommu_group *group,
+ struct iommu_attach_handle *handle);
int iommu_attach_group_handle(struct iommu_domain *domain,
struct iommu_group *group,
struct iommu_attach_handle *handle);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ed6c5cb60c5a..64ae1cf571aa 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3262,6 +3262,9 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
static void __iommu_release_dma_ownership(struct iommu_group *group)
{
+ if (!xa_empty(&group->pasid_array))
+ xa_erase(&group->pasid_array, IOMMU_NO_PASID);
+
if (WARN_ON(!group->owner_cnt || !group->owner ||
!xa_empty(&group->pasid_array)))
return;
@@ -3495,6 +3498,33 @@ iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int
}
EXPORT_SYMBOL_NS_GPL(iommu_attach_handle_get, IOMMUFD_INTERNAL);
+/**
+ * iommu_init_pasid_array - Initialize pasid array in the domain group
+ *
+ * Returns 0 on success. Error code on failure
+ *
+ * An IOMMU_NO_PASID element is *NOT* replaced if there is one already there.
+ */
+int iommu_init_pasid_array(struct iommu_domain *domain,
+ struct iommu_group *group,
+ struct iommu_attach_handle *handle)
+{
+ int ret;
+
+ if (handle)
+ handle->domain = domain;
+
+ mutex_lock(&group->mutex);
+ ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+ mutex_unlock(&group->mutex);
+
+ if (ret == -EBUSY)
+ ret = 0;
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_init_pasid_array, IOMMUFD_INTERNAL);
+
/**
* iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group
* @domain: IOMMU domain to attach
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index a643d5c7c535..ea7f1bf64892 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -178,6 +178,25 @@ static int __fault_domain_replace_dev(struct iommufd_device *idev,
return ret;
}
+static int iommufd_init_pasid_array(struct iommu_domain *domain,
+ struct iommufd_device *idev)
+{
+ int ret;
+ struct iommufd_attach_handle *handle;
+ struct iommu_group *group = idev->igroup->group;
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return -ENOMEM;
+ handle->idev = idev;
+
+ ret = iommu_init_pasid_array(domain, group, &handle->handle);
+ if (ret)
+ kfree(handle);
+
+ return ret;
+}
+
int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt,
struct iommufd_hw_pagetable *old)
@@ -190,6 +209,9 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
ret = iommufd_fault_iopf_enable(idev);
if (ret)
return ret;
+
+ if (iommufd_init_pasid_array(hwpt->domain, idev))
+ return -EINVAL;
}
ret = __fault_domain_replace_dev(idev, hwpt, old);
--
2.43.0On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: > From: Joel Granados<j.granados@samsung.com> > > iommu_report_device_fault expects a pasid array to have an > iommu_attach_handle when a fault is detected. The iommu_attach_handle is expected only when an iopf-capable domain is attached to the device or PASID. The iommu_report_device_fault() treats it as a fault when a fault occurs, but no iopf-capable domain is attached. > Add this handle when the > replacing hwpt has a valid iommufd fault object. Remove it when we > release ownership of the group. The iommu_attach_handle is managed by the caller (iommufd here for example). Therefore, before iommu_attach_handle tries to attach a domain to an iopf-capable device or pasid, it should allocate the handle and pass it to the domain attachment interfaces. Conversely, the handle can only be freed after the domain is detached. Thanks, baolu
On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote: > On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: > > From: Joel Granados<j.granados@samsung.com> > > > > iommu_report_device_fault expects a pasid array to have an > > iommu_attach_handle when a fault is detected. > > The iommu_attach_handle is expected only when an iopf-capable domain is > attached to the device or PASID. The iommu_report_device_fault() treats > it as a fault when a fault occurs, but no iopf-capable domain is > attached. > > > Add this handle when the > > replacing hwpt has a valid iommufd fault object. Remove it when we > > release ownership of the group. > > The iommu_attach_handle is managed by the caller (iommufd here for > example). Therefore, before iommu_attach_handle tries to attach a domain > to an iopf-capable device or pasid, it should allocate the handle and > pass it to the domain attachment interfaces. What do you mean here? 1. Do you want to move the iommufd_init_pasid_array call up to iommufd_hwpt_replace_device? 2. Do you want to move it further up to iommufd_device_do_replace? Note that all this implemented on a call to replace HWPT. So a non-iopf-capable VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl has already been completed before the one that calls iommufd_device_do_replace. > Conversely, the handle can > only be freed after the domain is detached. Do you have a function in specific where you would put the free handle logic? Best -- Joel Granados
On 9/11/24 6:56 PM, Joel Granados wrote:
> On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote:
>> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote:
>>> From: Joel Granados<j.granados@samsung.com>
>>>
>>> iommu_report_device_fault expects a pasid array to have an
>>> iommu_attach_handle when a fault is detected.
>> The iommu_attach_handle is expected only when an iopf-capable domain is
>> attached to the device or PASID. The iommu_report_device_fault() treats
>> it as a fault when a fault occurs, but no iopf-capable domain is
>> attached.
>>
>>> Add this handle when the
>>> replacing hwpt has a valid iommufd fault object. Remove it when we
>>> release ownership of the group.
>> The iommu_attach_handle is managed by the caller (iommufd here for
>> example). Therefore, before iommu_attach_handle tries to attach a domain
>> to an iopf-capable device or pasid, it should allocate the handle and
>> pass it to the domain attachment interfaces.
> What do you mean here?
> 1. Do you want to move the iommufd_init_pasid_array call up to
> iommufd_hwpt_replace_device?
> 2. Do you want to move it further up to iommufd_device_do_replace?
I'm having trouble understanding the need for iommu_init_pasid_array()
in the core.
>
> Note that all this implemented on a call to replace HWPT. So a
> non-iopf-capable VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl has already been
> completed before the one that calls iommufd_device_do_replace.
>
>> Conversely, the handle can
>> only be freed after the domain is detached.
> Do you have a function in specific where you would put the free handle
> logic?
drivers/iommu/iommufd/fault.c
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);
}
Thanks,
baolu
On Thu, Sep 12, 2024 at 12:21:53PM +0800, Baolu Lu wrote:
> On 9/11/24 6:56 PM, Joel Granados wrote:
> > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote:
> >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote:
> >>> From: Joel Granados<j.granados@samsung.com>
> >>>
> >>> iommu_report_device_fault expects a pasid array to have an
> >>> iommu_attach_handle when a fault is detected.
> >> The iommu_attach_handle is expected only when an iopf-capable domain is
> >> attached to the device or PASID. The iommu_report_device_fault() treats
> >> it as a fault when a fault occurs, but no iopf-capable domain is
> >> attached.
> >>
> >>> Add this handle when the
> >>> replacing hwpt has a valid iommufd fault object. Remove it when we
> >>> release ownership of the group.
> >> The iommu_attach_handle is managed by the caller (iommufd here for
> >> example). Therefore, before iommu_attach_handle tries to attach a domain
> >> to an iopf-capable device or pasid, it should allocate the handle and
> >> pass it to the domain attachment interfaces.
> > What do you mean here?
> > 1. Do you want to move the iommufd_init_pasid_array call up to
> > iommufd_hwpt_replace_device?
> > 2. Do you want to move it further up to iommufd_device_do_replace?
>
> I'm having trouble understanding the need for iommu_init_pasid_array()
> in the core.
>
> >
> > Note that all this implemented on a call to replace HWPT. So a
> > non-iopf-capable VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl has already been
> > completed before the one that calls iommufd_device_do_replace.
> >
> >> Conversely, the handle can
> >> only be freed after the domain is detached.
> > Do you have a function in specific where you would put the free handle
> > logic?
>
> drivers/iommu/iommufd/fault.c
>
> 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);
> }
Actually there is no need to add the xa_erase call in
__iommu_release_dma_ownership because it is *already* handled here.
Sorry about this, I think this was left from a previous version of the
patch. I'll remove the superfluous xa_erase call.
Thx for the review
--
Joel Granados
On Thu, Sep 12, 2024 at 12:21:53PM +0800, Baolu Lu wrote:
> On 9/11/24 6:56 PM, Joel Granados wrote:
> > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote:
> >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote:
> >>> From: Joel Granados<j.granados@samsung.com>
> >>>
> >>> iommu_report_device_fault expects a pasid array to have an
> >>> iommu_attach_handle when a fault is detected.
> >> The iommu_attach_handle is expected only when an iopf-capable domain is
> >> attached to the device or PASID. The iommu_report_device_fault() treats
> >> it as a fault when a fault occurs, but no iopf-capable domain is
> >> attached.
> >>
> >>> Add this handle when the
> >>> replacing hwpt has a valid iommufd fault object. Remove it when we
> >>> release ownership of the group.
> >> The iommu_attach_handle is managed by the caller (iommufd here for
> >> example). Therefore, before iommu_attach_handle tries to attach a domain
> >> to an iopf-capable device or pasid, it should allocate the handle and
> >> pass it to the domain attachment interfaces.
> > What do you mean here?
> > 1. Do you want to move the iommufd_init_pasid_array call up to
> > iommufd_hwpt_replace_device?
> > 2. Do you want to move it further up to iommufd_device_do_replace?
>
> I'm having trouble understanding the need for iommu_init_pasid_array()
> in the core.
The iommu_init_pasid_array function is there because I needed to access
struct iommu_group->pasid_array. The struct declaration for iommu_group
is in the core (drivers/iommu/iommu.c), so I thought that the function
to modify a member of that struct would go in core also.
To move all the logic into iommufd_init_pasid_array in iommufd, we would
need to move the iommu_group struct (include/linux/iommu.h?). Here is a
proposed patch that would make that happen (and removes
iommu_init_pasid_array). What do you think?
diff --git i/drivers/iommu/iommu-priv.h w/drivers/iommu/iommu-priv.h
index 493e501badc7..de5b54eaa8bf 100644
--- i/drivers/iommu/iommu-priv.h
+++ w/drivers/iommu/iommu-priv.h
@@ -38,9 +38,6 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
ioasid_t pasid,
unsigned int type);
-int iommu_init_pasid_array(struct iommu_domain *domain,
- struct iommu_group *group,
- struct iommu_attach_handle *handle);
int iommu_attach_group_handle(struct iommu_domain *domain,
struct iommu_group *group,
struct iommu_attach_handle *handle);
diff --git i/drivers/iommu/iommu.c w/drivers/iommu/iommu.c
index 64ae1cf571aa..83bc0bfd7bb0 100644
--- i/drivers/iommu/iommu.c
+++ w/drivers/iommu/iommu.c
@@ -44,24 +44,6 @@ static unsigned int iommu_def_domain_type __read_mostly;
static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
static u32 iommu_cmd_line __read_mostly;
-struct iommu_group {
- struct kobject kobj;
- struct kobject *devices_kobj;
- struct list_head devices;
- struct xarray pasid_array;
- struct mutex mutex;
- void *iommu_data;
- void (*iommu_data_release)(void *iommu_data);
- char *name;
- int id;
- struct iommu_domain *default_domain;
- struct iommu_domain *blocking_domain;
- struct iommu_domain *domain;
- struct list_head entry;
- unsigned int owner_cnt;
- void *owner;
-};
-
struct group_device {
struct list_head list;
struct device *dev;
@@ -3498,33 +3480,6 @@ iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int
}
EXPORT_SYMBOL_NS_GPL(iommu_attach_handle_get, IOMMUFD_INTERNAL);
-/**
- * iommu_init_pasid_array - Initialize pasid array in the domain group
- *
- * Returns 0 on success. Error code on failure
- *
- * An IOMMU_NO_PASID element is *NOT* replaced if there is one already there.
- */
-int iommu_init_pasid_array(struct iommu_domain *domain,
- struct iommu_group *group,
- struct iommu_attach_handle *handle)
-{
- int ret;
-
- if (handle)
- handle->domain = domain;
-
- mutex_lock(&group->mutex);
- ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
- mutex_unlock(&group->mutex);
-
- if (ret == -EBUSY)
- ret = 0;
-
- return ret;
-}
-EXPORT_SYMBOL_NS_GPL(iommu_init_pasid_array, IOMMUFD_INTERNAL);
-
/**
* iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group
* @domain: IOMMU domain to attach
diff --git i/drivers/iommu/iommufd/fault.c w/drivers/iommu/iommufd/fault.c
index ea7f1bf64892..51cb70465b87 100644
--- i/drivers/iommu/iommufd/fault.c
+++ w/drivers/iommu/iommufd/fault.c
@@ -189,8 +189,15 @@ static int iommufd_init_pasid_array(struct iommu_domain *domain,
if (!handle)
return -ENOMEM;
handle->idev = idev;
+ handle->handle.domain = domain;
+
+ mutex_lock(&group->mutex);
+ ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+ mutex_unlock(&group->mutex);
+
+ if (ret == -EBUSY)
+ ret = 0;
- ret = iommu_init_pasid_array(domain, group, &handle->handle);
if (ret)
kfree(handle);
diff --git i/include/linux/iommu.h w/include/linux/iommu.h
index bd722f473635..c34aa737aeb2 100644
--- i/include/linux/iommu.h
+++ w/include/linux/iommu.h
@@ -31,8 +31,25 @@
*/
#define IOMMU_PRIV (1 << 5)
+struct iommu_group {
+ struct kobject kobj;
+ struct kobject *devices_kobj;
+ struct list_head devices;
+ struct xarray pasid_array;
+ struct mutex mutex;
+ void *iommu_data;
+ void (*iommu_data_release)(void *iommu_data);
+ char *name;
+ int id;
+ struct iommu_domain *default_domain;
+ struct iommu_domain *blocking_domain;
+ struct iommu_domain *domain;
+ struct list_head entry;
+ unsigned int owner_cnt;
+ void *owner;
+};
+
struct iommu_ops;
-struct iommu_group;
struct bus_type;
struct device;
struct iommu_domain;
--
Joel Granados
On 2024/9/12 16:25, Joel Granados wrote: > /** > * iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group > * @domain: IOMMU domain to attach > diff --git i/drivers/iommu/iommufd/fault.c w/drivers/iommu/iommufd/fault.c > index ea7f1bf64892..51cb70465b87 100644 > --- i/drivers/iommu/iommufd/fault.c > +++ w/drivers/iommu/iommufd/fault.c > @@ -189,8 +189,15 @@ static int iommufd_init_pasid_array(struct iommu_domain *domain, > if (!handle) > return -ENOMEM; > handle->idev = idev; > + handle->handle.domain = domain; > + > + mutex_lock(&group->mutex); > + ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL); > + mutex_unlock(&group->mutex); > + > + if (ret == -EBUSY) > + ret = 0; > > - ret = iommu_init_pasid_array(domain, group, &handle->handle); > if (ret) > kfree(handle); This is supposed to be done automatically when an iopf capable domain is attached to or replaced with a device. Please refer to iommufd_fault_domain_attach_dev() and iommufd_fault_domain_replace_dev(). Is there anything preventing this from happening? Thanks, baolu
On Thu, Sep 12, 2024 at 07:22:30PM +0800, Baolu Lu wrote:
> On 2024/9/12 16:25, Joel Granados wrote:
> > /**
> > * iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group
> > * @domain: IOMMU domain to attach
> > diff --git i/drivers/iommu/iommufd/fault.c w/drivers/iommu/iommufd/fault.c
> > index ea7f1bf64892..51cb70465b87 100644
> > --- i/drivers/iommu/iommufd/fault.c
> > +++ w/drivers/iommu/iommufd/fault.c
> > @@ -189,8 +189,15 @@ static int iommufd_init_pasid_array(struct iommu_domain *domain,
> > if (!handle)
> > return -ENOMEM;
> > handle->idev = idev;
> > + handle->handle.domain = domain;
> > +
> > + mutex_lock(&group->mutex);
> > + ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
> > + mutex_unlock(&group->mutex);
> > +
> > + if (ret == -EBUSY)
> > + ret = 0;
> >
> > - ret = iommu_init_pasid_array(domain, group, &handle->handle);
> > if (ret)
> > kfree(handle);
>
> This is supposed to be done automatically when an iopf capable domain is
> attached to or replaced with a device. Please refer to
> iommufd_fault_domain_attach_dev() and
> iommufd_fault_domain_replace_dev().
>
> Is there anything preventing this from happening?
Nope, You are correct. This does indeed happen through
iommufd_fault_domain_replace_dev
-> __fault_domain_replace_dev
-> iommu_replace_group_handle.
This was probably left from trying to do this outside the replace path.
Sorry for the noise; will remove it in V2.
Thx for the review
Best
--
Joel Granados
On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote: > On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: > > From: Joel Granados<j.granados@samsung.com> > > > > iommu_report_device_fault expects a pasid array to have an > > iommu_attach_handle when a fault is detected. > > The iommu_attach_handle is expected only when an iopf-capable domain is > attached to the device or PASID. The iommu_report_device_fault() treats > it as a fault when a fault occurs, but no iopf-capable domain is > attached. I don't follow. The way that I read it: if the pasid_array x-array does not have an iommu_attach_handle indexed by either fault->prm.pasid or IOMMU_NO_PASID, it will follow the err_bad_iopf and return -EINVAL (please correct me if I'm wrong). So the iommu_attach_handle is *always* expected. Would it be more clear for it to be: """ The iommu_report_device_fault function expects the pasid_array x-array to have an iommu_attach_handle indexed by a PASID. Add one indexed with IOMMU_NO_PASID when the replacing HWPT has a valid iommufd fault object. Remove it when we release ownership of the group. """ best -- Joel Granados
On 9/11/24 5:55 PM, Joel Granados wrote: > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote: >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: >>> From: Joel Granados<j.granados@samsung.com> >>> >>> iommu_report_device_fault expects a pasid array to have an >>> iommu_attach_handle when a fault is detected. >> The iommu_attach_handle is expected only when an iopf-capable domain is >> attached to the device or PASID. The iommu_report_device_fault() treats >> it as a fault when a fault occurs, but no iopf-capable domain is >> attached. > I don't follow. The way that I read it: if the pasid_array x-array does > not have an iommu_attach_handle indexed by either fault->prm.pasid or > IOMMU_NO_PASID, it will follow the err_bad_iopf and return -EINVAL > (please correct me if I'm wrong). So the iommu_attach_handle is*always* > expected. > > Would it be more clear for it to be: > """ > The iommu_report_device_fault function expects the pasid_array x-array > to have an iommu_attach_handle indexed by a PASID. Add one indexed with > IOMMU_NO_PASID when the replacing HWPT has a valid iommufd fault object. > Remove it when we release ownership of the group. Can you please explain why iommu core needs to remove the attach handle when the ownership of the group is changed? Thanks, baolu
On Thu, Sep 12, 2024 at 12:17:35PM +0800, Baolu Lu wrote: > On 9/11/24 5:55 PM, Joel Granados wrote: > > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote: > >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: > >>> From: Joel Granados<j.granados@samsung.com> > >>> > >>> iommu_report_device_fault expects a pasid array to have an > >>> iommu_attach_handle when a fault is detected. > >> The iommu_attach_handle is expected only when an iopf-capable domain is > >> attached to the device or PASID. The iommu_report_device_fault() treats > >> it as a fault when a fault occurs, but no iopf-capable domain is > >> attached. > > I don't follow. The way that I read it: if the pasid_array x-array does > > not have an iommu_attach_handle indexed by either fault->prm.pasid or > > IOMMU_NO_PASID, it will follow the err_bad_iopf and return -EINVAL > > (please correct me if I'm wrong). So the iommu_attach_handle is*always* > > expected. > > > > Would it be more clear for it to be: > > """ > > The iommu_report_device_fault function expects the pasid_array x-array > > to have an iommu_attach_handle indexed by a PASID. Add one indexed with > > IOMMU_NO_PASID when the replacing HWPT has a valid iommufd fault object. > > Remove it when we release ownership of the group. > > Can you please explain why iommu core needs to remove the attach handle > when the ownership of the group is changed? It does not. Probably left from a previous version of the patch. Sorry for the noise. I have reamoved the xa_erase from __iommu_release_dma_ownership. Will send a V2 once we finish discussing the rest of your comments. Best and thankyou for your feedback -- Joel Granados
On 9/5/24 11:30 AM, Baolu Lu wrote: > On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote: >> From: Joel Granados<j.granados@samsung.com> >> >> iommu_report_device_fault expects a pasid array to have an >> iommu_attach_handle when a fault is detected. > > The iommu_attach_handle is expected only when an iopf-capable domain is > attached to the device or PASID. The iommu_report_device_fault() treats > it as a fault when a fault occurs, but no iopf-capable domain is > attached. > >> Add this handle when the >> replacing hwpt has a valid iommufd fault object. Remove it when we >> release ownership of the group. > > The iommu_attach_handle is managed by the caller (iommufd here for > example). Therefore, before iommu_attach_handle tries to attach a domain > to an iopf-capable device or pasid, it should allocate the handle and Correct: "... attach an iopf-capable domain to device or pasid ..." Sorry for the typo. > pass it to the domain attachment interfaces. Conversely, the handle can > only be freed after the domain is detached. > > Thanks, > baolu
© 2016 - 2025 Red Hat, Inc.