workaround to support access to large 64-bit physical addresses

[aap-sc]

Current spike implementation erroneously assumes that a maximum physical address is limited to (1 << MAX_PADDR_BITS).

There are few issues with this assumption:

1. if satp.MODE == Bare, then virtual addresses are equal to physical and there are no limitations and no additional requirements on the number of address bits and address values one may use.
2. the actual limit can only be derived from the current privilege mode and CSR configuration and thus can be determined only in runtime.

This patch implements a simple workaround by making queries to the platform configuration as a last-ditch effort before memory access abort.

To figure out if the current configuration supports large 64-bit addresses only ordinary memory regions are taken into account - bus devices (abstract devices) are excluded. Proper support for such cases may require significant refactoring.

[aap-sc]

@timsifive , @aswaterman would you kindly take a look?

This commit fixes erroneous parsing of memory regions at large physical addresses (in case of 64-bit overflow) and allows access to such regions.

As for the tests, I used the following (added locally to risv-tests repository). Since it requires a custom spike configuration - I've never published this test.

#include "riscv_test.h"
#include "test_macros.h"

RVTEST_RV64M
RVTEST_CODE_BEGIN

  la  x5, large_addr
  lw  x2, 0(x5)
  addi x3, x2, 1
  sw  x3, 0(x2)
  lwu  x4, 0(x2)
  lwu  x6, 8(x5)
  beq x6, x4, pass
  j fail
  TEST_PASSFAIL

.balign 8
large_addr:
  .8byte 0xffffffffffff0000
  .8byte 0xffff0001


RVTEST_CODE_END

  .data
RVTEST_DATA_BEGIN

RVTEST_DATA_END

[scottj97]

Current spike implementation erroneously assumes that a maximum physical address is limited to (1 << MAX_PADDR_BITS).

I don't understand this statement. That is not an assumption; it's a definition of MAX_PADDR_BITS. If your implementation supports a 64-bit paddr, then you should set MAX_PADDR_BITS to 64. (If that doesn't work, then we can fix that.)

[aswaterman]

I disagree; this is not "erroneous." Spike's "platform" constrains the address map so that physical addresses are less than 2^56, even when VM isn't present. There's nothing wrong with such a constraint.

This is an awful lot of change to support a use case that doesn't seem pressing. Even Xeons only support a 52-bit physical address space, AFAIK.

[aap-sc]

I don't understand this statement. That is not an assumption; it's a definition of MAX_PADDR_BITS. If your implementation supports a 64-bit paddr, then you should set MAX_PADDR_BITS to 64. (If that doesn't work, then we can fix that.)

The problem is that MAX_PADDR_BITS is a define. So you can't get it out-of-the-box.

@aswaterman :

I disagree; this is not "erroneous." Spike's "platform" constrains the address map so that physical addresses are less than 2^56, even when VM isn't present. There's nothing wrong with such a constraint.

Well... It's just according to privileged spec such modes of operation should be possible. I mean satp.MODE == Bare and we are in machine mode. Spike "platform" is configured via command line and passed memory map can be located at large addresses.

This is an awful lot of change to support a use case that doesn't seem pressing

Most of the change is fixing mem_cfg to allow configurations like { base = 0xffff_ffff_ffff_f000, size: 0x1000 }. It can be moved to a separate patch if it is too confusing.

The actual workaround is small and localized in sim_t::paddr_ok function (it's just around 5-6 lines).

That being said - if you believe this is an overkill then we can drop it, of course

[aap-sc]

@aswaterman I feel like I need to elaborate a little more.

1. If we try to configure current version of spike as follows:

./spike -m0x80000000:0x3000,0xfffffffffffff000:0x1000 <path_to_elf>

then it will just print help prompt since the current parser of memory regions is a little buggy. Changes that touch mem_cfg_t address this issue and allows for such memory regions to be parsed properly. This is like 90% of this patch :).

2. The actual workaround is these extra checks:

 if (mems.empty())
    return false;

  assert(std::is_sorted(mems.begin(), mems.end(), is_ascending));
  // last-ditch effort to detect if the platform has physical memory
  // at the addresses greater than (1 << MAX_PADDR_BITS)
  reg_t base_addr = mems.back().first;
  reg_t size = mems.back().second->size();
  reg_t last_available_pa = base_addr + size - 1;
  return (last_available_pa >= (1ull << MAX_PADDR_BITS));

If you still believe this is of little use, then let's close the pr then.

[aswaterman]

@aap-sc Let's begin by separating out these PRs. I fully support fixing mem_cfg_t and the command-line parsing, regardless of whether / how we expand the physical-address space.

[aswaterman]

As for expanding the physical-address space: can we not just change MAX_PADDR_BITS to 64, then delete the paddr_ok function (and replace all of its uses with true)?

[scottj97]

I suspect that would break my use case, where I have a sparse memory implemented in Spike to cover all non-device memory under 1 << MAX_PADDR_BITS. Of course I could always locally undo whatever change is made here.

…

On Mon, Sep 26, 2022 at 6:21 PM Andrew Waterman ***@***.***> wrote: As for expanding the physical-address space: can we not just change MAX_PADDR_BITS to 64, then delete the paddr_ok function (and replace all of its uses with true)? — Reply to this email directly, view it on GitHub <#1100 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVCTXD6M3ZRE3XNQPCGK3WAIVZDANCNFSM6AAAAAAQWGR54Q> . You are receiving this because you commented.Message ID: ***@***.***>

[aswaterman]

I see. Other alternative suggestions welcome.

[scottj97]

./spike -m0x80000000:0x3000,0xfffffffffffff000:0x1000 <path_to_elf>

I agree there is a real problem here. Part of the problem is that MAX_PADDR_BITS is a compile-time define -- but that's true of dozens of platform choices within Spike. We currently assume that any user who wants to customize any of those platform choices will maintain a local fork of Spike with source code changes. (Eventually Spike might support riscv-config for runtime platform configuration but that is a huge task.)

A second problem is that -m will silently accept memories beyond (1 << MAX_PADDR_BITS) even though such memories will not be accessible. It should error out immediately when it sees such an argument.

A third problem is that the parser for -m is buggy.

The second and third seem easy to fix. The first could potentially be fixed by discovering this value from the device tree (if it's there?), or calculated based on the largest address given to -m.

I don't agree with OP about this:

the actual limit can only be derived from the current privilege mode and CSR configuration and thus can be determined only in runtime.

Because MAX_PADDR_BITS is intended to describe a physical hardware limit. There simply aren't enough wires on the address bus to support any larger address.

So let's go back to Andrew's question:

This is an awful lot of change to support a use case that doesn't seem pressing. Even Xeons only support a 52-bit physical address space, AFAIK.

[aap-sc]

@aswaterman :

@aap-sc Let's begin by separating out these PRs. I fully support fixing mem_cfg_t and the command-line parsing, regardless of whether / how we expand the physical-address space.

Sure. I should've probably done this ins the first place. I'll create a separate MR with relevant changes - if we decide to adopt those, then maybe we could continue this discussion.

As for expanding the physical-address space: can we not just change MAX_PADDR_BITS to 64, then delete the paddr_ok function (and replace all of its uses with true)?

I think that this paddr_ok function may be useful. For example if we somehow know that the current paging mode is Sv39 / Sv48 that some extra checks could be implemented (but determining the current translation mode may be a difficult task). There are comments about it in the patch.

@scottj97 :

A second problem is that -m will silently accept memories beyond (1 << MAX_PADDR_BITS) even though such memories will not be accessible. It should error out immediately when it sees such an argument.

I'm not sure about this (please, see below).

Because MAX_PADDR_BITS is intended to describe a physical hardware limit. There simply aren't enough wires on the address bus to support any larger address.

Well... Here is the issue. My understanding is that spike is deemed to be a golden model as per various RISCV specifications (including the privileged one). RISCV privileged spec does not have this physical limit you've mentioned. In fact it explicitly allows for all 64-bit to be accessible. For example, when satp.MODE == Bare. That's why I believe such accesses should be allowed.

As for this one:

I suspect that would break my use case, where I have a sparse memory implemented in Spike to cover all non-device memory under 1 << MAX_PADDR_BITS.

I can't really comment on that :(. Maybe the relevant codebase is based on a wrong assumption, but I'm not sure.

[scottj97]

Well... Here is the issue. My understanding is that spike is deemed to be a golden model as per various RISCV specifications (including the privileged one). RISCV privileged spec does not have this physical limit you've mentioned. In fact it explicitly allows for all 64-bit to be accessible. For example, when satp.MODE == Bare.

Every real CPU implementation has limitations. Spike is designed to model one particular CPU implementation, and that's not necessarily the maximum possible RISC-V implementation. All real RISC-V CPUs in existence have less than 64 bits of address bus, so every real RISC-V CPU has the behavior that is modeled here in Spike. The exact size will vary, but removing this functionality altogether would be a step backwards in Spike's ability to model a real CPU.

That's why I believe such accesses should be allowed.

So set MAX_PADDR_BITS to 64 in your fork of Spike and be done with it.

[scottj97]

A second problem is that -m will silently accept memories beyond (1 << MAX_PADDR_BITS) even though such memories will not be accessible. It should error out immediately when it sees such an argument.

A third problem is that the parser for -m is buggy.

@aap-sc are you interested in solving these two problems as part of this PR?

[aap-sc]

@scottj97 , sorry for the delay. I was a little upset by the pushback I've got - now I'm better :).

@aap-sc are you interested in solving these two problems as part of this PR?

Kind of ... I've re-organized the patch set as follows:

bb4a523 this fixes processing of mem_cfg objects and refactors (a little bit) the relevant codebase. This addresses problem 3.
98dd935 this addresses problem 2. That is the " It should error out immediately when it sees such an argument.".

If these 2 commits are deemed to be fine - I can move them to a separate PR to address memory configuration parsing/processing.

As for the third commit (that is 86b2085)... I still hope to initiate yet another round of convincing you that it may be ok :). But IMHO it only makes sense if the first 2 commits are adopted.

[scottj97]

Excellent, I like the way this is going now.

Change bb4a523 does way too much in a single commit. This needs to be broken up into many smaller commits. I see that it:

1. Removes constructor on mem_cfg_t -- why?
2. Moves sanity checks out of constructor into is_valid_region() -- why?
3. Moves a sanity check base + size < base out of parse_mem_layout() and into is_valid_region() and subtly changes it from base + size < base to base + size <= base, if I'm reading it correctly
4. Adds several trivial accessor functions to mem_cfg_t that seem unnecessary; can we make base and size const instead? Then we wouldn't need to keep calling is_valid_region() and could keep the checks in the constructor
5. Redoes the memory merging logic
6. Changes one mem_cfg_t instantiation to use {} instead of ()
7. Redoes the way mems is constructed, using std::transform

Each of these should be its own commit -- in some cases, several commits -- with the commit message explaining what problem it is solving, or what benefit it provides.

I haven't looked in detail at the other two changes except to note that you have not accepted that MAX_PADDR_BITS is a useful feature modeling real CPU behavior, as explained above. The commit messages and comments in the code describe it as erroneous behavior, which it is not. It does exactly what it is designed to do, and is necessary to model every real RISC-V CPU in existence.

It's also problematic to decide the maximum paddr based only on memories and neglect devices. In my experience, it is common to have devices at addresses larger than any memory region.

[aap-sc]

Each of these should be its own commit -- in some cases, several commits -- with the commit message explaining what problem it is solving, or what benefit it provides.

Got it. Thank you very much for the feedback. I'll try to reorganize these changes further (in a separate pull request). I'll provide answers for your questions once done there (if you don't mind to participate in review).

The commit messages and comments in the code describe it as erroneous behavior, which it is not. It does exactly what it is designed to do, and is necessary to model every real RISC-V CPU in existence.

My bad. The wording needs to be changed. Will address.

It's also problematic to decide the maximum paddr based only on memories and neglect devices. In my experience, it is common to have devices at addresses larger than any memory region.

Yes... The primary reason is that the current interface (abstract_device_t, if I understand the code correctly) does not provide a way to determine the size of the underlying area.

[EccoTheDolphin]

Ok, so I've tried to minimize changes and to make them independent. The whole set of changes is here. Namely:

408c96f : to address problem 3 (buggy error-reporting in parser) (PR)
f047869 : to address problem 2 (bail-out if a memory region has [base + size] larger than MAX_PADDR_BITS)

dc814d2: mem merging refactoring (NFT change)
918fede: bullet-proof (hopefully) code to handle merging of memory addresses.

Will try to create separate PR's based on these changes. If all these changes are adopted - then I'll to come back to this PR.

[aap-sc]

Will try to create separate PR's based on these changes. If all these changes are adopted - then I'll to come back to this PR.

Actually it looks like #1788 finally implemented the remaining part (that is to allow memory regions to span the whole address space).

[aswaterman]

Right, I forgot to update this discussion.