diff mbox series

[2/2] irqchip: nvic: Use GENERIC_IRQ_MULTI_HANDLER

Message ID 20211201110259.84857-2-vladimir.murzin@arm.com (mailing list archive)
State New, archived
Headers show
Series [1/2] irqchip: nvic: Fix offset for Interrupt Priority Offsets | expand

Commit Message

Vladimir Murzin Dec. 1, 2021, 11:02 a.m. UTC
Rather then restructuring the ARMv7M entrly logic per TODO, just move
NVIC to GENERIC_IRQ_MULTI_HANDLER.

Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 arch/arm/include/asm/v7m.h  |  3 ++-
 arch/arm/kernel/entry-v7m.S | 10 +++-------
 drivers/irqchip/Kconfig     |  1 +
 drivers/irqchip/irq-nvic.c  | 22 +++++-----------------
 4 files changed, 11 insertions(+), 25 deletions(-)

Comments

Mark Rutland Dec. 1, 2021, 11:58 a.m. UTC | #1
Adding Ard and Arnd, since this looks like it might interact with their series
moving other platforms to GENERIC_IRQ_MULTI_HANDLER:

  https://lore.kernel.org/linux-arm-kernel/20211130125901.3054-1-ardb@kernel.org/

On Wed, Dec 01, 2021 at 11:02:59AM +0000, Vladimir Murzin wrote:
> Rather then restructuring the ARMv7M entrly logic per TODO, just move
> NVIC to GENERIC_IRQ_MULTI_HANDLER.
> 
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  arch/arm/include/asm/v7m.h  |  3 ++-
>  arch/arm/kernel/entry-v7m.S | 10 +++-------
>  drivers/irqchip/Kconfig     |  1 +
>  drivers/irqchip/irq-nvic.c  | 22 +++++-----------------
>  4 files changed, 11 insertions(+), 25 deletions(-)

Nice; thanks for doing this!

FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> 
> diff --git a/arch/arm/include/asm/v7m.h b/arch/arm/include/asm/v7m.h
> index 2cb00d1..4512f7e 100644
> --- a/arch/arm/include/asm/v7m.h
> +++ b/arch/arm/include/asm/v7m.h
> @@ -13,6 +13,7 @@
>  #define V7M_SCB_ICSR_PENDSVSET			(1 << 28)
>  #define V7M_SCB_ICSR_PENDSVCLR			(1 << 27)
>  #define V7M_SCB_ICSR_RETTOBASE			(1 << 11)
> +#define V7M_SCB_ICSR_VECTACTIVE			0x000001ff
>  
>  #define V7M_SCB_VTOR			0x08
>  
> @@ -38,7 +39,7 @@
>  #define V7M_SCB_SHCSR_MEMFAULTENA		(1 << 16)
>  
>  #define V7M_xPSR_FRAMEPTRALIGN			0x00000200
> -#define V7M_xPSR_EXCEPTIONNO			0x000001ff
> +#define V7M_xPSR_EXCEPTIONNO			V7M_SCB_ICSR_VECTACTIVE
>  
>  /*
>   * When branching to an address that has bits [31:28] == 0xf an exception return
> diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
> index 7bde93c..520dd43 100644
> --- a/arch/arm/kernel/entry-v7m.S
> +++ b/arch/arm/kernel/entry-v7m.S
> @@ -39,14 +39,10 @@ __irq_entry:
>  	@
>  	@ Invoke the IRQ handler
>  	@
> -	mrs	r0, ipsr
> -	ldr	r1, =V7M_xPSR_EXCEPTIONNO
> -	and	r0, r1
> -	sub	r0, #16
> -	mov	r1, sp
> +	mov	r0, sp
>  	stmdb	sp!, {lr}
> -	@ routine called with r0 = irq number, r1 = struct pt_regs *
> -	bl	nvic_handle_irq
> +	@ routine called with r0 = struct pt_regs *
> +	bl	generic_handle_arch_irq
>  
>  	pop	{lr}
>  	@
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957..488eaa1 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -58,6 +58,7 @@ config ARM_NVIC
>  	bool
>  	select IRQ_DOMAIN_HIERARCHY
>  	select GENERIC_IRQ_CHIP
> +	select GENERIC_IRQ_MULTI_HANDLER
>  
>  config ARM_VIC
>  	bool
> diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c
> index ba4759b..125f9c1 100644
> --- a/drivers/irqchip/irq-nvic.c
> +++ b/drivers/irqchip/irq-nvic.c
> @@ -37,25 +37,12 @@
>  
>  static struct irq_domain *nvic_irq_domain;
>  
> -static void __nvic_handle_irq(irq_hw_number_t hwirq)
> +static void __irq_entry nvic_handle_irq(struct pt_regs *regs)
>  {
> -	generic_handle_domain_irq(nvic_irq_domain, hwirq);
> -}
> +	unsigned long icsr = readl_relaxed(BASEADDR_V7M_SCB + V7M_SCB_ICSR);
> +	irq_hw_number_t hwirq = (icsr & V7M_SCB_ICSR_VECTACTIVE) - 16;
>  
> -/*
> - * TODO: restructure the ARMv7M entry logic so that this entry logic can live
> - * in arch code.
> - */
> -asmlinkage void __exception_irq_entry
> -nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs)
> -{
> -	struct pt_regs *old_regs;
> -
> -	irq_enter();
> -	old_regs = set_irq_regs(regs);
> -	__nvic_handle_irq(hwirq);
> -	set_irq_regs(old_regs);
> -	irq_exit();
> +	generic_handle_domain_irq(nvic_irq_domain, hwirq);
>  }
>  
>  static int nvic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> @@ -141,6 +128,7 @@ static int __init nvic_of_init(struct device_node *node,
>  	for (i = 0; i < irqs; i += 4)
>  		writel_relaxed(0, nvic_base + NVIC_IPR + i);
>  
> +	set_handle_irq(nvic_handle_irq);
>  	return 0;
>  }
>  IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);
> -- 
> 2.7.4
>
Arnd Bergmann Dec. 1, 2021, 12:30 p.m. UTC | #2
On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>
> Rather then restructuring the ARMv7M entrly logic per TODO, just move
> NVIC to GENERIC_IRQ_MULTI_HANDLER.
>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks for finishing this one off as well! I don't know if you did this as a
reaction to me doing the same for the three other platforms that don't
use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
coincidence. It sounds like this should be part of Ard's IRQ stack series
as well then (adding him to Cc), as it fits in with my patches there.

Would you mind also implementing and testing the missing bits
for the stack switching on nommu irq entry, corresponding to [2] so
we can just use that part unconditionally as well?

       Arnd

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=arm-up-ti-in-task-v3&id=f3ad7675e9e02996331c69a48f45db60ac8e9852
and parents
[2] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/diff/arch/arm/kernel/entry-armv.S?h=arm-up-ti-in-task-v3&id=950bf934b1c1c4efe579f40a82a0fca0ae223d2a
Vladimir Murzin Dec. 1, 2021, 1:43 p.m. UTC | #3
On 12/1/21 12:30 PM, Arnd Bergmann wrote:
> On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>
>> Rather then restructuring the ARMv7M entrly logic per TODO, just move
>> NVIC to GENERIC_IRQ_MULTI_HANDLER.
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Thanks for finishing this one off as well! I don't know if you did this as a
> reaction to me doing the same for the three other platforms that don't
> use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
> coincidence. It sounds like this should be part of Ard's IRQ stack series
> as well then (adding him to Cc), as it fits in with my patches there.

It was reaction to Mark's TODO statement in his original submission [1].
I'd let maintainers to decide on the path it should land :) 

> 
> Would you mind also implementing and testing the missing bits
> for the stack switching on nommu irq entry, corresponding to [2] so
> we can just use that part unconditionally as well?

I can have a look but no promises since I have few issues with NOMMU
ATM (one of them seems to be cross-arch [2])

[1] https://lore.kernel.org/linux-arm-kernel/20211021180236.37428-10-mark.rutland@arm.com/
[2] https://lore.kernel.org/linux-mm/20211130172954.129587-1-vladimir.murzin@arm.com/T/

Cheers
Vladimir

> 
>        Arnd
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=arm-up-ti-in-task-v3&id=f3ad7675e9e02996331c69a48f45db60ac8e9852
> and parents
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/diff/arch/arm/kernel/entry-armv.S?h=arm-up-ti-in-task-v3&id=950bf934b1c1c4efe579f40a82a0fca0ae223d2a
>
Arnd Bergmann Dec. 1, 2021, 2:05 p.m. UTC | #4
On Wed, Dec 1, 2021 at 2:43 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> On 12/1/21 12:30 PM, Arnd Bergmann wrote:
> > On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> >>
> >> Rather then restructuring the ARMv7M entrly logic per TODO, just move
> >> NVIC to GENERIC_IRQ_MULTI_HANDLER.
> >>
> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Thanks for finishing this one off as well! I don't know if you did this as a
> > reaction to me doing the same for the three other platforms that don't
> > use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
> > coincidence. It sounds like this should be part of Ard's IRQ stack series
> > as well then (adding him to Cc), as it fits in with my patches there.
>
> It was reaction to Mark's TODO statement in his original submission [1].
> I'd let maintainers to decide on the path it should land :)

Ah, nice, so it was a coincidence after all. I guess my patches were easier
because of Mark's (and Marc's) earlier work on related bits.

> > Would you mind also implementing and testing the missing bits
> > for the stack switching on nommu irq entry, corresponding to [2] so
> > we can just use that part unconditionally as well?
>
> I can have a look but no promises since I have few issues with NOMMU
> ATM (one of them seems to be cross-arch [2])

