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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1908211235180.2223@nanos.tec.linutronix.de>
Date:   Wed, 21 Aug 2019 13:20:53 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Tanwar, Rahul" <rahul.tanwar@...ux.intel.com>
cc:     mingo@...hat.com, bp@...en8.de, hpa@...or.com, tony.luck@...el.com,
        x86@...nel.org, andriy.shevchenko@...el.com, alan@...ux.intel.com,
        rppt@...ux.ibm.com, linux-kernel@...r.kernel.org,
        qi-ming.wu@...el.com, cheol.yong.kim@...el.com,
        rahul.tanwar@...el.com
Subject: Re: [PATCH] x86/apic: Update virtual irq base for DT/OF based system
 as well

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