openhwgroup / cva6

The CORE-V CVA6 is an Application class 6-stage RISC-V CPU capable of booting Linux

https://docs.openhwgroup.org/projects/cva6-user-manual/

Other

2.14k stars 652 forks source link

[BUG] : MSTATUS.mpp #2274

Closed AyoubJalali closed 1 week ago

AyoubJalali commented 1 week ago

Is there an existing CVA6 bug for this?

[X] I have searched the existing bug issues

Bug Description

Hello, This is a RTL issue related to mstatus's field mpp, according to the CV32A65X Spec, mpp flied is a WARL that can hold only the value 3'h11 (PRIV_LVL_M), after debugging the RTL Code coverage I have a coverage condition that should not be covered in our configuration : that means that the mpp hold the value 3'h10 (PRIV_LVL_HS), that's a bug in the cv32a65x, also the hole code seems weird, how can test if mpp == PRIV_LVL_HS if we don't support Hypervisor mode.

this code was added by the PR #2035

Moschn commented 1 week ago

Hmm, I am responsible for this behavior. The MPP field is supposed to hold the previous privilege mode before a trap. So to me it felt clear that without the RVH extension, the core cannot support the H privilege mode. After double checking, the H privilege mode is specified as reserved in the risc-v spec.

Maybe I misunderstood something but it seems as if MPP should not accept PRIV_LEVEL_HS if the hypervisor mode is not enabled, which is the current behavior in the RTL.

Moschn commented 1 week ago

Oh I understand the issue now. There are a few parts to this:

The current RTL is partially wrong. The intended behavior is that if the MPP field is written with HS mode, then this is an illegal write and the value in the field should remain on a legal value (WARL). The current behavior just sets the MPP value to U-mode, which is incorrect as there are some configurations where this mode does not exist. Therefore, mstatus_d.mpp should not be set to user mode, but rather to the previous MPP value mstatus_q.mpp.
However, this line should be able to be tested by writing HS mode to MPP and then reading it back. The read-back value should not be HS (if the HS mode is not enabled). Essentially, just testing the WARL behavior of the MPP field.

AyoubJalali commented 1 week ago

Hello @Moschn, the bug is still there, you fix a part of it, because the code shown still there, you'll find it in here this also need fix

JeanRochCoulon commented 1 week ago

@AyoubJalali I am preparing a fix, to be reviewed by both of you

Moschn commented 1 week ago

@AyoubJalali you are correct. I tried to fix it in #2284.

@JeanRochCoulon Oh, I saw your message too late. Feel free to close my PR and use yours.

JeanRochCoulon commented 1 week ago

2285 should fix the issue. @AyoubJalali ?

AyoubJalali commented 1 week ago

2285 should fix the issue. @AyoubJalali ?

LGTM