Check tcontrol.mte in trigger_t::allow_action()

[rtwfroody]

Otherwise native triggers in M-Mode would not be taken, even if tcontrol.mte is set.

[rtwfroody]

@YenHaoChen Does this look right to you?

[YenHaoChen]

The PR modifies the function trigger_t::allow_action(). Previously, the trigger_t::allow_action() aimed at Solution 1 to the reentrancy problem. The PR adds the behavior of Solution 2 to the reentrancy problem into the trigger_t::allow_action().

Based on my reading, it makes more sense to skip the whole checking of Solution 1 if tcontrol exists. The following shows a prototype of skipping. The difference between this PR and the prototype is the effectiveness of (v)status.SIE. The PR keeps the effectiveness of (v)status.SIE if tcontrol exists, which is not the behavior stated in the debug spec.

bool trigger_t::allow_action(const state_t * const state) const
{
  if (get_action() == ACTION_DEBUG_EXCEPTION) {
    // Solution 2 (read-only-0 medeleg[3], tcontrol exist)
    if (...exist tcontrol...)
      return (state->prv != PRV_M) || (state->tcontrol->read() & CSR_TCONTROL_MTE);

    // Solution 1 (writable medeleg[3], no tcontrol)
    const bool mstatus_mie = state->mstatus->read() & MSTATUS_MIE;
    const bool sstatus_sie = state->sstatus->read() & MSTATUS_SIE;
    const bool vsstatus_sie = state->vsstatus->read() & MSTATUS_SIE;
    const bool medeleg_breakpoint = (state->medeleg->read() >> CAUSE_BREAKPOINT) & 1;
    const bool hedeleg_breakpoint = (state->hedeleg->read() >> CAUSE_BREAKPOINT) & 1;
    return (state->prv != PRV_M || mstatus_mie) &&
           (state->prv != PRV_S || state->v || !medeleg_breakpoint || sstatus_sie) &&
           (state->prv != PRV_S || !state->v || !medeleg_breakpoint || !hedeleg_breakpoint || vsstatus_sie);
  }
  return true;
}

We must have a new option for the tcontrol existence to make this right. A previous discussion (#1742 (comment)) concluded that the option should come through the ISA string. Unfortunately, no extension controls the existence of tcontrol (nor the writability of medeleg[3]). Will we have a sub-extension of Sdtrig, e.g., Sdtcontrol? Alternatively, how about adding a runtime option (flag) to Spike, e.g., --exist-tcontrol? (Notice that the Sdtcontrol or --exist-tcontrol implies a read-only-0 medeleg[3].) @rtwfroody @aswaterman

[aswaterman]

@YenHaoChen this is not only an issue for Spike; it is also an issue for software and for describing HW implementations. IMO, the best fix is to define this optionality through one or more ISA extensions. If that proves to be the right way to go, then this needs to be resolved through the TG.

cc @bruceable: I think this one might be of interest to you.

[rtwfroody]

For better or worse, the debug spec gives implementations a lot of options. I had hoped that https://github.com/riscv/configuration-structure would have solved this issue by now, but I see no signs of progress there.

I still think that is the right solution, because there are a lot of implementation decisions and we can't give them all separate extension names.

Since Andrew doesn't want command line arguments to choose the spike behavior, spike should implement exactly one of the two options. I prefer option 2 as it's the most useful of the two options.

Software can try to write tcontrol.mte to see if solution 2 is implemented, and can similarly experiment to see if solution 1 is implemented.

Thoughts?

[aswaterman]

The direction that RISC-V is going is to give names to even small features. See e.g. the several extensions defined in the profiles https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc#rva23s64-mandatory-extensions -- things like "Shcounterenw: For any hpmcounter that is not read-only zero, the corresponding bit in hcounteren must be writable" are analogous to this case.

The debug spec isn't going to get an exemption from this treatment; the command-line option approach is just kicking the can down the road.

[bruceable]

I'm in agreement with Tim, solution 2 is the preferred choice. Native hw triggers have usefulness in M mode; solution 1 locks out the feature.

…

On Mon, Aug 19, 2024 at 2:14 PM Andrew Waterman ***@***.***> wrote: The direction that RISC-V is going is to give names to even small features, as a means to succinctly describe what an implementation does. See e.g. the several extensions defined in the profiles https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc#rva23s64-mandatory-extensions -- things like "Shcounterenw: For any hpmcounter that is not read-only zero, the corresponding bit in hcounteren must be writable" are analogous to this case. The debug spec isn't going to get an exemption from this treatment; the command-line option approach is just kicking the can down the road. — Reply to this email directly, view it on GitHub <#1777 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJOFJAGBFTUHVRQUNJSHAZLZSJNZVAVCNFSM6AAAAABMUP4LQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJXGQ3DOOBQGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[YenHaoChen]

I’m not a debugger developer, but I’m curious about this issue.

Native hw triggers have usefulness in M mode; solution 1 locks out the feature.

From what I’ve read, both Solution 1 and Solution 2 address the same reentrancy problem and offer the same feature. Am I misunderstanding something?

If Solution 2 offers more features than Solution 1, is handling the EBREAK exception in S-mode less valuable compared to the additional features provided by Solution 2?

[rtwfroody]

The direction that RISC-V is going is to give names to even small features. See e.g. the several extensions defined in the profiles https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc#rva23s64-mandatory-extensions -- things like "Shcounterenw: For any hpmcounter that is not read-only zero, the corresponding bit in hcounteren must be writable" are analogous to this case.

The debug spec isn't going to get an exemption from this treatment; the command-line option approach is just kicking the can down the road.

OK. While the Debug TG is figuring out what the requirements are and responding to them, I'd like to see spike pick option 2 and implement only that. Implementing both is definitely against the intention of the spec, and arguably against the letter of the spec also. I'm not asking for a build or run-time option. Let's just axe the simpler feature (which is probably just removing a couple of lines). If that is OK, I'll put together a PR.

With my spec hat on: Is there anything written down with guidelines or something that would help us decide what kind of optional feature would warrant a separate extension name?

[rtwfroody]

From what I’ve read, both Solution 1 and Solution 2 address the same reentrancy problem and offer the same feature. Am I misunderstanding something?

In solution 1, triggers are always disabled in M-Mode. In solution 2, triggers are only disabled in M-Mode if mte is 0. This allows clever M-Mode code to enable triggers for part of its execution, which means it can be debugged more effectively.

[aswaterman]

Whatever we do, note that there is an SBI expectation that S-mode EBREAK instructions are sent to the S-mode trap handler. Normally that is handled by setting medeleg[3]=1. If that is no longer a valid option, then some changes to OpenSBI, pk, etc. will be needed to maintain that API.

[rtwfroody]

The behavior we're talking about here is just for triggers. EBREAK instructions are their own thing, both in the spec and in the spike implementation.

[aswaterman]

@rtwfroody I don't follow. How does wiring medeleg[3] to 0 not affect EBREAK? What's the mechanism for delegating ebreaks within S-mode to S-mode when the delegation setting is off?

[rtwfroody]

@rtwfroody I don't follow. How does wiring medeleg[3] to 0 not affect EBREAK? What's the mechanism for delegating ebreaks within S-mode to S-mode when the delegation setting is off?

You are right. I was being too hasty. Option 2 only makes sense on systems without S-Mode, so for spike that means it's only possible when --priv=m is passed.

So our options are:

1. Implement solution 1 only.
2. Implement solution 2 when --priv=m is passed, otherwise implement solution 1 only.

Which do you prefer? 1 is simpler. 2 is more useful if you think of spike as a reference platform.

[aswaterman]

@rtwfroody OK, thanks for the sanity check.

I think your second option will maximize utility while minimizing breakage, but if it's too complex, I don't have a problem with the first option. Also curious to hear @YenHaoChen's opinion.

[YenHaoChen]

I am good with Option 2.

How about --priv=mu? Do we want to go with Solution 2 since it also does not have S-mode? In this case, the prototype is as follows. (We may need to pass additional argument proc to trigger_t::allow_action(). Please do not hestitate to let me know if you want me to provide a working commit.)

bool trigger_t::allow_action(const state_t * const state) const
{
  if (get_action() == ACTION_DEBUG_EXCEPTION) {
    // Solution 2 (read-only-0 medeleg[3], tcontrol exist)
    if (!proc->extension_enabled('S'))
      return (state->prv != PRV_M) || (state->tcontrol->read() & CSR_TCONTROL_MTE);

    // Solution 1 (writable medeleg[3], no tcontrol)
    const bool mstatus_mie = state->mstatus->read() & MSTATUS_MIE;
    const bool sstatus_sie = state->sstatus->read() & MSTATUS_SIE;
    const bool vsstatus_sie = state->vsstatus->read() & MSTATUS_SIE;
    const bool medeleg_breakpoint = (state->medeleg->read() >> CAUSE_BREAKPOINT) & 1;
    const bool hedeleg_breakpoint = (state->hedeleg->read() >> CAUSE_BREAKPOINT) & 1;
    return (state->prv != PRV_M || mstatus_mie) &&
           (state->prv != PRV_S || state->v || !medeleg_breakpoint || sstatus_sie) &&
           (state->prv != PRV_S || !state->v || !medeleg_breakpoint || !hedeleg_breakpoint || vsstatus_sie);
  }
  return true;
}