* 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:12 UTC (permalink / raw)
To: christophe.leroy; +Cc: muriloo, linuxppc-dev
In-Reply-To: <6b3b54e5-cbe3-c693-23ea-26928e7597c8@c-s.fr>
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?
^ permalink raw reply
* [PATCH v2] powerpc/uaccess: fix warning/error with access_ok()
From: Christophe Leroy @ 2018-10-18 9:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
With the following peace of code, the following compilation warning
is encountered:
if (_IOC_DIR(ioc) != _IOC_NONE) {
int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
if (!access_ok(verify, ioarg, _IOC_SIZE(ioc))) {
drivers/platform/test/dev.c: In function ‘my_ioctl’:
drivers/platform/test/dev.c:219:7: warning: unused variable ‘verify’ [-Wunused-variable]
int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
This patch fixes it by handing the type to __access_ok(), changing it
to an inline function for PPC64 as already done for PPC32
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: fixed the three direct users of __access_ok()
arch/powerpc/include/asm/uaccess.h | 13 ++++++++-----
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/lib/sstep.c | 4 ++--
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 15bea9a0f260..97faf0353919 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -47,13 +47,16 @@ static inline void set_fs(mm_segment_t fs)
* This check is sufficient because there is a large enough
* gap between user addresses and the kernel addresses
*/
-#define __access_ok(addr, size, segment) \
- (((addr) <= (segment).seg) && ((size) <= (segment).seg))
+static inline int __access_ok(int type, unsigned long addr, unsigned long size,
+ mm_segment_t seg)
+{
+ return addr <= seg.seg && size <= seg.seg;
+}
#else
-static inline int __access_ok(unsigned long addr, unsigned long size,
- mm_segment_t seg)
+static inline int __access_ok(int type, unsigned long addr, unsigned long size,
+ mm_segment_t seg)
{
if (addr > seg.seg)
return 0;
@@ -64,7 +67,7 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
#define access_ok(type, addr, size) \
(__chk_user_ptr(addr), \
- __access_ok((__force unsigned long)(addr), (size), get_fs()))
+ __access_ok((type), (__force unsigned long)(addr), (size), get_fs()))
/*
* These are the main single-value transfer routines. They automatically
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 7ad304a3cc7d..4cc84fe13f9c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1310,7 +1310,7 @@ void show_user_instructions(struct pt_regs *regs)
* Make sure the NIP points at userspace, not kernel text/data or
* elsewhere.
*/
- if (!__access_ok(pc, NR_INSN_TO_PRINT * sizeof(int), USER_DS)) {
+ if (!__access_ok(VERIFY_READ, pc, NR_INSN_TO_PRINT * sizeof(int), USER_DS)) {
pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
current->comm, current->pid);
return;
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index d81568f783e5..ff117418257c 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -110,9 +110,9 @@ static nokprobe_inline long address_ok(struct pt_regs *regs,
{
if (!user_mode(regs))
return 1;
- if (__access_ok(ea, nb, USER_DS))
+ if (__access_ok(VERIFY_WRITE, ea, nb, USER_DS))
return 1;
- if (__access_ok(ea, 1, USER_DS))
+ if (__access_ok(VERIFY_WRITE, ea, 1, USER_DS))
/* Access overlaps the end of the user region */
regs->dar = USER_DS.seg;
else
--
2.13.3
^ permalink raw reply related
* Re: SV: MPC8321 boot failure
From: gregkh @ 2018-10-18 9:34 UTC (permalink / raw)
To: David Gounaris
Cc: linuxppc-dev@lists.ozlabs.org, stable@vger.kernel.org,
Mathias Thore
In-Reply-To: <CY1PR10MB0204796CBD711EBA4F5286A4EFF80@CY1PR10MB0204.namprd10.prod.outlook.com>
On Thu, Oct 18, 2018 at 08:51:46AM +0000, David Gounaris wrote:
> Hi, I can also confirm that it works after cherry-picking the proposed commit.
>
> Reported-and-tested-by: David Gounaris <david.gounaris@infinera.com<mailto:David.Gounaris@infinera.com>>
>
Now queued up, thanks.
greg k-h
^ permalink raw reply
* [bug report] powerpc/perf: Add nest IMC PMU support
From: Dan Carpenter @ 2018-10-18 9:33 UTC (permalink / raw)
To: anju; +Cc: linuxppc-dev
Hello Anju T Sudhakar,
The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
Jul 19, 2017, leads to the following static checker warning:
arch/powerpc/perf/imc-pmu.c:506 nest_imc_event_init()
warn: 'pcni' can't be NULL.
arch/powerpc/perf/imc-pmu.c
485 if (event->cpu < 0)
486 return -EINVAL;
487
488 pmu = imc_event_to_pmu(event);
489
490 /* Sanity check for config (event offset) */
491 if ((config & IMC_EVENT_OFFSET_MASK) > pmu->counter_mem_size)
492 return -EINVAL;
493
494 /*
495 * Nest HW counter memory resides in a per-chip reserve-memory (HOMER).
496 * Get the base memory addresss for this cpu.
497 */
498 chip_id = cpu_to_chip_id(event->cpu);
499 pcni = pmu->mem_info;
^^^^^^^^^^^^^^^^^^^^
500 do {
501 if (pcni->id == chip_id) {
502 flag = true;
503 break;
504 }
505 pcni++;
^^^^^^
506 } while (pcni);
^^^^
This will loop until we crash. I'm not sure what was intended.
507
508 if (!flag)
509 return -ENODEV;
510
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()
From: Christophe LEROY @ 2018-10-18 9:28 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: jannh, muriloo
In-Reply-To: <20181005132123.6038-1-mpe@ellerman.id.au>
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 ?
Christophe
> pr_info("%s[%d]: code: ", current->comm, current->pid);
>
> for (i = 0; i < instructions_to_print; i++) {
>
^ permalink raw reply
* Re: [PATCH] powerpc/uaccess: fix warning/error with access_ok()
From: Christophe LEROY @ 2018-10-18 9:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <40873f45f922b28c727c013e3883be467fc31c56.1539852411.git.christophe.leroy@c-s.fr>
Le 18/10/2018 à 10:48, Christophe Leroy a écrit :
> With the following peace of code, the following compilation warning
> is encountered:
>
> if (_IOC_DIR(ioc) != _IOC_NONE) {
> int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
>
> if (!access_ok(verify, ioarg, _IOC_SIZE(ioc))) {
>
> drivers/platform/test/dev.c: In function ‘my_ioctl’:
> drivers/platform/test/dev.c:219:7: warning: unused variable ‘verify’ [-Wunused-variable]
> int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
>
> This patch fixes it by handing the type to __access_ok(), changing it
> to an inline function for PPC64 as already done for PPC32
Oops, not that easy, there are places using __access_ok() directly,
those need to be modified as well.
Christophe
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/uaccess.h | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 15bea9a0f260..97faf0353919 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -47,13 +47,16 @@ static inline void set_fs(mm_segment_t fs)
> * This check is sufficient because there is a large enough
> * gap between user addresses and the kernel addresses
> */
> -#define __access_ok(addr, size, segment) \
> - (((addr) <= (segment).seg) && ((size) <= (segment).seg))
> +static inline int __access_ok(int type, unsigned long addr, unsigned long size,
> + mm_segment_t seg)
> +{
> + return addr <= seg.seg && size <= seg.seg;
> +}
>
> #else
>
> -static inline int __access_ok(unsigned long addr, unsigned long size,
> - mm_segment_t seg)
> +static inline int __access_ok(int type, unsigned long addr, unsigned long size,
> + mm_segment_t seg)
> {
> if (addr > seg.seg)
> return 0;
> @@ -64,7 +67,7 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
>
> #define access_ok(type, addr, size) \
> (__chk_user_ptr(addr), \
> - __access_ok((__force unsigned long)(addr), (size), get_fs()))
> + __access_ok((type), (__force unsigned long)(addr), (size), get_fs()))
>
> /*
> * These are the main single-value transfer routines. They automatically
>
^ permalink raw reply
* [PATCH] powerpc/uaccess: fix warning/error with access_ok()
From: Christophe Leroy @ 2018-10-18 8:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
With the following peace of code, the following compilation warning
is encountered:
if (_IOC_DIR(ioc) != _IOC_NONE) {
int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
if (!access_ok(verify, ioarg, _IOC_SIZE(ioc))) {
drivers/platform/test/dev.c: In function ‘my_ioctl’:
drivers/platform/test/dev.c:219:7: warning: unused variable ‘verify’ [-Wunused-variable]
int verify = _IOC_DIR(ioc) & _IOC_READ ? VERIFY_WRITE : VERIFY_READ;
This patch fixes it by handing the type to __access_ok(), changing it
to an inline function for PPC64 as already done for PPC32
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/uaccess.h | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 15bea9a0f260..97faf0353919 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -47,13 +47,16 @@ static inline void set_fs(mm_segment_t fs)
* This check is sufficient because there is a large enough
* gap between user addresses and the kernel addresses
*/
-#define __access_ok(addr, size, segment) \
- (((addr) <= (segment).seg) && ((size) <= (segment).seg))
+static inline int __access_ok(int type, unsigned long addr, unsigned long size,
+ mm_segment_t seg)
+{
+ return addr <= seg.seg && size <= seg.seg;
+}
#else
-static inline int __access_ok(unsigned long addr, unsigned long size,
- mm_segment_t seg)
+static inline int __access_ok(int type, unsigned long addr, unsigned long size,
+ mm_segment_t seg)
{
if (addr > seg.seg)
return 0;
@@ -64,7 +67,7 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
#define access_ok(type, addr, size) \
(__chk_user_ptr(addr), \
- __access_ok((__force unsigned long)(addr), (size), get_fs()))
+ __access_ok((type), (__force unsigned long)(addr), (size), get_fs()))
/*
* These are the main single-value transfer routines. They automatically
--
2.13.3
^ permalink raw reply related
* [PATCH kernel 2/2] powerpc/powernv/pseries: Rework device adding to IOMMU groups
From: Alexey Kardashevskiy @ 2018-10-18 7:52 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Sam Bobroff, kvm-ppc, David Gibson
In-Reply-To: <20181018075243.4798-1-aik@ozlabs.ru>
The powernv platform registers IOMMU groups and adds devices to them
from the pci_controller_ops::setup_bridge() hook except one case when
virtual functions (SRIOV VFs) are added from a bus notifier.
The pseries platform registers IOMMU groups from
the pci_controller_ops::dma_bus_setup() hook and adds devices from
the pci_controller_ops::dma_dev_setup() hook. The very same bus notifier
used for powernv does not add devices for pseries though as
__of_scan_bus() adds devices first, then it does the bus/dev DMA setup.
Both platforms use iommu_add_device() which takes a device and expects
it to have a valid IOMMU table struct with an iommu_table_group pointer
which in turn points the iommu_group struct (which represents
an IOMMU group). Although the helper seems easy to use, it relies on
some pre-existing device configuration and associated data structures
which it does not really need.
This simplifies iommu_add_device() to take the table_group pointer
directly. Pseries already has a table_group pointer handy and the bus
notified is not used anyway. For powernv, this copies the existing bus
notifier, makes it work for powernv only which means an easy way of
getting to the table_group pointer. This was tested on VFs but should
also support physical PCI hotplug.
Since iommu_add_device() receives the table_group pointer directly,
pseries does not do TCE cache invalidation (the hypervisor does) nor
allow multiple groups per a VFIO container (in other words sharing
an IOMMU table between partitionable endpoints), this removes
iommu_table_group_link from pseries.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/include/asm/iommu.h | 12 +++----
arch/powerpc/kernel/iommu.c | 58 ++-----------------------------
arch/powerpc/platforms/powernv/pci-ioda.c | 10 +-----
arch/powerpc/platforms/powernv/pci.c | 43 ++++++++++++++++++++++-
arch/powerpc/platforms/pseries/iommu.c | 46 ++++++++++++------------
5 files changed, 74 insertions(+), 95 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 726f07b..39bee10 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -220,9 +220,9 @@ struct iommu_table_group {
extern void iommu_register_group(struct iommu_table_group *table_group,
int pci_domain_number, unsigned long pe_num);
-extern int iommu_add_device(struct device *dev);
+extern int iommu_add_device(struct iommu_table_group *table_group,
+ struct device *dev);
extern void iommu_del_device(struct device *dev);
-extern int __init tce_iommu_bus_notifier_init(void);
extern long iommu_tce_xchg(struct mm_struct *mm, struct iommu_table *tbl,
unsigned long entry, unsigned long *hpa,
enum dma_data_direction *direction);
@@ -233,7 +233,8 @@ static inline void iommu_register_group(struct iommu_table_group *table_group,
{
}
-static inline int iommu_add_device(struct device *dev)
+static inline int iommu_add_device(struct iommu_table_group *table_group,
+ struct device *dev)
{
return 0;
}
@@ -241,11 +242,6 @@ static inline int iommu_add_device(struct device *dev)
static inline void iommu_del_device(struct device *dev)
{
}
-
-static inline int __init tce_iommu_bus_notifier_init(void)
-{
- return 0;
-}
#endif /* !CONFIG_IOMMU_API */
int dma_iommu_mapping_error(struct device *dev, dma_addr_t dma_addr);
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 47d75c5..fb14fbf 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -1094,11 +1094,8 @@ void iommu_release_ownership(struct iommu_table *tbl)
}
EXPORT_SYMBOL_GPL(iommu_release_ownership);
-int iommu_add_device(struct device *dev)
+int iommu_add_device(struct iommu_table_group *table_group, struct device *dev)
{
- struct iommu_table *tbl;
- struct iommu_table_group_link *tgl;
-
/*
* The sysfs entries should be populated before
* binding IOMMU group. If sysfs entries isn't
@@ -1114,32 +1111,10 @@ int iommu_add_device(struct device *dev)
return -EBUSY;
}
- tbl = get_iommu_table_base(dev);
- if (!tbl) {
- pr_debug("%s: Skipping device %s with no tbl\n",
- __func__, dev_name(dev));
- return 0;
- }
-
- tgl = list_first_entry_or_null(&tbl->it_group_list,
- struct iommu_table_group_link, next);
- if (!tgl) {
- pr_debug("%s: Skipping device %s with no group\n",
- __func__, dev_name(dev));
- return 0;
- }
pr_debug("%s: Adding %s to iommu group %d\n",
- __func__, dev_name(dev),
- iommu_group_id(tgl->table_group->group));
+ __func__, dev_name(dev), iommu_group_id(table_group->group));
- if (PAGE_SIZE < IOMMU_PAGE_SIZE(tbl)) {
- pr_err("%s: Invalid IOMMU page size %lx (%lx) on %s\n",
- __func__, IOMMU_PAGE_SIZE(tbl),
- PAGE_SIZE, dev_name(dev));
- return -EINVAL;
- }
-
- return iommu_group_add_device(tgl->table_group->group, dev);
+ return iommu_group_add_device(table_group->group, dev);
}
EXPORT_SYMBOL_GPL(iommu_add_device);
@@ -1159,31 +1134,4 @@ void iommu_del_device(struct device *dev)
iommu_group_remove_device(dev);
}
EXPORT_SYMBOL_GPL(iommu_del_device);
-
-static int tce_iommu_bus_notifier(struct notifier_block *nb,
- unsigned long action, void *data)
-{
- struct device *dev = data;
-
- switch (action) {
- case BUS_NOTIFY_ADD_DEVICE:
- return iommu_add_device(dev);
- case BUS_NOTIFY_DEL_DEVICE:
- if (dev->iommu_group)
- iommu_del_device(dev);
- return 0;
- default:
- return 0;
- }
-}
-
-static struct notifier_block tce_iommu_bus_nb = {
- .notifier_call = tce_iommu_bus_notifier,
-};
-
-int __init tce_iommu_bus_notifier_init(void)
-{
- bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
- return 0;
-}
#endif /* CONFIG_IOMMU_API */
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 4dc4a5fed..0228164 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1940,7 +1940,7 @@ static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe,
set_dma_offset(&dev->dev, pe->tce_bypass_base);
#ifdef CONFIG_IOMMU_API
if (add_to_group)
- iommu_add_device(&dev->dev);
+ iommu_add_device(&pe->table_group, &dev->dev);
#endif
if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
@@ -2369,14 +2369,6 @@ static long pnv_pci_ioda2_setup_default_config(struct pnv_ioda_pe *pe)
if (!pnv_iommu_bypass_disabled)
pnv_pci_ioda2_set_bypass(pe, true);
- /*
- * Setting table base here only for carrying iommu_group
- * further down to let iommu_add_device() do the job.
- * pnv_pci_ioda_dma_dev_setup will override it later anyway.
- */
- if (pe->flags & PNV_IODA_PE_DEV)
- set_iommu_table_base(&pe->pdev->dev, tbl);
-
return 0;
}
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 13aef23..98e02c1 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -1127,4 +1127,45 @@ void __init pnv_pci_init(void)
set_pci_dma_ops(&dma_iommu_ops);
}
-machine_subsys_initcall_sync(powernv, tce_iommu_bus_notifier_init);
+static int pnv_tce_iommu_bus_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct pci_dev *pdev;
+ struct pci_dn *pdn;
+ struct pnv_ioda_pe *pe;
+ struct pci_controller *hose;
+ struct pnv_phb *phb;
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ pdev = to_pci_dev(dev);
+ pdn = pci_get_pdn(pdev);
+ hose = pci_bus_to_host(pdev->bus);
+ phb = hose->private_data;
+
+ WARN_ON_ONCE(!phb);
+ if (!pdn || pdn->pe_number == IODA_INVALID_PE || !phb)
+ return 0;
+
+ pe = &phb->ioda.pe_array[pdn->pe_number];
+ iommu_add_device(&pe->table_group, dev);
+ return 0;
+ case BUS_NOTIFY_DEL_DEVICE:
+ iommu_del_device(dev);
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static struct notifier_block pnv_tce_iommu_bus_nb = {
+ .notifier_call = pnv_tce_iommu_bus_notifier,
+};
+
+static int __init pnv_tce_iommu_bus_notifier_init(void)
+{
+ bus_register_notifier(&pci_bus_type, &pnv_tce_iommu_bus_nb);
+ return 0;
+}
+machine_subsys_initcall_sync(powernv, pnv_tce_iommu_bus_notifier_init);
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index eae2578..38b6dd0 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -60,7 +60,6 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
{
struct iommu_table_group *table_group;
struct iommu_table *tbl;
- struct iommu_table_group_link *tgl;
table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
node);
@@ -71,22 +70,13 @@ static struct iommu_table_group *iommu_pseries_alloc_group(int node)
if (!tbl)
goto free_group;
- tgl = kzalloc_node(sizeof(struct iommu_table_group_link), GFP_KERNEL,
- node);
- if (!tgl)
- goto free_table;
-
INIT_LIST_HEAD_RCU(&tbl->it_group_list);
kref_init(&tbl->it_kref);
- tgl->table_group = table_group;
- list_add_rcu(&tgl->next, &tbl->it_group_list);
table_group->tables[0] = tbl;
return table_group;
-free_table:
- kfree(tbl);
free_group:
kfree(table_group);
return NULL;
@@ -96,23 +86,12 @@ static void iommu_pseries_free_group(struct iommu_table_group *table_group,
const char *node_name)
{
struct iommu_table *tbl;
-#ifdef CONFIG_IOMMU_API
- struct iommu_table_group_link *tgl;
-#endif
if (!table_group)
return;
tbl = table_group->tables[0];
#ifdef CONFIG_IOMMU_API
- tgl = list_first_entry_or_null(&tbl->it_group_list,
- struct iommu_table_group_link, next);
-
- WARN_ON_ONCE(!tgl);
- if (tgl) {
- list_del_rcu(&tgl->next);
- kfree(tgl);
- }
if (table_group->group) {
iommu_group_put(table_group->group);
BUG_ON(table_group->group);
@@ -1240,7 +1219,7 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
}
set_iommu_table_base(&dev->dev, pci->table_group->tables[0]);
- iommu_add_device(&dev->dev);
+ iommu_add_device(pci->table_group, &dev->dev);
}
static int dma_set_mask_pSeriesLP(struct device *dev, u64 dma_mask)
@@ -1455,4 +1434,27 @@ static int __init disable_multitce(char *str)
__setup("multitce=", disable_multitce);
+static int tce_iommu_bus_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+
+ switch (action) {
+ case BUS_NOTIFY_DEL_DEVICE:
+ iommu_del_device(dev);
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static struct notifier_block tce_iommu_bus_nb = {
+ .notifier_call = tce_iommu_bus_notifier,
+};
+
+static int __init tce_iommu_bus_notifier_init(void)
+{
+ bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
+ return 0;
+}
machine_subsys_initcall_sync(pseries, tce_iommu_bus_notifier_init);
--
2.11.0
^ permalink raw reply related
* [PATCH kernel 1/2] powerpc/pseries: Remove IOMMU API support for non-LPAR systems
From: Alexey Kardashevskiy @ 2018-10-18 7:52 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Sam Bobroff, kvm-ppc, David Gibson
In-Reply-To: <20181018075243.4798-1-aik@ozlabs.ru>
The pci_dma_bus_setup_pSeries and pci_dma_dev_setup_pSeries hooks are
registered for the pseries platform which does not have FW_FEATURE_LPAR;
these would be pre-powernv platforms which we never supported PCI pass
through for anyway so remove it.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Propably should remove all pseries-but-not-lpar code.
---
arch/powerpc/platforms/pseries/iommu.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index cf90582..eae2578 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -648,7 +648,6 @@ static void pci_dma_bus_setup_pSeries(struct pci_bus *bus)
iommu_table_setparms(pci->phb, dn, tbl);
tbl->it_ops = &iommu_table_pseries_ops;
iommu_init_table(tbl, pci->phb->node);
- iommu_register_group(pci->table_group, pci_domain_nr(bus), 0);
/* Divide the rest (1.75GB) among the children */
pci->phb->dma_window_size = 0x80000000ul;
@@ -759,10 +758,7 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev)
iommu_table_setparms(phb, dn, tbl);
tbl->it_ops = &iommu_table_pseries_ops;
iommu_init_table(tbl, phb->node);
- iommu_register_group(PCI_DN(dn)->table_group,
- pci_domain_nr(phb->bus), 0);
set_iommu_table_base(&dev->dev, tbl);
- iommu_add_device(&dev->dev);
return;
}
@@ -773,11 +769,10 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev)
while (dn && PCI_DN(dn) && PCI_DN(dn)->table_group == NULL)
dn = dn->parent;
- if (dn && PCI_DN(dn)) {
+ if (dn && PCI_DN(dn))
set_iommu_table_base(&dev->dev,
PCI_DN(dn)->table_group->tables[0]);
- iommu_add_device(&dev->dev);
- } else
+ else
printk(KERN_WARNING "iommu: Device %s has no iommu table\n",
pci_name(dev));
}
--
2.11.0
^ permalink raw reply related
* [PATCH kernel 0/2] powerpc/iommu: Redo iommu groups
From: Alexey Kardashevskiy @ 2018-10-18 7:52 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy, Sam Bobroff, kvm-ppc, David Gibson
The aim is to:
1. simplify the code
2. get rid of iommu_table_group_link.
Now: iommu_table points to a list of iommu_table_group_link which
point to iommu_table_group (powerpc specific part of an IOMMU group);
one iommu_table_group_link per iommu_table_group per IODA PE.
Plan: add support of multiple IODA PEs per one generic IOMMU group,
I am just not sure how yet. We already have a case of GPU+NPU groups,
and a worse one is coming, with 3xGPU + 6xNVLink2.
Please comment. Thanks.
Alexey Kardashevskiy (2):
powerpc/pseries: Remove IOMMU API support for non-LPAR systems
powerpc/powernv/pseries: Rework device adding to IOMMU groups
arch/powerpc/include/asm/iommu.h | 12 +++----
arch/powerpc/kernel/iommu.c | 58 ++-----------------------------
arch/powerpc/platforms/powernv/pci-ioda.c | 10 +-----
arch/powerpc/platforms/powernv/pci.c | 43 ++++++++++++++++++++++-
arch/powerpc/platforms/pseries/iommu.c | 55 ++++++++++++++---------------
5 files changed, 76 insertions(+), 102 deletions(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH] powerpc: Add missing include <asm/stacktrace.h>
From: Mathieu Malaterre @ 2018-10-18 6:34 UTC (permalink / raw)
To: Christophe LEROY; +Cc: Paul Mackerras, linuxppc-dev, LKML
In-Reply-To: <373dd30b-70d1-5e06-c55e-45f8f6763edf@c-s.fr>
On Thu, Oct 18, 2018 at 6:37 AM Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 17/10/2018 à 21:25, Mathieu Malaterre a écrit :
> > In commit 88b0fe175735 ("powerpc: Add show_user_instructions()") the
> > function show_user_instructions was added.
> >
> > This commit adds an include of header file <asm/stacktrace.h> to provide
> > the missing function prototype. Silence the following gcc warning
> > (treated as error with W=1):
> >
> > arch/powerpc/kernel/process.c:1302:6: error: no previous prototype for ‘show_user_instructions’ [-Werror=missing-prototypes]
> >
> > Signed-off-by: Mathieu Malaterre <malat@debian.org>
>
> This is already fixed, see
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c9386bfd37d37f29588de9ea9add455510049c33
Excellent !
Sorry for the noise :(
> Christophe
>
> > ---
> > arch/powerpc/kernel/process.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index bb6ac471a784..1c64491e9702 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -65,6 +65,7 @@
> > #include <asm/livepatch.h>
> > #include <asm/cpu_has_feature.h>
> > #include <asm/asm-prototypes.h>
> > +#include <asm/stacktrace.h>
> >
> > #include <linux/kprobes.h>
> > #include <linux/kdebug.h>
> >
^ permalink raw reply
* [PATCH] powerpc/8xx: add missing header in 8xx_mmu.c
From: Christophe Leroy @ 2018-10-18 5:22 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
arch/powerpc/mm/8xx_mmu.c:174:6: error: no previous prototype for ‘set_context’ [-Werror=missing-prototypes]
void set_context(unsigned long id, pgd_t *pgd)
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/8xx_mmu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/mm/8xx_mmu.c b/arch/powerpc/mm/8xx_mmu.c
index 36484a2ef915..64ee7597380e 100644
--- a/arch/powerpc/mm/8xx_mmu.c
+++ b/arch/powerpc/mm/8xx_mmu.c
@@ -13,6 +13,7 @@
*/
#include <linux/memblock.h>
+#include <linux/mmu_context.h>
#include <asm/fixmap.h>
#include <asm/code-patching.h>
--
2.13.3
^ permalink raw reply related
* [PATCH] powerpc/mm: remove unused variable
From: Christophe Leroy @ 2018-10-18 5:22 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In file included from ./include/linux/hugetlb.h:445:0,
from arch/powerpc/kernel/setup-common.c:37:
./arch/powerpc/include/asm/hugetlb.h: In function ‘huge_ptep_clear_flush’:
./arch/powerpc/include/asm/hugetlb.h:154:8: error: variable ‘pte’ set but not used [-Werror=unused-but-set-variable]
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/hugetlb.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h
index 2d00cc530083..cb812b131a37 100644
--- a/arch/powerpc/include/asm/hugetlb.h
+++ b/arch/powerpc/include/asm/hugetlb.h
@@ -148,8 +148,7 @@ static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
unsigned long addr, pte_t *ptep)
{
- pte_t pte;
- pte = huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
+ huge_ptep_get_and_clear(vma->vm_mm, addr, ptep);
flush_hugetlb_page(vma, addr);
}
--
2.13.3
^ permalink raw reply related
* Re: MPC8321 boot failure
From: Christophe LEROY @ 2018-10-18 4:49 UTC (permalink / raw)
To: David Gounaris, linuxppc-dev@lists.ozlabs.org,
gregkh@linuxfoundation.org
Cc: stable@vger.kernel.org, Mathias Thore
In-Reply-To: <0cbfec90-0e58-e267-71ae-c5fc8a8e0640@c-s.fr>
Hi,
I can now confirm that the boot failure is due to the absence of commit
8183d99f4a22 ("powerpc/lib/feature-fixups: use raw_patch_instruction()")
Greg, could you please apply that patch to 4.14 stable ?
Thanks
Christophe
Le 17/10/2018 à 18:36, Christophe LEROY a écrit :
> Hi,
>
> Yes I discovered the same issue today on MPC8321E, I plan to look at it
> more closely tomorrow morning (Paris Time).
>
> I think we are missing commit 8183d99f4a22c2abbc543847a588df3666ef0c0c ,
> I didn't realise it when we applied the serie to 4.14,
> patch_instruction() is called too early without that patch.
>
> If you have opportunity to test now, you are welcome, otherwise I'll
> test it tomorrow.
>
> Christophe
>
> Le 17/10/2018 à 17:18, David Gounaris a écrit :
>> Hello, I got into troubles when I upgraded to Linux kernel 4.14.76 on
>> boards with MPC8321.
>>
>>
>> The symptom that I see is that the boot process gets cyclic, and no
>> printouts are seen from the Linux kernel. It seems like it resets.
>>
>>
>> When I revert the following commits it works again.
>>
>> af1a8101794dfea897290e057f61086dabfe6c91, powerpc/lib: fix book3s/32
>> boot failure due to code patching
>> 609fbeddb24c4035d24fc32d82dc08b30ae3dfc0, powerpc: Avoid code patching
>> freed init sections
>>
>> Any ideas of how to continue?
>>
>> BR / David Gounaris
>>
>>
>>
^ permalink raw reply
* Re: [PATCH] powerpc: Add missing include <asm/stacktrace.h>
From: Christophe LEROY @ 2018-10-18 4:37 UTC (permalink / raw)
To: Mathieu Malaterre, Michael Ellerman
Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <20181017192558.8039-1-malat@debian.org>
Le 17/10/2018 à 21:25, Mathieu Malaterre a écrit :
> In commit 88b0fe175735 ("powerpc: Add show_user_instructions()") the
> function show_user_instructions was added.
>
> This commit adds an include of header file <asm/stacktrace.h> to provide
> the missing function prototype. Silence the following gcc warning
> (treated as error with W=1):
>
> arch/powerpc/kernel/process.c:1302:6: error: no previous prototype for ‘show_user_instructions’ [-Werror=missing-prototypes]
>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
This is already fixed, see
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c9386bfd37d37f29588de9ea9add455510049c33
Christophe
> ---
> arch/powerpc/kernel/process.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index bb6ac471a784..1c64491e9702 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -65,6 +65,7 @@
> #include <asm/livepatch.h>
> #include <asm/cpu_has_feature.h>
> #include <asm/asm-prototypes.h>
> +#include <asm/stacktrace.h>
>
> #include <linux/kprobes.h>
> #include <linux/kdebug.h>
>
^ permalink raw reply
* Re: [PATCH 1/5] powerpc/64s: Kernel Hypervisor Restricted Access Prevention
From: Russell Currey @ 2018-10-18 2:03 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey
In-Reply-To: <87bm7sero1.fsf@concordia.ellerman.id.au>
On Wed, 2018-10-17 at 22:30 +1100, Michael Ellerman wrote:
> Russell Currey <ruscur@russell.cc> writes:
> > diff --git a/arch/powerpc/kernel/entry_64.S
> > b/arch/powerpc/kernel/entry_64.S
> > index 7b1693adff2a..090f72cbb02d 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -286,6 +286,9 @@ BEGIN_FTR_SECTION
> > HMT_MEDIUM_LOW
> > END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> >
> > + /* headed back to userspace, so unlock the AMR */
> > + UNLOCK_AMR(r2)
> > +
>
> This one needs an ifdef, or preferable an empty version in a header
> for
> non-book3s 64, otherwise we get:
>
> arch/powerpc/kernel/entry_64.S: Assembler messages:
> arch/powerpc/kernel/entry_64.S:290: Error: unrecognized opcode:
> `unlock_amr(%r2)'
> scripts/Makefile.build:405: recipe for target
> 'arch/powerpc/kernel/entry_64.o' failed
>
> That's a corenet64-ish defconfig.
Yep, sorry. I knew it wouldn't build on non-64s but I just wanted to
get the main part out there so people could start looking at it. Will
fix.
- Russell
>
> cheers
^ permalink raw reply
* Re: [PATCH 1/5] powerpc/64s: Kernel Hypervisor Restricted Access Prevention
From: Russell Currey @ 2018-10-18 2:02 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, linuxppc-dev
In-Reply-To: <20181017225913.6026e6f4@roar.ozlabs.ibm.com>
On Wed, 2018-10-17 at 22:59 +1000, Nicholas Piggin wrote:
> On Wed, 17 Oct 2018 17:44:19 +1100
> Russell Currey <ruscur@russell.cc> wrote:
>
> > Kernel Hypervisor Restricted Access Prevention (KHRAP) utilises a
> > feature
> > of the Radix MMU which disallows read and write access to userspace
> > addresses. By utilising this, the kernel is prevented from
> > accessing
> > user data from outside of trusted paths that perform proper safety
> > checks,
> > such as copy_{to/from}_user() and friends.
> >
> > Userspace access is disabled from early boot and is only enabled
> > when:
> >
> > - exiting the kernel and entering userspace
> > - performing an operation like copy_{to/from}_user()
> > - context switching to a process that has access enabled
> >
> > and similarly, access is disabled again when exiting userspace and
> > entering
> > the kernel.
> >
> > This feature has a slight performance impact which I roughly
> > measured to be
> > 4% slower (performing 1GB of 1 byte read()/write() syscalls), and
> > is gated
> > behind the CONFIG_PPC_RADIX_KHRAP option for performance-critical
> > builds.
> >
> > This feature can be tested by using the lkdtm driver
> > (CONFIG_LKDTM=y) and
> > performing the following:
> >
> > echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT
> >
> > if enabled, this should send SIGSEGV to the thread.
> >
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> > More detailed benchmarks soon, there's more optimisations here as
> > well.
>
> Nice, this turned out to be a lot neater than I feared! Good stuff.
>
> > @@ -240,6 +240,22 @@ BEGIN_FTR_SECTION_NESTED(941)
> > \
> > mtspr SPRN_PPR,ra;
> > \
> > END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,941)
> >
> > +#define LOCK_AMR(reg)
> > \
> > +BEGIN_MMU_FTR_SECTION_NESTED(69)
> > \
> > + LOAD_REG_IMMEDIATE(reg,AMR_LOCKED); \
> > + isync;
> > \
> > + mtspr SPRN_AMR,reg;
> > \
> > + isync;
> > \
> > +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP
> > ,69)
> > +
> > +#define UNLOCK_AMR(reg)
> > \
> > +BEGIN_MMU_FTR_SECTION_NESTED(420)
> > \
> > + li reg,0; \
> > + isync;
> > \
> > + mtspr SPRN_AMR,reg;
> > \
> > + isync;
> > \
> > +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP
> > ,420)
>
> I wonder if you can skip the first isync on the way in and the second
> isync on the way out because the interrupt and return should be
> context
> synchronizing. Might not make a difference though.
Ben thought we wouldn't need at least one of them, but it's
implementation dependent, so there might be some concern with future
chips actually needing both isyncs or something. There weren't any
consequences to leaving out isyncs, I'll do some quick benchmarking to
see if it's any meaningful speedup to leave one out.
>
> What do you think about making the name match the C code a bit more.
> Like AMR_LOCK_USER_ACCESS()?
That is a good idea.
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alistair Popple @ 2018-10-18 1:05 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <45098cfd-5420-f4bd-f7fe-394ce2c704d0@ozlabs.ru>
Hi Alexey,
> > wouldn't you also need to do that somewhere? Unless the driver
> > does it at startup?
>
> VFIO performs GPU reset so I'd expect the GPUs to flush its caches
> without any software interactions. Am I hoping for too much here?
Sadly you are. It's not the GPU caches that need flushing, it's the CPU caches.
This needs to happen as part of the reset sequence, so I guess you would need
to add it to the VFIO driver.
- Alistair
>
> > - Alistair
> >
> >>> - Alistair
> >>>
> >>>>> - Alistair
> >>>>>
> >>>>>>> - Alistair
> >>>>>>>
> >>>>>>> On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy
wrote:
> >>>>>>>> Ping?
> >>>>>>>>
> >>>>>>>> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
> >>>>>>>>> The skiboot firmware has a hot reset handler which fences the
> >>>>>>>>> NVIDIA V100
> >>>>>>>>> GPU RAM on Witherspoons and makes accesses no-op instead of
> >>>>>>>>> throwing HMIs:
> >>>>>>>>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
> >>>>>>>>>
> >>>>>>>>> Now we are going to pass V100 via VFIO which most certainly
> >>>>>>>>> involves
> >>>>>>>>> KVM guests which are often terminated without getting a chance to
> >>>>>>>>> offline
> >>>>>>>>> GPU RAM so we end up with a running machine with misconfigured
> >>>>>>>>> memory.
> >>>>>>>>> Accessing this memory produces hardware management interrupts
> >>>>>>>>> (HMI)
> >>>>>>>>> which bring the host down.
> >>>>>>>>>
> >>>>>>>>> To suppress HMIs, this wires up this hot reset hook to
> >>>>>>>>> vfio_pci_disable()
> >>>>>>>>> via pci_disable_device() which switches NPU2 to a safe mode and
> >>>>>>>>> prevents
> >>>>>>>>> HMIs.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>>>>> ---
> >>>>>>>>> Changes:
> >>>>>>>>> v2:
> >>>>>>>>> * updated the commit log
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
> >>>>>>>>> 1 file changed, 10 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>>>> b/arch/powerpc/platforms/powernv/pci-ioda.c index
> >>>>>>>>> cde7102..e37b9cc 100644
> >>>>>>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>>>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct
> >>>>>>>>> pci_dev *pdev)>>>>>>>>>
> >>>>>>>>> pnv_ioda_release_pe(pe);
> >>>>>>>>>
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
> >>>>>>>>> +{
> >>>>>>>>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
> >>>>>>>>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
> >>>>>>>>> +
> >>>>>>>>> + if (eehpe && eeh_ops && eeh_ops->reset)
> >>>>>>>>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
> >>>>>>>>> {
> >>>>>>>>>
> >>>>>>>>> struct pnv_phb *phb = hose->private_data;
> >>>>>>>>>
> >>>>>>>>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops
> >>>>>>>>> pnv_npu_ioda_controller_ops = {>>>>>>>>>
> >>>>>>>>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
> >>>>>>>>> .dma_set_mask = pnv_npu_dma_set_mask,
> >>>>>>>>> .shutdown = pnv_pci_ioda_shutdown,
> >>>>>>>>>
> >>>>>>>>> + .disable_device = pnv_npu_disable_device,
> >>>>>>>>>
> >>>>>>>>> };
> >>>>>>>>>
> >>>>>>>>> static const struct pci_controller_ops
> >>>>>>>>> pnv_npu_ocapi_ioda_controller_ops = {
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Alexey Kardashevskiy @ 2018-10-18 0:31 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Piotr Jaroszynski,
Reza Arbab, David Gibson
In-Reply-To: <20181017155252.2f15d0f0@w520.home>
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...
> 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.
>
>> 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.
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Piotr Jaroszyński @ 2018-10-17 23:42 UTC (permalink / raw)
To: Alex Williamson, Alexey Kardashevskiy
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Reza Arbab,
David Gibson
In-Reply-To: <20181017155252.2f15d0f0@w520.home>
On 10/17/18 2:52 PM, 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 ;) 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?
The tgt addresses are read by the GPU driver for each GPU from the
device tree properties and are used to program routing for all the GPUs
in the VM. Each GPU needs to know:
1) Its own address range so that it can route ATS accesses to it
directly to its RAM.
2) All direct peer GPUs (connected with nvlink directly) address ranges
so that it can route accesses to peers through links going directly to
those peers.
3) Everything else gets routed to the links going to the CPU.
Anticipating a question about the security implications of allowing the
guest to configure this routing, no, this is not a problem:
1) If a range gets misprogrammed causing an access to be routed to the
CPU nvlink incorrectly, the CPU still receives the full physical address
of the access and can drop or re-route it correctly.
2) If a range gets misprogrammed causing an access to go to a peer GPU
incorrectly, the guest can corrupt memory of that peer GPU, but it fully
owns that GPU anyway.
3) If a range gets misprogrammed causing an access to go to local GPU
memory incorrectly, the guest can corrupt that memory, but it fully owns
it anyway.
Thanks,
Piotr
>
>> 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).
>
>>>> /*
>>>> * 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? Is this tgt base used in
> the process of doing address translation shootdowns?
>
>>>> +};
>>>> +
>>>> /**
>>>> * 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? And the ATSD register that we need on it is not
> accessible through these PCI representations of the sub-pieces of the
> NPU? Thanks,
>
> 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-17 21:52 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Piotr Jaroszynski,
Reza Arbab, David Gibson
In-Reply-To: <71c11c53-c83d-b0b6-5036-574df45009e4@ozlabs.ru>
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 ;) 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?
> 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).
> >> /*
> >> * 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? Is this tgt base used in
the process of doing address translation shootdowns?
> >> +};
> >> +
> >> /**
> >> * 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? And the ATSD register that we need on it is not
accessible through these PCI representations of the sub-pieces of the
NPU? Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v4 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: Alan Tull @ 2018-10-17 21:30 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: <1539657458-24401-2-git-send-email-frowand.list@gmail.com>
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.
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";
};
};
};
};
I'll look at it some more tomorrow and try to figure out what's
special about this node.
Alan
> + } 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 00/18] of: overlay: validation checks, subsequent fixes
From: Alan Tull @ 2018-10-17 21:16 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: <bbbbe52c-8415-e22a-48d7-95f919942c68@gmail.com>
On Tue, Oct 16, 2018 at 10:08 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 10/16/18 02:47, Michael Ellerman wrote:
> > frowand.list@gmail.com writes:
> >
> >> 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
> >
> > Hi Frank,
> >
> > Do you have this series in a git tree somewhere?
> >
> > I tried applying it on top of linux-next but hit some conflicts which I
> > couldn't easily resolve.
> >
> > cheers
> >
>
>
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frowand/linux.git
>
> $ git checkout v4.19-rc1--kfree_validate--v4
>
> $ git log --oneline v4.19-rc1..
> 2ba1b7d353dd of: unittest: initialize args before calling of_*parse_*()
> 4f9108209f79 of: unittest: find overlays[] entry by name instead of index
> 353403c76ff8 of: unittest: allow base devicetree to have symbol metadata
> 8fc37e04a01b of: overlay: set node fields from properties when add new overlay n
> 05d5df0e5151 of: unittest: remove unused of_unittest_apply_overlay() argument
> 8c021cba757a of: overlay: check prevents multiple fragments touching same proper
> 797a6f66e039 of: overlay: check prevents multiple fragments add or delete same n
> c385e25a040d of: overlay: test case of two fragments adding same node
> c88fd240f0e0 of: overlay: make all pr_debug() and pr_err() messages unique
> 1028a215d32a of: overlay: validate overlay properties #address-cells and #size-c
> f1a97ef74ce4 of: overlay: reorder fields in struct fragment
> ffe78cf7a1fb of: dynamic: change type of of_{at,de}tach_node() to void
> 5f5ff8ec0c0c of: overlay: do not duplicate properties from overlay for new nodes
> 06e72dcb2bb0 of: overlay: use prop add changeset entry for property in new nodes
> a02f8d326a08 powerpc/pseries: add of_node_put() in dlpar_detach_node()
> e203be664330 of: overlay: add missing of_node_get() in __of_attach_node_sysfs
> 8eb46208e7c8 of: overlay: add missing of_node_put() after add new node to change
> b22067db7cf9 of: overlay: add tests to validate kfrees from overlay removal
That branch is a real time saver, thanks!
ALan
^ permalink raw reply
* [PATCH v07 5/5] migration/memory: Support 'ibm,dynamic-memory-v2'
From: Michael Bringmann @ 2018-10-17 19:38 UTC (permalink / raw)
To: linuxppc-dev, mwb
Cc: Juliet Kim, Thomas Falcon, Tyrel Datwyler, Nathan Fontenot
In-Reply-To: <20181017193745.4844.11915.stgit@ltcalpine2-lp9.aus.stglabs.ibm.com>
migration/memory: This patch adds recognition for changes to the
associativity of memory blocks described by 'ibm,dynamic-memory-v2'.
If the associativity of an LMB has changed, it should be readded to
the system in order to update local and general kernel data structures.
This patch builds upon previous enhancements that scan the device-tree
"ibm,dynamic-memory" properties using the base LMB array, and a copy
derived from the updated properties.
Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/hotplug-memory.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 6856010..03c5e49 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -1187,7 +1187,8 @@ static int pseries_memory_notifier(struct notifier_block *nb,
err = pseries_remove_mem_node(rd->dn);
break;
case OF_RECONFIG_UPDATE_PROPERTY:
- if (!strcmp(rd->prop->name, "ibm,dynamic-memory")) {
+ if (!strcmp(rd->prop->name, "ibm,dynamic-memory") ||
+ !strcmp(rd->prop->name, "ibm,dynamic-memory-v2")) {
struct drmem_lmb_info *dinfo =
drmem_lmbs_init(rd->prop);
if (!dinfo)
^ 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