Skip to content

Conversation

@hirooih
Copy link
Contributor

@hirooih hirooih commented Nov 22, 2025

This PR sets the NAPOT range size considering the minimum PMP granularity supported by the hart.

This PR is separate from #632 as suggested by @en-sc .

The issue this PR solves.

  • set_pmp_deny() is called by two tests, MemTestReadInvalid() and EtriggerTest(), with the argument bad_address.
  • The default value of size for set_pmp_deny() is 4 KB.
  • MemTestReadInvalid() requires 4 bytes and EtriggerTest() requires 1 byte for size, but they use the default value (4 KB).
  • From debug/targets/RISC-V/spike32.py and debug/targets/RISC-V/spike64.py:
    ram = 0x1010_0000      // '_' is added
   ...
    bad_address = ram - 8  // 0x100f_fff8
  • From testlib.py L1496
        self.gdb.p("$pmpaddr0="
                       f"0x{((address >> 2) | ((size - 1) >> 3)):x}")

pmpaddr0 is set to 0x100f_ffff >> 2 (1MB NAPOT range starting 0x1000_0000).

How this PR works

  • The argument size of set_pmp_deny() now has no default value. The caller sets it to the value required by the test.
  • The NAPOT range size is calculated as the minimum size required by the size argument and the minimum supported PMP granularity.
  • set_pmp_deny() raises a TestNotApplicable error if the value of the argument address is inconsistent with the calculated size. This error allows users to set bad_address to a proper value.

@hirooih
Copy link
Contributor Author

hirooih commented Nov 22, 2025

@en-sc,

Thank you for your suggestions. Let me ask you some about your concern.

From #632 (comment):

size is based on the result of communication with the target -- it can change sporadically.
Therefore this test can sporadically be marked as not applicable.

I think this is not avoidable. The tests implemented now require at most 4 bytes. A test which requires a large size may be added in future. In this case the maintainer of targets may have to change the value of bad_address.

  1. minimum_pmp_granularity is a target property specified in the config.

What is the difference between the value of minimum_pmp_granularity and the return value of get_minimum_pmp_granularity() I proposed? I don't understand what issue this property solves.

If we introduce the target property minimum_pmp_granularity, we have to set it for each target under targets/.
I don't have enough information for each target.

@en-sc
Copy link
Collaborator

en-sc commented Nov 24, 2025

What is the difference between the value of minimum_pmp_granularity and the return value of get_minimum_pmp_granularity() I proposed?

Imagine the read of pmpaddr0 gets corrupted (e.g. HW bug). This may cause the test to be marked as not applicable -- and the testsuite will pass without failures. This is not correct.

If we introduce the target property minimum_pmp_granularity, we have to set it for each target under targets/.
I don't have enough information for each target.

AFAIU, minimum_pmp_granularity only makes sense for the case of support_set_pmp_deny = True -- there are no such targets in the repo.
As for the custom targets -- once the user will get an exception when running the testsuite, I feel like he will understand what is the cause. After all the property can be found in a couple runs of the test that checks it with no prior knowledge of the target. I don't think it's too much to ask.

In any case -- If you don't feel like doing it, IMHO the patch can be accepted as-is. It is a great improvement.

@hirooih
Copy link
Contributor Author

hirooih commented Nov 24, 2025

@en-sc, thank you for your feedback.

Imagine the read of pmpaddr0 gets corrupted (e.g. HW bug). This may cause the test to be marked as not applicable -- and the testsuite will pass without failures. This is not correct.

AFAIU, minimum_pmp_granularity only makes sense for the case of support_set_pmp_deny = True -- there are no such targets in the repo.

Both make sense to me. Now I understand.

Give me some more days.

- set size considering the minimum PMP granularity supported by the hart.

Signed-off-by: Hiroo HAYASHI <[email protected]>
@hirooih hirooih force-pushed the debug-enhance-set_pmp_deny branch from 9bf5a2f to cba3b86 Compare November 25, 2025 15:17
@hirooih
Copy link
Contributor Author

hirooih commented Nov 25, 2025

@en-sc,

I have updated this PR.
Could you review this again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants