drivers: intc: plic: Fix memory-mapped register offset calculation #65283

p-woj · 2023-11-16T09:34:33Z

This MR fixes register offset calculation in the PLIC driver.

ffb8f31 removed a pointer-arithmetic-induced multiplication of the calculated register offset. As a result the register offsets are 4 times too small. For example, riscv_plic_irq_enable(10251) with 2nd level interrupts enabled and the default 8-bit size currently writes to offset 0x1 instead of 0x4.

Only interrupt numbers above 0x1f are affected.

Fixes: #65321

fkokosinski · 2023-11-16T10:23:22Z

Assigned myself because this is a RISC-V related change.

fkokosinski · 2023-11-16T10:25:19Z

@ycsin - could you take a look? It seems that it reverts some changes that you've introduced,

ycsin

Good catch! Thanks for the fix

had another look

ycsin

I had another look and thought that this fix changes the original intention of the function to calculate the index

What do you think if we have another function to do the conversion, i.e.:

static inline uint32_t offset_to_mem_addr(uint32_t offset)
{
	return offset * sizeof(uint32_t);
}

and apply that to every mem_addr_t arithmetics as necessary?

p-woj · 2023-11-16T12:49:58Z

Thanks for the suggestion. I also adapted existing uses of the same pattern in plic_init and riscv_plic_set_priority.

cfriedt · 2023-11-16T12:57:15Z

drivers/interrupt_controller/intc_plic.c

