| Message ID | 20240628085538.47049-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make set_dev_pasid op supportting domain replacement | expand |
On 2024/6/28 16:55, Yi Liu wrote: > To handle domain replacement, set_dev_pasid op needs to modify a present > pasid entry. One way is sharing the most logics of remove_dev_pasid() in > the beginning of set_dev_pasid() to remove the old config. But this means > the set_dev_pasid path needs to rollback to the old config if it fails to > set up the new pasid entry. This needs to invoke the set_dev_pasid op of > the old domain. It breaks the iommu layering a bit. Another way is > implementing the set_dev_pasid() without rollback to old hardware config. > This can be achieved by implementing it in the order of preparing the > dev_pasid info for the new domain, modify the pasid entry, then undo the > dev_pasid info of the old domain, and if failed, undo the dev_pasid info > of the new domain. This would keep the old domain unchanged. > > Following the second way, needs to make the pasid entry set up helpers > support modifying present pasid entry. > > Signed-off-by: Yi Liu<yi.l.liu@intel.com> > --- > drivers/iommu/intel/pasid.c | 37 ++++++++++++------------------------- > 1 file changed, 12 insertions(+), 25 deletions(-) > > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c > index b18eebb479de..5d3a12b081a2 100644 > --- a/drivers/iommu/intel/pasid.c > +++ b/drivers/iommu/intel/pasid.c > @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu, > return -EINVAL; > } > > + /* Clear the old configuration if it already exists */ > + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); > + > spin_lock(&iommu->lock); > pte = intel_pasid_get_entry(dev, pasid); > if (!pte) { > @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu, > return -ENODEV; > } > > - if (pasid_pte_is_present(pte)) { > - spin_unlock(&iommu->lock); > - return -EBUSY; > - } > - > - pasid_clear_entry(pte); > - > /* Setup the first level page table pointer: */ > pasid_set_flptr(pte, (u64)__pa(pgd)); The above changes the previous assumption that when a new page table is about to be set up on a PASID, there should be no existing one still in place. Is this a requirement for the replace functionality? Best regards, baolu
On 2024/6/28 17:52, Baolu Lu wrote: > On 2024/6/28 16:55, Yi Liu wrote: >> To handle domain replacement, set_dev_pasid op needs to modify a present >> pasid entry. One way is sharing the most logics of remove_dev_pasid() in >> the beginning of set_dev_pasid() to remove the old config. But this means >> the set_dev_pasid path needs to rollback to the old config if it fails to >> set up the new pasid entry. This needs to invoke the set_dev_pasid op of >> the old domain. It breaks the iommu layering a bit. Another way is >> implementing the set_dev_pasid() without rollback to old hardware config. >> This can be achieved by implementing it in the order of preparing the >> dev_pasid info for the new domain, modify the pasid entry, then undo the >> dev_pasid info of the old domain, and if failed, undo the dev_pasid info >> of the new domain. This would keep the old domain unchanged. >> >> Following the second way, needs to make the pasid entry set up helpers >> support modifying present pasid entry. >> >> Signed-off-by: Yi Liu<yi.l.liu@intel.com> >> --- >> drivers/iommu/intel/pasid.c | 37 ++++++++++++------------------------- >> 1 file changed, 12 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c >> index b18eebb479de..5d3a12b081a2 100644 >> --- a/drivers/iommu/intel/pasid.c >> +++ b/drivers/iommu/intel/pasid.c >> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu >> *iommu, >> return -EINVAL; >> } >> + /* Clear the old configuration if it already exists */ >> + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); >> + [1] >> spin_lock(&iommu->lock); >> pte = intel_pasid_get_entry(dev, pasid); >> if (!pte) { >> @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu >> *iommu, >> return -ENODEV; >> } >> - if (pasid_pte_is_present(pte)) { >> - spin_unlock(&iommu->lock); >> - return -EBUSY; >> - } >> - >> - pasid_clear_entry(pte); >> - >> /* Setup the first level page table pointer: */ >> pasid_set_flptr(pte, (u64)__pa(pgd)); > > The above changes the previous assumption that when a new page table is > about to be set up on a PASID, there should be no existing one still in > place. actually, this does not break the assumption. In [1]. it already clears the pasid entry. > Is this a requirement for the replace functionality? Replace would surely need to modify a present pasid entry. But this patch only moves adds the pasid entry tear down and prq drain into the helpers.
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Friday, June 28, 2024 4:56 PM > > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c > index b18eebb479de..5d3a12b081a2 100644 > --- a/drivers/iommu/intel/pasid.c > +++ b/drivers/iommu/intel/pasid.c > @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu > *iommu, > return -EINVAL; > } > > + /* Clear the old configuration if it already exists */ > + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); > + > spin_lock(&iommu->lock); > pte = intel_pasid_get_entry(dev, pasid); > if (!pte) { with this change there will be two invocations on intel_pasid_tear_down_entry() in the call stack of RID attach: intel_iommu_attach_device() device_block_translation() intel_pasid_tear_down_entry() dmar_domain_attach_device() domain_setup_first_level() intel_pasid_tear_down_entry() it's not being a real problem as intel_pasid_tear_down_entry() exits early if the pasid entry is non-present, but it will likely cause confusion when reading the code. What about moving it into intel_iommu_set_dev_pasid() to better show the purpose?
On 2024/7/15 15:53, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Friday, June 28, 2024 4:56 PM >> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c >> index b18eebb479de..5d3a12b081a2 100644 >> --- a/drivers/iommu/intel/pasid.c >> +++ b/drivers/iommu/intel/pasid.c >> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu >> *iommu, >> return -EINVAL; >> } >> >> + /* Clear the old configuration if it already exists */ >> + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); >> + >> spin_lock(&iommu->lock); >> pte = intel_pasid_get_entry(dev, pasid); >> if (!pte) { > > with this change there will be two invocations on > intel_pasid_tear_down_entry() in the call stack of RID attach: > > intel_iommu_attach_device() > device_block_translation() > intel_pasid_tear_down_entry() > dmar_domain_attach_device() > domain_setup_first_level() > intel_pasid_tear_down_entry() > > it's not being a real problem as intel_pasid_tear_down_entry() > exits early if the pasid entry is non-present, but it will likely cause > confusion when reading the code. > > What about moving it into intel_iommu_set_dev_pasid() to > better show the purpose? I see. will do.
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c index b18eebb479de..5d3a12b081a2 100644 --- a/drivers/iommu/intel/pasid.c +++ b/drivers/iommu/intel/pasid.c @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu, return -EINVAL; } + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); + spin_lock(&iommu->lock); pte = intel_pasid_get_entry(dev, pasid); if (!pte) { @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu, return -ENODEV; } - if (pasid_pte_is_present(pte)) { - spin_unlock(&iommu->lock); - return -EBUSY; - } - - pasid_clear_entry(pte); - /* Setup the first level page table pointer: */ pasid_set_flptr(pte, (u64)__pa(pgd)); @@ -378,6 +374,9 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu, pgd_val = virt_to_phys(pgd); did = domain_id_iommu(domain, iommu); + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); + spin_lock(&iommu->lock); pte = intel_pasid_get_entry(dev, pasid); if (!pte) { @@ -385,12 +384,6 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu, return -ENODEV; } - if (pasid_pte_is_present(pte)) { - spin_unlock(&iommu->lock); - return -EBUSY; - } - - pasid_clear_entry(pte); pasid_set_domain_id(pte, did); pasid_set_slptr(pte, pgd_val); pasid_set_address_width(pte, domain->agaw); @@ -488,6 +481,9 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu, u16 did = FLPT_DEFAULT_DID; struct pasid_entry *pte; + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); + spin_lock(&iommu->lock); pte = intel_pasid_get_entry(dev, pasid); if (!pte) { @@ -495,12 +491,6 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu, return -ENODEV; } - if (pasid_pte_is_present(pte)) { - spin_unlock(&iommu->lock); - return -EBUSY; - } - - pasid_clear_entry(pte); pasid_set_domain_id(pte, did); pasid_set_address_width(pte, iommu->agaw); pasid_set_translation_type(pte, PASID_ENTRY_PGTT_PT); @@ -606,18 +596,15 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev, return -EINVAL; } + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, false, true); + spin_lock(&iommu->lock); pte = intel_pasid_get_entry(dev, pasid); if (!pte) { spin_unlock(&iommu->lock); return -ENODEV; } - if (pasid_pte_is_present(pte)) { - spin_unlock(&iommu->lock); - return -EBUSY; - } - - pasid_clear_entry(pte); if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL) pasid_set_flpm(pte, 1);
To handle domain replacement, set_dev_pasid op needs to modify a present pasid entry. One way is sharing the most logics of remove_dev_pasid() in the beginning of set_dev_pasid() to remove the old config. But this means the set_dev_pasid path needs to rollback to the old config if it fails to set up the new pasid entry. This needs to invoke the set_dev_pasid op of the old domain. It breaks the iommu layering a bit. Another way is implementing the set_dev_pasid() without rollback to old hardware config. This can be achieved by implementing it in the order of preparing the dev_pasid info for the new domain, modify the pasid entry, then undo the dev_pasid info of the old domain, and if failed, undo the dev_pasid info of the new domain. This would keep the old domain unchanged. Following the second way, needs to make the pasid entry set up helpers support modifying present pasid entry. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/pasid.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-)