[pmp tests] fence.i needed before TEST_FOR_EXECUTION

[kumaransvyoma]

In all the tests, load/store instructions happen to the pmp regions. Before checking the effect of it, fence is require to see the changes for any DUT. spike does this immediately, so issue will not be seen in models

[kumaransvyoma]

The changes were updated in this repository : https://github.com/kumaransvyoma/riscv-arch-test-pmp/tree/pmp-fence

[allenjbaum]

Can you explain why a store followed by a load from the same address requires a fence.i between the load and the store?
Fence.i is required if a store to some address is followed by executing from that address, and perhaps (I'm not sure) if a write to a PMP CSR is followed by executing from a memory location affected by that entry that was written.
That doesn't seem to be the case for the tests you've changed. Am I missing something?

[lavanyajagan]

To state the issue more clearly: The stores were done to code segment, so fence.i is required to clear and refetch from the icache so we can see the new instructions

[allenjbaum]

That matters - but only if you are executing code at an address you just stored into, and the few examples I saw didn't seem to be doing that. The snippets I saw were just storing, then loading from the same address. Maybe I wasn't looking at the entire picture?

…

On Mon, Nov 25, 2024 at 11:22 PM Lavanya J ***@***.***> wrote: To state the issue more clearly: The stores were done to code segment, so fence.i is required to clear and refetch from the icache so we can see the new instructions — Reply to this email directly, view it on GitHub <#569 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJTHUWR5L2LE7D5NL7T2CQORTAVCNFSM6AAAAABSNH7UMKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJZHA2TINJYGA> . You are receiving this because you commented.Message ID: ***@***.***>

[davidharrishmc]

Sorry for the duplicate #588. See where the fence.i needs to be inserted.

.macro VERIFICATION_RWX		ADDRESS	
   	LA(a5, \ADDRESS)	// Fetch the address to be checked
		LI(a4, 	NOP)		// Load the new value (NOP Instruction ID)
...
		sw	a4,0(a5)	// Load data from it (CHECK FOR READ) ; (NOT TRAP => W enabled)
		nop
		nop
		sd	a4,0(a5)	// Load data from it (CHECK FOR READ) ; (NOT TRAP => W enabled)
		nop
		nop
...
    	ld	a4,0(a5)	// Load data from it (CHECK FOR READ) ; (NOT TRAP => W enabled)
    	nop
    	nop
		fence.i
		RVTEST_SIGUPD(x13,a4)
		jal	\ADDRESS	// Test for execution, an instruction is placed at this address

[lavanyajagan]

From our design team (SHAKTI out-of-order), 2 tests (pmp64-NAPOT-RWX.S and TOR-RWX.S) require the fence.

I am attaching their analysis as below:
We observed that the list of tests that we had sent earlier were only mismatching due to wrong paths. However, we did find tests with the actual issue where we need a fence. Here is one example, it is from pmp64-NAPOT-RWX.S (one of the tests we initially got back with, requesting for the addition of a fence before the following sequence)

    core   0: 3 0x00000000800005a0 (0x01300713) x14 0x0000000000000013                                      core   0: 3 0x00000000800005a0 (0x01300713) x14 0x0000000000000013
    core   0: 3 0x00000000800005a4 (0x00e78023) mem 0x0000000080000820 0x13                                 core   0: 3 0x00000000800005a4 (0x00e78023) mem 0x0000000080000820 0x13
    core   0: 3 0x00000000800005a8 (0x00000013)                                                             core   0: 3 0x00000000800005a8 (0x00000013)
    core   0: 3 0x00000000800005ac (0x00000013)                                                             core   0: 3 0x00000000800005ac (0x00000013)
    core   0: 3 0x00000000800005b0 (0x00e79023) mem 0x0000000080000820 0x0013                               core   0: 3 0x00000000800005b0 (0x00e79023) mem 0x0000000080000820 0x0013
    core   0: 3 0x00000000800005b4 (0x00000013)                                                             core   0: 3 0x00000000800005b4 (0x00000013)
    core   0: 3 0x00000000800005b8 (0x00000013)                                                             core   0: 3 0x00000000800005b8 (0x00000013)
    core   0: 3 0x00000000800005bc (0x00e7a023) mem 0x0000000080000820 0x00000013                           core   0: 3 0x00000000800005bc (0x00e7a023) mem 0x0000000080000820 0x00000013
    core   0: 3 0x00000000800005c0 (0x00000013)                                                             core   0: 3 0x00000000800005c0 (0x00000013)
    core   0: 3 0x00000000800005c4 (0x00000013)                                                             core   0: 3 0x00000000800005c4 (0x00000013)
    core   0: 3 0x00000000800005c8 (0x00e7b023) mem 0x0000000080000820 0x0000000000000013                   core   0: 3 0x00000000800005c8 (0x00e7b023) mem 0x0000000080000820 0x0000000000000013
    core   0: 3 0x00000000800005cc (0x00000013)                                                             core   0: 3 0x00000000800005cc (0x00000013)
    core   0: 3 0x00000000800005d0 (0x00000013)                                                             core   0: 3 0x00000000800005d0 (0x00000013)
    core   0: 3 0x00000000800005d4 (0x00078703) x14 0x0000000000000013 mem 0x0000000080000820               core   0: 3 0x00000000800005d4 (0x00078703) x14 0x0000000000000013 mem 0x0000000080000820
    core   0: 3 0x00000000800005d8 (0x00000013)                                                             core   0: 3 0x00000000800005d8 (0x00000013)
    core   0: 3 0x00000000800005dc (0x00000013)                                                             core   0: 3 0x00000000800005dc (0x00000013)
    core   0: 3 0x00000000800005e0 (0x00e6b023) mem 0x0000000080004218 0x0000000000000013                   core   0: 3 0x00000000800005e0 (0x00e6b023) mem 0x0000000080004218 0x0000000000000013
    core   0: 3 0x00000000800005e4 (0x00079703) x14 0x0000000000000013 mem 0x0000000080000820               core   0: 3 0x00000000800005e4 (0x00079703) x14 0x0000000000000013 mem 0x0000000080000820
    core   0: 3 0x00000000800005e8 (0x00000013)                                                             core   0: 3 0x00000000800005e8 (0x00000013)
    core   0: 3 0x00000000800005ec (0x00000013)                                                             core   0: 3 0x00000000800005ec (0x00000013)
    core   0: 3 0x00000000800005f0 (0x00e6b423) mem 0x0000000080004220 0x0000000000000013                   core   0: 3 0x00000000800005f0 (0x00e6b423) mem 0x0000000080004220 0x0000000000000013
    core   0: 3 0x00000000800005f4 (0x0007a703) x14 0x0000000000000013 mem 0x0000000080000820               core   0: 3 0x00000000800005f4 (0x0007a703) x14 0x0000000000000013 mem 0x0000000080000820
    core   0: 3 0x00000000800005f8 (0x00000013)                                                             core   0: 3 0x00000000800005f8 (0x00000013)
    core   0: 3 0x00000000800005fc (0x00000013)                                                             core   0: 3 0x00000000800005fc (0x00000013)
    core   0: 3 0x0000000080000600 (0x0007b703) x14 0x0000000000000013 mem 0x0000000080000820               core   0: 3 0x0000000080000600 (0x0007b703) x14 0x0000000000000013 mem 0x0000000080000820
    core   0: 3 0x0000000080000604 (0x00000013)                                                             core   0: 3 0x0000000080000604 (0x00000013)
    core   0: 3 0x0000000080000608 (0x00000013)                                                             core   0: 3 0x0000000080000608 (0x00000013)
    core   0: 3 0x000000008000060c (0x00e6b823) mem 0x0000000080004228 0x0000000000000013                   core   0: 3 0x000000008000060c (0x00e6b823) mem 0x0000000080004228 0x0000000000000013
    core   0: 3 0x0000000080000610 (0x210000ef) x 1 0x0000000080000614                                      core   0: 3 0x0000000080000610 (0x210000ef) x1  0x0000000080000614
    core   0: 3 0x0000000080000820 (0x00148493) x 9 0x00000000000000ff                                    | core   0: 3 0x0000000080000820 (0x00000013)
    core   0: 3 0x0000000080000824 (0x00000013)                                                           | core   0: 3 0x0000000080000940 (0x1000006f)
    core   0: 3 0x0000000080000828 (0x00000013)                                                           | core   0: 3 0x0000000080000a40 (0x34011173) x2  0x0000000080003000 c832_mscratch 0x01fddb7d5bfddb7f
                                                                                                          > core   0: 3 0x0000000080000a44 (0x28b13423) mem 0x0000000080003288 0x0000000000000000
                                                                                                          > core   0: 3 0x0000000080000a48 (0x0b8005ef) x11 0x0000000080000a4c
                                                                                                          > core   0: 3 0x0000000080000b00 (0x28a13023) mem 0x0000000080003280 0x0000000000000001

At 0x80M820, we do not see the write that has just gone through and as a result we have incorrect execution. The following section is from the patched test with the fence included, here we can observe correct execution (write has been reflected).

    core   0: 3 0x0000000080000610 (0x00e6b823) mem 0x0000000080004228 0x0000000000000013                   core   0: 3 0x0000000080000610 (0x00e6b823) mem 0x0000000080004228 0x0000000000000013
    core   0: 3 0x0000000080000614 (0x22c000ef) x 1 0x0000000080000618                                      core   0: 3 0x0000000080000614 (0x22c000ef) x1  0x0000000080000618
    core   0: 3 0x0000000080000840 (0x00000013)                                                             core   0: 3 0x0000000080000840 (0x00000013)

[allenjbaum]

OK, that's a known problem; It not a store/load dependency, it's a store/execute dependency and the fix is coming (in a few other places as well!) There needs to be a fence.i inbetween the store and the jal.
I'll leave open until the fix is merged