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

AR: Clarify itrigger and trigger number translation #903

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Conversation

timsifive
Copy link
Contributor

itrigger numbers relate to the value written to *cause when the trap is taken.

Addresses #889.

Comment on lines 1139 to 1141
trap is taken. For interrupts whose numbers are translated, that means
the number checked is the one in the mode that the trap handler executes
in.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that this sentence will make sense to people who aren't familiar with AIA. I would say something like:

Suggested change
trap is taken. For interrupts whose numbers are translated, that means
the number checked is the one in the mode that the trap handler executes
in.
trap is taken. For virtualized interrupts that have different interrupt numbers
when viewed from different modes, that means
the number checked is the one in the mode that the trap handler executes
in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also need to say something about how we don't care why the interrupt is taken? It could be that the input wire asserted, it could be that M mode wrote mie.STIE=1 in the original priv arch, it could be that the hypervisor is playing with hvip, etc. This was one of John's concerns. We don't have to spell out the cases but we should say something about how the trigger is based on the interrupt trap regardless of the original cause of the interrupt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of agree with you, but I'm not sure how to say that. Aren't interrupt traps by definition taken in response to interrupts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that John's question was whether one category of reasons for taking an interrupt would be treated differently than another category of reasons for taking an interrupt.

Rereading what's currently there, it basically says that the trigger matches/fires only if we trap but it doesn't say that it fires every time that we trap. Maybe it's as simple as saying "The trigger fires if and only if the hart takes a trap because of the interrupt." That would presumably clarify that the reason for the trap doesn't matter.


Hardware may only support a subset of interrupts for this trigger. A
debugger must read back \RcsrTdataTwo after writing it to confirm the
requested functionality is actually supported.

The trigger only fires if the hart takes a trap because of the
interrupt. (E.g.\ it does not fire when a timer interrupt occurs but that
interrupt is not enabled in \Rmie.)
interrupt. (E.g.\ it does not fire for an interrupt that is masked.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible, I like to avoid "masked" because sometimes you have positive masks (A & mask) and sometimes you have negative masks (A & ~mask) so it can be ambiguous. What you have is probably not that ambiguous but how about using something closer to the original language? That also applies to global enable (mstatus.MIE) rather than just local enables (mie).

Suggested change
interrupt. (E.g.\ it does not fire for an interrupt that is masked.)
interrupt. (E.g.\ it does not fire for an interrupt that is not enabled.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

disabled? Done.

@timsifive
Copy link
Contributor Author

I've rewritten this, hopefully to be more clear.

Comment on lines 1135 to 1136
For interrupts where it is enabled, this trigger fires when an interrupt
trap is taken from a privilege mode where the trigger is enabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't sure what the first "it" is. It seems like it refers to an interrupt but singular "it" doesn't match "interrupts" plural. Then I wondered if it would be clearer to just remove everything before the comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about:
This trigger fires when an interrupt trap is taken from a privilege mode
where the trigger is enabled and that interrupt is also enabled in the
trigger.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I understand. There are a lot of enables here: the mode bits in tdata1, the interrupt number in tdata2, and the interrupt enables (both mie and mstatus.MIE). I like something more like this tweak to the earlier language:

    For an interrupt where that interrupt number is enabled in tdata2, this trigger fires when an interrupt
    trap is taken from a privilege mode where the trigger is enabled.

That doesn't actually talk about the interrupt enables but that's implicit when you talk about taking a trap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to explicitly call out tdata2 here, because nmi is also an option. I agree there are a lot of enables. Maybe I just need to capitulate and make a numbered list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it even simpler. We don't need to talk about the mode at all in this sentence. It's already in the bits. The only real need is to indicate what the trigger is useful for. 2 paragraphs down (When the trigger matches...) already makes it clear that it only happens when a trap is actually taken. We don't need the parenthetical about interrupts being masked or whatever. The trigger fires when the trap is taken. Done.

itrigger numbers relate to the value written to *cause when the trap is
taken.

Addresses #889.
Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

That's simpler but still conveys all the necessary information. Looks good.

@timsifive timsifive merged commit f5b2ed3 into master Oct 12, 2023
1 check passed
@timsifive timsifive deleted the itrigger_fire branch October 12, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants