sail-riscv icon indicating copy to clipboard operation
sail-riscv copied to clipboard

Check the pmpcfg_n cause segmentation fault

Open KotorinMinami opened this issue 4 months ago • 7 comments

When I rebased #601 to the latest version, I found that checking if the L field in the Pmpcfg register was set caused a segmentation fault.

$ ./build/c_emulator/sail_riscv_sim build/test/2025-06-22/riscv-tests/rv64mi-p-ma_addr 
Running file build/test/2025-06-22/riscv-tests/rv64mi-p-ma_addr.
Segmentation fault         (core dumped) ./build/c_emulator/sail_riscv_sim build/test/2025-06-22/riscv-tests/rv64mi-p-ma_addr

These are the functions I checked in model/riscv_pmp_regs:

function checkPmpNcfgL() -> bool = {
  foreach (i from 0 to 63) {
    let test = pmpcfg_n[i];
    if pmpLocked(test) then return true
  };
  false
}

I call this function in model/riscv_sys_regs as follows:

val checkPmpNcfgL : unit -> bool

function legalize_mseccfg(o : Seccfg, v : bits(64)) -> Seccfg = {
  let sseed_read_only_zero =
      (config extensions.Zkr.sseed_read_only_zero : bool)
    | not(currentlyEnabled(Ext_S))
    | not(currentlyEnabled(Ext_Zkr));
  let useed_read_only_zero =
      (config extensions.Zkr.useed_read_only_zero : bool)
    | not(currentlyEnabled(Ext_U))
    | not(currentlyEnabled(Ext_Zkr));
  let rlb_read_only_zero =
      not(currentlyEnabled(Ext_Smepmp))
    | (o[RLB] == 0b0 & checkPmpNcfgL());
  let mml_unable_to_set =
      not(currentlyEnabled(Ext_Smepmp))
    | (o[MML] == 0b1);
  let mmwp_unable_to_set =
      not(currentlyEnabled(Ext_Smepmp))
    | (o[MMWP] == 0b1);

  let v = Mk_Seccfg(v);
  [o with
    SSEED = if sseed_read_only_zero then 0b0 else v[SSEED],
    USEED = if useed_read_only_zero then 0b0 else v[USEED],
    RLB   = if rlb_read_only_zero then 0b0 else v[RLB],
    MMWP  = if mmwp_unable_to_set then o[MMWP] else v[MMWP],
    MML   = if mml_unable_to_set then o[MML] else v[MML],
  ]
}

Can anyone share similar experiences to help me out?

KotorinMinami avatar Jul 23 '25 14:07 KotorinMinami

Nothing you do in Sail should trigger a segmentation fault. Did you push the code that triggers this to https://github.com/riscv/sail-riscv/pull/601 already? I can try to reproduce it if so.

Otherwise it might be useful to build with debug info, and try using address sanitiser or valgrind to get more information.

Alasdair avatar Jul 23 '25 14:07 Alasdair

yes, then the CI told me that every test false because segmentation fault and I have found that it's such code that cause the fault.

KotorinMinami avatar Jul 23 '25 15:07 KotorinMinami

I can reproduce this. The problem is the code is this:

register mseccfg : Seccfg = legalize_mseccfg(Mk_Seccfg(zeros()), zeros())

In legalize_mseccfg() it calls checkPmpNcfgL(63) which eventually reads pmpcfg_n.

However if you look at model_init then the call to initialise mseccfg is before pmpcfg_n is initialised:

void model_init() {
...
  zmseccfg = zlegalizze_mseccfg(z3zE1752, UINT64_C(0x0000000000000000));
...
  zinitializze_registers(UNIT);
}

I'm kind of surprised we didn't run into this before. It seems to only happen if the register is a vector of bitfields; if it's just a vector of bits(..) then it gets initialised directly in model_init() rather than in zinitializze_registers().

Here's a smaller reproducer:

bitfield Pmpcfg_ent : bits(8) = {
  L : 7,
}

register pmpcfg_n : vector(64, Pmpcfg_ent)

register foo : bits(8) = pmpcfg_n[63].bits

function main() -> unit = {
  print_endline(bits_str(foo));
}

Timmmm avatar Jul 24 '25 16:07 Timmmm

If you initialise the register then it works:

register pmpcfg_n : vector(64, Pmpcfg_ent) = vector_init(Mk_Pmpcfg_ent(zeros()))

Timmmm avatar Jul 24 '25 16:07 Timmmm

Ok, I see the problem. Looks like we need to initialise registers in the correct dependency order? Probably a good time to re-factor the register initialisation logic anyway.

On the other hand here it is revealing an underlying bug, because I assume it isn't intended to use an undefined pmpcfg_n to determine the initial state of mseccfg.

Alasdair avatar Jul 24 '25 19:07 Alasdair

Yeah maybe time to just require all registers to be initialised full stop. Still you'll probably need to deal with the order anyway I would have thought?

Timmmm avatar Jul 24 '25 21:07 Timmmm

Not sure what you mean by register initialization, but there are very few bits that need iniitialization (e.g. PMP lock bits, mode bits, and mseccfg bits). but they should all be initialized before the model is out of reset and anything is being fetched. If that's not working, I'd expect some odd behavior - but not a segfault.

On Thu, Jul 24, 2025 at 2:16 PM Tim Hutt @.***> wrote:

Timmmm left a comment (riscv/sail-riscv#1162) https://github.com/riscv/sail-riscv/issues/1162#issuecomment-3114984272

Yeah maybe time to just require all registers to be initialised full stop. Still you'll probably need to deal with the order anyway I would have thought?

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/issues/1162#issuecomment-3114984272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJV5X4HYZLTVU4I3QWT3KFEJPAVCNFSM6AAAAACCGDVQRWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMJUHE4DIMRXGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

allenjbaum avatar Jul 25 '25 00:07 allenjbaum