riscv / riscv-j-extension

Working Draft of the RISC-V J Extension Specification
https://jira.riscv.org/browse/RVG-128
Creative Commons Attribution 4.0 International
162 stars 17 forks source link

Potential problems with masking loads/stores but not pc #63

Closed dramforever closed 7 months ago

dramforever commented 7 months ago

According to the current spec, Load and store instructions have their addresses masked, but instruction fetch and page table accesses don’t. This seems to create an excessively confusing scenario for software looking to emulate such accesses, such as emulating misaligned accesses or MMIO accesses.

For example, hlvx.{wu,hu} are defined to mask the address (because they’re load instructions), but the instruction fetch they’re designed to emulate doesn’t. If PMLEN > NVBITS, this would need significant effort for software to work around; even if PMLEN <= NVBITS, it seems to still be a glaring inconsistency.

This can create a problem in, for example, the following scenario:

To correctly fetch this instruction from memory, with the current spec, the hypervisor would need to check at which privilege level the MMIO instruction happened, temporarily disable pointer masking at that level, use hlvx.{hu,wu} to fetch the address, and restore the pointer masking state.

(A similar situation applies for mstatus.MPRV == 1 && mstatus.MXR == 1 loads in M-mode.)

This doesn't seem like it is an intended effect of the pointer masking extensions. This wouldn’t be a problem if those implicit accesses are simply defined to also mask their addresses.

Another option would be to make hlvx.* and mstatus.MPRV == 1 && mstatus.MXR == 1 loads not mask the address.

martinmaas commented 7 months ago

Thanks for bringing up these points. This is a topic that has seen a significant amount of discussion within the TG and with the architecture review committee. A previous version of the standard did support instruction pointer masking, but it was removed since there was not deemed to be a sufficient use case and it would have significantly increased the complexity.

The cases mentioned are each a bit different, so I will address them one by one:

  1. Page table accesses: Pointer masking is a concept that needs to be used in conjunction with code running with HWASAN (or another use case of pointer masking) on top of a particular address space – i.e., pointer masking is intended to be equivalent to manually performing an arithmetic unmasking operation for every load or store, but it is not intended to fundamentally change the address translation once the address reaches the MMU. Page tables are accessed outside of this address space and use addresses that are not part of that application code. It is therefore not clear which privilege mode's pointer masking setting should be applied. Furthermore, if pointer masking were to affect page table accesses, it means that enabling or disabling pointer masking could result in different translations for the same address, which would be very difficult to reason about. The pointer masking spec therefore addresses this in a different way: It requires in Section 2.6 that code must manually perform any masking of addresses before they are written into a register (e.g., satp), since that is the point in time when an address turns from application-visible to only being used within the MMU. The same applies for writing page table entries, which need to contain unmasked addresses.

  2. Emulating misaligned accesses: This is also defined in Section 2.6: "pointer masking is applied for hardware writes to a CSR (e.g., when the hardware writes the transformed address to stval when taking an exception)". The intention is that (e.g.,) the M mode handler emulating a misaligned accesses already sees the unmasked address. The idea is that the handler can be exactly the same whether or not pointer masking is enabled, and does not need to be aware of the pointer masking setting of the privilege mode causing the trap.

  3. Emulating instruction fetch: I agree that this case could potentially use more discussion and that it might make sense for accesses via hlvx.* and mstatus.MXR == 1 to not mask the address. We will discuss this in the TG and with the architecture review committee.

dramforever commented 7 months ago

Thanks for the response, points 1 and 2 of your response quite reasonably addresses my concerns, and we can await further discussion on point 3.

This also gave me a new idea: I wonder if we can, like page table addresses, require a correctly masked pc at all times. The problem scenario I had requires PMLEN > NVBITS, which would create a "shadow region" where pc can point to but load/store instructions can't touch. If we require a correctly masked pc for instruction fetch then no such shadow region exists, and in the scenario I gave the exception would be an instruction fetch fault, and the hypervisor would not try to emulate anything.

If we do so, "hlvx.* and mstatus.MXR == 1 don't mask address" should probably be changed into faulting on "shadow region" pc for consistency.

(I hope I'm not missing that the spec already requires this, but I don't find occurrences of "pc", "jump" etc in the spec.)

martinmaas commented 7 months ago

My sense is that creating new constraints on instruction fetch would likely result in additional complexity similar to applying pointer masking – which was deemed too complex during architecture review without a strong use case (the advantage of the current standard is that it does not affect instruction fetch at all, which is a major source of complexity).

It seems to me like changing hlvx etc. would handle the issue with significantly less complexity.

Adam-pi3 commented 7 months ago

"hlvx.* and mstatus.MXR == 1 don't mask address" seems like a reasonable solution for that problem.

martinmaas commented 7 months ago

The new version of the spec (0.8.2 and later) now addresses this issue.

dramforever commented 7 months ago

That looks good to me as well