* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Christophe LEROY @ 2018-10-18 11:18 UTC (permalink / raw)
To: Jann Horn; +Cc: muriloo, linuxppc-dev
In-Reply-To: <CAG48ez2C9M8zgmubC2DLwqWzS7WzUj=nKhUuT4KkfWwS4v1Dig@mail.gmail.com>
Le 18/10/2018 à 13:12, Jann Horn a écrit :
> On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
> <christophe.leroy@c-s.fr> wrote:
>> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>>> Recently we implemented show_user_instructions() which dumps the code
>>> around the NIP when a user space process dies with an unhandled
>>> signal. This was modelled on the x86 code, and we even went so far as
>>> to implement the exact same bug, namely that if the user process
>>> crashed with its NIP pointing into the kernel we will dump kernel text
>>> to dmesg. eg:
>>>
>>> bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>>> bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>>> bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>>>
>>> This was discovered on x86 by Jann Horn and fixed in commit
>>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>>>
>>> Fix it by checking the adjusted NIP value (pc) and number of
>>> instructions against USER_DS, and bail if we fail the check, eg:
>>>
>>> bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>>> bad-bctr[2969]: Bad NIP, not dumping instructions.
>>>
>>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> arch/powerpc/kernel/process.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>> index 913c5725cdb2..bb6ac471a784 100644
>>> --- a/arch/powerpc/kernel/process.c
>>> +++ b/arch/powerpc/kernel/process.c
>>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>>>
>>> pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>>>
>>> + /*
>>> + * Make sure the NIP points at userspace, not kernel text/data or
>>> + * elsewhere.
>>> + */
>>> + if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>>> + pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>>> + current->comm, current->pid);
>>> + return;
>>> + }
>>> +
>>
>> Is there any reason for not using access_ok() here ?
>
> It's probably more robust this way, in case someone decides to call
> into this from kernel exception context at some point, or something
> like that?
>
But access_ok() uses current->thread.addr_limit, while USER_DS may
provide a larger segment:
#ifdef __powerpc64__
/* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
#define USER_DS MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
#else
#define USER_DS MAKE_MM_SEG(TASK_SIZE - 1)
#endif
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Jann Horn @ 2018-10-18 11:31 UTC (permalink / raw)
To: christophe.leroy
Cc: linuxppc-dev, Ingo Molnar, Borislav Petkov, Andy Lutomirski,
muriloo, Thomas Gleixner
In-Reply-To: <73bd3821-2d5d-39f6-4d23-f8eae6d74cb3@c-s.fr>
+cc x86 folks
On Thu, Oct 18, 2018 at 1:18 PM Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
> Le 18/10/2018 à 13:12, Jann Horn a écrit :
> > On Thu, Oct 18, 2018 at 11:28 AM Christophe LEROY
> > <christophe.leroy@c-s.fr> wrote:
> >> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
> >>> Recently we implemented show_user_instructions() which dumps the code
> >>> around the NIP when a user space process dies with an unhandled
> >>> signal. This was modelled on the x86 code, and we even went so far as
> >>> to implement the exact same bug, namely that if the user process
> >>> crashed with its NIP pointing into the kernel we will dump kernel text
> >>> to dmesg. eg:
> >>>
> >>> bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
> >>> bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
> >>> bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
> >>>
> >>> This was discovered on x86 by Jann Horn and fixed in commit
> >>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
> >>>
> >>> Fix it by checking the adjusted NIP value (pc) and number of
> >>> instructions against USER_DS, and bail if we fail the check, eg:
> >>>
> >>> bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
> >>> bad-bctr[2969]: Bad NIP, not dumping instructions.
> >>>
> >>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> >>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> >>> ---
> >>> arch/powerpc/kernel/process.c | 10 ++++++++++
> >>> 1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >>> index 913c5725cdb2..bb6ac471a784 100644
> >>> --- a/arch/powerpc/kernel/process.c
> >>> +++ b/arch/powerpc/kernel/process.c
> >>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
> >>>
> >>> pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> >>>
> >>> + /*
> >>> + * Make sure the NIP points at userspace, not kernel text/data or
> >>> + * elsewhere.
> >>> + */
> >>> + if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> >>> + pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> >>> + current->comm, current->pid);
> >>> + return;
> >>> + }
> >>> +
> >>
> >> Is there any reason for not using access_ok() here ?
> >
> > It's probably more robust this way, in case someone decides to call
> > into this from kernel exception context at some point, or something
> > like that?
> >
>
> But access_ok() uses current->thread.addr_limit, while USER_DS may
> provide a larger segment:
>
> #ifdef __powerpc64__
> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
> #define USER_DS MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
> #else
> #define USER_DS MAKE_MM_SEG(TASK_SIZE - 1)
> #endif
Where do you write a smaller value than USER_DS into the addr_limit?
The kernel is full of places that assume that any access up to USER_DS
is safe; for example, perf_output_sample_ustack(),
get_perf_callchain(), do_exit(), flush_old_exec(), vma_dump_size(),
... - and I also don't see anything in the powerpc code that would
ever write a smaller value into the addr_limit.
I don't know powerpc well, but AFAIK the rule on X86 is basically that
even for compat tasks, attempting to access anything up to USER_DS is
safe because the kernel doesn't put any kernel mappings there. Is that
different on powerpc?
^ permalink raw reply
* Re: [PATCH 29/36] dt-bindings: arm: Convert Renesas board/soc bindings to json-schema
From: Simon Horman @ 2018-10-18 13:01 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Kumar Gala, Grant Likely,
Sergei Shtylyov, Arnd Bergmann, Tom Rini, Frank Rowand,
Linus Walleij, Pantelis Antoniou, linux-kernel@vger.kernel.org,
Bjorn Andersson, open list:MEDIA DRIVERS FOR RENESAS - FCP,
Mark Brown, Geert Uytterhoeven, Jonathan Cameron, Olof Johansson,
linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_Jsq+8EwYzBmzSWL0q0YB3rP-xdrZreRnt1Rj-h=D8OTNVwA@mail.gmail.com>
On Mon, Oct 08, 2018 at 09:05:58AM -0500, Rob Herring wrote:
> On Mon, Oct 8, 2018 at 3:02 AM Simon Horman <horms@verge.net.au> wrote:
> >
> > On Fri, Oct 05, 2018 at 11:58:41AM -0500, Rob Herring wrote:
> > > Convert Renesas SoC bindings to DT schema format using json-schema.
> > >
> > > Cc: Simon Horman <horms@verge.net.au>
> > > Cc: Magnus Damm <magnus.damm@gmail.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: linux-renesas-soc@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> >
> > This seems fine to me other than that it does not seem
> > to apply cleanly to next.
> >
> > shmobile.txt sees a couple of updates per release cycle so from my point of
> > view it would ideal if this change could hit -rc1 to allow patches for
> > v4.21 to be accepted smoothly (already one from Sergei will need rebasing).
>
> When we get to the point of merging (which isn't going to be 4.20),
> you and other maintainers can probably take all these patches. Other
> than the few restructuring patches, the only dependency is the build
> support which isn't a dependency to apply it, but build it. I plan to
> build any patches as part of reviewing at least early on. OTOH, the
> build support is small enough and self contained that maybe it can
> just be applied for 4.20.
Thanks, understood.
My preference would be to, as you suggest, take changes like
this through the renesas tree.
^ permalink raw reply
* Re: [PATCH 05/36] dt-bindings: arm: renesas: Move 'renesas,prr' binding to its own doc
From: Simon Horman @ 2018-10-18 13:04 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Kumar Gala, Grant Likely, Arnd Bergmann,
Tom Rini, Frank Rowand, Linus Walleij, Pantelis Antoniou,
linux-kernel@vger.kernel.org, Bjorn Andersson,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Mark Brown,
Geert Uytterhoeven, Jonathan Cameron, Olof Johansson,
linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_JsqJGGRfoEb_TjZy2X9QwjfAdoxs_EB+cLNmszHOE8-OtHw@mail.gmail.com>
On Mon, Oct 08, 2018 at 09:59:19AM -0500, Rob Herring wrote:
> On Mon, Oct 8, 2018 at 2:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Rob,
> >
> > On Fri, Oct 5, 2018 at 6:58 PM Rob Herring <robh@kernel.org> wrote:
> > > In preparation to convert board-level bindings to json-schema, move
> > > various misc SoC bindings out to their own file.
> > >
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Simon Horman <horms@verge.net.au>
> > > Cc: Magnus Damm <magnus.damm@gmail.com>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-renesas-soc@vger.kernel.org
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> >
> > Looks good to me, but needs a rebase, as the PRR section has been extended
> > in -next.
>
> Is this something you all can apply still for 4.20?
Sorry, missing this until now. It was too late.
Shall I take it for v4.21?
^ permalink raw reply
* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Michael Ellerman @ 2018-10-18 13:16 UTC (permalink / raw)
To: Christophe LEROY, linuxppc-dev; +Cc: jannh, muriloo
In-Reply-To: <6b3b54e5-cbe3-c693-23ea-26928e7597c8@c-s.fr>
Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 05/10/2018 à 15:21, Michael Ellerman a écrit :
>> Recently we implemented show_user_instructions() which dumps the code
>> around the NIP when a user space process dies with an unhandled
>> signal. This was modelled on the x86 code, and we even went so far as
>> to implement the exact same bug, namely that if the user process
>> crashed with its NIP pointing into the kernel we will dump kernel text
>> to dmesg. eg:
>>
>> bad-bctr[2996]: segfault (11) at c000000000010000 nip c000000000010000 lr 12d0b0894 code 1
>> bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 fb810050 7f8802a6
>> bad-bctr[2996]: code: 3860001c f8010080 48242371 60000000 <7c7b1b79> 4082002c e8010080 eb610048
>>
>> This was discovered on x86 by Jann Horn and fixed in commit
>> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode RIP").
>>
>> Fix it by checking the adjusted NIP value (pc) and number of
>> instructions against USER_DS, and bail if we fail the check, eg:
>>
>> bad-bctr[2969]: segfault (11) at c000000000010000 nip c000000000010000 lr 107930894 code 1
>> bad-bctr[2969]: Bad NIP, not dumping instructions.
>>
>> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/kernel/process.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 913c5725cdb2..bb6ac471a784 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>>
>> pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>>
>> + /*
>> + * Make sure the NIP points at userspace, not kernel text/data or
>> + * elsewhere.
>> + */
>> + if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
>> + pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
>> + current->comm, current->pid);
>> + return;
>> + }
>> +
>
> Is there any reason for not using access_ok() here ?
I wanted to check against USER_DS explicitly. But maybe that was
over-paranoid of me.
cheers
^ permalink raw reply
* [PATCH] powerpc/mm: Make pte_pgprot return all pte bits
From: Michael Ellerman @ 2018-10-18 13:33 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aneesh.kumar
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Other archs do the same and instead of adding required pte bits (which
got masked out) in __ioremap_at(), make sure we filter only pfn bits
out.
Fixes: 26973fa5ac0e ("powerpc/mm: use pte helpers in generic code")
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/book3s/32/pgtable.h | 6 ------
arch/powerpc/include/asm/book3s/64/pgtable.h | 8 --------
arch/powerpc/include/asm/nohash/32/pte-40x.h | 5 -----
arch/powerpc/include/asm/nohash/32/pte-44x.h | 5 -----
arch/powerpc/include/asm/nohash/32/pte-8xx.h | 5 -----
arch/powerpc/include/asm/nohash/32/pte-fsl-booke.h | 5 -----
arch/powerpc/include/asm/nohash/pgtable.h | 1 -
arch/powerpc/include/asm/nohash/pte-book3e.h | 5 -----
arch/powerpc/include/asm/pgtable.h | 10 ++++++++++
9 files changed, 10 insertions(+), 40 deletions(-)
Resend so patchwork picks it up.
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 0fbd4c642b51..e61dd3ae5bc0 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -48,11 +48,6 @@ static inline bool pte_user(pte_t pte)
#define _PAGE_CHG_MASK (PTE_RPN_MASK | _PAGE_HASHPTE | _PAGE_DIRTY | \
_PAGE_ACCESSED | _PAGE_SPECIAL)
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_COHERENT | _PAGE_NO_CACHE | \
- _PAGE_WRITETHRU | _PAGE_USER | _PAGE_ACCESSED | \
- _PAGE_RW | _PAGE_DIRTY)
-
/*
* We define 2 sets of base prot bits, one for basic pages (ie,
* cacheable kernel and user pages) and one for non cacheable
@@ -396,7 +391,6 @@ static inline int pte_young(pte_t pte) { return !!(pte_val(pte) & _PAGE_ACCESSE
static inline int pte_special(pte_t pte) { return !!(pte_val(pte) & _PAGE_SPECIAL); }
static inline int pte_none(pte_t pte) { return (pte_val(pte) & ~_PTE_NONE_MASK) == 0; }
static inline bool pte_exec(pte_t pte) { return true; }
-static inline pgprot_t pte_pgprot(pte_t pte) { return __pgprot(pte_val(pte) & PAGE_PROT_BITS); }
static inline int pte_present(pte_t pte)
{
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 9db2b8eba61d..d33421648d39 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -128,13 +128,6 @@
#define H_PTE_PKEY (H_PTE_PKEY_BIT0 | H_PTE_PKEY_BIT1 | H_PTE_PKEY_BIT2 | \
H_PTE_PKEY_BIT3 | H_PTE_PKEY_BIT4)
-/*
- * Mask of bits returned by pte_pgprot()
- */
-#define PAGE_PROT_BITS (_PAGE_SAO | _PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT | \
- H_PAGE_4K_PFN | _PAGE_PRIVILEGED | _PAGE_ACCESSED | \
- _PAGE_READ | _PAGE_WRITE | _PAGE_DIRTY | _PAGE_EXEC | \
- _PAGE_SOFT_DIRTY | H_PTE_PKEY)
/*
* We define 2 sets of base prot bits, one for basic pages (ie,
* cacheable kernel and user pages) and one for non cacheable
@@ -496,7 +489,6 @@ static inline bool pte_exec(pte_t pte)
return !!(pte_raw(pte) & cpu_to_be64(_PAGE_EXEC));
}
-static inline pgprot_t pte_pgprot(pte_t pte) { return __pgprot(pte_val(pte) & PAGE_PROT_BITS); }
#ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
static inline bool pte_soft_dirty(pte_t pte)
diff --git a/arch/powerpc/include/asm/nohash/32/pte-40x.h b/arch/powerpc/include/asm/nohash/32/pte-40x.h
index 7a8b3c94592f..661f4599f2fc 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-40x.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-40x.h
@@ -73,11 +73,6 @@
/* Until my rework is finished, 40x still needs atomic PTE updates */
#define PTE_ATOMIC_UPDATES 1
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_NO_CACHE | \
- _PAGE_WRITETHRU | _PAGE_USER | _PAGE_ACCESSED | \
- _PAGE_RW | _PAGE_HWWRITE | _PAGE_DIRTY | _PAGE_EXEC)
-
#define _PAGE_BASE_NC (_PAGE_PRESENT | _PAGE_ACCESSED)
#define _PAGE_BASE (_PAGE_BASE_NC)
diff --git a/arch/powerpc/include/asm/nohash/32/pte-44x.h b/arch/powerpc/include/asm/nohash/32/pte-44x.h
index 8d6b268a986f..78bc304f750e 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-44x.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-44x.h
@@ -93,11 +93,6 @@
#define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW)
#define _PAGE_KERNEL_RWX (_PAGE_DIRTY | _PAGE_RW | _PAGE_EXEC)
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_COHERENT | _PAGE_NO_CACHE | \
- _PAGE_WRITETHRU | _PAGE_USER | _PAGE_ACCESSED | \
- _PAGE_RW | _PAGE_DIRTY | _PAGE_EXEC)
-
/* TODO: Add large page lowmem mapping support */
#define _PMD_PRESENT 0
#define _PMD_PRESENT_MASK (PAGE_MASK)
diff --git a/arch/powerpc/include/asm/nohash/32/pte-8xx.h b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
index 1c57efac089d..6bfe041ef59d 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-8xx.h
@@ -55,11 +55,6 @@
#define _PAGE_KERNEL_RW (_PAGE_SH | _PAGE_DIRTY)
#define _PAGE_KERNEL_RWX (_PAGE_SH | _PAGE_DIRTY | _PAGE_EXEC)
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_NO_CACHE | \
- _PAGE_ACCESSED | _PAGE_RO | _PAGE_NA | \
- _PAGE_SH | _PAGE_DIRTY | _PAGE_EXEC)
-
#define _PMD_PRESENT 0x0001
#define _PMD_PRESENT_MASK _PMD_PRESENT
#define _PMD_BAD 0x0fd0
diff --git a/arch/powerpc/include/asm/nohash/32/pte-fsl-booke.h b/arch/powerpc/include/asm/nohash/32/pte-fsl-booke.h
index 1ecf60fe0909..0fc1bd42bb3e 100644
--- a/arch/powerpc/include/asm/nohash/32/pte-fsl-booke.h
+++ b/arch/powerpc/include/asm/nohash/32/pte-fsl-booke.h
@@ -39,11 +39,6 @@
/* No page size encoding in the linux PTE */
#define _PAGE_PSIZE 0
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_COHERENT | _PAGE_NO_CACHE | \
- _PAGE_WRITETHRU | _PAGE_USER | _PAGE_ACCESSED | \
- _PAGE_RW | _PAGE_DIRTY | _PAGE_EXEC)
-
#define _PMD_PRESENT 0
#define _PMD_PRESENT_MASK (PAGE_MASK)
#define _PMD_BAD (~PAGE_MASK)
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 04e9f0922ad4..70ff23974b59 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -52,7 +52,6 @@ static inline int pte_none(pte_t pte) { return (pte_val(pte) & ~_PTE_NONE_MASK)
static inline bool pte_hashpte(pte_t pte) { return false; }
static inline bool pte_ci(pte_t pte) { return pte_val(pte) & _PAGE_NO_CACHE; }
static inline bool pte_exec(pte_t pte) { return pte_val(pte) & _PAGE_EXEC; }
-static inline pgprot_t pte_pgprot(pte_t pte) { return __pgprot(pte_val(pte) & PAGE_PROT_BITS); }
#ifdef CONFIG_NUMA_BALANCING
/*
diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h b/arch/powerpc/include/asm/nohash/pte-book3e.h
index 58eef8cb569d..f95ab6eaf441 100644
--- a/arch/powerpc/include/asm/nohash/pte-book3e.h
+++ b/arch/powerpc/include/asm/nohash/pte-book3e.h
@@ -82,11 +82,6 @@
#define _PTE_NONE_MASK 0
#endif
-/* Mask of bits returned by pte_pgprot() */
-#define PAGE_PROT_BITS (_PAGE_GUARDED | _PAGE_COHERENT | _PAGE_NO_CACHE | \
- _PAGE_WRITETHRU | _PAGE_USER | _PAGE_ACCESSED | \
- _PAGE_PRIVILEGED | _PAGE_RW | _PAGE_DIRTY | _PAGE_EXEC)
-
/*
* We define 2 sets of base prot bits, one for basic pages (ie,
* cacheable kernel and user pages) and one for non cacheable
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index fb4b85bba110..9679b7519a35 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -46,6 +46,16 @@ struct mm_struct;
/* Keep these as a macros to avoid include dependency mess */
#define pte_page(x) pfn_to_page(pte_pfn(x))
#define mk_pte(page, pgprot) pfn_pte(page_to_pfn(page), (pgprot))
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+ unsigned long pte_flags;
+
+ pte_flags = pte_val(pte) & ~PTE_RPN_MASK;
+ return __pgprot(pte_flags);
+}
/*
* ZERO_PAGE is a global shared page that is always zero: used
--
2.17.2
^ permalink raw reply related
* [PATCH] powerpc/8xx: Add DT node for using the SEC engine of the MPC885
From: Christophe Leroy @ 2018-10-18 13:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Rob Herring, Mark Rutland
Cc: devicetree, MAZEYRAT Ludovic, linuxppc-dev, linux-kernel,
linux-crypto
The MPC885 has SEC engine version 1.2 with the following details:
- Number of Crypto channels: 1
- Exec Units: DEU, MDEU and AESU
- Available descriptors: 00010, 00100, 00110, 01000, 11000, 11010
It is also supposed to have descriptor 00000, but it doesn't work
properly so we keep it out for the moment.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/boot/dts/mpc885ads.dts | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/boot/dts/mpc885ads.dts b/arch/powerpc/boot/dts/mpc885ads.dts
index 5b037f51741d..3aa300afbbca 100644
--- a/arch/powerpc/boot/dts/mpc885ads.dts
+++ b/arch/powerpc/boot/dts/mpc885ads.dts
@@ -72,7 +72,7 @@
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
- ranges = <0x0 0xff000000 0x4000>;
+ ranges = <0x0 0xff000000 0x28000>;
bus-frequency = <0>;
// Temporary -- will go away once kernel uses ranges for get_immrbase().
@@ -224,6 +224,17 @@
#size-cells = <0>;
};
};
+
+ crypto@20000 {
+ compatible = "fsl,sec1.2", "fsl,sec1.0";
+ reg = <0x20000 0x8000>;
+ interrupts = <1 1>;
+ interrupt-parent = <&PIC>;
+ fsl,num-channels = <1>;
+ fsl,channel-fifo-len = <24>;
+ fsl,exec-units-mask = <0x4c>;
+ fsl,descriptor-types-mask = <0x05000154>;
+ };
};
chosen {
--
2.13.3
^ permalink raw reply related
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Alex Williamson @ 2018-10-18 16:55 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Piotr Jaroszynski,
Reza Arbab, David Gibson
In-Reply-To: <2175dbbd-21d9-df26-67f5-4b41f90ab1bc@ozlabs.ru>
On Thu, 18 Oct 2018 11:31:33 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 18/10/2018 08:52, Alex Williamson wrote:
> > On Wed, 17 Oct 2018 12:19:20 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> >> On 17/10/2018 06:08, Alex Williamson wrote:
> >>> On Mon, 15 Oct 2018 20:42:33 +1100
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> POWER9 Witherspoon machines come with 4 or 6 V100 GPUs which are not
> >>>> pluggable PCIe devices but implement PCIe links for config space and MMIO.
> >>>> In addition to that the GPUs are interconnected to each other and also
> >>>> have direct links to the P9 CPU. The links are NVLink2 and provide direct
> >>>> access to the system RAM for GPUs via NPU (an NVLink2 "proxy" on P9 chip).
> >>>> These systems also support ATS (address translation services) which is
> >>>> a part of the NVLink2 prototol. Such GPUs also share on-board RAM
> >>>> (16GB in tested config) to the system via the same NVLink2 so a CPU has
> >>>> cache-coherent access to a GPU RAM.
> >>>>
> >>>> This exports GPU RAM to the userspace as a new PCI region. This
> >>>> preregisters the new memory as device memory as it might be used for DMA.
> >>>> This inserts pfns from the fault handler as the GPU memory is not onlined
> >>>> until the NVIDIA driver is loaded and trained the links so doing this
> >>>> earlier produces low level errors which we fence in the firmware so
> >>>> it does not hurt the host system but still better to avoid.
> >>>>
> >>>> This exports ATSD (Address Translation Shootdown) register of NPU which
> >>>> allows the guest to invalidate TLB. The register conviniently occupies
> >>>> a single 64k page. Since NPU maps the GPU memory, it has a "tgt" property
> >>>> (which is an abbreviated host system bus address). This exports the "tgt"
> >>>> as a capability so the guest can program it into the GPU so the GPU can
> >>>> know how to route DMA trafic.
> >>>
> >>> I'm not really following what "tgt" is and why it's needed. Is the GPU
> >>> memory here different than the GPU RAM region above? Why does the user
> >>> need the host system bus address of this "tgt" thing? Are we not able
> >>> to relocate it in guest physical address space, does this shootdown
> >>> only work in the host physical address space and therefore we need this
> >>> offset? Please explain, I'm confused.
> >>
> >>
> >> This "tgt" is made of:
> >> - "memory select" (bits 45, 46)
> >> - "group select" (bits 43, 44)
> >> - "chip select" (bit 42)
> >> - chip internal address (bits 0..41)
> >>
> >> These are internal to GPU and this is where GPU RAM is mapped into the
> >> GPU's real space, this fits 46 bits.
> >>
> >> On POWER9 CPU the bits are different and higher so the same memory is
> >> mapped higher on P9 CPU. Just because we can map it higher, I guess.
> >>
> >> So it is not exactly the address but this provides the exact physical
> >> location of the memory.
> >>
> >> We have a group of 3 interconnected GPUs, they got their own
> >> memory/group/chip numbers. The GPUs use ATS service to translate
> >> userspace to physical (host or guest) addresses. Now a GPU needs to know
> >> which specific link to use for a specific physical address, in other
> >> words what this physical address belongs to - a CPU or one of GPUs. This
> >> is when "tgt" is used by the GPU hardware.
> >
> > Clear as mud ;)
>
> /me is sad. I hope Piotr explained it better...
It's starting to be a bit more clear, and Piotr anticipated the
security questions I was mulling over.
> > So tgt, provided by the npu2 capability of the ATSD
> > region of the NPU tells the GPU (a completely separate device) how to
> > route it its own RAM via its NVLink interface? How can one tgt
> > indicate the routing for multiple interfaces?
>
> This NVLink DMA is using direct host physical addresses (no IOMMU, no
> filtering) which come from ATS. So unless we tell the GPU its own
> address range on the host CPU, it will route trafic via CPU. And the
> driver can also discover the NVLink topology and tell each GPU physical
> addresses of peer GPUs.
I think this is a key point too, the tgt seems to only identify the
routing for the GPU local RAM, but the guest driver is able to
conglomerate this information so each GPU knows how to get directly to
the other GPUs.
> >> A GPU could run all the DMA trafic via the system bus indeed, just not
> >> as fast.
> >>
> >> I am also struggling here and adding an Nvidia person in cc: (I should
> >> have done that when I posted the patches, my bad) to correct when/if I
> >> am wrong.
> >>
> >>
> >>
> >>>
> >>>> For ATS to work, the nest MMU (an NVIDIA block in a P9 CPU) needs to
> >>>> know LPID (a logical partition ID or a KVM guest hardware ID in other
> >>>> words) and PID (a memory context ID of an userspace process, not to be
> >>>> confused with a linux pid). This assigns a GPU to LPID in the NPU and
> >>>> this is why this adds a listener for KVM on an IOMMU group. A PID comes
> >>>> via NVLink from a GPU and NPU uses a PID wildcard to pass it through.
> >>>>
> >>>> This requires coherent memory and ATSD to be available on the host as
> >>>> the GPU vendor only supports configurations with both features enabled
> >>>> and other configurations are known not to work. Because of this and
> >>>> because of the ways the features are advertised to the host system
> >>>> (which is a device tree with very platform specific properties),
> >>>> this requires enabled POWERNV platform.
> >>>>
> >>>> This hardcodes the NVLink2 support for specific vendor and device IDs
> >>>> as there is no reliable way of knowing about coherent memory and ATS
> >>>> support. The GPU has an unique vendor PCIe capability 0x23 but it was
> >>>> confirmed that it does not provide required information (and it is still
> >>>> undisclosed what it actually does).
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>> drivers/vfio/pci/Makefile | 1 +
> >>>> drivers/vfio/pci/vfio_pci_private.h | 2 +
> >>>> include/uapi/linux/vfio.h | 18 ++
> >>>> drivers/vfio/pci/vfio_pci.c | 37 +++-
> >>>> drivers/vfio/pci/vfio_pci_nvlink2.c | 409 ++++++++++++++++++++++++++++++++++++
> >>>> drivers/vfio/pci/Kconfig | 4 +
> >>>> 6 files changed, 469 insertions(+), 2 deletions(-)
> >>>> create mode 100644 drivers/vfio/pci/vfio_pci_nvlink2.c
> >>>>
> >>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>> index 76d8ec0..9662c06 100644
> >>>> --- a/drivers/vfio/pci/Makefile
> >>>> +++ b/drivers/vfio/pci/Makefile
> >>>> @@ -1,5 +1,6 @@
> >>>>
> >>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>> +vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
> >>>>
> >>>> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> >>>> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> >>>> index 93c1738..7639241 100644
> >>>> --- a/drivers/vfio/pci/vfio_pci_private.h
> >>>> +++ b/drivers/vfio/pci/vfio_pci_private.h
> >>>> @@ -163,4 +163,6 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> >>>> return -ENODEV;
> >>>> }
> >>>> #endif
> >>>> +extern int vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev);
> >>>> +extern int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev);
> >>>> #endif /* VFIO_PCI_PRIVATE_H */
> >>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >>>> index f378b98..9e9a8d3 100644
> >>>> --- a/include/uapi/linux/vfio.h
> >>>> +++ b/include/uapi/linux/vfio.h
> >>>> @@ -303,6 +303,12 @@ struct vfio_region_info_cap_type {
> >>>> #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2)
> >>>> #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3)
> >>>>
> >>>> +/* NVIDIA GPU NVlink2 RAM */
> >>>> +#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM (1)
> >>>> +
> >>>> +/* IBM NPU NVlink2 ATSD */
> >>>> +#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1)
> >>>> +
> >>>
> >>> Please include some of the description in the commitlog here for
> >>> reference. Also please be explicit that these are vendor defined
> >>> regions and note the numerical vendor ID associated with them.
> >>
> >> These are PCI region subtypes which are not from any PCI spec and which
> >> we define as we like, are not they all "vendor"?
> >
> > No, it's not entirely obvious that they're vendor regions, I had to
> > look at the usage later in the patch. See linux-next where Gerd has
> > added an EDID region which is not a vendor region. You make use of
> > these by masking VFIO_REGION_TYPE_PCI_VENDOR_TYPE into the type, which
> > indicates they are a PCI vendor type region, which means they are
> > associated to a specific vendor. That vendor should be explicitly,
> > numerically, indicated (some hardware vendors own multiple PCI vendor
> > IDs or may acquire more).
>
> Ah, I see now. ok.
>
>
> >>>> /*
> >>>> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
> >>>> * which allows direct access to non-MSIX registers which happened to be within
> >>>> @@ -313,6 +319,18 @@ struct vfio_region_info_cap_type {
> >>>> */
> >>>> #define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE 3
> >>>>
> >>>> +/*
> >>>> + * Capability with compressed real address (aka SSA - small system address)
> >>>> + * where GPU RAM is mapped on a system bus. Used by a GPU for DMA routing.
> >>>> + */
> >>>> +#define VFIO_REGION_INFO_CAP_NPU2 4
> >>>> +
> >>>> +struct vfio_region_info_cap_npu2 {
> >>>> + struct vfio_info_cap_header header;
> >>>> + __u64 tgt;
> >>>> + /* size is defined in VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM */
> >>>
> >>> But this is a capability for the IBM_NVLINK2_ATSD? What is the
> >>> relevance of this comment? Is this capability relevant to the RAM or
> >>> ATSD?
> >>
> >> It is relevant to NPU (NVLink host bus adapter of POWER9) which maps the
> >> GPU RAM to the system bus and acts as a proxy to nestMMU (NVIDIA's unit
> >> in POWER9 CPU) for ATS/ATSD services so it is a property of NPU. But
> >> then one might ask "wait, here is the address, where is the size then",
> >> hence the comment...
> >
> > So the tgt field within the npu2 capability of the ATSD region on the
> > NPU device describes the GPU internal address of the GPU RAM which is
> > described by the NVLINK2 RAM region on the GPU device... *twitch* What
> > business does the NPU have exposing this piece of information and how
> > is it related to the ATSD region/register?
>
> NPU is the source of the information on the host. ATSD is just another
> feature of NPU.
>
> > Is this tgt base used in
> > the process of doing address translation shootdowns?
>
> No, only routing.
>
>
> >>>> +};
> >>>> +
> >>>> /**
> >>>> * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
> >>>> * struct vfio_irq_info)
> >>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >>>> index 4a3b93e..e9afd43 100644
> >>>> --- a/drivers/vfio/pci/vfio_pci.c
> >>>> +++ b/drivers/vfio/pci/vfio_pci.c
> >>>> @@ -224,6 +224,16 @@ static bool vfio_pci_nointx(struct pci_dev *pdev)
> >>>> return false;
> >>>> }
> >>>>
> >>>> +int __weak vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev)
> >>>> +{
> >>>> + return -ENODEV;
> >>>> +}
> >>>> +
> >>>> +int __weak vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
> >>>> +{
> >>>> + return -ENODEV;
> >>>> +}
> >>>> +
> >>>> static int vfio_pci_enable(struct vfio_pci_device *vdev)
> >>>> {
> >>>> struct pci_dev *pdev = vdev->pdev;
> >>>> @@ -302,14 +312,37 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> >>>> if (ret) {
> >>>> dev_warn(&vdev->pdev->dev,
> >>>> "Failed to setup Intel IGD regions\n");
> >>>> - vfio_pci_disable(vdev);
> >>>> - return ret;
> >>>> + goto disable_exit;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
> >>>> + pdev->device == 0x1db1) {
> >>>> + ret = vfio_pci_nvdia_v100_nvlink2_init(vdev);
> >>>> + if (ret) {
> >>>> + dev_warn(&vdev->pdev->dev,
> >>>> + "Failed to setup NVIDIA NV2 RAM region\n");
> >>>> + goto disable_exit;
> >>>> + }
> >>>> + }
> >>>
> >>> This device ID is not unique to POWER9 Witherspoon systems, I see your
> >>> comment in the commitlog, but this is clearly going to generate a
> >>> dev_warn and failure on an x86 system with the same hardware. Perhaps
> >>> this could be masked off with IS_ENABLED(CONFIG_VFIO_PCI_NVLINK2) like
> >>> the IGD code above this chunk does?
> >>
> >> Right, will fix.
> >>
> >>
> >>>> +
> >>>> + if (pdev->vendor == PCI_VENDOR_ID_IBM &&
> >>>> + pdev->device == 0x04ea) {
> >>>> + ret = vfio_pci_ibm_npu2_init(vdev);
> >>>> + if (ret) {
> >>>> + dev_warn(&vdev->pdev->dev,
> >>>> + "Failed to setup NVIDIA NV2 ATSD region\n");
> >>>> + goto disable_exit;
> >>>> }
> >>>
> >>> So the NPU is also actually owned by vfio-pci and assigned to the VM?
> >>
> >> Yes. On a running system it looks like:
> >>
> >> 0007:00:00.0 Bridge: IBM Device 04ea (rev 01)
> >> 0007:00:00.1 Bridge: IBM Device 04ea (rev 01)
> >> 0007:00:01.0 Bridge: IBM Device 04ea (rev 01)
> >> 0007:00:01.1 Bridge: IBM Device 04ea (rev 01)
> >> 0007:00:02.0 Bridge: IBM Device 04ea (rev 01)
> >> 0007:00:02.1 Bridge: IBM Device 04ea (rev 01)
> >> 0035:00:00.0 PCI bridge: IBM Device 04c1
> >> 0035:01:00.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >> 0035:02:04.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >> 0035:02:05.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >> 0035:02:0d.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >> 0035:03:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >> (rev a1
> >> 0035:04:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >> (rev a1)
> >> 0035:05:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >> (rev a1)
> >>
> >> One "IBM Device" bridge represents one NVLink2, i.e. a piece of NPU.
> >> They all and 3 GPUs go to the same IOMMU group and get passed through to
> >> a guest.
> >>
> >> The entire NPU does not have representation via sysfs as a whole though.
> >
> > So the NPU is a bridge, but it uses a normal header type so vfio-pci
> > will bind to it?
>
> An NPU is a NVLink bridge, it is not PCI in any sense. We (the host
> powerpc firmware known as "skiboot" or "opal") have chosen to emulate a
> virtual bridge per 1 NVLink on the firmware level. So for each physical
> NPU there are 6 virtual bridges. So the NVIDIA driver does not need to
> know much about NPUs.
>
> > And the ATSD register that we need on it is not
> > accessible through these PCI representations of the sub-pieces of the
> > NPU? Thanks,
>
> No, only via the device tree. The skiboot puts the ATSD register address
> to the PHB's DT property called 'ibm,mmio-atsd' of these virtual bridges.
Ok, so the NPU is essential a virtual device already, mostly just a
stub. But it seems that each NPU is associated to a specific GPU, how
is that association done? In the use case here it seems like it's just
a vehicle to provide this ibm,mmio-atsd property to guest DT and the tgt
routing information to the GPU. So if both of those were attached to
the GPU, there'd be no purpose in assigning the NPU other than it's in
the same IOMMU group with a type 0 header, so something needs to be
done with it. If it's a virtual device, perhaps it could have a type 1
header so vfio wouldn't care about it, then we would only assign the
GPU with these extra properties, which seems easier for management
tools and users. If the guest driver needs a visible NPU device, QEMU
could possibly emulate one to make the GPU association work
automatically. Maybe this isn't really a problem, but I wonder if
you've looked up the management stack to see what tools need to know to
assign these NPU devices and whether specific configurations are
required to make the NPU to GPU association work. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v4 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: Rob Herring @ 2018-10-18 17:03 UTC (permalink / raw)
To: frowand.list
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <1539657458-24401-2-git-send-email-frowand.list@gmail.com>
On Mon, Oct 15, 2018 at 07:37:21PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> Add checks:
> - attempted kfree due to refcount reaching zero before overlay
> is removed
> - properties linked to an overlay node when the node is removed
> - node refcount > one during node removal in a changeset destroy,
> if the node was created by the changeset
>
> After applying this patch, several validation warnings will be
> reported from the devicetree unittest during boot due to
> pre-existing devicetree bugs. The warnings will be similar to:
>
> OF: ERROR: of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest11/test-unittest111 contains unexpected properties
> OF: ERROR: memory leak - destroy cset entry: attach overlay node /testcase-data-2/substation@100/hvac-medium-2 expected refcount 1 instead of 2. of_node_get() / of_node_put() are unbalanced for this node.
These messages could be formatted more consistently. Put the path either
at the beginning (after any prefix) or end. Beginning is more like a
compiler error. End puts what the problem is before it's off the edge of
the screen.
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> Changes since v3:
> - Add expected value of refcount for destroy cset entry error. Also
> explain the cause of the error.
>
> drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
> drivers/of/overlay.c | 1 +
> include/linux/of.h | 15 ++++++++++-----
> 3 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index f4f8ed9b5454..24c97b7a050f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
> if (!of_node_check_flag(node, OF_DYNAMIC))
> return;
>
> + if (of_node_check_flag(node, OF_OVERLAY)) {
> +
> + if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
I worry the flags are getting unwieldy.
> + /* premature refcount of zero, do not free memory */
> + pr_err("ERROR: memory leak %s() overlay node %pOF before free overlay changeset\n",
> + __func__, node);
> + return;
> + }
> +
> + /*
> + * If node->properties non-empty then properties were added
> + * to this node either by different overlay that has not
> + * yet been removed, or by a non-overlay mechanism.
> + */
> + if (node->properties)
> + pr_err("ERROR: %s() overlay node %pOF contains unexpected properties\n",
> + __func__, node);
> + }
> +
> property_list_free(node->properties);
> property_list_free(node->deadprops);
>
> @@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
>
> static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
> {
> + if (ce->action == OF_RECONFIG_ATTACH_NODE &&
> + of_node_check_flag(ce->np, OF_OVERLAY)) {
> + if (kref_read(&ce->np->kobj.kref) > 1) {
> + pr_err("ERROR: memory leak - destroy cset entry: attach overlay node %pOF expected refcount 1 instead of %d. of_node_get() / of_node_put() are unbalanced for this node.\n",
> + ce->np, kref_read(&ce->np->kobj.kref));
> + } else {
> + of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
> + }
> + }
> +
> of_node_put(ce->np);
> list_del(&ce->node);
> kfree(ce);
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index eda57ef12fd0..1176cb4b6e4e 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -373,6 +373,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
> return -ENOMEM;
>
> tchild->parent = target_node;
> + of_node_set_flag(tchild, OF_OVERLAY);
>
> ret = of_changeset_attach_node(&ovcs->cset, tchild);
> if (ret)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 4d25e4f952d9..aa1dafaec6ae 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }
> extern struct device_node *of_stdout;
> extern raw_spinlock_t devtree_lock;
>
> -/* flag descriptions (need to be visible even when !CONFIG_OF) */
> -#define OF_DYNAMIC 1 /* node and properties were allocated via kmalloc */
> -#define OF_DETACHED 2 /* node has been detached from the device tree */
> -#define OF_POPULATED 3 /* device already created for the node */
> -#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children of this node */
> +/*
> + * struct device_node flag descriptions
> + * (need to be visible even when !CONFIG_OF)
> + */
> +#define OF_DYNAMIC 1 /* (and properties) allocated via kmalloc */
> +#define OF_DETACHED 2 /* detached from the device tree */
> +#define OF_POPULATED 3 /* device already created */
> +#define OF_POPULATED_BUS 4 /* platform bus created for children */
> +#define OF_OVERLAY 5 /* allocated for an overlay */
> +#define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */
>
> #define OF_BAD_ADDR ((u64)-1)
>
> --
> Frank Rowand <frank.rowand@sony.com>
>
^ permalink raw reply
* Re: [PATCH v4 02/18] of: overlay: add missing of_node_put() after add new node to changeset
From: Rob Herring @ 2018-10-18 17:05 UTC (permalink / raw)
To: frowand.list
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <1539657458-24401-3-git-send-email-frowand.list@gmail.com>
On Mon, Oct 15, 2018 at 07:37:22PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> The refcount of a newly added overlay node decrements to one
> (instead of zero) when the overlay changeset is destroyed. This
> change will cause the final decrement be to zero.
>
> After applying this patch, new validation warnings will be
> reported from the devicetree unittest during boot due to
> a pre-existing devicetree bug. The warnings will be similar to:
>
> OF: ERROR: memory leak of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest4 before free overlay changeset
Same comment on formatting.
>
> This pre-existing devicetree bug will also trigger a WARN_ONCE() from
> refcount_sub_and_test_checked() when an overlay changeset is
> destroyed without having first been applied. This scenario occurs
> when an error in the overlay is detected during the overlay changeset
> creation:
>
> WARNING: CPU: 0 PID: 1 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa8/0xbc
> refcount_t: underflow; use-after-free.
>
> (unwind_backtrace) from (show_stack+0x10/0x14)
> (show_stack) from (dump_stack+0x6c/0x8c)
> (dump_stack) from (__warn+0xdc/0x104)
> (__warn) from (warn_slowpath_fmt+0x44/0x6c)
> (warn_slowpath_fmt) from (refcount_sub_and_test_checked+0xa8/0xbc)
> (refcount_sub_and_test_checked) from (kobject_put+0x24/0x208)
> (kobject_put) from (of_changeset_destroy+0x2c/0xb4)
> (of_changeset_destroy) from (free_overlay_changeset+0x1c/0x9c)
> (free_overlay_changeset) from (of_overlay_remove+0x284/0x2cc)
> (of_overlay_remove) from (of_unittest_apply_revert_overlay_check.constprop.4+0xf8/0x1e8)
> (of_unittest_apply_revert_overlay_check.constprop.4) from (of_unittest_overlay+0x960/0xed8)
> (of_unittest_overlay) from (of_unittest+0x1cc4/0x2138)
> (of_unittest) from (do_one_initcall+0x4c/0x28c)
> (do_one_initcall) from (kernel_init_freeable+0x29c/0x378)
> (kernel_init_freeable) from (kernel_init+0x8/0x110)
> (kernel_init) from (ret_from_fork+0x14/0x2c)
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> drivers/of/overlay.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 1176cb4b6e4e..32cfee68f2e3 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -379,7 +379,9 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
> if (ret)
> return ret;
>
> - return build_changeset_next_level(ovcs, tchild, node);
> + ret = build_changeset_next_level(ovcs, tchild, node);
> + of_node_put(tchild);
> + return ret;
> }
>
> if (node->phandle && tchild->phandle)
> --
> Frank Rowand <frank.rowand@sony.com>
>
^ permalink raw reply
* Re: [PATCH v4 04/18] powerpc/pseries: add of_node_put() in dlpar_detach_node()
From: Rob Herring @ 2018-10-18 17:09 UTC (permalink / raw)
To: frowand.list
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <1539657458-24401-5-git-send-email-frowand.list@gmail.com>
On Mon, Oct 15, 2018 at 07:37:24PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> "of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
> added a missing of_node_get() to __of_attach_node_sysfs(). This
> results in a refcount imbalance for nodes attached with
> dlpar_attach_node(). The calling sequence from dlpar_attach_node()
> to __of_attach_node_sysfs() is:
>
> dlpar_attach_node()
> of_attach_node()
> __of_attach_node_sysfs()
IIRC, there's a long standing item in the todo (Grant's) to convert the
open coded dlpar code. Maybe you want to do that first?
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> ***** UNTESTED. I need people with the affected PowerPC systems
> ***** (systems that dynamically allocate and deallocate
> ***** devicetree nodes) to test this patch.
Can't we write a test case that does the same thing?
>
> arch/powerpc/platforms/pseries/dlpar.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index a0b20c03f078..e3010b14aea5 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -272,6 +272,8 @@ int dlpar_detach_node(struct device_node *dn)
> if (rc)
> return rc;
>
> + of_node_put(dn);
> +
> return 0;
> }
>
> --
> Frank Rowand <frank.rowand@sony.com>
>
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Piotr Jaroszynski @ 2018-10-18 17:37 UTC (permalink / raw)
To: Alex Williamson, Alexey Kardashevskiy
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Reza Arbab,
David Gibson
In-Reply-To: <20181018105503.088a343f@w520.home>
On 10/18/18 9:55 AM, Alex Williamson wrote:
> On Thu, 18 Oct 2018 11:31:33 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>> On 18/10/2018 08:52, Alex Williamson wrote:
>>> On Wed, 17 Oct 2018 12:19:20 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 17/10/2018 06:08, Alex Williamson wrote:
>>>>> On Mon, 15 Oct 2018 20:42:33 +1100
>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>
>>>>>> POWER9 Witherspoon machines come with 4 or 6 V100 GPUs which are not
>>>>>> pluggable PCIe devices but implement PCIe links for config space and MMIO.
>>>>>> In addition to that the GPUs are interconnected to each other and also
>>>>>> have direct links to the P9 CPU. The links are NVLink2 and provide direct
>>>>>> access to the system RAM for GPUs via NPU (an NVLink2 "proxy" on P9 chip).
>>>>>> These systems also support ATS (address translation services) which is
>>>>>> a part of the NVLink2 prototol. Such GPUs also share on-board RAM
>>>>>> (16GB in tested config) to the system via the same NVLink2 so a CPU has
>>>>>> cache-coherent access to a GPU RAM.
>>>>>>
>>>>>> This exports GPU RAM to the userspace as a new PCI region. This
>>>>>> preregisters the new memory as device memory as it might be used for DMA.
>>>>>> This inserts pfns from the fault handler as the GPU memory is not onlined
>>>>>> until the NVIDIA driver is loaded and trained the links so doing this
>>>>>> earlier produces low level errors which we fence in the firmware so
>>>>>> it does not hurt the host system but still better to avoid.
>>>>>>
>>>>>> This exports ATSD (Address Translation Shootdown) register of NPU which
>>>>>> allows the guest to invalidate TLB. The register conviniently occupies
>>>>>> a single 64k page. Since NPU maps the GPU memory, it has a "tgt" property
>>>>>> (which is an abbreviated host system bus address). This exports the "tgt"
>>>>>> as a capability so the guest can program it into the GPU so the GPU can
>>>>>> know how to route DMA trafic.
>>>>>
>>>>> I'm not really following what "tgt" is and why it's needed. Is the GPU
>>>>> memory here different than the GPU RAM region above? Why does the user
>>>>> need the host system bus address of this "tgt" thing? Are we not able
>>>>> to relocate it in guest physical address space, does this shootdown
>>>>> only work in the host physical address space and therefore we need this
>>>>> offset? Please explain, I'm confused.
>>>>
>>>>
>>>> This "tgt" is made of:
>>>> - "memory select" (bits 45, 46)
>>>> - "group select" (bits 43, 44)
>>>> - "chip select" (bit 42)
>>>> - chip internal address (bits 0..41)
>>>>
>>>> These are internal to GPU and this is where GPU RAM is mapped into the
>>>> GPU's real space, this fits 46 bits.
>>>>
>>>> On POWER9 CPU the bits are different and higher so the same memory is
>>>> mapped higher on P9 CPU. Just because we can map it higher, I guess.
>>>>
>>>> So it is not exactly the address but this provides the exact physical
>>>> location of the memory.
>>>>
>>>> We have a group of 3 interconnected GPUs, they got their own
>>>> memory/group/chip numbers. The GPUs use ATS service to translate
>>>> userspace to physical (host or guest) addresses. Now a GPU needs to know
>>>> which specific link to use for a specific physical address, in other
>>>> words what this physical address belongs to - a CPU or one of GPUs. This
>>>> is when "tgt" is used by the GPU hardware.
>>>
>>> Clear as mud ;)
>>
>> /me is sad. I hope Piotr explained it better...
>
> It's starting to be a bit more clear, and Piotr anticipated the
> security questions I was mulling over.
Great.
>
>>> So tgt, provided by the npu2 capability of the ATSD
>>> region of the NPU tells the GPU (a completely separate device) how to
>>> route it its own RAM via its NVLink interface? How can one tgt
>>> indicate the routing for multiple interfaces?
>>
>> This NVLink DMA is using direct host physical addresses (no IOMMU, no
>> filtering) which come from ATS. So unless we tell the GPU its own
>> address range on the host CPU, it will route trafic via CPU. And the
>> driver can also discover the NVLink topology and tell each GPU physical
>> addresses of peer GPUs.
>
> I think this is a key point too, the tgt seems to only identify the
> routing for the GPU local RAM, but the guest driver is able to
> conglomerate this information so each GPU knows how to get directly to
> the other GPUs.
Yes.
>
>>>> A GPU could run all the DMA trafic via the system bus indeed, just not
>>>> as fast.
>>>>
>>>> I am also struggling here and adding an Nvidia person in cc: (I should
>>>> have done that when I posted the patches, my bad) to correct when/if I
>>>> am wrong.
>>>>
>>>>
>>>>
>>>>>
>>>>>> For ATS to work, the nest MMU (an NVIDIA block in a P9 CPU) needs to
>>>>>> know LPID (a logical partition ID or a KVM guest hardware ID in other
>>>>>> words) and PID (a memory context ID of an userspace process, not to be
>>>>>> confused with a linux pid). This assigns a GPU to LPID in the NPU and
>>>>>> this is why this adds a listener for KVM on an IOMMU group. A PID comes
>>>>>> via NVLink from a GPU and NPU uses a PID wildcard to pass it through.
>>>>>>
>>>>>> This requires coherent memory and ATSD to be available on the host as
>>>>>> the GPU vendor only supports configurations with both features enabled
>>>>>> and other configurations are known not to work. Because of this and
>>>>>> because of the ways the features are advertised to the host system
>>>>>> (which is a device tree with very platform specific properties),
>>>>>> this requires enabled POWERNV platform.
>>>>>>
>>>>>> This hardcodes the NVLink2 support for specific vendor and device IDs
>>>>>> as there is no reliable way of knowing about coherent memory and ATS
>>>>>> support. The GPU has an unique vendor PCIe capability 0x23 but it was
>>>>>> confirmed that it does not provide required information (and it is still
>>>>>> undisclosed what it actually does).
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> ---
>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>> drivers/vfio/pci/vfio_pci_private.h | 2 +
>>>>>> include/uapi/linux/vfio.h | 18 ++
>>>>>> drivers/vfio/pci/vfio_pci.c | 37 +++-
>>>>>> drivers/vfio/pci/vfio_pci_nvlink2.c | 409 ++++++++++++++++++++++++++++++++++++
>>>>>> drivers/vfio/pci/Kconfig | 4 +
>>>>>> 6 files changed, 469 insertions(+), 2 deletions(-)
>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_nvlink2.c
>>>>>>
>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>> index 76d8ec0..9662c06 100644
>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>> @@ -1,5 +1,6 @@
>>>>>>
>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>> +vfio-pci-$(CONFIG_VFIO_PCI_NVLINK2) += vfio_pci_nvlink2.o
>>>>>>
>>>>>> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>>>>>> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
>>>>>> index 93c1738..7639241 100644
>>>>>> --- a/drivers/vfio/pci/vfio_pci_private.h
>>>>>> +++ b/drivers/vfio/pci/vfio_pci_private.h
>>>>>> @@ -163,4 +163,6 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
>>>>>> return -ENODEV;
>>>>>> }
>>>>>> #endif
>>>>>> +extern int vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev);
>>>>>> +extern int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev);
>>>>>> #endif /* VFIO_PCI_PRIVATE_H */
>>>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>>> index f378b98..9e9a8d3 100644
>>>>>> --- a/include/uapi/linux/vfio.h
>>>>>> +++ b/include/uapi/linux/vfio.h
>>>>>> @@ -303,6 +303,12 @@ struct vfio_region_info_cap_type {
>>>>>> #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2)
>>>>>> #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3)
>>>>>>
>>>>>> +/* NVIDIA GPU NVlink2 RAM */
>>>>>> +#define VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM (1)
>>>>>> +
>>>>>> +/* IBM NPU NVlink2 ATSD */
>>>>>> +#define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD (1)
>>>>>> +
>>>>>
>>>>> Please include some of the description in the commitlog here for
>>>>> reference. Also please be explicit that these are vendor defined
>>>>> regions and note the numerical vendor ID associated with them.
>>>>
>>>> These are PCI region subtypes which are not from any PCI spec and which
>>>> we define as we like, are not they all "vendor"?
>>>
>>> No, it's not entirely obvious that they're vendor regions, I had to
>>> look at the usage later in the patch. See linux-next where Gerd has
>>> added an EDID region which is not a vendor region. You make use of
>>> these by masking VFIO_REGION_TYPE_PCI_VENDOR_TYPE into the type, which
>>> indicates they are a PCI vendor type region, which means they are
>>> associated to a specific vendor. That vendor should be explicitly,
>>> numerically, indicated (some hardware vendors own multiple PCI vendor
>>> IDs or may acquire more).
>>
>> Ah, I see now. ok.
>>
>>
>>>>>> /*
>>>>>> * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
>>>>>> * which allows direct access to non-MSIX registers which happened to be within
>>>>>> @@ -313,6 +319,18 @@ struct vfio_region_info_cap_type {
>>>>>> */
>>>>>> #define VFIO_REGION_INFO_CAP_MSIX_MAPPABLE 3
>>>>>>
>>>>>> +/*
>>>>>> + * Capability with compressed real address (aka SSA - small system address)
>>>>>> + * where GPU RAM is mapped on a system bus. Used by a GPU for DMA routing.
>>>>>> + */
>>>>>> +#define VFIO_REGION_INFO_CAP_NPU2 4
>>>>>> +
>>>>>> +struct vfio_region_info_cap_npu2 {
>>>>>> + struct vfio_info_cap_header header;
>>>>>> + __u64 tgt;
>>>>>> + /* size is defined in VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM */
>>>>>
>>>>> But this is a capability for the IBM_NVLINK2_ATSD? What is the
>>>>> relevance of this comment? Is this capability relevant to the RAM or
>>>>> ATSD?
>>>>
>>>> It is relevant to NPU (NVLink host bus adapter of POWER9) which maps the
>>>> GPU RAM to the system bus and acts as a proxy to nestMMU (NVIDIA's unit
>>>> in POWER9 CPU) for ATS/ATSD services so it is a property of NPU. But
>>>> then one might ask "wait, here is the address, where is the size then",
>>>> hence the comment...
>>>
>>> So the tgt field within the npu2 capability of the ATSD region on the
>>> NPU device describes the GPU internal address of the GPU RAM which is
>>> described by the NVLINK2 RAM region on the GPU device... *twitch* What
>>> business does the NPU have exposing this piece of information and how
>>> is it related to the ATSD region/register?
>>
>> NPU is the source of the information on the host. ATSD is just another
>> feature of NPU.
>>
>>> Is this tgt base used in
>>> the process of doing address translation shootdowns?
>>
>> No, only routing.
>>
>>
>>>>>> +};
>>>>>> +
>>>>>> /**
>>>>>> * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
>>>>>> * struct vfio_irq_info)
>>>>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>>>>> index 4a3b93e..e9afd43 100644
>>>>>> --- a/drivers/vfio/pci/vfio_pci.c
>>>>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>>>>> @@ -224,6 +224,16 @@ static bool vfio_pci_nointx(struct pci_dev *pdev)
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>> +int __weak vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev)
>>>>>> +{
>>>>>> + return -ENODEV;
>>>>>> +}
>>>>>> +
>>>>>> +int __weak vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
>>>>>> +{
>>>>>> + return -ENODEV;
>>>>>> +}
>>>>>> +
>>>>>> static int vfio_pci_enable(struct vfio_pci_device *vdev)
>>>>>> {
>>>>>> struct pci_dev *pdev = vdev->pdev;
>>>>>> @@ -302,14 +312,37 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>>>>>> if (ret) {
>>>>>> dev_warn(&vdev->pdev->dev,
>>>>>> "Failed to setup Intel IGD regions\n");
>>>>>> - vfio_pci_disable(vdev);
>>>>>> - return ret;
>>>>>> + goto disable_exit;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
>>>>>> + pdev->device == 0x1db1) {
>>>>>> + ret = vfio_pci_nvdia_v100_nvlink2_init(vdev);
>>>>>> + if (ret) {
>>>>>> + dev_warn(&vdev->pdev->dev,
>>>>>> + "Failed to setup NVIDIA NV2 RAM region\n");
>>>>>> + goto disable_exit;
>>>>>> + }
>>>>>> + }
>>>>>
>>>>> This device ID is not unique to POWER9 Witherspoon systems, I see your
>>>>> comment in the commitlog, but this is clearly going to generate a
>>>>> dev_warn and failure on an x86 system with the same hardware. Perhaps
>>>>> this could be masked off with IS_ENABLED(CONFIG_VFIO_PCI_NVLINK2) like
>>>>> the IGD code above this chunk does?
>>>>
>>>> Right, will fix.
>>>>
>>>>
>>>>>> +
>>>>>> + if (pdev->vendor == PCI_VENDOR_ID_IBM &&
>>>>>> + pdev->device == 0x04ea) {
>>>>>> + ret = vfio_pci_ibm_npu2_init(vdev);
>>>>>> + if (ret) {
>>>>>> + dev_warn(&vdev->pdev->dev,
>>>>>> + "Failed to setup NVIDIA NV2 ATSD region\n");
>>>>>> + goto disable_exit;
>>>>>> }
>>>>>
>>>>> So the NPU is also actually owned by vfio-pci and assigned to the VM?
>>>>
>>>> Yes. On a running system it looks like:
>>>>
>>>> 0007:00:00.0 Bridge: IBM Device 04ea (rev 01)
>>>> 0007:00:00.1 Bridge: IBM Device 04ea (rev 01)
>>>> 0007:00:01.0 Bridge: IBM Device 04ea (rev 01)
>>>> 0007:00:01.1 Bridge: IBM Device 04ea (rev 01)
>>>> 0007:00:02.0 Bridge: IBM Device 04ea (rev 01)
>>>> 0007:00:02.1 Bridge: IBM Device 04ea (rev 01)
>>>> 0035:00:00.0 PCI bridge: IBM Device 04c1
>>>> 0035:01:00.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>> 0035:02:04.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>> 0035:02:05.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>> 0035:02:0d.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>> 0035:03:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>> (rev a1
>>>> 0035:04:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>> (rev a1)
>>>> 0035:05:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>> (rev a1)
>>>>
>>>> One "IBM Device" bridge represents one NVLink2, i.e. a piece of NPU.
>>>> They all and 3 GPUs go to the same IOMMU group and get passed through to
>>>> a guest.
>>>>
>>>> The entire NPU does not have representation via sysfs as a whole though.
>>>
>>> So the NPU is a bridge, but it uses a normal header type so vfio-pci
>>> will bind to it?
>>
>> An NPU is a NVLink bridge, it is not PCI in any sense. We (the host
>> powerpc firmware known as "skiboot" or "opal") have chosen to emulate a
>> virtual bridge per 1 NVLink on the firmware level. So for each physical
>> NPU there are 6 virtual bridges. So the NVIDIA driver does not need to
>> know much about NPUs.
>>
>>> And the ATSD register that we need on it is not
>>> accessible through these PCI representations of the sub-pieces of the
>>> NPU? Thanks,
>>
>> No, only via the device tree. The skiboot puts the ATSD register address
>> to the PHB's DT property called 'ibm,mmio-atsd' of these virtual bridges.
>
> Ok, so the NPU is essential a virtual device already, mostly just a
> stub. But it seems that each NPU is associated to a specific GPU, how
> is that association done? In the use case here it seems like it's just
> a vehicle to provide this ibm,mmio-atsd property to guest DT and the tgt
> routing information to the GPU. So if both of those were attached to
> the GPU, there'd be no purpose in assigning the NPU other than it's in
> the same IOMMU group with a type 0 header, so something needs to be
> done with it. If it's a virtual device, perhaps it could have a type 1
> header so vfio wouldn't care about it, then we would only assign the
> GPU with these extra properties, which seems easier for management
> tools and users. If the guest driver needs a visible NPU device, QEMU
> could possibly emulate one to make the GPU association work
> automatically. Maybe this isn't really a problem, but I wonder if
> you've looked up the management stack to see what tools need to know to
> assign these NPU devices and whether specific configurations are
> required to make the NPU to GPU association work. Thanks,
I'm not that familiar with how this was originally set up, but note that
Alexey is just making it work exactly like baremetal does. The baremetal
GPU driver works as-is in the VM and expects the same properties in the
device-tree. Obviously it doesn't have to be that way, but there is
value in keeping it identical.
Another probably bigger point is that the NPU device also implements the
nvlink HW interface and is required for actually training and
maintaining the link up. The driver in the guest trains the links by
programming both the GPU end and the NPU end of each link so the NPU
device needs to be exposed to the guest.
Thanks,
Piotr
>
> Alex
>
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Alex Williamson @ 2018-10-18 18:05 UTC (permalink / raw)
To: Piotr Jaroszynski
Cc: Reza Arbab, kvm, Alexey Kardashevskiy, Alistair Popple, kvm-ppc,
linuxppc-dev, David Gibson
In-Reply-To: <0e0db29d-a1e8-af85-b715-c1ba1a2f3875@nvidia.com>
On Thu, 18 Oct 2018 10:37:46 -0700
Piotr Jaroszynski <pjaroszynski@nvidia.com> wrote:
> On 10/18/18 9:55 AM, Alex Williamson wrote:
> > On Thu, 18 Oct 2018 11:31:33 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> >> On 18/10/2018 08:52, Alex Williamson wrote:
> >>> On Wed, 17 Oct 2018 12:19:20 +1100
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> On 17/10/2018 06:08, Alex Williamson wrote:
> >>>>> On Mon, 15 Oct 2018 20:42:33 +1100
> >>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>>>> +
> >>>>>> + if (pdev->vendor == PCI_VENDOR_ID_IBM &&
> >>>>>> + pdev->device == 0x04ea) {
> >>>>>> + ret = vfio_pci_ibm_npu2_init(vdev);
> >>>>>> + if (ret) {
> >>>>>> + dev_warn(&vdev->pdev->dev,
> >>>>>> + "Failed to setup NVIDIA NV2 ATSD region\n");
> >>>>>> + goto disable_exit;
> >>>>>> }
> >>>>>
> >>>>> So the NPU is also actually owned by vfio-pci and assigned to the VM?
> >>>>
> >>>> Yes. On a running system it looks like:
> >>>>
> >>>> 0007:00:00.0 Bridge: IBM Device 04ea (rev 01)
> >>>> 0007:00:00.1 Bridge: IBM Device 04ea (rev 01)
> >>>> 0007:00:01.0 Bridge: IBM Device 04ea (rev 01)
> >>>> 0007:00:01.1 Bridge: IBM Device 04ea (rev 01)
> >>>> 0007:00:02.0 Bridge: IBM Device 04ea (rev 01)
> >>>> 0007:00:02.1 Bridge: IBM Device 04ea (rev 01)
> >>>> 0035:00:00.0 PCI bridge: IBM Device 04c1
> >>>> 0035:01:00.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >>>> 0035:02:04.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >>>> 0035:02:05.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >>>> 0035:02:0d.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
> >>>> 0035:03:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >>>> (rev a1
> >>>> 0035:04:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >>>> (rev a1)
> >>>> 0035:05:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
> >>>> (rev a1)
> >>>>
> >>>> One "IBM Device" bridge represents one NVLink2, i.e. a piece of NPU.
> >>>> They all and 3 GPUs go to the same IOMMU group and get passed through to
> >>>> a guest.
> >>>>
> >>>> The entire NPU does not have representation via sysfs as a whole though.
> >>>
> >>> So the NPU is a bridge, but it uses a normal header type so vfio-pci
> >>> will bind to it?
> >>
> >> An NPU is a NVLink bridge, it is not PCI in any sense. We (the host
> >> powerpc firmware known as "skiboot" or "opal") have chosen to emulate a
> >> virtual bridge per 1 NVLink on the firmware level. So for each physical
> >> NPU there are 6 virtual bridges. So the NVIDIA driver does not need to
> >> know much about NPUs.
> >>
> >>> And the ATSD register that we need on it is not
> >>> accessible through these PCI representations of the sub-pieces of the
> >>> NPU? Thanks,
> >>
> >> No, only via the device tree. The skiboot puts the ATSD register address
> >> to the PHB's DT property called 'ibm,mmio-atsd' of these virtual bridges.
> >
> > Ok, so the NPU is essential a virtual device already, mostly just a
> > stub. But it seems that each NPU is associated to a specific GPU, how
> > is that association done? In the use case here it seems like it's just
> > a vehicle to provide this ibm,mmio-atsd property to guest DT and the tgt
> > routing information to the GPU. So if both of those were attached to
> > the GPU, there'd be no purpose in assigning the NPU other than it's in
> > the same IOMMU group with a type 0 header, so something needs to be
> > done with it. If it's a virtual device, perhaps it could have a type 1
> > header so vfio wouldn't care about it, then we would only assign the
> > GPU with these extra properties, which seems easier for management
> > tools and users. If the guest driver needs a visible NPU device, QEMU
> > could possibly emulate one to make the GPU association work
> > automatically. Maybe this isn't really a problem, but I wonder if
> > you've looked up the management stack to see what tools need to know to
> > assign these NPU devices and whether specific configurations are
> > required to make the NPU to GPU association work. Thanks,
>
> I'm not that familiar with how this was originally set up, but note that
> Alexey is just making it work exactly like baremetal does. The baremetal
> GPU driver works as-is in the VM and expects the same properties in the
> device-tree. Obviously it doesn't have to be that way, but there is
> value in keeping it identical.
>
> Another probably bigger point is that the NPU device also implements the
> nvlink HW interface and is required for actually training and
> maintaining the link up. The driver in the guest trains the links by
> programming both the GPU end and the NPU end of each link so the NPU
> device needs to be exposed to the guest.
Ok, so there is functionality in assigning the NPU device itself, it's
not just an attachment point for meta data. But it still seems there
must be some association of NPU to GPU, the tgt address seems to pair
the NPU with a specific GPU, they're not simply a fungible set of NPUs
and GPUs. Is that association explicit anywhere or is it related to
the topology or device numbering that needs to match between the host
and guest? Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v4 09/18] of: overlay: validate overlay properties #address-cells and #size-cells
From: Rob Herring @ 2018-10-18 18:13 UTC (permalink / raw)
To: frowand.list
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <1539657458-24401-10-git-send-email-frowand.list@gmail.com>
On Mon, Oct 15, 2018 at 07:37:29PM -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> If overlay properties #address-cells or #size-cells are already in
> the live devicetree for any given node, then the values in the
> overlay must match the values in the live tree.
>
> If the properties are already in the live tree then there is no
> need to create a changeset entry to add them since they must
> have the same value. This reduces the memory used by the
> changeset and eliminates a possible memory leak. This is
> verified by 12 fewer warnings during the devicetree unittest,
> as the possible memory leak warnings about #address-cells and
Still missing the rest...
And what about my other comments too?
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> Changes since v3:
> - for errors of an overlay changing the value of #size-cells or
> #address-cells, return -EINVAL so that overlay apply will fail
> - for errors of an overlay changing the value of #size-cells or
> #address-cells, make the message more direct.
> Old message:
> OF: overlay: ERROR: overlay and/or live tree #size-cells invalid in node /soc/base_fpga_region
> New message:
> OF: overlay: ERROR: changing value of /soc/base_fpga_region/#size-cells not allowed
>
> drivers/of/overlay.c | 42 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Piotr Jaroszynski @ 2018-10-18 18:40 UTC (permalink / raw)
To: Alex Williamson
Cc: Reza Arbab, kvm, Alexey Kardashevskiy, Alistair Popple, kvm-ppc,
linuxppc-dev, David Gibson
In-Reply-To: <20181018120502.057feb7a@w520.home>
On 10/18/18 11:05 AM, Alex Williamson wrote:
> On Thu, 18 Oct 2018 10:37:46 -0700
> Piotr Jaroszynski <pjaroszynski@nvidia.com> wrote:
>
>> On 10/18/18 9:55 AM, Alex Williamson wrote:
>>> On Thu, 18 Oct 2018 11:31:33 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 18/10/2018 08:52, Alex Williamson wrote:
>>>>> On Wed, 17 Oct 2018 12:19:20 +1100
>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>
>>>>>> On 17/10/2018 06:08, Alex Williamson wrote:
>>>>>>> On Mon, 15 Oct 2018 20:42:33 +1100
>>>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>>>>>> +
>>>>>>>> + if (pdev->vendor == PCI_VENDOR_ID_IBM &&
>>>>>>>> + pdev->device == 0x04ea) {
>>>>>>>> + ret = vfio_pci_ibm_npu2_init(vdev);
>>>>>>>> + if (ret) {
>>>>>>>> + dev_warn(&vdev->pdev->dev,
>>>>>>>> + "Failed to setup NVIDIA NV2 ATSD region\n");
>>>>>>>> + goto disable_exit;
>>>>>>>> }
>>>>>>>
>>>>>>> So the NPU is also actually owned by vfio-pci and assigned to the VM?
>>>>>>
>>>>>> Yes. On a running system it looks like:
>>>>>>
>>>>>> 0007:00:00.0 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0007:00:00.1 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0007:00:01.0 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0007:00:01.1 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0007:00:02.0 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0007:00:02.1 Bridge: IBM Device 04ea (rev 01)
>>>>>> 0035:00:00.0 PCI bridge: IBM Device 04c1
>>>>>> 0035:01:00.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>>>> 0035:02:04.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>>>> 0035:02:05.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>>>> 0035:02:0d.0 PCI bridge: PLX Technology, Inc. Device 8725 (rev ca)
>>>>>> 0035:03:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>>>> (rev a1
>>>>>> 0035:04:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>>>> (rev a1)
>>>>>> 0035:05:00.0 3D controller: NVIDIA Corporation GV100GL [Tesla V100 SXM2]
>>>>>> (rev a1)
>>>>>>
>>>>>> One "IBM Device" bridge represents one NVLink2, i.e. a piece of NPU.
>>>>>> They all and 3 GPUs go to the same IOMMU group and get passed through to
>>>>>> a guest.
>>>>>>
>>>>>> The entire NPU does not have representation via sysfs as a whole though.
>>>>>
>>>>> So the NPU is a bridge, but it uses a normal header type so vfio-pci
>>>>> will bind to it?
>>>>
>>>> An NPU is a NVLink bridge, it is not PCI in any sense. We (the host
>>>> powerpc firmware known as "skiboot" or "opal") have chosen to emulate a
>>>> virtual bridge per 1 NVLink on the firmware level. So for each physical
>>>> NPU there are 6 virtual bridges. So the NVIDIA driver does not need to
>>>> know much about NPUs.
>>>>
>>>>> And the ATSD register that we need on it is not
>>>>> accessible through these PCI representations of the sub-pieces of the
>>>>> NPU? Thanks,
>>>>
>>>> No, only via the device tree. The skiboot puts the ATSD register address
>>>> to the PHB's DT property called 'ibm,mmio-atsd' of these virtual bridges.
>>>
>>> Ok, so the NPU is essential a virtual device already, mostly just a
>>> stub. But it seems that each NPU is associated to a specific GPU, how
>>> is that association done? In the use case here it seems like it's just
>>> a vehicle to provide this ibm,mmio-atsd property to guest DT and the tgt
>>> routing information to the GPU. So if both of those were attached to
>>> the GPU, there'd be no purpose in assigning the NPU other than it's in
>>> the same IOMMU group with a type 0 header, so something needs to be
>>> done with it. If it's a virtual device, perhaps it could have a type 1
>>> header so vfio wouldn't care about it, then we would only assign the
>>> GPU with these extra properties, which seems easier for management
>>> tools and users. If the guest driver needs a visible NPU device, QEMU
>>> could possibly emulate one to make the GPU association work
>>> automatically. Maybe this isn't really a problem, but I wonder if
>>> you've looked up the management stack to see what tools need to know to
>>> assign these NPU devices and whether specific configurations are
>>> required to make the NPU to GPU association work. Thanks,
>>
>> I'm not that familiar with how this was originally set up, but note that
>> Alexey is just making it work exactly like baremetal does. The baremetal
>> GPU driver works as-is in the VM and expects the same properties in the
>> device-tree. Obviously it doesn't have to be that way, but there is
>> value in keeping it identical.
>>
>> Another probably bigger point is that the NPU device also implements the
>> nvlink HW interface and is required for actually training and
>> maintaining the link up. The driver in the guest trains the links by
>> programming both the GPU end and the NPU end of each link so the NPU
>> device needs to be exposed to the guest.
>
> Ok, so there is functionality in assigning the NPU device itself, it's
> not just an attachment point for meta data. But it still seems there
> must be some association of NPU to GPU, the tgt address seems to pair
> the NPU with a specific GPU, they're not simply a fungible set of NPUs
> and GPUs. Is that association explicit anywhere or is it related to
> the topology or device numbering that needs to match between the host
> and guest? Thanks,
GPUs are linked to NPU devices through device tree properties, I think.
Linux has a helper to look up linked NPU devices for a PCI device
pnv_pci_get_npu_dev() here:
https://elixir.bootlin.com/linux/latest/source/arch/powerpc/platforms/powernv/npu-dma.c#L86
From what Alexey said, it sounds like the NPU devices are actually
virtualized by the firmware, but they do map to real hardware in the end
and each GPU needs to be accompanied by the same NPU devices as on
baremetal. Currently, there are two types of P9+GV100 systems, one with
2 CPU<->GPU links per GPU and hence 2 linked NPU devices per GPU. And a
second system with 3 CPU<->GPU links per GPU and hence 3 linked NPU
devices per GPU.
Having the user assign all the NPUs and GPUs correctly does seem a bit
cumbersome, but maybe higher level tools on top of qemu can simplify
that somehow by pulling them in automatically? FWIW, whenever a GPU is
assigned to a guest, its linked NPU devices are likely not very useful
to anything else so grouping them together somehow would make sense.
>
> Alex
>
^ permalink raw reply
* Re: [PATCH v4 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: Frank Rowand @ 2018-10-18 19:01 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20181018170303.GA15557@bogus>
On 10/18/18 10:03, Rob Herring wrote:
> On Mon, Oct 15, 2018 at 07:37:21PM -0700, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Add checks:
>> - attempted kfree due to refcount reaching zero before overlay
>> is removed
>> - properties linked to an overlay node when the node is removed
>> - node refcount > one during node removal in a changeset destroy,
>> if the node was created by the changeset
>>
>> After applying this patch, several validation warnings will be
>> reported from the devicetree unittest during boot due to
>> pre-existing devicetree bugs. The warnings will be similar to:
>>
>> OF: ERROR: of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest11/test-unittest111 contains unexpected properties
>> OF: ERROR: memory leak - destroy cset entry: attach overlay node /testcase-data-2/substation@100/hvac-medium-2 expected refcount 1 instead of 2. of_node_get() / of_node_put() are unbalanced for this node.
>
> These messages could be formatted more consistently. Put the path either
> at the beginning (after any prefix) or end. Beginning is more like a
> compiler error. End puts what the problem is before it's off the edge of
> the screen.
The inconsistency makes the word flow more natural, but I agree that
consistency is more important. I think I can make all the messages
say the problem first, then provide the path at the end.
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>> Changes since v3:
>> - Add expected value of refcount for destroy cset entry error. Also
>> explain the cause of the error.
>>
>> drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
>> drivers/of/overlay.c | 1 +
>> include/linux/of.h | 15 ++++++++++-----
>> 3 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index f4f8ed9b5454..24c97b7a050f 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
>> if (!of_node_check_flag(node, OF_DYNAMIC))
>> return;
>>
>> + if (of_node_check_flag(node, OF_OVERLAY)) {
>> +
>> + if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
>
> I worry the flags are getting unwieldy.
I considered that. I think we are still ok, and I don't have a better
solution than adding flag values. (I did have some Rube Goldberg
variations.)
>
>> + /* premature refcount of zero, do not free memory */
>> + pr_err("ERROR: memory leak %s() overlay node %pOF before free overlay changeset\n",
>> + __func__, node);
>> + return;
>> + }
>> +
>> + /*
>> + * If node->properties non-empty then properties were added
>> + * to this node either by different overlay that has not
>> + * yet been removed, or by a non-overlay mechanism.
>> + */
>> + if (node->properties)
>> + pr_err("ERROR: %s() overlay node %pOF contains unexpected properties\n",
>> + __func__, node);
>> + }
>> +
>> property_list_free(node->properties);
>> property_list_free(node->deadprops);
>>
>> @@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
>>
>> static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
>> {
>> + if (ce->action == OF_RECONFIG_ATTACH_NODE &&
>> + of_node_check_flag(ce->np, OF_OVERLAY)) {
>> + if (kref_read(&ce->np->kobj.kref) > 1) {
>> + pr_err("ERROR: memory leak - destroy cset entry: attach overlay node %pOF expected refcount 1 instead of %d. of_node_get() / of_node_put() are unbalanced for this node.\n",
>> + ce->np, kref_read(&ce->np->kobj.kref));
>> + } else {
>> + of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
>> + }
>> + }
>> +
>> of_node_put(ce->np);
>> list_del(&ce->node);
>> kfree(ce);
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index eda57ef12fd0..1176cb4b6e4e 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -373,6 +373,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>> return -ENOMEM;
>>
>> tchild->parent = target_node;
>> + of_node_set_flag(tchild, OF_OVERLAY);
>>
>> ret = of_changeset_attach_node(&ovcs->cset, tchild);
>> if (ret)
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 4d25e4f952d9..aa1dafaec6ae 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }
>> extern struct device_node *of_stdout;
>> extern raw_spinlock_t devtree_lock;
>>
>> -/* flag descriptions (need to be visible even when !CONFIG_OF) */
>> -#define OF_DYNAMIC 1 /* node and properties were allocated via kmalloc */
>> -#define OF_DETACHED 2 /* node has been detached from the device tree */
>> -#define OF_POPULATED 3 /* device already created for the node */
>> -#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children of this node */
>> +/*
>> + * struct device_node flag descriptions
>> + * (need to be visible even when !CONFIG_OF)
>> + */
>> +#define OF_DYNAMIC 1 /* (and properties) allocated via kmalloc */
>> +#define OF_DETACHED 2 /* detached from the device tree */
>> +#define OF_POPULATED 3 /* device already created */
>> +#define OF_POPULATED_BUS 4 /* platform bus created for children */
>> +#define OF_OVERLAY 5 /* allocated for an overlay */
>> +#define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */
>>
>> #define OF_BAD_ADDR ((u64)-1)
>>
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>
>
^ permalink raw reply
* Re: [PATCH v4 02/18] of: overlay: add missing of_node_put() after add new node to changeset
From: Frank Rowand @ 2018-10-18 19:02 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20181018170502.GB15557@bogus>
On 10/18/18 10:05, Rob Herring wrote:
> On Mon, Oct 15, 2018 at 07:37:22PM -0700, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> The refcount of a newly added overlay node decrements to one
>> (instead of zero) when the overlay changeset is destroyed. This
>> change will cause the final decrement be to zero.
>>
>> After applying this patch, new validation warnings will be
>> reported from the devicetree unittest during boot due to
>> a pre-existing devicetree bug. The warnings will be similar to:
>>
>> OF: ERROR: memory leak of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest4 before free overlay changeset
>
> Same comment on formatting.
OK.
>>
>> This pre-existing devicetree bug will also trigger a WARN_ONCE() from
>> refcount_sub_and_test_checked() when an overlay changeset is
>> destroyed without having first been applied. This scenario occurs
>> when an error in the overlay is detected during the overlay changeset
>> creation:
>>
>> WARNING: CPU: 0 PID: 1 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa8/0xbc
>> refcount_t: underflow; use-after-free.
>>
>> (unwind_backtrace) from (show_stack+0x10/0x14)
>> (show_stack) from (dump_stack+0x6c/0x8c)
>> (dump_stack) from (__warn+0xdc/0x104)
>> (__warn) from (warn_slowpath_fmt+0x44/0x6c)
>> (warn_slowpath_fmt) from (refcount_sub_and_test_checked+0xa8/0xbc)
>> (refcount_sub_and_test_checked) from (kobject_put+0x24/0x208)
>> (kobject_put) from (of_changeset_destroy+0x2c/0xb4)
>> (of_changeset_destroy) from (free_overlay_changeset+0x1c/0x9c)
>> (free_overlay_changeset) from (of_overlay_remove+0x284/0x2cc)
>> (of_overlay_remove) from (of_unittest_apply_revert_overlay_check.constprop.4+0xf8/0x1e8)
>> (of_unittest_apply_revert_overlay_check.constprop.4) from (of_unittest_overlay+0x960/0xed8)
>> (of_unittest_overlay) from (of_unittest+0x1cc4/0x2138)
>> (of_unittest) from (do_one_initcall+0x4c/0x28c)
>> (do_one_initcall) from (kernel_init_freeable+0x29c/0x378)
>> (kernel_init_freeable) from (kernel_init+0x8/0x110)
>> (kernel_init) from (ret_from_fork+0x14/0x2c)
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>> drivers/of/overlay.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 1176cb4b6e4e..32cfee68f2e3 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -379,7 +379,9 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>> if (ret)
>> return ret;
>>
>> - return build_changeset_next_level(ovcs, tchild, node);
>> + ret = build_changeset_next_level(ovcs, tchild, node);
>> + of_node_put(tchild);
>> + return ret;
>> }
>>
>> if (node->phandle && tchild->phandle)
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>
>
^ permalink raw reply
* Re: [PATCH v4 04/18] powerpc/pseries: add of_node_put() in dlpar_detach_node()
From: Frank Rowand @ 2018-10-18 19:09 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20181018170931.GC15557@bogus>
On 10/18/18 10:09, Rob Herring wrote:
> On Mon, Oct 15, 2018 at 07:37:24PM -0700, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> "of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
>> added a missing of_node_get() to __of_attach_node_sysfs(). This
>> results in a refcount imbalance for nodes attached with
>> dlpar_attach_node(). The calling sequence from dlpar_attach_node()
>> to __of_attach_node_sysfs() is:
>>
>> dlpar_attach_node()
>> of_attach_node()
>> __of_attach_node_sysfs()
>
> IIRC, there's a long standing item in the todo (Grant's) to convert the
> open coded dlpar code. Maybe you want to do that first?
I'd like to avoid extra delays to getting the current (with necesary
fixes) series accepted because the series is rather intrusive and
could have conflicts with other patches.
I'm also worried that I don't have access to any of the systems that
use the dynamic overlay code, and I don't have any way to test the
changes.
Can we encourage the users of this code to convert the open coded
dlpar code?
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> ***** UNTESTED. I need people with the affected PowerPC systems
>> ***** (systems that dynamically allocate and deallocate
>> ***** devicetree nodes) to test this patch.
>
> Can't we write a test case that does the same thing?
I could write a simplistic test case, but I'm concerned that
simplistic is not anywhere near sufficient. And my test case
would reflect the same assumptions I already have when I
wrote this patch.
>>
>> arch/powerpc/platforms/pseries/dlpar.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
>> index a0b20c03f078..e3010b14aea5 100644
>> --- a/arch/powerpc/platforms/pseries/dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/dlpar.c
>> @@ -272,6 +272,8 @@ int dlpar_detach_node(struct device_node *dn)
>> if (rc)
>> return rc;
>>
>> + of_node_put(dn);
>> +
>> return 0;
>> }
>>
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>
>
^ permalink raw reply
* Re: [PATCH v4 09/18] of: overlay: validate overlay properties #address-cells and #size-cells
From: Frank Rowand @ 2018-10-18 19:13 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alan Tull, linux-fpga, Pantelis Antoniou,
linux-kernel, Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <20181018181338.GD15557@bogus>
On 10/18/18 11:13, Rob Herring wrote:
> On Mon, Oct 15, 2018 at 07:37:29PM -0700, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> If overlay properties #address-cells or #size-cells are already in
>> the live devicetree for any given node, then the values in the
>> overlay must match the values in the live tree.
>>
>> If the properties are already in the live tree then there is no
>> need to create a changeset entry to add them since they must
>> have the same value. This reduces the memory used by the
>> changeset and eliminates a possible memory leak. This is
>> verified by 12 fewer warnings during the devicetree unittest,
>> as the possible memory leak warnings about #address-cells and
>
> Still missing the rest...
>
> And what about my other comments too?
Apologies, paper bag. Will fix all.
-Frank
>
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>> Changes since v3:
>> - for errors of an overlay changing the value of #size-cells or
>> #address-cells, return -EINVAL so that overlay apply will fail
>> - for errors of an overlay changing the value of #size-cells or
>> #address-cells, make the message more direct.
>> Old message:
>> OF: overlay: ERROR: overlay and/or live tree #size-cells invalid in node /soc/base_fpga_region
>> New message:
>> OF: overlay: ERROR: changing value of /soc/base_fpga_region/#size-cells not allowed
>>
>> drivers/of/overlay.c | 42 +++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 39 insertions(+), 3 deletions(-)
>
^ permalink raw reply
* Regression: compile error on mpc83xx
From: Radu Rendec @ 2018-10-18 19:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood, Li Yang, Jia Hongtao
Hi everyone,
I'm getting the following compile error for mpc83xx in v4.9.115:
arch/powerpc/sysdev/built-in.o: In function `fsl_of_msi_probe':
fsl_msi.c:(.text+0x1548): undefined reference to `fsl_mpic_primary_get_version'
This seems to have been fixed in v3.12-rc1 by commit df1024ad8728. Then,
somewhere between v4.0 and v4.1 the following two commits changed the
arch/powerpc/include/asm/mpic.h file and that function again:
5e86bfde9cd9
807d38b73b63
After those two commits, it seems we're back to square one. I haven't
tested, but it looks like there haven't been any other changes to that
file up to the latest master in Linus' tree, so I believe the problem is
still there in the latest kernel.
Should commit df1024ad8728 be reapplied?
Thanks,
Radu Rendec
^ permalink raw reply
* Re: [PATCH v4 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: Alan Tull @ 2018-10-18 20:24 UTC (permalink / raw)
To: Frank Rowand
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-fpga, Pantelis Antoniou, linux-kernel, Rob Herring,
Moritz Fischer, Paul Mackerras, linuxppc-dev
In-Reply-To: <CANk1AXS-DMhA3=8v9N28K_V6K18HcgN_Esuo=-ddmeBLO46-Rw@mail.gmail.com>
On Wed, Oct 17, 2018 at 4:30 PM Alan Tull <atull@kernel.org> wrote:
>
> On Mon, Oct 15, 2018 at 9:39 PM <frowand.list@gmail.com> wrote:
>
> Hi Frank,
>
> >
> > From: Frank Rowand <frank.rowand@sony.com>
> >
> > Add checks:
> > - attempted kfree due to refcount reaching zero before overlay
> > is removed
> > - properties linked to an overlay node when the node is removed
> > - node refcount > one during node removal in a changeset destroy,
> > if the node was created by the changeset
> >
> > After applying this patch, several validation warnings will be
> > reported from the devicetree unittest during boot due to
> > pre-existing devicetree bugs. The warnings will be similar to:
> >
> > OF: ERROR: of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest11/test-unittest111 contains unexpected properties
> > OF: ERROR: memory leak - destroy cset entry: attach overlay node /testcase-data-2/substation@100/hvac-medium-2 expected refcount 1 instead of 2. of_node_get() / of_node_put() are unbalanced for this node.
> >
> > Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> > ---
> > Changes since v3:
> > - Add expected value of refcount for destroy cset entry error. Also
> > explain the cause of the error.
> >
> > drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
> > drivers/of/overlay.c | 1 +
> > include/linux/of.h | 15 ++++++++++-----
> > 3 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index f4f8ed9b5454..24c97b7a050f 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
> > if (!of_node_check_flag(node, OF_DYNAMIC))
> > return;
> >
> > + if (of_node_check_flag(node, OF_OVERLAY)) {
> > +
> > + if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
> > + /* premature refcount of zero, do not free memory */
> > + pr_err("ERROR: memory leak %s() overlay node %pOF before free overlay changeset\n",
> > + __func__, node);
> > + return;
> > + }
> > +
> > + /*
> > + * If node->properties non-empty then properties were added
> > + * to this node either by different overlay that has not
> > + * yet been removed, or by a non-overlay mechanism.
> > + */
> > + if (node->properties)
> > + pr_err("ERROR: %s() overlay node %pOF contains unexpected properties\n",
> > + __func__, node);
> > + }
> > +
> > property_list_free(node->properties);
> > property_list_free(node->deadprops);
> >
> > @@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
> >
> > static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
> > {
> > + if (ce->action == OF_RECONFIG_ATTACH_NODE &&
> > + of_node_check_flag(ce->np, OF_OVERLAY)) {
> > + if (kref_read(&ce->np->kobj.kref) > 1) {
> > + pr_err("ERROR: memory leak - destroy cset entry: attach overlay node %pOF expected refcount 1 instead of %d. of_node_get() / of_node_put() are unbalanced for this node.\n",
> > + ce->np, kref_read(&ce->np->kobj.kref));
>
> Still testing as much as I have time to do.
>
> I'm hitting this error message once when removing an overlay that adds
> several child nodes. The only node I get the message for was a node
> that added a fixed-clock (the other nodes didn't trigger the error).
> Then even if I edited all the rest of the overlay DTS and removed all
> other child nodes and all references to the clock from other nodes, I
> still got the error.
>
> Removing dtbo: 1-socfpga_arria10_socdk_sdmmc_ghrd_ovl_ext_cfg.dtb
> [ 72.032270] OF: ERROR: memory leak - destroy cset entry: attach
> overlay node /soc/base_fpga_region/clk_0 expected refcount 1 instead
> of 2. of_node_get() / of_node_put() are unbalanced for this node.
Update: with some helpful offline debug patches from Frank, I was able
to find the source of the of_node_get/put unbalance. The fixed-rate
clock driver calls of_clk_add_provider() when probed but never calls
of_clk_del_provider()
This patchset quite likely will uncover other of_node_get/put
unbalances around the kernel.
Alan
>
> Here's the very stripped down overlay:
>
> /dts-v1/;
> /plugin/;
> / {
> fragment@0 {
> target-path = "/soc/base_fpga_region";
> #address-cells = <1>;
> #size-cells = <1>;
>
> __overlay__ {
> external-fpga-config;
>
> #address-cells = <1>;
> #size-cells = <1>;
>
> clk_0: clk_0 {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <100000000>; /* 100.00 MHz */
> clock-output-names = "clk_0-clk";
> };
> };
> };
> };
^ permalink raw reply
* [PATCH v5 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: frowand.list @ 2018-10-18 22:46 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539902796-8382-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
Add checks:
- attempted kfree due to refcount reaching zero before overlay
is removed
- properties linked to an overlay node when the node is removed
- node refcount > one during node removal in a changeset destroy,
if the node was created by the changeset
After applying this patch, several validation warnings will be
reported from the devicetree unittest during boot due to
pre-existing devicetree bugs. The warnings will be similar to:
OF: ERROR: of_node_release(), unexpected properties in /testcase-data/overlay-node/test-bus/test-unittest11
OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data-2/substation@100/
hvac-medium-2
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
Changes since v4:
- make error message format consistent, error first, path last
drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
drivers/of/overlay.c | 1 +
include/linux/of.h | 15 ++++++++++-----
3 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index f4f8ed9b5454..12c3f9a15e94 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
if (!of_node_check_flag(node, OF_DYNAMIC))
return;
+ if (of_node_check_flag(node, OF_OVERLAY)) {
+
+ if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
+ /* premature refcount of zero, do not free memory */
+ pr_err("ERROR: memory leak before free overlay changeset, %pOF\n",
+ node);
+ return;
+ }
+
+ /*
+ * If node->properties non-empty then properties were added
+ * to this node either by different overlay that has not
+ * yet been removed, or by a non-overlay mechanism.
+ */
+ if (node->properties)
+ pr_err("ERROR: %s(), unexpected properties in %pOF\n",
+ __func__, node);
+ }
+
property_list_free(node->properties);
property_list_free(node->deadprops);
@@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
{
+ if (ce->action == OF_RECONFIG_ATTACH_NODE &&
+ of_node_check_flag(ce->np, OF_OVERLAY)) {
+ if (kref_read(&ce->np->kobj.kref) > 1) {
+ pr_err("ERROR: memory leak, expected refcount 1 instead of %d, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node %pOF\n",
+ kref_read(&ce->np->kobj.kref), ce->np);
+ } else {
+ of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
+ }
+ }
+
of_node_put(ce->np);
list_del(&ce->node);
kfree(ce);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index eda57ef12fd0..1176cb4b6e4e 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -373,6 +373,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
return -ENOMEM;
tchild->parent = target_node;
+ of_node_set_flag(tchild, OF_OVERLAY);
ret = of_changeset_attach_node(&ovcs->cset, tchild);
if (ret)
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f952d9..aa1dafaec6ae 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }
extern struct device_node *of_stdout;
extern raw_spinlock_t devtree_lock;
-/* flag descriptions (need to be visible even when !CONFIG_OF) */
-#define OF_DYNAMIC 1 /* node and properties were allocated via kmalloc */
-#define OF_DETACHED 2 /* node has been detached from the device tree */
-#define OF_POPULATED 3 /* device already created for the node */
-#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children of this node */
+/*
+ * struct device_node flag descriptions
+ * (need to be visible even when !CONFIG_OF)
+ */
+#define OF_DYNAMIC 1 /* (and properties) allocated via kmalloc */
+#define OF_DETACHED 2 /* detached from the device tree */
+#define OF_POPULATED 3 /* device already created */
+#define OF_POPULATED_BUS 4 /* platform bus created for children */
+#define OF_OVERLAY 5 /* allocated for an overlay */
+#define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */
#define OF_BAD_ADDR ((u64)-1)
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v5 00/18] of: overlay: validation checks, subsequent fixes
From: frowand.list @ 2018-10-18 22:46 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
From: Frank Rowand <frank.rowand@sony.com>
Add checks to (1) overlay apply process and (2) memory freeing
triggered by overlay release. The checks are intended to detect
possible memory leaks and invalid overlays.
The checks revealed bugs in existing code. Fixed the bugs.
While fixing bugs, noted other issues, which are fixed in
separate patches.
***** Powerpc folks: I was not able to test the patches that
***** directly impact Powerpc systems that use dynamic
***** devicetree. Please review that code carefully and
***** test. The specific patches are: 03/16, 04/16, 07/16
FPGA folks:
I made the validation checks that should result in an
invalid live devicetree report "ERROR" and cause the overlay apply
to fail.
I made the memory leak validation tests report "WARNING" and allow
the overlay apply to complete successfully. Please let me know
if you encounter the warnings. There are at least two paths
forward to deal with the cases that trigger the warning: (1) change
the warning to an error and fail the overlay apply, or (2) find a
way to detect the potential memory leaks and free the memory
appropriately.
ALL people:
The validations do _not_ address another major concern I have with
releasing overlays, which is use after free errors.
Changes since v4:
- 01/18: make error message format consistent, error first, path last
- 09/18: create of_prop_val_eq() and change open code to use it
- 09/18: remove extra blank lines
Changes since v3:
- 01/18: Add expected value of refcount for destroy cset entry error. Also
explain the cause of the error.
- 09/18: for errors of an overlay changing the value of #size-cells or
#address-cells, return -EINVAL so that overlay apply will fail
- 09/18: for errors of an overlay changing the value of #size-cells or
#address-cells, make the message more direct.
Old message:
OF: overlay: ERROR: overlay and/or live tree #size-cells invalid in node /soc/base_fpga_region
New message:
OF: overlay: ERROR: changing value of /soc/base_fpga_region/#size-cells not allowed
- 13/18: Update patch comment header to state that this patch modifies the
previous patch to not return immediately on fragment error and
explain this is not a performance issue.
- 13/18: remove redundant "overlay" from two error messages. "OF: overlay:"
is already present in pr_fmt()
Changes since v2:
- 13/18: Use continue to reduce indentation in find_dup_cset_node_entry()
and find_dup_cset_prop()
Changes since v1:
- move patch 16/16 to 17/18
- move patch 15/16 to 18/18
- new patch 15/18
- new patch 16/18
- 05/18: add_changeset_node() header comment: incorrect comment for @target
- 18/18: add same fix for of_parse_phandle_with_args()
- 18/18: add same fix for of_parse_phandle_with_args_map()
*** BLURB HERE ***
Frank Rowand (18):
of: overlay: add tests to validate kfrees from overlay removal
of: overlay: add missing of_node_put() after add new node to changeset
of: overlay: add missing of_node_get() in __of_attach_node_sysfs
powerpc/pseries: add of_node_put() in dlpar_detach_node()
of: overlay: use prop add changeset entry for property in new nodes
of: overlay: do not duplicate properties from overlay for new nodes
of: dynamic: change type of of_{at,de}tach_node() to void
of: overlay: reorder fields in struct fragment
of: overlay: validate overlay properties #address-cells and
#size-cells
of: overlay: make all pr_debug() and pr_err() messages unique
of: overlay: test case of two fragments adding same node
of: overlay: check prevents multiple fragments add or delete same node
of: overlay: check prevents multiple fragments touching same property
of: unittest: remove unused of_unittest_apply_overlay() argument
of: overlay: set node fields from properties when add new overlay node
of: unittest: allow base devicetree to have symbol metadata
of: unittest: find overlays[] entry by name instead of index
of: unittest: initialize args before calling of_*parse_*()
arch/powerpc/platforms/pseries/dlpar.c | 15 +-
arch/powerpc/platforms/pseries/reconfig.c | 6 +-
drivers/of/dynamic.c | 68 +++--
drivers/of/kobj.c | 4 +-
drivers/of/overlay.c | 292 ++++++++++++++++-----
drivers/of/unittest-data/Makefile | 2 +
.../of/unittest-data/overlay_bad_add_dup_node.dts | 28 ++
.../of/unittest-data/overlay_bad_add_dup_prop.dts | 24 ++
drivers/of/unittest-data/overlay_base.dts | 1 +
drivers/of/unittest.c | 96 +++++--
include/linux/of.h | 25 +-
11 files changed, 439 insertions(+), 122 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_node.dts
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply
* [PATCH v5 02/18] of: overlay: add missing of_node_put() after add new node to changeset
From: frowand.list @ 2018-10-18 22:46 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539902796-8382-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
The refcount of a newly added overlay node decrements to one
(instead of zero) when the overlay changeset is destroyed. This
change will cause the final decrement be to zero.
After applying this patch, new validation warnings will be
reported from the devicetree unittest during boot due to
a pre-existing devicetree bug. The warnings will be similar to:
OF: ERROR: memory leak before free overlay changeset, /testcase-data/overlay-node/test-bus/test-unittest4
This pre-existing devicetree bug will also trigger a WARN_ONCE() from
refcount_sub_and_test_checked() when an overlay changeset is
destroyed without having first been applied. This scenario occurs
when an error in the overlay is detected during the overlay changeset
creation:
WARNING: CPU: 0 PID: 1 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa8/0xbc
refcount_t: underflow; use-after-free.
(unwind_backtrace) from (show_stack+0x10/0x14)
(show_stack) from (dump_stack+0x6c/0x8c)
(dump_stack) from (__warn+0xdc/0x104)
(__warn) from (warn_slowpath_fmt+0x44/0x6c)
(warn_slowpath_fmt) from (refcount_sub_and_test_checked+0xa8/0xbc)
(refcount_sub_and_test_checked) from (kobject_put+0x24/0x208)
(kobject_put) from (of_changeset_destroy+0x2c/0xb4)
(of_changeset_destroy) from (free_overlay_changeset+0x1c/0x9c)
(free_overlay_changeset) from (of_overlay_remove+0x284/0x2cc)
(of_overlay_remove) from (of_unittest_apply_revert_overlay_check.constprop.4+0xf8/0x1e8)
(of_unittest_apply_revert_overlay_check.constprop.4) from (of_unittest_overlay+0x960/0xed8)
(of_unittest_overlay) from (of_unittest+0x1cc4/0x2138)
(of_unittest) from (do_one_initcall+0x4c/0x28c)
(do_one_initcall) from (kernel_init_freeable+0x29c/0x378)
(kernel_init_freeable) from (kernel_init+0x8/0x110)
(kernel_init) from (ret_from_fork+0x14/0x2c)
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 1176cb4b6e4e..32cfee68f2e3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -379,7 +379,9 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
if (ret)
return ret;
- return build_changeset_next_level(ovcs, tchild, node);
+ ret = build_changeset_next_level(ovcs, tchild, node);
+ of_node_put(tchild);
+ return ret;
}
if (node->phandle && tchild->phandle)
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v5 03/18] of: overlay: add missing of_node_get() in __of_attach_node_sysfs
From: frowand.list @ 2018-10-18 22:46 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539902796-8382-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
There is a matching of_node_put() in __of_detach_node_sysfs()
Remove misleading comment from function header comment for
of_detach_node().
This patch may result in memory leaks from code that directly calls
the dynamic node add and delete functions directly instead of
using changesets.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
This patch should result in powerpc systems that dynamically
allocate a node, then later deallocate the node to have a
memory leak when the node is deallocated.
The next patch in the series will fix the leak.
drivers/of/dynamic.c | 3 ---
drivers/of/kobj.c | 4 +++-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 12c3f9a15e94..146681540487 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -272,9 +272,6 @@ void __of_detach_node(struct device_node *np)
/**
* of_detach_node() - "Unplug" a node from the device tree.
- *
- * The caller must hold a reference to the node. The memory associated with
- * the node is not freed until its refcount goes to zero.
*/
int of_detach_node(struct device_node *np)
{
diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 7a0a18980b98..c72eef988041 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -133,6 +133,9 @@ int __of_attach_node_sysfs(struct device_node *np)
}
if (!name)
return -ENOMEM;
+
+ of_node_get(np);
+
rc = kobject_add(&np->kobj, parent, "%s", name);
kfree(name);
if (rc)
@@ -159,6 +162,5 @@ void __of_detach_node_sysfs(struct device_node *np)
kobject_del(&np->kobj);
}
- /* finally remove the kobj_init ref */
of_node_put(np);
}
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
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