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

[Baolu Lu]

On 2022/11/11 16:16, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Friday, November 11, 2022 2:59 PM
> >
> > On 2022/11/11 11:45, Tian, Kevin wrote:
> > > > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > 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