Received-SPF: pass (google.com: domain of linux-kernel+bounces-139548-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Zhang, Tina" <tina.zhang@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>, Lu Baolu <baolu.lu@linux.intel.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>
CC: "Liu, Yi L" <yi.l.liu@intel.com>, Joerg Roedel <joro@8bytes.org>, "Will
 Deacon" <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/2] iommu/vt-d: Remove caching mode check before devtlb
 flush
Thread-Topic: [PATCH 2/2] iommu/vt-d: Remove caching mode check before devtlb
 flush
Thread-Index: AQHaiPoE+7Nf+/opPEuKjHtgvnvXxLFfjZQAgAKh7bA=
Date: Wed, 10 Apr 2024 23:49:01 +0000
Message-ID: <MW5PR11MB5881E15977B2987AE32510B289062@MW5PR11MB5881.namprd11.prod.outlook.com>
References: <20240407144232.190355-1-baolu.lu@linux.intel.com>
 <20240407144232.190355-2-baolu.lu@linux.intel.com>
 <BN9PR11MB52764F42C20B6B18DDE7E66B8C072@BN9PR11MB5276.namprd11.prod.outlook.com>
In-Reply-To: <BN9PR11MB52764F42C20B6B18DDE7E66B8C072@BN9PR11MB5276.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cYxmSKQuIK/UIur7e09ITmcn6rbAaUvZQ5LijVvy5y39uwMxBBeQo9X9wc3Z?=
 =?us-ascii?Q?nNjDhmZ5qkYzXWGHmnjOy3Xg+wF2o8E6rA8SMvwK1jp3b5hOBqULVzj8aiyC?=
 =?us-ascii?Q?Zfc3KtWyc/g1Fh11Xf3Q561zHOiC15RiLBMB7kmY43pZvhzgMaWnGgnIQqaR?=
 =?us-ascii?Q?X8KtX9SWdZ36G6ojJ7CuTW0TKaGvY2cFuerqKdwxgBcKsi1yqlE8qgfsbUSB?=
 =?us-ascii?Q?KhwueYnD++ye1G8JzkDnTOkug7l3nZynO+wveXMPPZb3+MwIVQ4nuEXnMkpC?=
 =?us-ascii?Q?CYGRf/eS/2PNRCmi/9JuSedj9sRLfO1xYZpGVjJgQLSAs6ui2AJo+KSfJiam?=
 =?us-ascii?Q?zeurHzZilLtU/AO9eFEw+aUh07+bsZ3MnL1yhhdsMifQBTxc5EL8pAQulYrs?=
 =?us-ascii?Q?nI02JygKv1YH4YIWL3X+FGikozr+ANn87Ga9ceNRUeAkNRW3YKJqqOahXjPI?=
 =?us-ascii?Q?Wg00cWVvD5WFlr5lXnRVVYHYjML/1y4oZ+XHEdlkQ7lC7yuAFjzJVuYbcqBJ?=
 =?us-ascii?Q?zpVpmGvPH43BMUnWJZe2c0NGf2mfCYCmScLTmwVRQaFIjICCzlFsMqocx6hf?=
 =?us-ascii?Q?ODrhaLNfHbGs66gLdgSur8CcNIQhiSbk2m82btmJDlRXe7Zas6GmGUK3+D4W?=
 =?us-ascii?Q?WJv0OHzbmpvYBTnThJUalyxaVa4Rsqxd8z8JP8UVXy4/45vMwH+QBD6RWYG+?=
 =?us-ascii?Q?GYBz7eDK+wnTJXcvB7mJkGKtWeAm93zBTx4I2KTroLjZuMxoNdJSZezPcPjp?=
 =?us-ascii?Q?yqzTdOfGgWBXM/EmBBtuNJc2bBUlzPOCtecw0ucY+erJRLLgxER9ERbjG8PA?=
 =?us-ascii?Q?ibinpPmVyBxcZlmO9qQ+PbIqq0eIRnoO7SjELQrsezXdjUCBJ5vQW8JlemzI?=
 =?us-ascii?Q?helfzN21Ag6pJgkpcIgOfX8TYRqa2lcGTxF5DCgba2+By6t/M39d+WqGFFyJ?=
 =?us-ascii?Q?lgtvvjTreKZZS1F/uZxA4G1xBcSfVMXPsdUxQuMEohaHJ6dP45pTdk9w2OIq?=
 =?us-ascii?Q?97Z04VkLXuz0N05uXcrtj2LjhQsLCXYsyi85uWL0PZU562dzKn36h53XpDRl?=
 =?us-ascii?Q?CrPv09DvHlVAIkrXLe6dKzxmFsGsF46TqPl2ocJLKr3peUxzsMnxTIqnoV5F?=
 =?us-ascii?Q?+ib8PjtarlrRajgj12T6fRhLLSXwnKHs8Athx/UTNFscNM0sX23WFH1V9bq5?=
 =?us-ascii?Q?K7LGilVDB0gOAXRv2/Bh8rfVOuz6I9tK5af08jpTZJhRHoANbHJ4PwmSkIYt?=
 =?us-ascii?Q?URneJTGafhNV+c3CQoCVoKL/tT2g9R3xfq3qct0sDnFYcLn98vrZLpdN5BUd?=
 =?us-ascii?Q?2dw2VeeMmQjkEVOZvGoyeztICsyWLytsBe+gGZZvFf0Nyaf+NSThNv6DKDGk?=
 =?us-ascii?Q?TNRgAJBsoHtRPX74LkTG2IDs51SE9OYVHx9+QgHd/ncpWkde25vV4fsMwehk?=
 =?us-ascii?Q?y7BqPS6dK9jF5eDq+LUTiGoOSPXbImQWiq/aJLJ/uSDGVxaRQcpANBYkIvx6?=
 =?us-ascii?Q?r1zVynqOGLYhPvD0cPrZ7Z9Hjox7eNdA/RNq4bckRil1ZcBzfq22J8z2Cmel?=
 =?us-ascii?Q?Ws/3z7RPuyvX7K1pjgTva1TqFRMmZnJG1r7jt31O?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW5PR11MB5881.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcbac058-0e04-4a44-7d76-08dc59b8cc30
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Apr 2024 23:49:01.3950
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: efxf4I7+kByok8EQo8YJ8fhknZxC9fd5HeZJrn5o+GLJYPacV+gfyXM+XBu931yH6POFxKM4QZ4lcGj+n4kk5w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5109
X-OriginatorOrg: intel.com

Hi,

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Tuesday, April 9, 2024 3:30 PM
> To: Lu Baolu <baolu.lu@linux.intel.com>; iommu@lists.linux.dev
> Cc: Liu, Yi L <yi.l.liu@intel.com>; Joerg Roedel <joro@8bytes.org>; Will
> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 2/2] iommu/vt-d: Remove caching mode check before
> devtlb flush
>=20
> > From: Lu Baolu <baolu.lu@linux.intel.com>
> > Sent: Sunday, April 7, 2024 10:43 PM
> >
> > The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
> > implementation caches not-present or erroneous translation-structure
> > entries except the first-stage translation. The caching mode is
> > unrelated to the device TLB , therefore there is no need to check it
> > before a device TLB invalidation operation.
> >
> > Before the scalable mode is introduced, caching mode is treated as an
> > indication that the driver is running in a VM guest. This is just a
> > software contract as shadow page table is the only way to implement a
> > virtual IOMMU. But the VT-d spec doesn't state this anywhere. After
> > the scalable mode is introduced, this doesn't stand for anymore, as
> > caching mode is not relevant for the first-stage translation. A
> > virtual IOMMU implementation is free to support first-stage
> > translation only with caching mode cleared.
>=20
> I didn't get the connection to the scalable mode.
>=20
> if required we can still use caching mode to imply running as a guest.
> Just need to make sure its implementation conforming to the VT-d spec.
>=20
> >
> > Remove the caching mode check before device TLB invalidation to ensure
> > compatibility with the scalable mode use cases.
> >
> > Fixes: 792fb43ce2c9 ("iommu/vt-d: Enable Intel IOMMU scalable mode by
> > default")
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > ---
> >  drivers/iommu/intel/iommu.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 493b6a600394..681789b1258d 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1501,7 +1501,7 @@ static void iommu_flush_iotlb_psi(struct
> > intel_iommu *iommu,
> >  	else
> >  		__iommu_flush_iotlb_psi(iommu, did, pfn, pages, ih);
> >
> > -	if (!cap_caching_mode(iommu->cap) && !map)
> > +	if (!map)
> >  		iommu_flush_dev_iotlb(domain, addr, mask);
>=20
> as commented earlier better squash this in patch1.
>=20
> >  }
> >
> > @@ -1575,8 +1575,7 @@ static void intel_flush_iotlb_all(struct
> > iommu_domain *domain)
> >  			iommu->flush.flush_iotlb(iommu, did, 0, 0,
> >  						 DMA_TLB_DSI_FLUSH);
> >
> > -		if (!cap_caching_mode(iommu->cap))
> > -			iommu_flush_dev_iotlb(dmar_domain, 0,
> > MAX_AGAW_PFN_WIDTH);
> > +		iommu_flush_dev_iotlb(dmar_domain, 0,
> > MAX_AGAW_PFN_WIDTH);
> >  	}
> >
>=20
> I'm hesitating to agree with this change. Strictly speaking it's correct.
> but w/o supporting batch invalidation this implies performance drop on
> viommu due to more VM-exits and there may incur user complaints when
> their VMs upgrade to a newer kernel version.
>=20
> So it'd be better to keep this behavior and fix it together with batch
> invalidation support. Anyway none of the viommu implementations today
> (either shadow or nested translation) relies on the correct devtlb behavi=
or
> from the guest otherwise it's already broken.
How about we split this change into a patch. I'm working on the batch inval=
idation patch-set now and I'm happy to include this code change into the ba=
tch invalidation series.

Regards,
-Tina