Skip to content

Conversation

@Zain2050
Copy link
Contributor

Closes Issue #1496

@davidharrishmc
Copy link
Contributor

PreUpdateDA portion looks good to me.
@rosethompson do you have time to review the other part?

@rosethompson
Copy link
Contributor

The changes in hptw.sv look correct to me. This is the correct way to generate a store/load page/access fault caused by a hardware page table walk. However, are the changes to the MMU redundant?

assign StoreAmoAccessFaultM = (PMAStoreAmoAccessFaultM | PMPStoreAmoAccessFaultM | AtomicMisalignedCausesAccessFaultM & WriteAccessM) & ~TLBMiss & ~(SelHPTW & (|CMOpM));

This does the same as assign HPTWStoreAmoAccessFault = LSUAccessFaultM & DTLBWalk & (MemRWM[0] | (|CMOpM));

Otherwise this looks great.

@Zain2050
Copy link
Contributor Author

Zain2050 commented Oct 9, 2025

The change in hptw.sv enabled faults for CMOpM, but instead of getting a StorePageFault I was getting a StoreAccessFault.

assign StoreAmoAccessFaultM = (PMAStoreAmoAccessFaultM | PMPStoreAmoAccessFaultM | AtomicMisalignedCausesAccessFaultM & WriteAccessM) & ~TLBMiss & ~(SelHPTW & (|CMOpM));
This change in mmu.sv is a fix for that.

During the page table walk we're getting a PMAStoreAmoAccessFaultM from pmachecker.

(line 79) assign PMAStoreAmoAccessFaultM = (WriteAccessM | (|CMOpM)) & PMAAccessFault;
(line 76) assign PMAAccessFault = SelRegions[0] & AccessRWXC | AtomicAccessM & ~AtomicAllowed;

SelRegions[0] & AccessRWXC seems to be the cause of the bug. During a page table walk, the physical address changes to 0x10 during L_ADR states. This causes SelRegions[0] to go high as that address doesn't lie within any address range.
SelRegions[0] is high during probably every walk, but here AccessRWXC causes a problem

(line 55) assign AccessRWXC = ReadAccessM | WriteAccessM | ExecuteAccessF | (|CMOpM);

During other store & load operations ReadAccessM, WriteAccessM & ExecuteAccessF are controlled by hptw and are only high during L_RD states, but that not the case for CBOpM i.e. it stays 2 for cbo.flush during entire walk.
This causes PMAAccessFault which causes PMAStoreAMOAccessFaultM which in turn leads to HPTWfaultM and stops the page table walk. This is not supposed to occur.

assign AccessRWXC = ReadAccessM | WriteAccessM | ExecuteAccessF | (|CMOpM);
During page table walk a PTE is either read or written (Svadu) but no CBO operations are performed DURING the page table walk, so we need to neglect CMOpM in PMA checks during page table walk.

That's why in that line in mmu.sv I'm ignoring StoreAccessFault due to CMOpM when SelHPTW=1.

However, I don't think that this is the right approach to make that fix. This might suppress some other desired StoreAccessFaults. I'll try to implement a better way do this.

@Zain2050
Copy link
Contributor Author

Zain2050 commented Oct 9, 2025

@rosethompson do you agree with this change? Tell me if anything is unclear.

@Zain2050
Copy link
Contributor Author

Zain2050 commented Oct 9, 2025

I have moved this logic ~(SelHPTW & (|CMOpM)) into both pmachecker & pmpchecker. Previous way of doing it (in mmu.sv) might have suppressed some other faults. Therefore, I've moved it into pmpchecker and pmachecker to eliminate it's possibility of breaking anything else.
The reason for making this change in pmpchecker is the same. I got curious about pmpchecker, that would it cause the same issue as pmachecker? Therefore, I wrote a test with no RW pmp permission at address 0x10 and indeed it caused a mismatch. That's why I've added the logic in pmachecker.sv as well

@rosethompson
Copy link
Contributor

rosethompson commented Oct 20, 2025

I understand what is wrong. The PMA and PMP checkers should not be getting CMO accesses while walking the page table. For load, store, and amo accesses, SelHPTW overrides the CPU's requested access (PreLSURWM) at line 327. However, we are missing this mux for CMOpM.

You'll need to add the similar CMOpM mux to drive CMOpM='0 when SelHPTW = 1.

@Zain2050
Copy link
Contributor Author

You'll need to add the similar CMOpM mux to drive CMOpM='0 when SelHPTW = 1.

Done.

@davidharrishmc
Copy link
Contributor

davidharrishmc commented Oct 21, 2025 via email

@Zain2050
Copy link
Contributor Author

Yes, it does. The previous changes had also resolved the problem & fixed the mismatches but this is a cleaner way to do it.

@davidharrishmc davidharrishmc merged commit 2499571 into openhwgroup:main Oct 21, 2025
1 check passed
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