Yes, I saw that, but I'm not too worried about this on Arm, as we don't
support that combination (SMP without MMU) in any of our platforms or
in Kconfig without additional patches.

What target are you actually testing on?

       Arnd
Vladimir Murzin Dec. 1, 2021, 2:15 p.m. UTC | #5
On 12/1/21 2:05 PM, Arnd Bergmann wrote:
> On Wed, Dec 1, 2021 at 2:43 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>> On 12/1/21 12:30 PM, Arnd Bergmann wrote:
>>> On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>>>
>>>> Rather then restructuring the ARMv7M entrly logic per TODO, just move
>>>> NVIC to GENERIC_IRQ_MULTI_HANDLER.
>>>>
>>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>>>
>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>
>>> Thanks for finishing this one off as well! I don't know if you did this as a
>>> reaction to me doing the same for the three other platforms that don't
>>> use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
>>> coincidence. It sounds like this should be part of Ard's IRQ stack series
>>> as well then (adding him to Cc), as it fits in with my patches there.
>>
>> It was reaction to Mark's TODO statement in his original submission [1].
>> I'd let maintainers to decide on the path it should land :)
> 
> Ah, nice, so it was a coincidence after all. I guess my patches were easier
> because of Mark's (and Marc's) earlier work on related bits.
> 
>>> Would you mind also implementing and testing the missing bits
>>> for the stack switching on nommu irq entry, corresponding to [2] so
>>> we can just use that part unconditionally as well?
>>
>> I can have a look but no promises since I have few issues with NOMMU
>> ATM (one of them seems to be cross-arch [2])
> 
> Yes, I saw that, but I'm not too worried about this on Arm, as we don't
> support that combination (SMP without MMU) in any of our platforms or
> in Kconfig without additional patches.

That's true, yet it seems to be broken for other arches which seems to be
supported (w/o additional patches?)

> 
> What target are you actually testing on?
> 

It is MPS3 board with range of FPGA images. M and R class supported yet
AN536 (Cortex-R52x2) has issues which prevents it using in SMP; recent 
images for M class require new timer (which is WIP).

I also have access to Fast Models - they are easier and quicker to test
and do not require visiting office :)

[1] https://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/download-fpga-images

Cheers
Vladimir

>        Arnd
>
Arnd Bergmann Dec. 1, 2021, 3:13 p.m. UTC | #6
On Wed, Dec 1, 2021 at 3:15 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> On 12/1/21 2:05 PM, Arnd Bergmann wrote:
> >
> > Yes, I saw that, but I'm not too worried about this on Arm, as we don't
> > support that combination (SMP without MMU) in any of our platforms or
> > in Kconfig without additional patches.
>
> That's true, yet it seems to be broken for other arches which seems to be
> supported (w/o additional patches?)

Sure, it should get fixed, it's just not on my list of high-priority problems.
For j2, I think there isn't much work going on at the moment, and I think
there are other problems on sh nommu smp. For riscv K210, I think there
isn't as much interest any more after MMU-based platforms are becoming
more available.

> > What target are you actually testing on?
> >
>
> It is MPS3 board with range of FPGA images. M and R class supported yet
> AN536 (Cortex-R52x2) has issues which prevents it using in SMP; recent
> images for M class require new timer (which is WIP).
>
> I also have access to Fast Models - they are easier and quicker to test
> and do not require visiting office :)

Ok, I see. What are your plans for the Cortex-R support? My impression
was that there isn't really much interest in upstream support as there is
no commercial SoC platform using 32-bit Cortex-R that makes sense to
run Linux on (any more), and Cortex-R82 would run with MMU enabled.
Do you expect this to change in the future?

As far as I can tell, there is no SMP-capable Cortex-M. If there is,
then Ard's THREAD_INFO_IN_TASK series may have another problem
because of the lack of the TPIDRURO register. Again, I'm not too
worried about that because mainline Linux does not support this
configuration, but it's something to keep in mind.

         Arnd
Vladimir Murzin Dec. 1, 2021, 3:51 p.m. UTC | #7
On 12/1/21 3:13 PM, Arnd Bergmann wrote:
> On Wed, Dec 1, 2021 at 3:15 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>> On 12/1/21 2:05 PM, Arnd Bergmann wrote:
>>>
>>> Yes, I saw that, but I'm not too worried about this on Arm, as we don't
>>> support that combination (SMP without MMU) in any of our platforms or
>>> in Kconfig without additional patches.
>>
>> That's true, yet it seems to be broken for other arches which seems to be
>> supported (w/o additional patches?)
> 
> Sure, it should get fixed, it's just not on my list of high-priority problems.
> For j2, I think there isn't much work going on at the moment, and I think
> there are other problems on sh nommu smp. For riscv K210, I think there
> isn't as much interest any more after MMU-based platforms are becoming
> more available.
> 
>>> What target are you actually testing on?
>>>
>>
>> It is MPS3 board with range of FPGA images. M and R class supported yet
>> AN536 (Cortex-R52x2) has issues which prevents it using in SMP; recent
>> images for M class require new timer (which is WIP).
>>
>> I also have access to Fast Models - they are easier and quicker to test
>> and do not require visiting office :)
> 
> Ok, I see. What are your plans for the Cortex-R support? My impression
> was that there isn't really much interest in upstream support as there is
> no commercial SoC platform using 32-bit Cortex-R that makes sense to
> run Linux on (any more), and Cortex-R82 would run with MMU enabled.
> Do you expect this to change in the future?

