Repository navigation
Isolate config register behaviour from misbehaving or confused software - #44
Merged
Merged
Conversation
Collaborator
|
Contributor
Author
Only writes to commit cause movement, not just writes to spm or flush. Yes, we can snapshot cfg_spm a while later than cfg_flushvif there's traffic that keeps in WaitAux. There will still only be one write to spm_lock, though: FssmWaitAux only captures cfg_spm in the same cycle it transitions to FsmInitFlush? I'm happy to add some extra state to copy that immediately if desired, but I don't think it's necessary here. My thought process was that as long as we only read it once, software can't mess up our internal state tracking. The issue I'm trying to avoid is software corrupting our internal state, not software giving itself race conditions about which value ends up being used. |
This does not change the behaviour. Also, because the HW we does not affect whether a RW register can be written to be SW (and in fact SW takes higher priority over HW if both try), let's just make all the _en bits default-disabled and only enable when needed.
Otherwise, software can manipulate the value of the cfg_spm register and cause the hardware to indicate that a way is flushed via the `conf_regs_o.flushed` register (then used by other parts of the hardware) when the actual way was never flushed and contains dirty data. This is under the assumption that software writes to the configuration registers after CommitCfg.
Previously, software could write to the register and every time we went through FsmInitFlush it would update the to_flush_d list. I believe that it was *fine*, in the sense that the list of thought to be 'flushed' ways would remain coherent with what was actually flushed no matter what software did. However, this is quite hard to reason about, and there's basically no reason why you would ever want to do this: you also don't expect that anything done after writing the CommitCfg register would take effect.
Previously, the loop condition for the FsmInitFlush case was duplicated in the FsmEndFlush case. This meant an extra set of the same code needed to be maintained. The tradeoff is that we take one extra cycle to finish clearing the flush on the last iteration. However, we've spent some large amount of cycles flushing all the cache lines to memory, so I don't think this is a big difference.
midnightveil
force-pushed
the
sw-isolation
branch
from
September 4, 2026 05:47
f581ece to
6459c63
Compare
Contributor
Author
|
(Pushed to fix commit typo) |
Collaborator
|
Makes sense, then I misunderstood the goal. |
phsauter
approved these changes
Sep 4, 2026
ricted98
pushed a commit
that referenced
this pull request
Oct 9, 2026
* config: clearly delineate software-writable vs software-ro This does not change the behaviour. Also, because the HW we does not affect whether a RW register can be written to be SW (and in fact SW takes higher priority over HW if both try), let's just make all the _en bits default-disabled and only enable when needed. * config: use 'saved' value of cfg_spm sw register in 'spm_lock_q' Otherwise, software can manipulate the value of the cfg_spm register and cause the hardware to indicate that a way is flushed via the `conf_regs_o.flushed` register (then used by other parts of the hardware) when the actual way was never flushed and contains dirty data. This is under the assumption that software writes to the configuration registers after CommitCfg. * config: read the software-input CfgFlush register only once Previously, software could write to the register and every time we went through FsmInitFlush it would update the to_flush_d list. I believe that it was *fine*, in the sense that the list of thought to be 'flushed' ways would remain coherent with what was actually flushed no matter what software did. However, this is quite hard to reason about, and there's basically no reason why you would ever want to do this: you also don't expect that anything done after writing the CommitCfg register would take effect. * config: cleanup duplicated loop condition Previously, the loop condition for the FsmInitFlush case was duplicated in the FsmEndFlush case. This meant an extra set of the same code needed to be maintained. The tradeoff is that we take one extra cycle to finish clearing the flush on the last iteration. However, we've spent some large amount of cycles flushing all the cache lines to memory, so I don't think this is a big difference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please review commit-by-commit.
The idea here is to make it easier to reason about the correctness of the config register-based flushing of the LLC, by saving a single copy of the CfgFlush and CfgSpm registers at the beginning of the flush. Without this, one has to reason that any TOCTOU issues are benign.
This was discussed previously with @niwis privately. One thing he raised was that it is generally assumed that trusted software has access to these registers, which is fair, although it does assume that software never makes any mistakes. My original proposal involved extra state being stored, but I've managed here to not require any more storage or flip-flops, just changing the time and ordering of when things are saved. I'd also argue that it's better to avoid software being able to corrupt hardware state, esp. with the issue with
CfgSpmidentified in one of the commits.Appended onto the end of this changeset is a cleanup to remove a duplicate loop condition, which will have a visible timing impact: it will take one extra cycle following the final flush operation as we need an extra state transition. I personally think this is fine.
Hopefully this changes make it clearer how this code is expected to behave as well.
An alternative, possibly nicer solution, would be to have support in the register bus to allow "locking out" of software writes, then we can safely assume they won't change.