Received-SPF: pass (google.com: domain of linux-kernel+bounces-110929-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: "Zhang, Tina" <tina.zhang@intel.com>
To: Dimitri Sivanich <sivanich@hpe.com>, Thomas Gleixner <tglx@linutronix.de>,
	Joerg Roedel <joro@8bytes.org>, Suravee Suthikulpanit
	<suravee.suthikulpanit@amd.com>, Will Deacon <will@kernel.org>, Robin Murphy
	<robin.murphy@arm.com>, David Woodhouse <dwmw2@infradead.org>, Lu Baolu
	<baolu.lu@linux.intel.com>, Mark Rutland <mark.rutland@arm.com>, "Peter
 Zijlstra" <peterz@infradead.org>, Arnd Bergmann <arnd@arndb.de>, YueHaibing
	<yuehaibing@huawei.com>, "iommu@lists.linux.dev" <iommu@lists.linux.dev>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Steve Wahl
	<steve.wahl@hpe.com>, "Anderson, Russ" <russ.anderson@hpe.com>
Subject: RE: [PATCH v2] iommu/vt-d: Allocate DMAR fault interrupts locally
Thread-Topic: [PATCH v2] iommu/vt-d: Allocate DMAR fault interrupts locally
Thread-Index: AQHae9GquCYqZSMVD0a638cWhBmXNbFDLUYw
Date: Fri, 22 Mar 2024 04:41:01 +0000
Message-ID: <MW5PR11MB58815A2B9929D89F5B32371989312@MW5PR11MB5881.namprd11.prod.outlook.com>
References: <Zfydpp2Hm+as16TY@hpe.com>
In-Reply-To: <Zfydpp2Hm+as16TY@hpe.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?L6IXeyDPYVLEiUihMvkMpr56Yth5fxMphB4ZRHdP62SFdt308C5k0jXQrJ08?=
 =?us-ascii?Q?1IWa8R/VWUCVZXXcQz1Ncf4RmWg6F6hdMT5exnTv6MFOSLcdHnjEZsu9mj09?=
 =?us-ascii?Q?6+NULVUN72HrJQqVAFOWhxSGfW4bQ01Ys6U/WP24TwX4cnbVUlxYfmAP1At3?=
 =?us-ascii?Q?uYuwwIZnCXzVAqiSeex6qmiOV9O1o2ba4uT8Z+dLkWT+rW81NbwbFnA4hjBa?=
 =?us-ascii?Q?9ubuGP/bNzGHa0ULm2y+zcYv0LYerPrzI0AO6E5GqMtvJJ4LK7Eja+wW9msl?=
 =?us-ascii?Q?CynNmm7KzH1sZzC/4M5XA2D6lY/o28N6QrgKH/8gMVuh410VOa3vRyy/4HXq?=
 =?us-ascii?Q?TDoNH/qvjThbKt4Dv8RIg6AwIGGAsvklNCHARPOAVwL0LfCm9iygbJ7Xq+fV?=
 =?us-ascii?Q?tCT3kycCz1tXMET7V5Bq0Dppwuel2p2tXjVBmliVo/W6lqadKBCvxYJcgEfP?=
 =?us-ascii?Q?MEt49GD+GtbRREfStQQw1Y+xVFcAX5vHYUf3eG1/vAarydXQaa2PmpBPMdzz?=
 =?us-ascii?Q?s3bzuOxQt806VSQ4umuZrq5bSjdcPARa1Y4dubAtkBPpb0bE3atHzdhGb5EQ?=
 =?us-ascii?Q?2XzxMCISAuE9hW4nHARAtjyXJFURpudt9xvtrzcG1HFtevLVVtBNy5ZnVvhm?=
 =?us-ascii?Q?wAhl2y7x6MLztafRmMnb47j60U/lxZQM0wTrmFW62Z78qSZT0OOZmZtSjAfu?=
 =?us-ascii?Q?7eRc8wpQHTDXXjg1H4ru4WIlHPPo7HNXQ8cRtbbDRivxTD5YhL+cZGESzHEe?=
 =?us-ascii?Q?/UasET/lshAomQ6WMEJ3sXzlbQfVMWdrF53G8xg3KtYfGbkCmomhGHvieax8?=
 =?us-ascii?Q?nzkcCdbH3NdaeP1ISclAvZXkXBR8yhgTSmjOALjPz0a6VoCNVKFJ36Sd7SvE?=
 =?us-ascii?Q?usjIZtREH4sQ6De3pKY6dvUhRADU+tRQUvuLUP7u5zAYbVD6sHj5eFzFEvy9?=
 =?us-ascii?Q?iyewSEaAMwMrEq+4h4EGwDMRtEUy87KCxv97qHGb9IS67m4BnF/g/4pljOZK?=
 =?us-ascii?Q?aZwwJg7IA0SD9n70FN4WDBt/XRWmQr1DWRfR1OiuhJUHSPhMAKlEMwc40AWP?=
 =?us-ascii?Q?E8nNHuVTWeqT5pTfWV1XWRDFp4t6aMVrbwdKACycQF5VykgnkC/e+TabMBis?=
 =?us-ascii?Q?VlAxB6AZXEDgB24I3/tfTFSxqHGC8Qvt0gYQYZtHUoef2MNgOKEaTk2DPyf5?=
 =?us-ascii?Q?fxsXBuz6OILypRTfMfxxI5W5iQHeS5ufZGwH06Y5m93hNjrUR5NjrdTEQ9Av?=
 =?us-ascii?Q?dpUn6dqQMKN6LjrZ+/l0LyBTNswcIleLhaL/qf1l7OkrSvwRdDmQt7iPVh+d?=
 =?us-ascii?Q?Q1/Md/Z4vN1nTNjE/68tm1XAoJUhhf/oCO+t9LbmW5eVL1vu/WZHmGsS5+jw?=
 =?us-ascii?Q?yvIPqw0OkE00ZjbepyY7Bc84FwQfzypggNxi/xIUbvYWjKUtUp1REnkN6Yh0?=
 =?us-ascii?Q?pZLujFZhFgN5JNCbyQ5uY8tXqnRuRf+/eAY1MmxOyTcH0+5lXDbxpQmWSqY6?=
 =?us-ascii?Q?zkGXKKs5K8VBn8FN1zYm9UQUl+Va2jFgn92XxBgylTDCPFHn+wgUMKds+5us?=
 =?us-ascii?Q?cIFaKcfO01DzHt1PrGOtEzyoInWLphRmb09AWFzb?=
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: 60fde1df-931b-4c61-2177-08dc4a2a46ac
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Mar 2024 04:41:01.4395
 (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: Ee0LZD0lg0htaQdjjepmxdjun4pOqpYz8fvEbNJfIzvcVIiwFruwGLpXNldrJsnywYJdrfxl3hHIgCg4Pfmpcg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6531
X-OriginatorOrg: intel.com

Hi Dimitri,


> -----Original Message-----
> From: Dimitri Sivanich <sivanich@hpe.com>
> Sent: Friday, March 22, 2024 4:51 AM
> To: Thomas Gleixner <tglx@linutronix.de>; Joerg Roedel <joro@8bytes.org>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Will Deacon
> <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; David
> Woodhouse <dwmw2@infradead.org>; Lu Baolu <baolu.lu@linux.intel.com>;
> Mark Rutland <mark.rutland@arm.com>; Peter Zijlstra
> <peterz@infradead.org>; Arnd Bergmann <arnd@arndb.de>; YueHaibing
> <yuehaibing@huawei.com>; iommu@lists.linux.dev; Dimitri Sivanich
> <sivanich@hpe.com>
> Cc: linux-kernel@vger.kernel.org; Steve Wahl <steve.wahl@hpe.com>;
> Anderson, Russ <russ.anderson@hpe.com>
> Subject: [PATCH v2] iommu/vt-d: Allocate DMAR fault interrupts locally
>=20
> The Intel IOMMU code currently tries to allocate all DMAR fault interrupt
> vectors on the boot cpu.  On large systems with high DMAR counts this
> results in vector exhaustion, and most of the vectors are not initially a=
llocated
> socket local.
>=20
> Instead, have a cpu on each node do the vector allocation for the DMARs o=
n
> that node.  The boot cpu still does the allocation for its node during it=
s boot
> sequence.
>=20
> Signed-off-by: Dimitri Sivanich <sivanich@hpe.com>
> ---
>=20
> v2: per Thomas Gleixner, implement this from a DYN CPU hotplug state,
> though
>     this implementation runs in CPUHP_AP_ONLINE_DYN space rather than
>     CPUHP_BP_PREPARE_DYN space.
>=20
>  drivers/iommu/amd/amd_iommu.h | 2 +-
>  drivers/iommu/amd/init.c      | 2 +-
>  drivers/iommu/intel/dmar.c    | 9 +++++++--
>  drivers/iommu/irq_remapping.c | 5 ++++-  drivers/iommu/irq_remapping.h |
> 2 +-
>  include/linux/dmar.h          | 2 +-
>  6 files changed, 15 insertions(+), 7 deletions(-)
>=20
> diff --git a/drivers/iommu/amd/amd_iommu.h
> b/drivers/iommu/amd/amd_iommu.h index f482aab420f7..410c360e7e24
> 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -33,7 +33,7 @@ int amd_iommu_prepare(void);  int
> amd_iommu_enable(void);  void amd_iommu_disable(void);  int
> amd_iommu_reenable(int mode); -int amd_iommu_enable_faulting(void);
> +int amd_iommu_enable_faulting(unsigned int cpu);
>  extern int amd_iommu_guest_ir;
>  extern enum io_pgtable_fmt amd_iommu_pgtable;  extern int
> amd_iommu_gpt_level; diff --git a/drivers/iommu/amd/init.c
> b/drivers/iommu/amd/init.c index e7a44929f0da..4782f690ed97 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3389,7 +3389,7 @@ int amd_iommu_reenable(int mode)
>  	return 0;
>  }
>=20
> -int __init amd_iommu_enable_faulting(void)
> +int __init amd_iommu_enable_faulting(unsigned int cpu)
>  {
>  	/* We enable MSI later when PCI is initialized */
>  	return 0;
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c inde=
x
> 36d7427b1202..7644a42f283c 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -2122,7 +2122,7 @@ int dmar_set_interrupt(struct intel_iommu
> *iommu)
>  	return ret;
>  }
>=20
> -int __init enable_drhd_fault_handling(void)
> +int enable_drhd_fault_handling(unsigned int cpu)
>  {
>  	struct dmar_drhd_unit *drhd;
>  	struct intel_iommu *iommu;
> @@ -2132,7 +2132,12 @@ int __init enable_drhd_fault_handling(void)
>  	 */
>  	for_each_iommu(iommu, drhd) {
>  		u32 fault_status;
> -		int ret =3D dmar_set_interrupt(iommu);
> +		int ret;
> +
> +		if (iommu->irq || iommu->node !=3D cpu_to_node(cpu))
> +			continue;
If iommu->irq is set, current logic will clear any previous faults by acces=
sing DMAR_FSTS_REG. However, the code change in this patch seems missing it=
.

The current logic:
int dmar_set_interrupt(struct intel_iommu *iommu)
{
        int irq, ret;

        /*
         * Check if the fault interrupt is already initialized.
         */
        if (iommu->irq)
                return 0;
        ...

int __init enable_drhd_fault_handling(void)
{
	...
        for_each_iommu(iommu, drhd) {
                u32 fault_status;
                int ret =3D dmar_set_interrupt(iommu);

                if (ret) {
                        pr_err("DRHD %Lx: failed to enable fault, interrupt=
, ret %d\n",
                               (unsigned long long)drhd->reg_base_addr, ret=
);
                        return -1;
                }

                /*
                 * Clear any previous faults.
                 */
                dmar_fault(iommu->irq, iommu);
                fault_status =3D readl(iommu->reg + DMAR_FSTS_REG);
                writel(fault_status, iommu->reg + DMAR_FSTS_REG);
        }
	...

Regards,

-Tina