Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check tcontrol.mte in trigger_t::allow_action() #1777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rtwfroody
Copy link
Contributor

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

Otherwise native triggers in M-Mode would not be taken, even if
tcontrol.mte is set.
@rtwfroody
Copy link
Contributor Author

@YenHaoChen Does this look right to you?

@YenHaoChen
Copy link
Collaborator

YenHaoChen commented Aug 18, 2024

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().

image
image

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
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link
Collaborator

aswaterman commented Aug 19, 2024

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
Copy link

bruceable commented Aug 19, 2024 via email

@YenHaoChen
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

aswaterman commented Aug 20, 2024

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
Copy link
Contributor Author

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.

rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Aug 22, 2024
The spec says to only implement a single option. Option 2 gives software
more flexibility, and we already have an implementation. Let's use that
one. See also riscv-software-src#1777.
@aswaterman
Copy link
Collaborator

aswaterman commented Aug 22, 2024

@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 added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Aug 23, 2024
The spec says to only implement a single option. Option 2 gives software
more flexibility, and we already have an implementation. Let's use that
one. See also riscv-software-src#1777.
@rtwfroody
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator

YenHaoChen commented Aug 23, 2024

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;
}

rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Aug 29, 2024
When S mode is present, use option 1 (disable triggers in M-Mode unless
MIE is set) from the Debug Spec. When S mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Aug 30, 2024
When S mode is present, use option 1 (disable triggers in M-Mode unless
MIE is set) from the Debug Spec. When S mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 2, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 2, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 4, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 5, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 6, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
rtwfroody added a commit to rtwfroody/riscv-isa-sim that referenced this pull request Sep 9, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
NewPaulWalker pushed a commit to OpenXiangShan/riscv-isa-sim that referenced this pull request Sep 30, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
huxuan0307 pushed a commit to OpenXiangShan/riscv-isa-sim that referenced this pull request Sep 30, 2024
When S-mode is present, use option 1 (disable triggers in M-mode unless
MIE is set) from the Debug Spec. When S-mode is not present, use option
2 (implement mte and mpte bits in tcontrol).

See discussion in riscv-software-src#1777.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants