Zephyr Shared ISRs

[sambhurst]

This PR implements a feature that allows for the use of shared interrupts in Zephyr. I've only tested it on the arm aarch32. Please see #49379 for more context. Also, I'm not a python programmer, so I'm sure my changes to gen_isr_tables.py can be improved upon.

[microbuilder]

Minor nitpick-ish comments are based on a quick browse through the PR .. I'll do a more thorough review and test when I'm back in the office next week.

[microbuilder]

This feature will also need some sort of test. For example: arm_irq_advanced_features

[galak]

would be nice to detect shared IRQs from devicetree :).

[carlocaione]

My biggest concern here is that this shared IRQ support is done leveraging the direct IRQs instead of regular IRQs (and the IRQ vector table instead of the ISR table). Now, direct IRQs are tricky, serviced in a special way and definitely harder to get them right.

So, why exactly not using the regular ISR table instead of the vector table?

[andyross]

Some notes. I like the feature, no so much the API. What users want is almost certainly "shared interrupts just work" and not "use these special rules to register shared interrupts".
That's the use case, because if the drivers for those shared ISRs are tightly coupled and willing to write special code, they'd be able to just share an ISR between them anyway.

When app authors want "shared IRQs" it's because hardware designers messed up and the software is having to make things work by sharing an interrupt across multiple drivers/modules that are not related and don't want to have to do anything special.

How about CONFIG_KERNEL_SHARED_IRQ instead (and maybe an ARCH_SUPPORT_SHARED_IRQ flag), and when you have that set the kernel automatically knows that sw_isr_table[] entries are linked list records and that it needs to traverse them instead of just running the one it finds?

[sambhurst]

My biggest concern here is that this shared IRQ support is done leveraging the direct IRQs instead of regular IRQs (and the IRQ vector table instead of the ISR table). Now, direct IRQs are tricky, serviced in a special way and definitely harder to get them right.

So, why exactly not using the regular ISR table instead of the vector table?

The regular ISR table pass an argument to the ISR, which can't be used with the shared ISRs because they would need different arguments passed to them.

[sambhurst]

This feature will also need some sort of test. For example: arm_irq_advanced_features

@microbuilder I'm getting error while running the arm_irq_advanced_features tests. I'll upload my test for this PR when I can get the other tests to pass.

[sambhurst]

@tejlmand could you take another look? Thanks

[sambhurst]

@tejlmand @carlocaione Could you take another look? Thanks

[Cherish-Gww]

@sambhurst
I found that in zephyr_pre1.elf stage struct device address is different with zephyr.elf
here is zephyr_pre1.elf:
1685: 0800be44 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_43
1723: 0800bea4 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_23
1919: 0800beec 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_53
1974: 0800be8c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_73
2064: 0800be5c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_75
2154: 0800be2c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_8
2228: 0800bf64 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_103
2256: 0800bf34 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_99
2321: 0800bebc 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_27
2417: 0800be74 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_74
2463: 0800bed4 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_25
2575: 0800bf4c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_34
2592: 0800bf1c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_102
2614: 0800bf04 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_13

and this is zephyr.elf
1673: 0800be7c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_43
1711: 0800bedc 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_23
1907: 0800bf24 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_53
1962: 0800bec4 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_73
2052: 0800be94 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_75
2142: 0800be64 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_8
2216: 0800bf9c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_103
2244: 0800bf6c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_99
2309: 0800bef4 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_27
2406: 0800beac 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_74
2453: 0800bf0c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_25
2565: 0800bf84 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_34
2582: 0800bf54 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_102
2605: 0800bf3c 24 OBJECT GLOBAL DEFAULT 5 __device_dts_ord_13

struct device address has been changed, shared interrupt point to zephyr.pre1.elf address.

this is isr_tables.c

const uintptr_t shared_irq_21_args[2]  __attribute__((section(".shared_irq_arg"))) = {
  0x800bf1c,
  0x800bf34,
};
const uintptr_t shared_irq_22_args[2]  __attribute__((section(".shared_irq_arg"))) = {
  0x800bf1c,
  0x800bf34,
};
__attribute__((section(".shared_irq"))) void shared_irq_21(const void *args){
  ((ISR)0x8004db1)((const void *)((uintptr_t *)args)[0]);
  ((ISR)0x8004db1)((const void *)((uintptr_t *)args)[1]);
}
__attribute__((section(".shared_irq"))) void shared_irq_22(const void *args){
  ((ISR)0x8004d11)((const void *)((uintptr_t *)args)[0]);
  ((ISR)0x8004d11)((const void *)((uintptr_t *)args)[1]);
}

I have tested this patch in newest zephyr, the struct device does have offset.
and I found the device address offset caused by the increase if the .text section, so the device address need add .text increase size is real device address.

