linux-kernel - Re: [PATCH v3 4/6] iommu: Factor iommu_iotlb_gather_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D76DA59C-023F-43D1-B4ED-BFA65D9D064F@gmail.com>
Date:   Fri, 11 Jun 2021 09:50:31 -0700
From:   Nadav Amit <nadav.amit@...il.com>
To:     Will Deacon <will@...nel.org>
Cc:     Joerg Roedel <joro@...tes.org>, Jiajun Cao <caojiajun@...are.com>,
        Robin Murphy <robin.murphy@....com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/6] iommu: Factor iommu_iotlb_gather_is_disjoint() out



> On Jun 11, 2021, at 6:57 AM, Will Deacon <will@...nel.org> wrote:
> 
> On Mon, Jun 07, 2021 at 11:25:39AM -0700, Nadav Amit wrote:
>> From: Nadav Amit <namit@...are.com>
>> 
>> Refactor iommu_iotlb_gather_add_page() and factor out the logic that
>> detects whether IOTLB gather range and a new range are disjoint. To be
>> used by the next patch that implements different gathering logic for
>> AMD.
>> 
>> Cc: Joerg Roedel <joro@...tes.org>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Jiajun Cao <caojiajun@...are.com>
>> Cc: Robin Murphy <robin.murphy@....com>
>> Cc: Lu Baolu <baolu.lu@...ux.intel.com>
>> Cc: iommu@...ts.linux-foundation.org
>> Cc: linux-kernel@...r.kernel.org>
>> Signed-off-by: Nadav Amit <namit@...are.com>
>> ---
>> include/linux/iommu.h | 41 +++++++++++++++++++++++++++++++++--------
>> 1 file changed, 33 insertions(+), 8 deletions(-)
> 
> [...]
> 
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index f254c62f3720..b5a2bfc68fb0 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -497,6 +497,28 @@ static inline void iommu_iotlb_sync(struct iommu_domain *domain,
>> 	iommu_iotlb_gather_init(iotlb_gather);
>> }
>> 
>> +/**
>> + * iommu_iotlb_gather_is_disjoint - Checks whether a new range is disjoint
>> + *
>> + * @gather: TLB gather data
>> + * @iova: start of page to invalidate
>> + * @size: size of page to invalidate
>> + *
>> + * Helper for IOMMU drivers to check whether a new range is and the gathered
>> + * range are disjoint.
> 
> I can't quite parse this. Delete the "is"?

Indeed. Will do (I mean I will do ;-) )

> 
>>    For many IOMMUs, flushing the IOMMU in this case is
>> + * better than merging the two, which might lead to unnecessary invalidations.
>> + */
>> +static inline
>> +bool iommu_iotlb_gather_is_disjoint(struct iommu_iotlb_gather *gather,
>> +				    unsigned long iova, size_t size)
>> +{
>> +	unsigned long start = iova, end = start + size - 1;
>> +
>> +	return gather->end != 0 &&
>> +		(end + 1 < gather->start || start > gather->end + 1);
>> +}
>> +
>> +
>> /**
>>  * iommu_iotlb_gather_add_range - Gather for address-based TLB invalidation
>>  * @gather: TLB gather data
>> @@ -533,20 +555,16 @@ static inline void iommu_iotlb_gather_add_page(struct iommu_domain *domain,
>> 					       struct iommu_iotlb_gather *gather,
>> 					       unsigned long iova, size_t size)
>> {
>> -	unsigned long start = iova, end = start + size - 1;
>> -
>> 	/*
>> 	 * If the new page is disjoint from the current range or is mapped at
>> 	 * a different granularity, then sync the TLB so that the gather
>> 	 * structure can be rewritten.
>> 	 */
>> -	if (gather->pgsize != size ||
>> -	    end + 1 < gather->start || start > gather->end + 1) {
>> -		if (gather->pgsize)
>> -			iommu_iotlb_sync(domain, gather);
>> -		gather->pgsize = size;
>> -	}
>> +	if ((gather->pgsize && gather->pgsize != size) ||
>> +	    iommu_iotlb_gather_is_disjoint(gather, iova, size))
>> +		iommu_iotlb_sync(domain, gather);
>> 
>> +	gather->pgsize = size;
> 
> Why have you made this unconditional? I think it's ok, but just not sure
> if it's necessary or not.

In regard to gather->pgsize, this function had (and has) an
invariant, in which gather->pgsize always represents the flushing
granularity of its range. Arguably, “size" should never be
zero, but lets assume for the matter of discussion that it might.

If “size” equals to “gather->pgsize”, then the assignment in
question has no impact.

Otherwise, if “size” is non-zero, then iommu_iotlb_sync() would
initialize the size and range (see iommu_iotlb_gather_init()),
and the invariant is kept.

Otherwise, “size” is zero, and “gather” already holds a range,
so gather->pgsize is non-zero and
(gather->pgsize && gather->pgsize != size) is true. Therefore,
again, iommu_iotlb_sync() would be called and initialize the
size.

I think that this change makes the code much simpler to read.
It probably has no performance impact as “gather” is probably
cached and anyhow accessed shortly after.

If anything, I can add a VM_BUG_ON() to check “size” is
non-zero, although this code seems correct regardless of that.


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)