Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arch/arm64: mem barrier interface Align with riscv #15072

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lipengfei28
Copy link
Contributor

arch/arm64: mem barrier interface Align with riscv

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Board: arm64 Size: M The size of the change in this PR is medium labels Dec 6, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 6, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements yet. While it provides a summary and mentions impact and testing, it lacks crucial details. Here's a breakdown:

Missing/Insufficient Information:

  • Summary: While the title gives a general idea, the summary needs more detail. It should explain why aligning the memory barrier interface with RISC-V is necessary. What problem does it solve? What are the benefits? Is this a bug fix, a performance improvement, or something else? It also lacks any mention of related issues.
  • Impact: The placeholder text remains. The author must fill out this section with specifics. Even if the answer is "NO" for each item, it should explicitly state that. For example, "Impact on user: NO". If there is an impact, it needs a description. For instance, if the change affects driver code, explain how.
  • Testing: The placeholder text remains. This section is critical. The author needs to provide concrete details about their testing environment (host OS, compiler version, target architecture, board, configuration) and include actual logs from before and after the change to demonstrate the impact and verify the fix/improvement. Simply saying "works as intended" is not sufficient.

To make this PR compliant, the author needs to:

  1. Expand the Summary: Provide a detailed explanation of the change's purpose and rationale. Include links to any related issues.
  2. Complete the Impact Section: Address each point specifically, even if the answer is "NO". Provide detailed explanations for any "YES" answers.
  3. Provide Thorough Testing Information: Specify the host and target environments used for testing and include actual logs demonstrating the behavior before and after the change.

By providing this missing information, the PR will meet the NuttX requirements and enable reviewers to effectively evaluate the proposed changes.

Copy link
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

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

I don't agree with this change. ISB, DMB and DSB are well documented ARM/ARM64 instructions, so the naming is coherent already.

If you insist, you can remove the ARM64_ prefix, but even that is unnecessary IMO, as it would require updating all downstream code as well for no benefit.

Can you explain what is the motivation for this change? Why must we use similar names to RISC-V in arch specific code?

@xiaoxiang781216
Copy link
Contributor

I don't agree with this change. ISB, DMB and DSB are well documented ARM/ARM64 instructions, so the naming is coherent already.

If you insist, you can remove the ARM64_ prefix, but even that is unnecessary IMO, as it would require updating all downstream code as well for no benefit.

Can you explain what is the motivation for this change? Why must we use similar names to RISC-V in arch specific code?

If you want to share the same driver around the different arch, it's important to keep this basic operation same around all arch.

@pussuw
Copy link
Contributor

pussuw commented Dec 6, 2024

I don't agree with this change. ISB, DMB and DSB are well documented ARM/ARM64 instructions, so the naming is coherent already.
If you insist, you can remove the ARM64_ prefix, but even that is unnecessary IMO, as it would require updating all downstream code as well for no benefit.
Can you explain what is the motivation for this change? Why must we use similar names to RISC-V in arch specific code?

If you want to share the same driver around the different arch, it's important to keep this basic operation same around all arch.

If a common set of barrier macros are needed in the common kernel drivers, then you can define a common set of macros that the arch specific macros are forwarded to, instead of renaming the existing ones.

@xiaoxiang781216
Copy link
Contributor

I don't agree with this change. ISB, DMB and DSB are well documented ARM/ARM64 instructions, so the naming is coherent already.
If you insist, you can remove the ARM64_ prefix, but even that is unnecessary IMO, as it would require updating all downstream code as well for no benefit.
Can you explain what is the motivation for this change? Why must we use similar names to RISC-V in arch specific code?

If you want to share the same driver around the different arch, it's important to keep this basic operation same around all arch.

If a common set of barrier macros are needed in the common kernel drivers, then you can define a common set of macros that the arch specific macros are forwarded to, instead of renaming the existing ones.

Why do you insist keep two set of macros (__DSB and ARM64_DSB/RISCV_DSB) which has the same functionality? developers may randomly select one to use, which just make the confusion and inconsistence.

@@ -179,8 +179,8 @@ static int create_spgtables(arch_addrenv_t *addrenv)

/* Synchronize data and instruction pipelines */

ARM64_DSB();
ARM64_ISB();
__MB();
Copy link
Contributor

Choose a reason for hiding this comment

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

@lipengfei28 let's change to UP_MB or up_mb to indicate the barrier macro is the standard API between kernel/driver and arch. and replace ALL SP_XXX with the new macros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,I will change to UP_MB as standard API,

@pussuw
Copy link
Contributor

pussuw commented Dec 7, 2024

I don't agree with this change. ISB, DMB and DSB are well documented ARM/ARM64 instructions, so the naming is coherent already.
If you insist, you can remove the ARM64_ prefix, but even that is unnecessary IMO, as it would require updating all downstream code as well for no benefit.
Can you explain what is the motivation for this change? Why must we use similar names to RISC-V in arch specific code?

If you want to share the same driver around the different arch, it's important to keep this basic operation same around all arch.

If a common set of barrier macros are needed in the common kernel drivers, then you can define a common set of macros that the arch specific macros are forwarded to, instead of renaming the existing ones.

Why do you insist keep two set of macros (__DSB and ARM64_DSB/RISCV_DSB) which has the same functionality? developers may randomly select one to use, which just make the confusion and inconsistence.

Because ISB, DMB and DSB are arch specific instructions. The barrier macro names (DMB and ISB) are poorly chosen for risc-v as it does not have these instructions (fence with params is used instead).

I think in arch specific code it is fine to use ISB DSB and DMB because like I said they are well documented ARM/ARM64 instructions. If generic barrrier macros are needed in the common kernel code, UP_MB and such is better like you said.

If you insist we remove the arch specific macros then go ahead but I don't see the confusion, as they are macros for well known ARM instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Board: arm64 Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants