Skip to content

Add scontext2 alias for scontext#535

Merged
timsifive merged 7 commits into
masterfrom
scontext2-csr
Sep 3, 2020
Merged

Add scontext2 alias for scontext#535
timsifive merged 7 commits into
masterfrom
scontext2-csr

Conversation

@ernie-sifive
Copy link
Copy Markdown
Collaborator

In the interest of making scontext usable, this PR suggests an alternate CSR address (scontext2 at 0x5a8) through which to access it that is consistent with its desired accessibility, namely Supervisor RW. Retaining the existing scontext register at 0x7aa maintains backward compatibility.

I am suggesting a CSR address consistent with mcontext at 0x7a8 but in the Supervisor CSR range per convention outlined in the RISCV Privileged Architecture Spec. I don't know how CSR numbers are allocated, so assuming reaction to this idea is favorable, there might be an additional step to request this allocation.

Copy link
Copy Markdown
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.

This makes sense to me.
Like you, I don't know how to apply for a supervisor CSR number. Can you e-mail the privileged group, or somebody else who is likely to know?

@ernie-sifive ernie-sifive requested a review from aswaterman July 27, 2020 20:44
@ernie-sifive
Copy link
Copy Markdown
Collaborator Author

@aswaterman There has been customer and internal interest in adding an option to the core trigger hardware to qualify breakpoints based on a context ID for systems that reuse virtual addresses in multiple contexts.  What do you think of an alias for scontext in a range that is accessible in S-mode?

@aswaterman
Copy link
Copy Markdown
Member

Wouldn't it make sense to use the ASID for this rather than inventing a new concept?

@timsifive
Copy link
Copy Markdown
Contributor

The original proposal actually used ASID, and then was changed to this format. The discussion is at #363 (comment)

Using satp seems less flexible because it's limited to just address translation. But it is faster when context switching.

I'd lean towards not switch to asid, because scontext is already in the ratified 0.13 debug spec.

@aswaterman
Copy link
Copy Markdown
Member

aswaterman commented Jul 29, 2020

OS probably won't use the feature as specified, since it doesn't affect OS functionality and adds context-switch latency, so it will likely end up being vestigial. OTOH, OS will be forced to use ASID for performance reasons.

The "scontext is already ratified" argument isn't especially compelling, since satp.ASID has already been ratified, too.

@ernie-sifive
Copy link
Copy Markdown
Collaborator Author

ASID width can be small, even 0. When the OS recycles an ASID, it will make ASID useless for trigger qualification.

From a hardware perspective, an scontext register seems like it would add less hardware than an ASID since ASID affects address translation and isn't just a simple read/write register field.

@aswaterman
Copy link
Copy Markdown
Member

HW-cost argument doesn't hold up (can implement ASID bits but have them not do anything), but ASID-recycling argument is valid.

Is there precedent for this kind of feature in Linux ports to other ISAs? Have they taken the same approach?

@bruceable
Copy link
Copy Markdown

bruceable commented Jul 29, 2020 via email

@aswaterman
Copy link
Copy Markdown
Member

No need for citations; just trying to make sure we’re on well-trodden ground here.

Are those registers updated as part of the context-switch code? I assume they’re disabled by default and enabled by a Kconfig option, given that not all implementations support the feature?

@pdonahue-ventana
Copy link
Copy Markdown
Collaborator

ARMv8 has CONTEXTIDR_EL2 and CONTEXTIDR_EL1 that are separate from VMID and ASID. From D2.1.2 of ARMv8 F.b:

A CONTEXTIDR_ELx identifies the current Context ID, that is used by:
​> ​• The debug logic, for breakpoint and watchpoint matching.
​> ​• Implemented trace logic, to identify the current process.

​My recollection from ~4 years ago is that the Linux kernel is built by default without support for context switching CONTEXTIDR_EL1. If you want to use it for debug/trace, you have to build a kernel with that enabled. (I couldn't quickly find a reference for this so you may not want to take my word for it.)

@aswaterman
Copy link
Copy Markdown
Member

Thanks, Paul; you answered my question as I was typing it.

@bruceable
Copy link
Copy Markdown

bruceable commented Jul 29, 2020 via email

@ernie-sifive
Copy link
Copy Markdown
Collaborator Author

So back to the original question, @aswaterman: is the 0x5a8 CSR address for scontext acceptable and if so, can the Debug TG just use it or is there a process to follow with the Privileged Spec TG to get this address allocated?

@aswaterman
Copy link
Copy Markdown
Member

The proposal and the rationale need to go to the tech-privileged list to get feedback from others, since this is a full-blown privarch feature.

@jscheid-ventana
Copy link
Copy Markdown

​My recollection from ~4 years ago is that the Linux kernel is built by default without support for context switching CONTEXTIDR_EL1. If you want to use it for debug/trace, you have to build a kernel with that enabled. (I couldn't quickly find a reference for this so you may not want to take my word for it.)

Yes, because it is not useful for self-hosted debug, where the PID/ASID association is easy to maintain. It's optional because it's only useful for external debug/trace use of triggers, where the ASID could change underneath you or be reused/swapped during a session.

@ernie-sifive
Copy link
Copy Markdown
Collaborator Author

@timsifive The scontext change to the Privileged Spec in riscv/riscv-isa-manual#559 has been merged, so this PR is ready for final review and merging.

Comment thread xml/hwbp_registers.xml Outdated
Comment thread xml/hwbp_registers.xml Outdated
Copy link
Copy Markdown
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.

I'll merge this tomorrow, assuming there are no other comments.

@timsifive timsifive merged commit 4f625ca into master Sep 3, 2020
@timsifive timsifive deleted the scontext2-csr branch September 3, 2020 19:15
Comment thread xml/hwbp_registers.xml
Comment on lines +180 to +186
<register name="Machine Context" short="mcontext" address="0x7a8">
This register is an alias for \RcsrHcontext.
</register>

<register name="Machine Supervisor Context" short="mscontext" address="0x7aa">
This register is an alias for \RcsrScontext included for backward compatibility.
</register>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does mscontext say it's included for backward compatibility, but mcontext doesn't say the same thing?

@pdonahue-ventana, thoughts?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

0x7a8 is the mcontext CSR index. 0x7aa is in an M-mode only access range so wasn't suitable for scontext, which should be writable in S-mode. So we moved scontext to 0x5a8, which is writable in S-mode, but left this 0x7aa alias in the doc in case there was an implementation that used it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep. Debug 0.13 didn't have the benefit of an architecture review committee which would presumably notice the improper CSR number.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That all makes sense, just not sure why we have mcontext at all. But maybe it's not related to this change. I'll e-mail for more discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants