Received-SPF: pass (google.com: domain of linux-kernel+bounces-136335-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jacob Pan <jacob.jun.pan@linux.intel.com>
CC: "sivanich@hpe.com" <sivanich@hpe.com>, Thomas Gleixner
	<tglx@linutronix.de>, LKML <linux-kernel@vger.kernel.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>, Lu Baolu
	<baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, "Liu, Yi L"
	<yi.l.liu@intel.com>, "steve.wahl@hpe.com" <steve.wahl@hpe.com>, "Anderson,
 Russ" <russ.anderson@hpe.com>, Peter Zijlstra <peterz@infradead.org>, "Will
 Deacon" <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>
Subject: RE: [PATCH 2/2] iommu/vt-d: Share DMAR fault IRQ to prevent vector
 exhaustion
Thread-Topic: [PATCH 2/2] iommu/vt-d: Share DMAR fault IRQ to prevent vector
 exhaustion
Thread-Index: AQHahiB7n7Y2ch8WjEeOJ91b45rj4rFeEdyggAB/KgCAABntAIAA3vIA
Date: Tue, 9 Apr 2024 07:07:22 +0000
Message-ID: <BN9PR11MB52763561BFD87DE35FF4A0048C072@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240403234548.989061-1-jacob.jun.pan@linux.intel.com>
	<20240403234548.989061-2-jacob.jun.pan@linux.intel.com>
	<BN9PR11MB527677A69A2004A951165CC38C002@BN9PR11MB5276.namprd11.prod.outlook.com>
	<20240408090556.6165e603@jacob-builder>
 <20240408103844.1b567bce@jacob-builder>
In-Reply-To: <20240408103844.1b567bce@jacob-builder>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?UKU5K8jWnipABayClJP5wzN4CziX4PlOOEr1zRt9WeWNKlAPk8ybcBQ3lv++?=
 =?us-ascii?Q?aMf9tU/D580l5pX4YwHzKFaakFPy01hA70cCfqpgd2lKWdQbdrL9Zt8BA+oK?=
 =?us-ascii?Q?/WMwZoe3zh01v4l74FoSHji1UxOzIozMNBwiQeNtbrnmbv9zX/ZfjZiQgcTC?=
 =?us-ascii?Q?7BP0f3us2MnOL0E2OsmoE0VnScB4fG/pH3IqpiR96HpV+7+tQSnIHbNVKFqH?=
 =?us-ascii?Q?3pjx8M2Dsn7ojLci3lFMvtGwSbRMdibuND41KOUv/ZVyZaVQX+46xZ2rawYQ?=
 =?us-ascii?Q?/O0/m0i4zwCPComBANX9Dhlq3EMqjXIFeHkvLtjNAMVArX/AvAd7pPuByGJP?=
 =?us-ascii?Q?Dd63EGaxbJRBt+F1V5IvPg9RU9BQ5EoDHZzK0sPjj/nXwOGRl3cLGkunhHPV?=
 =?us-ascii?Q?sSoaKdenjDYv8KkQ8wbItZwDV6pUawMGcV0mTfUlqTR5+yxKtWwm6WGWF5nE?=
 =?us-ascii?Q?yQ+WU8BIPOasTNJ3Co30tat8wHq7ees13LFh7/TFfFnLc4kM51nOSPvMDyRj?=
 =?us-ascii?Q?Oz4cBV+zWcvXOY7laTZ1cw8PRxgWim9LwwGDUnAw+61inXMJcPKJFwqi/Dsp?=
 =?us-ascii?Q?iO5a/5lRCbi0PwDSuk7KkT0f/VxnPTJF5fH5hYG9sEpZgpgg32Dg+nso/AvD?=
 =?us-ascii?Q?/Qv9EUiKbozxilh59ellzeUvZ9txFYKDVidKaivlcG9D4zRc9O1Lih7zrgoe?=
 =?us-ascii?Q?dFWSMuh7DmmYyJIASSRkYDTJm2/heT67EGeh06RUz9y0kSpuVUoMfyixnbFo?=
 =?us-ascii?Q?qgNJPulSS5ZgZslIhC0yFMh2P7zEVSYpfVIUQUw4l3rLBuqjuYcm9lghfHwv?=
 =?us-ascii?Q?atM5hRptH/TKlufUBiobOPMRO4WLcupshEfSzSDV92HZLXwiMgKizOjKGJKW?=
 =?us-ascii?Q?C5qBNC/NbJYDwTYGCecEAdD5wPDj0mTCn59/aWw6umomdsVSNx8puxvZ1m1p?=
 =?us-ascii?Q?ZSJ8X6Q8LFHq8TjQPIiOsSzDUnUeqrII68s1hgQPT8Ye8MC/tMrirZjaQ+2W?=
 =?us-ascii?Q?CFiHrBHFiT73a0zUrp2KtOYZA/JEA7nAy8nUUEh18i9YXuauQwXZGEerbCFP?=
 =?us-ascii?Q?nT8MxrbAoYGbHMCB21XZcZtbQdGZ7G6OEBTPHnoIcEidr18RVkIhnu0GZB/b?=
 =?us-ascii?Q?uqE4f/thAIMAUzd+CRhigZ3gPUELrgVJpmmEkXMJUlgicIHADwlJDQaOvump?=
 =?us-ascii?Q?XY3qz7XZJjpImSypfZ3025Q+V3VccBtctHipCab/29f/PATn7FMkXzvHPeCc?=
 =?us-ascii?Q?eFydDLudKfGD18+SxWx+cLCNLjdjuRykVmvoYGblT0TgQnYCGMdx1afR1rAE?=
 =?us-ascii?Q?pyz730NUe27sf/r2eNrdWstkxeD3E+nDbOw+uQGpruvQ6Jy8xloj15Cy9HfT?=
 =?us-ascii?Q?OrbsolpGzEsW7EonuPo+XX2IJpSXS4ggXFIwz0gTfvr4zPlsHXnenQIQiEu6?=
 =?us-ascii?Q?EId6xiiTk8EjpryPDduBJn73C8fB86N4pRIVyR2iNd0gJG8D9e34+ufpANkh?=
 =?us-ascii?Q?0ZovnWwAZEYKSmNbPLCnH2AXREP4Y6wFbd9Su/dawOMOUPEqjxMP6CY49jYJ?=
 =?us-ascii?Q?b8p7XCA//DueFiw3KMHslgKO5CV4jv60e19+N/8n?=
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: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 88c29516-b4b3-484d-1fb8-08dc5863b444
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Apr 2024 07:07:22.8857
 (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: NJXhFT0aomHbnaqQWCK1+92khZIzD8YlgaT29Q5CxmmooBUaUuBuqwt62my8dhPxtzsv+qiQP8MZCgUX2uAu2w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7848
X-OriginatorOrg: intel.com

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Tuesday, April 9, 2024 1:39 AM
>=20
> Hi Jacob,
>=20
> On Mon, 8 Apr 2024 09:05:56 -0700, Jacob Pan
> <jacob.jun.pan@linux.intel.com> wrote:
>=20
> > Hi Kevin,
> >
> > On Mon, 8 Apr 2024 08:48:54 +0000, "Tian, Kevin" <kevin.tian@intel.com>
> > wrote:
> >
> > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > > Sent: Thursday, April 4, 2024 7:46 AM
> > > >
> > > > @@ -1182,7 +1182,6 @@ static void free_iommu(struct intel_iommu
> > > > *iommu)
> > > >  			iommu->pr_irq =3D 0;
> > > >  		}
> > > >  		free_irq(iommu->fault_irq, iommu);
> > > > -		dmar_free_hwirq(iommu->fault_irq);
> > >
> > > You still want to free the vector for the iommu which first gets the
> > > vector allocated.
> > >
> > I think we always want to keep this vector since the system always need=
s
> > one vector to share. We will never offline all the IOMMUs, right?

Not about offline. Just about the common rule of cleaning up a resource
when all of its users are destroyed.

> > > > +
> > > > +	/*
> > > > +	 * Only the owner IOMMU of the shared IRQ has its fault event
> > > > +	 * interrupt unmasked after request_irq(), the rest are
> > > > explicitly
> > > > +	 * unmasked.
> > > > +	 */
> > > > +	if (!(iommu->flags & VTD_FLAG_FAULT_IRQ_OWNER))
> > > > +		dmar_fault_irq_unmask(iommu);
> > > > +
> > >
> > > em there is a problem in dmar_msi_mask() and dmar_msi_mask()
> > > which only touches the owner IOMMU. With this shared vector
> > > approach we should mask/unmask all IOMMU's together.
> > I thought about this as well, in addition to fault_irq,
> > dmar_msi_mask/unmask() are used for other DMAR irqs, page request and
> > perfmon. So we need a special case for fault_irq there, it is not prett=
y.

yes, that is the part which I don't really like.

> >
> > I added a special case here in this patch, thinking we never mask the
> > fault_irq since we need to cover the lifetime of the system. I have loo=
ked
> > at:
> > 1.IOMMU suspend/resume, no mask/unmask
> Actually, we do call mask/unmask in suspend/unmask noirq phase.
> And DMAR-MSI chip has IRQCHIP_SKIP_SET_WAKE flag.
>=20
> So you are right, I am missing this case where non-owner IOMMU's fault_ir=
qs
> are not masked/unmasked.
>=20

and it's not good to code a mask/unmask callback upon fixed assumptions
on when irq core may call mask/unmask as the latter part can change=20
w/o noting the broken assumption in such callback.