search

riscv-admin / dev-partners

This repo is for tracking of RISC-V Development Partners Activities
3 stars 0 forks source link

Pointer Masking ACT #15

Open jjscheel opened 1 year ago

jjscheel commented 1 year ago

Technical Group

Pointer Masking TG

ratification-pkg

Pointer Masking

Technical Liaison

Martin Maas, Adam Zabrocki

Task Category

Arch Tests

Task Sub Category

Ratification Target

3Q2023

Statement of Work (SOW)

SOW: link

SOW Signoffs: (delete those not needed)

Waiver

Pull Request Details

jjscheel commented 3 months ago

@UmerShahidengr, any update here?

UmerShahidengr commented 3 months ago

Update June 11th, 2024: The ACTs are available here, its PR has not been added yet as its coverpoint definitions are still missing.

jjscheel commented 3 months ago

Thanks, @UmerShahidengr. When will the coverpoint definitions be drafted and ready for submission with PR?

UmerShahidengr commented 3 months ago

@jjscheel we will target to complete it in the next month.

jjscheel commented 2 months ago

Coverage information from @Adam-pi3:

Definitions

Based on those definitions, we proposed the following test plans (“coverpoints”):

PM works on Physical Address (PA) or Virtual Address (VA) but not on both at the same time. If current mode currently leverages Virtual Memory feature, PM is masking VA. Otherwise PM is enforced on PA. For example, if U-mode code is running under Virtual Memory controlled by S-mode, PM for S and U mode is masking VA. However, if at some point CPU switches to M-mode which operates on physical memory, PM on M-mode would be enforce on PA. The following tests should be executed for PA and VA independently.

  1. Standard LOAD / STORE execution and validate expected results (e.g., read value or generated fault), during:
    1. Normal execution
      1. With "PM enabled" in all "RISC-V modes" [should cover PM function cross {all_lsu_instruction, [bare_mode, nvmpu, hash_mpn], [mmode, smode, umode]} ]
      2. With "PM disabled" in all "RISC-V modes"
    2. Execution in exception context - if normal execution generates an exception and continue execution logic in exception context, verify if PM state is NOT modified.
      1. With "PM enabled" in all "RISC-V modes"
      2. With "PM disabled" in all "RISC-V modes"
    3. Execution in interrupt context - if normal execution is interrupted and continue execution logic in interrupt context, verify if PM state is NOT modified.
      1. With "PM enabled" in all "RISC-V modes"
      2. With "PM disabled" in all "RISC-V modes"
    4. Execution in NMI (with disabled IRQs) - if normal execution is interrupted by NMI with IRQ disabled and continue execution logic in NMI context, verify if PM state is NOT modified.
      1. With "PM enabled" in all "NVRISC-V modes"
      2. With "PM disabled" in all "NVRISC-V modes"
  2. Execute LOAD / STORE and validate expected results (e.g., read value or generated fault), when referencing:
    1. "Aligned access"
      1. With "PM enabled" in all "RISC-V modes"
      2. With "PM disabled" in all "NVRISC-V modes"
    2. "Non-aligned access"
      1. With "PM enabled" in all "RISC-V modes"
      2. With "PM disabled" in all "RISC-V modes"
    3. "Edge" of the page
      1. Crossing page boundary
        1. With "PM enabled" in all "RISC-V modes"
        2. With "PM disabled" in all "RISC-V modes"
      2. Not crossing page boundary
        1. With "PM enabled" in all "RISC-V modes"
        2. With "PM disabled" in all "RISC-V modes"
  3. Initialization values:
    1. CSR PM related registers
  4. Non-FNL SW test plan - Context switch - validate if all PM related CSRs are preserved during context switch when:
    1. All PM related CSRs are enabled in all "RISC-V modes" at the same time
    2. All PM related CSRs are enabled in just one specific mode "RISC-V modes" at the time
    3. - out of scope
jjscheel commented 2 months ago

@UmerShahidengr ^^

Can you please review and comment if this helps with the coverpoint definitions?

UmerShahidengr commented 2 months ago

@jjscheel thank you so much for sharing, I have written some coverpoints based on the proposed testplan. There are some issues in Sail model which is causing some delay. For example, one problem is to define PMM bits in mseccfg register, this register is XLEN bit register, and PMM bits are 33 .. 32. These bits can only be defined for RV64 (as J-extension is only for RV64, not for RV32). There is no way to tell Sail that PMM bitfield is only for RV64 version and not for RV32 version. If we declare mseccfg to be a 64bit register then it does not build for RV32 and other Zicsr tests start failing, otherwise if we declare mseccfg to XLEN bit and build Sail for RV32, then it starts giving Syntax error as bit 32,32 are out of bound for RV32.

