Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
On Tue, 07 Feb 2023 17:18:27 +, Paul Liu wrote: > > Hi Marc, > > I think you are the author. I'm just making some minor fixes and > then upstreaming to the mailing list. What is the correct way to > make the Signed-off-by list? In general, and unless you have completely rewritten the patch (it doesn't seem so in this particular case), you should keep the original authorship and sign-off chain, and add your own Signed-off-by: tag *after* the previous ones. You should also document what changes you have made, if any, when picking up the patch. When posting it, it should read something like: From: Random Developer Fix foo handling in bar(). Signed-off-by: Random Developer Signed-off-by: Random Committer [Paul: picked from the FooBar tree, fixed the return value for bar() and rebased to upstream] Signed-off-by: Paul Liu Link: https://git.foobar.com/commit/?df2d85d0b0b5b1533d6db9079f0a0a7b73ef6a34 where "Random Developer" is the original author, and "Random Committer" is someone who picked up the patch the first place. The important thing here is that, just by looking at the sign-off chain, you can tell how the patch has been handled. The additional information (enclosed in square bracket) is optional but much appreciated by the reviewers, and so is the link to the original patch, which helps seeing it in context. If the commits have lost the original authorship (which sometimes happen as you rebase patches and resolve conflicts), you can fix it with: git commit --amend --author="Random Developer " on each individual commit. Tom's email also has a good pointer to what is expected from contributors (most of which is applicable to a large number of open-source projects). Hope this helps, M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
Hi Marc, I think you are the author. I'm just making some minor fixes and then upstreaming to the mailing list. What is the correct way to make the Signed-off-by list? Yours, Paul On Wed, 8 Feb 2023 at 00:35, Marc Zyngier wrote: > On 2023-02-07 16:20, Ying-Chun Liu (PaulLiu) wrote: > > Exposing set/way cache maintenance to a virtual machine is unsafe, not > > least because the instructions are not permission-checked but also > > because they are not broadcast between CPUs. Consequently, KVM traps > > and > > emulates such maintenance in the host kernel using by-VA operations and > > looping over the stage-2 page-tables. However, when running under > > protected KVM, these instructions are not able to be emulated and will > > instead result in an exception being delivered to the guest. > > > > Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select > > this option and perform by-VA cache maintenance instead of using the > > set/way instructions. > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) > > Signed-off-by: Marc Zyngier > > Signed-off-by: Will Deacon > > Cc: Tom Rini > > The sign-off chain looks pretty odd. Either you are the author > of this patch, and I have nothing to do on the sign-off list, > or I'm the author and the authorship is wrong. Similar things > would apply for Will. > > So which one is it? > > M. > -- > Jazz is not dead. It just smells funny... >
Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
On Tue, Feb 07, 2023 at 05:06:39PM +, Marc Zyngier wrote: > On Tue, 07 Feb 2023 16:40:05 +, > Tom Rini wrote: > > > > On Tue, Feb 07, 2023 at 04:35:25PM +, Marc Zyngier wrote: > > > On 2023-02-07 16:20, Ying-Chun Liu (PaulLiu) wrote: > > > > Exposing set/way cache maintenance to a virtual machine is unsafe, not > > > > least because the instructions are not permission-checked but also > > > > because they are not broadcast between CPUs. Consequently, KVM traps and > > > > emulates such maintenance in the host kernel using by-VA operations and > > > > looping over the stage-2 page-tables. However, when running under > > > > protected KVM, these instructions are not able to be emulated and will > > > > instead result in an exception being delivered to the guest. > > > > > > > > Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select > > > > this option and perform by-VA cache maintenance instead of using the > > > > set/way instructions. > > > > > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) > > > > Signed-off-by: Marc Zyngier > > > > Signed-off-by: Will Deacon > > > > Cc: Tom Rini > > > > > > The sign-off chain looks pretty odd. Either you are the author > > > of this patch, and I have nothing to do on the sign-off list, > > > or I'm the author and the authorship is wrong. Similar things > > > would apply for Will. > > > > > > So which one is it? > > > > As my first guess here is copy and adopting code from Linux, this is > > not following the documented procedure here: > > https://u-boot.readthedocs.io/en/latest/develop/sending_patches.html#attributing-code-copyrights-signing > > > > Which if not sufficiently clear, please ask / suggest changes to. I see > > right now it isn't specific about cc'ing the original authors (who may, > > or may not, be interested, so blanket policy doesn't apply) but I would > > hope is clear enough that what's done in this example isn't right. > > No, this really is u-boot code written as part of Android, from where > the patch has been directly lifted[1]. > > Same goes for Pierre-Clement's patch that is part of the same series. > > I'm not overly attached to this code (I have bad memories from it), > but I think the OP may be unaware of these rules. In any case, I'm > supportive of this code making it in upstream u-boot. I just want it > to be done correctly. > > Thanks, > > M. > > [1] > https://android.googlesource.com/platform/external/u-boot/+/db5507f47f4f57f766d52f753ff2cc761afc213b Oh, hummm. I guess whatever the normal policy for upstreaming patches from an Android kernel tree, to mainline, would be the starting point here? Referencing the Android tree would be good too. -- Tom signature.asc Description: PGP signature
Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
On Tue, 07 Feb 2023 16:40:05 +, Tom Rini wrote: > > On Tue, Feb 07, 2023 at 04:35:25PM +, Marc Zyngier wrote: > > On 2023-02-07 16:20, Ying-Chun Liu (PaulLiu) wrote: > > > Exposing set/way cache maintenance to a virtual machine is unsafe, not > > > least because the instructions are not permission-checked but also > > > because they are not broadcast between CPUs. Consequently, KVM traps and > > > emulates such maintenance in the host kernel using by-VA operations and > > > looping over the stage-2 page-tables. However, when running under > > > protected KVM, these instructions are not able to be emulated and will > > > instead result in an exception being delivered to the guest. > > > > > > Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select > > > this option and perform by-VA cache maintenance instead of using the > > > set/way instructions. > > > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) > > > Signed-off-by: Marc Zyngier > > > Signed-off-by: Will Deacon > > > Cc: Tom Rini > > > > The sign-off chain looks pretty odd. Either you are the author > > of this patch, and I have nothing to do on the sign-off list, > > or I'm the author and the authorship is wrong. Similar things > > would apply for Will. > > > > So which one is it? > > As my first guess here is copy and adopting code from Linux, this is > not following the documented procedure here: > https://u-boot.readthedocs.io/en/latest/develop/sending_patches.html#attributing-code-copyrights-signing > > Which if not sufficiently clear, please ask / suggest changes to. I see > right now it isn't specific about cc'ing the original authors (who may, > or may not, be interested, so blanket policy doesn't apply) but I would > hope is clear enough that what's done in this example isn't right. No, this really is u-boot code written as part of Android, from where the patch has been directly lifted[1]. Same goes for Pierre-Clement's patch that is part of the same series. I'm not overly attached to this code (I have bad memories from it), but I think the OP may be unaware of these rules. In any case, I'm supportive of this code making it in upstream u-boot. I just want it to be done correctly. Thanks, M. [1] https://android.googlesource.com/platform/external/u-boot/+/db5507f47f4f57f766d52f753ff2cc761afc213b -- Without deviation from the norm, progress is not possible.
Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
On Tue, Feb 07, 2023 at 04:35:25PM +, Marc Zyngier wrote: > On 2023-02-07 16:20, Ying-Chun Liu (PaulLiu) wrote: > > Exposing set/way cache maintenance to a virtual machine is unsafe, not > > least because the instructions are not permission-checked but also > > because they are not broadcast between CPUs. Consequently, KVM traps and > > emulates such maintenance in the host kernel using by-VA operations and > > looping over the stage-2 page-tables. However, when running under > > protected KVM, these instructions are not able to be emulated and will > > instead result in an exception being delivered to the guest. > > > > Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select > > this option and perform by-VA cache maintenance instead of using the > > set/way instructions. > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) > > Signed-off-by: Marc Zyngier > > Signed-off-by: Will Deacon > > Cc: Tom Rini > > The sign-off chain looks pretty odd. Either you are the author > of this patch, and I have nothing to do on the sign-off list, > or I'm the author and the authorship is wrong. Similar things > would apply for Will. > > So which one is it? As my first guess here is copy and adopting code from Linux, this is not following the documented procedure here: https://u-boot.readthedocs.io/en/latest/develop/sending_patches.html#attributing-code-copyrights-signing Which if not sufficiently clear, please ask / suggest changes to. I see right now it isn't specific about cc'ing the original authors (who may, or may not, be interested, so blanket policy doesn't apply) but I would hope is clear enough that what's done in this example isn't right. -- Tom signature.asc Description: PGP signature
Re: [PATCH 1/2] arm: cpu: Add optional CMOs by VA
On 2023-02-07 16:20, Ying-Chun Liu (PaulLiu) wrote: Exposing set/way cache maintenance to a virtual machine is unsafe, not least because the instructions are not permission-checked but also because they are not broadcast between CPUs. Consequently, KVM traps and emulates such maintenance in the host kernel using by-VA operations and looping over the stage-2 page-tables. However, when running under protected KVM, these instructions are not able to be emulated and will instead result in an exception being delivered to the guest. Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select this option and perform by-VA cache maintenance instead of using the set/way instructions. Signed-off-by: Ying-Chun Liu (PaulLiu) Signed-off-by: Marc Zyngier Signed-off-by: Will Deacon Cc: Tom Rini The sign-off chain looks pretty odd. Either you are the author of this patch, and I have nothing to do on the sign-off list, or I'm the author and the authorship is wrong. Similar things would apply for Will. So which one is it? M. -- Jazz is not dead. It just smells funny...
[PATCH 1/2] arm: cpu: Add optional CMOs by VA
Exposing set/way cache maintenance to a virtual machine is unsafe, not
least because the instructions are not permission-checked but also
because they are not broadcast between CPUs. Consequently, KVM traps and
emulates such maintenance in the host kernel using by-VA operations and
looping over the stage-2 page-tables. However, when running under
protected KVM, these instructions are not able to be emulated and will
instead result in an exception being delivered to the guest.
Introduce CONFIG_CMO_BY_VA_ONLY so that virtual platforms can select
this option and perform by-VA cache maintenance instead of using the
set/way instructions.
Signed-off-by: Ying-Chun Liu (PaulLiu)
Signed-off-by: Marc Zyngier
Signed-off-by: Will Deacon
Cc: Tom Rini
---
arch/arm/cpu/armv8/Kconfig| 4 ++
arch/arm/cpu/armv8/cache.S| 50 +-
arch/arm/cpu/armv8/cache_v8.c | 97 ++-
arch/arm/cpu/armv8/cpu.c | 30 +++
4 files changed, 155 insertions(+), 26 deletions(-)
diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
index 1305238c9d..7d5cf1594d 100644
--- a/arch/arm/cpu/armv8/Kconfig
+++ b/arch/arm/cpu/armv8/Kconfig
@@ -1,5 +1,9 @@
if ARM64
+config CMO_BY_VA_ONLY
+ bool "Force cache maintenance to be exclusively by VA"
+ depends on !SYS_DISABLE_DCACHE_OPS
+
config ARMV8_SPL_EXCEPTION_VECTORS
bool "Install crash dump exception vectors"
depends on SPL
diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
index d1cee23437..3fe935cf28 100644
--- a/arch/arm/cpu/armv8/cache.S
+++ b/arch/arm/cpu/armv8/cache.S
@@ -12,6 +12,7 @@
#include
#include
+#ifndef CONFIG_CMO_BY_VA_ONLY
/*
* void __asm_dcache_level(level)
*
@@ -116,6 +117,41 @@ ENTRY(__asm_invalidate_dcache_all)
ENDPROC(__asm_invalidate_dcache_all)
.popsection
+.pushsection .text.__asm_flush_l3_dcache, "ax"
+WEAK(__asm_flush_l3_dcache)
+ mov x0, #0 /* return status as success */
+ ret
+ENDPROC(__asm_flush_l3_dcache)
+.popsection
+
+.pushsection .text.__asm_invalidate_l3_icache, "ax"
+WEAK(__asm_invalidate_l3_icache)
+ mov x0, #0 /* return status as success */
+ ret
+ENDPROC(__asm_invalidate_l3_icache)
+.popsection
+
+#else /* CONFIG_CMO_BY_VA */
+
+/*
+ * Define these so that they actively clash with in implementation
+ * accidentally selecting CONFIG_CMO_BY_VA
+ */
+
+.pushsection .text.__asm_invalidate_l3_icache, "ax"
+ENTRY(__asm_invalidate_l3_icache)
+ mov x0, xzr
+ ret
+ENDPROC(__asm_invalidate_l3_icache)
+.popsection
+.pushsection .text.__asm_flush_l3_dcache, "ax"
+ENTRY(__asm_flush_l3_dcache)
+ mov x0, xzr
+ ret
+ENDPROC(__asm_flush_l3_dcache)
+.popsection
+#endif /* CONFIG_CMO_BY_VA */
+
/*
* void __asm_flush_dcache_range(start, end)
*
@@ -189,20 +225,6 @@ WEAK(__asm_invalidate_l3_dcache)
ENDPROC(__asm_invalidate_l3_dcache)
.popsection
-.pushsection .text.__asm_flush_l3_dcache, "ax"
-WEAK(__asm_flush_l3_dcache)
- mov x0, #0 /* return status as success */
- ret
-ENDPROC(__asm_flush_l3_dcache)
-.popsection
-
-.pushsection .text.__asm_invalidate_l3_icache, "ax"
-WEAK(__asm_invalidate_l3_icache)
- mov x0, #0 /* return status as success */
- ret
-ENDPROC(__asm_invalidate_l3_icache)
-.popsection
-
/*
* void __asm_switch_ttbr(ulong new_ttbr)
*
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
index 2a226fd063..f333ad8889 100644
--- a/arch/arm/cpu/armv8/cache_v8.c
+++ b/arch/arm/cpu/armv8/cache_v8.c
@@ -163,6 +163,83 @@ static u64 *find_pte(u64 addr, int level)
return NULL;
}
+#ifdef CONFIG_CMO_BY_VA_ONLY
+static void __cmo_on_leaves(void (*cmo_fn)(unsigned long, unsigned long),
+ u64 pte, int level, u64 base)
+{
+ u64 *ptep;
+ int i;
+
+ ptep = (u64 *)(pte & GENMASK_ULL(47, PAGE_SHIFT));
+ for (i = 0; i < PAGE_SIZE / sizeof(u64); i++) {
+ u64 end, va = base + i * BIT(level2shift(level));
+ u64 type, attrs;
+
+ pte = ptep[i];
+ type = pte & PTE_TYPE_MASK;
+ attrs = pte & PMD_ATTRINDX_MASK;
+ debug("PTE %llx at level %d VA %llx\n", pte, level, va);
+
+ /* Not valid? next! */
+ if (!(type & PTE_TYPE_VALID))
+ continue;
+
+ /* Not a leaf? Recurse on the next level */
+ if (!(type == PTE_TYPE_BLOCK ||
+ (level == 3 && type == PTE_TYPE_PAGE))) {
+ __cmo_on_leaves(cmo_fn, pte, level + 1, va);
+ continue;
+ }
+
+ /*
+* From this point, this must be a leaf.
+*
+* Start excluding non memory mappings
+*/
+ if (attrs != PTE_BLOCK_MEMTYPE(MT_NORMAL) &&
+
