[PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Rahul Tanwar]

'ioapic_dynirq_base' contains the virtual IRQ base number. Presently, it is
updated to the end of hardware IRQ numbers but this is done only when IOAPIC
configuration type is IOAPIC_DOMAIN_LEGACY or IOAPIC_DOMAIN_STRICT. There is
a third type IOAPIC_DOMAIN_DYNAMIC which applies when IOAPIC configuration
comes from devicetree.
Please see dtb_add_ioapic() in arch/x86/kernel/devicetree.c

In case of IOAPIC_DOMAIN_DYNAMIC (DT/OF based system), 'ioapic_dynirq_base'
remains to zero initialized value. This means that for OF based systems,
virtual IRQ base will get set to zero. Zero value for a virtual IRQ is a
invalid value.
Please see https://yarchive.net/comp/linux/zero.html for more details.

Update 'ioapic_dynirq_base' for IOAPIC_DOMAIN_DYNAMIC case as well just like
it is presently updated for IOAPIC_DOMAIN_LEGACY & IOAPIC_DOMAIN_STRICT i.e.
set the virtual IRQ base to the end of hardware IRQ numbers when IOAPIC
configuration comes from devicetree.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 arch/x86/kernel/apic/io_apic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c7bb6c69f21c..fe50cd91122b 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2336,7 +2336,8 @@ static int mp_irqdomain_create(int ioapic)
 	ip->irqdomain->parent = parent;
 
 	if (cfg->type == IOAPIC_DOMAIN_LEGACY ||
-	    cfg->type == IOAPIC_DOMAIN_STRICT)
+	    cfg->type == IOAPIC_DOMAIN_STRICT ||
+	    cfg->type == IOAPIC_DOMAIN_DYNAMIC)
 		ioapic_dynirq_base = max(ioapic_dynirq_base,
 					 gsi_cfg->gsi_end + 1);
 
-- 
2.11.0

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Thomas Gleixner]

On Wed, 21 Aug 2019, Rahul Tanwar wrote:

> 'ioapic_dynirq_base' contains the virtual IRQ base number. Presently, it is
> updated to the end of hardware IRQ numbers but this is done only when IOAPIC
> configuration type is IOAPIC_DOMAIN_LEGACY or IOAPIC_DOMAIN_STRICT. There is
> a third type IOAPIC_DOMAIN_DYNAMIC which applies when IOAPIC configuration
> comes from devicetree.
> Please see dtb_add_ioapic() in arch/x86/kernel/devicetree.c

We know how DT based ioapics are added. No need to point to it.

> In case of IOAPIC_DOMAIN_DYNAMIC (DT/OF based system), 'ioapic_dynirq_base'
> remains to zero initialized value. This means that for OF based systems,
> virtual IRQ base will get set to zero. Zero value for a virtual IRQ is a
> invalid value.
> Please see https://yarchive.net/comp/linux/zero.html for more details.

First of all, please do not use random archive links. See
Documentation/process/ how links to LKML archives should look like

Secondly, this link is irrelevant. ioapic_dynirq_base has nothing to do
with virtual IRQ number 0. It's a boundary for the dynamic allocation of
virtual interrupt numbers so that the core allocator does not pick
interrupts out of the IOAPIC's fixed interrupt number space.

This can be legitimately 0 when IOAPIC is not enabled at all.

Can you please explain what kind of problem you were seing and what this
really fixes?
 
Thanks,

	tglx

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Tanwar, Rahul]

On 21/8/2019 4:34 PM, Thomas Gleixner wrote:

> Secondly, this link is irrelevant. ioapic_dynirq_base has nothing to do
> with virtual IRQ number 0. It's a boundary for the dynamic allocation of
> virtual interrupt numbers so that the core allocator does not pick
> interrupts out of the IOAPIC's fixed interrupt number space.
>
> This can be legitimately 0 when IOAPIC is not enabled at all.
>
> Can you please explain what kind of problem you were seing and what this
> really fixes?

The problem is that device tree infrastructure considers 0 IRQ value as 
invalid/error

value whereas for ACPI, 0 is a valid value. Without this change, the 
problem that we

see is that the first driver using of_irq_get_xx() or its variants fails 
because of 0 IRQ

number. With this change, allocated IRQ number is never 0 so it works ok.

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Thomas Gleixner]

On Wed, 21 Aug 2019, Tanwar, Rahul wrote:
> On 21/8/2019 4:34 PM, Thomas Gleixner wrote:
> 
> > Secondly, this link is irrelevant. ioapic_dynirq_base has nothing to do
> > with virtual IRQ number 0. It's a boundary for the dynamic allocation of
> > virtual interrupt numbers so that the core allocator does not pick
> > interrupts out of the IOAPIC's fixed interrupt number space.
> > 
> > This can be legitimately 0 when IOAPIC is not enabled at all.
> > 
> > Can you please explain what kind of problem you were seing and what this
> > really fixes?
>
> The problem is that device tree infrastructure considers 0 IRQ value as
> invalid/error value whereas for ACPI, 0 is a valid value.

Sure.

> Without this change, the problem that we see is that the first driver
> using of_irq_get_xx() or its variants fails because of 0 IRQ number. With
> this change, allocated IRQ number is never 0 so it works ok.

Well, this still is not a proper explanation. Just because it works does
not make it correct in the first place.

ioapic_dynirq_base is pretty much irrelevant for a DT machine. The reason
why it exists is that for regular BIOS the interrupt numbers are hard
mapped to the IOAPIC pins. ioapic_dynirq_base is used to protect this hard
mapped interrupt number space. The core allocator does not allocate from
that space unless it is explicitely told to do so, which is the case for
IOAPIC_DOMAIN_STRICT where the allocation tells the core to allocate the
associated GSI number.

On DT the interrupt number is irrelevant as DT describes the irq controller
and the pin to which a device is connected and does not make assumptions
about the interrupt number. So the core can freely allocate any available
interrupt number except 0. That's already prevented in the core code.

But x86 implements arch_dynirq_lower_bound() which overrides the core limit
and because ioapic_dynirq_base is zero in the DT case it allows VIRQ 0 to
be allocated which then causes of_irq*() to fail.

So your change prevents that by excluding the 'GSI' range from allocation,
which means that the first irq number which is handed out is 24, assumed
you have one IOAPIC with 24 pins.

That's fine as the interrupt number space is big enough, but it needs

    - a coherent explanation in the changelog

    - proper comments to that effect in the code

Also this is presumably a stable candidate and needs a Fixes: ... tag.

Thanks,

	tglx

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Andy Shevchenko]

On Wed, Aug 21, 2019 at 01:20:53PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Tanwar, Rahul wrote:
> > On 21/8/2019 4:34 PM, Thomas Gleixner wrote:
> > 
> > > Secondly, this link is irrelevant. ioapic_dynirq_base has nothing to do
> > > with virtual IRQ number 0. It's a boundary for the dynamic allocation of
> > > virtual interrupt numbers so that the core allocator does not pick
> > > interrupts out of the IOAPIC's fixed interrupt number space.
> > > 
> > > This can be legitimately 0 when IOAPIC is not enabled at all.
> > > 
> > > Can you please explain what kind of problem you were seing and what this
> > > really fixes?
> >
> > The problem is that device tree infrastructure considers 0 IRQ value as
> > invalid/error value whereas for ACPI, 0 is a valid value.
> 
> Sure.
> 
> > Without this change, the problem that we see is that the first driver
> > using of_irq_get_xx() or its variants fails because of 0 IRQ number. With
> > this change, allocated IRQ number is never 0 so it works ok.
> 
> Well, this still is not a proper explanation. Just because it works does
> not make it correct in the first place.
> 
> ioapic_dynirq_base is pretty much irrelevant for a DT machine. The reason
> why it exists is that for regular BIOS the interrupt numbers are hard
> mapped to the IOAPIC pins. ioapic_dynirq_base is used to protect this hard
> mapped interrupt number space. The core allocator does not allocate from
> that space unless it is explicitely told to do so, which is the case for
> IOAPIC_DOMAIN_STRICT where the allocation tells the core to allocate the
> associated GSI number.
> 
> On DT the interrupt number is irrelevant as DT describes the irq controller
> and the pin to which a device is connected and does not make assumptions
> about the interrupt number. So the core can freely allocate any available
> interrupt number except 0. That's already prevented in the core code.
> 
> But x86 implements arch_dynirq_lower_bound() which overrides the core limit
> and because ioapic_dynirq_base is zero in the DT case it allows VIRQ 0 to
> be allocated which then causes of_irq*() to fail.
> 
> So your change prevents that by excluding the 'GSI' range from allocation,
> which means that the first irq number which is handed out is 24, assumed
> you have one IOAPIC with 24 pins.

I have tested this on the ACPI-based system where we have 55 lines of IOAPIC,
no PIC, and some GPIO lines. Overall I see that nr_irqs is 512 and shifting
by 55 freezes 10% of the space for nothing. Luckily we have SPARSE_IRQS
selected for any X86, so, it wouldn't waste memory.

I think we may do slightly better if we just limit the change to the certain
cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Thomas Gleixner]

On Wed, 21 Aug 2019, Andy Shevchenko wrote:
> On Wed, Aug 21, 2019 at 01:20:53PM +0200, Thomas Gleixner wrote:
> > But x86 implements arch_dynirq_lower_bound() which overrides the core limit
> > and because ioapic_dynirq_base is zero in the DT case it allows VIRQ 0 to
> > be allocated which then causes of_irq*() to fail.
> > 
> > So your change prevents that by excluding the 'GSI' range from allocation,
> > which means that the first irq number which is handed out is 24, assumed
> > you have one IOAPIC with 24 pins.
> 
> I have tested this on the ACPI-based system where we have 55 lines of IOAPIC,
> no PIC, and some GPIO lines. Overall I see that nr_irqs is 512 and shifting
> by 55 freezes 10% of the space for nothing. Luckily we have SPARSE_IRQS
> selected for any X86, so, it wouldn't waste memory.
>
> I think we may do slightly better if we just limit the change to the certain
> cases.

For DT we can actually avoid that completely. See below.

For ACPI not unfortunately as the stupid GSI mapping is hard coded.

Thanks,

	tglx

8<-------------
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2438,7 +2438,13 @@ unsigned int arch_dynirq_lower_bound(uns
 	 * dmar_alloc_hwirq() may be called before setup_IO_APIC(), so use
 	 * gsi_top if ioapic_dynirq_base hasn't been initialized yet.
 	 */
-	return ioapic_initialized ? ioapic_dynirq_base : gsi_top;
+	if (!ioapic_initialized)
+		return gsi_top;
+	/*
+	 * For DT enabled machines ioapic_dynirq_base is irrelevant and not
+	 * updated. So simply return @from if ioapic_dynirq_base == 0.
+	 */
+	return ioapic_dynirq_base ? : from;
 }
 
 #ifdef CONFIG_X86_32

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Andy Shevchenko]

On Wed, Aug 21, 2019 at 03:16:31PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Andy Shevchenko wrote:
> > On Wed, Aug 21, 2019 at 01:20:53PM +0200, Thomas Gleixner wrote:
> > > But x86 implements arch_dynirq_lower_bound() which overrides the core limit
> > > and because ioapic_dynirq_base is zero in the DT case it allows VIRQ 0 to
> > > be allocated which then causes of_irq*() to fail.
> > > 
> > > So your change prevents that by excluding the 'GSI' range from allocation,
> > > which means that the first irq number which is handed out is 24, assumed
> > > you have one IOAPIC with 24 pins.
> > 
> > I have tested this on the ACPI-based system where we have 55 lines of IOAPIC,
> > no PIC, and some GPIO lines. Overall I see that nr_irqs is 512 and shifting
> > by 55 freezes 10% of the space for nothing. Luckily we have SPARSE_IRQS
> > selected for any X86, so, it wouldn't waste memory.
> >
> > I think we may do slightly better if we just limit the change to the certain
> > cases.
> 
> For DT we can actually avoid that completely. See below.
> 
> For ACPI not unfortunately as the stupid GSI mapping is hard coded.

The below works better for my case, so, if you are going with that
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> 
> Thanks,
> 
> 	tglx
> 
> 8<-------------
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2438,7 +2438,13 @@ unsigned int arch_dynirq_lower_bound(uns
>  	 * dmar_alloc_hwirq() may be called before setup_IO_APIC(), so use
>  	 * gsi_top if ioapic_dynirq_base hasn't been initialized yet.
>  	 */
> -	return ioapic_initialized ? ioapic_dynirq_base : gsi_top;
> +	if (!ioapic_initialized)
> +		return gsi_top;
> +	/*
> +	 * For DT enabled machines ioapic_dynirq_base is irrelevant and not
> +	 * updated. So simply return @from if ioapic_dynirq_base == 0.
> +	 */
> +	return ioapic_dynirq_base ? : from;
>  }
>  
>  #ifdef CONFIG_X86_32

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system as well

[Tanwar, Rahul]

Hi Thomas,

On 22/8/2019 12:47 AM, Andy Shevchenko wrote:
>> For DT we can actually avoid that completely. See below.
>>
>> For ACPI not unfortunately as the stupid GSI mapping is hard coded.
> The below works better for my case, so, if you are going with that
> Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>
>
> 8<-------------
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2438,7 +2438,13 @@ unsigned int arch_dynirq_lower_bound(uns
>   	 * dmar_alloc_hwirq() may be called before setup_IO_APIC(), so use
>   	 * gsi_top if ioapic_dynirq_base hasn't been initialized yet.
>   	 */
> -	return ioapic_initialized ? ioapic_dynirq_base : gsi_top;
> +	if (!ioapic_initialized)
> +		return gsi_top;
> +	/*
> +	 * For DT enabled machines ioapic_dynirq_base is irrelevant and not
> +	 * updated. So simply return @from if ioapic_dynirq_base == 0.
> +	 */
> +	return ioapic_dynirq_base ? : from;
>   }
>   
>   #ifdef CONFIG_X86_32


I have also tested above and it works fine. In fact, my first patch to

resolve it during internal review was exactly on similar lines. So if

you are going to add above then i will stop following up on this

topic further. Thanks.


Regards,

Rahul

[tip: x86/urgent] x86/apic: Fix arch_dynirq_lower_bound() bug for DT enabled machines

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     3e5bedc2c258341702ddffbd7688c5e6eb01eafa
Gitweb:        https://git.kernel.org/tip/3e5bedc2c258341702ddffbd7688c5e6eb01eafa
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 21 Aug 2019 15:16:31 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 26 Aug 2019 12:11:23 +02:00

x86/apic: Fix arch_dynirq_lower_bound() bug for DT enabled machines

Rahul Tanwar reported the following bug on DT systems:

> 'ioapic_dynirq_base' contains the virtual IRQ base number. Presently, it is
> updated to the end of hardware IRQ numbers but this is done only when IOAPIC
> configuration type is IOAPIC_DOMAIN_LEGACY or IOAPIC_DOMAIN_STRICT. There is
> a third type IOAPIC_DOMAIN_DYNAMIC which applies when IOAPIC configuration
> comes from devicetree.
>
> See dtb_add_ioapic() in arch/x86/kernel/devicetree.c
>
> In case of IOAPIC_DOMAIN_DYNAMIC (DT/OF based system), 'ioapic_dynirq_base'
> remains to zero initialized value. This means that for OF based systems,
> virtual IRQ base will get set to zero.

Such systems will very likely not even boot.

For DT enabled machines ioapic_dynirq_base is irrelevant and not
updated, so simply map the IRQ base 1:1 instead.

Reported-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
Tested-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: alan@linux.intel.com
Cc: bp@alien8.de
Cc: cheol.yong.kim@intel.com
Cc: qi-ming.wu@intel.com
Cc: rahul.tanwar@intel.com
Cc: rppt@linux.ibm.com
Cc: tony.luck@intel.com
Link: http://lkml.kernel.org/r/20190821081330.1187-1-rahul.tanwar@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/apic/io_apic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c7bb6c6..d6af97f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2438,7 +2438,13 @@ unsigned int arch_dynirq_lower_bound(unsigned int from)
 	 * dmar_alloc_hwirq() may be called before setup_IO_APIC(), so use
 	 * gsi_top if ioapic_dynirq_base hasn't been initialized yet.
 	 */
-	return ioapic_initialized ? ioapic_dynirq_base : gsi_top;
+	if (!ioapic_initialized)
+		return gsi_top;
+	/*
+	 * For DT enabled machines ioapic_dynirq_base is irrelevant and not
+	 * updated. So simply return @from if ioapic_dynirq_base == 0.
+	 */
+	return ioapic_dynirq_base ? : from;
 }
 
 #ifdef CONFIG_X86_32