We have seen one potential solution applied in Sail code here but this ad-hoc has worked for a bitfield name of a single bit (MBE, which does not exist in RV32 but exists in Rv64), since PMM is multi-bit field so managing this case is becoming complicated.

@billmcspadden-riscv can you please comment on how to address this issue?

martinmaas commented 1 month ago

Thank you so much @UmerShahidengr! Regarding your question:

The approach in the Sail code you linked looks correct to me in this case as well (update the field only when XLEN == 64). What prevents this approach from being used for PMM?

UmerShahidengr commented 1 month ago

Update July 23rd, 2024: Thank you @martinmaas for the detailed comment. We used the same approach and it worked. The Sail PR has already been reviewed, now we are proceeding towards the ACTs

martinmaas commented 1 month ago

This is great to hear! I just took a look at the Sail PR and it looks great – please let me know if and when I should take a look at the ACTs as well. Thank you once again for all the work on this!

jjscheel commented 1 month ago

@martinmaas, if @UmerShahidengr can pull together a RFC PR, that would be easiest to review. But, yes, I think we're close to verifying all tests a feature complete and coverpoints defined. @UmerShahidengr, do you agree?

jjscheel commented 1 month ago

Ooops, I forgot that @UmerShahidengr reported the tests are only about 70% complete. So, @martinmaas, we are not yet ready for a review.

However, we could review the coverpoints file if @Adam-pi3 is willing and @UmerShahidengr will share. That would only leave the testcases when done.

jjscheel commented 1 month ago

@UmerShahidengr, can you create an (RFC) PR for the cover points? They're functionally complete, right? This would allow Martin and Adam to review and comment.

UmerShahidengr commented 1 month ago

Update August 6th, 2024 The ACTs are released as PR. PR can be reviewed here

jjscheel commented 1 month ago

Thanks, @UmerShahidengr. That's exciting.

@martinmaas, @Adam-pi3, can you please take a look at the tests to ensure they map to the proposed coverage from Adam? Please post your confirmation of completeness of the tests and/or raise any issues here.

Once we get the coverage PR, we'll need to 2 more things to proceed to Ratification:

  1. Review of the tests in the coverage reports to ensure it matches Adam's outline. This completes the Freeze waiver.
  2. Review of the information reported to confirm we are passing all tests. This completes the Ratification requirement for testing.
Adam-pi3 commented 1 month ago

I schimmed through the tests and I've been in sync with @UmerShahidengr regarding the functional problem which @UmerShahidengr already fixed. Nevertheless, per my understanding as of now the test covers: 1.i. 2.i 3

there is missing for 1.ii/iii/iv, 2.ii/iii/iv and 4. Did I miss them? Additionally, for the test which we have, I believe we should verify address with and without tag value. For now we only generate addresses without tag value. Example:

jjscheel commented 3 weeks ago

@UmerShahidengr, can you respond to @Adam-pi3 review comments? I'm particularly interested in whether the mentioned gaps exist in the current ACT tests? Are they included in the cover reports?

UmerShahidengr commented 3 weeks ago

@Adam-pi3 There is the inline reply to each coverpoint required:

Standard LOAD / STORE Execution and Validation of Expected Results

Execute LOAD / STORE and Validate Expected Results

Initialization Values

Non-FNL SW Test Plan - Context Switch

martinmaas commented 2 weeks ago

Thanks @UmerShahidengr! I'll let @Adam-pi3 chime in as well, but it makes sense to me that some of the features cannot be supported in the Sail model.

However, I had the same question as Adam:

Additionally, for the test which we have, I believe we should verify address with and without tag value

The part that was unclear to me is where it checks that the address that is being accessed after the masking operation is the correct one. For example, how do we check whether the implementation masked 7 bits or 16 bits? Or is the idea that we will trap on every access and log the address, thus making the masked address part of the signature?

It is not checked within the trap handler if PM state is getting modified or not, but after exiting the trap, it is checked in most of the tests if the state persists or not.

I probably overlooked something really simple, but wasn't able to find where this check is happening. Can you please point me to the logic?

Adam-pi3 commented 2 days ago

Look OK (especially if something is not supported). However, some questions which @martinmaas also pointed out are still open.

jjscheel commented 2 days ago

@UmerShahidengr, I believe you have answers for some of @martinmaas questions. Can you kindly post them for all to see?