* [PATCH v1 24/27] powerpc: define subarch SLB_ADDR_LIMIT_DEFAULT
From: Christophe Leroy @ 2019-03-20 10:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1553076180.git.christophe.leroy@c-s.fr>
This patch defines a subarch specific SLB_ADDR_LIMIT_DEFAULT
to remove the #ifdefs around the setup of mm->context.slb_addr_limit
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/64/slice.h | 2 ++
arch/powerpc/include/asm/nohash/32/slice.h | 2 ++
arch/powerpc/kernel/setup-common.c | 8 +-------
arch/powerpc/mm/slice.c | 6 +-----
4 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/slice.h b/arch/powerpc/include/asm/book3s/64/slice.h
index af498b0da21a..8da15958dcd1 100644
--- a/arch/powerpc/include/asm/book3s/64/slice.h
+++ b/arch/powerpc/include/asm/book3s/64/slice.h
@@ -13,6 +13,8 @@
#define SLICE_NUM_HIGH (H_PGTABLE_RANGE >> SLICE_HIGH_SHIFT)
#define GET_HIGH_SLICE_INDEX(addr) ((addr) >> SLICE_HIGH_SHIFT)
+#define SLB_ADDR_LIMIT_DEFAULT DEFAULT_MAP_WINDOW_USER64
+
#else /* CONFIG_PPC_MM_SLICES */
#define get_slice_psize(mm, addr) ((mm)->context.user_psize)
diff --git a/arch/powerpc/include/asm/nohash/32/slice.h b/arch/powerpc/include/asm/nohash/32/slice.h
index 777d62e40ac0..39eb0154ae2d 100644
--- a/arch/powerpc/include/asm/nohash/32/slice.h
+++ b/arch/powerpc/include/asm/nohash/32/slice.h
@@ -13,6 +13,8 @@
#define SLICE_NUM_HIGH 0ul
#define GET_HIGH_SLICE_INDEX(addr) (addr & 0)
+#define SLB_ADDR_LIMIT_DEFAULT DEFAULT_MAP_WINDOW
+
#endif /* CONFIG_PPC_MM_SLICES */
#endif /* _ASM_POWERPC_NOHASH_32_SLICE_H */
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 2e5dfb6e0823..af2682d052a2 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -948,14 +948,8 @@ void __init setup_arch(char **cmdline_p)
init_mm.brk = klimit;
#ifdef CONFIG_PPC_MM_SLICES
-#ifdef CONFIG_PPC64
if (!radix_enabled())
- init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW_USER64;
-#elif defined(CONFIG_PPC_8xx)
- init_mm.context.slb_addr_limit = DEFAULT_MAP_WINDOW;
-#else
-#error "context.addr_limit not initialized."
-#endif
+ init_mm.context.slb_addr_limit = SLB_ADDR_LIMIT_DEFAULT;
#endif
#ifdef CONFIG_SPAPR_TCE_IOMMU
diff --git a/arch/powerpc/mm/slice.c b/arch/powerpc/mm/slice.c
index 50b1a5528384..64513cf47e5b 100644
--- a/arch/powerpc/mm/slice.c
+++ b/arch/powerpc/mm/slice.c
@@ -652,11 +652,7 @@ void slice_init_new_context_exec(struct mm_struct *mm)
* case of fork it is just inherited from the mm being
* duplicated.
*/
-#ifdef CONFIG_PPC64
- mm->context.slb_addr_limit = DEFAULT_MAP_WINDOW_USER64;
-#else
- mm->context.slb_addr_limit = DEFAULT_MAP_WINDOW;
-#endif
+ mm->context.slb_addr_limit = SLB_ADDR_LIMIT_DEFAULT;
mm->context.user_psize = psize;
--
2.13.3
^ permalink raw reply related
* [PATCH v1 25/27] powerpc/mm: flatten function __find_linux_pte()
From: Christophe Leroy @ 2019-03-20 10:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1553076180.git.christophe.leroy@c-s.fr>
__find_linux_pte() is full of if/else which is hard to
follow allthough the handling is pretty simple.
This patch flattens the function by getting rid of as much if/else
as possible. In order to ease the review, this is done in two steps.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/pgtable.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 9f4ccd15849f..d332abeedf0a 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -339,12 +339,16 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
*/
if (pgd_none(pgd))
return NULL;
- else if (pgd_huge(pgd)) {
- ret_pte = (pte_t *) pgdp;
+
+ if (pgd_huge(pgd)) {
+ ret_pte = (pte_t *)pgdp;
goto out;
- } else if (is_hugepd(__hugepd(pgd_val(pgd))))
+ }
+ if (is_hugepd(__hugepd(pgd_val(pgd)))) {
hpdp = (hugepd_t *)&pgd;
- else {
+ goto out_huge;
+ }
+ {
/*
* Even if we end up with an unmap, the pgtable will not
* be freed, because we do an rcu free and here we are
@@ -356,12 +360,16 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
if (pud_none(pud))
return NULL;
- else if (pud_huge(pud)) {
+
+ if (pud_huge(pud)) {
ret_pte = (pte_t *) pudp;
goto out;
- } else if (is_hugepd(__hugepd(pud_val(pud))))
+ }
+ if (is_hugepd(__hugepd(pud_val(pud)))) {
hpdp = (hugepd_t *)&pud;
- else {
+ goto out_huge;
+ }
+ {
pdshift = PMD_SHIFT;
pmdp = pmd_offset(&pud, ea);
pmd = READ_ONCE(*pmdp);
@@ -386,12 +394,16 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
if (pmd_huge(pmd) || pmd_large(pmd)) {
ret_pte = (pte_t *) pmdp;
goto out;
- } else if (is_hugepd(__hugepd(pmd_val(pmd))))
+ }
+ if (is_hugepd(__hugepd(pmd_val(pmd)))) {
hpdp = (hugepd_t *)&pmd;
- else
- return pte_offset_kernel(&pmd, ea);
+ goto out_huge;
+ }
+
+ return pte_offset_kernel(&pmd, ea);
}
}
+out_huge:
if (!hpdp)
return NULL;
--
2.13.3
^ permalink raw reply related
* [PATCH v1 26/27] powerpc/mm: flatten function __find_linux_pte() step 2
From: Christophe Leroy @ 2019-03-20 10:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1553076180.git.christophe.leroy@c-s.fr>
__find_linux_pte() is full of if/else which is hard to
follow allthough the handling is pretty simple.
Previous patch left { } blocks. This patch removes the first one
by shifting its content to the left.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/pgtable.c | 62 +++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index d332abeedf0a..c1c6d0b79baa 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -369,39 +369,37 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
hpdp = (hugepd_t *)&pud;
goto out_huge;
}
- {
- pdshift = PMD_SHIFT;
- pmdp = pmd_offset(&pud, ea);
- pmd = READ_ONCE(*pmdp);
- /*
- * A hugepage collapse is captured by pmd_none, because
- * it mark the pmd none and do a hpte invalidate.
- */
- if (pmd_none(pmd))
- return NULL;
-
- if (pmd_trans_huge(pmd) || pmd_devmap(pmd)) {
- if (is_thp)
- *is_thp = true;
- ret_pte = (pte_t *) pmdp;
- goto out;
- }
- /*
- * pmd_large check below will handle the swap pmd pte
- * we need to do both the check because they are config
- * dependent.
- */
- if (pmd_huge(pmd) || pmd_large(pmd)) {
- ret_pte = (pte_t *) pmdp;
- goto out;
- }
- if (is_hugepd(__hugepd(pmd_val(pmd)))) {
- hpdp = (hugepd_t *)&pmd;
- goto out_huge;
- }
-
- return pte_offset_kernel(&pmd, ea);
+ pdshift = PMD_SHIFT;
+ pmdp = pmd_offset(&pud, ea);
+ pmd = READ_ONCE(*pmdp);
+ /*
+ * A hugepage collapse is captured by pmd_none, because
+ * it mark the pmd none and do a hpte invalidate.
+ */
+ if (pmd_none(pmd))
+ return NULL;
+
+ if (pmd_trans_huge(pmd) || pmd_devmap(pmd)) {
+ if (is_thp)
+ *is_thp = true;
+ ret_pte = (pte_t *)pmdp;
+ goto out;
+ }
+ /*
+ * pmd_large check below will handle the swap pmd pte
+ * we need to do both the check because they are config
+ * dependent.
+ */
+ if (pmd_huge(pmd) || pmd_large(pmd)) {
+ ret_pte = (pte_t *)pmdp;
+ goto out;
}
+ if (is_hugepd(__hugepd(pmd_val(pmd)))) {
+ hpdp = (hugepd_t *)&pmd;
+ goto out_huge;
+ }
+
+ return pte_offset_kernel(&pmd, ea);
}
out_huge:
if (!hpdp)
--
2.13.3
^ permalink raw reply related
* [PATCH v1 27/27] powerpc/mm: flatten function __find_linux_pte() step 3
From: Christophe Leroy @ 2019-03-20 10:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
aneesh.kumar
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1553076180.git.christophe.leroy@c-s.fr>
__find_linux_pte() is full of if/else which is hard to
follow allthough the handling is pretty simple.
Previous patches left a { } block. This patch removes it.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/pgtable.c | 98 +++++++++++++++++++++++------------------------
1 file changed, 49 insertions(+), 49 deletions(-)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index c1c6d0b79baa..db4a6253df92 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -348,59 +348,59 @@ pte_t *__find_linux_pte(pgd_t *pgdir, unsigned long ea,
hpdp = (hugepd_t *)&pgd;
goto out_huge;
}
- {
- /*
- * Even if we end up with an unmap, the pgtable will not
- * be freed, because we do an rcu free and here we are
- * irq disabled
- */
- pdshift = PUD_SHIFT;
- pudp = pud_offset(&pgd, ea);
- pud = READ_ONCE(*pudp);
- if (pud_none(pud))
- return NULL;
+ /*
+ * Even if we end up with an unmap, the pgtable will not
+ * be freed, because we do an rcu free and here we are
+ * irq disabled
+ */
+ pdshift = PUD_SHIFT;
+ pudp = pud_offset(&pgd, ea);
+ pud = READ_ONCE(*pudp);
- if (pud_huge(pud)) {
- ret_pte = (pte_t *) pudp;
- goto out;
- }
- if (is_hugepd(__hugepd(pud_val(pud)))) {
- hpdp = (hugepd_t *)&pud;
- goto out_huge;
- }
- pdshift = PMD_SHIFT;
- pmdp = pmd_offset(&pud, ea);
- pmd = READ_ONCE(*pmdp);
- /*
- * A hugepage collapse is captured by pmd_none, because
- * it mark the pmd none and do a hpte invalidate.
- */
- if (pmd_none(pmd))
- return NULL;
-
- if (pmd_trans_huge(pmd) || pmd_devmap(pmd)) {
- if (is_thp)
- *is_thp = true;
- ret_pte = (pte_t *)pmdp;
- goto out;
- }
- /*
- * pmd_large check below will handle the swap pmd pte
- * we need to do both the check because they are config
- * dependent.
- */
- if (pmd_huge(pmd) || pmd_large(pmd)) {
- ret_pte = (pte_t *)pmdp;
- goto out;
- }
- if (is_hugepd(__hugepd(pmd_val(pmd)))) {
- hpdp = (hugepd_t *)&pmd;
- goto out_huge;
- }
+ if (pud_none(pud))
+ return NULL;
- return pte_offset_kernel(&pmd, ea);
+ if (pud_huge(pud)) {
+ ret_pte = (pte_t *)pudp;
+ goto out;
}
+ if (is_hugepd(__hugepd(pud_val(pud)))) {
+ hpdp = (hugepd_t *)&pud;
+ goto out_huge;
+ }
+ pdshift = PMD_SHIFT;
+ pmdp = pmd_offset(&pud, ea);
+ pmd = READ_ONCE(*pmdp);
+ /*
+ * A hugepage collapse is captured by pmd_none, because
+ * it mark the pmd none and do a hpte invalidate.
+ */
+ if (pmd_none(pmd))
+ return NULL;
+
+ if (pmd_trans_huge(pmd) || pmd_devmap(pmd)) {
+ if (is_thp)
+ *is_thp = true;
+ ret_pte = (pte_t *)pmdp;
+ goto out;
+ }
+ /*
+ * pmd_large check below will handle the swap pmd pte
+ * we need to do both the check because they are config
+ * dependent.
+ */
+ if (pmd_huge(pmd) || pmd_large(pmd)) {
+ ret_pte = (pte_t *)pmdp;
+ goto out;
+ }
+ if (is_hugepd(__hugepd(pmd_val(pmd)))) {
+ hpdp = (hugepd_t *)&pmd;
+ goto out_huge;
+ }
+
+ return pte_offset_kernel(&pmd, ea);
+
out_huge:
if (!hpdp)
return NULL;
--
2.13.3
^ permalink raw reply related
* Re: [PATCH] compiler: allow all arches to enable CONFIG_OPTIMIZE_INLINING
From: Masahiro Yamada @ 2019-03-20 10:18 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arch, linux-s390, Dave Hansen, X86 ML, linux-mips,
Linux Kernel Mailing List, Paul Burton, Ingo Molnar, linux-mtd,
Andrew Morton, linuxppc-dev, linux-arm-kernel
In-Reply-To: <CAK8P3a0-7LRE4ByO2Uf4tb5mR49SbZzBu0AgHpS=03FAMWFLgQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]
Hi Arnd,
On Wed, Mar 20, 2019 at 6:39 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Mar 20, 2019 at 7:41 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>
> > It is unclear to me how to fix it.
> > That's why I ended up with "depends on !MIPS".
> >
> >
> > MODPOST vmlinux.o
> > arch/mips/mm/sc-mips.o: In function `mips_sc_prefetch_enable.part.2':
> > sc-mips.c:(.text+0x98): undefined reference to `mips_gcr_base'
> > sc-mips.c:(.text+0x9c): undefined reference to `mips_gcr_base'
> > sc-mips.c:(.text+0xbc): undefined reference to `mips_gcr_base'
> > sc-mips.c:(.text+0xc8): undefined reference to `mips_gcr_base'
> > sc-mips.c:(.text+0xdc): undefined reference to `mips_gcr_base'
> > arch/mips/mm/sc-mips.o:sc-mips.c:(.text.unlikely+0x44): more undefined
> > references to `mips_gcr_base'
> >
> >
> > Perhaps, MIPS folks may know how to fix it.
>
> I would guess like this:
>
> diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
> index 8bc5df49b0e1..a27483fedb7d 100644
> --- a/arch/mips/include/asm/mips-cm.h
> +++ b/arch/mips/include/asm/mips-cm.h
> @@ -79,7 +79,7 @@ static inline int mips_cm_probe(void)
> *
> * Returns true if a CM is present in the system, else false.
> */
> -static inline bool mips_cm_present(void)
> +static __always_inline bool mips_cm_present(void)
> {
> #ifdef CONFIG_MIPS_CM
> return mips_gcr_base != NULL;
> @@ -93,7 +93,7 @@ static inline bool mips_cm_present(void)
> *
> * Returns true if the system implements an L2-only sync region, else false.
> */
> -static inline bool mips_cm_has_l2sync(void)
> +static __always_inline bool mips_cm_has_l2sync(void)
> {
> #ifdef CONFIG_MIPS_CM
> return mips_cm_l2sync_base != NULL;
>
Thanks, I applied the above, but I still see
undefined reference to `mips_gcr_base'
I attached .config to produce this error.
I use prebuilt mips-linux-gcc from
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
--
Best Regards
Masahiro Yamada
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 13655 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v2] powerpc: mute unused-but-set-variable warnings
From: Michael Ellerman @ 2019-03-20 12:42 UTC (permalink / raw)
To: Qian Cai, Christophe Leroy, benh, paulus; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b3e7d016-74ca-aaaa-02e7-39fbc2200013@lca.pw>
Qian Cai <cai@lca.pw> writes:
> On 3/19/19 5:21 AM, Christophe Leroy wrote:
>> Is there a reason for resending ? AFAICS, both are identical and still marked
>> new in patchwork:
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=76055
>>
>
> "RESEND" because of no maintainer response for more than one week.
I don't know who told you to RESEND after a week, but especially at this
point in the development cycle a week is *way* too short.
And for trivial patches like this I may not get to them for several
weeks, I have other problems to fix like time going backward :)
In future please check patchwork and then if the patch is still new
after several weeks just send a ping in reply to that patch. A full
RESEND means I now have two identical patches to deal with in patchwork,
which makes more work for me.
cheers
^ permalink raw reply
* Re: Shift overflow warnings in arch/powerpc/boot/addnote.c on 32-bit builds
From: Michael Ellerman @ 2019-03-20 12:45 UTC (permalink / raw)
To: Mark Cave-Ayland, linuxppc-dev; +Cc: paulus
In-Reply-To: <c6ba8992-a3f3-2c99-6d6d-7ee9d41a22c5@ilande.co.uk>
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> Hi all,
>
> Whilst building the latest git master on my G4 I noticed the following shift overflow
> warnings in the build log for arch/powerpc/boot/addnote.c:
>
>
> arch/powerpc/boot/addnote.c: In function ‘main’:
> arch/powerpc/boot/addnote.c:75:47: warning: right shift count >= width of type
> [-Wshift-count-overflow]
> #define PUT_64BE(off, v)((PUT_32BE((off), (v) >> 32L), \
> ^~
> arch/powerpc/boot/addnote.c:72:39: note: in definition of macro ‘PUT_16BE’
> #define PUT_16BE(off, v)(buf[off] = ((v) >> 8) & 0xff, \
> ^
> arch/powerpc/boot/addnote.c:75:27: note: in expansion of macro ‘PUT_32BE’
> #define PUT_64BE(off, v)((PUT_32BE((off), (v) >> 32L), \
> ^~~~~~~~
> arch/powerpc/boot/addnote.c:94:50: note: in expansion of macro ‘PUT_64BE’
> #define PUT_64(off, v) (e_data == ELFDATA2MSB ? PUT_64BE(off, v) : \
> ^~~~~~~~
> arch/powerpc/boot/addnote.c:183:3: note: in expansion of macro ‘PUT_64’
> PUT_64(ph + PH_OFFSET, ns);
> ^~~~~~
I don't think there's any situation in which a 32-bit addnote will be
run against a 64-bit ELF is there?
So I don't think there's an actual bug, but it would be good if we could
make the warning go away.
cheers
^ permalink raw reply
* Re: [PATCH v3] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Michael Ellerman @ 2019-03-20 12:47 UTC (permalink / raw)
To: Laurent Vivier; +Cc: linuxppc-dev, linux-kernel, David Gibson
In-Reply-To: <663cdede-afd1-bafa-3979-e820214f029d@redhat.com>
Laurent Vivier <lvivier@redhat.com> writes:
> Hi Michael,
>
> as it seems good now, could you pick up this patch for merging?
I'll start picking up patches for next starting after rc2, so next week.
If you think it's a bug fix I can put it into fixes now, but I don't
think it's a bug fix is it?
cheers
^ permalink raw reply
* Re: Disable kcov for slb routines.
From: Michael Ellerman @ 2019-03-20 12:51 UTC (permalink / raw)
To: Mahesh Jagannath Salgaonkar, Michael Ellerman, linuxppc-dev
Cc: syzkaller, Paul Mackerras, Nicholas Piggin, Andrew Donnellan
In-Reply-To: <3e0bd37b-5e34-f056-250c-c36bd495ea94@linux.vnet.ibm.com>
Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
> On 3/14/19 5:13 PM, Michael Ellerman wrote:
>> On Mon, 2019-03-04 at 08:25:51 UTC, Mahesh J Salgaonkar wrote:
>>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>>
>>> The kcov instrumentation inside SLB routines causes duplicate SLB entries
>>> to be added resulting into SLB multihit machine checks.
>>> Disable kcov instrumentation on slb.o
>>>
>>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>> Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
>>> Tested-by: Satheesh Rajendran <sathnaga@linux.vent.ibm.com>
>>
>> Applied to powerpc next, thanks.
>>
>> https://git.kernel.org/powerpc/c/19d6907521b04206676741b26e05a152
>>
>> cheers
>>
>
> There was a v2 at http://patchwork.ozlabs.org/patch/1051718/, looks like
> v1 got picked up. But I see the applied commit does address Andrew's
> comments.
Sorry not sure how I missed v2.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Make some functions static
From: Michael Ellerman @ 2019-03-20 12:52 UTC (permalink / raw)
To: Mathieu Malaterre, Christophe Leroy; +Cc: linuxppc-dev, Paul Mackerras, LKML
In-Reply-To: <CA+7wUszPviAFVjyU-=qAJfOpe_NxmimWd2-tc148NLUEc4Oj0w@mail.gmail.com>
Mathieu Malaterre <malat@debian.org> writes:
> On Tue, Mar 12, 2019 at 10:14 PM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>>
>>
>> Le 12/03/2019 à 21:31, Mathieu Malaterre a écrit :
>> > In commit cb9e4d10c448 ("[POWERPC] Add support for 750CL Holly board")
>> > new functions were added. Since these functions can be made static,
>> > make it so. While doing so, it turns out that holly_power_off and
>> > holly_halt are unused, so remove them.
>>
>> I would have said 'since these functions are only used in this C file,
>> make them static'.
>>
>> I think this could be split in two patches:
>> 1/ Remove unused functions, ie holly_halt() and holly_power_off().
>> 2/ Make the other ones static.
>
> Michael do you want two patches ?
That would be better if it's not too much trouble. A patch with a title
of "Make some functions static" shouldn't really be deleting functions
entirely.
cheers
^ permalink raw reply
* [PATCH] powerpc/dts/fsl: add crypto node alias for B4
From: Horia Geantă @ 2019-03-20 12:55 UTC (permalink / raw)
To: Rob Herring, Mark Rutland
Cc: devicetree, Scott Wood, Paul Mackerras, linuxppc-dev
crypto node alias is needed by U-boot to identify the node and
perform fix-ups, like adding "fsl,sec-era" property.
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
arch/powerpc/boot/dts/fsl/b4qds.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/boot/dts/fsl/b4qds.dtsi b/arch/powerpc/boot/dts/fsl/b4qds.dtsi
index 999efd3bc167..05be919f3545 100644
--- a/arch/powerpc/boot/dts/fsl/b4qds.dtsi
+++ b/arch/powerpc/boot/dts/fsl/b4qds.dtsi
@@ -40,6 +40,7 @@
interrupt-parent = <&mpic>;
aliases {
+ crypto = &crypto;
phy_sgmii_10 = &phy_sgmii_10;
phy_sgmii_11 = &phy_sgmii_11;
phy_sgmii_1c = &phy_sgmii_1c;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
From: Michael Ellerman @ 2019-03-20 12:57 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
In-Reply-To: <b1d292b5-5844-34c6-c388-2ad2045e1904@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>
>> This patch implements a framework for Kernel Userspace Access
>> Protection.
>>
>> Then subarches will have the possibility to provide their own
>> implementation by providing setup_kuap() and
>> allow/prevent_user_access().
>>
>> Some platforms will need to know the area accessed and whether it is
>> accessed from read, write or both. Therefore source, destination and
>> size and handed over to the two functions.
>>
>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>> read/write wrappers. Drop the 32-bit code for now until we have an
>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> v5: Futex ops need read/write so use allow_user_acccess() there.
>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>> Allow subarch to override allow_read/write_from/to_user().
>
> Those little helpers that will just call allow_user_access() when
> distinct read/write handling is not performed looks overkill to me.
>
> Can't the subarch do it by itself based on the nullity of from/to ?
>
> static inline void allow_user_access(void __user *to, const void __user
> *from,
> unsigned long size)
> {
> if (to & from)
> set_kuap(0);
> else if (to)
> set_kuap(AMR_KUAP_BLOCK_READ);
> else if (from)
> set_kuap(AMR_KUAP_BLOCK_WRITE);
> }
You could implement it that way, but it reads better at the call sites
if we have:
allow_write_to_user(uaddr, sizeof(*uaddr));
vs:
allow_user_access(uaddr, NULL, sizeof(*uaddr));
So I'm inclined to keep them. It should all end up inlined and generate
the same code at the end of the day.
cheers
^ permalink raw reply
* Re: [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
From: Michael Ellerman @ 2019-03-20 12:58 UTC (permalink / raw)
To: Akshay Adiga; +Cc: linuxppc-dev, npiggin
In-Reply-To: <20190313081630.GB4047@aks.ibm>
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> writes:
> On Fri, Mar 08, 2019 at 12:16:11PM +1100, Michael Ellerman wrote:
>> In order to implement KUAP (Kernel Userspace Access Protection) on
>> Power9 we will be using the AMR, and therefore indirectly the
>> UAMOR/AMOR.
>>
>> So save/restore these regs in the idle code.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> v5: Unchanged.
>> v4: New.
>>
>> arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
>> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> Opps.. i posted a comment on the v4.
>
> It would be good if we can make AMOR/UAMOR/AMR save-restore
> code power9 only.
Yes that would be a good optimisation.
If you can send an incremental patch against this one I'll squash it in.
If not I'll try and get it done at some point before merging.
cheers
^ permalink raw reply
* Re: [v2, 01/10] powerpc/6xx: fix setup and use of SPRN_SPRG_PGDIR for hash32
From: Michael Ellerman @ 2019-03-20 13:04 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b348c3da010023d1c163ddd527aab6aba87f73ee.1552292207.git.christophe.leroy@c-s.fr>
On Mon, 2019-03-11 at 08:30:27 UTC, Christophe Leroy wrote:
> Not only the 603 but all 6xx need SPRN_SPRG_PGDIR to be initialised at
> startup. This patch move it from __setup_cpu_603() to start_here()
> and __secondary_start(), close to the initialisation of SPRN_THREAD.
>
> Previously, virt addr of PGDIR was retrieved from thread struct.
> Now that it is the phys addr which is stored in SPRN_SPRG_PGDIR,
> hash_page() shall not convert it to phys anymore.
> This patch removes the conversion.
>
> Fixes: 93c4a162b014("powerpc/6xx: Store PGDIR physical address in a SPRG")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/4622a2d43101ea2e3d54a2af090f25a5
cheers
^ permalink raw reply
* Re: powerpc/vdso64: Fix CLOCK_MONOTONIC inconsistencies across Y2038
From: Michael Ellerman @ 2019-03-20 13:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: arnd, sboyd, linux-kernel, john.stultz, tglx, jaydee
In-Reply-To: <20190313131438.8212-1-mpe@ellerman.id.au>
On Wed, 2019-03-13 at 13:14:38 UTC, Michael Ellerman wrote:
> Jakub Drnec reported:
> Setting the realtime clock can sometimes make the monotonic clock go
> back by over a hundred years. Decreasing the realtime clock across
> the y2k38 threshold is one reliable way to reproduce. Allegedly this
> can also happen just by running ntpd, I have not managed to
> reproduce that other than booting with rtc at >2038 and then running
> ntp. When this happens, anything with timers (e.g. openjdk) breaks
> rather badly.
>
> And included a test case (slightly edited for brevity):
> #define _POSIX_C_SOURCE 199309L
> #include <stdio.h>
> #include <time.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> long get_time(void) {
> struct timespec tp;
> clock_gettime(CLOCK_MONOTONIC, &tp);
> return tp.tv_sec + tp.tv_nsec / 1000000000;
> }
>
> int main(void) {
> long last = get_time();
> while(1) {
> long now = get_time();
> if (now < last) {
> printf("clock went backwards by %ld seconds!\n", last - now);
> }
> last = now;
> sleep(1);
> }
> return 0;
> }
>
> Which when run concurrently with:
> # date -s 2040-1-1
> # date -s 2037-1-1
>
> Will detect the clock going backward.
>
> The root cause is that wtom_clock_sec in struct vdso_data is only a
> 32-bit signed value, even though we set its value to be equal to
> tk->wall_to_monotonic.tv_sec which is 64-bits.
>
> Because the monotonic clock starts at zero when the system boots the
> wall_to_montonic.tv_sec offset is negative for current and future
> dates. Currently on a freshly booted system the offset will be in the
> vicinity of negative 1.5 billion seconds.
>
> However if the wall clock is set past the Y2038 boundary, the offset
> from wall to monotonic becomes less than negative 2^31, and no longer
> fits in 32-bits. When that value is assigned to wtom_clock_sec it is
> truncated and becomes positive, causing the VDSO assembly code to
> calculate CLOCK_MONOTONIC incorrectly.
>
> That causes CLOCK_MONOTONIC to jump ahead by ~4 billion seconds which
> it is not meant to do. Worse, if the time is then set back before the
> Y2038 boundary CLOCK_MONOTONIC will jump backward.
>
> We can fix it simply by storing the full 64-bit offset in the
> vdso_data, and using that in the VDSO assembly code. We also shuffle
> some of the fields in vdso_data to avoid creating a hole.
>
> The original commit that added the CLOCK_MONOTONIC support to the VDSO
> did actually use a 64-bit value for wtom_clock_sec, see commit
> a7f290dad32e ("[PATCH] powerpc: Merge vdso's and add vdso support to
> 32 bits kernel") (Nov 2005). However just 3 days later it was
> converted to 32-bits in commit 0c37ec2aa88b ("[PATCH] powerpc: vdso
> fixes (take #2)"), and the bug has existed since then AFAICS.
>
> Fixes: 0c37ec2aa88b ("[PATCH] powerpc: vdso fixes (take #2)")
> Cc: stable@vger.kernel.org # v2.6.15+
> Link: http://lkml.kernel.org/r/HaC.ZfES.62bwlnvAvMP.1STMMj@seznam.cz
> Reported-by: Jakub Drnec <jaydee@email.cz>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Applied to powerpc fixes.
https://git.kernel.org/powerpc/c/b5b4453e7912f056da1ca7572574cada
cheers
^ permalink raw reply
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
From: Christophe Leroy @ 2019-03-20 13:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
In-Reply-To: <87r2b1ya8b.fsf@concordia.ellerman.id.au>
Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>> This patch implements a framework for Kernel Userspace Access
>>> Protection.
>>>
>>> Then subarches will have the possibility to provide their own
>>> implementation by providing setup_kuap() and
>>> allow/prevent_user_access().
>>>
>>> Some platforms will need to know the area accessed and whether it is
>>> accessed from read, write or both. Therefore source, destination and
>>> size and handed over to the two functions.
>>>
>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>> Allow subarch to override allow_read/write_from/to_user().
>>
>> Those little helpers that will just call allow_user_access() when
>> distinct read/write handling is not performed looks overkill to me.
>>
>> Can't the subarch do it by itself based on the nullity of from/to ?
>>
>> static inline void allow_user_access(void __user *to, const void __user
>> *from,
>> unsigned long size)
>> {
>> if (to & from)
>> set_kuap(0);
>> else if (to)
>> set_kuap(AMR_KUAP_BLOCK_READ);
>> else if (from)
>> set_kuap(AMR_KUAP_BLOCK_WRITE);
>> }
>
> You could implement it that way, but it reads better at the call sites
> if we have:
>
> allow_write_to_user(uaddr, sizeof(*uaddr));
> vs:
> allow_user_access(uaddr, NULL, sizeof(*uaddr));
>
> So I'm inclined to keep them. It should all end up inlined and generate
> the same code at the end of the day.
>
I was not suggesting to completly remove allow_write_to_user(), I fully
agree that it reads better at the call sites.
I was just thinking that allow_write_to_user() could remain generic and
call the subarch specific allow_user_access() instead of making multiple
subarch's allow_write_to_user()
But both solution are OK for me at the end.
Christophe
^ permalink raw reply
* Re: [PATCH] compiler: allow all arches to enable CONFIG_OPTIMIZE_INLINING
From: Arnd Bergmann @ 2019-03-20 13:04 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-arch, linux-s390, Dave Hansen, X86 ML, linux-mips,
Linux Kernel Mailing List, Paul Burton, Ingo Molnar, linux-mtd,
Andrew Morton, linuxppc-dev, linux-arm-kernel
In-Reply-To: <CAK7LNATjXoNrbt2SSgR4omiQEBppOkF6mdcVJjpw2B+ihGQvGw@mail.gmail.com>
On Wed, Mar 20, 2019 at 11:19 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Wed, Mar 20, 2019 at 6:39 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Wed, Mar 20, 2019 at 7:41 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> >
> > > It is unclear to me how to fix it.
> > > That's why I ended up with "depends on !MIPS".
> > >
> > >
> > > MODPOST vmlinux.o
> > > arch/mips/mm/sc-mips.o: In function `mips_sc_prefetch_enable.part.2':
> > > sc-mips.c:(.text+0x98): undefined reference to `mips_gcr_base'
> > > sc-mips.c:(.text+0x9c): undefined reference to `mips_gcr_base'
> > > sc-mips.c:(.text+0xbc): undefined reference to `mips_gcr_base'
> > > sc-mips.c:(.text+0xc8): undefined reference to `mips_gcr_base'
> > > sc-mips.c:(.text+0xdc): undefined reference to `mips_gcr_base'
> > > arch/mips/mm/sc-mips.o:sc-mips.c:(.text.unlikely+0x44): more undefined
> > > references to `mips_gcr_base'
> > >
> > >
> > > Perhaps, MIPS folks may know how to fix it.
> >
> > I would guess like this:
> >
> > diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
> > index 8bc5df49b0e1..a27483fedb7d 100644
> > --- a/arch/mips/include/asm/mips-cm.h
> > +++ b/arch/mips/include/asm/mips-cm.h
> > @@ -79,7 +79,7 @@ static inline int mips_cm_probe(void)
> > *
> > * Returns true if a CM is present in the system, else false.
> > */
> > -static inline bool mips_cm_present(void)
> > +static __always_inline bool mips_cm_present(void)
> > {
> > #ifdef CONFIG_MIPS_CM
> > return mips_gcr_base != NULL;
> > @@ -93,7 +93,7 @@ static inline bool mips_cm_present(void)
> > *
> > * Returns true if the system implements an L2-only sync region, else false.
> > */
> > -static inline bool mips_cm_has_l2sync(void)
> > +static __always_inline bool mips_cm_has_l2sync(void)
> > {
> > #ifdef CONFIG_MIPS_CM
> > return mips_cm_l2sync_base != NULL;
> >
>
>
> Thanks, I applied the above, but I still see
> undefined reference to `mips_gcr_base'
>
>
> I attached .config to produce this error.
>
> I use prebuilt mips-linux-gcc from
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
I got to this patch experimentally, it fixes the problem for me:
diff --git a/arch/mips/mm/sc-mips.c b/arch/mips/mm/sc-mips.c
index 394673991bab..d70d02da038b 100644
--- a/arch/mips/mm/sc-mips.c
+++ b/arch/mips/mm/sc-mips.c
@@ -181,7 +181,7 @@ static int __init mips_sc_probe_cm3(void)
return 0;
}
-static inline int __init mips_sc_probe(void)
+static __always_inline int __init mips_sc_probe(void)
{
struct cpuinfo_mips *c = ¤t_cpu_data;
unsigned int config1, config2;
diff --git a/arch/mips/include/asm/bitops.h b/arch/mips/include/asm/bitops.h
index 830c93a010c3..186c28463bf3 100644
--- a/arch/mips/include/asm/bitops.h
+++ b/arch/mips/include/asm/bitops.h
@@ -548,7 +548,7 @@ static inline unsigned long __fls(unsigned long word)
* Returns 0..SZLONG-1
* Undefined if no bit exists, so code should check against 0 first.
*/
-static inline unsigned long __ffs(unsigned long word)
+static __always_inline unsigned long __ffs(unsigned long word)
{
return __fls(word & -word);
}
It does look like a gcc bug though, as at least some of the references
are from a function that got split out from an inlined function but that
has no remaining call sites.
Arnd
^ permalink raw reply related
* Re: [PATCH v3] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Laurent Vivier @ 2019-03-20 13:06 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, David Gibson
In-Reply-To: <871s31zp9h.fsf@concordia.ellerman.id.au>
On 20/03/2019 13:47, Michael Ellerman wrote:
> Laurent Vivier <lvivier@redhat.com> writes:
>> Hi Michael,
>>
>> as it seems good now, could you pick up this patch for merging?
>
> I'll start picking up patches for next starting after rc2, so next week.
>
> If you think it's a bug fix I can put it into fixes now, but I don't
> think it's a bug fix is it?
No, it's only cosmetic.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v2] kmemleak: skip scanning holes in the .bss section
From: Michael Ellerman @ 2019-03-20 13:15 UTC (permalink / raw)
To: Catalin Marinas, Qian Cai
Cc: linux-kernel, kvm-ppc, linux-mm, akpm, linuxppc-dev
In-Reply-To: <20190319115747.GB59586@arrakis.emea.arm.com>
Catalin Marinas <catalin.marinas@arm.com> writes:
> Hi Qian,
>
> On Wed, Mar 13, 2019 at 10:57:17AM -0400, Qian Cai wrote:
>> @@ -1531,7 +1547,14 @@ static void kmemleak_scan(void)
>>
>> /* data/bss scanning */
>> scan_large_block(_sdata, _edata);
>> - scan_large_block(__bss_start, __bss_stop);
>> +
>> + if (bss_hole_start) {
>> + scan_large_block(__bss_start, bss_hole_start);
>> + scan_large_block(bss_hole_stop, __bss_stop);
>> + } else {
>> + scan_large_block(__bss_start, __bss_stop);
>> + }
>> +
>> scan_large_block(__start_ro_after_init, __end_ro_after_init);
>
> I'm not a fan of this approach but I couldn't come up with anything
> better. I was hoping we could check for PageReserved() in scan_block()
> but on arm64 it ends up not scanning the .bss at all.
>
> Until another user appears, I'm ok with this patch.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
I actually would like to rework this kvm_tmp thing to not be in bss at
all. It's a bit of a hack and is incompatible with strict RWX.
If we size it a bit more conservatively we can hopefully just reserve
some space in the text section for it.
I'm not going to have time to work on that immediately though, so if
people want this fixed now then this patch could go in as a temporary
solution.
cheers
^ permalink raw reply
* Re: [PATCH] compiler: allow all arches to enable CONFIG_OPTIMIZE_INLINING
From: Arnd Bergmann @ 2019-03-20 13:34 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-arch, linux-s390, Dave Hansen, the arch/x86 maintainers,
linux-mips, Linux Kernel Mailing List, Paul Burton, Ingo Molnar,
linux-mtd, Andrew Morton, linuxppc-dev, Linux ARM
In-Reply-To: <CAK8P3a3BG_mxYxxCx4S_+ZKAer_+5FpmkzLk0VrACZekuD=2GQ@mail.gmail.com>
On Wed, Mar 20, 2019 at 10:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I've added your patch to my randconfig test setup and will let you
> know if I see anything noticeable. I'm currently testing clang-arm32,
> clang-arm64 and gcc-x86.
This is the only additional bug that has come up so far:
`.exit.text' referenced in section `.alt.smp.init' of
drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section
`exit.text' of drivers/char/ipmi/ipmi_msghandler.o
diff --git a/arch/arm/kernel/atags.h b/arch/arm/kernel/atags.h
index 201100226301..84b12e33104d 100644
--- a/arch/arm/kernel/atags.h
+++ b/arch/arm/kernel/atags.h
@@ -5,7 +5,7 @@ void convert_to_tag_list(struct tag *tags);
const struct machine_desc *setup_machine_tags(phys_addr_t __atags_pointer,
unsigned int machine_nr);
#else
-static inline const struct machine_desc *
+static __always_inline const struct machine_desc *
setup_machine_tags(phys_addr_t __atags_pointer, unsigned int machine_nr)
{
early_print("no ATAGS support: can't continue\n");
^ permalink raw reply related
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-20 15:34 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Ross Zwisler, Andrew Morton, linuxppc-dev, Kirill A . Shutemov
In-Reply-To: <878sxa7ys5.fsf@linux.ibm.com>
On Wed, Mar 20, 2019 at 1:09 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
>
> > Dan Williams <dan.j.williams@intel.com> writes:
> >
> >>
> >>> Now what will be page size used for mapping vmemmap?
> >>
> >> That's up to the architecture's vmemmap_populate() implementation.
> >>
> >>> Architectures
> >>> possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
> >>> device-dax with struct page in the device will have pfn reserve area aligned
> >>> to PAGE_SIZE with the above example? We can't map that using
> >>> PMD_SIZE page size?
> >>
> >> IIUC, that's a different alignment. Currently that's handled by
> >> padding the reservation area up to a section (128MB on x86) boundary,
> >> but I'm working on patches to allow sub-section sized ranges to be
> >> mapped.
> >
> > I am missing something w.r.t code. The below code align that using nd_pfn->align
> >
> > if (nd_pfn->mode == PFN_MODE_PMEM) {
> > unsigned long memmap_size;
> >
> > /*
> > * vmemmap_populate_hugepages() allocates the memmap array in
> > * HPAGE_SIZE chunks.
> > */
> > memmap_size = ALIGN(64 * npfns, HPAGE_SIZE);
> > offset = ALIGN(start + SZ_8K + memmap_size + dax_label_reserve,
> > nd_pfn->align) - start;
> > }
> >
> > IIUC that is finding the offset where to put vmemmap start. And that has
> > to be aligned to the page size with which we may end up mapping vmemmap
> > area right?
Right, that's the physical offset of where the vmemmap ends, and the
memory to be mapped begins.
> > Yes we find the npfns by aligning up using PAGES_PER_SECTION. But that
> > is to compute howmany pfns we should map for this pfn dev right?
> >
>
> Also i guess those 4K assumptions there is wrong?
Yes, I think to support non-4K-PAGE_SIZE systems the 'pfn' metadata
needs to be revved and the PAGE_SIZE needs to be recorded in the
info-block.
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-03-20 16:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <20190204144048-mutt-send-email-mst@kernel.org>
Hello Michael,
Sorry for the delay in responding. We had some internal discussions on
this.
Michael S. Tsirkin <mst@redhat.com> writes:
> On Mon, Feb 04, 2019 at 04:14:20PM -0200, Thiago Jung Bauermann wrote:
>>
>> Hello Michael,
>>
>> Michael S. Tsirkin <mst@redhat.com> writes:
>>
>> > On Tue, Jan 29, 2019 at 03:42:44PM -0200, Thiago Jung Bauermann wrote:
>> So while ACCESS_PLATFORM solves our problems for secure guests, we can't
>> turn it on by default because we can't affect legacy systems. Doing so
>> would penalize existing systems that can access all memory. They would
>> all have to unnecessarily go through address translations, and take a
>> performance hit.
>
> So as step one, you just give hypervisor admin an option to run legacy
> systems faster by blocking secure mode. I don't see why that is
> so terrible.
There are a few reasons why:
1. It's bad user experience to require people to fiddle with knobs for
obscure reasons if it's possible to design things such that they Just
Work.
2. "User" in this case can be a human directly calling QEMU, but could
also be libvirt or one of its users, or some other framework. This means
having to adjust and/or educate an open-ended number of people and
software. It's best avoided if possible.
3. The hypervisor admin and the admin of the guest system don't
necessarily belong to the same organization (e.g., cloud provider and
cloud customer), so there may be some friction when they need to
coordinate to get this right.
4. A feature of our design is that the guest may or may not decide to
"go secure" at boot time, so it's best not to depend on flags that may
or may not have been set at the time QEMU was started.
>> The semantics of ACCESS_PLATFORM assume that the hypervisor/QEMU knows
>> in advance - right when the VM is instantiated - that it will not have
>> access to all guest memory.
>
> Not quite. It just means that hypervisor can live with not having
> access to all memory. If platform wants to give it access
> to all memory that is quite all right.
Except that on powerpc it also means "there's an IOMMU present" and
there's no way to say "bypass IOMMU translation". :-/
>> Another way of looking at this issue which also explains our reluctance
>> is that the only difference between a secure guest and a regular guest
>> (at least regarding virtio) is that the former uses swiotlb while the
>> latter doens't.
>
> But swiotlb is just one implementation. It's a guest internal thing. The
> issue is that memory isn't host accessible.
From what I understand of the ACCESS_PLATFORM definition, the host will
only ever try to access memory addresses that are supplied to it by the
guest, so all of the secure guest memory that the host cares about is
accessible:
If this feature bit is set to 0, then the device has same access to
memory addresses supplied to it as the driver has. In particular,
the device will always use physical addresses matching addresses
used by the driver (typically meaning physical addresses used by the
CPU) and not translated further, and can access any address supplied
to it by the driver. When clear, this overrides any
platform-specific description of whether device access is limited or
translated in any way, e.g. whether an IOMMU may be present.
All of the above is true for POWER guests, whether they are secure
guests or not.
Or are you saying that a virtio device may want to access memory
addresses that weren't supplied to it by the driver?
>> And from the device's point of view they're
>> indistinguishable. It can't tell one guest that is using swiotlb from
>> one that isn't. And that implies that secure guest vs regular guest
>> isn't a virtio interface issue, it's "guest internal affairs". So
>> there's no reason to reflect that in the feature flags.
>
> So don't. The way not to reflect that in the feature flags is
> to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
>
>
> Without ACCESS_PLATFORM
> virtio has a very specific opinion about the security of the
> device, and that opinion is that device is part of the guest
> supervisor security domain.
Sorry for being a bit dense, but not sure what "the device is part of
the guest supervisor security domain" means. In powerpc-speak,
"supervisor" is the operating system so perhaps that explains my
confusion. Are you saying that without ACCESS_PLATFORM, the guest
considers the host to be part of the guest operating system's security
domain? If so, does that have any other implication besides "the host
can access any address supplied to it by the driver"? If that is the
case, perhaps the definition of ACCESS_PLATFORM needs to be amended to
include that information because it's not part of the current
definition.
>> That said, we still would like to arrive at a proper design for this
>> rather than add yet another hack if we can avoid it. So here's another
>> proposal: considering that the dma-direct code (in kernel/dma/direct.c)
>> automatically uses swiotlb when necessary (thanks to Christoph's recent
>> DMA work), would it be ok to replace virtio's own direct-memory code
>> that is used in the !ACCESS_PLATFORM case with the dma-direct code? That
>> way we'll get swiotlb even with !ACCESS_PLATFORM, and virtio will get a
>> code cleanup (replace open-coded stuff with calls to existing
>> infrastructure).
>
> Let's say I have some doubts that there's an API that
> matches what virtio with its bag of legacy compatibility exactly.
Ok.
>> > But the name "sev_active" makes me scared because at least AMD guys who
>> > were doing the sensible thing and setting ACCESS_PLATFORM
>>
>> My understanding is, AMD guest-platform knows in advance that their
>> guest will run in secure mode and hence sets the flag at the time of VM
>> instantiation. Unfortunately we dont have that luxury on our platforms.
>
> Well you do have that luxury. It looks like that there are existing
> guests that already acknowledge ACCESS_PLATFORM and you are not happy
> with how that path is slow. So you are trying to optimize for
> them by clearing ACCESS_PLATFORM and then you have lost ability
> to invoke DMA API.
>
> For example if there was another flag just like ACCESS_PLATFORM
> just not yet used by anyone, you would be all fine using that right?
Yes, a new flag sounds like a great idea. What about the definition
below?
VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
exception that the IOMMU is explicitly defined to be off or bypassed
when accessing memory addresses supplied to the device by the
driver. This flag should be set by the guest if offered, but to
allow for backward-compatibility device implementations allow for it
to be left unset by the guest. It is an error to set both this flag
and VIRTIO_F_ACCESS_PLATFORM.
> Is there any justification to doing that beyond someone putting
> out slow code in the past?
The definition of the ACCESS_PLATFORM flag is generic and captures the
notion of memory access restrictions for the device. Unfortunately, on
powerpc pSeries guests it also implies that the IOMMU is turned on even
though pSeries guests have never used IOMMU for virtio devices. Combined
with the lack of a way to turn off or bypass the IOMMU for virtio
devices, this means that existing guests in the field are compelled to
use the IOMMU even though that never was the case before, and said
guests having no mechanism to turn it off.
Therefore, we need a new flag to signal the memory access restriction
present in secure guests which doesn't also imply turning on the IOMMU.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v3 2/5] ocxl: Clean up printf formats
From: Joe Perches @ 2019-03-20 17:24 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Frederic Barrat, linuxppc-dev
In-Reply-To: <20190320053448.2098-3-alastair@au1.ibm.com>
On Wed, 2019-03-20 at 16:34 +1100, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Use %# instead of using a literal '0x'
I do not suggest this as reasonable.
There are 10's of thousands of uses of 0x%x in the kernel
and converting them to save a byte seems unnecessary.
$ git grep -P '0x%[\*\d\.]*[xX]' | wc -l
26120
And the %#x style is by far the lesser used form
$ git grep -P '%#[\*\d\.]*[xX]' | wc -l
2726
Also, the sized form of %#[size]x is frequently misused
where the size does not account for the initial 0x output.
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
[]
> @@ -178,9 +178,9 @@ static int read_dvsec_vendor(struct pci_dev *dev)
> pci_read_config_dword(dev, pos + OCXL_DVSEC_VENDOR_DLX_VERS, &dlx);
>
> dev_dbg(&dev->dev, "Vendor specific DVSEC:\n");
> - dev_dbg(&dev->dev, " CFG version = 0x%x\n", cfg);
> - dev_dbg(&dev->dev, " TLX version = 0x%x\n", tlx);
> - dev_dbg(&dev->dev, " DLX version = 0x%x\n", dlx);
> + dev_dbg(&dev->dev, " CFG version = %#x\n", cfg);
> + dev_dbg(&dev->dev, " TLX version = %#x\n", tlx);
> + dev_dbg(&dev->dev, " DLX version = %#x\n", dlx);
etc...
^ permalink raw reply
* Re: [PATCH v2] kmemleak: skip scanning holes in the .bss section
From: Catalin Marinas @ 2019-03-20 18:16 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-kernel, kvm-ppc, linux-mm, Qian Cai, akpm, linuxppc-dev
In-Reply-To: <87lg19y9dp.fsf@concordia.ellerman.id.au>
On Thu, Mar 21, 2019 at 12:15:46AM +1100, Michael Ellerman wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > On Wed, Mar 13, 2019 at 10:57:17AM -0400, Qian Cai wrote:
> >> @@ -1531,7 +1547,14 @@ static void kmemleak_scan(void)
> >>
> >> /* data/bss scanning */
> >> scan_large_block(_sdata, _edata);
> >> - scan_large_block(__bss_start, __bss_stop);
> >> +
> >> + if (bss_hole_start) {
> >> + scan_large_block(__bss_start, bss_hole_start);
> >> + scan_large_block(bss_hole_stop, __bss_stop);
> >> + } else {
> >> + scan_large_block(__bss_start, __bss_stop);
> >> + }
> >> +
> >> scan_large_block(__start_ro_after_init, __end_ro_after_init);
> >
> > I'm not a fan of this approach but I couldn't come up with anything
> > better. I was hoping we could check for PageReserved() in scan_block()
> > but on arm64 it ends up not scanning the .bss at all.
> >
> > Until another user appears, I'm ok with this patch.
> >
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> I actually would like to rework this kvm_tmp thing to not be in bss at
> all. It's a bit of a hack and is incompatible with strict RWX.
>
> If we size it a bit more conservatively we can hopefully just reserve
> some space in the text section for it.
>
> I'm not going to have time to work on that immediately though, so if
> people want this fixed now then this patch could go in as a temporary
> solution.
I think I have a simpler idea. Kmemleak allows punching holes in
allocated objects, so just turn the data/bss sections into dedicated
kmemleak objects. This happens when kmemleak is initialised, before the
initcalls are invoked. The kvm_free_tmp() would just free the
corresponding part of the bss.
Patch below, only tested briefly on arm64. Qian, could you give it a try
on powerpc? Thanks.
--------8<------------------------------
diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
index 683b5b3805bd..c4b8cb3c298d 100644
--- a/arch/powerpc/kernel/kvm.c
+++ b/arch/powerpc/kernel/kvm.c
@@ -712,6 +712,8 @@ static void kvm_use_magic_page(void)
static __init void kvm_free_tmp(void)
{
+ kmemleak_free_part(&kvm_tmp[kvm_tmp_index],
+ ARRAY_SIZE(kvm_tmp) - kvm_tmp_index);
free_reserved_area(&kvm_tmp[kvm_tmp_index],
&kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
}
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 707fa5579f66..0f6adcbfc2c7 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1529,11 +1529,6 @@ static void kmemleak_scan(void)
}
rcu_read_unlock();
- /* data/bss scanning */
- scan_large_block(_sdata, _edata);
- scan_large_block(__bss_start, __bss_stop);
- scan_large_block(__start_ro_after_init, __end_ro_after_init);
-
#ifdef CONFIG_SMP
/* per-cpu sections scanning */
for_each_possible_cpu(i)
@@ -2071,6 +2066,15 @@ void __init kmemleak_init(void)
}
local_irq_restore(flags);
+ /* register the data/bss sections */
+ create_object((unsigned long)_sdata, _edata - _sdata,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+ create_object((unsigned long)__bss_start, __bss_stop - __bss_start,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+ create_object((unsigned long)__start_ro_after_init,
+ __end_ro_after_init - __start_ro_after_init,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+
/*
* This is the point where tracking allocations is safe. Automatic
* scanning is started during the late initcall. Add the early logged
^ permalink raw reply related
* Re: [PATCH kernel RFC 2/2] vfio-pci-nvlink2: Implement interconnect isolation
From: Alex Williamson @ 2019-03-20 19:09 UTC (permalink / raw)
To: David Gibson
Cc: Jose Ricardo Ziviani, Alexey Kardashevskiy,
Daniel Henrique Barboza, kvm-ppc, Piotr Jaroszynski,
Leonardo Augusto Guimarães Garcia, linuxppc-dev
In-Reply-To: <20190320043824.GG31018@umbus.fritz.box>
On Wed, 20 Mar 2019 15:38:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Mar 19, 2019 at 10:36:19AM -0600, Alex Williamson wrote:
> > On Fri, 15 Mar 2019 19:18:35 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > > The NVIDIA V100 SXM2 GPUs are connected to the CPU via PCIe links and
> > > (on POWER9) NVLinks. In addition to that, GPUs themselves have direct
> > > peer to peer NVLinks in groups of 2 to 4 GPUs. At the moment the POWERNV
> > > platform puts all interconnected GPUs to the same IOMMU group.
> > >
> > > However the user may want to pass individual GPUs to the userspace so
> > > in order to do so we need to put them into separate IOMMU groups and
> > > cut off the interconnects.
> > >
> > > Thankfully V100 GPUs implement an interface to do by programming link
> > > disabling mask to BAR0 of a GPU. Once a link is disabled in a GPU using
> > > this interface, it cannot be re-enabled until the secondary bus reset is
> > > issued to the GPU.
> > >
> > > This defines a reset_done() handler for V100 NVlink2 device which
> > > determines what links need to be disabled. This relies on presence
> > > of the new "ibm,nvlink-peers" device tree property of a GPU telling which
> > > PCI peers it is connected to (which includes NVLink bridges or peer GPUs).
> > >
> > > This does not change the existing behaviour and instead adds
> > > a new "isolate_nvlink" kernel parameter to allow such isolation.
> > >
> > > The alternative approaches would be:
> > >
> > > 1. do this in the system firmware (skiboot) but for that we would need
> > > to tell skiboot via an additional OPAL call whether or not we want this
> > > isolation - skiboot is unaware of IOMMU groups.
> > >
> > > 2. do this in the secondary bus reset handler in the POWERNV platform -
> > > the problem with that is at that point the device is not enabled, i.e.
> > > config space is not restored so we need to enable the device (i.e. MMIO
> > > bit in CMD register + program valid address to BAR0) in order to disable
> > > links and then perhaps undo all this initialization to bring the device
> > > back to the state where pci_try_reset_function() expects it to be.
> >
> > The trouble seems to be that this approach only maintains the isolation
> > exposed by the IOMMU group when vfio-pci is the active driver for the
> > device. IOMMU groups can be used by any driver and the IOMMU core is
> > incorporating groups in various ways.
>
> I don't think that reasoning is quite right. An IOMMU group doesn't
> necessarily represent devices which *are* isolated, just devices which
> *can be* isolated. There are plenty of instances when we don't need
> to isolate devices in different IOMMU groups: passing both groups to
> the same guest or userspace VFIO driver for example, or indeed when
> both groups are owned by regular host kernel drivers.
>
> In at least some of those cases we also don't want to isolate the
> devices when we don't have to, usually for performance reasons.
I see IOMMU groups as representing the current isolation of the device,
not just the possible isolation. If there are ways to break down that
isolation then ideally the group would be updated to reflect it. The
ACS disable patches seem to support this, at boot time we can choose to
disable ACS at certain points in the topology to favor peer-to-peer
performance over isolation. This is then reflected in the group
composition, because even though ACS *can be* enabled at the given
isolation points, it's intentionally not with this option. Whether or
not a given user who owns multiple devices needs that isolation is
really beside the point, the user can choose to connect groups via IOMMU
mappings or reconfigure the system to disable ACS and potentially more
direct routing. The IOMMU groups are still accurately reflecting the
topology and IOMMU based isolation.
> > So, if there's a device specific
> > way to configure the isolation reported in the group, which requires
> > some sort of active management against things like secondary bus
> > resets, then I think we need to manage it above the attached endpoint
> > driver.
>
> The problem is that above the endpoint driver, we don't actually have
> enough information about what should be isolated. For VFIO we want to
> isolate things if they're in different containers, for most regular
> host kernel drivers we don't need to isolate at all (although we might
> as well when it doesn't have a cost).
This idea that we only want to isolate things if they're in different
containers is bogus, imo. There are performance reasons why we might
not want things isolated, but there are also address space reasons why
we do. If there are direct routes between devices, the user needs to
be aware of the IOVA pollution, if we maintain singleton groups, they
don't. Granted we don't really account for this well in most
userspaces and fumble through it by luck of the address space layout
and lack of devices really attempting peer to peer access.
For in-kernel users, we're still theoretically trying to isolate
devices such that they have restricted access to only the resources
they need. Disabling things like ACS in the topology reduces that
isolation. AFAICT, most users don't really care about that degree of
isolation, so they run with iommu=pt for native driver performance
while still having the IOMMU available for isolation use cases running
in parallel. We don't currently have support for on-demand enabling
isolation.
> The host side nVidia GPGPU
> drivers also won't want to isolate the (host owned) NVLink devices
> from each other, since they'll want to use the fast interconnects
This falls into the same mixed use case scenario above where we don't
really have a good solution today. Things like ACS are dynamically
configurable, but we don't expose any interfaces to let drivers or
users change it (aside from setpci, which we don't account for
dynamically). We assume a simplistic model where if you want IOMMU,
then you must also want the maximum configurable isolation.
Dynamically changing routing is not necessarily the most foolproof
thing either with potentially in-flight transactions and existing DMA
mappings, which is why I've suggested a couple times that perhaps we
could do a software hot-unplug of a sub-hierarchy, muck with isolation
at the remaining node, then re-discover the removed devices.
Of course when we bring NVIDIA into the mix, I have little sympathy
that the NVLink interfaces are all proprietary and we have no idea how
to make those dynamic changes or discover the interconnected-ness of a
device. Thanks,
Alex
^ 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