linux-kernel - RE: [PATCH v2] iommu/vt-d: calculate mask for non-aligned flushes

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: <BN9PR11MB52761E41CAD28CFCB935688C8C1A9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Fri, 25 Mar 2022 06:13:43 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     David Stevens <stevensd@...omium.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>
CC:     "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] iommu/vt-d: calculate mask for non-aligned flushes

> From: David Stevens
> Sent: Tuesday, March 22, 2022 2:36 PM
> 
> From: David Stevens <stevensd@...omium.org>
> 
> Calculate the appropriate mask for non-size-aligned page selective
> invalidation. Since psi uses the mask value to mask out the lower order
> bits of the target address, properly flushing the iotlb requires using a
> mask value such that [pfn, pfn+pages) all lie within the flushed
> size-aligned region.  This is not normally an issue because iova.c
> always allocates iovas that are aligned to their size. However, iovas
> which come from other sources (e.g. userspace via VFIO) may not be
> aligned.
> 
> Signed-off-by: David Stevens <stevensd@...omium.org>
> ---
> v1 -> v2:
>  - Calculate an appropriate mask for non-size-aligned iovas instead
>    of falling back to domain selective flush.
> 
>  drivers/iommu/intel/iommu.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 5b196cfe9ed2..ab2273300346 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1717,7 +1717,8 @@ static void iommu_flush_iotlb_psi(struct
> intel_iommu *iommu,
>  				  unsigned long pfn, unsigned int pages,
>  				  int ih, int map)
>  {
> -	unsigned int mask = ilog2(__roundup_pow_of_two(pages));
> +	unsigned int aligned_pages = __roundup_pow_of_two(pages);
> +	unsigned int mask = ilog2(aligned_pages);
>  	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
>  	u16 did = domain->iommu_did[iommu->seq_id];
> 
> @@ -1729,10 +1730,30 @@ static void iommu_flush_iotlb_psi(struct
> intel_iommu *iommu,
>  	if (domain_use_first_level(domain)) {
>  		domain_flush_piotlb(iommu, domain, addr, pages, ih);
>  	} else {
> +		unsigned long bitmask = aligned_pages - 1;
> +
> +		/*
> +		 * PSI masks the low order bits of the base address. If the
> +		 * address isn't aligned to the mask, then compute a mask
> value
> +		 * needed to ensure the target range is flushed.
> +		 */
> +		if (unlikely(bitmask & pfn)) {
> +			unsigned long end_pfn = pfn + pages - 1, shared_bits;
> +
> +			/*
> +			 * Since end_pfn <= pfn + bitmask, the only way bits
> +			 * higher than bitmask can differ in pfn and end_pfn
> is
> +			 * by carrying. This means after masking out bitmask,
> +			 * high bits starting with the first set bit in
> +			 * shared_bits are all equal in both pfn and end_pfn.
> +			 */
> +			shared_bits = ~(pfn ^ end_pfn) & ~bitmask;
> +			mask = shared_bits ? __ffs(shared_bits) :
> BITS_PER_LONG;
> +		}

While it works I wonder whether below is simpler regarding to readability:

	} else {
+		/*
+		 * PSI masks the low order bits of the base address. If the
+		 * address isn't aligned to the mask and [pfn, pfn+pages)
+		 * don't all lie within the flushed size-aligned region,
+		 * simply increment the mask by one to cover the trailing pages.
+		 */
+		if (unlikely((pfn & (aligned_pages - 1)) &&
+			     (pfn + pages - 1 >= ALIGN(pfn, aligned_pages))))
+			mask++;

Thanks
Kevin