diff mbox series

[v3,4/7] iommu/vt-d: Fold dmar_remove_one_dev_info() into its caller

Message ID 20221114014049.3959-5-baolu.lu@linux.intel.com
State New
Headers
Series iommu/vt-d: Some cleanups |

Commit Message

Baolu Lu Nov. 14, 2022, 1:40 a.m. UTC 
  Fold dmar_remove_one_dev_info() into intel_iommu_release_device() which
is its only caller. Replace most of the code with
device_block_translation() to make the code neat and tidy.

Rename iommu_disable_dev_iotlb() to iommu_disable_pci_caps() to pair with
iommu_enable_pci_caps().

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

Comments

Tian, Kevin Nov. 16, 2022, 3:53 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, November 14, 2022 9:41 AM
> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct
> device *dev)
>  {
>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
> 
> -	dmar_remove_one_dev_info(dev);
> +	iommu_disable_pci_caps(info);
> +	domain_context_clear(info);
> +	device_block_translation(dev);

clear context after blocking translation.

with that,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Baolu Lu Nov. 16, 2022, 4:36 a.m. UTC | #2 
On 11/16/22 11:53 AM, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Monday, November 14, 2022 9:41 AM
>> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct
>> device *dev)
>>   {
>>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>
>> -	dmar_remove_one_dev_info(dev);
>> +	iommu_disable_pci_caps(info);
>> +	domain_context_clear(info);
>> +	device_block_translation(dev);
> clear context after blocking translation.

Unfortunately domain_context_clear() needs reference to info->domain
(for domain id when flushing cache), which is cleared in
device_block_translation().

Best regards,
baolu
Tian, Kevin Nov. 16, 2022, 5:35 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, November 16, 2022 12:36 PM
> 
> On 11/16/22 11:53 AM, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@linux.intel.com>
> >> Sent: Monday, November 14, 2022 9:41 AM
> >> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct
> >> device *dev)
> >>   {
> >>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
> >>
> >> -	dmar_remove_one_dev_info(dev);
> >> +	iommu_disable_pci_caps(info);
> >> +	domain_context_clear(info);
> >> +	device_block_translation(dev);
> > clear context after blocking translation.
> 
> Unfortunately domain_context_clear() needs reference to info->domain
> (for domain id when flushing cache), which is cleared in
> device_block_translation().
> 

this sounds an ordering problem. clearing context should be after
blocking translation in concept.
Baolu Lu Nov. 16, 2022, 8:02 a.m. UTC | #4 
On 2022/11/16 13:35, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, November 16, 2022 12:36 PM
>>
>> On 11/16/22 11:53 AM, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Monday, November 14, 2022 9:41 AM
>>>> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct
>>>> device *dev)
>>>>    {
>>>>    	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>>
>>>> -	dmar_remove_one_dev_info(dev);
>>>> +	iommu_disable_pci_caps(info);
>>>> +	domain_context_clear(info);
>>>> +	device_block_translation(dev);
>>> clear context after blocking translation.
>> Unfortunately domain_context_clear() needs reference to info->domain
>> (for domain id when flushing cache), which is cleared in
>> device_block_translation().
>>
> this sounds an ordering problem. clearing context should be after
> blocking translation in concept.

At present, when the default domain is attached to the device, we first
populate the pasid table entry, and then populate the device context
entry. Above code is just the reverse operation.

Can you see any practical problems caused by this sequence? If so, it
seems that we should carefully consider whether such problems already
exist.

Best regards,
baolu
Tian, Kevin Nov. 16, 2022, 9:15 a.m. UTC | #5 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, November 16, 2022 4:03 PM
> 
> On 2022/11/16 13:35, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@linux.intel.com>
> >> Sent: Wednesday, November 16, 2022 12:36 PM
> >>
> >> On 11/16/22 11:53 AM, Tian, Kevin wrote:
> >>>> From: Lu Baolu<baolu.lu@linux.intel.com>
> >>>> Sent: Monday, November 14, 2022 9:41 AM
> >>>> @@ -4562,7 +4538,10 @@ static void
> intel_iommu_release_device(struct
> >>>> device *dev)
> >>>>    {
> >>>>    	struct device_domain_info *info = dev_iommu_priv_get(dev);
> >>>>
> >>>> -	dmar_remove_one_dev_info(dev);
> >>>> +	iommu_disable_pci_caps(info);
> >>>> +	domain_context_clear(info);
> >>>> +	device_block_translation(dev);
> >>> clear context after blocking translation.
> >> Unfortunately domain_context_clear() needs reference to info->domain
> >> (for domain id when flushing cache), which is cleared in
> >> device_block_translation().
> >>
> > this sounds an ordering problem. clearing context should be after
> > blocking translation in concept.
> 
> At present, when the default domain is attached to the device, we first
> populate the pasid table entry, and then populate the device context
> entry. Above code is just the reverse operation.
> 
> Can you see any practical problems caused by this sequence? If so, it
> seems that we should carefully consider whether such problems already
> exist.
> 

there is no problem with existing code. Just after this patch the order
looks weird based on the literal name of those functions.

domain_context_clear() is a big hammer to disable the context entry,
implying translation must be blocked. Then calling another block
translation afterwards becomes unnecessary.

Probably it should be split into two functions with one requiring 
info->domain called before block translation and the rest which
actually clears the context entry being the last step?
Baolu Lu Nov. 16, 2022, 12:08 p.m. UTC | #6 
On 2022/11/16 17:15, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, November 16, 2022 4:03 PM
>>
>> On 2022/11/16 13:35, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Wednesday, November 16, 2022 12:36 PM
>>>>
>>>> On 11/16/22 11:53 AM, Tian, Kevin wrote:
>>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>> Sent: Monday, November 14, 2022 9:41 AM
>>>>>> @@ -4562,7 +4538,10 @@ static void
>> intel_iommu_release_device(struct
>>>>>> device *dev)
>>>>>>     {
>>>>>>     	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>>>>>
>>>>>> -	dmar_remove_one_dev_info(dev);
>>>>>> +	iommu_disable_pci_caps(info);
>>>>>> +	domain_context_clear(info);
>>>>>> +	device_block_translation(dev);
>>>>> clear context after blocking translation.
>>>> Unfortunately domain_context_clear() needs reference to info->domain
>>>> (for domain id when flushing cache), which is cleared in
>>>> device_block_translation().
>>>>
>>> this sounds an ordering problem. clearing context should be after
>>> blocking translation in concept.
>>
>> At present, when the default domain is attached to the device, we first
>> populate the pasid table entry, and then populate the device context
>> entry. Above code is just the reverse operation.
>>
>> Can you see any practical problems caused by this sequence? If so, it
>> seems that we should carefully consider whether such problems already
>> exist.
>>
> 
> there is no problem with existing code. Just after this patch the order
> looks weird based on the literal name of those functions.
> 
> domain_context_clear() is a big hammer to disable the context entry,
> implying translation must be blocked. Then calling another block
> translation afterwards becomes unnecessary.
> 
> Probably it should be split into two functions with one requiring
> info->domain called before block translation and the rest which
> actually clears the context entry being the last step?

This is what the existing code does. Perhaps I should drop this patch,
or only rename iommu_disable_dev_iotlb() to iommu_disable_pci_caps().

Best regards,
baolu
diff mbox series

Patch

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3d3fa182fcb1..f165e0d37bab 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1443,7 +1443,7 @@  static void iommu_enable_pci_caps(struct device_domain_info *info)
 	}
 }
 
-static void iommu_disable_dev_iotlb(struct device_domain_info *info)
+static void iommu_disable_pci_caps(struct device_domain_info *info)
 {
 	struct pci_dev *pdev;
 
@@ -4089,30 +4089,6 @@  static void domain_context_clear(struct device_domain_info *info)
 			       &domain_context_clear_one_cb, info);
 }
 
-static void dmar_remove_one_dev_info(struct device *dev)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct dmar_domain *domain = info->domain;
-	struct intel_iommu *iommu = info->iommu;
-	unsigned long flags;
-
-	if (!dev_is_real_dma_subdevice(info->dev)) {
-		if (dev_is_pci(info->dev) && sm_supported(iommu))
-			intel_pasid_tear_down_entry(iommu, info->dev,
-					PASID_RID2PASID, false);
-
-		iommu_disable_dev_iotlb(info);
-		domain_context_clear(info);
-	}
-
-	spin_lock_irqsave(&domain->lock, flags);
-	list_del(&info->link);
-	spin_unlock_irqrestore(&domain->lock, flags);
-
-	domain_detach_iommu(domain, iommu);
-	info->domain = NULL;
-}
-
 /*
  * Clear the page table pointer in context or pasid table entries so that
  * all DMA requests without PASID from the device are blocked. If the page
@@ -4124,7 +4100,7 @@  static void device_block_translation(struct device *dev)
 	struct intel_iommu *iommu = info->iommu;
 	unsigned long flags;
 
-	iommu_disable_dev_iotlb(info);
+	iommu_disable_pci_caps(info);
 	if (!dev_is_real_dma_subdevice(dev)) {
 		if (sm_supported(iommu))
 			intel_pasid_tear_down_entry(iommu, dev,
@@ -4562,7 +4538,10 @@  static void intel_iommu_release_device(struct device *dev)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 
-	dmar_remove_one_dev_info(dev);
+	iommu_disable_pci_caps(info);
+	domain_context_clear(info);
+	device_block_translation(dev);
+
 	intel_pasid_free_table(dev);
 	dev_iommu_priv_set(dev, NULL);
 	kfree(info);