@@ -115,7 +120,8 @@ static inline const struct device *get_plic_dev_from_irq(uint32_t irq)
 static int riscv_plic_is_edge_irq(const struct device *dev, uint32_t local_irq)
 {
 	const struct plic_config *config = dev->config;
-	mem_addr_t trig_addr = config->trig + local_irq_to_reg_offset(local_irq);


Shouldn't we just fix local_irq_to_reg_offset() in this PR? Offset is in bytes, so we're basically leaving a land mine for whoever hits this problem next time.

Also, kind of wondering how that even passed CI... we have tests, right? Is it a build-only test? Would maybe explain how local_irq_to_reg_offset() was somehow skipped.

I think what @ycsin meant is that reg_offset is the offset expressed in the number of registers.

As for tests, I'm not sure if these run in CI, but on the QEMU virt platforms the uart (the only device connected to the plic) interrupt number is small enough such that the calculated offset was (correctly) 0, so the lacking multiplication was of course irrelevant.

Maybe a misnomer - reg_index would be better in the case.

ycsin

If you dont mind, could you please fix the naming of get_claim_complete_offset & get_threshold_priority_offset to _addr as well in this PR (as another commit) so that things will be consistent in the driver?

p-woj · 2023-11-16T14:59:07Z

Thanks for the suggestions - I fixed the naming of these functions and also changed "offset" to "index" in reg_offset for clarity.

cfriedt · 2023-11-16T15:07:18Z

Just (another) suggestion

diff --git a/drivers/interrupt_controller/intc_plic.c b/drivers/interrupt_controller/intc_plic.c
index 41ee75823b..0fe7b902d0 100644
--- a/drivers/interrupt_controller/intc_plic.c
+++ b/drivers/interrupt_controller/intc_plic.c
@@ -59,11 +59,16 @@ struct plic_config {
 static uint32_t save_irq;
 static const struct device *save_dev;
 
-static inline uint32_t local_irq_to_reg_offset(uint32_t local_irq)
+static inline uint32_t local_irq_to_reg_index(uint32_t local_irq)
 {
        return local_irq >> LOG2(PLIC_REG_SIZE);
 }
 
+static inline uint32_t local_irq_to_reg_offset(uint32_t local_irq)
+{
+       return local_irq_to_reg_index(local_irq) * sizeof(uint32_t);
+}
+
 static inline uint32_t get_plic_enabled_size(const struct device *dev)
 {
        const struct plic_config *config = dev->config;

cfriedt · 2023-11-16T15:14:40Z

If you dont mind, could you please fix the naming of get_claim_complete_offset & get_threshold_priority_offset to _addr as well in this PR (as another commit) so that things will be consistent in the driver?

@p-woj - would also be good to change get_claim_complete_addr() and get_threshold_priority_offset() to return type mem_addr_t so that the type assignment is consistent.

Honestly, not 100% sure about the utility of using mem_addr_t (i.e. uintptr_t) at all in this context, and we shouldn't be defining new types unnecessarily. We should use a pointer type when referring to a memory-mapped location in the address space.

Using integer types when referring to memory locations really only makes sense in a limited number of circumstances (e.g. referring to a location outside of the memory-mapped address space). Here, I think it's being used to circumvent having to deal with pointer arithmetic (which was a mistake we have just paid for).

@fkokosinski - what are your thoughts there?

`get_claim_complete_offset` and `get_threshold_priority_offset` actually return addresses directly. Rename them to `_addr` for consistency within the driver. Also change their return type to `mem_addr_t`. Signed-off-by: Piotr Wojnarowski <pwojnarowski@antmicro.com>

Previously, the PLIC's registers were accessed through uint32_t *, so all calculated offsets were effectively multiplied by sizeof(uint32_t). Do the same manuallly now that we have mem_addr_t/sys_read32. Signed-off-by: Piotr Wojnarowski <pwojnarowski@antmicro.com>

p-woj · 2023-11-16T15:23:41Z

Thanks, that's a much better approach. Now has the types fixed as well

cfriedt

Works for me - It looks like mem_addr_t is baked in already way up the stack, so save that for another time.

cfriedt · 2023-11-16T15:29:16Z

@ycsin @p-woj - just for our own peace of mind - is this actually getting run through some tests? If not, we should add one in this PR as a second commit.

p-woj · 2023-11-16T16:50:31Z

I don't think it is currently covered by any tests.

We need this small patch to enable running ROBOT_FILES=shell_module.robot west build -p -b mpfs_icicle -s samples/subsys/shell/shell_module -t run_renode_test to test this regression. IIUC to have Twister run it automatically we'd have to enable it as a default platform for tests, so I think this might be something for a separate PR.

diff --git a/boards/riscv/mpfs_icicle/board.cmake b/boards/riscv/mpfs_icicle/board.cmake
index 0fed4848fa..1f3346da5a 100644
--- a/boards/riscv/mpfs_icicle/board.cmake
+++ b/boards/riscv/mpfs_icicle/board.cmake
@@ -2,3 +2,4 @@
 
 set(SUPPORTED_EMU_PLATFORMS renode)
 set(RENODE_SCRIPT ${CMAKE_CURRENT_LIST_DIR}/support/mpfs250t.resc)
+set(RENODE_UART sysbus.mmuart0)

mpfs_icicle is used here because it's one of the platforms where we originally found this regression and it already has emulation support.

cfriedt · 2023-11-16T19:31:07Z

I think this might be something for a separate PR.

We definitely want to have the test that verifies the fix in the same PR as the fix.

You will need to create an issue as well (if one doesn't already exist), since it's a regression. Not sure if this needs to be backported.

@ycsin - can you suggest a way to test this on a possibly alternate platform? Does the fix need to be backported to v3.5-branch?

cfriedt

Needs test / issue

cfriedt · 2023-11-17T01:18:44Z

One way to easily test this (without relying on a particular hardware configuration) is to simply do some whitebox testing.

I.e. in intc_plic.c, do something like this:

#ifdef CONFIG_TEST_INTC_PLIC
#define INTC_PLIC_STATIC
#else
#define INTC_PLIC_STATIC static
#endif

In tests/drivers/build_all/interrupt_controller/intc_plic/Kconfig add something like

config TEST_INTC_PLIC
	bool
	default y
	help
	   Declare some intc_plic.c functions in the global scope for verification.

Remove the definition of main() from tests/drivers/build_all/interrupt_controller/intc_plic/src/main.c
Create a proper ZTest testsuite
Re-declare the necessary intc_plic.c function prototypes locally
Remove build_only: true from testcase.yaml

p-woj · 2023-11-17T15:44:31Z

I kept the existing build-only tests as they were, since the build_all directory has only build_only tests, and added a separate ZTest suite which is very whitebox - it tests local_irq_to_reg_index and local_irq_to_reg_offset directly. Please take a look.

Add a test to verify that the regression from ffb8f31 is fixed. Signed-off-by: Piotr Wojnarowski <pwojnarowski@antmicro.com>

cfriedt · 2023-11-17T17:55:46Z

I kept the existing build-only tests as they were, since the build_all directory has only build_only tests, and added a separate ZTest suite which is very whitebox - it tests local_irq_to_reg_index and local_irq_to_reg_offset directly. Please take a look.

I will defer to @ycsin's review

ycsin

LGTM

p-woj requested review from dcpleung and nashif as code owners November 16, 2023 09:34

zephyrbot added the area: Interrupt Controller label Nov 16, 2023

p-woj force-pushed the plic_width branch from 2841775 to 94e490a Compare November 16, 2023 09:42

kgugala added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Nov 16, 2023

fkokosinski mentioned this pull request Nov 16, 2023

MAINTAINERS: add the PLIC interrupt controller driver to the RISC-V area #65286

Merged

fkokosinski self-assigned this Nov 16, 2023

ycsin requested review from cfriedt, jgl-meta and ycsin November 16, 2023 12:00

ycsin previously approved these changes Nov 16, 2023

View reviewed changes

ycsin added bug The issue is a bug, or the PR is fixing a bug Regression Something, which was working, does not anymore labels Nov 16, 2023

ycsin reviewed Nov 16, 2023

View reviewed changes

p-woj force-pushed the plic_width branch from 94e490a to 191d0d7 Compare November 16, 2023 12:49

cfriedt reviewed Nov 16, 2023

View reviewed changes

ycsin previously approved these changes Nov 16, 2023

View reviewed changes

p-woj dismissed ycsin’s stale review via af45a34 November 16, 2023 14:58

p-woj force-pushed the plic_width branch from 191d0d7 to af45a34 Compare November 16, 2023 14:58

p-woj added 2 commits November 16, 2023 16:17

p-woj force-pushed the plic_width branch from af45a34 to 74a1554 Compare November 16, 2023 15:22

cfriedt approved these changes Nov 16, 2023

View reviewed changes

cfriedt requested changes Nov 16, 2023

View reviewed changes

p-woj mentioned this pull request Nov 16, 2023

PLIC interrupt configuration doesn't work for interrupt numbers above 0x1f #65321

Closed

zephyrbot requested review from carlocaione, edersondisouza, fkokosinski, katsuster, kgugala, mgielda and tgorochowik November 17, 2023 15:45

tests: drivers: intc_plic: Add tests for register index and offset

33e5145

Add a test to verify that the regression from ffb8f31 is fixed. Signed-off-by: Piotr Wojnarowski <pwojnarowski@antmicro.com>

p-woj force-pushed the plic_width branch from c4a6fbd to 33e5145 Compare November 17, 2023 15:48

ycsin approved these changes Nov 18, 2023

View reviewed changes

cfriedt approved these changes Nov 18, 2023

View reviewed changes

fkokosinski approved these changes Nov 20, 2023

View reviewed changes

carlescufi merged commit 6670dbe into zephyrproject-rtos:main Nov 20, 2023
18 checks passed

p-woj deleted the plic_width branch November 20, 2023 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: intc: plic: Fix memory-mapped register offset calculation #65283

drivers: intc: plic: Fix memory-mapped register offset calculation #65283

p-woj commented Nov 16, 2023 •

edited

fkokosinski commented Nov 16, 2023

fkokosinski commented Nov 16, 2023

ycsin left a comment

ycsin left a comment

p-woj commented Nov 16, 2023

cfriedt Nov 16, 2023

p-woj Nov 16, 2023 •

edited

cfriedt Nov 16, 2023

ycsin left a comment

p-woj commented Nov 16, 2023

cfriedt commented Nov 16, 2023 •

edited

cfriedt commented Nov 16, 2023 •

edited

p-woj commented Nov 16, 2023

cfriedt left a comment

cfriedt commented Nov 16, 2023

p-woj commented Nov 16, 2023

cfriedt commented Nov 16, 2023 •

edited

cfriedt left a comment

cfriedt commented Nov 17, 2023

p-woj commented Nov 17, 2023

cfriedt commented Nov 17, 2023

ycsin left a comment

drivers: intc: plic: Fix memory-mapped register offset calculation #65283

drivers: intc: plic: Fix memory-mapped register offset calculation #65283

Conversation

p-woj commented Nov 16, 2023 • edited

fkokosinski commented Nov 16, 2023

fkokosinski commented Nov 16, 2023

ycsin left a comment

Choose a reason for hiding this comment

ycsin left a comment

Choose a reason for hiding this comment

p-woj commented Nov 16, 2023

cfriedt Nov 16, 2023

Choose a reason for hiding this comment

p-woj Nov 16, 2023 • edited

Choose a reason for hiding this comment

cfriedt Nov 16, 2023

Choose a reason for hiding this comment

ycsin left a comment

Choose a reason for hiding this comment

p-woj commented Nov 16, 2023

cfriedt commented Nov 16, 2023 • edited

cfriedt commented Nov 16, 2023 • edited

p-woj commented Nov 16, 2023

cfriedt left a comment

Choose a reason for hiding this comment

cfriedt commented Nov 16, 2023

p-woj commented Nov 16, 2023

cfriedt commented Nov 16, 2023 • edited

cfriedt left a comment

Choose a reason for hiding this comment

cfriedt commented Nov 17, 2023

p-woj commented Nov 17, 2023

cfriedt commented Nov 17, 2023

ycsin left a comment

Choose a reason for hiding this comment

p-woj commented Nov 16, 2023 •

edited

p-woj Nov 16, 2023 •

edited

cfriedt commented Nov 16, 2023 •

edited

cfriedt commented Nov 16, 2023 •

edited

cfriedt commented Nov 16, 2023 •

edited