Messages in this thread | | | Date | Wed, 16 Nov 2022 20:08:30 +0800 | Subject | Re: [PATCH v3 4/7] iommu/vt-d: Fold dmar_remove_one_dev_info() into its caller | From | Baolu Lu <> |
| |
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
| |