update:
I think .shared_irq and .shared_irq_arg need put in a standalone section after device section, so that could`t influence device address.

here is my solution:
move .shared_irq and .shared_irq.arg section to sw_isr_table. beacuse sw_isr_table is blew in device section.

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] rom_start         PROGBITS        08000000 0000d4 0002cc 00 WAX  0   0  4
  [ 2] text              PROGBITS        080002cc 0003a0 004ab0 00  AX  0   0  4
  [ 3] .ARM.exidx        ARM_EXIDX       08004d7c 004e50 000008 00  AL  2   0  4
  [ 4] initlevel         PROGBITS        08004d84 004e58 0000b0 00   A  0   0  4
  [ 5] devices           PROGBITS        08004e34 004f08 000198 00   A  0   0  4
  [ 6] sw_isr_table      PROGBITS        08004fcc 0050a0 000558 00 WAX  0   0  4

in file zephyr/cmake/linker_script/common/common-rom.cmake Line: 26 Add

  if (CONFIG_ISR_SHARE_IRQ)
    zephyr_linker_section_configure(SECTION sw_isr_table INPUT ".shared_irq")
    zephyr_linker_section_configure(SECTION sw_isr_table INPUT ".shared_irq_arg")
  endif()

and in file zephyr/include/zephyr/linker/common-rom/common-rom-kernel-devices.ld Line: 47 Add

		*(.shared_irq)
        	*(.shared_irq_arg)

then I got same struct device address. and do not forget remove put them in text section code.

[Cherish-Gww]

.text section is before .device, in final link, it will change .device section address.

[Cherish-Gww]

Move .shared_irq and shared_irq_arg to .sw_isr_table will not influence the .device address

[carlocaione]

this is not possible. The code dealing with the shared interrupt must be somewhere in the .text section, see #49427 (review). If not, must be in a section with the correct settings for eXecution for MMU / MPU.

[Cherish-Gww]

this is not possible. The code dealing with the shared interrupt must be somewhere in the .text section, see #49427 (review). If not, must be in a section with the correct settings for eXecution for MMU / MPU.

Put text section blew device is it possible?

[carlocaione]

Not quite sure I understood the question. If you are asking whether the current placement of the shared_irq code in the text section is correct then it is not. The shifting of the addresses was a problem already seen in the past, see #49427 (review)

[carlocaione]

We need to have:

• The shared_irq code in a section that is mapped as eXecutable (.text is a natural choice, but I'm open to anything really) because the script is generating code at build time that (of course) needs to be eXecuted.
• As pointed out by @tejlmand the shifting addresses are allowed only between stage 0 and 1. Having shifting addresses between other linking stages could be problematic.

[Cherish-Gww]

We need to have:

• The shared_irq code in a section that is mapped as eXecutable (.text is a natural choice, but I'm open to anything really) because the script is generating code at build time that (of course) needs to be eXecuted.
• As pointed out by @tejlmand the shifting addresses are allowed only between stage 0 and 1. Having shifting addresses between other linking stages could be problematic.

Here's my opinion:

• The code does not have to be in the text section.
• sw_isr_table link stage is below text and device, so that function in text section address and device in device section address will not be change.

so put shared_irq and shared_irq_arg to sw_isr_table will not effect.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[erwango]

@sambhurst For my information, do you plan to complete this PR ?

[sambhurst]

@erwango I'd like to finish this PR but there doesn't seem to be a general agreement on how this feature should be implemented. To simplify the effort, I proposed to just implement and test it for Arm Cortex devices and later roll it out to other devices, but this proposal wasn't well received. So, unless we can arrive at a consensus, we can close this PR.

[Cherish-Gww]

@erwango I'd like to finish this PR but there doesn't seem to be a general agreement on how this feature should be implemented. To simplify the effort, I proposed to just implement and test it for Arm Cortex devices and later roll it out to other devices, but this proposal wasn't well received. So, unless we can arrive at a consensus, we can close this PR.

Did this issue come up in your test? and my solution is right?
#49427 (comment)

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[aurel32]

I would like to get shared IRQ working, it's important for some MCU. For what I understood the remaining issues are:

• Device addresses change between linking stages due to shared IRQ handlers being added for the text section
• The shared IRQ handlers must be in the text section as they need to be executable. This is the root of the issues on some other architectures (e.g. arm64)

Both issues seems to contradict themselves, therefore I have just tried an alternative way with a common shared irq handler getting passed the data, i.e. both argument and function. For that I reused the existing _isr_table_entry struct, but that can be changed. The common shared irq handler is mark as KEEP in the linker file to avoid moving symbols between linking stages, as it is not referenced in the first linking stage. It seems to work fine on a Cortex-M CPU. My WIP code is there: aurel32@fedbe2b

Before continuing in that direction, and then getting things working on at least a second architecture, I would like to know if that strategy is acceptable? Things like naming, comments and so on, can probably be improved, but before investing time on that I would like to get some feedback about the big picture.

[Cherish-Gww]

• The shared IRQ handlers must be in the text section as they need to be executable. This is the root of the issues on some other architectures (e.g. arm64)

I agree with your patch,
an independent section needs to declare its executable permissions in the MPU,
This may be the reason for solving the above problem on arm64.
This requires modifying all used MPU codes in the soc directory.
I think the best solution is link struct device before text.

[carlocaione]

I would like to know if that strategy is acceptable?

Please, open a (different) PR with your code so people can have a chance to take a look and we can check if CI is happy with this solution as well.

[Cherish-Gww]

I would like to know if that strategy is acceptable?

Please, open a (different) PR with your code so people can have a chance to take a look and we can check if CI is happy with this solution as well.

@carlocaione @aurel32
I did not submit a PR, here are my thoughts, I'm just a little white, please don't mock me:

    #include <zephyr/linker/common-rom.ld>        /* Move common-rom.ld before text section linking */
 
    SECTION_PROLOGUE(_TEXT_SECTION_NAME,,)
	{
	__text_region_start = .;
#include <zephyr/linker/kobject-text.ld>
	*(.text)
	*(".text.*")
	*(".TEXT.*")
	*(.gnu.linkonce.t.*)
        KEEP(*(.shared_irq))

I have tried on qemu_cortex_m3 board, hello_world runs well.

the elf section header is:
Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] rom_start         PROGBITS        00000000 0000d4 0000ec 00 WAX  0   0  4
  [ 2] initlevel         PROGBITS        000000ec 0001c0 000038 00   A  0   0  4
  [ 3] devices           PROGBITS        00000124 0001f8 000048 00   A  0   0  4  /* struct device section */
  [ 4] sw_isr_table      PROGBITS        0000016c 000240 000158 00  WA  0   0  4
  [ 5] device_handles    PROGBITS        000002c4 000398 000012 00   A  0   0  2
  [ 6] log_const_se[...] PROGBITS        000002d8 0003ac 000010 00   A  0   0  4
  [ 7] text              PROGBITS        000002e8 0003bc 001e20 00  AX  0   0  4  /* the text section */
  [ 8] .ARM.exidx        ARM_EXIDX       00002108 0021dc 000008 00  AL  7   0  4    
  [ 9] rodata            PROGBITS        00002110 0021e4 0000ec 00   A  0   0  4
  [10] .ramfunc          PROGBITS        20000000 0022fe 000000 00   W  0   0  1
  [11] datas             PROGBITS        20000000 0022d0 000024 00  WA  0   0  4
  [12] device_states     PROGBITS        20000024 0022f4 000006 00  WA  0   0  1
  [13] bss               NOBITS          20000030 002300 00023e 00  WA  0   0  8
  [14] noinit            NOBITS          20000270 002300 000d00 00  WA  0   0  8
  [15] .comment          PROGBITS        00000000 0022fe 000020 01  MS  0   0  1
  [16] .debug_aranges    PROGBITS        00000000 002320 000ad0 00      0   0  8
  [17] .debug_info       PROGBITS        00000000 002df0 026c4c 00      0   0  1
  [18] .debug_abbrev     PROGBITS        00000000 029a3c 005a98 00      0   0  1
  [19] .debug_line       PROGBITS        00000000 02f4d4 00c9e4 00      0   0  1
  [20] .debug_frame      PROGBITS        00000000 03beb8 001618 00      0   0  4
  [21] .debug_str        PROGBITS        00000000 03d4d0 00452c 01  MS  0   0  1
  [22] .debug_loc        PROGBITS        00000000 0419fc 00ba68 00      0   0  1
  [23] .debug_ranges     PROGBITS        00000000 04d468 001f90 00      0   0  8
  [24] .ARM.attributes   ARM_ATTRIBUTES  00000000 04f3f8 00002b 00      0   0  1
  [25] .last_section     PROGBITS        00002226 0022fa 000004 00   A  0   0  1
  [26] .symtab           SYMTAB          00000000 04f424 0033c0 10     27 364  4
  [27] .strtab           STRTAB          00000000 0527e4 00300f 00      0   0  1
  [28] .shstrtab         STRTAB          00000000 0557f3 000132 00      0   0  1

@aurel32 use KEEP in link script make text size not change is the best way, not my thought.

[aurel32]

I would like to know if that strategy is acceptable?

Please, open a (different) PR with your code so people can have a chance to take a look and we can check if CI is happy with this solution as well.

@carlocaione @aurel32 I did not submit a PR, here are my thoughts, I'm just a little white, please don't mock me:

    #include <zephyr/linker/common-rom.ld>        /* Move common-rom.ld before text section linking */
 
    SECTION_PROLOGUE(_TEXT_SECTION_NAME,,)
	{
	__text_region_start = .;
#include <zephyr/linker/kobject-text.ld>
	*(.text)
	*(".text.*")
	*(".TEXT.*")
	*(.gnu.linkonce.t.*)
        KEEP(*(.shared_irq))

I have tried on qemu_cortex_m3 board, hello_world runs well.

Thanks for testing ! On my side I forgot to mention here that I opened a PR as requested, with the test passing on all tested platforms, that is: qemu_cortex_m0 qemu_cortex_m3 qemu_cortex_a9 qemu_cortex_a53 qemu_cortex_a53_smp

[Cherish-Gww]

@aurel32
Your solution is more reasonable

[sambhurst]

@aurel32 Thanks for picking this up.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.