| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <acd5c4a6-040c-48ff-9e6b-1a33e5ff118d@linux.intel.com> Date: Tue, 29 Oct 2024 13:39:50 +0800 From: Baolu Lu <baolu.lu@...ux.intel.com> To: Yi Liu <yi.l.liu@...el.com>, Joel Granados <joel.granados@...nel.org> Cc: baolu.lu@...ux.intel.com, David Woodhouse <dwmw2@...radead.org>, Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>, Jason Gunthorpe <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>, Klaus Jensen <its@...elevant.dk>, linux-kernel@...r.kernel.org, iommu@...ts.linux.dev, Klaus Jensen <k.jensen@...sung.com> Subject: Re: [PATCH v4 2/5] iommu/vt-d: Remove the pasid present check in prq_event_thread On 2024/10/29 13:13, Yi Liu wrote: > On 2024/10/29 11:12, Baolu Lu wrote: >> On 2024/10/28 18:24, Joel Granados wrote: >>> On Mon, Oct 28, 2024 at 03:50:46PM +0800, Yi Liu wrote: >>>> On 2024/10/16 05:08, Joel Granados wrote: >>>>> From: Klaus Jensen<k.jensen@...sung.com> >>>>> >>>>> PASID is not strictly needed when handling a PRQ event; remove the >>>>> check >>>>> for the pasid present bit in the request. This change was not included >>>>> in the creation of prq.c to emphasize the change in capability checks >>>>> when handing PRQ events. >>>>> >>>>> Signed-off-by: Klaus Jensen<k.jensen@...sung.com> >>>>> Reviewed-by: Kevin Tian<kevin.tian@...el.com> >>>>> Signed-off-by: Joel Granados<joel.granados@...nel.org> >>>> looks like the PRQ draining is missed for the PRI usage. When a pasid >>>> entry is destroyed, it might need to add helper similar to the >>>> intel_drain_pasid_prq() to drain PRQ for the non-pasid usage. >>> These types of user space PRIs (non-pasid, non-svm) are created by >>> making use of iommufd_hwpt_replace_device. Which adds an entry to the >>> pasid_array indexed on IOMMU_NO_PASID (0U) via the following path: >>> >>> iommufd_hwpt_replace_device >>> -> iommufd_fault_domain_repalce_dev >>> -> __fault_domain_replace_dev >>> -> iommu_replace_group_handle >> -> __iommu_group_set_domain >> -> intel_iommu_attach_device >> -> device_block_translation >> -> intel_pasid_tear_down_entry(IOMMU_NO_PASID) >> >> Here a domain is removed from the pasid entry, hence we need to flush >> all page requests that are pending in the IOMMU page request queue or >> the PCI fabric. >> >>> -> xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL); >>> >>> It is my understanding that this will provide the needed relation >>> between the device and the prq in such a way that when remove_dev_pasid >>> is called, intel_iommu_drain_pasid_prq will be called with the >>> appropriate pasid value set to IOMMU_NO_PASID. Please correct me if I'm >>> mistaken. >> >> Removing a domain from a RID and a PASID are different paths. >> Previously, this IOMMU driver only supported page requests on PASID >> (non-IOMMU_NO_PASID). It is acceptable that it does not flush the PRQ in >> the domain-removing RID path. >> >> With the changes made in this series, the driver now supports page >> requests for RID. It should also flush the PRQ when removing a domain >> from a PASID entry for IOMMU_NO_PASID. >> >>> >>> Does this answer your question? Do you have a specific path that you are >>> looking at where a specific non-pasid drain is needed? >> >> Perhaps we can simply add below change. >> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index e860bc9439a2..a24a42649621 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -4283,7 +4283,6 @@ static void intel_iommu_remove_dev_pasid(struct >> device *dev, ioasid_t pasid, >> intel_iommu_debugfs_remove_dev_pasid(dev_pasid); >> kfree(dev_pasid); >> intel_pasid_tear_down_entry(iommu, dev, pasid, false); >> - intel_drain_pasid_prq(dev, pasid); >> } >> >> static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c >> index 2e5fa0a23299..8639f3eb4264 100644 >> --- a/drivers/iommu/intel/pasid.c >> +++ b/drivers/iommu/intel/pasid.c >> @@ -265,6 +265,7 @@ void intel_pasid_tear_down_entry(struct >> intel_iommu *iommu, struct device *dev, >> iommu->flush.flush_iotlb(iommu, did, 0, 0, >> DMA_TLB_DSI_FLUSH); >> >> devtlb_invalidation_with_pasid(iommu, dev, pasid); >> + intel_drain_pasid_prq(dev, pasid); >> } >> >> /* >> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c >> index 078d1e32a24e..ff88f31053d1 100644 >> --- a/drivers/iommu/intel/svm.c >> +++ b/drivers/iommu/intel/svm.c >> @@ -304,9 +304,6 @@ void intel_drain_pasid_prq(struct device *dev, u32 >> pasid) >> int qdep; >> >> info = dev_iommu_priv_get(dev); >> - if (WARN_ON(!info || !dev_is_pci(dev))) >> - return; >> - >> if (!info->pri_enabled) >> return; >> >> Generally, intel_drain_pasid_prq() should be called if >> >> - a translation is removed from a pasid entry; and >> - PRI on this device is enabled. > > If the @pasid==IOMMU_NO_PASID, PRQ drain should use the iotlb invalidation > and dev-tlb invalidation descriptors. So extra code change is needed in > intel_drain_pasid_prq(). Or perhaps it's better to have a separate helper > for draining prq for non-pasid case. According to VT-d spec, section 7.10, "Software Steps to Drain Page Requests & Responses", we can simply replace p_iotlb_inv_dsc and p_dev_tlb_inv_dsc with iotlb_inv_dsc and dev_tlb_inv_dsc. Any significant negative performance impact? -- baolu
Powered by blists - more mailing lists