| Message ID | 20241104132513.15890-3-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
On 11/4/24 21:25, Yi Liu wrote: > There is a wrapper of iommu_attach_group_handle(), so making a wrapper for > iommu_replace_group_handle() for further code refactor. No functional change > intended. This patch is not a simple, non-functional refactoring. It allocates attach_handle for all devices in domain attach/replace interfaces, regardless of whether the domain is iopf-capable. Therefore, the commit message should be rephrased to accurately reflect the patch's purpose and rationale. -- baolu
On 2024/11/5 13:06, Baolu Lu wrote: > On 11/4/24 21:25, Yi Liu wrote: >> There is a wrapper of iommu_attach_group_handle(), so making a wrapper for >> iommu_replace_group_handle() for further code refactor. No functional change >> intended. > > This patch is not a simple, non-functional refactoring. It allocates > attach_handle for all devices in domain attach/replace interfaces, > regardless of whether the domain is iopf-capable. Therefore, the commit > message should be rephrased to accurately reflect the patch's purpose > and rationale. This patch splits the __fault_domain_replace_dev() a lot, the else branch of the below code was lifted to the iommufd_fault_domain_replace_dev(). While the new __fault_domain_replace_dev() will only be called when the hwpt->fault is valid. So the iommu_attach_handle is still allocated only for the iopf-capable path. When the hwpt->fault is invalid, the iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with a null iommu_attach_handle. What you described is done in the patch 04 of this series. :) - if (hwpt->fault) { - handle = kzalloc(sizeof(*handle), GFP_KERNEL); - if (!handle) - return -ENOMEM; - - 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); - }
On 2024/11/5 16:01, Yi Liu wrote: > On 2024/11/5 13:06, Baolu Lu wrote: >> On 11/4/24 21:25, Yi Liu wrote: >>> There is a wrapper of iommu_attach_group_handle(), so making a >>> wrapper for >>> iommu_replace_group_handle() for further code refactor. No functional >>> change >>> intended. >> >> This patch is not a simple, non-functional refactoring. It allocates >> attach_handle for all devices in domain attach/replace interfaces, >> regardless of whether the domain is iopf-capable. Therefore, the commit >> message should be rephrased to accurately reflect the patch's purpose >> and rationale. > > This patch splits the __fault_domain_replace_dev() a lot, the else > branch of the below code was lifted to the > iommufd_fault_domain_replace_dev(). > While the new __fault_domain_replace_dev() will only be called when the > hwpt->fault is valid. So the iommu_attach_handle is still allocated only > for the iopf-capable path. When the hwpt->fault is invalid, the > iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with > a null iommu_attach_handle. What you described is done in the patch 04 of > this series.
On 2024/11/5 16:03, Baolu Lu wrote: > On 2024/11/5 16:01, Yi Liu wrote: >> On 2024/11/5 13:06, Baolu Lu wrote: >>> On 11/4/24 21:25, Yi Liu wrote: >>>> There is a wrapper of iommu_attach_group_handle(), so making a wrapper for >>>> iommu_replace_group_handle() for further code refactor. No functional >>>> change >>>> intended. >>> >>> This patch is not a simple, non-functional refactoring. It allocates >>> attach_handle for all devices in domain attach/replace interfaces, >>> regardless of whether the domain is iopf-capable. Therefore, the commit >>> message should be rephrased to accurately reflect the patch's purpose >>> and rationale. >> >> This patch splits the __fault_domain_replace_dev() a lot, the else branch >> of the below code was lifted to the iommufd_fault_domain_replace_dev(). >> While the new __fault_domain_replace_dev() will only be called when the >> hwpt->fault is valid. So the iommu_attach_handle is still allocated only >> for the iopf-capable path. When the hwpt->fault is invalid, the >> iommufd_fault_domain_replace_dev() calls iommu_replace_group_handle() with >> a null iommu_attach_handle. What you described is done in the patch 04 of >> this series.
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index e590973ce5cf..230d37c17102 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -146,33 +146,23 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, kfree(handle); } -static int __fault_domain_replace_dev(struct iommufd_device *idev, - struct iommufd_hw_pagetable *hwpt, - struct iommufd_hw_pagetable *old) +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; + struct iommufd_attach_handle *handle; 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->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); - } + handle = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; - if (!ret && curr) { - iommufd_auto_response_faults(old, curr); - kfree(curr); - } + handle->idev = idev; + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, &handle->handle); + if (ret) + kfree(handle); return ret; } @@ -183,6 +173,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, { bool iopf_off = !hwpt->fault && old->fault; bool iopf_on = hwpt->fault && !old->fault; + struct iommufd_attach_handle *curr; int ret; if (iopf_on) { @@ -191,13 +182,24 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, return ret; } - ret = __fault_domain_replace_dev(idev, hwpt, old); + curr = iommufd_device_get_attach_handle(idev); + + if (hwpt->fault) + ret = __fault_domain_replace_dev(idev, hwpt, old); + else + ret = iommu_replace_group_handle(idev->igroup->group, + hwpt->domain, NULL); if (ret) { if (iopf_on) iommufd_fault_iopf_disable(idev); return ret; } + if (curr) { + iommufd_auto_response_faults(old, curr); + kfree(curr); + } + if (iopf_off) iommufd_fault_iopf_disable(idev);
There is a wrapper of iommu_attach_group_handle(), so making a wrapper for iommu_replace_group_handle() for further code refactor. No functional change intended. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/iommufd/fault.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)