* Re: Stack protector crash in pnv_smp_cpu_kill_self()
From: Segher Boessenkool @ 2018-10-16 14:23 UTC (permalink / raw)
To: Michael Ellerman
Cc: linuxppc-dev@ozlabs.org, Gautham R Shenoy, Nicholas Piggin,
Abdul Haleem
In-Reply-To: <87ftx6kowh.fsf@concordia.ellerman.id.au>
On Wed, Oct 17, 2018 at 12:21:50AM +1100, Michael Ellerman wrote:
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>
> > Looks like a lack of initialisation of the canary for the non-boot CPUs
> > on SMP, you applied this morning the patch I sent you for that.
> >
> > Is the patch in ?
>
> Yeah it is.
>
> $ git log --oneline 4ffe713b7587 arch/powerpc/kernel/smp.c
> 8e8a31d7fd54 powerpc: Use cpu_smallcore_sibling_mask at SMT level on bigcores
> 425752c63b6f powerpc: Detect the presence of big-cores via "ibm, thread-groups"
> 7241d26e8175 powerpc/64: properly initialise the stackprotector canary on SMP.
>
>
> It only happens on a specific Power9 machine, not in sim, but it's 100%
> reproducible on that hardware.
>
> The canary value has changed (?!).
>
> The value in paca->canary and current->canary agree, but they don't
> match what's in the stack.
>
> Clearly the idle code is doing something I don't understand :)
Did something actually corrupt the stack?
Segher
^ permalink raw reply
* [PATCH v2] powerpc/topology: Update numa mask when cpu node mapping changes
From: Srikar Dronamraju @ 2018-10-16 14:58 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Srikar Dronamraju, Michael Bringmann
Commit 2ea626306810 ("powerpc/topology: Get topology for shared
processors at boot") will update the cpu node topology for shared lpars
on PowerVM.
However shared lpars on PowerVM also support VPHN and PRRN events.
On receiving a VPHN, PRRN events, cpu to node mapping might change.
Scheduler maintains sched_domains_numa_masks[], which is currently not
updated on cpu to node mapping changes. This can lead to machine
hangs or performance hit.
Fix numa_update_cpu_topology() to update sched_domains_numa_masks[].
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
Changelog: v1->v2:
- Updated changelog.
- Updated comments to refer to topology_inited.
arch/powerpc/mm/numa.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 3ea5b05ee6be..767c363ebafa 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -29,6 +29,7 @@
#include <linux/seq_file.h>
#include <linux/uaccess.h>
#include <linux/slab.h>
+#include <linux/sched/hotplug.h>
#include <asm/cputhreads.h>
#include <asm/sparsemem.h>
#include <asm/prom.h>
@@ -1424,6 +1425,20 @@ int numa_update_cpu_topology(bool cpus_locked)
changed = 1;
}
+ /*
+ * Scheduler maintains a sched_domain_numa_masks table that needs to
+ * be updated whenever cpu-node mapping changes. Cpu-node mapping
+ * changes only with VPHN and PRRN. 'topology_inited' indicates if
+ * cpu-hotplug infrastructure is initialized and good to use.
+ */
+ if (!(topology_inited && (vphn_enabled || prrn_enabled)))
+ goto out;
+
+ for_each_cpu(cpu, &updated_cpus) {
+ sched_cpu_deactivate(cpu);
+ sched_cpu_activate(cpu);
+ }
+
out:
kfree(updates);
topology_update_needed = 0;
--
2.12.3
^ permalink raw reply related
* [PATCH 4.18 086/135] KVM: PPC: Book3S HV: Dont use compound_order to determine host mapping size
From: Greg Kroah-Hartman @ 2018-10-16 17:05 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Greg Kroah-Hartman, Nicholas Piggin, kvm-ppc, stable,
Aneesh Kumar K.V, linuxppc-dev, David Gibson
In-Reply-To: <20181016170515.447235311@linuxfoundation.org>
4.18-stable review patch. If anyone has any objections, please let me know.
------------------
From: Nicholas Piggin <npiggin@gmail.com>
[ Upstream commit 71d29f43b6332badc5598c656616a62575e83342 ]
THP paths can defer splitting compound pages until after the actual
remap and TLB flushes to split a huge PMD/PUD. This causes radix
partition scope page table mappings to get out of synch with the host
qemu page table mappings.
This results in random memory corruption in the guest when running
with THP. The easiest way to reproduce is use KVM balloon to free up
a lot of memory in the guest and then shrink the balloon to give the
memory back, while some work is being done in the guest.
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/powerpc/kvm/book3s_64_mmu_radix.c | 91 +++++++++++++--------------------
1 file changed, 37 insertions(+), 54 deletions(-)
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -538,8 +538,8 @@ int kvmppc_book3s_radix_page_fault(struc
unsigned long ea, unsigned long dsisr)
{
struct kvm *kvm = vcpu->kvm;
- unsigned long mmu_seq, pte_size;
- unsigned long gpa, gfn, hva, pfn;
+ unsigned long mmu_seq;
+ unsigned long gpa, gfn, hva;
struct kvm_memory_slot *memslot;
struct page *page = NULL;
long ret;
@@ -636,9 +636,10 @@ int kvmppc_book3s_radix_page_fault(struc
*/
hva = gfn_to_hva_memslot(memslot, gfn);
if (upgrade_p && __get_user_pages_fast(hva, 1, 1, &page) == 1) {
- pfn = page_to_pfn(page);
upgrade_write = true;
} else {
+ unsigned long pfn;
+
/* Call KVM generic code to do the slow-path check */
pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
writing, upgrade_p);
@@ -652,63 +653,45 @@ int kvmppc_book3s_radix_page_fault(struc
}
}
- /* See if we can insert a 1GB or 2MB large PTE here */
- level = 0;
- if (page && PageCompound(page)) {
- pte_size = PAGE_SIZE << compound_order(compound_head(page));
- if (pte_size >= PUD_SIZE &&
- (gpa & (PUD_SIZE - PAGE_SIZE)) ==
- (hva & (PUD_SIZE - PAGE_SIZE))) {
- level = 2;
- pfn &= ~((PUD_SIZE >> PAGE_SHIFT) - 1);
- } else if (pte_size >= PMD_SIZE &&
- (gpa & (PMD_SIZE - PAGE_SIZE)) ==
- (hva & (PMD_SIZE - PAGE_SIZE))) {
- level = 1;
- pfn &= ~((PMD_SIZE >> PAGE_SHIFT) - 1);
- }
- }
-
/*
- * Compute the PTE value that we need to insert.
+ * Read the PTE from the process' radix tree and use that
+ * so we get the shift and attribute bits.
*/
- if (page) {
- pgflags = _PAGE_READ | _PAGE_EXEC | _PAGE_PRESENT | _PAGE_PTE |
- _PAGE_ACCESSED;
- if (writing || upgrade_write)
- pgflags |= _PAGE_WRITE | _PAGE_DIRTY;
- pte = pfn_pte(pfn, __pgprot(pgflags));
+ local_irq_disable();
+ ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
+ pte = *ptep;
+ local_irq_enable();
+
+ /* Get pte level from shift/size */
+ if (shift == PUD_SHIFT &&
+ (gpa & (PUD_SIZE - PAGE_SIZE)) ==
+ (hva & (PUD_SIZE - PAGE_SIZE))) {
+ level = 2;
+ } else if (shift == PMD_SHIFT &&
+ (gpa & (PMD_SIZE - PAGE_SIZE)) ==
+ (hva & (PMD_SIZE - PAGE_SIZE))) {
+ level = 1;
} else {
- /*
- * Read the PTE from the process' radix tree and use that
- * so we get the attribute bits.
- */
- local_irq_disable();
- ptep = __find_linux_pte(vcpu->arch.pgdir, hva, NULL, &shift);
- pte = *ptep;
- local_irq_enable();
- if (shift == PUD_SHIFT &&
- (gpa & (PUD_SIZE - PAGE_SIZE)) ==
- (hva & (PUD_SIZE - PAGE_SIZE))) {
- level = 2;
- } else if (shift == PMD_SHIFT &&
- (gpa & (PMD_SIZE - PAGE_SIZE)) ==
- (hva & (PMD_SIZE - PAGE_SIZE))) {
- level = 1;
- } else if (shift && shift != PAGE_SHIFT) {
- /* Adjust PFN */
- unsigned long mask = (1ul << shift) - PAGE_SIZE;
- pte = __pte(pte_val(pte) | (hva & mask));
- }
- pte = __pte(pte_val(pte) | _PAGE_EXEC | _PAGE_ACCESSED);
- if (writing || upgrade_write) {
- if (pte_val(pte) & _PAGE_WRITE)
- pte = __pte(pte_val(pte) | _PAGE_DIRTY);
- } else {
- pte = __pte(pte_val(pte) & ~(_PAGE_WRITE | _PAGE_DIRTY));
+ level = 0;
+ if (shift > PAGE_SHIFT) {
+ /*
+ * If the pte maps more than one page, bring over
+ * bits from the virtual address to get the real
+ * address of the specific single page we want.
+ */
+ unsigned long rpnmask = (1ul << shift) - PAGE_SIZE;
+ pte = __pte(pte_val(pte) | (hva & rpnmask));
}
}
+ pte = __pte(pte_val(pte) | _PAGE_EXEC | _PAGE_ACCESSED);
+ if (writing || upgrade_write) {
+ if (pte_val(pte) & _PAGE_WRITE)
+ pte = __pte(pte_val(pte) | _PAGE_DIRTY);
+ } else {
+ pte = __pte(pte_val(pte) & ~(_PAGE_WRITE | _PAGE_DIRTY));
+ }
+
/* Allocate space in the tree and write the PTE */
ret = kvmppc_create_pte(kvm, pte, gpa, level, mmu_seq);
^ permalink raw reply
* What is an address translation in powerISA jarogn ?
From: Raz @ 2018-10-16 17:58 UTC (permalink / raw)
To: linuxppc-dev
Section 5.7.3
"Storage accesses in real, hypervisor real, and virtual real
addressing modes are performed in a manner that depends on the
contents of MSR HV , VPM, VRMASD, HRMOR, RMLS, RMOR (see Chapter 2),
bit 0 of the
effective address (EA0),"
Hello
1. If MSR_IR = 0 and MSR_DR = 0, does it mean that addresses are not
translated by the MMU ?
2. If EA0 is the 63-rd bit of the effective address e address ? Does
this mean that the translation model is
derived from the address ? a non privileged context may access
privileged memory.
thank you
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Alex Williamson @ 2018-10-16 19:08 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Reza Arbab,
David Gibson
In-Reply-To: <20181015094233.1324-4-aik@ozlabs.ru>
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.
> 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.
> /*
> * 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?
> +};
> +
> /**
> * 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?
> +
> + 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?
> }
>
> vfio_pci_probe_mmaps(vdev);
>
> return 0;
> +
> +disable_exit:
> + vfio_pci_disable(vdev);
> + return ret;
> }
>
> static void vfio_pci_disable(struct vfio_pci_device *vdev)
> diff --git a/drivers/vfio/pci/vfio_pci_nvlink2.c b/drivers/vfio/pci/vfio_pci_nvlink2.c
> new file mode 100644
> index 0000000..c9d2b55
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_nvlink2.c
> @@ -0,0 +1,409 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VFIO PCI NVIDIA Whitherspoon GPU support a.k.a. NVLink2.
> + *
> + * Copyright (C) 2018 IBM Corp. All rights reserved.
> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Register an on-GPU RAM region for cacheable access.
> + *
> + * Derived from original vfio_pci_igd.c:
> + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> + * Author: Alex Williamson <alex.williamson@redhat.com>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/pci.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/sched/mm.h>
> +#include <linux/mmu_context.h>
> +#include <asm/kvm_ppc.h>
> +#include "vfio_pci_private.h"
> +
Please steal some of your description in the commitlog to document
inline what this is all about.
> +struct vfio_pci_nvgpu_data {
> + unsigned long gpu_hpa;
> + unsigned long useraddr;
> + unsigned long size;
> + void *base;
> + struct mm_struct *mm;
> + struct mm_iommu_table_group_mem_t *mem;
> + struct pci_dev *gpdev;
> + struct notifier_block group_notifier;
> +};
> +
> +static size_t vfio_pci_nvgpu_rw(struct vfio_pci_device *vdev,
> + char __user *buf, size_t count, loff_t *ppos, bool iswrite)
> +{
> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - VFIO_PCI_NUM_REGIONS;
> + struct vfio_pci_nvgpu_data *data = vdev->region[i].data;
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> + if (pos >= vdev->region[i].size)
> + return -EINVAL;
> +
> + count = min(count, (size_t)(vdev->region[i].size - pos));
> +
> + if (iswrite) {
> + if (copy_from_user(data->base + pos, buf, count))
> + return -EFAULT;
> + } else {
> + if (copy_to_user(buf, data->base + pos, count))
> + return -EFAULT;
> + }
> + *ppos += count;
> +
> + return count;
> +}
> +
> +static void vfio_pci_nvgpu_release(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region)
> +{
> + struct vfio_pci_nvgpu_data *data = region->data;
> + long ret;
> + struct pci_controller *hose;
> + struct pci_dev *npdev;
> +
> + /* If there were any mappings at all... */
> + if (data->mm) {
> + ret = mm_iommu_put(data->mm, data->mem);
> + WARN_ON(ret);
> +
> + mmdrop(data->mm);
> + }
> +
> + vfio_unregister_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
> + &data->group_notifier);
> +
> + npdev = pnv_pci_get_npu_dev(data->gpdev, 0);
> + hose = pci_bus_to_host(npdev->bus);
> +
> + pnv_npu2_map_lpar_dev(hose, data->gpdev, 0, MSR_DR | MSR_PR | MSR_HV);
> +
> + memunmap(data->base);
> + kfree(data);
> +}
> +
> +static int vfio_pci_nvgpu_mmap_fault(struct vm_fault *vmf)
> +{
> + int ret;
> + struct vm_area_struct *vma = vmf->vma;
> + struct vfio_pci_region *region = vma->vm_private_data;
> + struct vfio_pci_nvgpu_data *data = region->data;
> + unsigned long vmf_off = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
> + unsigned long nv2pg = data->gpu_hpa >> PAGE_SHIFT;
> + unsigned long vm_pgoff = vma->vm_pgoff &
> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> + unsigned long pfn = nv2pg + vm_pgoff + vmf_off;
> +
> + ret = vm_insert_pfn(vma, vmf->address, pfn);
> + pr_debug("NVLink2: vmf=%lx hpa=%lx ret=%d\n",
> + vmf->address, pfn << PAGE_SHIFT, ret);
Tracing would probably be a better option if you intend to keep this.
> + if (ret)
> + return VM_FAULT_SIGSEGV;
> +
> + return VM_FAULT_NOPAGE;
> +}
> +
> +static const struct vm_operations_struct vfio_pci_nvgpu_mmap_vmops = {
> + .fault = vfio_pci_nvgpu_mmap_fault,
> +};
> +
> +static int vfio_pci_nvgpu_mmap(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region, struct vm_area_struct *vma)
> +{
> + long ret;
> + struct vfio_pci_nvgpu_data *data = region->data;
> +
> + if (data->useraddr)
> + return -EPERM;
> +
> + if (vma->vm_end - vma->vm_start > data->size)
> + return -EINVAL;
> +
> + vma->vm_private_data = region;
> + vma->vm_flags |= VM_PFNMAP;
> + vma->vm_ops = &vfio_pci_nvgpu_mmap_vmops;
> +
> + /*
> + * Calling mm_iommu_newdev() here once as the region is not
> + * registered yet and therefore right initialization will happen now.
> + * Other places will use mm_iommu_find() which returns
> + * registered @mem and does not go gup().
> + */
> + data->useraddr = vma->vm_start;
> + data->mm = current->mm;
> +
> + atomic_inc(&data->mm->mm_count);
> + ret = mm_iommu_newdev(data->mm, data->useraddr,
> + (vma->vm_end - vma->vm_start) >> PAGE_SHIFT,
> + data->gpu_hpa, &data->mem);
> +
> + pr_debug("VFIO NVLINK2 mmap: useraddr=%lx hpa=%lx size=%lx ret=%ld\n",
> + data->useraddr, data->gpu_hpa,
> + vma->vm_end - vma->vm_start, ret);
Same
> +
> + return ret;
> +}
> +
> +static const struct vfio_pci_regops vfio_pci_nvgpu_regops = {
> + .rw = vfio_pci_nvgpu_rw,
> + .release = vfio_pci_nvgpu_release,
> + .mmap = vfio_pci_nvgpu_mmap,
> +};
> +
> +static int vfio_pci_nvgpu_group_notifier(struct notifier_block *nb,
> + unsigned long action, void *opaque)
> +{
> + struct kvm *kvm = opaque;
> + struct vfio_pci_nvgpu_data *data = container_of(nb,
> + struct vfio_pci_nvgpu_data,
> + group_notifier);
> +
> + if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
> + struct pci_controller *hose;
> + struct pci_dev *npdev;
> + struct pnv_phb *nphb;
> +
> + npdev = pnv_pci_get_npu_dev(data->gpdev, 0);
> + hose = pci_bus_to_host(npdev->bus);
> + nphb = hose->private_data;
> +
> + if (!kvm) {
> + if (pnv_npu2_map_lpar_dev(hose, data->gpdev, 0,
> + MSR_DR | MSR_PR | MSR_HV))
> + return NOTIFY_BAD;
> + } else {
> + if (pnv_npu2_map_lpar_dev(hose, data->gpdev,
> + kvm->arch.lpid, MSR_DR | MSR_PR))
> + return NOTIFY_BAD;
> + }
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +int vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev)
> +{
> + int ret;
> + u64 reg[2];
> + struct device_node *npu_node, *mem_node;
> + struct pci_dev *npu_dev;
> + struct vfio_pci_nvgpu_data *data;
> + uint32_t mem_phandle = 0;
> + unsigned long events = VFIO_GROUP_NOTIFY_SET_KVM;
> +
> + npu_dev = pnv_pci_get_npu_dev(vdev->pdev, 0);
> + if (!npu_dev)
> + return -EINVAL;
> +
> + npu_node = pci_device_to_OF_node(npu_dev);
> + if (!npu_node)
> + return -EINVAL;
> +
> + if (of_property_read_u32(npu_node, "memory-region", &mem_phandle))
> + return -EINVAL;
> +
> + mem_node = of_find_node_by_phandle(mem_phandle);
> + if (!mem_node)
> + return -EINVAL;
> +
> + if (of_property_read_variable_u64_array(mem_node, "reg", reg,
> + ARRAY_SIZE(reg), ARRAY_SIZE(reg)) !=
> + ARRAY_SIZE(reg))
> + return -EINVAL;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->gpu_hpa = reg[0];
> + data->size = reg[1];
> + data->base = memremap(data->gpu_hpa, data->size, MEMREMAP_WB);
> + if (!data->base) {
> + ret = -ENOMEM;
> + goto free_exit;
> + }
> +
> + dev_dbg(&vdev->pdev->dev, "%lx..%lx\n", data->gpu_hpa,
> + data->gpu_hpa + data->size - 1);
> +
> + data->gpdev = vdev->pdev;
> + data->group_notifier.notifier_call = vfio_pci_nvgpu_group_notifier;
> +
> + ret = vfio_register_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
> + &events, &data->group_notifier);
> + if (ret)
> + goto free_exit;
> +
> + ret = vfio_pci_register_dev_region(vdev,
> + PCI_VENDOR_ID_NVIDIA | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
> + VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM,
> + &vfio_pci_nvgpu_regops, data->size,
> + VFIO_REGION_INFO_FLAG_READ, data);
Clearly WRITE and MMAP flags are supported above as well as READ.
> + if (ret)
> + goto unreg_exit;
> +
> + return 0;
> +unreg_exit:
> + vfio_unregister_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
> + &data->group_notifier);
> +free_exit:
> + kfree(data);
> +
> + return ret;
> +}
> +
> +/*
> + * IBM NPU2 bridge
> + */
> +struct vfio_pci_npu2_data {
> + void *base;
> + unsigned long mmio_atsd;
> + unsigned long gpu_tgt;
> +};
> +
> +static size_t vfio_pci_npu2_rw(struct vfio_pci_device *vdev,
> + char __user *buf, size_t count, loff_t *ppos, bool iswrite)
> +{
> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - VFIO_PCI_NUM_REGIONS;
> + struct vfio_pci_npu2_data *data = vdev->region[i].data;
> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +
> + if (pos >= vdev->region[i].size)
> + return -EINVAL;
> +
> + count = min(count, (size_t)(vdev->region[i].size - pos));
> +
> + if (iswrite) {
> + if (copy_from_user(data->base + pos, buf, count))
> + return -EFAULT;
> + } else {
> + if (copy_to_user(buf, data->base + pos, count))
> + return -EFAULT;
> + }
> + *ppos += count;
> +
> + return count;
> +}
> +
> +static int vfio_pci_npu2_mmap(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region, struct vm_area_struct *vma)
> +{
> + int ret;
> + struct vfio_pci_npu2_data *data = region->data;
> + unsigned long req_len = vma->vm_end - vma->vm_start;
> +
> + if (req_len != PAGE_SIZE)
> + return -EINVAL;
> +
> + vma->vm_flags |= VM_PFNMAP;
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +
> + ret = remap_pfn_range(vma, vma->vm_start, data->mmio_atsd >> PAGE_SHIFT,
> + req_len, vma->vm_page_prot);
> + pr_debug("VFIO NPU2 mmap: %lx %lx size=%lx ret=%d\n",
> + vma->vm_start, data->mmio_atsd,
> + vma->vm_end - vma->vm_start, ret);
For commit or convert to tracing?
> +
> + return ret;
> +}
> +
> +static void vfio_pci_npu2_release(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region)
> +{
> + struct vfio_pci_npu2_data *data = region->data;
> +
> + memunmap(data->base);
> + kfree(data);
> +}
> +
> +static int vfio_pci_npu2_add_capability(struct vfio_pci_device *vdev,
> + struct vfio_pci_region *region, struct vfio_info_cap *caps)
> +{
> + struct vfio_pci_npu2_data *data = region->data;
> + struct vfio_region_info_cap_npu2 cap;
> +
> + cap.header.id = VFIO_REGION_INFO_CAP_NPU2;
> + cap.header.version = 1;
> + cap.tgt = data->gpu_tgt;
> +
> + return vfio_info_add_capability(caps, &cap.header, sizeof(cap));
> +}
> +
> +static const struct vfio_pci_regops vfio_pci_npu2_regops = {
> + .rw = vfio_pci_npu2_rw,
> + .mmap = vfio_pci_npu2_mmap,
> + .release = vfio_pci_npu2_release,
> + .add_capability = vfio_pci_npu2_add_capability,
> +};
> +
> +int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
> +{
> + int ret;
> + struct vfio_pci_npu2_data *data;
> + struct device_node *nvlink_dn;
> + u32 nvlink_index = 0;
> + struct pci_dev *npdev = vdev->pdev;
> + struct device_node *npu_node = pci_device_to_OF_node(npdev);
> + struct pci_controller *hose = pci_bus_to_host(npdev->bus);
> + u64 mmio_atsd = 0;
> + u64 tgt = 0;
> +
> + /*
> + * NPU2 normally has 8 ATSD registers (for concurrency) and 6 links
> + * so we can allocate one register per link.
> + * Since skiboot only exposes one (a bug), use this as a fallback
> + * which is safe as we do not split GPUs attached to the same NPU.
> + */
> + nvlink_dn = of_parse_phandle(npdev->dev.of_node, "ibm,nvlink", 0);
> + if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
> + &nvlink_index)))
> + return -ENODEV;
> +
> + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", nvlink_index,
> + &mmio_atsd)) {
> + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", 0,
> + &mmio_atsd)) {
> + dev_warn(&vdev->pdev->dev, "No ATSD found\n");
> + return -EFAULT;
> + }
> + dev_warn(&vdev->pdev->dev, "Fallback to ATSD#0\n");
> + }
> +
> + if (of_property_read_u64(npu_node, "ibm,device-tgt-addr", &tgt)) {
> + dev_warn(&vdev->pdev->dev, "No ibm,device-tgt-addr found\n");
> + return -EFAULT;
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->mmio_atsd = mmio_atsd;
> + data->gpu_tgt = tgt;
> + data->base = memremap(data->mmio_atsd, SZ_64K, MEMREMAP_WT);
> + if (!data->base) {
> + ret = -ENOMEM;
> + goto free_exit;
> + }
> +
> + ret = vfio_pci_register_dev_region(vdev,
> + PCI_VENDOR_ID_IBM | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
> + VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD,
> + &vfio_pci_npu2_regops, PAGE_SIZE,
> + VFIO_REGION_INFO_FLAG_READ, data);
READ|WRITE|MMAP
> + if (ret)
> + goto free_exit;
> +
> + return 0;
> +
> +free_exit:
> + kfree(data);
> +
> + return ret;
> +}
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index 42dc1d3..1a58979 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -38,3 +38,7 @@ config VFIO_PCI_IGD
> and LPC bridge config space.
>
> To enable Intel IGD assignment through vfio-pci, say Y.
> +
> +config VFIO_PCI_NVLINK2
> + bool "VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs"
> + depends on VFIO_PCI && PPC_POWERNV
^ permalink raw reply
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Tyrel Datwyler @ 2018-10-16 19:57 UTC (permalink / raw)
To: Michael Ellerman, Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <87sh16lo72.fsf@concordia.ellerman.id.au>
On 10/15/2018 05:39 PM, Michael Ellerman wrote:
> Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index 2b796da..9c76345 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>> return rc;
>> }
>>
>> +static int dlpar_memory_readd_multiple(void)
>> +{
>> + struct drmem_lmb *lmb;
>> + int rc;
>> +
>> + pr_info("Attempting to update multiple LMBs\n");
>> +
>> + for_each_drmem_lmb(lmb) {
>> + if (drmem_lmb_update(lmb)) {
>> + rc = dlpar_memory_readd_helper(lmb);
>> + drmem_remove_lmb_update(lmb);
>> + }
>> + }
>> +
>> + return rc;
>> +}
>
> This leaves rc potentially uninitialised.
>
> What should the result be in that case, -EINVAL ?
On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call.
-Tyrel
>
> cheers
>
^ permalink raw reply
* Re: linux-next: Tree for Oct 15
From: Mike Rapoport @ 2018-10-16 13:36 UTC (permalink / raw)
To: Andrew Morton
Cc: Stephen Rothwell, Mike Rapoport, Linux Kernel Mailing List,
Linux-Next Mailing List, Rob Herring, PowerPC, Guenter Roeck
In-Reply-To: <20181015151319.3a443f0421c20bd7ed055cb9@linux-foundation.org>
On Mon, Oct 15, 2018 at 03:13:19PM -0700, Andrew Morton wrote:
> On Tue, 16 Oct 2018 07:24:39 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > On Tue, 16 Oct 2018 07:12:40 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > >
> > > On Mon, 15 Oct 2018 11:26:37 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > ALl ppc qemu tests (including big endian pseries) also generate a warning.
> > > >
> > > > WARNING: CPU: 0 PID: 0 at mm/memblock.c:1301 .memblock_alloc_range_nid+0x20/0x68
> >
> > That is:
> >
> > static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
> > phys_addr_t align, phys_addr_t start,
> > phys_addr_t end, int nid,
> > enum memblock_flags flags)
> > {
> > if (WARN_ON_ONCE(!align))
> > align = SMP_CACHE_BYTES;
> >
> > Looks like patch
> >
> > "memblock: stop using implicit alignment to SMP_CACHE_BYTES"
> >
> > missed some places ...
>
> To be expected, I guess. I'm pretty relaxed about this ;) Let's do
> another sweep in a week or so, after which we'll have a couple of
> months to mop up any leftovers.
After some more grepping and spatching I've found these:
From 8b014bae53a78ab747dbb76b9aff7df4cefcb604 Mon Sep 17 00:00:00 2001
From: Mike Rapoport <rppt@linux.ibm.com>
Date: Tue, 16 Oct 2018 16:03:00 +0300
Subject: [PATCH] memblock: fix missed uses of implicit aligment
A couple of memblock*alloc uses were missed during conversion from implicit
alignment setting with 'align = 0' to explictly using SMP_CACHE_BYTES.
Fix them now.
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
arch/powerpc/kernel/paca.c | 2 +-
drivers/firmware/efi/memmap.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index f331a00..913bfca 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -198,7 +198,7 @@ void __init allocate_paca_ptrs(void)
paca_nr_cpu_ids = nr_cpu_ids;
paca_ptrs_size = sizeof(struct paca_struct *) * nr_cpu_ids;
- paca_ptrs = __va(memblock_phys_alloc(paca_ptrs_size, 0));
+ paca_ptrs = __va(memblock_phys_alloc(paca_ptrs_size, SMP_CACHE_BYTES));
memset(paca_ptrs, 0x88, paca_ptrs_size);
}
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index ef618bc..fa2904f 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -15,7 +15,7 @@
static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
{
- return memblock_phys_alloc(size, 0);
+ return memblock_phys_alloc(size, SMP_CACHE_BYTES);
}
static phys_addr_t __init __efi_memmap_alloc_late(unsigned long size)
--
2.7.4
--
Sincerely yours,
Mike.
^ permalink raw reply related
* Re: [PATCH 2/4] mm: speed up mremap by 500x on large regions (v2)
From: Joel Fernandes @ 2018-10-16 19:43 UTC (permalink / raw)
To: Vlastimil Babka
Cc: linux-mips, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Catalin Marinas, Dave Hansen, Will Deacon, mhocko, linux-mm,
lokeshgidra, sparclinux, linux-riscv, kvmarm, Jonas Bonn,
linux-s390, dancol, Yoshinori Sato, Max Filippov, linux-hexagon,
Helge Deller, maintainer:X86 ARCHITECTURE 32-BIT AND 64-BIT,
hughd, James E.J. Bottomley, kasan-dev, elfring,
Christoph Hellwig, Ingo Molnar, Geert Uytterhoeven,
Andrey Ryabinin, linux-snps-arc, kernel-team, Sam Creasey,
linux-xtensa, Jeff Dike, linux-alpha, linux-um,
Stefan Kristiansson, Julia Lawall, linux-m68k, Borislav Petkov,
Andy Lutomirski, Ley Foon Tan, kirill, Stafford Horne,
Guan Xuetao, Chris Zankel, Tony Luck, linux-parisc, pantin,
linux-kernel, Fenghua Yu, minchan, Thomas Gleixner,
Richard Weinberger, anton.ivanov, nios2-dev, akpm, linuxppc-dev,
David S. Miller
In-Reply-To: <35b9c85a-b366-9ca3-5647-c2568c811961@suse.cz>
On Tue, Oct 16, 2018 at 01:29:52PM +0200, Vlastimil Babka wrote:
> On 10/16/18 12:33 AM, Joel Fernandes wrote:
> > On Mon, Oct 15, 2018 at 02:42:09AM -0700, Christoph Hellwig wrote:
> >> On Fri, Oct 12, 2018 at 06:31:58PM -0700, Joel Fernandes (Google) wrote:
> >>> Android needs to mremap large regions of memory during memory management
> >>> related operations.
> >>
> >> Just curious: why?
> >
> > In Android we have a requirement of moving a large (up to a GB now, but may
> > grow bigger in future) memory range from one location to another.
>
> I think Christoph's "why?" was about the requirement, not why it hurts
> applications. I admit I'm now also curious :)
This issue was discovered when we wanted to be able to move the physical
pages of a memory range to another location quickly so that, after the
application threads are resumed, UFFDIO_REGISTER_MODE_MISSING userfaultfd
faults can be received on the original memory range. The actual operations
performed on the memory range are beyond the scope of this discussion. The
user threads continue to refer to the old address which will now fault. The
reason we want retain the old memory range and receives faults there is to
avoid the need to fix the addresses all over the address space of the threads
after we finish with performing operations on them in the fault handlers, so
we mremap it and receive faults at the old addresses.
Does that answer your question?
thanks,
- Joel
^ permalink raw reply
* [PATCH] powerpc/ftrace: Handle large kernel configs
From: Naveen N. Rao @ 2018-10-16 20:25 UTC (permalink / raw)
To: Michael Ellerman, Michael Neuling, Nicholas Piggin,
Steven Rostedt
Cc: linuxppc-dev
Currently, we expect to be able to reach ftrace_caller() from all
ftrace-enabled functions through a single relative branch. With large
kernel configs, we see functions outside of 32MB of ftrace_caller()
causing ftrace_init() to bail.
In such configurations, gcc/ld emits two types of trampolines for mcount():
1. A long_branch, which has a single branch to mcount() for functions that
are one hop away from mcount():
c0000000019e8544 <00031b56.long_branch._mcount>:
c0000000019e8544: 4a 69 3f ac b c00000000007c4f0 <._mcount>
2. A plt_branch, for functions that are farther away from mcount():
c0000000051f33f8 <0008ba04.plt_branch._mcount>:
c0000000051f33f8: 3d 82 ff a4 addis r12,r2,-92
c0000000051f33fc: e9 8c 04 20 ld r12,1056(r12)
c0000000051f3400: 7d 89 03 a6 mtctr r12
c0000000051f3404: 4e 80 04 20 bctr
We can reuse those trampolines for ftrace if we can have those
trampolines go to ftrace_caller() instead. However, with ABIv2, we
cannot depend on r2 being valid. As such, we use only the long_branch
trampolines by patching those to instead branch to ftrace_caller or
ftrace_regs_caller.
In addition, we add additional trampolines around .text and .init.text
to catch locations that are covered by the plt branches. This allows
ftrace to work with most large kernel configurations.
For now, we always patch the trampolines to go to ftrace_regs_caller,
which is slightly inefficient. This can be optimized further at a later
point.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
Since RFC:
- Change to patch long_branch to go to ftrace_caller, rather than
patching mcount()
- Stop using plt_branch since it can't be relied on for ABIv2
- Add trampolines around .text and .init.text to catch remaining
locations
- Naveen
arch/powerpc/kernel/trace/ftrace.c | 261 +++++++++++++++++++++++++-
arch/powerpc/kernel/trace/ftrace_64.S | 12 ++
arch/powerpc/kernel/vmlinux.lds.S | 13 +-
3 files changed, 281 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 4bfbb54dee51..4bf051d3e21e 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -30,6 +30,16 @@
#ifdef CONFIG_DYNAMIC_FTRACE
+
+/*
+ * We generally only have a single long_branch tramp and at most 2 or 3 plt
+ * tramps generated. But, we don't use the plt tramps currently. We also allot
+ * 2 tramps after .text and .init.text. So, we only end up with around 3 usable
+ * tramps in total. Set aside 8 just to be sure.
+ */
+#define NUM_FTRACE_TRAMPS 8
+static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
+
static unsigned int
ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
{
@@ -85,13 +95,16 @@ static int test_24bit_addr(unsigned long ip, unsigned long addr)
return create_branch((unsigned int *)ip, addr, 0);
}
-#ifdef CONFIG_MODULES
-
static int is_bl_op(unsigned int op)
{
return (op & 0xfc000003) == 0x48000001;
}
+static int is_b_op(unsigned int op)
+{
+ return (op & 0xfc000003) == 0x48000000;
+}
+
static unsigned long find_bl_target(unsigned long ip, unsigned int op)
{
static int offset;
@@ -104,6 +117,7 @@ static unsigned long find_bl_target(unsigned long ip, unsigned int op)
return ip + (long)offset;
}
+#ifdef CONFIG_MODULES
#ifdef CONFIG_PPC64
static int
__ftrace_make_nop(struct module *mod,
@@ -270,6 +284,146 @@ __ftrace_make_nop(struct module *mod,
#endif /* PPC64 */
#endif /* CONFIG_MODULES */
+static unsigned long find_ftrace_tramp(unsigned long ip)
+{
+ int i;
+
+ /*
+ * We have the compiler generated long_branch tramps at the end
+ * and we prefer those
+ */
+ for (i = NUM_FTRACE_TRAMPS - 1; i >= 0; i--)
+ if (!ftrace_tramps[i])
+ continue;
+ else if (create_branch((void *)ip, ftrace_tramps[i], 0))
+ return ftrace_tramps[i];
+
+ return 0;
+}
+
+static int add_ftrace_tramp(unsigned long tramp)
+{
+ int i;
+
+ for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
+ if (!ftrace_tramps[i]) {
+ ftrace_tramps[i] = tramp;
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
+ * If this is a compiler generated long_branch trampoline (essentially, a
+ * trampoline that has a branch to _mcount()), we re-write the branch to
+ * instead go to ftrace_[regs_]caller() and note down the location of this
+ * trampoline.
+ */
+static int setup_mcount_compiler_tramp(unsigned long tramp)
+{
+ int i, op;
+ unsigned long ptr;
+ static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];
+
+ /* Is this a known long jump tramp? */
+ for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
+ if (!ftrace_tramps[i])
+ break;
+ else if (ftrace_tramps[i] == tramp)
+ return 0;
+
+ /* Is this a known plt tramp? */
+ for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
+ if (!ftrace_plt_tramps[i])
+ break;
+ else if (ftrace_plt_tramps[i] == tramp)
+ return -1;
+
+ /* New trampoline -- read where this goes */
+ if (probe_kernel_read(&op, (void *)tramp, sizeof(int))) {
+ pr_debug("Fetching opcode failed.\n");
+ return -1;
+ }
+
+ /* Is this a 24 bit branch? */
+ if (!is_b_op(op)) {
+ pr_debug("Trampoline is not a long branch tramp.\n");
+ return -1;
+ }
+
+ /* lets find where the pointer goes */
+ ptr = find_bl_target(tramp, op);
+
+ if (ptr != ppc_global_function_entry((void *)_mcount)) {
+ pr_debug("Trampoline target %p is not _mcount\n", (void *)ptr);
+ return -1;
+ }
+
+ /* Let's re-write the tramp to go to ftrace_[regs_]caller */
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ ptr = ppc_global_function_entry((void *)ftrace_regs_caller);
+#else
+ ptr = ppc_global_function_entry((void *)ftrace_caller);
+#endif
+ if (!create_branch((void *)tramp, ptr, 0)) {
+ pr_debug("%ps is not reachable from existing mcount tramp\n",
+ (void *)ptr);
+ return -1;
+ }
+
+ if (patch_branch((unsigned int *)tramp, ptr, 0)) {
+ pr_debug("REL24 out of range!\n");
+ return -1;
+ }
+
+ if (add_ftrace_tramp(tramp)) {
+ pr_debug("No tramp locations left\n");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned long tramp, ip = rec->ip;
+ unsigned int op;
+
+ /* Read where this goes */
+ if (probe_kernel_read(&op, (void *)ip, sizeof(int))) {
+ pr_err("Fetching opcode failed.\n");
+ return -EFAULT;
+ }
+
+ /* Make sure that that this is still a 24bit jump */
+ if (!is_bl_op(op)) {
+ pr_err("Not expected bl: opcode is %x\n", op);
+ return -EINVAL;
+ }
+
+ /* Let's find where the pointer goes */
+ tramp = find_bl_target(ip, op);
+
+ pr_devel("ip:%lx jumps to %lx", ip, tramp);
+
+ if (setup_mcount_compiler_tramp(tramp)) {
+ /* Are other trampolines reachable? */
+ if (!find_ftrace_tramp(ip)) {
+ pr_err("No ftrace trampolines reachable from %ps\n",
+ (void *)ip);
+ return -EINVAL;
+ }
+ }
+
+ if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
+ pr_err("Patching NOP failed.\n");
+ return -EPERM;
+ }
+
+ return 0;
+}
+
int ftrace_make_nop(struct module *mod,
struct dyn_ftrace *rec, unsigned long addr)
{
@@ -286,7 +440,8 @@ int ftrace_make_nop(struct module *mod,
old = ftrace_call_replace(ip, addr, 1);
new = PPC_INST_NOP;
return ftrace_modify_code(ip, old, new);
- }
+ } else if (core_kernel_text(ip))
+ return __ftrace_make_nop_kernel(rec, addr);
#ifdef CONFIG_MODULES
/*
@@ -456,6 +611,53 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
#endif /* CONFIG_PPC64 */
#endif /* CONFIG_MODULES */
+static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
+{
+ unsigned int op;
+ void *ip = (void *)rec->ip;
+ unsigned long tramp, entry, ptr;
+
+ /* Make sure we're being asked to patch branch to a known ftrace addr */
+ entry = ppc_global_function_entry((void *)ftrace_caller);
+ ptr = ppc_global_function_entry((void *)addr);
+
+ if (ptr != entry) {
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ entry = ppc_global_function_entry((void *)ftrace_regs_caller);
+ if (ptr != entry) {
+#endif
+ pr_err("Unknown ftrace addr to patch: %ps\n", (void *)ptr);
+ return -EINVAL;
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ }
+#endif
+ }
+
+ /* Make sure we have a nop */
+ if (probe_kernel_read(&op, ip, sizeof(op))) {
+ pr_err("Unable to read ftrace location %p\n", ip);
+ return -EFAULT;
+ }
+
+ if (op != PPC_INST_NOP) {
+ pr_err("Unexpected call sequence at %p: %x\n", ip, op);
+ return -EINVAL;
+ }
+
+ tramp = find_ftrace_tramp((unsigned long)ip);
+ if (!tramp) {
+ pr_err("No ftrace trampolines reachable from %ps\n", ip);
+ return -EINVAL;
+ }
+
+ if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
+ pr_err("Error patching branch to ftrace tramp!\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned long ip = rec->ip;
@@ -471,7 +673,8 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
old = PPC_INST_NOP;
new = ftrace_call_replace(ip, addr, 1);
return ftrace_modify_code(ip, old, new);
- }
+ } else if (core_kernel_text(ip))
+ return __ftrace_make_call_kernel(rec, addr);
#ifdef CONFIG_MODULES
/*
@@ -603,6 +806,12 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
old = ftrace_call_replace(ip, old_addr, 1);
new = ftrace_call_replace(ip, addr, 1);
return ftrace_modify_code(ip, old, new);
+ } else if (core_kernel_text(ip)) {
+ /*
+ * We always patch out of range locations to go to the regs
+ * variant, so there is nothing to do here
+ */
+ return 0;
}
#ifdef CONFIG_MODULES
@@ -654,10 +863,54 @@ void arch_ftrace_update_code(int command)
ftrace_modify_all_code(command);
}
+#ifdef CONFIG_PPC64
+#define PACATOC offsetof(struct paca_struct, kernel_toc)
+
+#define PPC_LO(v) ((v) & 0xffff)
+#define PPC_HI(v) (((v) >> 16) & 0xffff)
+#define PPC_HA(v) PPC_HI ((v) + 0x8000)
+
+extern unsigned int ftrace_tramp_text[], ftrace_tramp_init[];
+
+int __init ftrace_dyn_arch_init(void)
+{
+ int i;
+ unsigned int *tramp[] = { ftrace_tramp_text, ftrace_tramp_init };
+ u32 stub_insns[] = {
+ 0xe98d0000 | PACATOC, /* ld r12,PACATOC(r13) */
+ 0x3d8c0000, /* addis r12,r12,<high> */
+ 0x398c0000, /* addi r12,r12,<low> */
+ 0x7d8903a6, /* mtctr r12 */
+ 0x4e800420, /* bctr */
+ };
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ unsigned long addr = ppc_global_function_entry((void *)ftrace_regs_caller);
+#else
+ unsigned long addr = ppc_global_function_entry((void *)ftrace_caller);
+#endif
+ long reladdr = addr - kernel_toc_addr();
+
+ if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
+ pr_err("Address of %ps out of range of kernel_toc.\n",
+ (void *)addr);
+ return -1;
+ }
+
+ for (i = 0; i < 2; i++) {
+ memcpy(tramp[i], stub_insns, sizeof(stub_insns));
+ tramp[i][1] |= PPC_HA(reladdr);
+ tramp[i][2] |= PPC_LO(reladdr);
+ add_ftrace_tramp((unsigned long)tramp[i]);
+ }
+
+ return 0;
+}
+#else
int __init ftrace_dyn_arch_init(void)
{
return 0;
}
+#endif
#endif /* CONFIG_DYNAMIC_FTRACE */
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
diff --git a/arch/powerpc/kernel/trace/ftrace_64.S b/arch/powerpc/kernel/trace/ftrace_64.S
index e25f77c10a72..1782af2d1496 100644
--- a/arch/powerpc/kernel/trace/ftrace_64.S
+++ b/arch/powerpc/kernel/trace/ftrace_64.S
@@ -14,6 +14,18 @@
#include <asm/ppc-opcode.h>
#include <asm/export.h>
+.pushsection ".tramp.ftrace.text","aw",@progbits;
+.globl ftrace_tramp_text
+ftrace_tramp_text:
+ .space 64
+.popsection
+
+.pushsection ".tramp.ftrace.init","aw",@progbits;
+.globl ftrace_tramp_init
+ftrace_tramp_init:
+ .space 64
+.popsection
+
_GLOBAL(mcount)
_GLOBAL(_mcount)
EXPORT_SYMBOL(_mcount)
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 07ae018e550e..b9045abea1b7 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -99,6 +99,9 @@ SECTIONS
#endif
/* careful! __ftr_alt_* sections need to be close to .text */
*(.text.hot TEXT_MAIN .text.fixup .text.unlikely .fixup __ftr_alt_* .ref.text);
+#ifdef CONFIG_PPC64
+ *(.tramp.ftrace.text);
+#endif
SCHED_TEXT
CPUIDLE_TEXT
LOCK_TEXT
@@ -181,7 +184,15 @@ SECTIONS
*/
. = ALIGN(STRICT_ALIGN_SIZE);
__init_begin = .;
- INIT_TEXT_SECTION(PAGE_SIZE) :kernel
+ . = ALIGN(PAGE_SIZE);
+ .init.text : AT(ADDR(.init.text) - LOAD_OFFSET) {
+ _sinittext = .;
+ INIT_TEXT
+ _einittext = .;
+#ifdef CONFIG_PPC64
+ *(.tramp.ftrace.init);
+#endif
+ } :kernel
/* .exit.text is discarded at runtime, not link time,
* to deal with references from __bug_table
--
2.19.1
^ permalink raw reply related
* Re: linux-next: Tree for Oct 15
From: Stephen Rothwell @ 2018-10-16 20:41 UTC (permalink / raw)
To: Mike Rapoport
Cc: Mike Rapoport, Linux Kernel Mailing List, Linux-Next Mailing List,
Rob Herring, Andrew Morton, PowerPC, Guenter Roeck
In-Reply-To: <20181016133656.GA10925@rapoport-lnx>
[-- Attachment #1: Type: text/plain, Size: 773 bytes --]
Hi Mike,
On Tue, 16 Oct 2018 16:36:56 +0300 Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> After some more grepping and spatching I've found these:
>
> From 8b014bae53a78ab747dbb76b9aff7df4cefcb604 Mon Sep 17 00:00:00 2001
> From: Mike Rapoport <rppt@linux.ibm.com>
> Date: Tue, 16 Oct 2018 16:03:00 +0300
> Subject: [PATCH] memblock: fix missed uses of implicit aligment
>
> A couple of memblock*alloc uses were missed during conversion from implicit
> alignment setting with 'align = 0' to explictly using SMP_CACHE_BYTES.
> Fix them now.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
Thanks.
I have added that to linux-next today as it should get rid of the stack
trace we are seeing in PowerPC boots.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Michael Bringmann @ 2018-10-16 21:31 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Ellerman, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <80494db2-6a01-059f-890c-df7c410714f7@linux.vnet.ibm.com>
On 10/16/2018 02:57 PM, Tyrel Datwyler wrote:
> On 10/15/2018 05:39 PM, Michael Ellerman wrote:
>> Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index 2b796da..9c76345 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>>> return rc;
>>> }
>>>
>>> +static int dlpar_memory_readd_multiple(void)
>>> +{
>>> + struct drmem_lmb *lmb;
>>> + int rc;
>>> +
>>> + pr_info("Attempting to update multiple LMBs\n");
>>> +
>>> + for_each_drmem_lmb(lmb) {
>>> + if (drmem_lmb_update(lmb)) {
>>> + rc = dlpar_memory_readd_helper(lmb);
>>> + drmem_remove_lmb_update(lmb);
>>> + }
>>> + }
>>> +
>>> + return rc;
>>> +}
>>
>> This leaves rc potentially uninitialised.
>>
>> What should the result be in that case, -EINVAL ?
>
> On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call.
Correct. But that is what happens when we compress common code
between two disparate uses i.e. updating memory association after
a migration event with no reporting mechanism other than the console
log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr.
I could discard the return value from dlpar_memory_readd_helper entirely
in this function and just return 0, but in my experience, once errors start
to occur in memory dlpar ops, they tend to keep on occurring, so I was
returning the last one. We could also make the code smart enough to
capture and return the first/last non-zero return code. I didn't believe
that the frequency of errors for this operation warranted the overhead.
>
> -Tyrel
Michael
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH 4.18 086/135] KVM: PPC: Book3S HV: Dont use compound_order to determine host mapping size
From: Paul Mackerras @ 2018-10-16 22:32 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Aneesh Kumar K.V, Nicholas Piggin, linux-kernel, kvm-ppc,
Sasha Levin, stable, linuxppc-dev, David Gibson
In-Reply-To: <20181016170521.241312038@linuxfoundation.org>
On Tue, Oct 16, 2018 at 07:05:16PM +0200, Greg Kroah-Hartman wrote:
> 4.18-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Nicholas Piggin <npiggin@gmail.com>
>
> [ Upstream commit 71d29f43b6332badc5598c656616a62575e83342 ]
If you take 71d29f43b633 then you also need 6579804c4317 ("KVM: PPC:
Book3S HV: Avoid crash from THP collapse during radix page fault",
2018-10-04).
Thanks,
Paul.
^ permalink raw reply
* Re: [PATCH v5 3/6] clk: qoriq: increase array size of cmux_to_group
From: Stephen Boyd @ 2018-10-16 22:44 UTC (permalink / raw)
To: Vabhav Sharma, arnd, catalin.marinas, devicetree, gregkh,
kstewart, leoyang.li, linux-arm-kernel, linux-clk,
linux-kernel-owner, linux-kernel, linux-pm, linuxppc-dev,
mark.rutland, mturquette, oss, rjw, robh+dt, shawnguo,
sudeep.holla, viresh.kumar, will.deacon, yamada.masahiro
Cc: ulf.hansson, Yogesh Gaur, udit.kumar, pankaj.bansal, linux,
adrian.hunter, V.Sethi, Vabhav Sharma
In-Reply-To: <1539536883-1928-4-git-send-email-vabhav.sharma@nxp.com>
Quoting Vabhav Sharma (2018-10-14 10:08:00)
> From: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
>
> Increase size of cmux_to_group array, to accomdate entry of
> -1 termination.
>
> Added -1, terminated, entry for 4080_cmux_grpX.
>
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> Signed-off-by: Vabhav Sharma <vabhav.sharma@nxp.com>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: What is an address translation in powerISA jarogn ?
From: Benjamin Herrenschmidt @ 2018-10-17 0:14 UTC (permalink / raw)
To: Raz, linuxppc-dev
In-Reply-To: <CAPB=Z-ospEB9cyjaqk1zKy8axVKnri4=A=wBRHB_fMM_MKgCbA@mail.gmail.com>
On Tue, 2018-10-16 at 20:58 +0300, Raz wrote:
> Section 5.7.3
> "Storage accesses in real, hypervisor real, and virtual real
> addressing modes are performed in a manner that depends on the
> contents of MSR HV , VPM, VRMASD, HRMOR, RMLS, RMOR (see Chapter 2),
> bit 0 of the
> effective address (EA0),"
>
> Hello
> 1. If MSR_IR = 0 and MSR_DR = 0, does it mean that addresses are not
> translated by the MMU ?
It depends.
If HV=1 (hypervisor mode), then they are only translated to the extent
that HRMOR is applied if the MSB is 0, and untranslated if the MSB is
1.
If HV=0 (guest mode), then they *are* translated but using a different
mechanism than what's normally used when IR/DR=1. This mechanism
depends on whether you are using the Radix or the Hash MMU, and the top
2 bits are ignored.
With hash MMU, it's using things like VRMASD etc... (RMOR is deprecated
afaik) to lookup a "Virtual real mode" area in the hash table. It's
essentially a mapping of the guest "physical" space to real physical
space. It's usually initialized (and maintained) by the HV but the
guest can extend it using things like H_ENTER afaik.
With the radix MMU, it's the guest physical space as mapped by the 2nd
level page tables maintained by the hypervisor.
> 2. If EA0 is the 63-rd bit of the effective address e address ? Does
> this mean that the translation model is
> derived from the address ? a non privileged context may access
> privileged memory.
Nope.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Michael Ellerman @ 2018-10-17 0:43 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <80494db2-6a01-059f-890c-df7c410714f7@linux.vnet.ibm.com>
Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> On 10/15/2018 05:39 PM, Michael Ellerman wrote:
>> Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> index 2b796da..9c76345 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>>> return rc;
>>> }
>>>
>>> +static int dlpar_memory_readd_multiple(void)
>>> +{
>>> + struct drmem_lmb *lmb;
>>> + int rc;
>>> +
>>> + pr_info("Attempting to update multiple LMBs\n");
>>> +
>>> + for_each_drmem_lmb(lmb) {
>>> + if (drmem_lmb_update(lmb)) {
>>> + rc = dlpar_memory_readd_helper(lmb);
>>> + drmem_remove_lmb_update(lmb);
>>> + }
>>> + }
>>> +
>>> + return rc;
>>> +}
>>
>> This leaves rc potentially uninitialised.
>>
>> What should the result be in that case, -EINVAL ?
>
> On another note if there are multiple LMBs to update the value of rc
> only reflects the final dlpar_memory_readd_helper() call.
Good point.
cheers
^ permalink raw reply
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Michael Ellerman @ 2018-10-17 0:48 UTC (permalink / raw)
To: Michael Bringmann, Tyrel Datwyler, linuxppc-dev
Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <8fa9d2c2-97a9-69ac-1e3b-cdf867e225e2@linux.vnet.ibm.com>
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> On 10/16/2018 02:57 PM, Tyrel Datwyler wrote:
>> On 10/15/2018 05:39 PM, Michael Ellerman wrote:
>>> Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
>>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> index 2b796da..9c76345 100644
>>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>>>> return rc;
>>>> }
>>>>
>>>> +static int dlpar_memory_readd_multiple(void)
>>>> +{
>>>> + struct drmem_lmb *lmb;
>>>> + int rc;
>>>> +
>>>> + pr_info("Attempting to update multiple LMBs\n");
>>>> +
>>>> + for_each_drmem_lmb(lmb) {
>>>> + if (drmem_lmb_update(lmb)) {
>>>> + rc = dlpar_memory_readd_helper(lmb);
>>>> + drmem_remove_lmb_update(lmb);
>>>> + }
>>>> + }
>>>> +
>>>> + return rc;
>>>> +}
>>>
>>> This leaves rc potentially uninitialised.
>>>
>>> What should the result be in that case, -EINVAL ?
>>
>> On another note if there are multiple LMBs to update the value of rc only reflects the final dlpar_memory_readd_helper() call.
>
> Correct. But that is what happens when we compress common code
> between two disparate uses i.e. updating memory association after
> a migration event with no reporting mechanism other than the console
> log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr.
>
> I could discard the return value from dlpar_memory_readd_helper entirely
> in this function and just return 0, but in my experience, once errors start
> to occur in memory dlpar ops, they tend to keep on occurring, so I was
> returning the last one. We could also make the code smart enough to
> capture and return the first/last non-zero return code. I didn't believe
> that the frequency of errors for this operation warranted the overhead.
The actual error value is probably not very relevant.
But dropping errors entirely is almost always a bad idea.
So I think you should at least return an error if any error occurred,
that way at least an error will be returned up to the caller(s).
Something like:
int rc;
rc = 0;
for_each_drmem_lmb(lmb) {
if (drmem_lmb_update(lmb)) {
rc |= dlpar_memory_readd_helper(lmb);
drmem_remove_lmb_update(lmb);
}
}
if (rc)
return -EIO;
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/io: remove old GCC version implementation
From: Michael Ellerman @ 2018-10-17 0:51 UTC (permalink / raw)
To: Joakim Tjernlund, christophe.leroy@c-s.fr, paulus@samba.org,
benh@kernel.crashing.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <8eb877e31130aee531409d18e19894d64690163d.camel@infinera.com>
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> writes:
> On Tue, 2018-10-16 at 12:33 +0000, Christophe Leroy wrote:
>>
>>
>> GCC 4.6 is the minimum supported now.
>
> Ouch, from kernel 4.19 or earlier even ?
Yes.
Though to be honest we haven't been testing much with compilers older
than that for several years.
If you care about something older please let us know :)
cheers
^ permalink raw reply
* Crash on FSL Book3E due to pte_pgprot()? (was Re: [PATCH v3 12/24] powerpc/mm: use pte helpers in generic code)
From: Michael Ellerman @ 2018-10-17 0:59 UTC (permalink / raw)
To: Christophe Leroy, Bartlomiej Zolnierkiewicz,
Benjamin Herrenschmidt, Dominik Brodowski, Geoff Levand,
Jens Axboe, Kumar Gala, Li Yang, Nicholas Piggin, Paul Mackerras,
Scott Wood, aneesh.kumar, benh@kernel.crashing.org
Cc: linux-fbdev, linux-kernel, dri-devel, linux-block, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <343c844bbc5081d13ee4c9aa27ff3118f607e1cc.1539092112.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Get rid of platform specific _PAGE_XXXX in powerpc common code and
> use helpers instead.
>
> mm/dump_linuxpagetables.c will be handled separately
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/book3s/32/pgtable.h | 9 +++------
> arch/powerpc/include/asm/nohash/32/pgtable.h | 12 ++++++++----
> arch/powerpc/include/asm/nohash/pgtable.h | 3 +--
> arch/powerpc/mm/pgtable.c | 21 +++++++--------------
> arch/powerpc/mm/pgtable_32.c | 15 ++++++++-------
> arch/powerpc/mm/pgtable_64.c | 14 +++++++-------
> arch/powerpc/xmon/xmon.c | 12 +++++++-----
> 7 files changed, 41 insertions(+), 45 deletions(-)
So turns out this patch *also* breaks my p5020ds :)
Even with patch 4 merged, see next.
It's the same crash:
pcieport 2000:00:00.0: AER enabled with IRQ 480
Unable to handle kernel paging request for data at address 0x8000080080080000
Faulting instruction address: 0xc0000000000192cc
Oops: Kernel access of bad area, sig: 11 [#1]
BE SMP NR_CPUS=24 CoreNet Generic
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc3-gcc7x-g98c847323b3a #1
NIP: c0000000000192cc LR: c0000000005d0f9c CTR: 0000000000100000
REGS: c0000000f31bb400 TRAP: 0300 Not tainted (4.19.0-rc3-gcc7x-g98c847323b3a)
MSR: 0000000080029000 <CE,EE,ME> CR: 24000224 XER: 00000000
DEAR: 8000080080080000 ESR: 0000000000800000 IRQMASK: 0
GPR00: c0000000005d0f84 c0000000f31bb688 c00000000117dc00 8000080080080000
GPR04: 0000000000000000 0000000000400000 00000ffbff241010 c0000000f31b8000
GPR08: 0000000000000000 0000000000100000 0000000000000000 c0000000012d4710
GPR12: 0000000084000422 c0000000012ff000 c000000000002774 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000 8000080080080000 c0000000ffff89a8
GPR28: c0000000f3576400 c0000000f3576410 0000000000400000 c0000000012ecc98
NIP [c0000000000192cc] ._memset_io+0x6c/0x9c
LR [c0000000005d0f9c] .fsl_qman_probe+0x198/0x928
Call Trace:
[c0000000f31bb688] [c0000000005d0f84] .fsl_qman_probe+0x180/0x928 (unreliable)
[c0000000f31bb728] [c0000000006432ec] .platform_drv_probe+0x60/0xb4
[c0000000f31bb7a8] [c00000000064083c] .really_probe+0x294/0x35c
[c0000000f31bb848] [c000000000640d2c] .__driver_attach+0x148/0x14c
[c0000000f31bb8d8] [c00000000063d7dc] .bus_for_each_dev+0xb0/0x118
[c0000000f31bb988] [c00000000063ff28] .driver_attach+0x34/0x4c
[c0000000f31bba08] [c00000000063f648] .bus_add_driver+0x174/0x2bc
[c0000000f31bbaa8] [c0000000006418bc] .driver_register+0x90/0x180
[c0000000f31bbb28] [c000000000643270] .__platform_driver_register+0x60/0x7c
[c0000000f31bbba8] [c000000000ee2a70] .fsl_qman_driver_init+0x24/0x38
[c0000000f31bbc18] [c0000000000023fc] .do_one_initcall+0x64/0x2b8
[c0000000f31bbcf8] [c000000000e9f480] .kernel_init_freeable+0x3a8/0x494
[c0000000f31bbda8] [c000000000002798] .kernel_init+0x24/0x148
[c0000000f31bbe28] [c0000000000009e8] .ret_from_kernel_thread+0x58/0x70
Instruction dump:
4e800020 2ba50003 40dd003c 3925fffc 5488402e 7929f082 7d082378 39290001
550a801e 7d2903a6 7d4a4378 794a0020 <91430000> 38630004 4200fff8 70a50003
Comparing a working vs broken kernel, it seems to boil down to the fact
that we're filtering out more PTE bits now that we use pte_pgprot() in
ioremap_prot().
With the old code we get:
ioremap_prot: addr 0xff800000 flags 0x241215
ioremap_prot: addr 0xff800000 flags 0x241215
map_kernel_page: ea 0x8000080080080000 pa 0xff800000 pte 0xff800241215
And now we get:
ioremap_prot: addr 0xff800000 flags 0x241215 pte 0x241215
ioremap_prot: addr 0xff800000 pte 0x241215
ioremap_prot: addr 0xff800000 prot 0x241014
map_kernel_page: ea 0x8000080080080000 pa 0xff800000 pte 0xff800241014
So we're losing 0x201, which for nohash book3e is:
#define _PAGE_PRESENT 0x000001 /* software: pte contains a translation */
#define _PAGE_PSIZE_4K 0x000200
I haven't worked out if it's one or both of those that matter.
The question is what's the right way to fix it? Should pte_pgprot() not
be filtering those bits out on book3e?
cheers
^ permalink raw reply
* Re: [PATCH kernel 3/3] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] [10de:1db1] subdriver
From: Alexey Kardashevskiy @ 2018-10-17 1:19 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, Alistair Popple, linuxppc-dev, kvm-ppc, Piotr Jaroszynski,
Reza Arbab, David Gibson
In-Reply-To: <20181016130824.20be215b@w520.home>
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.
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"?
>
>> /*
>> * 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...
>
>> +};
>> +
>> /**
>> * 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.
>
>> }
>>
>> vfio_pci_probe_mmaps(vdev);
>>
>> return 0;
>> +
>> +disable_exit:
>> + vfio_pci_disable(vdev);
>> + return ret;
>> }
>>
>> static void vfio_pci_disable(struct vfio_pci_device *vdev)
>> diff --git a/drivers/vfio/pci/vfio_pci_nvlink2.c b/drivers/vfio/pci/vfio_pci_nvlink2.c
>> new file mode 100644
>> index 0000000..c9d2b55
>> --- /dev/null
>> +++ b/drivers/vfio/pci/vfio_pci_nvlink2.c
>> @@ -0,0 +1,409 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * VFIO PCI NVIDIA Whitherspoon GPU support a.k.a. NVLink2.
>> + *
>> + * Copyright (C) 2018 IBM Corp. All rights reserved.
>> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Register an on-GPU RAM region for cacheable access.
>> + *
>> + * Derived from original vfio_pci_igd.c:
>> + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
>> + * Author: Alex Williamson <alex.williamson@redhat.com>
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/pci.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/vfio.h>
>> +#include <linux/sched/mm.h>
>> +#include <linux/mmu_context.h>
>> +#include <asm/kvm_ppc.h>
>> +#include "vfio_pci_private.h"
>> +
>
> Please steal some of your description in the commitlog to document
> inline what this is all about.
>
>> +struct vfio_pci_nvgpu_data {
>> + unsigned long gpu_hpa;
>> + unsigned long useraddr;
>> + unsigned long size;
>> + void *base;
>> + struct mm_struct *mm;
>> + struct mm_iommu_table_group_mem_t *mem;
>> + struct pci_dev *gpdev;
>> + struct notifier_block group_notifier;
>> +};
>> +
>> +static size_t vfio_pci_nvgpu_rw(struct vfio_pci_device *vdev,
>> + char __user *buf, size_t count, loff_t *ppos, bool iswrite)
>> +{
>> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - VFIO_PCI_NUM_REGIONS;
>> + struct vfio_pci_nvgpu_data *data = vdev->region[i].data;
>> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +
>> + if (pos >= vdev->region[i].size)
>> + return -EINVAL;
>> +
>> + count = min(count, (size_t)(vdev->region[i].size - pos));
>> +
>> + if (iswrite) {
>> + if (copy_from_user(data->base + pos, buf, count))
>> + return -EFAULT;
>> + } else {
>> + if (copy_to_user(buf, data->base + pos, count))
>> + return -EFAULT;
>> + }
>> + *ppos += count;
>> +
>> + return count;
>> +}
>> +
>> +static void vfio_pci_nvgpu_release(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region)
>> +{
>> + struct vfio_pci_nvgpu_data *data = region->data;
>> + long ret;
>> + struct pci_controller *hose;
>> + struct pci_dev *npdev;
>> +
>> + /* If there were any mappings at all... */
>> + if (data->mm) {
>> + ret = mm_iommu_put(data->mm, data->mem);
>> + WARN_ON(ret);
>> +
>> + mmdrop(data->mm);
>> + }
>> +
>> + vfio_unregister_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
>> + &data->group_notifier);
>> +
>> + npdev = pnv_pci_get_npu_dev(data->gpdev, 0);
>> + hose = pci_bus_to_host(npdev->bus);
>> +
>> + pnv_npu2_map_lpar_dev(hose, data->gpdev, 0, MSR_DR | MSR_PR | MSR_HV);
>> +
>> + memunmap(data->base);
>> + kfree(data);
>> +}
>> +
>> +static int vfio_pci_nvgpu_mmap_fault(struct vm_fault *vmf)
>> +{
>> + int ret;
>> + struct vm_area_struct *vma = vmf->vma;
>> + struct vfio_pci_region *region = vma->vm_private_data;
>> + struct vfio_pci_nvgpu_data *data = region->data;
>> + unsigned long vmf_off = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>> + unsigned long nv2pg = data->gpu_hpa >> PAGE_SHIFT;
>> + unsigned long vm_pgoff = vma->vm_pgoff &
>> + ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>> + unsigned long pfn = nv2pg + vm_pgoff + vmf_off;
>> +
>> + ret = vm_insert_pfn(vma, vmf->address, pfn);
>> + pr_debug("NVLink2: vmf=%lx hpa=%lx ret=%d\n",
>> + vmf->address, pfn << PAGE_SHIFT, ret);
>
> Tracing would probably be a better option if you intend to keep this.
Ok, tracing it will be here and later.
>
>> + if (ret)
>> + return VM_FAULT_SIGSEGV;
>> +
>> + return VM_FAULT_NOPAGE;
>> +}
>> +
>> +static const struct vm_operations_struct vfio_pci_nvgpu_mmap_vmops = {
>> + .fault = vfio_pci_nvgpu_mmap_fault,
>> +};
>> +
>> +static int vfio_pci_nvgpu_mmap(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region, struct vm_area_struct *vma)
>> +{
>> + long ret;
>> + struct vfio_pci_nvgpu_data *data = region->data;
>> +
>> + if (data->useraddr)
>> + return -EPERM;
>> +
>> + if (vma->vm_end - vma->vm_start > data->size)
>> + return -EINVAL;
>> +
>> + vma->vm_private_data = region;
>> + vma->vm_flags |= VM_PFNMAP;
>> + vma->vm_ops = &vfio_pci_nvgpu_mmap_vmops;
>> +
>> + /*
>> + * Calling mm_iommu_newdev() here once as the region is not
>> + * registered yet and therefore right initialization will happen now.
>> + * Other places will use mm_iommu_find() which returns
>> + * registered @mem and does not go gup().
>> + */
>> + data->useraddr = vma->vm_start;
>> + data->mm = current->mm;
>> +
>> + atomic_inc(&data->mm->mm_count);
>> + ret = mm_iommu_newdev(data->mm, data->useraddr,
>> + (vma->vm_end - vma->vm_start) >> PAGE_SHIFT,
>> + data->gpu_hpa, &data->mem);
>> +
>> + pr_debug("VFIO NVLINK2 mmap: useraddr=%lx hpa=%lx size=%lx ret=%ld\n",
>> + data->useraddr, data->gpu_hpa,
>> + vma->vm_end - vma->vm_start, ret);
>
> Same
>
>> +
>> + return ret;
>> +}
>> +
>> +static const struct vfio_pci_regops vfio_pci_nvgpu_regops = {
>> + .rw = vfio_pci_nvgpu_rw,
>> + .release = vfio_pci_nvgpu_release,
>> + .mmap = vfio_pci_nvgpu_mmap,
>> +};
>> +
>> +static int vfio_pci_nvgpu_group_notifier(struct notifier_block *nb,
>> + unsigned long action, void *opaque)
>> +{
>> + struct kvm *kvm = opaque;
>> + struct vfio_pci_nvgpu_data *data = container_of(nb,
>> + struct vfio_pci_nvgpu_data,
>> + group_notifier);
>> +
>> + if (action == VFIO_GROUP_NOTIFY_SET_KVM) {
>> + struct pci_controller *hose;
>> + struct pci_dev *npdev;
>> + struct pnv_phb *nphb;
>> +
>> + npdev = pnv_pci_get_npu_dev(data->gpdev, 0);
>> + hose = pci_bus_to_host(npdev->bus);
>> + nphb = hose->private_data;
>> +
>> + if (!kvm) {
>> + if (pnv_npu2_map_lpar_dev(hose, data->gpdev, 0,
>> + MSR_DR | MSR_PR | MSR_HV))
>> + return NOTIFY_BAD;
>> + } else {
>> + if (pnv_npu2_map_lpar_dev(hose, data->gpdev,
>> + kvm->arch.lpid, MSR_DR | MSR_PR))
>> + return NOTIFY_BAD;
>> + }
>> + }
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +int vfio_pci_nvdia_v100_nvlink2_init(struct vfio_pci_device *vdev)
>> +{
>> + int ret;
>> + u64 reg[2];
>> + struct device_node *npu_node, *mem_node;
>> + struct pci_dev *npu_dev;
>> + struct vfio_pci_nvgpu_data *data;
>> + uint32_t mem_phandle = 0;
>> + unsigned long events = VFIO_GROUP_NOTIFY_SET_KVM;
>> +
>> + npu_dev = pnv_pci_get_npu_dev(vdev->pdev, 0);
>> + if (!npu_dev)
>> + return -EINVAL;
>> +
>> + npu_node = pci_device_to_OF_node(npu_dev);
>> + if (!npu_node)
>> + return -EINVAL;
>> +
>> + if (of_property_read_u32(npu_node, "memory-region", &mem_phandle))
>> + return -EINVAL;
>> +
>> + mem_node = of_find_node_by_phandle(mem_phandle);
>> + if (!mem_node)
>> + return -EINVAL;
>> +
>> + if (of_property_read_variable_u64_array(mem_node, "reg", reg,
>> + ARRAY_SIZE(reg), ARRAY_SIZE(reg)) !=
>> + ARRAY_SIZE(reg))
>> + return -EINVAL;
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->gpu_hpa = reg[0];
>> + data->size = reg[1];
>> + data->base = memremap(data->gpu_hpa, data->size, MEMREMAP_WB);
>> + if (!data->base) {
>> + ret = -ENOMEM;
>> + goto free_exit;
>> + }
>> +
>> + dev_dbg(&vdev->pdev->dev, "%lx..%lx\n", data->gpu_hpa,
>> + data->gpu_hpa + data->size - 1);
>> +
>> + data->gpdev = vdev->pdev;
>> + data->group_notifier.notifier_call = vfio_pci_nvgpu_group_notifier;
>> +
>> + ret = vfio_register_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
>> + &events, &data->group_notifier);
>> + if (ret)
>> + goto free_exit;
>> +
>> + ret = vfio_pci_register_dev_region(vdev,
>> + PCI_VENDOR_ID_NVIDIA | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
>> + VFIO_REGION_SUBTYPE_NVIDIA_NVLINK2_RAM,
>> + &vfio_pci_nvgpu_regops, data->size,
>> + VFIO_REGION_INFO_FLAG_READ, data);
>
> Clearly WRITE and MMAP flags are supported above as well as READ.
Ah, right. These are informational flags so I did not hit problems
before. I should probably check for these in "[PATCH kernel 1/3]
vfio_pci: Allow mapping extra regions" and block mmap if MMAP is not set
then.
>
>> + if (ret)
>> + goto unreg_exit;
>> +
>> + return 0;
>> +unreg_exit:
>> + vfio_unregister_notifier(&data->gpdev->dev, VFIO_GROUP_NOTIFY,
>> + &data->group_notifier);
>> +free_exit:
>> + kfree(data);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * IBM NPU2 bridge
>> + */
>> +struct vfio_pci_npu2_data {
>> + void *base;
>> + unsigned long mmio_atsd;
>> + unsigned long gpu_tgt;
>> +};
>> +
>> +static size_t vfio_pci_npu2_rw(struct vfio_pci_device *vdev,
>> + char __user *buf, size_t count, loff_t *ppos, bool iswrite)
>> +{
>> + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - VFIO_PCI_NUM_REGIONS;
>> + struct vfio_pci_npu2_data *data = vdev->region[i].data;
>> + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
>> +
>> + if (pos >= vdev->region[i].size)
>> + return -EINVAL;
>> +
>> + count = min(count, (size_t)(vdev->region[i].size - pos));
>> +
>> + if (iswrite) {
>> + if (copy_from_user(data->base + pos, buf, count))
>> + return -EFAULT;
>> + } else {
>> + if (copy_to_user(buf, data->base + pos, count))
>> + return -EFAULT;
>> + }
>> + *ppos += count;
>> +
>> + return count;
>> +}
>> +
>> +static int vfio_pci_npu2_mmap(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region, struct vm_area_struct *vma)
>> +{
>> + int ret;
>> + struct vfio_pci_npu2_data *data = region->data;
>> + unsigned long req_len = vma->vm_end - vma->vm_start;
>> +
>> + if (req_len != PAGE_SIZE)
>> + return -EINVAL;
>> +
>> + vma->vm_flags |= VM_PFNMAP;
>> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +
>> + ret = remap_pfn_range(vma, vma->vm_start, data->mmio_atsd >> PAGE_SHIFT,
>> + req_len, vma->vm_page_prot);
>> + pr_debug("VFIO NPU2 mmap: %lx %lx size=%lx ret=%d\n",
>> + vma->vm_start, data->mmio_atsd,
>> + vma->vm_end - vma->vm_start, ret);
>
> For commit or convert to tracing?
>
>> +
>> + return ret;
>> +}
>> +
>> +static void vfio_pci_npu2_release(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region)
>> +{
>> + struct vfio_pci_npu2_data *data = region->data;
>> +
>> + memunmap(data->base);
>> + kfree(data);
>> +}
>> +
>> +static int vfio_pci_npu2_add_capability(struct vfio_pci_device *vdev,
>> + struct vfio_pci_region *region, struct vfio_info_cap *caps)
>> +{
>> + struct vfio_pci_npu2_data *data = region->data;
>> + struct vfio_region_info_cap_npu2 cap;
>> +
>> + cap.header.id = VFIO_REGION_INFO_CAP_NPU2;
>> + cap.header.version = 1;
>> + cap.tgt = data->gpu_tgt;
>> +
>> + return vfio_info_add_capability(caps, &cap.header, sizeof(cap));
>> +}
>> +
>> +static const struct vfio_pci_regops vfio_pci_npu2_regops = {
>> + .rw = vfio_pci_npu2_rw,
>> + .mmap = vfio_pci_npu2_mmap,
>> + .release = vfio_pci_npu2_release,
>> + .add_capability = vfio_pci_npu2_add_capability,
>> +};
>> +
>> +int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
>> +{
>> + int ret;
>> + struct vfio_pci_npu2_data *data;
>> + struct device_node *nvlink_dn;
>> + u32 nvlink_index = 0;
>> + struct pci_dev *npdev = vdev->pdev;
>> + struct device_node *npu_node = pci_device_to_OF_node(npdev);
>> + struct pci_controller *hose = pci_bus_to_host(npdev->bus);
>> + u64 mmio_atsd = 0;
>> + u64 tgt = 0;
>> +
>> + /*
>> + * NPU2 normally has 8 ATSD registers (for concurrency) and 6 links
>> + * so we can allocate one register per link.
>> + * Since skiboot only exposes one (a bug), use this as a fallback
>> + * which is safe as we do not split GPUs attached to the same NPU.
>> + */
>> + nvlink_dn = of_parse_phandle(npdev->dev.of_node, "ibm,nvlink", 0);
>> + if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
>> + &nvlink_index)))
>> + return -ENODEV;
>> +
>> + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", nvlink_index,
>> + &mmio_atsd)) {
>> + if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", 0,
>> + &mmio_atsd)) {
>> + dev_warn(&vdev->pdev->dev, "No ATSD found\n");
>> + return -EFAULT;
>> + }
>> + dev_warn(&vdev->pdev->dev, "Fallback to ATSD#0\n");
>> + }
>> +
>> + if (of_property_read_u64(npu_node, "ibm,device-tgt-addr", &tgt)) {
>> + dev_warn(&vdev->pdev->dev, "No ibm,device-tgt-addr found\n");
>> + return -EFAULT;
>> + }
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->mmio_atsd = mmio_atsd;
>> + data->gpu_tgt = tgt;
>> + data->base = memremap(data->mmio_atsd, SZ_64K, MEMREMAP_WT);
>> + if (!data->base) {
>> + ret = -ENOMEM;
>> + goto free_exit;
>> + }
>> +
>> + ret = vfio_pci_register_dev_region(vdev,
>> + PCI_VENDOR_ID_IBM | VFIO_REGION_TYPE_PCI_VENDOR_TYPE,
>> + VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD,
>> + &vfio_pci_npu2_regops, PAGE_SIZE,
>> + VFIO_REGION_INFO_FLAG_READ, data);
>
> READ|WRITE|MMAP
>
>> + if (ret)
>> + goto free_exit;
>> +
>> + return 0;
>> +
>> +free_exit:
>> + kfree(data);
>> +
>> + return ret;
>> +}
>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>> index 42dc1d3..1a58979 100644
>> --- a/drivers/vfio/pci/Kconfig
>> +++ b/drivers/vfio/pci/Kconfig
>> @@ -38,3 +38,7 @@ config VFIO_PCI_IGD
>> and LPC bridge config space.
>>
>> To enable Intel IGD assignment through vfio-pci, say Y.
>> +
>> +config VFIO_PCI_NVLINK2
>> + bool "VFIO PCI support for P9 Witherspoon machine with NVIDIA V100 GPUs"
>> + depends on VFIO_PCI && PPC_POWERNV
>
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel 2/4] powerpc/mm/iommu/vfio_spapr_tce: Change mm_iommu_get to reference a region
From: David Gibson @ 2018-10-17 0:46 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <20181015092416.47380-3-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 3941 bytes --]
On Mon, Oct 15, 2018 at 08:24:14PM +1100, Alexey Kardashevskiy wrote:
> We are going to add another helper to preregister device memory so
> instead of having mm_iommu_new() which pre-registers the normal memory
> and references the region, we need separate helpers for pre-registerign
> and referencing.
>
> To make the mm_iommu_get name reflect what it is supposed to do, this
> changes mm_iommu_get() to reference the region so from now on for every
> mm_iommu_get() we need a matching mm_iommu_put().
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
.. ah, I see.
I think this should be folded with the first patch, so we don't have
an interim step where mm_iommu_get() has a misleading name.
> ---
> arch/powerpc/mm/mmu_context_iommu.c | 5 +++++
> drivers/vfio/vfio_iommu_spapr_tce.c | 33 ++++++++++++++++++++++-----------
> 2 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
> index 8eeb99d..a8c4a3c 100644
> --- a/arch/powerpc/mm/mmu_context_iommu.c
> +++ b/arch/powerpc/mm/mmu_context_iommu.c
> @@ -373,13 +373,18 @@ struct mm_iommu_table_group_mem_t *mm_iommu_get(struct mm_struct *mm,
> {
> struct mm_iommu_table_group_mem_t *mem, *ret = NULL;
>
> + mutex_lock(&mem_list_mutex);
> +
> list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list, next) {
> if ((mem->ua == ua) && (mem->entries == entries)) {
> ret = mem;
> + ++mem->used;
> break;
> }
> }
>
> + mutex_unlock(&mem_list_mutex);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(mm_iommu_get);
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 1701798..56db071 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -151,7 +151,8 @@ static long tce_iommu_unregister_pages(struct tce_container *container,
> {
> struct mm_iommu_table_group_mem_t *mem;
> struct tce_iommu_prereg *tcemem;
> - bool found = false;
> + bool found;
> + long ret;
>
> if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
> return -EINVAL;
> @@ -168,9 +169,13 @@ static long tce_iommu_unregister_pages(struct tce_container *container,
> }
>
> if (!found)
> - return -ENOENT;
> + ret = -ENOENT;
> + else
> + ret = tce_iommu_prereg_free(container, tcemem);
>
> - return tce_iommu_prereg_free(container, tcemem);
> + mm_iommu_put(container->mm, mem);
> +
> + return ret;
> }
>
> static long tce_iommu_register_pages(struct tce_container *container,
> @@ -188,19 +193,21 @@ static long tce_iommu_register_pages(struct tce_container *container,
> mem = mm_iommu_get(container->mm, vaddr, entries);
> if (mem) {
> list_for_each_entry(tcemem, &container->prereg_list, next) {
> - if (tcemem->mem == mem)
> - return -EBUSY;
> + if (tcemem->mem == mem) {
> + ret = -EBUSY;
> + goto put_exit;
> + }
> }
> + } else {
> + ret = mm_iommu_new(container->mm, vaddr, entries, &mem);
> + if (ret)
> + return ret;
> }
>
> - ret = mm_iommu_new(container->mm, vaddr, entries, &mem);
> - if (ret)
> - return ret;
> -
> tcemem = kzalloc(sizeof(*tcemem), GFP_KERNEL);
> if (!tcemem) {
> - mm_iommu_put(container->mm, mem);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto put_exit;
> }
>
> tcemem->mem = mem;
> @@ -209,6 +216,10 @@ static long tce_iommu_register_pages(struct tce_container *container,
> container->enabled = true;
>
> return 0;
> +
> +put_exit:
> + mm_iommu_put(container->mm, mem);
> + return ret;
> }
>
> static bool tce_page_is_contained(struct page *page, unsigned page_shift)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH kernel 1/4] powerpc/mm/iommu: Rename mm_iommu_get
From: David Gibson @ 2018-10-17 0:44 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <20181015092416.47380-2-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 5232 bytes --]
On Mon, Oct 15, 2018 at 08:24:13PM +1100, Alexey Kardashevskiy wrote:
> Normally mm_iommu_get() is supposed to add a reference and
> mm_iommu_put() to remove it. However historically mm_iommu_find() does
> the referencing and mm_iommu_get() is doing allocation and referencing.
>
> This is step 1 towards simpler mm_iommu_get().
>
> This renames:
> - mm_iommu_get to mm_iommu_new;
> - mm_iommu_find to mm_iommu_get.
>
> This should cause no behavioural change.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Erm.. as far as I can tell, mm_iommu_find() doesn't adjust any
reference counts, so renaming it to mm_iommu_get() doesn't really make
sense.
> ---
> arch/powerpc/include/asm/mmu_context.h | 4 ++--
> arch/powerpc/mm/mmu_context_iommu.c | 8 ++++----
> drivers/vfio/vfio_iommu_spapr_tce.c | 6 +++---
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index b694d6a..59d4941 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -21,7 +21,7 @@ struct mm_iommu_table_group_mem_t;
>
> extern int isolate_lru_page(struct page *page); /* from internal.h */
> extern bool mm_iommu_preregistered(struct mm_struct *mm);
> -extern long mm_iommu_get(struct mm_struct *mm,
> +extern long mm_iommu_new(struct mm_struct *mm,
> unsigned long ua, unsigned long entries,
> struct mm_iommu_table_group_mem_t **pmem);
> extern long mm_iommu_put(struct mm_struct *mm,
> @@ -32,7 +32,7 @@ extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup(struct mm_struct *mm,
> unsigned long ua, unsigned long size);
> extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup_rm(
> struct mm_struct *mm, unsigned long ua, unsigned long size);
> -extern struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm,
> +extern struct mm_iommu_table_group_mem_t *mm_iommu_get(struct mm_struct *mm,
> unsigned long ua, unsigned long entries);
> extern long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem,
> unsigned long ua, unsigned int pageshift, unsigned long *hpa);
> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
> index 56c2234..8eeb99d 100644
> --- a/arch/powerpc/mm/mmu_context_iommu.c
> +++ b/arch/powerpc/mm/mmu_context_iommu.c
> @@ -126,7 +126,7 @@ static int mm_iommu_move_page_from_cma(struct page *page)
> return 0;
> }
>
> -long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> +long mm_iommu_new(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> struct mm_iommu_table_group_mem_t **pmem)
> {
> struct mm_iommu_table_group_mem_t *mem;
> @@ -252,7 +252,7 @@ long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(mm_iommu_get);
> +EXPORT_SYMBOL_GPL(mm_iommu_new);
>
> static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem)
> {
> @@ -368,7 +368,7 @@ struct mm_iommu_table_group_mem_t *mm_iommu_lookup_rm(struct mm_struct *mm,
> return ret;
> }
>
> -struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm,
> +struct mm_iommu_table_group_mem_t *mm_iommu_get(struct mm_struct *mm,
> unsigned long ua, unsigned long entries)
> {
> struct mm_iommu_table_group_mem_t *mem, *ret = NULL;
> @@ -382,7 +382,7 @@ struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm,
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(mm_iommu_find);
> +EXPORT_SYMBOL_GPL(mm_iommu_get);
>
> long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem,
> unsigned long ua, unsigned int pageshift, unsigned long *hpa)
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index ad63725..1701798 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -156,7 +156,7 @@ static long tce_iommu_unregister_pages(struct tce_container *container,
> if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
> return -EINVAL;
>
> - mem = mm_iommu_find(container->mm, vaddr, size >> PAGE_SHIFT);
> + mem = mm_iommu_get(container->mm, vaddr, size >> PAGE_SHIFT);
> if (!mem)
> return -ENOENT;
>
> @@ -185,7 +185,7 @@ static long tce_iommu_register_pages(struct tce_container *container,
> ((vaddr + size) < vaddr))
> return -EINVAL;
>
> - mem = mm_iommu_find(container->mm, vaddr, entries);
> + mem = mm_iommu_get(container->mm, vaddr, entries);
> if (mem) {
> list_for_each_entry(tcemem, &container->prereg_list, next) {
> if (tcemem->mem == mem)
> @@ -193,7 +193,7 @@ static long tce_iommu_register_pages(struct tce_container *container,
> }
> }
>
> - ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
> + ret = mm_iommu_new(container->mm, vaddr, entries, &mem);
> if (ret)
> return ret;
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH kernel 3/4] powerpc/mm/iommu: Make mm_iommu_new() fail on existing regions
From: David Gibson @ 2018-10-17 1:00 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <20181015092416.47380-4-aik@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]
On Mon, Oct 15, 2018 at 08:24:15PM +1100, Alexey Kardashevskiy wrote:
> Since we are going to have 2 different preregistering helpers, let's
> make it clear that mm_iommu_new() is only for the normal (i.e. not device)
> memory and for existing areas mm_iommu_get() should be used instead.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
I think the idea is sensible. However (and, yes, this is really an
existing bug) - shouldn't we check for a request to add anything
overlapping with an existing region, not just one that exactly
matches?
> ---
> arch/powerpc/mm/mmu_context_iommu.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
> index a8c4a3c..839dbce 100644
> --- a/arch/powerpc/mm/mmu_context_iommu.c
> +++ b/arch/powerpc/mm/mmu_context_iommu.c
> @@ -141,8 +141,7 @@ long mm_iommu_new(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list,
> next) {
> if ((mem->ua == ua) && (mem->entries == entries)) {
> - ++mem->used;
> - *pmem = mem;
> + ret = -EBUSY;
> goto unlock_exit;
> }
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 00/18] of: overlay: validation checks, subsequent fixes
From: Frank Rowand @ 2018-10-17 3:08 UTC (permalink / raw)
To: Michael Ellerman, Rob Herring, Pantelis Antoniou,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <87murekyts.fsf@concordia.ellerman.id.au>
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
^ permalink raw reply
* Re: [PATCH kernel 3/4] powerpc/mm/iommu: Make mm_iommu_new() fail on existing regions
From: Alexey Kardashevskiy @ 2018-10-17 3:34 UTC (permalink / raw)
To: David Gibson; +Cc: Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <20181017010022.GA30180@umbus.fritz.box>
On 17/10/2018 12:00, David Gibson wrote:
> On Mon, Oct 15, 2018 at 08:24:15PM +1100, Alexey Kardashevskiy wrote:
>> Since we are going to have 2 different preregistering helpers, let's
>> make it clear that mm_iommu_new() is only for the normal (i.e. not device)
>> memory and for existing areas mm_iommu_get() should be used instead.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> I think the idea is sensible. However (and, yes, this is really an
> existing bug) - shouldn't we check for a request to add anything
> overlapping with an existing region, not just one that exactly
> matches?
The overlap check is below the changed hunk:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/mm/mmu_context_iommu.c#n150
>
>> ---
>> arch/powerpc/mm/mmu_context_iommu.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
>> index a8c4a3c..839dbce 100644
>> --- a/arch/powerpc/mm/mmu_context_iommu.c
>> +++ b/arch/powerpc/mm/mmu_context_iommu.c
>> @@ -141,8 +141,7 @@ long mm_iommu_new(struct mm_struct *mm, unsigned long ua, unsigned long entries,
>> list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list,
>> next) {
>> if ((mem->ua == ua) && (mem->entries == entries)) {
>> - ++mem->used;
>> - *pmem = mem;
>> + ret = -EBUSY;
>> goto unlock_exit;
>> }
>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel 3/4] powerpc/mm/iommu: Make mm_iommu_new() fail on existing regions
From: David Gibson @ 2018-10-17 4:45 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <39886bd3-5122-de68-f167-e010a1d12c33@ozlabs.ru>
[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]
On Wed, Oct 17, 2018 at 02:34:32PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 17/10/2018 12:00, David Gibson wrote:
> > On Mon, Oct 15, 2018 at 08:24:15PM +1100, Alexey Kardashevskiy wrote:
> >> Since we are going to have 2 different preregistering helpers, let's
> >> make it clear that mm_iommu_new() is only for the normal (i.e. not device)
> >> memory and for existing areas mm_iommu_get() should be used instead.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> > I think the idea is sensible. However (and, yes, this is really an
> > existing bug) - shouldn't we check for a request to add anything
> > overlapping with an existing region, not just one that exactly
> > matches?
>
> The overlap check is below the changed hunk:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/mm/mmu_context_iommu.c#n150
Ah, right.
In that case can't you just drop this whole if. I don't see that
there's any use in giving different error codes for "tried to register
exactly a region you registered before" and "tried to register a
region overlapping one you registered before.
>
>
> >
> >> ---
> >> arch/powerpc/mm/mmu_context_iommu.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
> >> index a8c4a3c..839dbce 100644
> >> --- a/arch/powerpc/mm/mmu_context_iommu.c
> >> +++ b/arch/powerpc/mm/mmu_context_iommu.c
> >> @@ -141,8 +141,7 @@ long mm_iommu_new(struct mm_struct *mm, unsigned long ua, unsigned long entries,
> >> list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list,
> >> next) {
> >> if ((mem->ua == ua) && (mem->entries == entries)) {
> >> - ++mem->used;
> >> - *pmem = mem;
> >> + ret = -EBUSY;
> >> goto unlock_exit;
> >> }
> >>
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox