Implement Hypervisor extension by alexmikhalevich · Pull Request #41 · cartesi/machine-emulator

alexmikhalevich · 2023-05-24T14:26:16Z

Hypervisor extension implementation.

src/riscv-constants.h

src/interpret.cpp

src/riscv-constants.h

src/translate-virtual-address.h

src/interpret.cpp

edubart · 2023-05-25T13:47:14Z

src/interpret.cpp

-                enabled_ints = ~a.read_mideleg();
-            }
-            break;
+    auto mstatus = a.read_mstatus();


I miss the comments explaining what was going on that you added and were lost here.

yeah I'll do this after we settle on the final decision regarding this discussion

edubart · 2023-05-25T13:57:11Z

src/interpret.cpp

+static inline uint32_t get_enabled_irq_mask_HS(STATE_ACCESS &a, uint64_t sie, uint8_t priv) {
+    uint32_t deleg = a.read_mideleg() & ~a.read_hideleg();
+    uint32_t enabled_HS = is_virtual_mode(a) || priv < PRV_HS || (priv == PRV_HS && sie);
+    return (deleg & -enabled_HS);


Shouldn't you do an & sie in the final return? Checking just if sie has any bit set feels odd for me, actually it should check if the corresponding deleg and sie is bits are enabled. Same apply for other get_enabled_irq_mask_* functions. I think these functions may need some rewriting considering this and the previous comments.

this is done at the end of the get_pending_irq_mask method here. maybe the naming of those methods is misleading, do you have any ideas what would be a better naming here?

src/interpret.cpp

edubart · 2023-05-25T19:03:58Z

src/riscv-constants.h

-    PRV_HS = 2, ///< Hypervisor-extended supervisor mode
-    PRV_M = 3   ///< Machine mode
+    PRV_U = 0,     ///< User mode
+    PRV_HS = 1,    ///< Supervisor mode


After reading the spec and the code, I feel that PRV_HS should be renamed back to PRV_S. Because when VRT is 1, we are in VS mode, and the checks have HS in them, so it is misleading.

edubart

I commented on things that may have to be improved after reading the spec. Despite having some issues this is already working well, most issues commented are edge cases or minor details. Great work @alexmikhalevich !

edubart · 2023-05-26T11:27:44Z

src/interpret.cpp

+
+template <typename STATE_ACCESS>
+static execute_status write_csr_vsstatus(STATE_ACCESS &a, uint64_t val) {
+    a.write_vsstatus(val);


A write filter is missing here, we should not allow to writes to WPRI fields of vsstatus to preserve backward compability.

FS and SD fields are not being handled here, they should be handled is similar way to mstatus. While it was handled in write_csr_sstatus, I think you should move the vsstatus related logic from write_csr_sstatus to here and call this function.

When doing this, FS field should always mark state as dirty, like is done in write_csr_mstatus and according to the spec for both mstatus and vsstatus:

Modifying the floating-point state when V=1 causes both fields to be set to 3 (Dirty).

Meaning if vsstatus.FS is set as dirty, then mstatus.FS will also be set as dirty, and both vsstatus.SD and mstatus.SD must also be set.

edubart · 2023-05-26T11:44:16Z

src/interpret.cpp

+    if (is_virtual_mode(a)) {
+        uint64_t vsstatus = a.read_vsstatus() & VSSTATUS_R_MASK;
+        vsstatus = (vsstatus & ~VSSTATUS_W_MASK) | (val & VSSTATUS_W_MASK);
+        if ((vsstatus & MSTATUS_FS_MASK) == MSTATUS_FS_MASK) {


Despite MSTATUS_FS_MASK being set, it's not ever read. You will find multiple of the following checks in the code:

// If FS is OFF, attempts to read or write the float state will cause an illegal instruction exception. if (unlikely((mstatus & MSTATUS_FS_MASK) == MSTATUS_FS_OFF)) { return execute_status::failure; }

They all should be extended taking this in consideration:

When V=1, both vsstatus.FS and the HS-level sstatus.FS are in effect. Attempts to execute a
floating-point instruction when either field is 0 (Off) raise an illegal-instruction exception.

This mean a exception should be raised if either mstatus.FS is OFF, or vsstatus.FS is Off while in virtual mode.

src/interpret.cpp

edubart · 2023-05-26T13:09:02Z

src/interpret.cpp

+            return execute_HSV_H(a, pc, mcycle, insn);
+        case insn_privileged_funct7::HSV_W:
+            return execute_HSV_W(a, pc, mcycle, insn);
+        // case insn_privileged_funct7::HLV_WU:  // TODO


This needs to be implemented.

The case still commented.

right, that's because this insn is processed in another switch. I removed this comment.

edubart · 2023-06-30T14:25:50Z

src/interpret.cpp

+    // hgatp can only be changed when virtual mode is off, and when virtual mode is off
+    // the address translation function will not use it. Enabling virtual mode will trigger
+    // a TLB shootdown.
+


This is missing:

When mstatus.TVM=1, attempts to read or write hgatp while executing
in HS-mode will raise an illegal instruction exception.

Setting TVM=1 prevents HS-mode from accessing hgatp or executing HFENCE.GVMA or HIN-
VAL.GVMA

The above is also missing in read_csr_hgatp.

edubart · 2023-06-30T15:10:43Z

src/interpret.cpp

+    // a TLB shootdown.
+
+    auto mode = val >> 60;
+    if (mode == 0 || (mode >= 8 && mode <= 10)) {


You can use the enums SATP_MODE_BARE, SATP_MODE_SV39 and SATP_MODE_SV57 instead of these constants here.
And SATP_MODE_SHIFT for the 60 above.

I also think a switch makes the code more clear, like in write_csr_satp. Same applies to write_csr_vsatp, it could have used a switch.

edubart · 2023-06-30T15:24:09Z

src/interpret.cpp

@@ -1975,7 +2687,10 @@ static NO_INLINE execute_status write_csr_satp(STATE_ACCESS &a, uint64_t val) {
    }

    uint64_t old_satp = a.read_satp();


There is an extra unnecessary read of satp here when read_iflags_VRT is true, you could move this read in the else of the bellow if.

edubart · 2023-06-30T15:40:10Z

src/interpret.cpp

@@ -1975,7 +2687,10 @@ static NO_INLINE execute_status write_csr_satp(STATE_ACCESS &a, uint64_t val) {
    }


The TSR and TVM fields of mstatus affect execution only in HS-mode, not in VS-mode. The TW
field affects execution in all modes except M-mode.

This is not respected here, TVM is affecting VS-mode.

edubart · 2023-06-30T15:42:48Z

src/interpret.cpp

+    auto mode = current_mode(a);
+    auto priv = (mode & ACCESS_MODE_PRV_MASK) >> ACCESS_MODE_PRV_SHIFT;
+    uint64_t mstatus = a.read_mstatus();
+    if (unlikely(priv == PRV_U || (priv == PRV_S && (mstatus & MSTATUS_TVM_MASK)))) {


The TSR and TVM fields of mstatus affect execution only in HS-mode, not in VS-mode.

setting TVM=1 does not cause traps for accesses to vsatp or instructions
HFENCE.VVMA or HINVAL.VVMA, or for any actions taken in VS-mode

This is not respected here, TVM is affecting VS-mode.

edubart · 2023-06-30T20:38:47Z

src/interpret.cpp

+    uint64_t old_vsstatus = a.read_vsstatus() & VSSTATUS_R_MASK;
+    uint64_t vsstatus = (old_vsstatus & ~VSSTATUS_W_MASK) | (val & VSSTATUS_W_MASK);
+    auto mstatus = a.read_mstatus();
+    if ((vsstatus & MSTATUS_FS_MASK) == MSTATUS_FS_MASK) {


I think this if should have been (vsstatus & MSTATUS_FS_MASK) != MSTATUS_FS_OFF.

edubart · 2023-06-30T20:49:08Z

src/interpret.cpp

+        vsstatus |= MSTATUS_FS_DIRTY;
+        vsstatus |= MSTATUS_SD_MASK;
+    } else {
+        mstatus &= ~MSTATUS_SD_MASK;


The MSTATUS_FS_MASK could still be dirty, and SD cannot be cleared in this case. So I think this line should be removed.

edubart · 2023-06-30T20:58:02Z

src/interpret.cpp

@@ -2198,6 +2924,12 @@ static inline execute_status write_csr_fflags(STATE_ACCESS &a, uint64_t val) {
    if (unlikely((mstatus & MSTATUS_FS_MASK) == MSTATUS_FS_OFF)) {
        return execute_status::failure;
    }
+    if (a.read_iflags_VRT()) {
+        auto vsstatus = a.read_vsstatus();
+        if (unlikely((vsstatus & MSTATUS_FS_MASK) == MSTATUS_FS_OFF)) {


This check is correct, however there are other places missing the same check, such as:

read_csr_fflags

read_csr_frm

read_csr_fcsr

the two switches in execute_insn

edubart · 2023-06-30T21:43:59Z

src/interpret.cpp

@@ -5407,17 +6608,22 @@ template <typename STATE_ACCESS>
 static FORCE_INLINE fetch_status fetch_translate_pc_slow(STATE_ACCESS &a, uint64_t &pc, uint64_t vaddr,
    unsigned char **phptr) {
    uint64_t paddr{};
+    MODE_constants access_mode = get_current_memory_access_mode(a);


When MPRV=1, load
and store memory addresses are translated and protected, and endianness is applied, as though
the current privilege mode were set to MPP. Instruction address-translation and protection are
unaffected by the setting of MPRV.

get_current_memory_access_mode will always consider MRPV, but for fetch MRPV should be ignored.

Our old code respected this, with this (in translate_virtual_address):

// When MPRV is set, data loads and stores use privilege in MPP // instead of the current privilege level (code access is unaffected) if (xwr_shift != PTE_XWR_X_SHIFT && (mstatus & MSTATUS_MPRV_MASK)) { priv = (mstatus & MSTATUS_MPP_MASK) >> MSTATUS_MPP_SHIFT; }

edubart · 2023-06-30T21:48:27Z

src/translate-virtual-address.h

-                    }
+
+            // when checking the U bit for G translation, the current privilege mode is always taken to be U-mode
+            if constexpr (TRANSLATION_MODE == TRANSLATION_G) {


After making all G translations to use priv=PRV_U, this if could just be removed to simplify this code. The else branch already has the same logic.

alexmikhalevich requested review from diegonehab, edubart, mpernambuco, mpolitzer and vfusco May 24, 2023 14:26

alexmikhalevich self-assigned this May 24, 2023

edubart added enhancement New feature or request epic labels May 24, 2023

edubart linked an issue May 24, 2023 that may be closed by this pull request

RISC-V Hypervisor extension support #44

Open

edubart requested changes May 24, 2023

View reviewed changes

edubart requested changes May 25, 2023

View reviewed changes

edubart requested changes May 26, 2023

View reviewed changes

vfusco deleted the branch main May 29, 2023 22:51

vfusco closed this May 29, 2023

diegonehab reopened this May 30, 2023

diegonehab changed the base branch from develop to main May 30, 2023 16:04

alexmikhalevich force-pushed the hypervisor branch from e49b5a3 to 3b25b67 Compare June 5, 2023 15:30

alexmikhalevich force-pushed the hypervisor branch from 9b9d7cd to ac02322 Compare June 19, 2023 20:20

edubart requested changes Jul 10, 2023

View reviewed changes

alexmikhalevich force-pushed the hypervisor branch from 23c19c1 to e08cc68 Compare July 17, 2023 06:36

alexmikhalevich force-pushed the hypervisor branch from e08cc68 to ef476ff Compare July 24, 2023 12:23

alexmikhalevich force-pushed the hypervisor branch 2 times, most recently from 4c72df3 to f3cd606 Compare August 17, 2023 13:36

feat: implement hypervisor extension

72c94d9

alexmikhalevich force-pushed the hypervisor branch 2 times, most recently from 2f9548f to aeef15b Compare October 15, 2023 00:08

fixup! feat: implement hypervisor extension

c287764

alexmikhalevich force-pushed the hypervisor branch from aeef15b to c287764 Compare October 15, 2023 00:12

alexmikhalevich added 2 commits October 14, 2023 22:32

fixup! feat: implement hypervisor extension

799e5ce

fixup! feat: implement hypervisor extension

9e21789

alexmikhalevich force-pushed the hypervisor branch from 48d0559 to 9e21789 Compare October 29, 2023 23:07

		@@ -1975,7 +2687,10 @@ static NO_INLINE execute_status write_csr_satp(STATE_ACCESS &a, uint64_t val) {
		}

		uint64_t old_satp = a.read_satp();

Conversation

alexmikhalevich commented May 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edubart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment