Skip to content

Conversation

jayishnu637
Copy link

Description

The CVA6 processor core lacked support for the RISC-V Svnapot (Naturally Aligned Power-of-Two) extension. The primary motivation for this feature is to improve MMU performance by allowing a single TLB entry to map a large, contiguous 64KiB memory region, thereby reducing TLB misses for memory-intensive applications. With this implementation Svnapot will be supported in CVA6 core.

Changes

1. Page Table Walker (cva6_ptw.sv):

  • The PTW now correctly detects a 64KiB NAPOT PTE (N=1, PPN[3:0] = 4'b1000).

  • Upon detection, it forwards the unmodified PTE to the TLB update interface.

  • A new signal, napot_bits, is propagated alongside the PTE to flag it as a NAPOT entry for the TLB.

2. TLBs (cva6_tlb.sv and cva6_shared_tlb.sv):

  • The TLB storage (tags_q, content_q) has been extended to include the napot_bits field, allowing each entry to be identified as a standard or NAPOT page.

  • Tag Comparison Logic: The lookup logic has been significantly updated. When comparing tags for an entry where napot_bits is set, the lower 4 bits of the Virtual Page Number (VPN) are masked (ignored). This ensures that any virtual address within the 64KiB range correctly matches the single NAPOT TLB entry.

  • On-Hit PPN Patching: This is the core of the fix. Upon a successful tag match with a NAPOT entry, the final PPN is constructed by taking the stored PPN template and overwriting its lower 4 bits (PPN[3:0]) with the corresponding bits from the incoming virtual address (vaddr[15:12]).

3. Memory Management Unit (cva6_mmu.sv):

  • The MMU has been updated to correctly handle the patched PPN and napot_bits coming from the ITLB and DTLB on a hit, ensuring the final physical address is calculated correctly.

How to Test

CC @Jbalkind, @niwis

@jayishnu637
Copy link
Author

@JeanRochCoulon got some issues while rebasing the branch. So, created this new Pull Request and closed the previous one. Please look into it.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

@YazanHussnain-10x YazanHussnain-10x left a comment

Choose a reason for hiding this comment

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

@jayishnu637
I have added some comments. Which I think needs to be resolved first, then move forward.
I have some concerns on your svnapot extension micro-architecture.

I'm reviewing your previous PR on Saturday. But you closed that one to rebase with main. Therefore, I’ve shifted my comments to this PR

Copy link
Contributor

❌ failed run, report available here.

@jayishnu637
Copy link
Author

@YazanHussnain-10x I've pushed a new commit addressing all the points you raised. Thanks for pointing those issues out.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

6 similar comments
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

@jayishnu637
Copy link
Author

Hi team,

I've pushed fixes for the previous compilation errors in this PR.

However, the only test failing is "Environment check" with a generic Environment failure detected error (Job ID: 273740): Environment failure detected. Some reports might be missing.

Any guidance on how to resolve this environment issue would be greatly appreciated.

Copy link
Contributor

❌ failed run, report available here.

2 similar comments
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

@JeanRochCoulon
Copy link
Contributor

Hello @jayishnu637 I relaunched the CI. It passes right now.
@YazanHussnain-10x do you approve the PR?

Copy link
Contributor

@YazanHussnain-10x YazanHussnain-10x left a comment

Choose a reason for hiding this comment

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

Hi @jayishnu637 ,
I have left some more comments.
I looked at the cva6_mmu and cva6_ptw files so far. I'll read through the rest later.

Sorry to review so little each day, but I am working on it...

@YazanHussnain-10x
Copy link
Contributor

@JeanRochCoulon

@YazanHussnain-10x do you approve the PR?

Not yet. I’ve added some additional comments and will be completing the rest of my review shortly.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

@jayishnu637
Copy link
Author

Hi @YazanHussnain-10x,
resolved all the issues pointed at, in your recent comments.

Thanks

Copy link
Contributor

❌ failed run, report available here.

2 similar comments
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

1 similar comment
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

2 similar comments
Copy link
Contributor

❌ failed run, report available here.

Copy link
Contributor

❌ failed run, report available here.

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.

3 participants