Received-SPF: pass (google.com: domain of linux-kernel+bounces-112081-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 22 Mar 2024 16:01:04 -0700
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Dimitri Sivanich <sivanich@hpe.com>
Cc: 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, linux-kernel@vger.kernel.org, Steve Wahl
 <steve.wahl@hpe.com>, Russ Anderson <russ.anderson@hpe.com>,
 jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH v2] iommu/vt-d: Allocate DMAR fault interrupts locally
Message-ID: <20240322160104.22af19bd@jacob-builder>
In-Reply-To: <Zfydpp2Hm+as16TY@hpe.com>
References: <Zfydpp2Hm+as16TY@hpe.com>
Organization: OTC
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi Dimitri,

On Thu, 21 Mar 2024 15:50:46 -0500, Dimitri Sivanich <sivanich@hpe.com>
wrote:

> 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
> allocated socket local.
> 
> Instead, have a cpu on each node do the vector allocation for the DMARs on
> that node.  The boot cpu still does the allocation for its node during its
> boot sequence.
> 
> Signed-off-by: Dimitri Sivanich <sivanich@hpe.com>
> ---
> 
> 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.
> 

I tested on a dual socket system (192 core) successfully with the following:
1. After boot DMAR-MSI spread from BSP to the first CPU of the second numa
node
2. Offline/Online all CPUs in the 2nd node

Code looks good to me.


Thanks,

Jacob

>  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(-)
> 
> 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;
>  }
>  
> -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
> index 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;
>  }
>  
> -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 = dmar_set_interrupt(iommu);
> +		int ret;
> +
> +		if (iommu->irq || iommu->node != cpu_to_node(cpu))
> +			continue;
> +
> +		ret = dmar_set_interrupt(iommu);
>  
>  		if (ret) {
>  			pr_err("DRHD %Lx: failed to enable fault,
> interrupt, ret %d\n", diff --git a/drivers/iommu/irq_remapping.c
> b/drivers/iommu/irq_remapping.c index ee59647c2050..2f7281ccc05f 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -151,7 +151,10 @@ int __init irq_remap_enable_fault_handling(void)
>  	if (!remap_ops->enable_faulting)
>  		return -ENODEV;
>  
> -	return remap_ops->enable_faulting();
> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "dmar:enable_fault_handling",
> +			  remap_ops->enable_faulting, NULL);
> +
> +	return remap_ops->enable_faulting(smp_processor_id());
>  }
>  
>  void panic_if_irq_remap(const char *msg)
> diff --git a/drivers/iommu/irq_remapping.h b/drivers/iommu/irq_remapping.h
> index 8c89cb947cdb..0d6f140b5e01 100644
> --- a/drivers/iommu/irq_remapping.h
> +++ b/drivers/iommu/irq_remapping.h
> @@ -41,7 +41,7 @@ struct irq_remap_ops {
>  	int  (*reenable)(int);
>  
>  	/* Enable fault handling */
> -	int  (*enable_faulting)(void);
> +	int  (*enable_faulting)(unsigned int);
>  };
>  
>  extern struct irq_remap_ops intel_irq_remap_ops;
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index e34b601b71fd..499bb2c63483 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -117,7 +117,7 @@ extern int dmar_remove_dev_scope(struct
> dmar_pci_notify_info *info, int count);
>  /* Intel IOMMU detection */
>  void detect_intel_iommu(void);
> -extern int enable_drhd_fault_handling(void);
> +extern int enable_drhd_fault_handling(unsigned int cpu);
>  extern int dmar_device_add(acpi_handle handle);
>  extern int dmar_device_remove(acpi_handle handle);
>  


Thanks,

Jacob