Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush

[Yi Liu]

On 2024/4/16 11:00, Baolu Lu wrote:
> On 4/15/24 2:56 PM, Yi Liu wrote:
> > On 2024/4/15 13:00, Baolu Lu wrote:
> > > On 4/15/24 10:43 AM, Yi Liu wrote:
> > > > On 2024/4/15 10:22, Zhang, Tina wrote:
> > > > > > -----Original Message-----
> > > > > > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > > > Sent: Monday, April 15, 2024 9:39 AM
> > > > > > To: iommu@xxxxxxxxxxxxxxx
> > > > > > Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; Liu, Yi L 
> > > > > > <yi.l.liu@xxxxxxxxx>; Jacob
> > > > > > Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; 
> > > > > > Will
> > > > > > Deacon <will@xxxxxxxxxx>; Robin Murphy <robin.murphy@xxxxxxx>; linux-
> > > > > > kernel@xxxxxxxxxxxxxxx; Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > > > > > Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
> > > > > > device TLB flush
> > > > > >
> > > > > > The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
> > > > > > implementation caches not-present or erroneous translation-structure 
> > > > > > entries
> > > > > > except for the first-stage translation. The caching mode is 
> > > > > > irrelevant to the
> > > > > > device TLB, therefore there is no need to check it before a device TLB
> > > > > > invalidation operation.
> > > > > >
> > > > > > Remove two caching mode checks before device TLB invalidation in the 
> > > > > > driver.
> > > > > > The removal of these checks doesn't change the driver's behavior in 
> > > > > > critical
> > > > > > map/unmap paths. Hence, there is no functionality or performance impact,
> > > > > > especially since commit <29b32839725f> ("iommu/vt-d:
> > > > > > Do not use flush-queue when caching-mode is on") has already disabled
> > > > > > flush-queue for caching mode. Therefore, caching mode will never call
> > > > > > intel_flush_iotlb_all().
> > > > > The current logic is if the caching mode is being used and a domain 
> > > > > isn't using first level I/O page table, then flush-queue won't be 
> > > > > used. Otherwise, the flush-queue can be enabled.
> > > > > See https://github.com/torvalds/linux/commit/257ec29074
> > > > >
> > > > > In other words, if the caching mode is being used and a domain is 
> > > > > using first level I/O page table, the flush-queue can be used for this 
> > > > > domain to flush iotlb. Could the code change in this patch bring any 
> > > > > performance impact to this case?
> > > >
> > > > This seems to have performance deduction in the nested translation case.
> > > > The iommufd nested support bas been merged in 6.8, while the Qemu side
> > > > is wip. So this performance deduction does not happen until Qemu is
> > > > done. Should this also be considered as a performance regression? TBH.
> > >
> > > Caching mode is irrelevant to first-stage and nesting translations. If
> > > the QEMU implementation still relies on caching mode for nesting
> > > support, it's already broken.
> >
> > For first-stage, yes, caching mode is irrelevant. But for nesting, still
> > be relevant. Because it relies on caching mode to capture the mofifications
> > of the pasid entry. Without caching mode, vIOMMU device model within QEMU 
> > will miss the newly setup pasid entries, hence no way to setup nested
> > translation at all.
>
> Okay, it's reasonable that nesting translation still uses second-stage.

the key point is not nesting uses second-stage, it is because of the guest
pasid entry capturing :)

> But the removed checks in this patch don't impact the pasid table change
> capture, right?

yes. The removal in this patch is irrelevant to it.

So Tina's concern makes sense. But I don't know if it should be considered
as a performance regression since nesting is not really fully ready (QEMU
side is not there yet).

BTW. I'm not sure if you have considered to correct the other caching mode
checks before dev-TLB invalidation. e.g. the CM check in 
drivers/iommu/intel/pasid.c.

--
Regards,
Yi Liu