AR: Comment on unimplemented CSR behavior. #870

timsifive · 2023-08-22T19:05:12Z

After a very long e-mail discussion, we landed on maintaining
unimplemented CSR behavior as-is. Just add a quick note to be explicit
about that intention. The end of the discussion is basically:
1. If Sdtrig CSRs are accessed when they are not implemented or Sdtrig
   is not implemented, the hart will trap. This is to maintain backwards
   compatibility with existing implementations that use this for
   discovery.
2. If Sdext CSRs are accessed when they are not implemented, the hart
   will trap. (No need to specify behavior when Sdext is not
   implemented, because in that case Debug Mode doesn't exist and the
   CSRs aren't accessible anyway.) This is to be consistent with Sdtrig
   behavior.

pdonahue-ventana · 2023-08-22T22:06:26Z

It is my understanding that there is no requirement for any particular behavior in the (as yet undocumented) behavior on accesses to unimplemented CSRs. They no longer need to trap but they also don't need to return 0.

Consider implementation X:

no triggers are implemented
nothing ever causes an illegal instruction exception. Look at all the gates we save. And medeleg[2] can be hardwired, saving one whole flop.
tinfo isn't implemented. Why would we if we have no triggers and we're skimping elsewhere?
reads of tinfo return the value from pmpcfg4 (or whatever). That's a nearby encoding and doing this saves a gate or two on this super-cheapskate implementation.

We get to step 4 of the enumeration recipe. Reading tinfo didn't cause an exception and it returned non-zero. The recipe says to parse the read value to discover the trigger type but we're actually parsing something that's not tinfo at all. I see no possible way that this can work. While the recipe will work on sane implementations, I think the general requirement is that the debugger needs to already know what is implemented. Unified discovery or maybe reading JTAG IDCODE and looking up in some database that the debugger as access to.

I think that it might be best to leave the recipe as-is and preface it all with "If an implementation raises an illegal instruction exception on accesses to unimplemented CSRs then the enumeration recipe is below. If it does not raise illegal instruction exceptions then the enumeration method is beyond the scope of this specification." That might get the ARC's attention about their contemplated backward-incompatible change to the priv spec.

timsifive · 2023-08-28T23:02:35Z

This version is awkward, but I dislike it less than simply removing the requirement to raise an exception, because deep down I believe that the privspec won't be changed in a way that makes discovery impossible.

pdonahue-ventana · 2023-08-29T04:35:09Z

Sdext.tex

+    At the time of this writing the Privileged Spec says one thing, but the
+    maintainers are planning to change it in the next revision.


Suggested change

At the time of this writing the Privileged Spec says one thing, but the

maintainers are planning to change it in the next revision.

Privileged spec v1.12 requires accesses to unimplemented CSRs to raise illegal instruction

exceptions, though that requirement may be relaxed in later versions.

pdonahue-ventana · 2023-08-29T04:47:11Z

Sdtrig.tex

@@ -26,16 +27,18 @@ \chapter{Sdtrig (ISA Extension)}

 \section{Enumeration}

-\begin{steps}{Each trigger may support a variety of features. A debugger can
+\begin{steps}{Each trigger may support a variety of features.
+    If Sdtrig is implemented or unimplemented CSRs raise an exception,


Is it possible for Sdtrig to be implemented but with zero triggers? That seems kind of silly but I think that technically somebody could claim that their zero trigger core is Sdtrig complliant. That serves no real purpose but looks slightly more impressive in marketing Powerpoint (which is slimy but technically legit). To avoid this, I think that we should have a sentence somewhere that says that an Sdtrig-compliant implementation has at least one trigger (unless I'm forgetting some Sdtrig feature other than triggers that somebody might want to implement without doing triggers). That prevents any Powerpoint ethical gray areas and also makes this sentence correct.

That was explicitly allowed back when there was a single debug extension, to keep triggers optional. You're right that it doesn't make sense to allow Sdtrig to be implemented with 0 triggers. I support the idea, but I don't think that fits in this PR, and it's also technically backwards incompatible.

I don't think that it's incompatible. Old implementations would have said that they do the debug spec but there was no "Sdtrig" until recently. I guess that somebody in the last year or so could have claimed Sdtrig compliance without any triggers but that seems unlikely and I don't think that adding this new requirement would require them to change anything other than some Powerpoint slides.

This is along the lines of what I was talking about last week when I suggested explicitly saying that you can do external debug without Sdtrig, but you pointed out the diagram with the dotted line around the TM.

If we don't say that Sdtrig compliance => non-zero number of triggers then I worry that this recipe doesn't work for implementations that don't get exceptions on accesses to unimplemented CSRs. (In real life, I hope that everyone gets exceptions AND has at least one trigger if they claim compliance, but in theory there's a problem.)

I made #872 for this.

pdonahue-ventana · 2023-08-29T04:48:13Z

Sdtrig.tex

+    At the time of this writing the Privileged Spec says one thing, but the
+    maintainers are planning to change it in the next revision.


Suggested change

At the time of this writing the Privileged Spec says one thing, but the

maintainers are planning to change it in the next revision.

Privileged spec v1.12 requires accesses to unimplemented CSRs to raise illegal instruction

exceptions, though that requirement may be relaxed in later versions.

pdonahue-ventana · 2023-08-29T04:48:58Z

xml/hwbp_registers.xml

@@ -149,8 +149,8 @@
    <register name="Trigger Info" short="tinfo" address="0x7a4">
        This register is optional if no triggers are implemented, or if
        \FcsrTdataOneType is not writable and \FcsrTinfoVersion would be 0. In
-        this case the debugger can read the only supported type from
-        \RcsrTdataOne.
+        this case reading this register must return 0 or raise an exception, and


Suggested change

this case reading this register must return 0 or raise an exception, and

this case reading this register must return 0 or raise an illegal instruction exception, and

Also, I think that it's ambiguous whether it must (return 0 OR (exception AND debugger can ...)) vs. ((return 0 OR exception) AND debugger can ...). The comma might be intended to help but it's not obvious to me until I think about it some more. Maybe two sentences: It must return 0 or exception. If either of those happen then the debugger can ...

Agreed about 2 sentences.

How strongly do you feel about "illegal instruction exception?" I was deliberately vague because the hypervisor extension mandates that CSRs that appear not to exist to a guest must raise a virtual instruction exception. But maybe we should just ignore that possibility, because I bet there are other places in the spec where we don't think about that scenario.

Sure, we can leave the type of exception off if you want. That requires the reader to figure it out but it's not rocket science. It's not like any sane reader would expect a page fault.

Based on discussion in #870, where Paul mentioned: > I don't think that it's incompatible. Old implementations would have > said that they do the debug spec but there was no "Sdtrig" until > recently. I guess that somebody in the last year or so could have > claimed Sdtrig compliance without any triggers but that seems unlikely > and I don't think that adding this new requirement would require them to > change anything other than some Powerpoint slides.

timsifive · 2023-08-30T20:34:35Z

I'm putting this on hold, because now maybe ARC is going to allow the original phrasing where unimplemented CSRs raise an exception.

After a very long e-mail discussion, we landed on maintaining unimplemented CSR behavior as-is. Just add a quick note to be explicit about that intention. The end of the discussion is basically: 1. If Sdtrig CSRs are accessed when they are not implemented or Sdtrig is not implemented, the hart will trap. This is to maintain backwards compatibility with existing implementations that use this for discovery. 2. If Sdext CSRs are accessed when they are not implemented, the hart will trap. (No need to specify behavior when Sdext is not implemented, because in that case Debug Mode doesn't exist and the CSRs aren't accessible anyway.) This is to be consistent with Sdtrig behavior.

pdonahue-ventana

I am somewhat concerned that a subset of AR people think this is fine (per email discussions) but that others will disagree. But let's move forward with this. We're 100% compatible with the current priv spec and 1.13 is somewhat speculative.

timsifive requested a review from pdonahue-ventana August 22, 2023 19:05

timsifive force-pushed the exception branch from ab4808f to 4970b55 Compare August 22, 2023 19:05

timsifive added Backwards Incompatible Change 1.0 labels Aug 22, 2023

timsifive force-pushed the exception branch from 4970b55 to 7ec4e61 Compare August 28, 2023 23:01

pdonahue-ventana reviewed Aug 29, 2023

View reviewed changes

timsifive force-pushed the exception branch from 7ec4e61 to a2ce10b Compare August 30, 2023 20:22

timsifive mentioned this pull request Aug 30, 2023

Sdtrig must implement at least one trigger. #872

Merged

timsifive force-pushed the exception branch from a2ce10b to 5553718 Compare September 3, 2023 22:11

timsifive force-pushed the exception branch from 5553718 to b972694 Compare September 11, 2023 17:15

timsifive removed the Backwards Incompatible Change label Sep 11, 2023

timsifive changed the title ~~AR: Can't require exception for CSR access.~~ AR: Comment on unimplemented CSR behavior. Sep 11, 2023

timsifive requested a review from pdonahue-ventana September 11, 2023 19:50

pdonahue-ventana approved these changes Sep 11, 2023

View reviewed changes

timsifive merged commit 27d6029 into master Sep 12, 2023
1 check passed

timsifive deleted the exception branch September 12, 2023 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AR: Comment on unimplemented CSR behavior. #870

AR: Comment on unimplemented CSR behavior. #870

timsifive commented Aug 22, 2023 •

edited

Loading

pdonahue-ventana commented Aug 22, 2023

timsifive commented Aug 28, 2023

pdonahue-ventana Aug 29, 2023

timsifive Aug 29, 2023

pdonahue-ventana Aug 29, 2023

timsifive Aug 29, 2023

pdonahue-ventana Aug 30, 2023

timsifive Aug 30, 2023

pdonahue-ventana Aug 29, 2023

pdonahue-ventana Aug 29, 2023

pdonahue-ventana Aug 29, 2023

timsifive Aug 29, 2023

pdonahue-ventana Aug 30, 2023

timsifive commented Aug 30, 2023

pdonahue-ventana left a comment

		At the time of this writing the Privileged Spec says one thing, but the
		maintainers are planning to change it in the next revision.

	this case reading this register must return 0 or raise an exception, and
	this case reading this register must return 0 or raise an illegal instruction exception, and

AR: Comment on unimplemented CSR behavior. #870

AR: Comment on unimplemented CSR behavior. #870

Conversation

timsifive commented Aug 22, 2023 • edited Loading

pdonahue-ventana commented Aug 22, 2023

timsifive commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsifive commented Aug 30, 2023

pdonahue-ventana left a comment

Choose a reason for hiding this comment

timsifive commented Aug 22, 2023 •

edited

Loading