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

Q about Table 9 (action encoding) #995

Open
martinberger opened this issue Mar 27, 2024 · 8 comments
Open

Q about Table 9 (action encoding) #995

martinberger opened this issue Mar 27, 2024 · 8 comments

Comments

@martinberger
Copy link

martinberger commented Mar 27, 2024

I have a question about Table 9 (action encoding) in Section 5.2 on Page 59 in 1.0.0-rc2. In the row for the action with encoding 1 we have the sentence

"hardware should clear the action field whenever the action field is 1, the new value of dmode would be 0, and the new value of he action field would be 1."

It is unclear because the sentences asks to "clear" the action field but also asks to set it to 1. This clearing to 1 should only happen if the action field is 1.

(There is also a typo: "he action field" should the "the action field".)

@pdonahue-ventana
Copy link
Collaborator

IF the current action field is 1 and hardware is doing a write that would write (1) dmode=0 and (2) action=1 THEN hardware should write action=0 (rather than allowing item 2).

@tomaird
Copy link

tomaird commented Apr 4, 2024

What if the current action field is 0 (or any legal non-1 value), and hardware does a write that would write (1) dmode=0 and (2) action=1, should hardware write action=0 or 1?

The spec currently suggests it should write action=1 as normal. But this would also result in dmode=0 and action=1. Which seems to conflict with the description for action=1 - "This action is only legal when the trigger’s dmode is 1".

Is the intention that the hardware should never get in a state where dmode=0 and action=1 or is there some other way of handling this state. i.e. if a trigger fires when dmode=0 and action=1 it should just do nothing?

@tomaird
Copy link

tomaird commented Apr 4, 2024

From your answers to previous issues (#746 and #749), it seems the intention is that it's impossible to get in the state where dmode=0 and action=1, in which case I think the spec wording is unclear.

I propose this sentence:

Since the tdata registers are WARL, hardware should clear the action field whenever the action field is 1, the new value of dmode would be 0, and the new value of he action field would be 1.

Should be changed to this:

Since the tdata registers are WARL, hardware should clear the action field whenever the new value of dmode would be 0, and the new value of the action field would be 1.

Since it doesn't actually matter what the current value of action is, it only matters what the resulting combination of action and dmode are

@pdonahue-ventana
Copy link
Collaborator

Yes, it's illegal to have dmode=0 and action=1 and the WARL logic must prevent that situation. The spec provides a recommendation for one specific situation (see #501 for some history).

A debugger can attempt to write dmode=0 and action=1 in multiple scenarios:

  1. The previous state had action=1 and dmode=1.
    • The current spec directly addresses this.
    • This might be a buggy external debugger (but not a buggy native debugger).
    • The WARL hardware could either:
      • write dmode=1 and action=1 (dropping the write)
      • write dmode=0 and something fixed like action=0 (which is the current spec's recommendation)
  2. The previous state had action!=1 and dmode=1.
    • This might be a buggy external debugger (but not a buggy native debugger).
    • The WARL hardware could either:
      • write dmode=1 and action=1 (dropping the write to dmode)
      • write dmode=0 and something fixed like action=0 (which is your proposal)
      • write dmode=0 and keep the original value of action
  3. The previous state had action!=1 and dmode=0.
    • This might be a buggy native or external debugger.
    • The WARL hardware must write dmode=0 (which was both the previous and new value)
    • The WARL hardware could either:
      • write something fixed like action=0 (which is your proposal)
      • keep the original value of action

I think that the spec didn't want to pick one of the options on scenarios 2 and 3. I don't really know why.

@tomaird
Copy link

tomaird commented Apr 5, 2024

I don't think it's clear enough that setting action=1 and dmode=0 is a mandatory restriction but the example way of dealing with it is only a suggestion. The two flow together to imply that both are restrictions.

I also think it's quite confusing for the spec to provide a suggestion for 1 scenario and not for the other 2.

It would be better to either:

  1. Provide a suggestion for 1 scenario, but make it clear that it's a suggestion and also that there are other scenarios to consider
  2. Provide a suggestion for all 3 scenarios, again making it clear that they are suggestions.
  3. Provide no suggestions, only what is actually restricted, and leave it entirely up to each implementation to decide how to handle all the scenarios

@pdonahue-ventana
Copy link
Collaborator

I'd personally choose 3 and say something like "Since the tdata registers are WARL, hardware must prevent them from containing dmode=0 and action=1." @rtwfroody: You came up with the original language. Thoughts?

@rtwfroody
Copy link
Collaborator

I agree that the right thing is to document the invalid value, and leave it up to the hardware what valid value it ends up with.

Since the tdata registers are WARL, hardware must prevent them from containing dmode=0 and action=1.

If we say must, then that's not backwards-compatible. "Should" is backwards-compatible.

Do we need ARC review to make the change using "should?"

@pdonahue-ventana
Copy link
Collaborator

There was always a requirement that hardware must prevent dmode=0 and action=1. "This action is only legal when the trigger's dmode is 1" means that dmode=0 and action=1 is illegal. WARL says that "If a value is unsupported, the implementation converts the value to one that is supported." Previously, there was a recommendation about the specific supported value that hardware would convert to in one particular case. My suggestion is to remove that and just specify the requirement.

We could remove that WARL sentence altogether and just say "This action is only legal when the trigger's dmode is 1" but apparently people didn't understand that the WARL hardware needs to prevent the illegal situation.

rtwfroody added a commit that referenced this issue Apr 24, 2024
Instead of describing a recommended implementation (which didn't cover
all cases), explicitly state the value that is illegal and let hardware
choose how to do that.

Addresses #995.
rtwfroody added a commit that referenced this issue Apr 25, 2024
Instead of describing a recommended implementation (which didn't cover
all cases), explicitly state the value that is illegal and let hardware
choose how to do that.

Addresses #995.
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

No branches or pull requests

4 participants