Re: [PATCH v2 2/8] iommu/vt-d: Improve iommu_enable_pci_caps()

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2022/11/11 16:16, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, November 11, 2022 2:59 PM
>>
>> On 2022/11/11 11:45, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, November 8, 2022 3:34 PM
>>>>
>>>> The PCI subsystem triggers WARN() if a feature is repeatedly enabled.
>>>> This improves iommu_enable_pci_caps() to avoid unnecessary kernel
>>>> traces through checking and enabling. This also adds kernel messages
>>>> if any feature enabling results in failure. It is worth noting that
>>>> PRI depends on ATS. This adds a check as well.
>>>
>>> Cannot we have a helper to check whether this device has been attached
>>> to any domain? If no in the blocking path then disable PCI caps. If no
>>> in the attaching path then enable PCI caps.
>>>
>>> I just didn't get the point of leaving them enabled while the device can
>>> not do any DMA at all.
>>
>> Ideally, the kernel owns the default policy (default on or off). The
>> upper layers are able to control it over IOMMUFD uAPI or kerneld kAPI.
>> I can't see the benefits of associating these features with the
>> existence of any domain.
> 
> we don't have such uAPI or kAPI today.
> 
> the current behavior before your change is default off and then toggled
> along with attach/detach domain. as only one domain is allowed per
> RID it implies the capabilities are toggled along with DMA allow/block.
> 
> now you change it to a messy model:
> 
>    - default off when the device is probed
>    - turn on at the 1st domain attach and never turn off until release
>    - but iommu_enable_pci_caps() is still called at every domain attach
>      with band-aid to allow re-entrant
> 
> this isn't like a good cleanup...

Fair enough. We should not bury this behavior change in a cleanup
series. Okay! I will keep the previous behavior.

Best regards,
baolu