Received-SPF: pass (google.com: domain of linux-kernel+bounces-87630-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Thomas Gleixner <tglx@linutronix.de>
To: Dimitri Sivanich <sivanich@hpe.com>, Dimitri Sivanich
 <sivanich@hpe.com>, David Woodhouse <dwmw2@infradead.org>, Lu Baolu
 <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, Will Deacon
 <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
 iommu@lists.linux.dev
Cc: linux-kernel@vger.kernel.org, Steve Wahl <steve.wahl@hpe.com>, Russ
 Anderson <russ.anderson@hpe.com>
Subject: Re: [PATCH] Allocate DMAR fault interrupts locally
In-Reply-To: <ZeDj/LK56borSxO4@hpe.com>
References: <ZeDj/LK56borSxO4@hpe.com>
Date: Thu, 29 Feb 2024 23:18:37 +0100
Message-ID: <87plwe7w3m.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Dimitri!

On Thu, Feb 29 2024 at 14:07, Dimitri Sivanich wrote:

The subject lacks a subsystem prefix. You're doing this for how many
decades now?

> The Intel IOMMU code currently tries to allocate all DMAR fault interrupt
>  
> +#ifdef CONFIG_X86_LOCAL_APIC

I seriously doubt that this code can ever be compiled w/o X86_LOCAL_APIC:

obj-$(CONFIG_DMAR_TABLE) += dmar.o

config DMAR_TABLE
        bool

config INTEL_IOMMU
        depends on PCI_MSI && ACPI && X86
        select DMAR_TABLE

config IRQ_REMAP
        depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI
        select DMAR_TABLE

config X86_LOCAL_APIC
        def_bool y
        depends on X86_64 || SMP || X86_32_NON_STANDARD || X86_UP_APIC || PCI_MSI

What are you trying to achieve here other than #ifdef voodoo?

> +static void __init irq_remap_enable_fault_handling_thr(struct work_struct *work)
> +{
> +	irq_remap_enable_fault_handling();

because if INTEL_IOMMU=y and IRQ_REMAP=n then X86_LOCAL_APIC=y and this
muck gets invoked for nothing. 'git grep irq_remap_enable_fault_handling
include/' might give you a hint.

> +}
> +
> +static int __init assign_dmar_vectors(void)
> +{
> +	struct work_struct irq_remap_work;
> +	int nid;
> +
> +	INIT_WORK(&irq_remap_work, irq_remap_enable_fault_handling_thr);
> +	cpus_read_lock();
> +	for_each_online_node(nid) {
> +		/* Boot cpu dmar vectors are assigned before the rest */
> +		if (nid == cpu_to_node(get_boot_cpu_id()))
> +			continue;
> +		schedule_work_on(cpumask_first(cpumask_of_node(nid)),
> +				 &irq_remap_work);
> +		flush_work(&irq_remap_work);
> +	}
> +	cpus_read_unlock();
> +	return 0;
> +}
> +
> +arch_initcall(assign_dmar_vectors);

Stray newline before arch_initcall(), but that's not the problem.

The real problems are:

 1) This approach only works when _ALL_ APs have been brought up during
    boot. With 'maxcpus=N' on the command line this will fail to enable
    fault handling when the APs which have not been brought up initially
    are onlined later on.

    This might be working in practice because intel_iommu_init() will
    enable the interrupts later on via init_dmars() unconditionally, but
    that's far from correct because IRQ_REMAP does not depend on
    INTEL_IOMMU.

 2) It leaves a gap where the reporting is not working between bringing
    up the APs during boot and this initcall. Mostly theoretical, but
    that does not make it more correct either.

What you really want is a cpu hotplug state in the CPUHP_BP_PREPARE_DYN
space which enables the interrupt for the node _before_ the first AP of
the node is brought up. That will solve the problem nicely w/o any of
the above issues.

Thanks,

        tglx