There are not many patches to make Linux run on Cortex-R - it is mostly to
undo multiplform, so I'm fine keeping them downstream.

I cannot say how much interest in running Linux with 32-bit Cortex-R, yet
occasionally people ask me how to do that (the same applies to M-class).

> 
> As far as I can tell, there is no SMP-capable Cortex-M. If there is,
> then Ard's THREAD_INFO_IN_TASK series may have another problem
> because of the lack of the TPIDRURO register. Again, I'm not too
> worried about that because mainline Linux does not support this
> configuration, but it's something to keep in mind.

I'm not aware of any SMP-capable Cortex-M either :)

Cheers
Vladimir

> 
>          Arnd
>
Ard Biesheuvel Dec. 2, 2021, 8:35 a.m. UTC | #8
On Wed, 1 Dec 2021 at 13:31, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> >
> > Rather then restructuring the ARMv7M entrly logic per TODO, just move
> > NVIC to GENERIC_IRQ_MULTI_HANDLER.
> >
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks for finishing this one off as well! I don't know if you did this as a
> reaction to me doing the same for the three other platforms that don't
> use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
> coincidence. It sounds like this should be part of Ard's IRQ stack series
> as well then (adding him to Cc), as it fits in with my patches there.
>

Yes, I'll add this one to my series. But only the 2/2, the other one
looks like a bugfix and can be put into the patch tracker.

> Would you mind also implementing and testing the missing bits
> for the stack switching on nommu irq entry, corresponding to [2] so
> we can just use that part unconditionally as well?
>

It seems trivial enough to add that for v7m as well, so I will include
a patch for it in my v3. But I won't be able to test it, though.
Marc Zyngier Dec. 2, 2021, 9:05 a.m. UTC | #9
On 2021-12-02 08:35, Ard Biesheuvel wrote:
> On Wed, 1 Dec 2021 at 13:31, Arnd Bergmann <arnd@arndb.de> wrote:
>> 
>> On Wed, Dec 1, 2021 at 12:02 PM Vladimir Murzin 
>> <vladimir.murzin@arm.com> wrote:
>> >
>> > Rather then restructuring the ARMv7M entrly logic per TODO, just move
>> > NVIC to GENERIC_IRQ_MULTI_HANDLER.
>> >
>> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> 
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> 
>> Thanks for finishing this one off as well! I don't know if you did 
>> this as a
>> reaction to me doing the same for the three other platforms that don't
>> use GENERIC_IRQ_MULTI_HANDLER yet[1], or if this was just a lucky
>> coincidence. It sounds like this should be part of Ard's IRQ stack 
>> series
>> as well then (adding him to Cc), as it fits in with my patches there.
>> 
> 
> Yes, I'll add this one to my series. But only the 2/2, the other one
> looks like a bugfix and can be put into the patch tracker.

I'll pick 1/2 now and send it as part of the irqchip fixes.

         M.
Arnd Bergmann Dec. 2, 2021, 9:23 a.m. UTC | #10
On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> It seems trivial enough to add that for v7m as well, so I will include
> a patch for it in my v3. But I won't be able to test it, though.

Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
Cortex-M based platform and should be able to test this by merging in your
changes from [1] after you have uploaded the latest version.

        Arnd

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3
Marc Zyngier Dec. 2, 2021, 9:31 a.m. UTC | #11
On Wed, 01 Dec 2021 11:02:59 +0000,
Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> 
> Rather then restructuring the ARMv7M entrly logic per TODO, just move
> NVIC to GENERIC_IRQ_MULTI_HANDLER.
> 
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>

Assuming this will be part of Ard's series:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.
Vladimir Murzin Dec. 2, 2021, 9:33 a.m. UTC | #12
On 12/2/21 9:23 AM, Arnd Bergmann wrote:
> On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> It seems trivial enough to add that for v7m as well, so I will include
>> a patch for it in my v3. But I won't be able to test it, though.
> 
> Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
> Cortex-M based platform and should be able to test this by merging in your
> changes from [1] after you have uploaded the latest version.

I'm happy to test it as well. I'm wondering if there are specific scenarios
(or maybe some tests) to consider?

Cheers
Vladimir 

> 
>         Arnd
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3
>
Arnd Bergmann Dec. 2, 2021, 9:45 a.m. UTC | #13
On Thu, Dec 2, 2021 at 10:33 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>
> On 12/2/21 9:23 AM, Arnd Bergmann wrote:
> > On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> It seems trivial enough to add that for v7m as well, so I will include
> >> a patch for it in my v3. But I won't be able to test it, though.
> >
> > Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
> > Cortex-M based platform and should be able to test this by merging in your
> > changes from [1] after you have uploaded the latest version.
>
> I'm happy to test it as well. I'm wondering if there are specific scenarios
> (or maybe some tests) to consider?

If the IRQ stacks fail in some way, it most likely won't boot at all, so as long
as you can get into user space, I'd be 99% confident that it's good.

For the MMU based platforms, the series lets us detect stack overflows
when either the per-thread stack or the IRQ stack runs into an unmapped
vmalloc page, and there are some unit tests that exercise this.
I don't think you can do this with the MPU though, and certainly not when
the MPU is disabled.

       Arnd
Ard Biesheuvel Dec. 2, 2021, 11:05 a.m. UTC | #14
On Thu, 2 Dec 2021 at 10:45, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Dec 2, 2021 at 10:33 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> >
> > On 12/2/21 9:23 AM, Arnd Bergmann wrote:
> > > On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> It seems trivial enough to add that for v7m as well, so I will include
> > >> a patch for it in my v3. But I won't be able to test it, though.
> > >
> > > Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
> > > Cortex-M based platform and should be able to test this by merging in your
> > > changes from [1] after you have uploaded the latest version.
> >
> > I'm happy to test it as well. I'm wondering if there are specific scenarios
> > (or maybe some tests) to consider?
>
> If the IRQ stacks fail in some way, it most likely won't boot at all, so as long
> as you can get into user space, I'd be 99% confident that it's good.
>
> For the MMU based platforms, the series lets us detect stack overflows
> when either the per-thread stack or the IRQ stack runs into an unmapped
> vmalloc page, and there are some unit tests that exercise this.
> I don't think you can do this with the MPU though, and certainly not when
> the MPU is disabled.
>

Latest version of the code is here, if anyone wants to test it:
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3

Once Mr. Robot confirms that nothing is broken, I'll send it out to the ML.
Vladimir Murzin Dec. 2, 2021, 2:12 p.m. UTC | #15
On 12/2/21 11:05 AM, Ard Biesheuvel wrote:
> On Thu, 2 Dec 2021 at 10:45, Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Thu, Dec 2, 2021 at 10:33 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>>
>>> On 12/2/21 9:23 AM, Arnd Bergmann wrote:
>>>> On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>
>>>>> It seems trivial enough to add that for v7m as well, so I will include
>>>>> a patch for it in my v3. But I won't be able to test it, though.
>>>>
>>>> Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
>>>> Cortex-M based platform and should be able to test this by merging in your
>>>> changes from [1] after you have uploaded the latest version.
>>>
>>> I'm happy to test it as well. I'm wondering if there are specific scenarios
>>> (or maybe some tests) to consider?
>>
>> If the IRQ stacks fail in some way, it most likely won't boot at all, so as long
>> as you can get into user space, I'd be 99% confident that it's good.
>>
>> For the MMU based platforms, the series lets us detect stack overflows
>> when either the per-thread stack or the IRQ stack runs into an unmapped
>> vmalloc page, and there are some unit tests that exercise this.
>> I don't think you can do this with the MPU though, and certainly not when
>> the MPU is disabled.
>>
> 
> Latest version of the code is here, if anyone wants to test it:
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3
> 

I'm afraid it is broken (even w/o last patch). I'm getting

[    0.071350] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
[    0.071747] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
[    0.084497] bad: scheduling from the idle thread!
[    0.084744] CPU: 0 PID: 0 Comm: kworker/0:0H Not tainted 5.16.0-rc1-0f57a2aad139 #2428
[    0.085357] Hardware name: Generic DT based system

$ git bisect log
git bisect start
# good: [fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf] Linux 5.16-rc1
git bisect good fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
# bad: [e42c59caa4d8f37444388cc388bfc55653bf00dc] ARM: v7m: enable support for IRQ stacks
git bisect bad e42c59caa4d8f37444388cc388bfc55653bf00dc
# good: [cbf7b30e70b7e2eacebd885674675746e93583f6] ARM: entry: rework stack realignment code in svc_entry
git bisect good cbf7b30e70b7e2eacebd885674675746e93583f6
# good: [02c0315277721c4fc2082850da0714127594473d] irqchip: nvic: Use GENERIC_IRQ_MULTI_HANDLER
git bisect good 02c0315277721c4fc2082850da0714127594473d
# good: [9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4] ARM: percpu: add SMP_ON_UP support
git bisect good 9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4
# good: [8adfea395669037ee79620b8b534e101d5ce65e6] ARM: smp: defer TPIDRURO update for SMP v6 configurations too
git bisect good 8adfea395669037ee79620b8b534e101d5ce65e6
# bad: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems
git bisect bad 0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d
# first bad commit: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems

At the first glance it seems that __switch_to in entry-v7m.S missing set_current

Cheers
Vladimir

> Once Mr. Robot confirms that nothing is broken, I'll send it out to the ML.
>
Ard Biesheuvel Dec. 2, 2021, 4:46 p.m. UTC | #16
On Thu, 2 Dec 2021 at 15:12, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>
> On 12/2/21 11:05 AM, Ard Biesheuvel wrote:
> > On Thu, 2 Dec 2021 at 10:45, Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> On Thu, Dec 2, 2021 at 10:33 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> >>>
> >>> On 12/2/21 9:23 AM, Arnd Bergmann wrote:
> >>>> On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>>
> >>>>> It seems trivial enough to add that for v7m as well, so I will include
> >>>>> a patch for it in my v3. But I won't be able to test it, though.
> >>>>
> >>>> Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
> >>>> Cortex-M based platform and should be able to test this by merging in your
> >>>> changes from [1] after you have uploaded the latest version.
> >>>
> >>> I'm happy to test it as well. I'm wondering if there are specific scenarios
> >>> (or maybe some tests) to consider?
> >>
> >> If the IRQ stacks fail in some way, it most likely won't boot at all, so as long
> >> as you can get into user space, I'd be 99% confident that it's good.
> >>
> >> For the MMU based platforms, the series lets us detect stack overflows
> >> when either the per-thread stack or the IRQ stack runs into an unmapped
> >> vmalloc page, and there are some unit tests that exercise this.
> >> I don't think you can do this with the MPU though, and certainly not when
> >> the MPU is disabled.
> >>
> >
> > Latest version of the code is here, if anyone wants to test it:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3
> >
>
> I'm afraid it is broken (even w/o last patch). I'm getting
>
> [    0.071350] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> [    0.071747] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
> [    0.084497] bad: scheduling from the idle thread!
> [    0.084744] CPU: 0 PID: 0 Comm: kworker/0:0H Not tainted 5.16.0-rc1-0f57a2aad139 #2428
> [    0.085357] Hardware name: Generic DT based system
>
> $ git bisect log
> git bisect start
> # good: [fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf] Linux 5.16-rc1
> git bisect good fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
> # bad: [e42c59caa4d8f37444388cc388bfc55653bf00dc] ARM: v7m: enable support for IRQ stacks
> git bisect bad e42c59caa4d8f37444388cc388bfc55653bf00dc
> # good: [cbf7b30e70b7e2eacebd885674675746e93583f6] ARM: entry: rework stack realignment code in svc_entry
> git bisect good cbf7b30e70b7e2eacebd885674675746e93583f6
> # good: [02c0315277721c4fc2082850da0714127594473d] irqchip: nvic: Use GENERIC_IRQ_MULTI_HANDLER
> git bisect good 02c0315277721c4fc2082850da0714127594473d
> # good: [9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4] ARM: percpu: add SMP_ON_UP support
> git bisect good 9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4
> # good: [8adfea395669037ee79620b8b534e101d5ce65e6] ARM: smp: defer TPIDRURO update for SMP v6 configurations too
> git bisect good 8adfea395669037ee79620b8b534e101d5ce65e6
> # bad: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems
> git bisect bad 0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d
> # first bad commit: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems
>
> At the first glance it seems that __switch_to in entry-v7m.S missing set_current
>

Thanks for testing, and thanks for bisecting. Unfortunately, I can't
easily test these changes myself.

Could you please check whether the following change would be
sufficient to fix this?

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=tmp

