Add Hypervisor 2-Stage Address Translation

[nadime15]

This is an initial draft of the two-stage address translation support for the Hypervisor extension and based on #1419 and #1110 (the actual new code is introduced starting from commit 52ba617).

The code is still very experimental and I plan to keep improving and changing it. I would appreciate early feedback on the overall direction.

There are two main problems to address.

1. Dealing with implicit VS-Stage Page Faults and Exception handling complexity

With two stage translation, exception handling becomes more complicated. Previously it was enough to propagate the faulting VA and the causing exception. Due to virtualization, we now have several relevant addresses that are required to set stval (and htval) correctly.

These include the GVA (Guest Virtual Address) for explicit VS-Stage faults and the GPA (Guest Physical Address) for G-Stage faults or the guest PTE address that triggered an exception during implicit VS-Stage PTE accesses (and maybe even more, I dont see the big picture yet).

This means the exception context must carry more information than before in order to trap correctly.

To pass this information up the call chain, we define a new struct called Exception_Context that carries more contextual data. There is also a new PTE_Error entry (PTW_Implicit_Error) that propagates the implicit error including the encountered exception during VS-Stage back up the call chain.

The trap handler is untouched to keep the draft in a manageable size.

2. Virtual address definitions introduced by the Hypervisor spec

The Hypervisor spec introduces 34 bit "virtual addresses" (GPA as part of G-Stage) for RV32. I think it's is debatable whether these should be modeled as virtual addresses or treated as physical addresses. This draft treats the address received from the VS-Stage as a physical address and continue working with it directly instead of casting it back to a virtual address.

There is still work to do on typing and refactoring of the TLB lookup, insertion, and flush logic.

Regular one stage address translation (S-Stage) is not affected by these changes and the code works as usual and compiles.

[github-actions]

Test Results

2 328 tests  ±0   2 328 ✅ ±0   32m 49s ⏱️ + 1m 30s
    1 suites ±0       0 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit c3eb833. ± Comparison against base commit 5f5f02d.

♻️ This comment has been updated with latest results.

[nadime15]

Here are the diagrams from this morning’s meeting. They might be helpful for one or the other.

1-Stage/2-Stage Address Translation

2-Stage Address Translation Fault Handling

[Timmmm]

Ok I've read slightly more of the spec. My first observation is that we don't really need to model Sv39x4, etc. separately. Since the only difference is the top two bits of the first VPN (what satp/hgatp point to), Sv39x4 is actually identical to Sv39 if you just calculate the root page location differently. In other words you can have translateAddr work for guest physical addresses with very minor changes something like this:

function translateAddr(
  vAddr : virtaddr,
  access : MemoryAccessType(mem_payload),
  translation_type : {Virtual, GuestPhysical},
) -> TR_Result(physaddr, ExceptionType) = {

  ...
  let satp_sxlen = get_satp_or_hgatp(sv_width, translation_type);
  ...

    let base_ppn = satp_to_ppn(satp_sxlen);
    let base_ppn = match translation_type {
      Virtual => base_ppn,
      GuestPhysical => base_ppn + vAddr[sv_width+1 .. sv_width],
    };

In other words we can treat Sv39x4 exactly the same as Sv39, if instead of setting the root page to just satp.PPN we set it to hgatp.PPN + (the top two bits of the guest physical address).

Then we can totally forget about the whole x4 thing. Does that sound right?

[nadime15]

Yes you are right, we dont actually need the Sv*x entries, but I am not sure it makes sense to get rid of them entirely.

I think your point is that we can effectively squeeze the S-Stage / VS-Stage and G-Stage into a single function (translateAddr), and then derive the correct values based on translation_type. PR #1419 already went in that direction by introducing things like xatp_mode_width, etc.

(I am a bit confused by the example though, you are using satp_to_ppn instead of something like satp_or_hgatp_to_ppn, and vsatp it is not considered even though its not the same as satp. I assume that was just quick pseudocode.)

For now (not 100% sure yet), I would personally prefer to keep the Sv*x entries. If we want to go down the more generic path, I would rather define higher level helper functions, for example, for fetching the base ppn or sv_width (similar to xatp_mode_width), that forwards to the appropriate underlying helpers (get_vsatp, get_satp, get_hgatp etc.) based on the selected mode.

My concern with removing these entries and making everything as “tight” as possible is that it may become slightly harder to understand, especially for someone who is still in the process of learning the spec. But honestly I am not sure either.

@radimkrcmar @trdthg Any thoughts?

[Timmmm]

I assume that was just quick pseudocode.

Yeah exactly; we'll need to rename things to xatp or something like that.

I would rather define higher level helper functions, for example, for fetching the base ppn or sv_width (similar to xatp_mode_width), that forwards to the appropriate underlying helpers (get_vsatp, get_satp, get_hgatp etc.) based on the selected mode.

I think this is a good idea. IMO this would greatly reduce the amount of code and also simplify it a fair bit, which would make it easier to understand. We should add a comment to explain it of course.

Honestly I think the spec would be clearer if it was written like this too. They could have ditched the entire explanation of extra modes and whatnot and just replaced it with a single paragraph:

To support RV32 34-bit physical addresses, guest physical address translation has a virtual address space four times larger than usual. hgatp.PPN points to four root page tables (and must be 16kB-aligned instead of 4-kB aligned), and the two bits above the normal virtual address width select which root page table to use. For example when hgatp.MODE selects Sv32, the lowest 32-bits of the guest physical address are translated in the usual way but using the root page table seleected by bits [33:32] of the guest physical address. Similarly if hgatp.MODE is configured as Sv39, then bits [40:39] of the guest physical address select the root page table.

Is that not easier to understand than introducing a load of extra modes, address formats, etc?

[trdthg]

I think it’s a good idea generally and we should give it a try. But I suspect some of the type issues might be tricky.

type vpn_bits('v), is_sv_any_mode('v) = bits('v - pagesize_bits)

[nadime15]

@Timmmm Haha yeah it is actually!

Ok thanks @Timmmm @trdthg, I will take another look and incooperate what we just discussed!

[radimkrcmar]

I don't follow this logic. The validity is only defined for virtual address (first stage), and it always sign extends from 39/48/57 bits to 64 bits on RV64, and compares the result.

The output of first stage is 34 or 56 bits. Bits that are not translatable by the second stage (above 41/50 on Sv39x4/Sv48x4) are required to be 0.
(VS thinks that it addresses a physical address, and it would be very impractical if that address changed based on what hgatp.mode HS picked.)

[nadime15]

I agree its confusing, thats why I added a TODO. The address validity check in translate_stage is a bit convoluted due to merging S/VS/G stages into one function even though these stages operate under different address types/lengths. The problem is also the transition from GVA to GPA in RV32 from 32bit to 34bit addresses.

The original code did the canonical check directly on virtaddr:

let svAddr = bits_of(vAddr)[sv_width - 1 .. 0];
if bits_of(vAddr) != sign_extend(svAddr)

This worked because virtaddr is xlenbits (32 or 64 bits), and comparing these values is straightforward.
The problem now is that translate_stage takes physaddr (34 bits on RV32, 64 bits on RV64) to support G-stage GPAs. The wrapper translate_vs_stage converts virtaddr to physaddr using zero_extend:

On RV32 with Sv32, virtaddr is 32 bits, but physaddr is 34 bits so we zero_extend, which sets upper 2 bits to 0
But sign_extend(svAddr) to 34 bits sets upper 2 bits based on bit 31 so now If bit 31 = 1: zero_extend gives 00, sign_extend gives 11, which would lead to a mismatch.

I don’t fully grasp the big picture yet, but in general it might not be a good idea to squeeze everything into a single function and rely on physical bits. We can have fewer physical bits than GPA (34 bits), so in that case we would already run into limitations and lose bits using translate_stage that uses phys. addresses (I think).

It might be worth it to define a new type only for GPA.

But to your point, you basically saying in the G_Stage stage we should zero_extend instead of sign_extend.

[radimkrcmar]

On RV32 with Sv32, virtaddr is 32 bits, but physaddr is 34 bits so we zero_extend, which sets upper 2 bits to 0
But sign_extend(svAddr) to 34 bits sets upper 2 bits based on bit 31 so now If bit 31 = 1: zero_extend gives 00, sign_extend gives 11, which would lead to a mismatch.

I think passing anything other than XLEN sized type as an input to the first stage should be an internal sail error, because it shouldn't have happened. The sign extension just compares whether the upper bits of the XLEN sized address are in the proper format (equal to the highest bit that is used in the Sv mode translation).

The output of the translation is then a 34/56 bit address (regardless of what the actual physical address width is).

But to your point, you basically saying in the G_Stage stage we should zero_extend instead of sign_extend.

Yes.

[radimkrcmar]

Btw. how are you thinking to implement HSV/HLV?
Those can execute at M/HS/HU modes, and require two stage translation... Would they be another MemoryAccessType?

[nadime15]

Btw. how are you thinking to implement HSV/HLV?
Those can execute at M/HS/HU modes, and require two stage translation... Would they be another MemoryAccessType?

PR #1419 addressed this by splitting translateAddr into two functions and introducing translateAddr_pv (called translateAddr_eff_priv in this PR). This new function allows the effective privilege level to be passed explicitly, rather than being computed internally by translateAddr (see how HLV is implemented in PR #1419) . As a result, translateAddr now simply wraps translateAddr_eff_priv.

So the implementation for HLV will just call:

translateAddr_pv(
  vaddr,
  Load(Data),
  privLevel_bits(zero_extend(hstatus[SPVP]), 0b1)
)

The full error handling happens within the instruction’s execute clause, instead of delegating to vmem_read or vmem_write, which already encapsulate this logic. We currently have a few cases, such as certain atomic instructions where calling translateAddr directly is still needed. So this would actually not change anything besides the fact that we call translateAddr_eff_priv instead of translateAddr for HSV/HLV.

[Timmmm]

What I was trying to say is, change the function signature to this:

private function translate_stage(
  // Zero extend for non-VS translations.
  addr   : bits(xlen + 2),
  access : MemoryAccessType(mem_payload),
  priv   : Privilege,
  stage  : TranslationStage
) -> TR_Result(physaddr, PTW_Error) = {

And then:

let extra_x4_bits = svAddr[sv_width + 1 .. sv_width]; // sv_width is still always 32, 39, 48, 56.
assert(stage != VS_Stage ==> extra_x4_bits == zeros());
let base_ppn = xatp_to_ppn(stage) + zero_extend(extra_x4_bits);

Also then I think you only need two values in TranslationStage: VS_Stage and S_Stage because from that point on there's no difference between S and GS (at least none I know about!).

You can also get rid of all the other changes in vmem_types.sail I think.

Does that make sense? I might not have explained super well... (Or I might have misunderstood!)

[nadime15]

Ahh ok, I get it now, that’s quite smart! But I’m not so sure this works, I’m probably missing something here.

1. I think we need to keep G_Stage in order to know which xatp register to access. G_Stage sets the effective address to User Mode (see the eff_priv in translate_stage), and we need to know that we are performing G-Stage translation in case of an implicit VS-Stage access (see pt_walk). Edit: You also need G-Stage to know whether you have consider VMID in TLB look ups, I believe.

2. Assuming we keep G_Stage, sv_width won’t return 32 in the case of RV32, right? It will return 34 bits, so this becomes a problem. A potential workaround would be to just do svAddr[xlen + 1 .. xlen].

3. Changing the length of base_ppn won’t work, at least as of now, because base_ppn is typed to be ppn_bits bits wide (24 or 46). So I think it’s a bit misleading to expand ppn_base to suddenly be 26 bits or 48 bits, and this potentially leads to even more non-trivial changes.

4. Even though I think this is quite smart, it’s not really obvious. In the pt_walk function we actually compute the PTE_Addr and dissect the different VPNs, etc. I think it’s probably not straightforward that you basically enlarged the page table base address much earlier in the call chain.

Another issue, unrelated to this specific one, which I forgot to mention, is that we also need to return either a 32 or 34 bit address depending on the stage we are currently in, or 64 bit. So the return type needs to be modified as well.

[Timmmm]

1. Good point! Alternatively you could pass xatp in as a parameter I guess. Up to you.

2. Not sure I understand this. Do you mean xatp_mode_width? I think that should always return 32, 39, 48, or 56.

3. I don't think we need to change the width of ppn_base - we just need to add a 2 bit value to it (not concatenate).

4. Yeah even though it is a bit of a sneaky trick, it is sooooo much simpler I think we should still do it (assuming it works and there isn't some catch I haven't thought of).

5. Are you sure? Isn't it always 34 or 64 bits?

[radimkrcmar]

I think the trick breaks TLB lookups, since they ignore the root page table address and would therefore alias addresses with different upper two bits. It seems that we're trying too hard to work around the type system, rather than using it...

[nadime15]

I think the trick breaks TLB lookups, since they ignore the root page table address and would therefore alias addresses with different upper two bits. It seems that we're trying too hard to work around the type system, rather than using it...

Yeah exactly.. I ran into this while implementing the discussed solution. I have a commit I am about to push that shows the progress and the issue that @radimkrcmar has mentionned

[nadime15]

See f8990ea and specifically match_TLB_Entry().

We only consider up to 36 bits for the VPN, which won’t work in the case of a GPA address. I think this trick really simplifies the code, but it seems we run into issues here.

It seems that we're trying too hard to work around the type system, rather than using it...

Any suggestions? In the previous commit we at least made sure that we had the full VPN. It was a bit more complicated, but I think it was more obvious what was actually going on.

Another option would be to expand translate() and pass these two additional bits to the function as well, or do something like G_Stage(0bXX) and pass them down the call chain. Personally, I’m still in favor of the original approach that explicitly extends the VPN, combined with using gpabits that were introduced by f8990ea.

I introduced gpabits that are either 34 bits or 64 bits long for VS/S/G-stage access. I think this addresses your comment (#1513 (comment)), @radimkrcmar. I also think this is cleaner than using bits(xlen + 2), which feels a bit weird, especially when xlen = 64.