fix: Print MTI value upon hart update for machine timer interrupt visibility

[Ali-Faraz-10xe]

Hi!

I have developed the ACTs (Architectural Compliance Tests) and coverage points for RISC-V compliance verification. As part of the coverage points, I needed support for printing the MTI (Machine Timer Interrupt) value upon a hart update to improve the visibility and tracking of machine interrupts during the test execution.

To achieve this, I made a small change to one of the print functions, ensuring that the MTI value is printed whenever the hart state is updated. This change is essential for accurate compliance verification and enhances the observability of machine interrupts.

Please review and consider adding this change to the original repository for inclusion in the RISC-V architecture test suite.

[Timmmm]

This doesn't seem to change functionality... But there's no way we're going to guarantee that this message output is stable.

Tests shouldn't be parsing log output. However since we haven't got a proper solution yet and this is a pretty small change it's probably ok for now. Just don't expect it to stay like this.

[Timmmm]

Can delete this line.

[Timmmm]

Can just be else no?

[Timmmm]

Message is wrong

[arichardson]

This is redundant due to the assignment above. Either change this to an else and delete the assignment above or drop this assignment.

[allenjbaum]

Let me see if I've got this straight. hart state gets updated at every cycle (even a noop updates the PC. so, does this message get printed every cycle? The count rate is then a constant 1/cycle, and track mcycle. That's legal. But I'm not sure how you can easily write a test where the reference model (e.g. Sail) updates at a different rate than the DUT - which is inevitably true since the rate is implementation dependent. We could pass a cycle divider number into sail, so it only counts on every n-th cycle, but that still isn't perfect. Test don't parse the log output, as such - but tools (e.g. ISAC) rely on it.

…

On Fri, Nov 22, 2024 at 12:46 PM Alexander Richardson < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In model/riscv_platform.sail <#620 (comment)>: > if get_config_print_platform() - then print_platform(" clint timer pending at mtime " ^ BitStr(mtime)); - mip[MTI] = 0b1 + then print_platform(" clint timer pending at mtime " ^ BitStr(mtime) ^ " (mip.MTI <- " ^ BitStr(mip[MTI]) ^ ")"); + } + else if mtimecmp >_u mtime then { + mip[MTI] = 0b0; This is redundant due to the assignment above. Either change this to an else and delete the assignment above or drop this assignment. — Reply to this email directly, view it on GitHub <#620 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJXWXFCJUDPEPXRYII32B6J2PAVCNFSM6AAAAABSHDASASVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINJVGYYTENRRGY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Ali-Faraz-10xe]

Thank you all for your detailed reviews and concerns. I've found a simpler solution that addresses the concerns:

1. I tested by removing the else condition entirely, since MTI=0 is already set at the start:

function clint_dispatch() -> unit = {
  if   get_config_print_platform()
  then print_platform("clint::tick mtime <- " ^ BitStr(mtime));
  mip[MTI] = 0b0;
  if mtimecmp <=_u mtime then {
    mip[MTI] = 0b1;
    if   get_config_print_platform()
    then print_platform(" clint timer pending at mtime " ^ BitStr(mtime) ^ " (mip.MTI <- " ^ BitStr(mip[MTI]) ^ ")");
  }
}

This simpler version:

• Removes redundant code

• Simplifies the logic

• Only prints when timer is actually pending

• Still achieves 100% coverage for my test cases

I've verified this gives me the coverage I need while making the code cleaner. Would this be a better approach?

[Timmmm]

That appears to be very similar to the existing code no? I think that's fine, but why do you need to change it at all?

Also you can hardcode "mip.MTI <- 1" in the message.

[MuhammadHammad001]

Also you can hardcode "mip.MTI <- 1" in the message.

But, IMO the value of mip.MTI needs to be updated using mip[MTI] = 0b1; for architectural state maintenance. (If you are saying to update it and then simply use 1 instead of the variable in the print statement, then, I guess I misinterpreted it:) )

I think the reason for updating the log message for mip.MTI value is to have a similar output format as of the mip.MSI here

    if   get_config_print_platform()
    then print_platform("clint[" ^ BitStr(addr) ^ "] <- " ^ BitStr(data) ^ " (mip.MSI <- " ^ BitStr(data[0]) ^ ")");
    mip[MSI] = [data[0]];
    clint_dispatch();
    MemValue(true)

To explain it a bit, whenever we have a software interrupt pending (by writing a value on the base address of MSIP), we see the following message in the log:

mem[X,0x8000029C] -> 0xA023
mem[X,0x8000029E] -> 0x01E7
[140] [M]: 0x8000029C (0x01E7A023) sw t5, 0x0(a5)
clint[0x00000000] <- 0x00000001 (mip.MSI <- 0b1)
clint::tick mtime <- 0x0000000000000001

But, for the case of timer interrupt pending which we can only be brought by making the mtime>= mtimecmp, we just simply see the following code, while on the back end (by the hart) the value of mip.MTI is updated to 1 which is not visible to the user:

mem[X,0x80000298] -> 0x2023
mem[X,0x8000029A] -> 0x01E8
[139] [M]: 0x80000298 (0x01E82023) sw t5, 0x0(a6)
clint<4>[0x0000BFF8] <- 0x0000FFFF (mtime)
clint::tick mtime <- 0x000000000000FFFF
 clint timer pending at mtime 0x000000000000FFFF

Therefore, updating the log message will change it to the following and will make the updated mip.MTI visible to the user as in the case of software interrupt:

mem[X,0x80000298] -> 0x2023
mem[X,0x8000029A] -> 0x01E8
[139] [M]: 0x80000298 (0x01E82023) sw t5, 0x0(a6)
clint<4>[0x0000BFF8] <- 0x0000FFFF (mtime)
clint::tick mtime <- 0x000000000000FFFF
clint timer pending at mtime 0x000000000000FFFF (mip.MTI <- 0b1)

[MuhammadHammad001]

Additionally, as soon as we write something on the memory mapped address MSIP, we see the reflected value updated in the mip.MSi in the log something like:

mem[X,0x800008FC] -> 0xA023
mem[X,0x800008FE] -> 0x0002
[203] [M]: 0x800008FC (0x0002A023) sw zero, 0x0(t0)
clint[0x00000000] <- 0x00000000 (mip.MSI <- 0b0)
clint::tick mtime <- 0x0000000000000002

Since, the value of mip.MTI depends on the mtime/mtimecmp update and IMO mip.MTI should also be visible on every mtime update by using the logic something similar to:

function clint_dispatch() -> unit = {
  if mtimecmp <=_u mtime then {
    mip[MTI] = 0b1;
    if   get_config_print_platform() then {
        print_platform("clint::tick mtime <- " ^ BitStr(mtime) ^ " (mip.MTI <- " ^ BitStr(mip[MTI]) ^ ")" ); 
        print_platform(" clint timer pending at mtime " ^ BitStr(mtime) );
  }
  else{
    mip[MTI] = 0b0;
    if   get_config_print_platform()
        then print_platform("clint::tick mtime <- " ^ BitStr(mtime) ^ " (mip.MTI <- " ^ BitStr(mip[MTI]) ^ ")" );
}
}

But, that's just a suggestion.

[Timmmm]

So as I understand it you just want to change the logging. How about this?

function clint_dispatch() -> unit = {
  mip[MTI] = bool_to_bits(mtimecmp <=_u mtime);

  if   get_config_print_platform()
  then print_platform(" clint mtime " ^ bits_str(mtime) ^ ": mip.MTI <- " ^ bits_str(mip[MTI]));
}

Much simpler.