| Message ID | 20230207162014.58664-2-paul.liu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: cpu: Add optional CMOs by VA | expand |
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) <paul.liu@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Will Deacon <willdeacon@google.com> > Cc: Tom Rini <trini@konsulko.com> 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.
On Tue, Feb 07, 2023 at 04:35:25PM +0000, 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) <paul.liu@linaro.org> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Will Deacon <willdeacon@google.com> > > Cc: Tom Rini <trini@konsulko.com> > > 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.
On Tue, 07 Feb 2023 16:40:05 +0000, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Feb 07, 2023 at 04:35:25PM +0000, 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) <paul.liu@linaro.org> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > Signed-off-by: Will Deacon <willdeacon@google.com> > > > Cc: Tom Rini <trini@konsulko.com> > > > > 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
On Tue, Feb 07, 2023 at 05:06:39PM +0000, Marc Zyngier wrote: > On Tue, 07 Feb 2023 16:40:05 +0000, > Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Feb 07, 2023 at 04:35:25PM +0000, 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) <paul.liu@linaro.org> > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > > Signed-off-by: Will Deacon <willdeacon@google.com> > > > > Cc: Tom Rini <trini@konsulko.com> > > > > > > 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.
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 <maz@kernel.org> 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) <paul.liu@linaro.org> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Will Deacon <willdeacon@google.com> > > Cc: Tom Rini <trini@konsulko.com> > > 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... >
On Tue, 07 Feb 2023 17:18:27 +0000, Paul Liu <paul.liu@linaro.org> 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: <quote> From: Random Developer <rd@ilikesoup.org> Fix foo handling in bar(). Signed-off-by: Random Developer <rd@ilikesoup.org> Signed-off-by: Random Committer <rc@gimmecheese.net> [Paul: picked from the FooBar tree, fixed the return value for bar() and rebased to upstream] Signed-off-by: Paul Liu <paul.liu@linaro.org> Link: https://git.foobar.com/commit/?df2d85d0b0b5b1533d6db9079f0a0a7b73ef6a34 </quote> 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 <rd@ilikesoup.org>" 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.
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 <asm/system.h> #include <linux/linkage.h> +#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) && + attrs != PTE_BLOCK_MEMTYPE(MT_NORMAL_NC)) + continue; + + end = va + BIT(level2shift(level)) - 1; + + /* No intersection with RAM? */ + if (end < gd->ram_base || + va >= (gd->ram_base + gd->ram_size)) + continue; + + /* + * OK, we have a partial RAM mapping. However, this + * can cover *more* than the RAM. Yes, u-boot is + * *that* braindead. Compute the intersection we care + * about, and not a byte more. + */ + va = max(va, (u64)gd->ram_base); + end = min(end, gd->ram_base + gd->ram_size); + + debug("Flush PTE %llx at level %d: %llx-%llx\n", + pte, level, va, end); + cmo_fn(va, end); + } +} + +static void apply_cmo_to_mappings(void (*cmo_fn)(unsigned long, unsigned long)) +{ + u64 va_bits; + int sl = 0; + + if (!gd->arch.tlb_addr) + return; + + get_tcr(NULL, &va_bits); + if (va_bits < 39) + sl = 1; + + __cmo_on_leaves(cmo_fn, gd->arch.tlb_addr, sl, 0); +} +#else +static inline void apply_cmo_to_mappings(void *dummy) {} +#endif + /* Returns and creates a new full table (512 entries) */ static u64 *create_table(void) { @@ -447,8 +524,12 @@ __weak void mmu_setup(void) */ void invalidate_dcache_all(void) { +#ifndef CONFIG_CMO_BY_VA_ONLY __asm_invalidate_dcache_all(); __asm_invalidate_l3_dcache(); +#else + apply_cmo_to_mappings(invalidate_dcache_range); +#endif } /* @@ -458,6 +539,7 @@ void invalidate_dcache_all(void) */ inline void flush_dcache_all(void) { +#ifndef CONFIG_CMO_BY_VA_ONLY int ret; __asm_flush_dcache_all(); @@ -466,6 +548,9 @@ inline void flush_dcache_all(void) debug("flushing dcache returns 0x%x\n", ret); else debug("flushing dcache successfully.\n"); +#else + apply_cmo_to_mappings(flush_dcache_range); +#endif } #ifndef CONFIG_SYS_DISABLE_DCACHE_OPS @@ -520,9 +605,19 @@ void dcache_disable(void) if (!(sctlr & CR_C)) return; + if (IS_ENABLED(CONFIG_CMO_BY_VA_ONLY)) { + /* + * When invalidating by VA, do it *before* turning the MMU + * off, so that at least our stack is coherent. + */ + flush_dcache_all(); + } + set_sctlr(sctlr & ~(CR_C|CR_M)); - flush_dcache_all(); + if (!IS_ENABLED(CONFIG_CMO_BY_VA_ONLY)) + flush_dcache_all(); + __asm_invalidate_tlb_all(); } diff --git a/arch/arm/cpu/armv8/cpu.c b/arch/arm/cpu/armv8/cpu.c index db5d460eb4..3c7f36ad8d 100644 --- a/arch/arm/cpu/armv8/cpu.c +++ b/arch/arm/cpu/armv8/cpu.c @@ -48,18 +48,26 @@ int cleanup_before_linux(void) disable_interrupts(); - /* - * Turn off I-cache and invalidate it - */ - icache_disable(); - invalidate_icache_all(); + if (IS_ENABLED(CONFIG_CMO_BY_VA_ONLY)) { + /* + * Disable D-cache. + */ + dcache_disable(); + } else { + /* + * Turn off I-cache and invalidate it + */ + icache_disable(); + invalidate_icache_all(); - /* - * turn off D-cache - * dcache_disable() in turn flushes the d-cache and disables MMU - */ - dcache_disable(); - invalidate_dcache_all(); + /* + * turn off D-cache + * dcache_disable() in turn flushes the d-cache and disables + * MMU + */ + dcache_disable(); + invalidate_dcache_all(); + } return 0; }