Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pmm Support #454

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

HAMZA-AFZAL404
Copy link

RISC-V Pointer Masking Extension Implementation

This PR features the implementation of the Pointer Masking Extension for the RISC-V architecture, enhancing the Base, Floating Point, Compressed, Vector and Atomics Instruction Set.

Changes

  1. Implemented the mseccfg register with associated changes in riscv_sys_regs.sail ,riscv_insts_zicsr.sail ,riscv_sys_control.sail ,riscv_csr_map.sail

  2. Updated the leglize_envcfg function and added functionsis_pmm_active ,legalize_mseccfg and get_pmm in riscv_sys_regs.sail.

  3. Introduced functions transform_VA and transform_PA in riscv_insts_base.sail.

  4. Implemented the function transform_effective_address. and Integrated transform_effective_address into the execution of LOAD and STORE functions of the targeted instruction files.

Testing

  1. The functionality has been verified with several tests. However, testing is still ongoing. Once completed, the repository link will be shared promptly.

  2. The self-checking tests have not been written yet. However, work has commenced on them and they should be available shortly.

c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link to the specification? Sorry, I might be being blind but I can't seem to find one that matches this!

Also this needs to be rebased, and the bitfield field access syntax has changed: mseccfg.PMM() -> mseccfg[PMM].

c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
if haveAtomics() then {
/* Get the address, X(rs1) (no offset).
* Extensions might perform additional checks on address validity.
*/
match ext_data_get_addr(rs1, zeros(), Read(Data), width) {
Ext_DataAddr_Error(e) => { ext_handle_data_check_error(e); RETIRE_FAIL },
Ext_DataAddr_OK(vaddr) => {
let vaddr_ = transform_effective_address(vaddr, pmm);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this go inside translateAddr? I think the only reason it couldn't is if transform_effective_address() would affect alignment but that sounds like it shouldn't be possible? If the address given to handle_mem_exception() needs to be different I would say add that as a field of TR_Failure.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to do this, but it will complicate the code. But what if the code is misaligned?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure because I haven't seen the specification. Please can you link to it, ideally in the top message?

model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
@Timmmm
Copy link
Collaborator

Timmmm commented Sep 4, 2024

Please can you link to the specification that this is intended to implement?

@martinmaas
Copy link

https://github.com/riscv/riscv-j-extension/blob/master/zjpm-spec.pdf

then { handle_mem_exception(vaddr, E_SAMO_Addr_Align()); RETIRE_FAIL }
else match translateAddr(vaddr, ReadWrite(Data, Data)) {
TR_Failure(e, _) => { handle_mem_exception(vaddr, e); RETIRE_FAIL },
let vaddr_ = transform_effective_address(vaddr, pmm);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let vaddr_ = transform_effective_address(vaddr, pmm);
let vaddr = transform_effective_address(vaddr, pmm);

Should reduce the diff.

Copy link
Collaborator

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've read the spec and this seems mostly ok. I've added some more comments.

I think the major thing is that the handling of pointer masking should be inside translateAddr. I checked very briefly and I think if you exclude Execute accesses then it matches the situations where pointer masking should be applied. (Double check that though - apart from fetches are there any translateAddr() calls that you didn't have to add pointer masking to?

IIRC pointer masking isn't applied to the xtval address (passed into handle_mem_exception()). To deal with that, add an address field to TR_Failure. Something like:

      if   check_misaligned(vaddr, width)
      then { handle_mem_exception(vaddr, E_Load_Addr_Align()); RETIRE_FAIL }
      else match translateAddr(vaddr, Read(Data)) {
        TR_Failure(xtval_addr, e, _) => { handle_mem_exception(xtval_addr, e); RETIRE_FAIL },

c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
handwritten_support/riscv_extras.lem Outdated Show resolved Hide resolved
model/riscv_insts_base.sail Show resolved Hide resolved
model/riscv_insts_base.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Outdated Show resolved Hide resolved
model/riscv_sys_regs.sail Show resolved Hide resolved
@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 11, 2024

Putting pointer masking inside translateAddr would compose poorly with DDC on CHERI, which should authorise the actual masked virtual address used.

@arichardson
Copy link
Collaborator

We already have ext_data_get_addr which can be used for the CHERI DDC hooks and I presume should also apply in all these cases for pointer masking. I recall the virtual memory managemtn instructions are explicitly opted out in the spec for some reason, but that should be easier to handle than adding yet another hook.

@Timmmm
Copy link
Collaborator

Timmmm commented Sep 12, 2024

Ah yeah we don't call translateAddr() for sfence.vma and friends so I think that should just work too.

You reminded me actually there's a bit of spec I didn't quite follow:

From an implementation perspective, ignoring bits is deeply connected to the maximum virtual and
physical address space supported by the processor (e.g., Bare, Sv48, Sv57). In particular, applying the
above transformation is cheap if it covers only bits that are not used by any supported address
translation mode (as it is equivalent to switching off validity checks). Masking NVBITS beyond those
bits is more expensive as it requires ignoring them in the TLB tag, and even more expensive if the
masked bits extend into the VBITS portion of the address (as it requires performing the actual sign
extension).

The Sail model has a TLB so maybe we need to do something, but I didn't quite get what it was trying to say here. It seems to be talking about 3 different cases:

  1. it covers only bits that are not used by any supported address translation mode
  2. it covers NVBITS beyond those bits
  3. the masked bits extend into the VBITS portion of the address

It just doesn't seem to make any sense. There are only two parts of the address - NVBITS and VBITS, so what three cases is this referring to?

Can anyone say if we need to change the TLB code? I'm not sure.

@UmerShahidengr
Copy link

Regarding the comment on @jrtc27 and @Timmmm of implementing Pointer Masking in translateAddr(), I would say that pointer masking is different than virtual address translation, this implementation is efficient if not to be bounded inside translateAddr() function, it should be inside a different function (which is defined as transform_effective_address() here

@arichardson
Copy link
Collaborator

Regarding the comment on @jrtc27 and @Timmmm of implementing Pointer Masking in translateAddr(), I would say that pointer masking is different than virtual address translation, this implementation is efficient if not to be bounded inside translateAddr() function, it should be inside a different function (which is defined as transform_effective_address() here

IMO this should be part of ext_data_get_addr which already exists.

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.

7 participants