| 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 |
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 >
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
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 >
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
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 >
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
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 >
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.
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.
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
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.
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 >
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
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.
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. >
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.
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. >
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.
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 --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);
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(-)