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

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]

Date: Tue, 23 Apr 2024 08:42:34 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
 Gunthorpe" <jgg@...pe.ca>
CC: "Zhang, Tina" <tina.zhang@...el.com>, "Liu, Yi L" <yi.l.liu@...el.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 02/12] iommu/vt-d: Add cache tag invalidation helpers

> From: Baolu Lu <baolu.lu@...ux.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.

> 
> 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?

>                  case CACHE_TAG_DEVTLB:
>                          info = dev_iommu_priv_get(tag->dev);
>                          sid = PCI_DEVID(info->bus, info->devfn);
> 
> > +		case CACHE_TAG_DEVTLB:
> > +			info = dev_iommu_priv_get(tag->dev);
> > +			sid = PCI_DEVID(info->bus, info->devfn);
> > +
> > +			if (tag->pasid == IOMMU_NO_PASID)
> > +				qi_flush_dev_iotlb(iommu, sid, info->pfsid,
> > +						   info->ats_qdep, addr,
> mask);
> > +			else
> > +				qi_flush_dev_iotlb_pasid(iommu, sid, info-
> >pfsid,
> > +							 tag->pasid, info-
> >ats_qdep,
> > +							 addr, mask);
> > +
> > +			quirk_extra_dev_tlb_flush(info, addr, mask, tag-
> >pasid, info->ats_qdep);
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&domain->cache_lock, flags);
> 
> Best regards,
> baolu