Thanks,
Ard.
Vladimir Murzin Dec. 2, 2021, 6:02 p.m. UTC | #17
On 12/2/21 4:46 PM, Ard Biesheuvel wrote:
> On Thu, 2 Dec 2021 at 15:12, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>
>> On 12/2/21 11:05 AM, Ard Biesheuvel wrote:
>>> On Thu, 2 Dec 2021 at 10:45, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>
>>>> On Thu, Dec 2, 2021 at 10:33 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>>>>
>>>>> On 12/2/21 9:23 AM, Arnd Bergmann wrote:
>>>>>> On Thu, Dec 2, 2021 at 9:35 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>>>
>>>>>>> It seems trivial enough to add that for v7m as well, so I will include
>>>>>>> a patch for it in my v3. But I won't be able to test it, though.
>>>>>>
>>>>>> Ok, perfect. Adding Jesse to Cc, they are in the process of adding another
>>>>>> Cortex-M based platform and should be able to test this by merging in your
>>>>>> changes from [1] after you have uploaded the latest version.
>>>>>
>>>>> I'm happy to test it as well. I'm wondering if there are specific scenarios
>>>>> (or maybe some tests) to consider?
>>>>
>>>> If the IRQ stacks fail in some way, it most likely won't boot at all, so as long
>>>> as you can get into user space, I'd be 99% confident that it's good.
>>>>
>>>> For the MMU based platforms, the series lets us detect stack overflows
>>>> when either the per-thread stack or the IRQ stack runs into an unmapped
>>>> vmalloc page, and there are some unit tests that exercise this.
>>>> I don't think you can do this with the MPU though, and certainly not when
>>>> the MPU is disabled.
>>>>
>>>
>>> Latest version of the code is here, if anyone wants to test it:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm-up-ti-in-task-v3
>>>
>>
>> I'm afraid it is broken (even w/o last patch). I'm getting
>>
>> [    0.071350] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>> [    0.071747] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear)
>> [    0.084497] bad: scheduling from the idle thread!
>> [    0.084744] CPU: 0 PID: 0 Comm: kworker/0:0H Not tainted 5.16.0-rc1-0f57a2aad139 #2428
>> [    0.085357] Hardware name: Generic DT based system
>>
>> $ git bisect log
>> git bisect start
>> # good: [fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf] Linux 5.16-rc1
>> git bisect good fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
>> # bad: [e42c59caa4d8f37444388cc388bfc55653bf00dc] ARM: v7m: enable support for IRQ stacks
>> git bisect bad e42c59caa4d8f37444388cc388bfc55653bf00dc
>> # good: [cbf7b30e70b7e2eacebd885674675746e93583f6] ARM: entry: rework stack realignment code in svc_entry
>> git bisect good cbf7b30e70b7e2eacebd885674675746e93583f6
>> # good: [02c0315277721c4fc2082850da0714127594473d] irqchip: nvic: Use GENERIC_IRQ_MULTI_HANDLER
>> git bisect good 02c0315277721c4fc2082850da0714127594473d
>> # good: [9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4] ARM: percpu: add SMP_ON_UP support
>> git bisect good 9cc24032abc2847a8e61e55e2ec9bd34e1eff1c4
>> # good: [8adfea395669037ee79620b8b534e101d5ce65e6] ARM: smp: defer TPIDRURO update for SMP v6 configurations too
>> git bisect good 8adfea395669037ee79620b8b534e101d5ce65e6
>> # bad: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems
>> git bisect bad 0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d
>> # first bad commit: [0f57a2aad13912dde5746ffc5f43bd46aa3d8b2d] ARM: implement THREAD_INFO_IN_TASK for uniprocessor systems
>>
>> At the first glance it seems that __switch_to in entry-v7m.S missing set_current
>>
> 
> Thanks for testing, and thanks for bisecting. Unfortunately, I can't
> easily test these changes myself.
> 
> Could you please check whether the following change would be
> sufficient to fix this?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=tmp
> 

Booted fine now.

$ cat /proc/interrupts

           CPU0       
 16:         25  nvic_irq   4 Edge      mps2-clkevt
 17:          0  nvic_irq  32 Edge      mps2-uart-rx
 18:       1124  nvic_irq  33 Edge      mps2-uart-tx
 19:          0  nvic_irq  47 Edge      mps2-uart-overrun
Err:          0

Note: 0 interrupts for mps2-uart-rx is expected since it is automated
environment

Cheers
Vladimir

> Thanks,
> Ard.
>
Ard Biesheuvel Dec. 2, 2021, 6:38 p.m. UTC | #18
On Thu, 2 Dec 2021 at 19:02, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>
> On 12/2/21 4:46 PM, Ard Biesheuvel wrote:
> > On Thu, 2 Dec 2021 at 15:12, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
> >>
...
> >> At the first glance it seems that __switch_to in entry-v7m.S missing set_current
> >>
> >
> > Thanks for testing, and thanks for bisecting. Unfortunately, I can't
> > easily test these changes myself.
> >
> > Could you please check whether the following change would be
> > sufficient to fix this?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=tmp
> >
>
> Booted fine now.
>
> $ cat /proc/interrupts
>
>            CPU0
>  16:         25  nvic_irq   4 Edge      mps2-clkevt
>  17:          0  nvic_irq  32 Edge      mps2-uart-rx
>  18:       1124  nvic_irq  33 Edge      mps2-uart-tx
>  19:          0  nvic_irq  47 Edge      mps2-uart-overrun
> Err:          0
>
> Note: 0 interrupts for mps2-uart-rx is expected since it is automated
> environment
>

Excellent, thanks again. I will fold in that change.
Jesse Taube Dec. 2, 2021, 8:27 p.m. UTC | #19
On 12/2/21 13:38, Ard Biesheuvel wrote:
> On Thu, 2 Dec 2021 at 19:02, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>
>> On 12/2/21 4:46 PM, Ard Biesheuvel wrote:
>>> On Thu, 2 Dec 2021 at 15:12, Vladimir Murzin <vladimir.murzin@arm.com> wrote:
>>>>
> ...
>>>> At the first glance it seems that __switch_to in entry-v7m.S missing set_current
>>>>
>>>
>>> Thanks for testing, and thanks for bisecting. Unfortunately, I can't
>>> easily test these changes myself.
>>>
>>> Could you please check whether the following change would be
>>> sufficient to fix this?
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/commit/?h=tmp
>>>
>>
>> Booted fine now.
>>
>> $ cat /proc/interrupts
>>
>>             CPU0
>>   16:         25  nvic_irq   4 Edge      mps2-clkevt
>>   17:          0  nvic_irq  32 Edge      mps2-uart-rx
>>   18:       1124  nvic_irq  33 Edge      mps2-uart-tx
>>   19:          0  nvic_irq  47 Edge      mps2-uart-overrun
>> Err:          0
>>
>> Note: 0 interrupts for mps2-uart-rx is expected since it is automated
>> environment
>>
> 
> Excellent, thanks again. I will fold in that change.
> 
It works for me now aswell!
diff mbox series

