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

define asynchronous interrupt priorities #802

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

Conversation

JamesKenneyImperas
Copy link

Hi @timsifive - I've made an attempt at this (reference #796)

Copy link
Contributor

@timsifive timsifive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec changes look right to me. I have two requests for the PR:

  1. Can you give it a more descriptive title? Something like "define asynchronous interrupt priorities" or anything where someone reading just the title could guess what the change relates to.
  2. Don't include the PDF in the PR. It just leads to conflicts and a lot of churn.

@JamesKenneyImperas JamesKenneyImperas changed the title tentative attempt to fix issue 796 define asynchronous interrupt priorities Feb 14, 2023
Copy link
Contributor

@timsifive timsifive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'd like @pdonahue-ventana's opinion too. He has a better head for stuff like this.

@pdonahue-ventana
Copy link
Collaborator

This table in the debug spec mirrors table 3.7 of the latest priv spec which doesn't have interrupts. This PR specifies the priority of interrupts with respect to non-debug exceptions, not just debug exceptions. I don't think that the priv spec defines that interrupt vs. synchronous exception priority and it's not appropriate for the debug spec to define that.

@JamesKenneyImperas
Copy link
Author

Hi @pdonahue-ventana,

Table 3.7 doesn't mention interrupts because they are not synchronous traps on the current instruction resulting from execution of that instruction. The behaviour of interrupts in the case we are considering here (enabling a pending interrupt by execution of xret or explicit CSR write) is explicitly covered by paragraph 3.1.9 (see issue #796). The Privileged Specification is clear that enabling a pending interrupt in this way results in the interrupt occurring at higher priority than anything else before the next instruction is executed. In debug terminology, it is effectively a high priority synchronous event with timing 'after' the current instruction.

The issue is that the Debug Specification introduces other high priority synchronous events with timing 'after' (e.g. stepi trigger events), and therefore needs to define the priority of interrupts with respect to those. For example, if you stepi over an xret that enables interrupts, is it the interrupt that happens next or the stepi breakpoint?

I agree that it feels a bit awkward inserting interrupts into the table as I have tried to do; can you think of a better way to make clear where interrupts sit?

Thanks,

James.

@pdonahue-ventana
Copy link
Collaborator

I see your point. Let me think about it.

@timsifive timsifive added the 1.0+ Nice-to-have for 1.0 label Apr 5, 2023
@timsifive
Copy link
Contributor

Is this a bugfix, clarification, or something else?

@JamesKenneyImperas
Copy link
Author

It's an attempt to specify something that is missing entirely at the moment - see our previous discussion on #796. Not sure whether that counts as a bugfix or clarification.

@YenHaoChen
Copy link
Contributor

This PR clarifies the priority between precise/synchronous interrupts and triggers with timing=after. The current Spike does not provide this behavior. Is there any update on this clarification?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0+ Nice-to-have for 1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants