Allows the platform to customize TLB operations #23
Conversation
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
Reviewer's GuideAdds a platform hook layer for local TLB-related fence operations and wires it up for the T-Head generic platform (including a new JTLB erratum quirk), so that platforms can override the default TLB flush behavior used by the core sbi_tlb logic. Sequence diagram for platform-overridable local TLB flush (sfence_vma path)sequenceDiagram
participant Hart as Hart
participant sbi_tlb as sbi_tlb_local_sfence_vma
participant PlatWrap as sbi_platform_local_sfence_vma
participant Plat as sbi_platform
participant Ops as sbi_platform_operations
participant THead as thead_jtlb_local_sfence_vma
Hart->>sbi_tlb: sbi_tlb_local_sfence_vma(tinfo)
sbi_tlb->>sbi_tlb: sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SFENCE_VMA_RCVD)
sbi_tlb->>PlatWrap: sbi_platform_local_sfence_vma(plat, tinfo)
PlatWrap->>Plat: sbi_platform_ops(plat)
Plat->>Ops: get local_sfence_vma
alt local_sfence_vma implemented
Ops->>THead: local_sfence_vma(tinfo)
THead-->>PlatWrap: return
PlatWrap-->>sbi_tlb: return SBI_OK
sbi_tlb-->>Hart: return (platform handled TLB flush)
else local_sfence_vma not implemented
PlatWrap-->>sbi_tlb: return SBI_ENOTSUPP
sbi_tlb->>sbi_tlb: execute default sfence.vma sequence
sbi_tlb-->>Hart: return (default handled TLB flush)
end
Class diagram for updated sbi_platform_operations and TLB hook integrationclassDiagram
class sbi_tlb_info {
+unsigned long start
+unsigned long size
+unsigned long asid
+unsigned long vmid
}
class sbi_platform_operations {
+get_tlb_num_entries() u32
+local_fence_i(tinfo sbi_tlb_info) void
+local_sfence_vma(tinfo sbi_tlb_info) void
+local_sfence_vma_asid(tinfo sbi_tlb_info) void
+local_hfence_gvma_vmid(tinfo sbi_tlb_info) void
+local_hfence_gvma(tinfo sbi_tlb_info) void
+local_hfence_vvma_asid(tinfo sbi_tlb_info) void
+local_hfence_vvma(tinfo sbi_tlb_info) void
}
class sbi_platform {
+sbi_platform_operations *platform_ops
}
class sbi_tlb {
+sbi_tlb_local_fence_i(tinfo sbi_tlb_info) void
+sbi_tlb_local_sfence_vma(tinfo sbi_tlb_info) void
+sbi_tlb_local_sfence_vma_asid(tinfo sbi_tlb_info) void
+sbi_tlb_local_hfence_gvma_vmid(tinfo sbi_tlb_info) void
+sbi_tlb_local_hfence_gvma(tinfo sbi_tlb_info) void
+sbi_tlb_local_hfence_vvma_asid(tinfo sbi_tlb_info) void
+sbi_tlb_local_hfence_vvma(tinfo sbi_tlb_info) void
}
class sbi_platform_wrappers {
+sbi_platform_local_fence_i(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_sfence_vma(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_sfence_vma_asid(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_hfence_gvma_vmid(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_hfence_gvma(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_hfence_vvma_asid(plat sbi_platform, tinfo sbi_tlb_info) u32
+sbi_platform_local_hfence_vvma(plat sbi_platform, tinfo sbi_tlb_info) u32
}
class thead_generic_quirks {
+unsigned long errata
}
class thead_generic_quirks_instances {
+thead_pmu_quirks
+thead_pmu_jtlb_quirks
}
class thead_jtlb_hooks {
+thead_jtlb_local_sfence_vma(tinfo sbi_tlb_info) void
+thead_jtlb_local_hfence_vvma_asid(tinfo sbi_tlb_info) void
}
sbi_platform o-- sbi_platform_operations : has
sbi_tlb --> sbi_platform_wrappers : calls
sbi_platform_wrappers --> sbi_platform_operations : dispatches
sbi_platform_operations --> sbi_tlb_info : uses
thead_generic_quirks_instances ..> thead_generic_quirks : instance_of
thead_generic_quirks_instances ..> sbi_platform_operations : configures
thead_generic_quirks_instances ..> thead_jtlb_hooks : selects
sbi_platform_operations ..> thead_jtlb_hooks : function_pointers
Flow diagram for T-Head generic platform init with JTLB erratum TLB hooksflowchart TD
A[thead_generic_platform_init] --> B[Read compatible from FDT]
B --> C[Match in thead_generic_match]
C --> D[Get thead_generic_quirks *quirks]
D --> E{quirks->errata & THEAD_QUIRK_ERRATA_THEAD_PMU}
E -- yes --> F[generic_platform_ops.extensions_init = thead_pmu_extensions_init]
E -- no --> G[Skip PMU extensions init]
D --> H{quirks->errata & THEAD_QUIRK_ERRATA_JTLB}
H -- yes --> I[generic_platform_ops.local_sfence_vma = thead_jtlb_local_sfence_vma]
I --> J[generic_platform_ops.local_sfence_vma_asid = thead_jtlb_local_hfence_vvma_asid]
H -- no --> K[Keep default TLB flush behavior]
F --> L[Return 0]
G --> L
J --> L
K --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
sbi_tlb_local_hfence_vvma()the platform hook check callssbi_platform_local_hfence_gvma(...)instead of the correspondingsbi_platform_local_hfence_vvma(...), which looks like a copy/paste error and will prevent platform-specific vvma handling from being used. - In
thead_jtlb_local_hfence_vvma_asid(), both the full-context and partial flush paths execute the samesfence.vma x0, asidsequence, so the conditional onstart/sizeis redundant; either simplify this or implement the intended range-based behavior. - In
thead_generic_platform_init(), the new conditionif (quirks->errata &THEAD_QUIRK_ERRATA_JTLB)is missing a space after&, which is inconsistent with surrounding style and existing code patterns for errata checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `sbi_tlb_local_hfence_vvma()` the platform hook check calls `sbi_platform_local_hfence_gvma(...)` instead of the corresponding `sbi_platform_local_hfence_vvma(...)`, which looks like a copy/paste error and will prevent platform-specific vvma handling from being used.
- In `thead_jtlb_local_hfence_vvma_asid()`, both the full-context and partial flush paths execute the same `sfence.vma x0, asid` sequence, so the conditional on `start/size` is redundant; either simplify this or implement the intended range-based behavior.
- In `thead_generic_platform_init()`, the new condition `if (quirks->errata &THEAD_QUIRK_ERRATA_JTLB)` is missing a space after `&`, which is inconsistent with surrounding style and existing code patterns for errata checks.
## Individual Comments
### Comment 1
<location> `lib/sbi/sbi_tlb.c:46-47` </location>
<code_context>
sbi_pmu_ctr_incr_fw(SBI_PMU_FW_HFENCE_VVMA_RCVD);
+ if (sbi_platform_local_hfence_gvma(
+ sbi_platform_thishart_ptr(), tinfo) == SBI_OK)
+ return;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** sbi_tlb_local_hfence_vvma is calling the GVMA hook instead of the VVMA hook, which looks like a copy-paste bug.
The new fast-path in `sbi_tlb_local_hfence_vvma` calls `sbi_platform_local_hfence_gvma(...)`, but there is now a dedicated `local_hfence_vvma` hook (with a wrapper in `sbi_platform.h`). As a result, any vvma-specific platform override is bypassed and the wrong hook type is used. This should instead call `sbi_platform_local_hfence_vvma(...)`.
</issue_to_address>
### Comment 2
<location> `platform/generic/thead/thead-generic.c:48-57` </location>
<code_context>
+ __asm__ __volatile__("sfence.vma");
+}
+
+static void thead_jtlb_local_hfence_vvma_asid(struct sbi_tlb_info *tinfo)
+{
+ unsigned long start = tinfo->start;
+ unsigned long size = tinfo->size;
+ unsigned long asid = tinfo->asid;
+
+ /* Flush entire MM context for a given ASID */
+ if ((start == 0 && size == 0) || (size == SBI_TLB_FLUSH_ALL)) {
+ __asm__ __volatile__("sfence.vma x0, %0"
+ :
+ : "r"(asid)
+ : "memory");
+ return;
+ }
+
+ __asm__ __volatile__("sfence.vma x0, %0"
+ :
+ : "r"(asid)
</code_context>
<issue_to_address>
**suggestion:** The hfence_vvma_asid helper always issues a full sfence.vma by ASID, making the range check redundant and the name misleading.
In `thead_jtlb_local_hfence_vvma_asid`, both the `if` branch and the fall-through execute `sfence.vma x0, asid`, so the `(start, size)` range logic is dead code. If the JTLB errata require a full ASID flush, consider removing the unused range handling and renaming the helper to indicate that behavior. If a partial flush is actually required, this helper should honor `start` and `size` instead of always flushing the full ASID.
Suggested implementation:
```c
static void thead_jtlb_local_hfence_vvma_asid_full(struct sbi_tlb_info *tinfo)
{
unsigned long asid = tinfo->asid;
/* Flush entire MM context for a given ASID */
__asm__ __volatile__("sfence.vma x0, %0"
:
: "r"(asid)
: "memory");
}
```
1. Update all call sites and function pointers that currently reference `thead_jtlb_local_hfence_vvma_asid` to use the new name `thead_jtlb_local_hfence_vvma_asid_full`, so the build does not break.
2. If this function is exposed via any ops table (e.g. `generic_platform_ops` or similar), adjust the corresponding entry to use the new symbol name.
3. If there is any documentation or comments referring to the old helper name, update them to clarify that this helper always performs a full ASID-based `sfence.vma` and does not honor address ranges.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+48
to
+57
| static void thead_jtlb_local_hfence_vvma_asid(struct sbi_tlb_info *tinfo) | ||
| { | ||
| unsigned long start = tinfo->start; | ||
| unsigned long size = tinfo->size; | ||
| unsigned long asid = tinfo->asid; | ||
|
|
||
| /* Flush entire MM context for a given ASID */ | ||
| if ((start == 0 && size == 0) || (size == SBI_TLB_FLUSH_ALL)) { | ||
| __asm__ __volatile__("sfence.vma x0, %0" | ||
| : |
There was a problem hiding this comment.
suggestion: The hfence_vvma_asid helper always issues a full sfence.vma by ASID, making the range check redundant and the name misleading.
In thead_jtlb_local_hfence_vvma_asid, both the if branch and the fall-through execute sfence.vma x0, asid, so the (start, size) range logic is dead code. If the JTLB errata require a full ASID flush, consider removing the unused range handling and renaming the helper to indicate that behavior. If a partial flush is actually required, this helper should honor start and size instead of always flushing the full ASID.
Suggested implementation:
static void thead_jtlb_local_hfence_vvma_asid_full(struct sbi_tlb_info *tinfo)
{
unsigned long asid = tinfo->asid;
/* Flush entire MM context for a given ASID */
__asm__ __volatile__("sfence.vma x0, %0"
:
: "r"(asid)
: "memory");
}- Update all call sites and function pointers that currently reference
thead_jtlb_local_hfence_vvma_asidto use the new namethead_jtlb_local_hfence_vvma_asid_full, so the build does not break. - If this function is exposed via any ops table (e.g.
generic_platform_opsor similar), adjust the corresponding entry to use the new symbol name. - If there is any documentation or comments referring to the old helper name, update them to clarify that this helper always performs a full ASID-based
sfence.vmaand does not honor address ranges.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by Sourcery
Introduce platform-specific hooks for local TLB and cache fence operations and wire them into the TLB handling path, with Thead generic platforms optionally overriding default behavior via errata quirks.
New Features:
Enhancements: