* [PATCH v2 10/10] powerpc/fsl_booke/kaslr: dump out kernel offset information on panic
From: Jason Yan @ 2019-07-30 7:42 UTC (permalink / raw)
To: mpe, linuxppc-dev, diana.craciun, christophe.leroy, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, Jason Yan, linux-kernel, jingxiangfeng,
zhaohongjiang, thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-1-yanaijie@huawei.com>
When kaslr is enabled, the kernel offset is different for every boot.
This brings some difficult to debug the kernel. Dump out the kernel
offset when panic so that we can easily debug the kernel.
Signed-off-by: Jason Yan <yanaijie@huawei.com>
Cc: Diana Craciun <diana.craciun@nxp.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
---
arch/powerpc/include/asm/page.h | 5 +++++
arch/powerpc/kernel/machine_kexec.c | 1 +
arch/powerpc/kernel/setup-common.c | 19 +++++++++++++++++++
3 files changed, 25 insertions(+)
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 60a68d3a54b1..cd3ac530e58d 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -317,6 +317,11 @@ struct vm_area_struct;
extern unsigned long kimage_vaddr;
+static inline unsigned long kaslr_offset(void)
+{
+ return kimage_vaddr - KERNELBASE;
+}
+
#include <asm-generic/memory_model.h>
#endif /* __ASSEMBLY__ */
#include <asm/slice.h>
diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
index c4ed328a7b96..078fe3d76feb 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -86,6 +86,7 @@ void arch_crash_save_vmcoreinfo(void)
VMCOREINFO_STRUCT_SIZE(mmu_psize_def);
VMCOREINFO_OFFSET(mmu_psize_def, shift);
#endif
+ vmcoreinfo_append_str("KERNELOFFSET=%lx\n", kaslr_offset());
}
/*
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 1f8db666468d..064075f02837 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -715,12 +715,31 @@ static struct notifier_block ppc_panic_block = {
.priority = INT_MIN /* may not return; must be done last */
};
+/*
+ * Dump out kernel offset information on panic.
+ */
+static int dump_kernel_offset(struct notifier_block *self, unsigned long v,
+ void *p)
+{
+ pr_emerg("Kernel Offset: 0x%lx from 0x%lx\n",
+ kaslr_offset(), KERNELBASE);
+
+ return 0;
+}
+
+static struct notifier_block kernel_offset_notifier = {
+ .notifier_call = dump_kernel_offset
+};
+
void __init setup_panic(void)
{
/* PPC64 always does a hard irq disable in its panic handler */
if (!IS_ENABLED(CONFIG_PPC64) && !ppc_md.panic)
return;
atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
+ if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_offset() > 0)
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &kernel_offset_notifier);
}
#ifdef CONFIG_CHECK_CACHE_COHERENCY
--
2.17.2
^ permalink raw reply related
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Arnd Bergmann @ 2019-07-30 7:34 UTC (permalink / raw)
To: Segher Boessenkool
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <20190729215200.GN31406@gate.crashing.org>
On Mon, Jul 29, 2019 at 11:52 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Mon, Jul 29, 2019 at 01:32:46PM -0700, Nathan Chancellor wrote:
> > For the record:
> >
> > https://godbolt.org/z/z57VU7
> >
> > This seems consistent with what Michael found so I don't think a revert
> > is entirely unreasonable.
>
> Try this:
>
> https://godbolt.org/z/6_ZfVi
>
> This matters in non-trivial loops, for example. But all current cases
> where such non-trivial loops are done with cache block instructions are
> actually written in real assembler already, using two registers.
> Because performance matters. Not that I recommend writing code as
> critical as memset in C with inline asm :-)
Upon a second look, I think the issue is that the "Z" is an input argument
when it should be an output. clang decides that it can make a copy of the
input and pass that into the inline asm. This is not the most efficient
way, but it seems entirely correct according to the constraints.
Changing it to an output "=Z" constraint seems to make it work:
https://godbolt.org/z/FwEqHf
Clang still doesn't use the optimum form, but it passes the correct pointer.
Arnd
^ permalink raw reply
* Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Michal Hocko @ 2019-07-30 8:14 UTC (permalink / raw)
To: Hoan Tran OS
Cc: Heiko Carstens, open list:MEMORY MANAGEMENT, Paul Mackerras,
H . Peter Anvin, sparclinux@vger.kernel.org, Alexander Duyck,
Will Deacon, linux-s390@vger.kernel.org, x86@kernel.org,
willy@infradead.org, Mike Rapoport, Christian Borntraeger,
Ingo Molnar, linux-arm-kernel@lists.infradead.org,
Catalin Marinas, Open Source Submission, Pavel Tatashin,
Vasily Gorbik, Will Deacon, Borislav Petkov, Thomas Gleixner,
Vlastimil Babka, Oscar Salvador, linux-kernel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <730368c5-1711-89ae-e3ef-65418b17ddc9@os.amperecomputing.com>
[Sorry for a late reply]
On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
> Hi,
>
> On 7/12/19 10:00 PM, Michal Hocko wrote:
[...]
> > Hmm, I thought this was selectable. But I am obviously wrong here.
> > Looking more closely, it seems that this is indeed only about
> > __early_pfn_to_nid and as such not something that should add a config
> > symbol. This should have been called out in the changelog though.
>
> Yes, do you have any other comments about my patch?
Not really. Just make sure to explicitly state that
CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
doesn't really deserve it's own config and can be pulled under NUMA.
> > Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
> > bucket? Do we have any NUMA architecture that doesn't enable it?
> >
>
> As I checked with arch Kconfig files, there are 2 architectures, riscv
> and microblaze, do not support NUMA but enable this config.
>
> And 1 architecture, alpha, supports NUMA but does not enable this config.
Care to have a look and clean this up please?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 02/10] powerpc: move memstart_addr and kernstart_addr to init-common.c
From: Christophe Leroy @ 2019-07-30 9:25 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-3-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> These two variables are both defined in init_32.c and init_64.c. Move
> them to init-common.c.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/mm/init-common.c | 5 +++++
> arch/powerpc/mm/init_32.c | 5 -----
> arch/powerpc/mm/init_64.c | 5 -----
> 3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index a84da92920f7..152ae0d21435 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -21,6 +21,11 @@
> #include <asm/pgtable.h>
> #include <asm/kup.h>
>
> +phys_addr_t memstart_addr = (phys_addr_t)~0ull;
> +EXPORT_SYMBOL_GPL(memstart_addr);
> +phys_addr_t kernstart_addr;
> +EXPORT_SYMBOL_GPL(kernstart_addr);
> +
> static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
> static bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>
> diff --git a/arch/powerpc/mm/init_32.c b/arch/powerpc/mm/init_32.c
> index b04896a88d79..872df48ae41b 100644
> --- a/arch/powerpc/mm/init_32.c
> +++ b/arch/powerpc/mm/init_32.c
> @@ -56,11 +56,6 @@
> phys_addr_t total_memory;
> phys_addr_t total_lowmem;
>
> -phys_addr_t memstart_addr = (phys_addr_t)~0ull;
> -EXPORT_SYMBOL(memstart_addr);
> -phys_addr_t kernstart_addr;
> -EXPORT_SYMBOL(kernstart_addr);
> -
> #ifdef CONFIG_RELOCATABLE
> /* Used in __va()/__pa() */
> long long virt_phys_offset;
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index a44f6281ca3a..c836f1269ee7 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -63,11 +63,6 @@
>
> #include <mm/mmu_decl.h>
>
> -phys_addr_t memstart_addr = ~0;
> -EXPORT_SYMBOL_GPL(memstart_addr);
> -phys_addr_t kernstart_addr;
> -EXPORT_SYMBOL_GPL(kernstart_addr);
> -
> #ifdef CONFIG_SPARSEMEM_VMEMMAP
> /*
> * Given an address within the vmemmap, determine the pfn of the page that
>
^ permalink raw reply
* Re: [PATCH v2 03/10] powerpc: introduce kimage_vaddr to store the kernel base
From: Christophe Leroy @ 2019-07-30 9:25 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-4-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> Now the kernel base is a fixed value - KERNELBASE. To support KASLR, we
> need a variable to store the kernel base.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/page.h | 2 ++
> arch/powerpc/mm/init-common.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 0d52f57fca04..60a68d3a54b1 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -315,6 +315,8 @@ void arch_free_page(struct page *page, int order);
>
> struct vm_area_struct;
>
> +extern unsigned long kimage_vaddr;
> +
> #include <asm-generic/memory_model.h>
> #endif /* __ASSEMBLY__ */
> #include <asm/slice.h>
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 152ae0d21435..d4801ce48dc5 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -25,6 +25,8 @@ phys_addr_t memstart_addr = (phys_addr_t)~0ull;
> EXPORT_SYMBOL_GPL(memstart_addr);
> phys_addr_t kernstart_addr;
> EXPORT_SYMBOL_GPL(kernstart_addr);
> +unsigned long kimage_vaddr = KERNELBASE;
> +EXPORT_SYMBOL_GPL(kimage_vaddr);
>
> static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
> static bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>
^ permalink raw reply
* Re: [PATCH v2 04/10] powerpc/fsl_booke/32: introduce create_tlb_entry() helper
From: Christophe Leroy @ 2019-07-30 9:26 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-5-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> Add a new helper create_tlb_entry() to create a tlb entry by the virtual
> and physical address. This is a preparation to support boot kernel at a
> randomized address.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/head_fsl_booke.S | 29 ++++++++++++++++++++++++++++
> arch/powerpc/mm/mmu_decl.h | 1 +
> 2 files changed, 30 insertions(+)
>
> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> index adf0505dbe02..04d124fee17d 100644
> --- a/arch/powerpc/kernel/head_fsl_booke.S
> +++ b/arch/powerpc/kernel/head_fsl_booke.S
> @@ -1114,6 +1114,35 @@ __secondary_hold_acknowledge:
> .long -1
> #endif
>
> +/*
> + * Create a 64M tlb by address and entry
> + * r3/r4 - physical address
> + * r5 - virtual address
> + * r6 - entry
> + */
> +_GLOBAL(create_tlb_entry)
> + lis r7,0x1000 /* Set MAS0(TLBSEL) = 1 */
> + rlwimi r7,r6,16,4,15 /* Setup MAS0 = TLBSEL | ESEL(r6) */
> + mtspr SPRN_MAS0,r7 /* Write MAS0 */
> +
> + lis r6,(MAS1_VALID|MAS1_IPROT)@h
> + ori r6,r6,(MAS1_TSIZE(BOOK3E_PAGESZ_64M))@l
> + mtspr SPRN_MAS1,r6 /* Write MAS1 */
> +
> + lis r6,MAS2_EPN_MASK(BOOK3E_PAGESZ_64M)@h
> + ori r6,r6,MAS2_EPN_MASK(BOOK3E_PAGESZ_64M)@l
> + and r6,r6,r5
> + ori r6,r6,MAS2_M@l
> + mtspr SPRN_MAS2,r6 /* Write MAS2(EPN) */
> +
> + ori r8,r4,(MAS3_SW|MAS3_SR|MAS3_SX)
> + mtspr SPRN_MAS3,r8 /* Write MAS3(RPN) */
> +
> + tlbwe /* Write TLB */
> + isync
> + sync
> + blr
> +
> /*
> * Create a tlb entry with the same effective and physical address as
> * the tlb entry used by the current running code. But set the TS to 1.
> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
> index 32c1a191c28a..a09f89d3aa0f 100644
> --- a/arch/powerpc/mm/mmu_decl.h
> +++ b/arch/powerpc/mm/mmu_decl.h
> @@ -142,6 +142,7 @@ extern unsigned long calc_cam_sz(unsigned long ram, unsigned long virt,
> extern void adjust_total_lowmem(void);
> extern int switch_to_as1(void);
> extern void restore_to_as0(int esel, int offset, void *dt_ptr, int bootcpu);
> +void create_tlb_entry(phys_addr_t phys, unsigned long virt, int entry);
> #endif
> extern void loadcam_entry(unsigned int index);
> extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>
^ permalink raw reply
* Re: [PATCH v2 05/10] powerpc/fsl_booke/32: introduce reloc_kernel_entry() helper
From: Christophe Leroy @ 2019-07-30 9:26 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-6-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> Add a new helper reloc_kernel_entry() to jump back to the start of the
> new kernel. After we put the new kernel in a randomized place we can use
> this new helper to enter the kernel and begin to relocate again.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/head_fsl_booke.S | 13 +++++++++++++
> arch/powerpc/mm/mmu_decl.h | 1 +
> 2 files changed, 14 insertions(+)
>
> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> index 04d124fee17d..2083382dd662 100644
> --- a/arch/powerpc/kernel/head_fsl_booke.S
> +++ b/arch/powerpc/kernel/head_fsl_booke.S
> @@ -1143,6 +1143,19 @@ _GLOBAL(create_tlb_entry)
> sync
> blr
>
> +/*
> + * Return to the start of the relocated kernel and run again
> + * r3 - virtual address of fdt
> + * r4 - entry of the kernel
> + */
> +_GLOBAL(reloc_kernel_entry)
> + mfmsr r7
> + rlwinm r7, r7, 0, ~(MSR_IS | MSR_DS)
> +
> + mtspr SPRN_SRR0,r4
> + mtspr SPRN_SRR1,r7
> + rfi
> +
> /*
> * Create a tlb entry with the same effective and physical address as
> * the tlb entry used by the current running code. But set the TS to 1.
> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
> index a09f89d3aa0f..804da298beb3 100644
> --- a/arch/powerpc/mm/mmu_decl.h
> +++ b/arch/powerpc/mm/mmu_decl.h
> @@ -143,6 +143,7 @@ extern void adjust_total_lowmem(void);
> extern int switch_to_as1(void);
> extern void restore_to_as0(int esel, int offset, void *dt_ptr, int bootcpu);
> void create_tlb_entry(phys_addr_t phys, unsigned long virt, int entry);
> +void reloc_kernel_entry(void *fdt, int addr);
> #endif
> extern void loadcam_entry(unsigned int index);
> extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>
^ permalink raw reply
* Re: [PATCH v2 06/10] powerpc/fsl_booke/32: implement KASLR infrastructure
From: Christophe Leroy @ 2019-07-30 9:34 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-7-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> This patch add support to boot kernel from places other than KERNELBASE.
> Since CONFIG_RELOCATABLE has already supported, what we need to do is
> map or copy kernel to a proper place and relocate. Freescale Book-E
> parts expect lowmem to be mapped by fixed TLB entries(TLB1). The TLB1
> entries are not suitable to map the kernel directly in a randomized
> region, so we chose to copy the kernel to a proper place and restart to
> relocate.
>
> The offset of the kernel was not randomized yet(a fixed 64M is set). We
> will randomize it in the next patch.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> arch/powerpc/Kconfig | 11 +++
> arch/powerpc/kernel/Makefile | 1 +
> arch/powerpc/kernel/early_32.c | 2 +-
> arch/powerpc/kernel/fsl_booke_entry_mapping.S | 13 ++-
> arch/powerpc/kernel/head_fsl_booke.S | 15 +++-
> arch/powerpc/kernel/kaslr_booke.c | 84 +++++++++++++++++++
> arch/powerpc/mm/mmu_decl.h | 6 ++
> arch/powerpc/mm/nohash/fsl_booke.c | 7 +-
> 8 files changed, 126 insertions(+), 13 deletions(-)
> create mode 100644 arch/powerpc/kernel/kaslr_booke.c
>
[...]
> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> index 2083382dd662..a466c0f0d028 100644
> --- a/arch/powerpc/kernel/head_fsl_booke.S
> +++ b/arch/powerpc/kernel/head_fsl_booke.S
> @@ -155,6 +155,8 @@ _ENTRY(_start);
> */
>
> _ENTRY(__early_start)
> + LOAD_REG_ADDR_PIC(r20, kimage_vaddr)
> + lwz r20,0(r20)
>
> #define ENTRY_MAPPING_BOOT_SETUP
> #include "fsl_booke_entry_mapping.S"
> @@ -277,8 +279,8 @@ set_ivor:
> ori r6, r6, swapper_pg_dir@l
> lis r5, abatron_pteptrs@h
> ori r5, r5, abatron_pteptrs@l
> - lis r4, KERNELBASE@h
> - ori r4, r4, KERNELBASE@l
> + lis r3, kimage_vaddr@ha
> + lwz r4, kimage_vaddr@l(r3)
> stw r5, 0(r4) /* Save abatron_pteptrs at a fixed location */
> stw r6, 0(r5)
>
> @@ -1067,7 +1069,14 @@ __secondary_start:
> mr r5,r25 /* phys kernel start */
> rlwinm r5,r5,0,~0x3ffffff /* aligned 64M */
> subf r4,r5,r4 /* memstart_addr - phys kernel start */
> - li r5,0 /* no device tree */
> +#ifdef CONFIG_RANDOMIZE_BASE
Is that #ifdef really necessary ? Wouldn't it also work as expected when
CONFIG_RANDOMIZE_BASE is not selected ?
> + lis r7,KERNELBASE@h
> + ori r7,r7,KERNELBASE@l
> + cmpw r20,r7 /* if kimage_vaddr != KERNELBASE, randomized */
> + beq 2f
> + li r4,0
> +#endif
> +2: li r5,0 /* no device tree */
> li r6,0 /* not boot cpu */
> bl restore_to_as0
>
> diff --git a/arch/powerpc/kernel/kaslr_booke.c b/arch/powerpc/kernel/kaslr_booke.c
> new file mode 100644
> index 000000000000..960bce4aa8b9
> --- /dev/null
> +++ b/arch/powerpc/kernel/kaslr_booke.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 Jason Yan <yanaijie@huawei.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/signal.h>
> +#include <linux/sched.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/ptrace.h>
> +#include <linux/mman.h>
> +#include <linux/mm.h>
> +#include <linux/swap.h>
> +#include <linux/stddef.h>
> +#include <linux/vmalloc.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/highmem.h>
> +#include <linux/memblock.h>
> +#include <asm/pgalloc.h>
> +#include <asm/prom.h>
> +#include <asm/io.h>
> +#include <asm/mmu_context.h>
> +#include <asm/pgtable.h>
> +#include <asm/mmu.h>
> +#include <linux/uaccess.h>
> +#include <asm/smp.h>
> +#include <asm/machdep.h>
> +#include <asm/setup.h>
> +#include <asm/paca.h>
> +#include <mm/mmu_decl.h>
> +
> +extern int is_second_reloc;
Couldn't the above be a bool ?
> +
> +static unsigned long __init kaslr_choose_location(void *dt_ptr, phys_addr_t size,
> + unsigned long kernel_sz)
> +{
> + /* return a fixed offset of 64M for now */
> + return 0x4000000;
64 << 20 would maybe be more explicit than 0x4000000.
Or return SZ_64M ?
Christophe
> +}
> +
> +/*
> + * To see if we need to relocate the kernel to a random offset
> + * void *dt_ptr - address of the device tree
> + * phys_addr_t size - size of the first memory block
> + */
> +notrace void __init kaslr_early_init(void *dt_ptr, phys_addr_t size)
> +{
> + unsigned long tlb_virt;
> + phys_addr_t tlb_phys;
> + unsigned long offset;
> + unsigned long kernel_sz;
> +
> + kernel_sz = (unsigned long)_end - KERNELBASE;
> +
> + offset = kaslr_choose_location(dt_ptr, size, kernel_sz);
> +
> + if (offset == 0)
> + return;
> +
> + kimage_vaddr += offset;
> + kernstart_addr += offset;
> +
> + is_second_reloc = 1;
> +
> + if (offset >= SZ_64M) {
> + tlb_virt = round_down(kimage_vaddr, SZ_64M);
> + tlb_phys = round_down(kernstart_addr, SZ_64M);
> +
> + /* Create kernel map to relocate in */
> + create_tlb_entry(tlb_phys, tlb_virt, 1);
> + }
> +
> + /* Copy the kernel to it's new location and run */
> + memcpy((void *)kimage_vaddr, (void *)KERNELBASE, kernel_sz);
> +
> + reloc_kernel_entry(dt_ptr, kimage_vaddr);
> +}
> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
> index 804da298beb3..9332772c8a66 100644
> --- a/arch/powerpc/mm/mmu_decl.h
> +++ b/arch/powerpc/mm/mmu_decl.h
> @@ -148,6 +148,12 @@ void reloc_kernel_entry(void *fdt, int addr);
> extern void loadcam_entry(unsigned int index);
> extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>
> +#ifdef CONFIG_RANDOMIZE_BASE
> +void kaslr_early_init(void *dt_ptr, phys_addr_t size);
> +#else
> +static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
> +#endif
> +
> struct tlbcam {
> u32 MAS0;
> u32 MAS1;
> diff --git a/arch/powerpc/mm/nohash/fsl_booke.c b/arch/powerpc/mm/nohash/fsl_booke.c
> index 556e3cd52a35..8d25a8dc965f 100644
> --- a/arch/powerpc/mm/nohash/fsl_booke.c
> +++ b/arch/powerpc/mm/nohash/fsl_booke.c
> @@ -263,7 +263,8 @@ void setup_initial_memory_limit(phys_addr_t first_memblock_base,
> int __initdata is_second_reloc;
> notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
> {
> - unsigned long base = KERNELBASE;
> + unsigned long base = kimage_vaddr;
> + phys_addr_t size;
>
> kernstart_addr = start;
> if (is_second_reloc) {
> @@ -291,7 +292,7 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
> start &= ~0x3ffffff;
> base &= ~0x3ffffff;
> virt_phys_offset = base - start;
> - early_get_first_memblock_info(__va(dt_ptr), NULL);
> + early_get_first_memblock_info(__va(dt_ptr), &size);
> /*
> * We now get the memstart_addr, then we should check if this
> * address is the same as what the PAGE_OFFSET map to now. If
> @@ -316,6 +317,8 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
> /* We should never reach here */
> panic("Relocation error");
> }
> +
> + kaslr_early_init(__va(dt_ptr), size);
> }
> #endif
> #endif
>
^ permalink raw reply
* Re: [PATCH v2 07/10] powerpc/fsl_booke/32: randomize the kernel image offset
From: Christophe Leroy @ 2019-07-30 9:44 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-8-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> After we have the basic support of relocate the kernel in some
> appropriate place, we can start to randomize the offset now.
>
> Entropy is derived from the banner and timer, which will change every
> build and boot. This not so much safe so additionally the bootloader may
> pass entropy via the /chosen/kaslr-seed node in device tree.
>
> We will use the first 512M of the low memory to randomize the kernel
> image. The memory will be split in 64M zones. We will use the lower 8
> bit of the entropy to decide the index of the 64M zone. Then we chose a
> 16K aligned offset inside the 64M zone to put the kernel in.
>
> KERNELBASE
>
> |--> 64M <--|
> | |
> +---------------+ +----------------+---------------+
> | |....| |kernel| | |
> +---------------+ +----------------+---------------+
> | |
> |-----> offset <-----|
>
> kimage_vaddr
>
> We also check if we will overlap with some areas like the dtb area, the
> initrd area or the crashkernel area. If we cannot find a proper area,
> kaslr will be disabled and boot from the original kernel.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> arch/powerpc/kernel/kaslr_booke.c | 334 +++++++++++++++++++++++++++++-
> 1 file changed, 332 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kaslr_booke.c b/arch/powerpc/kernel/kaslr_booke.c
> index 960bce4aa8b9..0bb02e45b928 100644
> --- a/arch/powerpc/kernel/kaslr_booke.c
> +++ b/arch/powerpc/kernel/kaslr_booke.c
> @@ -23,6 +23,8 @@
> #include <linux/delay.h>
> #include <linux/highmem.h>
> #include <linux/memblock.h>
> +#include <linux/libfdt.h>
> +#include <linux/crash_core.h>
> #include <asm/pgalloc.h>
> #include <asm/prom.h>
> #include <asm/io.h>
> @@ -34,15 +36,341 @@
> #include <asm/machdep.h>
> #include <asm/setup.h>
> #include <asm/paca.h>
> +#include <asm/kdump.h>
> #include <mm/mmu_decl.h>
> +#include <generated/compile.h>
> +#include <generated/utsrelease.h>
> +
> +#ifdef DEBUG
> +#define DBG(fmt...) printk(KERN_ERR fmt)
> +#else
> +#define DBG(fmt...)
> +#endif
> +
> +struct regions {
> + unsigned long pa_start;
> + unsigned long pa_end;
> + unsigned long kernel_size;
> + unsigned long dtb_start;
> + unsigned long dtb_end;
> + unsigned long initrd_start;
> + unsigned long initrd_end;
> + unsigned long crash_start;
> + unsigned long crash_end;
> + int reserved_mem;
> + int reserved_mem_addr_cells;
> + int reserved_mem_size_cells;
> +};
>
> extern int is_second_reloc;
>
> +/* Simplified build-specific string for starting entropy. */
> +static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
> + LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
> +
> +static __init void kaslr_get_cmdline(void *fdt)
> +{
> + const char *cmdline = CONFIG_CMDLINE;
> + if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
> + int node;
> + const u8 *prop;
> + node = fdt_path_offset(fdt, "/chosen");
> + if (node < 0)
> + goto out;
> +
> + prop = fdt_getprop(fdt, node, "bootargs", NULL);
> + if (!prop)
> + goto out;
> + cmdline = prop;
> + }
> +out:
> + strscpy(boot_command_line, cmdline, COMMAND_LINE_SIZE);
boot_command_line is set by early_init_devtree() in
arch/powerpc/kernel/prom.c
Is that too late for you ?
If so, what about calling early_init_dt_scan_chosen() instead of recoding ?
Christophe
^ permalink raw reply
* RE: [PATCH] powerpc/kmcent2: update the ethernet devices' phy properties
From: Madalin-cristian Bucur @ 2019-07-30 9:44 UTC (permalink / raw)
To: Scott Wood, Valentin Longchamp, linuxppc-dev@lists.ozlabs.org,
galak@kernel.crashing.org
Cc: netdev@vger.kernel.org
In-Reply-To: <2243421e574c72c5e75d27cc0122338e2e0bde63.camel@buserror.net>
> -----Original Message-----
> From: Scott Wood <oss@buserror.net>
> Sent: Sunday, July 28, 2019 10:27 PM
> To: Valentin Longchamp <valentin@longchamp.me>; linuxppc-
> dev@lists.ozlabs.org; galak@kernel.crashing.org
> Cc: Madalin-cristian Bucur <madalin.bucur@nxp.com>
> Subject: Re: [PATCH] powerpc/kmcent2: update the ethernet devices' phy
> properties
>
> On Sun, 2019-07-28 at 18:01 +0200, Valentin Longchamp wrote:
> > Hi Scott, Kumar,
> >
> > Looking at this patch I have realised that I had already submitted it
> > to the mailing list nearly 2 years ago:
> >
> > https://patchwork.ozlabs.org/patch/842944/
> >
> > Could you please make sure that this one gets merged in the next
> > window, so that I avoid forgetting such a patch a 2nd time ?
> >
> > Thanks a lot
>
> I added it to my patchwork todo list; thanks for the reminder.
>
> > Le dim. 14 juil. 2019 à 22:05, Valentin Longchamp
> > <valentin@longchamp.me> a écrit :
> > >
> > > Change all phy-connection-type properties to phy-mode that are better
> > > supported by the fman driver.
> > >
> > > Use the more readable fixed-link node for the 2 sgmii links.
> > >
> > > Change the RGMII link to rgmii-id as the clock delays are added by the
> > > phy.
> > >
> > > Signed-off-by: Valentin Longchamp <valentin@longchamp.me>
>
> I don't see any other uses of phy-mode in arch/powerpc/boot/dts/fsl, and I see
> lots of phy-connection-type with fman. Madalin, does this patch look OK?
>
> -Scott
Hi,
we are using "phy-connection-type" not "phy-mode" for the NXP (former Freescale)
DPAA platforms. While the two seem to be interchangeable ("phy-mode" seems to be
more recent, looking at the device tree bindings), the driver code in Linux seems
to use one or the other, not both so one should stick with the variant the driver
is using. To make things more complex, there may be dependencies in bootloaders,
I see code in u-boot using only "phy-connection-type" or only "phy-mode".
I'd leave "phy-connection-type" as is.
Madalin
> > > ---
> > > arch/powerpc/boot/dts/fsl/kmcent2.dts | 16 +++++++++++-----
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > index 48b7f9797124..c3e0741cafb1 100644
> > > --- a/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > +++ b/arch/powerpc/boot/dts/fsl/kmcent2.dts
> > > @@ -210,13 +210,19 @@
> > >
> > > fman@400000 {
> > > ethernet@e0000 {
> > > - fixed-link = <0 1 1000 0 0>;
> > > - phy-connection-type = "sgmii";
> > > + phy-mode = "sgmii";
> > > + fixed-link {
> > > + speed = <1000>;
> > > + full-duplex;
> > > + };
> > > };
> > >
> > > ethernet@e2000 {
> > > - fixed-link = <1 1 1000 0 0>;
> > > - phy-connection-type = "sgmii";
> > > + phy-mode = "sgmii";
> > > + fixed-link {
> > > + speed = <1000>;
> > > + full-duplex;
> > > + };
> > > };
> > >
> > > ethernet@e4000 {
> > > @@ -229,7 +235,7 @@
> > >
> > > ethernet@e8000 {
> > > phy-handle = <&front_phy>;
> > > - phy-connection-type = "rgmii";
> > > + phy-mode = "rgmii-id";
> > > };
> > >
> > > mdio0: mdio@fc000 {
> > > --
> > > 2.17.1
> > >
> >
> >
^ permalink raw reply
* Re: [PATCH v2 08/10] powerpc/fsl_booke/kaslr: clear the original kernel if randomized
From: Christophe Leroy @ 2019-07-30 9:47 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-9-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> The original kernel still exists in the memory, clear it now.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Is kaslr_second_init() the best name ?
What about kaslr_late_init() or kaslr_clear_early_kernel() or ... ?
Christophe
> ---
> arch/powerpc/kernel/kaslr_booke.c | 11 +++++++++++
> arch/powerpc/mm/mmu_decl.h | 2 ++
> arch/powerpc/mm/nohash/fsl_booke.c | 1 +
> 3 files changed, 14 insertions(+)
>
> diff --git a/arch/powerpc/kernel/kaslr_booke.c b/arch/powerpc/kernel/kaslr_booke.c
> index 0bb02e45b928..f032ac119457 100644
> --- a/arch/powerpc/kernel/kaslr_booke.c
> +++ b/arch/powerpc/kernel/kaslr_booke.c
> @@ -412,3 +412,14 @@ notrace void __init kaslr_early_init(void *dt_ptr, phys_addr_t size)
>
> reloc_kernel_entry(dt_ptr, kimage_vaddr);
> }
> +
> +void __init kaslr_second_init(void)
> +{
> + /* If randomized, clear the original kernel */
> + if (kimage_vaddr != KERNELBASE) {
> + unsigned long kernel_sz;
> +
> + kernel_sz = (unsigned long)_end - kimage_vaddr;
> + memzero_explicit((void *)KERNELBASE, kernel_sz);
> + }
> +}
> diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
> index 9332772c8a66..550c8d742f88 100644
> --- a/arch/powerpc/mm/mmu_decl.h
> +++ b/arch/powerpc/mm/mmu_decl.h
> @@ -150,8 +150,10 @@ extern void loadcam_multi(int first_idx, int num, int tmp_idx);
>
> #ifdef CONFIG_RANDOMIZE_BASE
> void kaslr_early_init(void *dt_ptr, phys_addr_t size);
> +void kaslr_second_init(void);
> #else
> static inline void kaslr_early_init(void *dt_ptr, phys_addr_t size) {}
> +static inline void kaslr_second_init(void) {}
> #endif
>
> struct tlbcam {
> diff --git a/arch/powerpc/mm/nohash/fsl_booke.c b/arch/powerpc/mm/nohash/fsl_booke.c
> index 8d25a8dc965f..fa5a87f5c08e 100644
> --- a/arch/powerpc/mm/nohash/fsl_booke.c
> +++ b/arch/powerpc/mm/nohash/fsl_booke.c
> @@ -269,6 +269,7 @@ notrace void __init relocate_init(u64 dt_ptr, phys_addr_t start)
> kernstart_addr = start;
> if (is_second_reloc) {
> virt_phys_offset = PAGE_OFFSET - memstart_addr;
> + kaslr_second_init();
> return;
> }
>
>
^ permalink raw reply
* Re: [PATCH v2 10/10] powerpc/fsl_booke/kaslr: dump out kernel offset information on panic
From: Christophe Leroy @ 2019-07-30 9:48 UTC (permalink / raw)
To: Jason Yan, mpe, linuxppc-dev, diana.craciun, benh, paulus,
npiggin, keescook, kernel-hardening
Cc: wangkefeng.wang, linux-kernel, jingxiangfeng, zhaohongjiang,
thunder.leizhen, fanchengyang, yebin10
In-Reply-To: <20190730074225.39544-11-yanaijie@huawei.com>
Le 30/07/2019 à 09:42, Jason Yan a écrit :
> When kaslr is enabled, the kernel offset is different for every boot.
> This brings some difficult to debug the kernel. Dump out the kernel
> offset when panic so that we can easily debug the kernel.
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Cc: Diana Craciun <diana.craciun@nxp.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/page.h | 5 +++++
> arch/powerpc/kernel/machine_kexec.c | 1 +
> arch/powerpc/kernel/setup-common.c | 19 +++++++++++++++++++
> 3 files changed, 25 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 60a68d3a54b1..cd3ac530e58d 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -317,6 +317,11 @@ struct vm_area_struct;
>
> extern unsigned long kimage_vaddr;
>
> +static inline unsigned long kaslr_offset(void)
> +{
> + return kimage_vaddr - KERNELBASE;
> +}
> +
> #include <asm-generic/memory_model.h>
> #endif /* __ASSEMBLY__ */
> #include <asm/slice.h>
> diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c
> index c4ed328a7b96..078fe3d76feb 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -86,6 +86,7 @@ void arch_crash_save_vmcoreinfo(void)
> VMCOREINFO_STRUCT_SIZE(mmu_psize_def);
> VMCOREINFO_OFFSET(mmu_psize_def, shift);
> #endif
> + vmcoreinfo_append_str("KERNELOFFSET=%lx\n", kaslr_offset());
> }
>
> /*
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 1f8db666468d..064075f02837 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -715,12 +715,31 @@ static struct notifier_block ppc_panic_block = {
> .priority = INT_MIN /* may not return; must be done last */
> };
>
> +/*
> + * Dump out kernel offset information on panic.
> + */
> +static int dump_kernel_offset(struct notifier_block *self, unsigned long v,
> + void *p)
> +{
> + pr_emerg("Kernel Offset: 0x%lx from 0x%lx\n",
> + kaslr_offset(), KERNELBASE);
> +
> + return 0;
> +}
> +
> +static struct notifier_block kernel_offset_notifier = {
> + .notifier_call = dump_kernel_offset
> +};
> +
> void __init setup_panic(void)
> {
> /* PPC64 always does a hard irq disable in its panic handler */
> if (!IS_ENABLED(CONFIG_PPC64) && !ppc_md.panic)
> return;
> atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
> + if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && kaslr_offset() > 0)
> + atomic_notifier_chain_register(&panic_notifier_list,
> + &kernel_offset_notifier);
> }
>
> #ifdef CONFIG_CHECK_CACHE_COHERENCY
>
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Michael Ellerman @ 2019-07-30 11:17 UTC (permalink / raw)
To: Arnd Bergmann, Segher Boessenkool
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a1GQSyCj1L8fFG4Pah8dr5Lanw=1yuimX1o+53ARzOX+Q@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> writes:
> On Mon, Jul 29, 2019 at 11:52 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>> On Mon, Jul 29, 2019 at 01:32:46PM -0700, Nathan Chancellor wrote:
>> > For the record:
>> >
>> > https://godbolt.org/z/z57VU7
>> >
>> > This seems consistent with what Michael found so I don't think a revert
>> > is entirely unreasonable.
>>
>> Try this:
>>
>> https://godbolt.org/z/6_ZfVi
>>
>> This matters in non-trivial loops, for example. But all current cases
>> where such non-trivial loops are done with cache block instructions are
>> actually written in real assembler already, using two registers.
>> Because performance matters. Not that I recommend writing code as
>> critical as memset in C with inline asm :-)
>
> Upon a second look, I think the issue is that the "Z" is an input argument
> when it should be an output. clang decides that it can make a copy of the
> input and pass that into the inline asm. This is not the most efficient
> way, but it seems entirely correct according to the constraints.
>
> Changing it to an output "=Z" constraint seems to make it work:
>
> https://godbolt.org/z/FwEqHf
>
> Clang still doesn't use the optimum form, but it passes the correct pointer.
Thanks Arnd. This seems like a better solution.
I'll drop the revert I have staged.
Segher does this look OK to you?
Nathan/Nick, are one of you able to test this with your clang CI?
cheers
^ permalink raw reply
* Re: [PATCH v5 2/4] powerpc/pseries: Update SCM hcall op-codes in hvcall.h
From: Michael Ellerman @ 2019-07-30 11:26 UTC (permalink / raw)
To: David Gibson, Vaibhav Jain
Cc: Oliver O'Halloran, Laurent Dufour, linuxppc-dev,
Aneesh Kumar K . V
In-Reply-To: <20190726085300.GA4865@umbus>
David Gibson <david@gibson.dropbear.id.au> writes:
> On Tue, Jul 23, 2019 at 09:43:55PM +0530, Vaibhav Jain wrote:
>> Update the hvcalls.h to include op-codes for new hcalls introduce to
>> manage SCM memory. Also update existing hcall definitions to reflect
>> current papr specification for SCM.
>>
>> The removed hcall op-codes H_SCM_MEM_QUERY, H_SCM_BLOCK_CLEAR were
>> transient proposals and there support was never implemented by
>> Power-VM nor they were used anywhere in Linux kernel. Hence we don't
>> expect anyone to be impacted by this change.
>>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>
> They really should not have been merged while only interim proposals.
> But since they have changed, better to update them than not, obviously.
Yes, absolutely agree. It wasn't clear when I merged them that they were
*unpublished* and *unstable*.
Unfortunately we can't realistically wait for these APIs to be in a
published PAPR spec because it seems to take about 5 years for a PAPR
spec to escape the IBM firewall.
cheers
^ permalink raw reply
* Re: [DOC][PATCH v5 1/4] powerpc: Document some HCalls for Storage Class Memory
From: Michael Ellerman @ 2019-07-30 12:06 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Laurent Dufour, Oliver O'Halloran,
Aneesh Kumar K . V, David Gibson
In-Reply-To: <20190723161357.26718-2-vaibhav@linux.ibm.com>
Hi Vaibhav,
Thanks for writing this documentation.
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> This doc patch provides an initial description of the HCall op-codes
> that are used by Linux kernel running as a guest operating
> system (LPAR) on top of PowerVM or any other sPAPR compliant
> hyper-visor (e.g qemu).
>
> Apart from documenting the HCalls the doc-patch also provides a
> rudimentary overview of how Hcalls are implemented inside the Linux
I prefer "hcall" rather than "HCall", "Hcall" or "HCALL".
> kernel and how information flows between kernel and PowerVM/KVM.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log:
>
> v5
> * First patch in this patchset.
> ---
> Documentation/powerpc/hcalls.txt | 140 +++++++++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
> create mode 100644 Documentation/powerpc/hcalls.txt
During this merge window all the existing documentation in there has
been converted to rst format. I suspect Jon will be annoyed at us if we
immediately start adding new plain text docs.
So we need to make this an rst file and do at least a minimal job of
making it valid ReST.
A while back I worked out how to actually build the rst docs, I'll try
and find my notes on that.
> diff --git a/Documentation/powerpc/hcalls.txt b/Documentation/powerpc/hcalls.txt
> new file mode 100644
> index 000000000000..cc9dd872cecd
> --- /dev/null
> +++ b/Documentation/powerpc/hcalls.txt
Can we call it papr_hcalls please.
> @@ -0,0 +1,140 @@
> +Hyper-visor Call Op-codes (HCALLS)
> +====================================
I'm not sure if "hyper-visor" is correct, but regardless that's not what
we commonly use, so please just spell it "hypervisor". And similarly for
"opcode".
> +Overview
> +=========
> +
> +Virtualization on PPC64 arch is based on the PAPR specification[1] which
We should probably say on "64-bit Power Book3S platforms".
Also the link you provide is to the "LoPAPR" specification. Which is not
quite the same as "PAPR", which used to be released via power.org, or
"PAPR+" which is the IBM internal version.
> +describes run-time environment for a guest operating system and how it should
> +interact with the hyper-visor for privileged operations. Currently there are two
> +PAPR compliant hypervisors (PHYP):
"PHYP" is only used as a another name for PowerVM (or part of PowerVM),
not as a generic term for a PAPR hypervisor.
I also don't think it's accurate to say Qemu/KVM is a "PAPR compliant"
hypervisor, it just implements (some of) the relevant parts of LoPAPR to
support Linux guests.
> +IBM PowerVM: IBM's proprietary hyper-visor that supports AIX, IBM-i and Linux as
> + supported guests (termed as Logical Partitions or LPARS).
> +
> +Qemu/KVM: Supports PPC64 linux guests running on a PPC64 linux host.
And actually other hosts via Qemu TCG but that's a bit of a tangent.
> +On PPC64 arch a virtualized guest kernel runs in a non-privileged mode (HV=0).
"virtualized guest" is redundant, just "guest" is sufficient IMHO.
I know what you're trying to say there, but it actually contradicts the
language that's used in the ISA.
If you look at Chapter 1 of Book III it defines these terms:
* hypervisor privileged
A term used to describe an instruction or facility that is available
only when the thread is in hypervisor state.
* privileged state and supervisor mode
Used interchangeably to refer to a state in which privileged
facilities are available.
* problem state and user mode
Used interchangeably to refer to a state in which privileged
facilities are not available.
So you might want to write it more like:
When running under a PAPR hypervisor the guest kernel runs in
supervisor mode (HV=0), and must issue hypercalls to the hypervisor
whenever it needs to perform an action that is hypervisor privileged or
for other services managed by the hypervisor.
> +Hence to perform a privileged operations the guest issues a Hyper-visor
> +Call (HCALL) with necessary input operands. PHYP after performing the privilege
> +operation returns a status code and output operands back to the guest.
> +
> +HCALL ABI
> +=========
> +The ABI specification for a HCall between guest os kernel and PHYP is
^
"OS", but probably just
"guest kernel" is fine.
> +described in [1]. The Opcode for Hcall is set in R3 and subsequent in-arguments
Where in [1] ?
It's more common to spell the GPRs with lower case 'r', eg. 'r3'.
> +for the Hcall are provided in registers R4-R12. On return from 'HVCS'
^the 'HVSC'
> +instruction the status code of HCall is available in R3 an the output parameters
^ ^ ^
return value the and
> +are returned in registers R4-R12.
> +
> +Powerpc arch code provides convenient wrappers named plpar_hcall_xxx defined in
> +header 'hvcall.h' to issue HCalls from the linux kernel running as guest.
> +
> +
> +DRC & DRC Indexes
> +=================
> +
> + PAPR Guest
> + DR1 Hypervisor OS
> + +--+ +----------+ +---------+
> + | |<------>| | | User |
> + +--+ DRC1 | | DRC | Space |
> + | | Index +---------+
> + DR2 | | | |
> + +--+ | |<------->| Kernel |
> + | |<----- >| | HCall | |
> + +--+ DRC2 +----------+ +---------+
> +
> +PHYP terms shared hardware resources like PCI devices, NVDimms etc available for
DIMM is an acronym too, so it should be either nvdimm or NVDIMM IMHO.
> +use by LPARs as Dynamic Resource (DR). When a DR is allocated to an LPAR, PHYP
> +creates a data-structure called Dynamic Resource Connector (DRC) to manage LPAR
> +access. An LPAR refers to a DRC via an opaque 32-bit number called DRC-Index.
> +The DRC-index value is provided to the LPAR via device-tree where its present
> +as an attribute in the device tree node associated with the DR.
> +
> +HCALL Op-codes
> +==============
> +
> +Below is a partial of of HCALLs that are supported by PHYP. For the
> +corresponding opcode values please look into the header
> +'arch/powerpc/include/asm/hvcall.h' :
> +
> +* H_SCM_READ_METADATA:
> + Input: drcIndex, offset, buffer-address, numBytesToRead
> + Out: None
> + Description:
> + Given a DRC Index of an NVDimm, read N-bytes from the the meta data area
> + associated with it, at a specified offset and copy it to provided buffer.
> + The metadata area stores configuration information such as label information,
> + bad-blocks etc. The metadata area is located out-of-band of NVDimm storage
> + area hence a separate access semantics is provided.
Can you also document the possible return values? (If they are defined).
> +* H_SCM_WRITE_METADATA:
> + Input: drcIndex, offset, data, numBytesToWrite
> + Out: None
> + Description:
> + Given a DRC Index of an NVDimm, write N-bytes from provided buffer at the
> + given offset to the the meta data area associated with the NVDimm.
> +
> +
> +* H_SCM_BIND_MEM:
> + Input: drcIndex, startingScmBlockIndex, numScmBlocksToBind, targetAddress
> + Out: guestMappedAddress, numScmBlockBound
> + Description:
> + Given a DRC-Index of an NVDimm, maps the SCM (Storage Class Memory) blocks to
> + continuous logical addresses in guest physical address space. The HCALL
> + arguments can be used to map partial range of SCM blocks instead of entire
> + NVDimm range to the LPAR.
What address space do targetAddress and guestMappedAddress exist in?
What are these blocks and how do we find out about them? ie. how do I
know what a valid block index is.
> +* H_SCM_UNBIND_MEM:
> + Input: drcIndex, startingScmLogicalMemoryAddress, numScmBlocksToUnbind
Similar question for startingScmLogicalMemoryAddress.
> + Out: numScmBlocksUnbound
> + Description:
> + Given a DRC-Index of an NVDimm, unmap one or more the SCM blocks from guest
^
of
> + physical address space. The HCALL can fail if the Guest has an active PTE
> + entry to the SCM block being unbinded.
unbound
> +* H_SCM_QUERY_BLOCK_MEM_BINDING:
> + Input: drcIndex, scmBlockIndex
> + Out: Guest-Physical-Address
> + Description:
> + Given a DRC-Index and an SCM Block index return the guest physical address to
> + which the SCM block is mapped to.
> +
> +* H_SCM_QUERY_LOGICAL_MEM_BINDING:
> + Input: Guest-Physical-Address
> + Out: drcIndex, scmBlockIndex
> + Description:
> + Given a guest physical address return which DRC Index and SCM block is mapped
> + to that address.
> +
> +* H_SCM_UNBIND_ALL:
> + Input: scmTargetScope, drcIndex
> + Out: None
> + Description:
> + Depending on the Target scope unmap all scm blocks belonging to all NVDimms
^
SCM for consistency.
> + or all scm blocks belonging to a single NVDimm identified by its drcIndex
> + from the LPAR memory.
> +
> +* H_SCM_HEALTH:
> + Input: drcIndex
> + Output: health-bitmap, health-bit-valid-bitmap
> + Description:
> + Given a DRC Index return the info on predictive failure and over all health of
overall
> + the NVDimm. The asserted bits in the health-bitmap indicate a single predictive
> + failure and health-bit-valid-bitmap indicate which bits in health-bitmap are
> + valid.
Presumably using bit endian bit ordering?
> +* H_SCM_PERFORMANCE_STATS:
> + Input: drcIndex, resultBuffer Addr
> + Out: None
> + Description:
> + Given a DRC Index collect the performance statistics for NVDimm and copy them
> + to the resultBuffer.
> +
> +
> +References
> +==========
> +[1]: "Linux on Power Architecture Platform Reference"
> + https://members.openpowerfoundation.org/document/dl/469
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Segher Boessenkool @ 2019-07-30 13:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a1GQSyCj1L8fFG4Pah8dr5Lanw=1yuimX1o+53ARzOX+Q@mail.gmail.com>
On Tue, Jul 30, 2019 at 09:34:28AM +0200, Arnd Bergmann wrote:
> Upon a second look, I think the issue is that the "Z" is an input argument
> when it should be an output. clang decides that it can make a copy of the
> input and pass that into the inline asm. This is not the most efficient
> way, but it seems entirely correct according to the constraints.
Most dcb* (and all icb*) do not change the memory pointed to. The
memory is an input here, logically as well, and that is obvious.
> Changing it to an output "=Z" constraint seems to make it work:
>
> https://godbolt.org/z/FwEqHf
>
> Clang still doesn't use the optimum form, but it passes the correct pointer.
As I said many times already, LLVM does not seem to treat all asm
operands as lvalues. That is a bug. And it is critical for memory
operands for example, as should be obvious if you look at at for a few
seconds (you pass *that* memory, not a copy of it). The thing you pass
has an identity. It's an lvalue. This is true for *all* inline asm
operands, not just output operands and memory operands, but it is most
obvious there.
Or, LLVM might have a bug elsewhere.
Either way, the asm is fine, and it has worked fine in GCC since
forever. Changing this constraint to be an output constraint would
just be obfuscation (we could change *all* operands to *everything* to
be inout ("+") constraints, and it won't affect correctness, just the
reader's sanity).
Segher
^ permalink raw reply
* [PATCH] powerpc/spe: Mark expected switch fall-throughs
From: Michael Ellerman @ 2019-07-30 14:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: gustavo
Mark switch cases where we are expecting to fall through.
Fixes errors such as below, seen with mpc85xx_defconfig:
arch/powerpc/kernel/align.c: In function 'emulate_spe':
arch/powerpc/kernel/align.c:178:8: error: this statement may fall through
ret |= __get_user_inatomic(temp.v[3], p++);
^~
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/align.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 7107ad86de65..92045ed64976 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -176,9 +176,11 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
ret |= __get_user_inatomic(temp.v[1], p++);
ret |= __get_user_inatomic(temp.v[2], p++);
ret |= __get_user_inatomic(temp.v[3], p++);
+ /* fall through */
case 4:
ret |= __get_user_inatomic(temp.v[4], p++);
ret |= __get_user_inatomic(temp.v[5], p++);
+ /* fall through */
case 2:
ret |= __get_user_inatomic(temp.v[6], p++);
ret |= __get_user_inatomic(temp.v[7], p++);
@@ -259,9 +261,11 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
ret |= __put_user_inatomic(data.v[1], p++);
ret |= __put_user_inatomic(data.v[2], p++);
ret |= __put_user_inatomic(data.v[3], p++);
+ /* fall through */
case 4:
ret |= __put_user_inatomic(data.v[4], p++);
ret |= __put_user_inatomic(data.v[5], p++);
+ /* fall through */
case 2:
ret |= __put_user_inatomic(data.v[6], p++);
ret |= __put_user_inatomic(data.v[7], p++);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] drivers/macintosh/smu.c: Mark expected switch fall-through
From: Michael Ellerman @ 2019-07-30 14:28 UTC (permalink / raw)
To: Stephen Rothwell, Benjamin Herrenschmidt
Cc: Linux kernel Mailing List, PowerPC, Kees Cook,
Gustavo A. R. Silva
In-Reply-To: <20190730143704.060a2606@canb.auug.org.au>
Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Mark switch cases where we are expecting to fall through.
>
> This patch fixes the following warning (Building: powerpc):
>
> drivers/macintosh/smu.c: In function 'smu_queue_i2c':
> drivers/macintosh/smu.c:854:21: warning: this statement may fall through [-Wimplicit-fallthrough=]
> cmd->info.devaddr &= 0xfe;
> ~~~~~~~~~~~~~~~~~~^~~~~~~
> drivers/macintosh/smu.c:855:2: note: here
> case SMU_I2C_TRANSFER_STDSUB:
> ^~~~
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> drivers/macintosh/smu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/macintosh/smu.c b/drivers/macintosh/smu.c
> index 276065c888bc..23f1f41c8602 100644
> --- a/drivers/macintosh/smu.c
> +++ b/drivers/macintosh/smu.c
> @@ -852,6 +852,7 @@ int smu_queue_i2c(struct smu_i2c_cmd *cmd)
> break;
> case SMU_I2C_TRANSFER_COMBINED:
> cmd->info.devaddr &= 0xfe;
> + /* fall through */
> case SMU_I2C_TRANSFER_STDSUB:
> if (cmd->info.sublen > 3)
> return -EINVAL;
Why do we think it's an expected fall through? I can't really convince
myself from the surrounding code that it's definitely intentional.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Arnd Bergmann @ 2019-07-30 14:30 UTC (permalink / raw)
To: Segher Boessenkool
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <20190730134856.GO31406@gate.crashing.org>
On Tue, Jul 30, 2019 at 3:49 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, Jul 30, 2019 at 09:34:28AM +0200, Arnd Bergmann wrote:
> > Upon a second look, I think the issue is that the "Z" is an input argument
> > when it should be an output. clang decides that it can make a copy of the
> > input and pass that into the inline asm. This is not the most efficient
> > way, but it seems entirely correct according to the constraints.
>
> Most dcb* (and all icb*) do not change the memory pointed to. The
> memory is an input here, logically as well, and that is obvious.
Ah, right. I had only thought of dcbz here, but you are right that using
an output makes little sense for the others.
readl() is another example where powerpc currently uses "Z" for an
input, which illustrates this even better.
> > Changing it to an output "=Z" constraint seems to make it work:
> >
> > https://godbolt.org/z/FwEqHf
> >
> > Clang still doesn't use the optimum form, but it passes the correct pointer.
>
> As I said many times already, LLVM does not seem to treat all asm
> operands as lvalues. That is a bug. And it is critical for memory
> operands for example, as should be obvious if you look at at for a few
> seconds (you pass *that* memory, not a copy of it). The thing you pass
> has an identity. It's an lvalue. This is true for *all* inline asm
> operands, not just output operands and memory operands, but it is most
> obvious there.
From experimentation, I would guess that llvm handles "m" correctly, but
not "Z". See https://godbolt.org/z/uqfDx_ for another example.
> Or, LLVM might have a bug elsewhere.
>
> Either way, the asm is fine, and it has worked fine in GCC since
> forever. Changing this constraint to be an output constraint would
> just be obfuscation (we could change *all* operands to *everything* to
> be inout ("+") constraints, and it won't affect correctness, just the
> reader's sanity).
I would still argue that for dcbz specifically, an output makes more
sense than an input, but as you say that does not solve the others.
Arnd
^ permalink raw reply
* Re: [PATCH] drivers/macintosh/smu.c: Mark expected switch fall-through
From: Stephen Rothwell @ 2019-07-30 14:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: PowerPC, Kees Cook, Linux kernel Mailing List,
Gustavo A. R. Silva
In-Reply-To: <878ssfzjdk.fsf@concordia.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 423 bytes --]
Hi Michael,
On Wed, 31 Jul 2019 00:28:55 +1000 Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Why do we think it's an expected fall through? I can't really convince
> myself from the surrounding code that it's definitely intentional.
Its been that way since this code was introduced by commit
0365ba7fb1fa ("[PATCH] ppc64: SMU driver update & i2c support")
in 2005 ...
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Segher Boessenkool @ 2019-07-30 16:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <CAK8P3a2755_6xq453C2AePLW8BeQk_Jg=HfjB_F-zyVMnQDfdg@mail.gmail.com>
On Tue, Jul 30, 2019 at 04:30:29PM +0200, Arnd Bergmann wrote:
> On Tue, Jul 30, 2019 at 3:49 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 09:34:28AM +0200, Arnd Bergmann wrote:
> > > Upon a second look, I think the issue is that the "Z" is an input argument
> > > when it should be an output. clang decides that it can make a copy of the
> > > input and pass that into the inline asm. This is not the most efficient
> > > way, but it seems entirely correct according to the constraints.
> >
> > Most dcb* (and all icb*) do not change the memory pointed to. The
> > memory is an input here, logically as well, and that is obvious.
>
> Ah, right. I had only thought of dcbz here, but you are right that using
> an output makes little sense for the others.
>
> readl() is another example where powerpc currently uses "Z" for an
> input, which illustrates this even better.
in_le32 and friends? Yeah, huh. If LLVM copies that to the stack as
well, its (not byte reversing) read will be atomic just fine, so things
will still work correctly.
The things defined with DEF_MMIO_IN_D (instead of DEF_MMIO_IN_X) do not
look like they will work correctly if an update form address is chosen,
but that won't happen because the constraint is "m" instead of "m<>",
making the %Un pretty useless (it will always be the empty string).
> > As I said many times already, LLVM does not seem to treat all asm
> > operands as lvalues. That is a bug. And it is critical for memory
> > operands for example, as should be obvious if you look at at for a few
> > seconds (you pass *that* memory, not a copy of it). The thing you pass
> > has an identity. It's an lvalue. This is true for *all* inline asm
> > operands, not just output operands and memory operands, but it is most
> > obvious there.
>
> >From experimentation, I would guess that llvm handles "m" correctly, but
> not "Z". See https://godbolt.org/z/uqfDx_ for another example.
Yeah, it does not treat "Z" as a memory constraint apparently, and it
special cases output operands and memory operands to be lvalues, but
does not do that for everything else as it should.
> > Or, LLVM might have a bug elsewhere.
> >
> > Either way, the asm is fine, and it has worked fine in GCC since
> > forever. Changing this constraint to be an output constraint would
> > just be obfuscation (we could change *all* operands to *everything* to
> > be inout ("+") constraints, and it won't affect correctness, just the
> > reader's sanity).
>
> I would still argue that for dcbz specifically, an output makes more
> sense than an input, but as you say that does not solve the others.
An output would be somewhat misleading. dcbz zeroes the whole aligned
cache block sized region of memory its operand points into. The kernel
dcbz functions do not easily know the cache block size I think, and
besides, you want a "memory" clobber anyway, also for the other dcb*,
so it won't help anything. Also, the compiler can almost never use the
extra info ("affects the aligned 32B or 128B block this points into")
usefully anyway; it will usually see it as "can alias pretty much
anything". Just use a "memory" clobber :-/
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Segher Boessenkool @ 2019-07-30 17:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <20190730161637.GP31406@gate.crashing.org>
On Tue, Jul 30, 2019 at 11:16:37AM -0500, Segher Boessenkool wrote:
> in_le32 and friends? Yeah, huh. If LLVM copies that to the stack as
> well, its (not byte reversing) read will be atomic just fine, so things
> will still work correctly.
>
> The things defined with DEF_MMIO_IN_D (instead of DEF_MMIO_IN_X) do not
> look like they will work correctly if an update form address is chosen,
> but that won't happen because the constraint is "m" instead of "m<>",
> making the %Un pretty useless (it will always be the empty string).
Btw, this is true since GCC 4.8; before 4.8, plain "m" *could* have an
automodify (autoinc, autodec, etc.) side effect. What is the minimum
GCC version required, these days?
https://gcc.gnu.org/PR44492
https://gcc.gnu.org/r161328
Segher
^ permalink raw reply
* Re: [PATCH] drivers/macintosh/smu.c: Mark expected switch fall-through
From: Kees Cook @ 2019-07-30 17:07 UTC (permalink / raw)
To: Michael Ellerman
Cc: Stephen Rothwell, Gustavo A. R. Silva, PowerPC,
Linux kernel Mailing List
In-Reply-To: <878ssfzjdk.fsf@concordia.ellerman.id.au>
On Wed, Jul 31, 2019 at 12:28:55AM +1000, Michael Ellerman wrote:
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
> > Mark switch cases where we are expecting to fall through.
> >
> > This patch fixes the following warning (Building: powerpc):
> >
> > drivers/macintosh/smu.c: In function 'smu_queue_i2c':
> > drivers/macintosh/smu.c:854:21: warning: this statement may fall through [-Wimplicit-fallthrough=]
> > cmd->info.devaddr &= 0xfe;
> > ~~~~~~~~~~~~~~~~~~^~~~~~~
> > drivers/macintosh/smu.c:855:2: note: here
> > case SMU_I2C_TRANSFER_STDSUB:
> > ^~~~
> >
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > ---
> > drivers/macintosh/smu.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/macintosh/smu.c b/drivers/macintosh/smu.c
> > index 276065c888bc..23f1f41c8602 100644
> > --- a/drivers/macintosh/smu.c
> > +++ b/drivers/macintosh/smu.c
> > @@ -852,6 +852,7 @@ int smu_queue_i2c(struct smu_i2c_cmd *cmd)
> > break;
> > case SMU_I2C_TRANSFER_COMBINED:
> > cmd->info.devaddr &= 0xfe;
> > + /* fall through */
> > case SMU_I2C_TRANSFER_STDSUB:
> > if (cmd->info.sublen > 3)
> > return -EINVAL;
>
> Why do we think it's an expected fall through? I can't really convince
> myself from the surrounding code that it's definitely intentional.
Yeah, good question. Just now when I went looking for who
used SMU_I2C_TRANSFER_COMBINED, I found the only caller in
arch/powerpc/platforms/powermac/low_i2c.c and it is clearly using a
fall-through for building the command for "stdsub" and "combined",
so I think that's justification enough:
switch(bus->mode) {
case pmac_i2c_mode_std:
if (subsize != 0)
return -EINVAL;
cmd->info.type = SMU_I2C_TRANSFER_SIMPLE;
break;
case pmac_i2c_mode_stdsub:
case pmac_i2c_mode_combined:
if (subsize > 3 || subsize < 1)
return -EINVAL;
cmd->info.sublen = subsize;
/* that's big-endian only but heh ! */
memcpy(&cmd->info.subaddr, ((char *)&subaddr) + (4 - subsize),
subsize);
if (bus->mode == pmac_i2c_mode_stdsub)
cmd->info.type = SMU_I2C_TRANSFER_STDSUB;
else
cmd->info.type = SMU_I2C_TRANSFER_COMBINED;
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Hoan Tran OS @ 2019-07-30 17:31 UTC (permalink / raw)
To: Michal Hocko
Cc: Heiko Carstens, open list:MEMORY MANAGEMENT, Paul Mackerras,
H . Peter Anvin, sparclinux@vger.kernel.org, Alexander Duyck,
Will Deacon, linux-s390@vger.kernel.org, x86@kernel.org,
willy@infradead.org, Mike Rapoport, Christian Borntraeger,
Ingo Molnar, linux-arm-kernel@lists.infradead.org,
Catalin Marinas, Open Source Submission, Pavel Tatashin,
Vasily Gorbik, Will Deacon, Borislav Petkov, Thomas Gleixner,
Vlastimil Babka, Oscar Salvador, linux-kernel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <20190730081415.GN9330@dhcp22.suse.cz>
Hi,
On 7/30/19 1:14 AM, Michal Hocko wrote:
> [Sorry for a late reply]
>
> On Mon 15-07-19 17:55:07, Hoan Tran OS wrote:
>> Hi,
>>
>> On 7/12/19 10:00 PM, Michal Hocko wrote:
> [...]
>>> Hmm, I thought this was selectable. But I am obviously wrong here.
>>> Looking more closely, it seems that this is indeed only about
>>> __early_pfn_to_nid and as such not something that should add a config
>>> symbol. This should have been called out in the changelog though.
>>
>> Yes, do you have any other comments about my patch?
>
> Not really. Just make sure to explicitly state that
> CONFIG_NODES_SPAN_OTHER_NODES is only about __early_pfn_to_nid and that
> doesn't really deserve it's own config and can be pulled under NUMA.
Yes, I will add this info into the patch description.
>
>>> Also while at it, does HAVE_MEMBLOCK_NODE_MAP fall into a similar
>>> bucket? Do we have any NUMA architecture that doesn't enable it?
>>>
>>
>> As I checked with arch Kconfig files, there are 2 architectures, riscv
>> and microblaze, do not support NUMA but enable this config.
>>
>> And 1 architecture, alpha, supports NUMA but does not enable this config.
>
> Care to have a look and clean this up please?
Sure, I'll take a look.
Thanks
Hoan
>
^ permalink raw reply
* Re: [PATCH] powerpc: workaround clang codegen bug in dcbz
From: Arnd Bergmann @ 2019-07-30 18:24 UTC (permalink / raw)
To: Segher Boessenkool
Cc: kbuild test robot, Nick Desaulniers, Linux Kernel Mailing List,
clang-built-linux, Paul Mackerras, Nathan Chancellor,
linuxppc-dev
In-Reply-To: <20190730161637.GP31406@gate.crashing.org>
On Tue, Jul 30, 2019 at 6:16 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, Jul 30, 2019 at 04:30:29PM +0200, Arnd Bergmann wrote:
> > On Tue, Jul 30, 2019 at 3:49 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > >
> > > On Tue, Jul 30, 2019 at 09:34:28AM +0200, Arnd Bergmann wrote:
> > > > Upon a second look, I think the issue is that the "Z" is an input argument
> > > > when it should be an output. clang decides that it can make a copy of the
> > > > input and pass that into the inline asm. This is not the most efficient
> > > > way, but it seems entirely correct according to the constraints.
> > >
> > > Most dcb* (and all icb*) do not change the memory pointed to. The
> > > memory is an input here, logically as well, and that is obvious.
> >
> > Ah, right. I had only thought of dcbz here, but you are right that using
> > an output makes little sense for the others.
> >
> > readl() is another example where powerpc currently uses "Z" for an
> > input, which illustrates this even better.
>
> in_le32 and friends? Yeah, huh. If LLVM copies that to the stack as
> well, its (not byte reversing) read will be atomic just fine, so things
> will still work correctly.
byteorder is fine, the problem I was thinking of is when moving the load/store
instructions around the barriers that synchronize with DMA, or turning
them into different-size accesses. Changing two consecutive 16-bit mmio reads
into an unaligned 32-bit read will rarely have the intended effect ;-)
Arnd
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox