Re: [PATCH v3 02/12] iommu/vt-d: Add cache tag invalidation helpers

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

On 4/23/24 4:42 PM, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Monday, April 22, 2024 1:30 PM
>>
>> On 4/16/24 4:06 PM, Lu Baolu wrote:
>>> +		case CACHE_TAG_NESTING_DEVTLB:
>>> +			/*
>>> +			 * Address translation cache in device side caches the
>>> +			 * result of nested translation. There is no easy way
>>> +			 * to identify the exact set of nested translations
>>> +			 * affected by a change in S2. So just flush the entire
>>> +			 * device cache.
>>> +			 */
>>> +			addr = 0;
>>> +			mask = MAX_AGAW_PFN_WIDTH;
>>> +			fallthrough;
>> I realized that the logic above is not right. Setting both @addr and
>> @mask to 0 doesn't means flush all caches on the device. I will change
>> it like below:
> I didn't get. Above code doesn't set @mask to 0.

Oh!? I have no idea why I read that as "mask = 0" now. Perhaps my brain
was on vacation earlier. :-)

> 
>> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
>> index e8418cdd8331..18debb82272a 100644
>> --- a/drivers/iommu/intel/cache.c
>> +++ b/drivers/iommu/intel/cache.c
>> @@ -302,9 +302,14 @@ void cache_tag_flush_range(struct dmar_domain
>> *domain, unsigned long start,
>>                            * affected by a change in S2. So just flush
>> the entire
>>                            * device cache.
>>                            */
>> -                       addr = 0;
>> -                       mask = MAX_AGAW_PFN_WIDTH;
>> -                       fallthrough;
>> +                       info = dev_iommu_priv_get(tag->dev);
>> +                       sid = PCI_DEVID(info->bus, info->devfn);
>> +
>> +                       qi_flush_dev_iotlb(iommu, sid, info->pfsid,
>> info->ats_qdep,
>> +                                          0, MAX_AGAW_PFN_WIDTH);
>> +                       quirk_extra_dev_tlb_flush(info, 0,
>> MAX_AGAW_PFN_WIDTH,
>> +                                                 IOMMU_NO_PASID,
>> info->ats_qdep);
>> +                       break;
> and I didn't get this change. It goes backward by ignoring tag->pasid.
> 
> what's the exact problem of the fallthrough logic in original code?

Sorry! Please ignore this.

Best regards,
baolu