| Message ID | 20240208082307.15759-1-yi.l.liu@intel.com |
|---|---|
| Headers | show |
| Series | Add missing cache flush and dirty tracking set for nested parent domain | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, February 8, 2024 4:23 PM > > static void intel_nested_domain_free(struct iommu_domain *domain) > { > + struct dmar_domain *dmar_domain = to_dmar_domain(domain); > + struct dmar_domain *s2_domain = dmar_domain->s2_domain; > + > + spin_lock(&s2_domain->s1_lock); > + list_del(&dmar_domain->s2_link); > + spin_unlock(&s2_domain->s1_lock); > kfree(to_dmar_domain(domain)); use 'dmar_domain'. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, February 8, 2024 4:23 PM > > +/* > + * Flush the relevant caches in nested translation if the domain > + * also serves as a parent > + */ > +static void parent_domain_flush(struct dmar_domain *domain, > + unsigned long pfn, > + unsigned long pages, int ih) > +{ > + struct dmar_domain *s1_domain; > + > + spin_lock(&domain->s1_lock); > + list_for_each_entry(s1_domain, &domain->s1_domains, s2_link) { > + struct iommu_domain_info *info; > + unsigned long i; > + > + xa_for_each(&s1_domain->iommu_array, i, info) > + __iommu_flush_iotlb_psi(info->iommu, info->did, > + pfn, pages, ih); > + } As Jason suggested before this xarray lacks of proper locking. but given it's rc fix I'm fine with it. @Baolu we do need fix it soon so this problem won't further expand like here. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, February 8, 2024 4:23 PM > > ATS-capable devices cache the result of nested translation. This result > relies on the mappings in s2 domain (a.k.a. parent). When there are > modifications in the s2 domain, the related nested translation caches on > the device should be flushed. This includes the devices that are attached > to the s1 domain. However, the existing code ignores this fact to only > loops its own devices. > > As there is no easy way to identify the exact set of nested translations > affected by the change of s2 domain. So, this just flushes the entire > device iotlb on the device. > > As above, driver loops the s2 domain's s1_domains list and loops the devices > list of each s1_domain to flush the entire device iotlb on the devices. > > Fixes: b41e38e22539 ("iommu/vt-d: Add nested domain allocation") > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, February 8, 2024 4:23 PM > > Add device_set_dirty_tracking() to loop all the devices and set the dirty > tracking per the @enable parameter. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 08/02/2024 08:23, Yi Liu wrote: > Add device_set_dirty_tracking() to loop all the devices and set the dirty > tracking per the @enable parameter. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Nice cleanup, Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > --- > drivers/iommu/intel/iommu.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index dae20991e036..7636d3f03905 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -4730,23 +4730,35 @@ static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type) > return vtd; > } > > +static int > +device_set_dirty_tracking(struct list_head *devices, bool enable) > +{ > + struct device_domain_info *info; > + int ret = 0; > + > + list_for_each_entry(info, devices, link) { > + ret = intel_pasid_setup_dirty_tracking(info->iommu, info->dev, > + IOMMU_NO_PASID, enable); > + if (ret) > + break; > + } > + > + return ret; > +} > + > static int intel_iommu_set_dirty_tracking(struct iommu_domain *domain, > bool enable) > { > struct dmar_domain *dmar_domain = to_dmar_domain(domain); > - struct device_domain_info *info; > int ret; > > spin_lock(&dmar_domain->lock); > if (dmar_domain->dirty_tracking == enable) > goto out_unlock; > > - list_for_each_entry(info, &dmar_domain->devices, link) { > - ret = intel_pasid_setup_dirty_tracking(info->iommu, info->dev, > - IOMMU_NO_PASID, enable); > - if (ret) > - goto err_unwind; > - } > + ret = device_set_dirty_tracking(&dmar_domain->devices, enable); > + if (ret) > + goto err_unwind; > > dmar_domain->dirty_tracking = enable; > out_unlock: > @@ -4755,10 +4767,8 @@ static int intel_iommu_set_dirty_tracking(struct iommu_domain *domain, > return 0; > > err_unwind: > - list_for_each_entry(info, &dmar_domain->devices, link) > - intel_pasid_setup_dirty_tracking(info->iommu, info->dev, > - IOMMU_NO_PASID, > - dmar_domain->dirty_tracking); > + device_set_dirty_tracking(&dmar_domain->devices, > + dmar_domain->dirty_tracking); > spin_unlock(&dmar_domain->lock); > return ret; > }
This series adds the missing cache flush and dirty track set for nested parent domain (it's s2_domain but used as parent) which has no insight into devices/DID's under the nested domain (a.k.a s1_domain). This results in missing cache flush per parent domain change and incomplete dirty tracking set on the parent domain. There was a discussion about this in the mailing list [1]. This series adds a s1_domains list in the parent domain to track the nested domains. Hence, the driver can loop the nested domains and use the DIDs of the nested domain to flush iotlb. The driver can also loop the nested domains and nested domain's devices list to flush device iotlb and set the dirty tracking completely. This series doesn't touch the pasid iotlb or the pasid devtlb as there is no support of attaching pasid to nested domain yet. It will be covered when that feature is enabled. The complete code can be found at[2], this has been tested with a hacky Qemu[3] to test the unmap on parent domain and dirty tracking set on parent domain. This just verifies the new path. So appreciated to see t-b with regression tests. This aims to the 6.8 rc#, so your special attention is welcomed. [1] https://lore.kernel.org/linux-iommu/92f8aaca-093d-4161-b8f2-5ab1680df769@intel.com/ [2] https://github.com/yiliu1765/iommufd/tree/vtd_nesting_fixes [3] https://github.com/yiliu1765/qemu/tree/tmp/for-testing-unmap-and-dirty-set-on-parent base commit: 547ab8fc4cb04a1a6b34377dd8fad34cd2c8a8e3 Regards, Yi Liu Yi Liu (8): iommu/vt-d: Track nested domains in parent iommu/vt-d: Add __iommu_flush_iotlb_psi() iommu/vt-d: Add missing iotlb flush for parent domain iommu/vt-d: Update iotlb in nested domain attach iommu/vt-d: Add missing device iotlb flush for parent domain iommu/vt-d: Remove @domain parameter from intel_pasid_setup_dirty_tracking() iommu/vt-d: Wrap the dirty tracking loop to be a helper iommu/vt-d: Add missing dirty tracking set for parent domain drivers/iommu/intel/iommu.c | 213 ++++++++++++++++++++++++++--------- drivers/iommu/intel/iommu.h | 7 ++ drivers/iommu/intel/nested.c | 12 ++ drivers/iommu/intel/pasid.c | 3 +- drivers/iommu/intel/pasid.h | 1 - 5 files changed, 182 insertions(+), 54 deletions(-)