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

RISCV64 TLB refuses to access upper half of physical address space #238

Closed
georgehodgkins opened this issue Aug 29, 2023 · 4 comments
Closed
Assignees
Labels
arch-riscv The RISC-V ISA bug

Comments

@georgehodgkins
Copy link

Describe the bug
The RISCV TLB model throws an illegal address exception if a program attempts to access an address in the upper half of the physical address space (any address with the MSB set). This is not requirement of the specification, which simply says that "the execution environment shall define what portions of the address space are legal to access". Since gem5 is a simulator, I believe that it should provide a maximally permissive environment for evaluating new designs by removing this artificial restriction.

In addition, it should be noted that the official RISCV ELF ABI includes a ELF symbol model (medlow) which places objects in the top 2GB of the RV64 address space, which this restriction makes inaccessible.

The logic implementing this restriction can be found at lines 358-371 of src/arch/riscv/tlb.cc on the current stable branch f29bfc0.

Affects version
Added in this commit, first released in stable version 20.0.0.

gem5 Modifications
I am testing a model of a novel memory device which requires a specific layout of the address space (that is how I encountered this error). However, the faulting address ranges are not backed by this custom device, but by an unmodified SimpleMemory (and regardless the access does not seem to get past the TLB).

To Reproduce
Generate a load, store, or fetch access to a physical address in the upper half of the address space on a RISCV64 platform. Probably the easiest way to accomplish this is to use a custom linker script, which allows you to specify addresses for all code sections.

Terminal Output
Just the normal output for a fault in machine mode (with debug-flags=All), nothing specific to this error:

0: system.cpu1: Fault (Address) at PC: (0xffffffffbffff804=>0xffffffffbffff808).(0=>1)                                                                              
0: system.cpu1.isa: Reading MiscReg PRV (0): 0x3.
0: system.cpu1.isa: Reading MiscReg STATUS (6): 0xa00002000.
0: system.cpu1.isa: Setting MiscReg STATUS (6) to 0xa00002000.
0: system.cpu1.isa: Reading MiscReg STATUS (6): 0xa00002000.
0: system.cpu1.isa: Setting MiscReg MCAUSE (83) to 0x1.
0: system.cpu1.isa: Setting MiscReg MEPC (82) to 0xffffffffbffff804.
0: system.cpu1.isa: Setting MiscReg MTVAL (84) to 0xffffffffbffff804.
0: system.cpu1.isa: Setting MiscReg PRV (0) to 0x3.

The faulting PC is the entry point of the program. The trace shows that functional accesses used to load the code into memory (presumably bypassing the TLB) succeed without error.

Expected behavior
An illegal fault error, even if the address is backed by a valid responding device and code is executing in M-mode with no PMP restrictions. If the restriction is disabled (i.e. by commenting out and recompiling), the access should then succeed.

Host Operating System
Ubuntu 22.04 (kernel 5.15.90-wsl)

Host ISA
x86-64

Compiler used
gcc 11.3.0

Additional information
According to code comments, the reason for the restriction is to pass "RISCV tests". So there may be a test somewhere that needs to be changed as well. I looked a bit in gem5/tests, but didn't see anything that looked right.

@BobbyRBruce BobbyRBruce self-assigned this Aug 29, 2023
@BobbyRBruce BobbyRBruce added the arch-riscv The RISC-V ISA label Aug 30, 2023
@BobbyRBruce
Copy link
Member

@hnpl, @aakahlow: Do you have time to look into this and maybe get a PR to fix this issue. @georgehodgkins has been quite helpful here in providing us with a detailed explanation of this bug. Could you double check his reasoning here and come up with a PR to fix this issue? From what I see, a fix shouldn't be too difficult.

@hnpl
Copy link
Contributor

hnpl commented Aug 30, 2023

Hi there, I agree this seems to be a case where gem5 being overly restrictive as I couldn't find anywhere in the RISCV spec saying that the MSB of a physical address space has a special meaning. Though, I'm not very familiar with RISCV physical memory ranges in general, so I'd let other people chime in before making the patch removing the check for the MSB of the physical address.

However, regarding the mentioned documentation, I believe the term address space referred by the specs (the RISCV user spec and the RISCV ELF ABI spec) is referring to virtual address space, while the check https://github.com/gem5/gem5/blob/v23.0.0.0/src/arch/riscv/tlb.cc#L362 is for physical address.

Also, I couldn't find any test in riscv-tests that corresponds to the comment in line https://github.com/gem5/gem5/blob/v23.0.0.0/src/arch/riscv/tlb.cc#L359. I believe that the test assumes the physical address with the MSB set does not correspond to any physical device.

@hnpl
Copy link
Contributor

hnpl commented Jan 9, 2024

Now that I read the RISC-V spec again, my understanding is that the only constraint of the bit 63 of physical address is that the bit must be the same as the MSB of PPN/PFN [1]. So the MSB of a physical address being 0 or being 1 are both legal.

I couldn't find any reference mentioning the bit 63 of a physical address. I believe we should remove the bits(req->getPaddr(), 63) == 1 requirement.

Interestingly, there's a constraint for the bit 63 of a PTE such that, the bit 63 of a PTE must be 0 if Svnapot does not present
[2].

Edit: The ref [1] refers to virtual address, not physical address.

Ref:
[1] https://github.com/riscv/riscv-isa-manual/blob/riscv-isa-release-056b6ff-2023-10-02/src/supervisor.adoc?plain=1#L1440
[2] https://github.com/riscv/riscv-isa-manual/blob/riscv-isa-release-056b6ff-2023-10-02/src/supervisor.adoc?plain=1#L1474

@hnpl
Copy link
Contributor

hnpl commented Jan 10, 2024

The riscv test that checks for the bit 63 is here https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18.

There's a relevant discussion here, https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo. Basically, the physical address must be zero-extended. Since there's currently no address translation scheme in RISC-V that results in 64-bit physical address, the bit 63 of a physical address is expected to be zero, hence the check.

I think it's better to check for the validity of a physical address elsewhere, e.g. the PMA.

hnpl added a commit to hnpl/gem5 that referenced this issue Jan 10, 2024
Currently, the TLB enforces that the bit 63 of a physical address
to be zero. This check stems from the riscv-tests that checks for
the bit 63 of a physical address [1]. This is due to the fact that
the ISA implicitly says that the physical address must be
zero-extended on the most significant bits that are not
translated [2]. More details on this issue is here [3].

The check for bit 63 of a physical address in the TLB is rather
too specific, and I believe the check of invalid physical address
is alread implemented in PMA. Thus, this change proposes to remove
this check from RISC-V TLB.

[1] https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18
[2] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo
[3] gem5#238

Change-Id: I247e4d4c75c1ef49a16882c431095f6e83f30383
Signed-off-by: Hoa Nguyen <hn@hnpl.org>
hnpl added a commit to hnpl/gem5 that referenced this issue Jan 11, 2024
Currently, the TLB enforces that the bit 63 of a physical address
to be zero. This check stems from the riscv-tests that checks for
the bit 63 of a physical address [1]. This is due to the fact that
the ISA implicitly says that the physical address must be
zero-extended on the most significant bits that are not
translated [2]. More details on this issue is here [3].

The check for bit 63 of a physical address in the TLB is rather
too specific, and I believe the check of invalid physical address
is alread implemented in PMA. Thus, this change proposes to remove
this check from RISC-V TLB.

[1] https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18
[2] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo
[3] gem5#238

Change-Id: I247e4d4c75c1ef49a16882c431095f6e83f30383
Signed-off-by: Hoa Nguyen <hn@hnpl.org>
hnpl added a commit to hnpl/gem5 that referenced this issue Jan 11, 2024
Currently, the TLB enforces that the bit 63 of a physical address
to be zero. This check stems from the riscv-tests that checks for
the bit 63 of a physical address [1]. This is due to the fact that
the ISA implicitly says that the physical address must be
zero-extended on the most significant bits that are not
translated [2]. More details on this issue is here [3].

The check for bit 63 of a physical address in the TLB is rather
too specific, and I believe the check of invalid physical address
is alread implemented in PMA. Thus, this change proposes to remove
this check from RISC-V TLB.

[1] https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18
[2] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo
[3] gem5#238

Change-Id: I247e4d4c75c1ef49a16882c431095f6e83f30383
Signed-off-by: Hoa Nguyen <hn@hnpl.org>
powerjg pushed a commit that referenced this issue Jan 12, 2024
Currently, the TLB enforces that the bit 63 of a physical address to be
zero. This check stems from the riscv-tests that checks for the bit 63
of a physical address [1]. This is due to the fact that the ISA
implicitly says that the physical address must be zero-extended on the
most significant bits that are not translated [2]. More details on this
issue is here [3].

The check for bit 63 of a physical address in the TLB is rather too
specific, and I believe the check of invalid physical address is alread
implemented in PMA. Thus, this change proposes to remove this check from
RISC-V TLB.

[1]
https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18
[2] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo
[3] #238

Change-Id: I247e4d4c75c1ef49a16882c431095f6e83f30383

Signed-off-by: Hoa Nguyen <hn@hnpl.org>
pranith pushed a commit to pranith/gem5 that referenced this issue Jan 16, 2024
)

Currently, the TLB enforces that the bit 63 of a physical address to be
zero. This check stems from the riscv-tests that checks for the bit 63
of a physical address [1]. This is due to the fact that the ISA
implicitly says that the physical address must be zero-extended on the
most significant bits that are not translated [2]. More details on this
issue is here [3].

The check for bit 63 of a physical address in the TLB is rather too
specific, and I believe the check of invalid physical address is alread
implemented in PMA. Thus, this change proposes to remove this check from
RISC-V TLB.

[1]
https://github.com/riscv-software-src/riscv-tests/blob/bd0a19c136927eaa3b7296a591a896c141affb6b/isa/rv64mi/access.S#L18
[2] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/8kO7X0y4ubo
[3] gem5#238

Change-Id: I247e4d4c75c1ef49a16882c431095f6e83f30383

Signed-off-by: Hoa Nguyen <hn@hnpl.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv The RISC-V ISA bug
Projects
None yet
Development

No branches or pull requests

4 participants