Patch

diff --git a/arch/arm/include/asm/v7m.h b/arch/arm/include/asm/v7m.h
index 2cb00d1..4512f7e 100644
--- a/arch/arm/include/asm/v7m.h
+++ b/arch/arm/include/asm/v7m.h
@@ -13,6 +13,7 @@ 
 #define V7M_SCB_ICSR_PENDSVSET			(1 << 28)
 #define V7M_SCB_ICSR_PENDSVCLR			(1 << 27)
 #define V7M_SCB_ICSR_RETTOBASE			(1 << 11)
+#define V7M_SCB_ICSR_VECTACTIVE			0x000001ff
 
 #define V7M_SCB_VTOR			0x08
 
@@ -38,7 +39,7 @@ 
 #define V7M_SCB_SHCSR_MEMFAULTENA		(1 << 16)
 
 #define V7M_xPSR_FRAMEPTRALIGN			0x00000200
-#define V7M_xPSR_EXCEPTIONNO			0x000001ff
+#define V7M_xPSR_EXCEPTIONNO			V7M_SCB_ICSR_VECTACTIVE
 
 /*
  * When branching to an address that has bits [31:28] == 0xf an exception return
diff --git a/arch/arm/kernel/entry-v7m.S b/arch/arm/kernel/entry-v7m.S
index 7bde93c..520dd43 100644
--- a/arch/arm/kernel/entry-v7m.S
+++ b/arch/arm/kernel/entry-v7m.S
@@ -39,14 +39,10 @@  __irq_entry:
 	@
 	@ Invoke the IRQ handler
 	@
-	mrs	r0, ipsr
-	ldr	r1, =V7M_xPSR_EXCEPTIONNO
-	and	r0, r1
-	sub	r0, #16
-	mov	r1, sp
+	mov	r0, sp
 	stmdb	sp!, {lr}
-	@ routine called with r0 = irq number, r1 = struct pt_regs *
-	bl	nvic_handle_irq
+	@ routine called with r0 = struct pt_regs *
+	bl	generic_handle_arch_irq
 
 	pop	{lr}
 	@
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 7038957..488eaa1 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -58,6 +58,7 @@  config ARM_NVIC
 	bool
 	select IRQ_DOMAIN_HIERARCHY
 	select GENERIC_IRQ_CHIP
+	select GENERIC_IRQ_MULTI_HANDLER
 
 config ARM_VIC
 	bool
diff --git a/drivers/irqchip/irq-nvic.c b/drivers/irqchip/irq-nvic.c
index ba4759b..125f9c1 100644
--- a/drivers/irqchip/irq-nvic.c
+++ b/drivers/irqchip/irq-nvic.c
@@ -37,25 +37,12 @@ 
 
 static struct irq_domain *nvic_irq_domain;
 
-static void __nvic_handle_irq(irq_hw_number_t hwirq)
+static void __irq_entry nvic_handle_irq(struct pt_regs *regs)
 {
-	generic_handle_domain_irq(nvic_irq_domain, hwirq);
-}
+	unsigned long icsr = readl_relaxed(BASEADDR_V7M_SCB + V7M_SCB_ICSR);
+	irq_hw_number_t hwirq = (icsr & V7M_SCB_ICSR_VECTACTIVE) - 16;
 
-/*
- * TODO: restructure the ARMv7M entry logic so that this entry logic can live
- * in arch code.
- */
-asmlinkage void __exception_irq_entry
-nvic_handle_irq(irq_hw_number_t hwirq, struct pt_regs *regs)
-{
-	struct pt_regs *old_regs;
-
-	irq_enter();
-	old_regs = set_irq_regs(regs);
-	__nvic_handle_irq(hwirq);
-	set_irq_regs(old_regs);
-	irq_exit();
+	generic_handle_domain_irq(nvic_irq_domain, hwirq);
 }
 
 static int nvic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
@@ -141,6 +128,7 @@  static int __init nvic_of_init(struct device_node *node,
 	for (i = 0; i < irqs; i += 4)
 		writel_relaxed(0, nvic_base + NVIC_IPR + i);
 
+	set_handle_irq(nvic_handle_irq);
 	return 0;
 }
 IRQCHIP_DECLARE(armv7m_nvic, "arm,armv7m-nvic", nvic_of_init);