Skip to content

Conversation

Zain2050
Copy link

@Zain2050 Zain2050 commented Oct 3, 2025

Related to Issue #513.
This PR is based upon PR #554. Can be merged after 554 is merged.

New Changes:
params: CTR_CYCLE_COUNTER and CCV (Smctr.yaml)
IDL: (Sctrclr.yaml)

Copy link
Collaborator

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, the following needs associated parameters:

An implementation that supports cycle counting must implement CCV and all CCM bits, but may implement 0..4 exponent bits in CCE. Unimplemented CCE bits are read-only 0. For implementations that
support transfer type filtering, it is recommended to implement at least 3 exponent bits. This allows capturing the full latency of most functions, when recording only calls and returns.

The CtrCycleCounter is reset on writes to xctrctl , and on execution of SCTRCLR, to ensure that any
accumulated cycle counts do not persist across a context switch.
schema:
type: integer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this to represent the optionality of the the CC field in the ctrdata CSR? If so, this seems like this should be a boolean?

Also, it's not clear that this is what the parameter is for, so maybe include a bit more about the purpose of the parameter:

The ctrdata register may optionally include a count of CPU cycles elapsed since the prior CTR record.

Copy link
Author

@Zain2050 Zain2050 Oct 4, 2025

Choose a reason for hiding this comment

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

This is a bit vague in the spec. From what I've understood. CtrCycleCounter and ctrdata.CC are different. CtrCycleCounter is an independant counter whose value is stored in ctrdata in case of a control transfer. From
section 11.5.3

The CC field is encoded such that CCE holds 0 if the CtrCycleCounter value is less than 4096, otherwise it holds the index of the most significant one bit in the CtrCycleCounter value, minus 11.

ctrdata for each entry is just a copy of CtrCycleCounter at the time of control transfer. Also,

The SCTRCLR instruction performs the following operations:
Zeroes all CTR Entry Registers, for all DEPTH values
Zeroes the CTR cycle counter and CCV

Here, zeroing out entry registers meaning zeroing out ctrdata as well. But it explicitly states to zero out CTR cycle counter and CCV.
As it's a counter therefore, it's type would be integer, right?
I'll improve the description of the param as it seems a bit unclear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not following, sorry. The purpose of parameters is for configurations to resolve optionality in the spec. (For example, when the spec says "should", "may", or recommends a behavior, but does not mandate it with "must".) What optionality is being resolved with this parameter?

Copy link
Author

Choose a reason for hiding this comment

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

I guess I might be using parameters the wrong way.
Spec states that STRCLR zeros out the CTR cycle counter.

The SCTRCLR instruction performs the following operations:
Zeroes all CTR Entry Registers, for all DEPTH values
Zeroes the CTR cycle counter and CCV

Therefore, I used params to represent the hardware counter and CCV flag. What would be the correct way to represent the counter so that I could zero it out in IDL?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, so "CCV" is a field in the ctrdata register. This register is not yet defined in UDB, but needs to be for this work to proceed. It would probably be defined much like other CSRs, but it might be a little weird in that I think it can only be accessed indirectly (siselect + sireg3). And there isn't a single set of CTR registers, it's a buffer of up to 256 records that can be accessed by being mapped to indirect register selections 0x200-0x2ff. And, how many records are actually supported is implementation-dependent (needs a configuration parameter) -- see sctrdepth.

So, it's a little complicated. #554 is a start on defining the extension, but isn't merged, and doesn't have the parameters defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The response was essentially that CtrCycleCounter is not directly visible machine state, only indirectly through ctrdata register(s). So, I think this needs to be implemented using a callout to a builtin function, akin to the use of read_mcycle() in cycle.yaml.

Copy link
Author

Choose a reason for hiding this comment

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

Okay. What about clearing out the entry registers? We don't have direct access to those registers. Should I use a builtin for that as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point, I don't think it's terribly important whether it is represented as a set of (indirect) actual CSRs, or as a set of indirect CSRs that have sw_write() and sw_read() methods that access some builtin memory buffer. The former is pretty straight-forward. The latter would require defining a builtin accessor function.

Copy link
Author

Choose a reason for hiding this comment

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

So, my current approach makes sense, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you are talking about configuration parameters, then no. :-)

Flipping through the extension documentation, I see at least these as needed parameters:

  • what DEPTH values are supported
  • whether MISP is supported
  • the optionality of each ctrdata field
  • number of supported CCE bits

For registers, you'll need to use indirect access as noted previously.

For CtrCycleCounter, you should implement as a call to a builtin function.

For CCV, each of those bits is contained in each Control Transfer Record, shadowed by the ctrdata register, accessed indirectly through sireg3 (when siselect = buffer offset/register index), so if you're already setting the register to zero, you have also set CCV to zero.

I hope that makes sense. :-)

accumulated cycle counts do not persist across a context switch.
schema:
type: integer
CCV:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, what is this parameter used for? Is it the "should" in the following description, as to whether that "should" is respected or not?

Copy link
Author

Choose a reason for hiding this comment

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

This represents the value that will be store in the CCV field of ctrdata when the next control transfer occurs. I have updated its description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not following here, either. Are you trying to parameterize the "should" in

This flag should additionally be cleared after any other implementation-specific scenarios where active cycles might not be counted in CTR_CYCLE_COUNTER.

?

And, it's defined as boolean, so what would be the value to be stored? Regardless, I'm not sure this really needs to be parameterized, but I want to understand your thinking first.

@Zain2050
Copy link
Author

Zain2050 commented Oct 10, 2025 via email

Comment on lines +61 to +66
for (U32 i = 0; i < (16 << CSR[sctrdepth].DEPTH); i++) {
CSR[siselect] = (0x200 + i);
CSR[sireg1] = 0;
CSR[sireg2] = 0;
CSR[sireg3] = 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, there is no sireg1... it's sireg. :-/

While this should accomplish the goal of clearing the CTR buffer, it has side-effect of changing the value of siselect. This should probably be saved and restored.

Now I'm wondering, though, if we also need to be concerned with changing the value of siselect at all here, because it would be an observable change during a trap, for example. The only way I can think to avoid that issue would be to bypass the indirect register usage and only manipulate the internal state directly.

So, let's define all of the CTR registers as CSRs. This is in line with how they are described in the spec under "CSR LIsting":

Image Image

You can use a "layout" to generate the 256 definitions.

Then, you can manipulate them directly without needing to change the value of siselect.

This is a nice case in support of #1169, but we don't have a means to treat these individual registers as an array. You could also use a layout to initialize/reset them all.

@ThinkOpenly ThinkOpenly changed the title fix(IDL): implement Smctr/Smctr in IDL fix(IDL): implement Smctr/Ssctr in IDL Oct 15, 2025
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.

3 participants