search

riscv / sail-riscv

Sail RISC-V model
https://lists.riscv.org/g/tech-golden-model
Other
463 stars 168 forks source link

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

Open Ali-Faraz-10xe opened 4 days ago

Ali-Faraz-10xe commented 4 days ago

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.

allenjbaum commented 3 days ago

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

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 https://github.com/riscv/sail-riscv/pull/620#discussion_r1854638069:

 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 https://github.com/riscv/sail-riscv/pull/620#pullrequestreview-2455612616, 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 commented 2 days ago

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:

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

Timmmm commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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.