Re: [PATCH 09/13] x86/irq: Add x86_non_ir_cpumask

From: David Woodhouse <dwmw2@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org
Cc: iommu <iommu@lists.linux-foundation.org>,
	kvm <kvm@vger.kernel.org>,
	linux-hyperv@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 09/13] x86/irq: Add x86_non_ir_cpumask
Date: Wed, 07 Oct 2020 08:25:57 +0100	[thread overview]
Message-ID: <3bc0d6d001b276f881310714fb59ea03ac0e5173.camel@infradead.org> (raw)
In-Reply-To: <87ft6r58vy.fsf@nanos.tec.linutronix.de>

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

On Tue, 2020-10-06 at 23:42 +0200, Thomas Gleixner wrote:
> On Mon, Oct 05 2020 at 16:28, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > This is the mask of CPUs to which IRQs can be delivered without
> > interrupt
> > remapping.
> >  
> > +/* Mask of CPUs which can be targeted by non-remapped interrupts.
> > */
> > +cpumask_t x86_non_ir_cpumask = { CPU_BITS_ALL };
> 
> What?

By default, if we didn't hit any limits, all CPUs can be targeted by
external interrupts. It's the default today.

Or at least we pretend it is, modulo the bugs :)

> >  #ifdef CONFIG_X86_32
> >  
> >  /*
> > @@ -1838,6 +1841,7 @@ static __init void x2apic_enable(void)
> >  static __init void try_to_enable_x2apic(int remap_mode)
> >  {
> >  	u32 apic_limit = 0;
> > +	int i;
> >  
> >  	if (x2apic_state == X2APIC_DISABLED)
> >  		return;
> > @@ -1880,6 +1884,14 @@ static __init void try_to_enable_x2apic(int remap_mode)
> >  	if (apic_limit)
> >  		x2apic_set_max_apicid(apic_limit);
> >  
> > +	/* Build the affinity mask for interrupts that can't be remapped. */
> > +	cpumask_clear(&x86_non_ir_cpumask);
> > +	i = min_t(unsigned int, num_possible_cpus() - 1, apic_limit);
> > +	for ( ; i >= 0; i--) {
> > +		if (cpu_physical_id(i) <= apic_limit)
> > +			cpumask_set_cpu(i, &x86_non_ir_cpumask);
> > +	}
> 
> Blink. If the APIC id is not linear with the cpu numbers then this
> results in a reduced addressable set of CPUs. WHY?

Hm, good question. That loop was cargo-culted from hyperv-iommu.c;
perhaps it makes more sense there because Hyper-V really does promise
that linearity, or perhaps it was already buggy. Will fix.

In fact, in apic.c I could probably just use the cpuid_to_apicid array
which is right there in the file.

> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index aa9a3b54a96c..4d0ef46fedb9 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -2098,6 +2098,8 @@ static int mp_alloc_timer_irq(int ioapic, int pin)
> >  		struct irq_alloc_info info;
> >  
> >  		ioapic_set_alloc_attr(&info, NUMA_NO_NODE, 0, 0);
> > +		if (domain->parent == x86_vector_domain)
> > +			info.mask = &x86_non_ir_cpumask;
> 
> We are not going to sprinkle such domain checks all over the
> place. Again, the mask is a property of the interrupt domain.

Yeah, that's a hangover from the first attempts which I forgot to
delete.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]