* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Benjamin Herrenschmidt @ 2014-11-19 20:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Wei Yang, linux-pci, linuxppc-dev, gwshan, Donald Dutile,
Myron Stowe
In-Reply-To: <20141119172350.GC23467@google.com>
On Wed, 2014-11-19 at 10:23 -0700, Bjorn Helgaas wrote:
>
> Yes, I've read that many times. What's missing is the connection between a
> PE and the things in the PCI specs (buses, devices, functions, MMIO address
> space, DMA, MSI, etc.) Presumably the PE structure imposes constraints on
> how the core uses the standard PCI elements, but we don't really have a
> clear description of those constraints yet.
Right, a "PE" is a HW concept in fact in our bridges, that essentially is
a shared isolation state between DMA, MMIO, MSIs, PCIe error messages,...
for a given "domain" or set of PCI functions.
The techniques of how the HW resources are mapped to PE and associated
constraints are slightly different from one generation of our chips to
the next. In general, P7 follows an architecture known as "IODA" and P8
"IODA2". I'm trying to get that spec made available via OpenPower but
that hasn't happened yet.
In this case we mostly care about IODA2 (P8), so I'll give a quick
description here. Wei, feel free to copy/paste that into a bit of doco
to throw into Documentation/powerpc/ along with your next spin of the patch.
The concept of "PE" is a way to group the various resources associated
with a device or a set of device to provide isolation between partitions
(ie. filtering of DMA, MSIs etc...) and to provide a mechanism to freeze
a device that is causing errors in order to limit the possibility of
propagation of bad data.
There is thus, in HW, a table of "PE" states that contains a pair of
"frozen" state bits (one for MMIO and one for DMA, they get set together
but can be cleared independently) for each PE.
When a PE is frozen, all stores in any direction are dropped and all loads
return all 1's value. MSIs are also blocked. There's a bit more state that
captures things like the details of the error that caused the freeze etc...
but that's not critical.
The interesting part is how the various type of PCIe transactions (MMIO,
DMA,...) are matched to their corresponding PEs.
I will provide a rought description of what we have on P8 (IODA2). Keep
in mind that this is all per PHB (host bridge). Each PHB is a completely
separate HW entity which replicates the entire logic, so has its own set
of PEs etc...
First, P8 has 256 PEs per PHB.
* Inbound
For DMA, MSIs and inbound PCIe error messages, we have a table (in memory but
accessed in HW by the chip) that provides a direct correspondence between
a PCIe RID (bus/dev/fn) with a PE number. We call this the RTT.
- For DMA we then provide an entire address space for each PE that can contains
two "windows", depending on the value of PCI bit 59. Each window can then be
configured to be remapped via a "TCE table" (iommu translation table), which has
various configurable characteristics which we can describe another day.
- For MSIs, we have two windows in the address space (one at the top of the 32-bit
space and one much higher) which, via a combination of the address and MSI value,
will result in one of the 2048 interrupts per bridge being triggered. There's
a PE value in the interrupt controller descriptor table as well which is compared
with the PE obtained from the RTT to "authorize" the device to emit that specific
interrupt.
- Error messages just use the RTT.
* Outbound. That's where the tricky part is.
The PHB basically has a concept of "windows" from the CPU address space to the
PCI address space. There is one M32 window and 16 M64 windows. They have different
characteristics. First what they have in common: they are configured to forward a
configurable portion of the CPU address space to the PCIe bus and must be naturally
aligned power of two in size. The rest is different:
- The M32 window:
* It is limited to 4G in size
* It drops the top bits of the address (above the size) and replaces them with
a configurable value. This is typically used to generate 32-bit PCIe accesses. We
configure that window at boot from FW and don't touch it from Linux, it's usually
set to forward a 2G portion of address space from the CPU to PCIe
0x8000_0000..0xffff_ffff. (Note: The top 64K are actually reserved for MSIs but
this is not a problem at this point, we just need to ensure Linux doesn't assign
anything there, the M32 logic ignores that however and will forward in that space
if we try).
* It is divided into 256 segments of equal size. A table in the chip provides
for each of these 256 segments a PE#. That allows to essentially assign portions
of the MMIO space to PEs on a segment granularity. For a 2G window, this is 8M.
Now, this is the "main" window we use in Linux today (excluding SR-IOV). We
basically use the trick of forcing the bridge MMIO windows onto a segment
alignment/granularity so that the space behind a bridge can be assigned to a PE.
Ideally we would like to be able to have individual functions in PE's but that
would mean using a completely different address allocation scheme where individual
function BARs can be "grouped" to fit in one or more segments....
- The M64 windows.
* Their smallest size is 1M
* They do not translate addresses (the address on PCIe is the same as the
address on the PowerBus. There is a way to also set the top 14 bits which are
not conveyed by PowerBus but we don't use this).
* They can be configured to be segmented or not. When segmented, they have
256 segments, however they are not remapped. The segment number *is* the PE
number. When no segmented, the PE number can be specified for the entire
window.
* They support overlaps in which case there is a well defined ordering of
matching (I don't remember off hand which of the lower or higher numbered
window takes priority but basically it's well defined).
We have code (fairly new compared to the M32 stuff) that exploits that for
large BARs in 64-bit space:
We create a single big M64 that covers the entire region of address space that
has been assigned by FW for the PHB (about 64G, ignore the space for the M32,
it comes out of a different "reserve"). We configure that window as segmented.
Then we do the same thing as with M32, using the bridge aligment trick, to
match to those giant segments.
Since we cannot remap, we have two additional constraints:
- We do the PE# allocation *after* the 64-bit space has been assigned since
the segments used will derive directly the PE#, we then "update" the M32 PE#
for the devices that use both 32-bit and 64-bit spaces or assign the remaining
PE# to 32-bit only devices.
- We cannot "group" segments in HW so if a device ends up using more than
one segment, we end up with more than one PE#. There is a HW mechanism to
make the freeze state cascade to "companion" PEs but that only work for PCIe
error messages (typically used so that if you freeze a switch, it freezes all
its children). So we do it in SW. We lose a bit of effectiveness of EEH in that
case, but that's the best we found. So when any of the PEs freezes, we freeze
the other ones for that "domain". We thus introduce the concept of "master PE"
which is the one used for DMA, MSIs etc... and "secondary PEs" that are used
for the remaining M64 segments.
We would like to investigate using additional M64's in "single PE" mode to
overlay over specific BARs to work around some of that, for example for devices
with very large BARs (some GPUs), it would make sense, but we haven't done it
yet.
Finally, the plan to use M64 for SR-IOV, which we describe a bit already, consists
of using those M64's. So for a given IOV BAR, we need to effectively reserve the
entire 256 segments (256 * IOV BAR size) and then "position" the BAR to start at
the beginning of a free range of segments/PEs inside that M64.
The goal is of course to be able to give a separate PE for each VF...
I hope that helps clarifying things a bit ...
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH V9 08/18] powrepc/pci: Refactor pci_dn
From: Bjorn Helgaas @ 2014-11-19 23:30 UTC (permalink / raw)
To: Wei Yang; +Cc: linux-pci, benh, linuxppc-dev, gwshan
In-Reply-To: <1414942894-17034-9-git-send-email-weiyang@linux.vnet.ibm.com>
On Sun, Nov 02, 2014 at 11:41:24PM +0800, Wei Yang wrote:
> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> pci_dn is the extension of PCI device node and it's created from
> device node. Unfortunately, VFs that are enabled dynamically by
> PF's driver and they don't have corresponding device nodes, and
> pci_dn. The patch refactors pci_dn to support VFs:
>
> * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
> to the child list of pci_dn of PF's bridge. pci_dn of other
> device put to the child list of pci_dn of its upstream bridge.
>
> * VF's pci_dn is expected to be created dynamically when applying
> final fixup to PF. VF's pci_dn will be destroyed when releasing
> PF's pci_dev instance. pci_dn of other device is still created
> from device node as before.
>
> * For one particular PCI device (VF or not), its pci_dn can be
> found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
> or parent's list. The fast path (fetching pci_dn through PCI
> device instance) is populated during early fixup time.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
> ...
> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev)
> +{
> +#ifdef CONFIG_PCI_IOV
> + struct pci_dn *parent, *pdn;
> + int i;
> +
> + /* Only support IOV for now */
> + if (!pdev->is_physfn)
> + return pci_get_pdn(pdev);
> +
> + /* Check if VFs have been populated */
> + pdn = pci_get_pdn(pdev);
> + if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
> + return NULL;
> +
> + pdn->flags |= PCI_DN_FLAG_IOV_VF;
> + parent = pci_bus_to_pdn(pdev->bus);
> + if (!parent)
> + return NULL;
> +
> + for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
> + pdn = add_one_dev_pci_info(parent, NULL,
> + pci_iov_virtfn_bus(pdev, i),
> + pci_iov_virtfn_devfn(pdev, i));
I'm not sure this makes sense, but I certainly don't know this code, so
maybe I'm missing something.
pci_iov_virtfn_bus() and pci_iov_virtfn_devfn() depend on
pdev->sriov->stride and pdev->sriov->offset. These are read from VF Stride
and First VF Offset in the SR-IOV capability by sriov_init(), which is
called before add_dev_pci_info():
pci_scan_child_bus
pci_scan_slot
pci_scan_single_device
pci_device_add
pci_init_capabilities
pci_iov_init(PF)
sriov_init(PF, pos)
pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0)
pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset)
pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride)
iov->offset = offset
iov->stride = stride
pci_bus_add_devices
pci_bus_add_device
pci_fixup_device(pci_fixup_final)
add_dev_pci_info
pci_iov_virtfn_bus
return ... + sriov->offset + (sriov->stride * id) ...
But both First VF Offset and VF Stride change when ARI Capable Hierarchy or
NumVFs changes (SR-IOV spec sec 3.3.9, 3.3.10). We set NumVFs to zero in
sriov_init() above. We will change NumVFs to something different when a
driver calls pci_enable_sriov():
pci_enable_sriov
sriov_enable
pci_write_config_word(dev, iov->pos + PCI_SRIOV_NUM_VF, nr_virtfn)
Now First VF Offset and VF Stride have changed from what they were when we
called pci_iov_virtfn_bus() above.
> + if (!pdn) {
> + pr_warn("%s: Cannot create firmware data "
> + "for VF#%d of %s\n",
> + __func__, i, pci_name(pdev));
> + return NULL;
> + }
> + }
> +#endif
> +
> + return pci_get_pdn(pdev);
> +}
> ...
> +static void pci_dev_pdn_create(struct pci_dev *pdev)
> +{
> + add_dev_pci_info(pdev);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_create);
There are no other callers of add_dev_pci_info(), so it seems pointless to
declare it in arch/powerpc/include/asm/pci-bridge.h and add this wrapper
around it.
> +
> +static void pci_dev_pdn_setup(struct pci_dev *pdev)
> +{
> + struct pci_dn *pdn;
> +
> + if (pdev->dev.archdata.firmware_data)
> + return;
> +
> + /* Setup the fast path */
> + pdn = pci_get_pdn(pdev);
> + pdev->dev.archdata.firmware_data = pdn;
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_setup);
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH V9 01/18] PCI/IOV: Export interface for retrieve VF's BDF
From: Bjorn Helgaas @ 2014-11-19 23:35 UTC (permalink / raw)
To: Wei Yang; +Cc: linux-pci, benh, linuxppc-dev, gwshan
In-Reply-To: <1414942894-17034-2-git-send-email-weiyang@linux.vnet.ibm.com>
On Sun, Nov 02, 2014 at 11:41:17PM +0800, Wei Yang wrote:
> When implementing the SR-IOV on PowerNV platform, some resource reservation is
> needed for VFs which don't exist at the bootup stage. To do the match between
> resources and VFs, the code need to get the VF's BDF in advance.
>
> In this patch, it exports the interface to retrieve VF's BDF:
> * Make the virtfn_bus as an interface
> * Make the virtfn_devfn as an interface
> * Rename them with more specific name
> * Code cleanup in pci_sriov_resource_alignment()
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
> drivers/pci/iov.c | 22 +++++++++++++---------
> include/linux/pci.h | 11 +++++++++++
> 2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 4d109c0..5e8091b 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -19,14 +19,18 @@
>
> #define VIRTFN_ID_LEN 16
>
> -static inline u8 virtfn_bus(struct pci_dev *dev, int id)
> +int pci_iov_virtfn_bus(struct pci_dev *dev, int id)
> {
> + if (!dev->is_physfn)
> + return -EINVAL;
> return dev->bus->number + ((dev->devfn + dev->sriov->offset +
> dev->sriov->stride * id) >> 8);
> }
>
> -static inline u8 virtfn_devfn(struct pci_dev *dev, int id)
> +int pci_iov_virtfn_devfn(struct pci_dev *dev, int id)
> {
> + if (!dev->is_physfn)
> + return -EINVAL;
> return (dev->devfn + dev->sriov->offset +
> dev->sriov->stride * id) & 0xff;
> }
I'm concerned about exporting these because they depend on First VF Offset
and VF Stride from the SR-IOV Capability, and those values change when the
ARI Capability Hierarchy setting or the NumVFs setting change (SR-IOV spec
sec 3.3.9, 3.3.10). The caller doesn't necessarily know about this
connection and may not be able to deal with the change.
I outlined one possible problem with this in patch 08/18.
Bjorn
^ permalink raw reply
* [PATCH v2] powerpc: Remove more traces of bootmem
From: Michael Ellerman @ 2014-11-20 0:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: David.Laight
Although we are now selecting NO_BOOTMEM, we still have some traces of
bootmem lying around. That is because even with NO_BOOTMEM there is
still a shim that converts bootmem calls into memblock calls, but
ultimately we want to remove all traces of bootmem.
Most of the patch is conversions from alloc_bootmem() to
memblock_virt_alloc(). In general a call such as:
p = (struct foo *)alloc_bootmem(x);
Becomes:
p = memblock_virt_alloc(x, 0);
We don't need the cast because memblock_virt_alloc() returns a void *.
The alignment value of zero tells memblock to use the default alignment,
which is SMP_CACHE_BYTES, the same value alloc_bootmem() uses.
We remove a number of NULL checks on the result of
memblock_virt_alloc(). That is because memblock_virt_alloc() will panic
if it can't allocate, in exactly the same way as alloc_bootmem(), so the
NULL checks are and always have been redundant.
The memory returned by memblock_virt_alloc() is already zeroed, so we
remove several memsets of the result of memblock_virt_alloc().
Finally we convert a few uses of __alloc_bootmem(x, y, MAX_DMA_ADDRESS)
to just plain memblock_virt_alloc(). We don't use memblock_alloc_base()
because MAX_DMA_ADDRESS is ~0ul on powerpc, so limiting the allocation
to that is pointless, 16XB ought to be enough for anyone.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/pci-common.c | 1 -
arch/powerpc/kernel/pci_32.c | 4 +---
arch/powerpc/kernel/setup_64.c | 2 +-
arch/powerpc/lib/alloc.c | 4 +---
arch/powerpc/mm/hugetlbpage.c | 2 +-
arch/powerpc/mm/mmu_context_nohash.c | 8 ++++----
arch/powerpc/platforms/cell/celleb_pci.c | 6 +++---
arch/powerpc/platforms/powermac/nvram.c | 6 +-----
arch/powerpc/platforms/powernv/pci-ioda.c | 12 +++---------
arch/powerpc/platforms/powernv/pci-p5ioc2.c | 16 ++++------------
arch/powerpc/platforms/ps3/setup.c | 7 +------
arch/powerpc/sysdev/fsl_pci.c | 1 -
12 files changed, 20 insertions(+), 49 deletions(-)
v2: Use memblock_virt_alloc() instead of open-coding __va(memblock_alloc(..)).
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index bc2dab52a991..37d512d35943 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -20,7 +20,6 @@
#include <linux/pci.h>
#include <linux/string.h>
#include <linux/init.h>
-#include <linux/bootmem.h>
#include <linux/delay.h>
#include <linux/export.h>
#include <linux/of_address.h>
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 432459c817fa..1f7930037cb7 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -199,9 +199,7 @@ pci_create_OF_bus_map(void)
struct property* of_prop;
struct device_node *dn;
- of_prop = (struct property*) alloc_bootmem(sizeof(struct property) + 256);
- if (!of_prop)
- return;
+ of_prop = memblock_virt_alloc(sizeof(struct property) + 256, 0);
dn = of_find_node_by_path("/");
if (dn) {
memset(of_prop, -1, sizeof(struct property) + 256);
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 6e5310ddf8c7..49f553bbb360 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -660,7 +660,7 @@ static void __init emergency_stack_init(void)
}
/*
- * Called into from start_kernel this initializes bootmem, which is used
+ * Called into from start_kernel this initializes memblock, which is used
* to manage page allocation until mem_init is called.
*/
void __init setup_arch(char **cmdline_p)
diff --git a/arch/powerpc/lib/alloc.c b/arch/powerpc/lib/alloc.c
index da22c84a8fed..4a6c2cf890d9 100644
--- a/arch/powerpc/lib/alloc.c
+++ b/arch/powerpc/lib/alloc.c
@@ -13,9 +13,7 @@ void * __init_refok zalloc_maybe_bootmem(size_t size, gfp_t mask)
if (mem_init_done)
p = kzalloc(size, mask);
else {
- p = alloc_bootmem(size);
- if (p)
- memset(p, 0, size);
+ p = memblock_virt_alloc(size, 0);
}
return p;
}
diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index af56de82375d..8c9b8115867c 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c
@@ -315,7 +315,7 @@ int alloc_bootmem_huge_page(struct hstate *hstate)
* If gpages can be in highmem we can't use the trick of storing the
* data structure in the page; allocate space for this
*/
- m = alloc_bootmem(sizeof(struct huge_bootmem_page));
+ m = memblock_virt_alloc(sizeof(struct huge_bootmem_page), 0);
m->phys = gpage_freearray[idx].gpage_list[--nr_gpages];
#else
m = phys_to_virt(gpage_freearray[idx].gpage_list[--nr_gpages]);
diff --git a/arch/powerpc/mm/mmu_context_nohash.c b/arch/powerpc/mm/mmu_context_nohash.c
index 928ebe79668b..9cba6cba2e50 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -421,12 +421,12 @@ void __init mmu_context_init(void)
/*
* Allocate the maps used by context management
*/
- context_map = alloc_bootmem(CTX_MAP_SIZE);
- context_mm = alloc_bootmem(sizeof(void *) * (last_context + 1));
+ context_map = memblock_virt_alloc(CTX_MAP_SIZE, 0);
+ context_mm = memblock_virt_alloc(sizeof(void *) * (last_context + 1), 0);
#ifndef CONFIG_SMP
- stale_map[0] = alloc_bootmem(CTX_MAP_SIZE);
+ stale_map[0] = memblock_virt_alloc(CTX_MAP_SIZE, 0);
#else
- stale_map[boot_cpuid] = alloc_bootmem(CTX_MAP_SIZE);
+ stale_map[boot_cpuid] = memblock_virt_alloc(CTX_MAP_SIZE, 0);
register_cpu_notifier(&mmu_context_cpu_nb);
#endif
diff --git a/arch/powerpc/platforms/cell/celleb_pci.c b/arch/powerpc/platforms/cell/celleb_pci.c
index 2b98a36ef8fb..3ce70ded2d6a 100644
--- a/arch/powerpc/platforms/cell/celleb_pci.c
+++ b/arch/powerpc/platforms/cell/celleb_pci.c
@@ -29,7 +29,7 @@
#include <linux/pci.h>
#include <linux/string.h>
#include <linux/init.h>
-#include <linux/bootmem.h>
+#include <linux/memblock.h>
#include <linux/pci_regs.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -401,11 +401,11 @@ error:
} else {
if (config && *config) {
size = 256;
- free_bootmem(__pa(*config), size);
+ memblock_free(__pa(*config), size);
}
if (res && *res) {
size = sizeof(struct celleb_pci_resource);
- free_bootmem(__pa(*res), size);
+ memblock_free(__pa(*res), size);
}
}
diff --git a/arch/powerpc/platforms/powermac/nvram.c b/arch/powerpc/platforms/powermac/nvram.c
index 014d06e6d46b..60b03a1703d1 100644
--- a/arch/powerpc/platforms/powermac/nvram.c
+++ b/arch/powerpc/platforms/powermac/nvram.c
@@ -513,11 +513,7 @@ static int __init core99_nvram_setup(struct device_node *dp, unsigned long addr)
printk(KERN_ERR "nvram: no address\n");
return -EINVAL;
}
- nvram_image = alloc_bootmem(NVRAM_SIZE);
- if (nvram_image == NULL) {
- printk(KERN_ERR "nvram: can't allocate ram image\n");
- return -ENOMEM;
- }
+ nvram_image = memblock_virt_alloc(NVRAM_SIZE, 0);
nvram_data = ioremap(addr, NVRAM_SIZE*2);
nvram_naddrs = 1; /* Make sure we get the correct case */
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index d03503515692..cc861c14840d 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1940,19 +1940,14 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
phb_id = be64_to_cpup(prop64);
pr_debug(" PHB-ID : 0x%016llx\n", phb_id);
- phb = alloc_bootmem(sizeof(struct pnv_phb));
- if (!phb) {
- pr_err(" Out of memory !\n");
- return;
- }
+ phb = memblock_virt_alloc(sizeof(struct pnv_phb), 0);
/* Allocate PCI controller */
- memset(phb, 0, sizeof(struct pnv_phb));
phb->hose = hose = pcibios_alloc_controller(np);
if (!phb->hose) {
pr_err(" Can't allocate PCI controller for %s\n",
np->full_name);
- free_bootmem((unsigned long)phb, sizeof(struct pnv_phb));
+ memblock_free(__pa(phb), sizeof(struct pnv_phb));
return;
}
@@ -2019,8 +2014,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
}
pemap_off = size;
size += phb->ioda.total_pe * sizeof(struct pnv_ioda_pe);
- aux = alloc_bootmem(size);
- memset(aux, 0, size);
+ aux = memblock_virt_alloc(size, 0);
phb->ioda.pe_alloc = aux;
phb->ioda.m32_segmap = aux + m32map_off;
if (phb->type == PNV_PHB_IODA1)
diff --git a/arch/powerpc/platforms/powernv/pci-p5ioc2.c b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
index 3336fcbdd08a..6ef6d4d8e7e2 100644
--- a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
+++ b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
@@ -122,12 +122,9 @@ static void __init pnv_pci_init_p5ioc2_phb(struct device_node *np, u64 hub_id,
return;
}
- phb = alloc_bootmem(sizeof(struct pnv_phb));
- if (phb) {
- memset(phb, 0, sizeof(struct pnv_phb));
- phb->hose = pcibios_alloc_controller(np);
- }
- if (!phb || !phb->hose) {
+ phb = memblock_virt_alloc(sizeof(struct pnv_phb), 0);
+ phb->hose = pcibios_alloc_controller(np);
+ if (!phb->hose) {
pr_err(" Failed to allocate PCI controller\n");
return;
}
@@ -216,12 +213,7 @@ void __init pnv_pci_init_p5ioc2_hub(struct device_node *np)
*
* XXX TODO: Make it chip local if possible
*/
- tce_mem = __alloc_bootmem(P5IOC2_TCE_MEMORY, P5IOC2_TCE_MEMORY,
- __pa(MAX_DMA_ADDRESS));
- if (!tce_mem) {
- pr_err(" Failed to allocate TCE Memory !\n");
- return;
- }
+ tce_mem = memblock_virt_alloc(P5IOC2_TCE_MEMORY, P5IOC2_TCE_MEMORY);
pr_debug(" TCE : 0x%016lx..0x%016lx\n",
__pa(tce_mem), __pa(tce_mem) + P5IOC2_TCE_MEMORY - 1);
rc = opal_pci_set_hub_tce_memory(hub_id, __pa(tce_mem),
diff --git a/arch/powerpc/platforms/ps3/setup.c b/arch/powerpc/platforms/ps3/setup.c
index 009a2004b876..799c8580ab09 100644
--- a/arch/powerpc/platforms/ps3/setup.c
+++ b/arch/powerpc/platforms/ps3/setup.c
@@ -125,12 +125,7 @@ static void __init prealloc(struct ps3_prealloc *p)
if (!p->size)
return;
- p->address = __alloc_bootmem(p->size, p->align, __pa(MAX_DMA_ADDRESS));
- if (!p->address) {
- printk(KERN_ERR "%s: Cannot allocate %s\n", __func__,
- p->name);
- return;
- }
+ p->address = memblock_virt_alloc(p->size, p->align);
printk(KERN_INFO "%s: %lu bytes at %p\n", p->name, p->size,
p->address);
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index d8484d7cffaa..6455c1eada1a 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -23,7 +23,6 @@
#include <linux/string.h>
#include <linux/init.h>
#include <linux/interrupt.h>
-#include <linux/bootmem.h>
#include <linux/memblock.h>
#include <linux/log2.h>
#include <linux/slab.h>
--
1.9.1
^ permalink raw reply related
* Re: [PATCH V9 08/18] powrepc/pci: Refactor pci_dn
From: Gavin Shan @ 2014-11-20 1:02 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Wei Yang, benh, linuxppc-dev, gwshan
In-Reply-To: <20141119233024.GF23467@google.com>
On Wed, Nov 19, 2014 at 04:30:24PM -0700, Bjorn Helgaas wrote:
>On Sun, Nov 02, 2014 at 11:41:24PM +0800, Wei Yang wrote:
>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>
>> pci_dn is the extension of PCI device node and it's created from
>> device node. Unfortunately, VFs that are enabled dynamically by
>> PF's driver and they don't have corresponding device nodes, and
>> pci_dn. The patch refactors pci_dn to support VFs:
>>
>> * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
>> to the child list of pci_dn of PF's bridge. pci_dn of other
>> device put to the child list of pci_dn of its upstream bridge.
>>
>> * VF's pci_dn is expected to be created dynamically when applying
>> final fixup to PF. VF's pci_dn will be destroyed when releasing
>> PF's pci_dev instance. pci_dn of other device is still created
>> from device node as before.
>>
>> * For one particular PCI device (VF or not), its pci_dn can be
>> found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
>> or parent's list. The fast path (fetching pci_dn through PCI
>> device instance) is populated during early fixup time.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> ...
>
>> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> + struct pci_dn *parent, *pdn;
>> + int i;
>> +
>> + /* Only support IOV for now */
>> + if (!pdev->is_physfn)
>> + return pci_get_pdn(pdev);
>> +
>> + /* Check if VFs have been populated */
>> + pdn = pci_get_pdn(pdev);
>> + if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
>> + return NULL;
>> +
>> + pdn->flags |= PCI_DN_FLAG_IOV_VF;
>> + parent = pci_bus_to_pdn(pdev->bus);
>> + if (!parent)
>> + return NULL;
>> +
>> + for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
>> + pdn = add_one_dev_pci_info(parent, NULL,
>> + pci_iov_virtfn_bus(pdev, i),
>> + pci_iov_virtfn_devfn(pdev, i));
>
>I'm not sure this makes sense, but I certainly don't know this code, so
>maybe I'm missing something.
>
For ARI, Richard had some patches to fix the issue from firmware side.
>pci_iov_virtfn_bus() and pci_iov_virtfn_devfn() depend on
>pdev->sriov->stride and pdev->sriov->offset. These are read from VF Stride
>and First VF Offset in the SR-IOV capability by sriov_init(), which is
>called before add_dev_pci_info():
>
> pci_scan_child_bus
> pci_scan_slot
> pci_scan_single_device
> pci_device_add
> pci_init_capabilities
> pci_iov_init(PF)
> sriov_init(PF, pos)
> pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0)
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset)
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride)
> iov->offset = offset
> iov->stride = stride
>
> pci_bus_add_devices
> pci_bus_add_device
> pci_fixup_device(pci_fixup_final)
> add_dev_pci_info
> pci_iov_virtfn_bus
> return ... + sriov->offset + (sriov->stride * id) ...
>
>But both First VF Offset and VF Stride change when ARI Capable Hierarchy or
>NumVFs changes (SR-IOV spec sec 3.3.9, 3.3.10). We set NumVFs to zero in
>sriov_init() above. We will change NumVFs to something different when a
>driver calls pci_enable_sriov():
>
> pci_enable_sriov
> sriov_enable
> pci_write_config_word(dev, iov->pos + PCI_SRIOV_NUM_VF, nr_virtfn)
>
>Now First VF Offset and VF Stride have changed from what they were when we
>called pci_iov_virtfn_bus() above.
>
It's the case we missed: First VF Offset and VF Stride can change when
PF's number of VFs is changed. It means the BDFN (Bus/Device/Function
number) for one VF can't be determined until PF's number of VFs is
populated and updated to HW (before calling to virtfn_add()).
The dynamically created pci_dn is used in PCI config accessors currently.
That means we have to get it ready before first PCI config request to the
VF in pci_setup_device(). In the code of old revision, we had some weak
function called in pci_alloc_dev(), which gave platform chance to create
pci_dn. I think we have to switch back to the old way in order to fix
the problem you catched. However, the old way is implemented with cost
of more weak function, which you're probably unhappy to see.
sriov_enable()
virtfn_add()
virtfn_add_bus()
pci_alloc_dev()
pci_setup_device()
>> + if (!pdn) {
>> + pr_warn("%s: Cannot create firmware data "
>> + "for VF#%d of %s\n",
>> + __func__, i, pci_name(pdev));
>> + return NULL;
>> + }
>> + }
>> +#endif
>> +
>> + return pci_get_pdn(pdev);
>> +}
>> ...
>
>> +static void pci_dev_pdn_create(struct pci_dev *pdev)
>> +{
>> + add_dev_pci_info(pdev);
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_create);
>
>There are no other callers of add_dev_pci_info(), so it seems pointless to
>declare it in arch/powerpc/include/asm/pci-bridge.h and add this wrapper
>around it.
>
Yep and will fix.
Thanks,
Gavin
>> +
>> +static void pci_dev_pdn_setup(struct pci_dev *pdev)
>> +{
>> + struct pci_dn *pdn;
>> +
>> + if (pdev->dev.archdata.firmware_data)
>> + return;
>> +
>> + /* Setup the fast path */
>> + pdn = pci_get_pdn(pdev);
>> + pdev->dev.archdata.firmware_data = pdn;
>> +}
>> +DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, pci_dev_pdn_setup);
>> --
>> 1.7.9.5
>>
>
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Yijing Wang @ 2014-11-20 2:00 UTC (permalink / raw)
To: Liviu Dudau
Cc: Tony Luck, Russell King, Arnd Bergmann, linux-pci@vger.kernel.org,
x86@kernel.org, Liviu Dudau, linux-kernel@vger.kernel.org,
huxinwei@huawei.com, Thierry Reding,
suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141119162914.GB9162@bart.dudau.co.uk>
>>> Something like this:
>>>
>>> struct pci_controller {
>>> struct pci_host_bridge bridge;
>>> /* private host bridge data here */
>>> .....
>>> };
>>>
>>> #define PCI_CONTROLLER(bus) ({
>>> struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \
>>> container_of(hb, struct pci_controller, bridge); })
>>>
>>>
>>> Then we can retrieve the host bridge structure from everywhere we have a device.
>>
>> Hi Liviu, it looks good to me, because this change will involve lots platforms,
>> I would think more about it. Thanks for your suggestion very much! :)
>
> Given that I also look at this area maybe we should join forces and divide the problem?
That's better, we could cooperate to do this. :)
>
> Best regards,
> Liviu
>
>>
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>>>
>>>>>> + host = kzalloc(sizeof(*host), GFP_KERNEL);
>>>>>> + if (!host)
>>>>>> + return NULL;
>>>>>
>>>>> devm_kzalloc maybe?
>>>>
>>>> I don't know much detail about devm_kzalloc(), but we have no pci host driver
>>>> here, and I found no devm_kzalloc() uses in core PCI code before.
>>>>
>>>>>
>>>>>> + if (!resources) {
>>>>>> + /* Use default IO/MEM/BUS resources*/
>>>>>> + pci_add_resource(&host->windows, &ioport_resource);
>>>>>> + pci_add_resource(&host->windows, &iomem_resource);
>>>>>> + pci_add_resource(&host->windows, &busn_resource);
>>>>>> + } else {
>>>>>> + list_for_each_entry_safe(window, n, resources, list)
>>>>>> + list_move_tail(&window->list, &host->windows);
>>>>>> + }
>>>>>
>>>>> I think we should assume that the correct resources are passed. You
>>>>> could add a wrapper around this function to convert old platforms
>>>>> though.
>>>>
>>>> OK, I will move these code out of pci_create_host_bridge, and add a wrapper
>>>> to setup the default resources.
>>>>
>>>>>
>>>>>> +EXPORT_SYMBOL(pci_create_host_bridge);
>>>>>
>>>>> EXPORT_SYMBOL_GPL() maybe?
>>>>
>>>> OK, will update it.
>>>>
>>>>>
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index 8b11b38..daa7f40 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
>>>>>> struct pci_host_bridge {
>>>>>> struct device dev;
>>>>>> struct pci_bus *bus; /* root bus */
>>>>>> + struct list_head list;
>>>>>> struct list_head windows; /* pci_host_bridge_windows */
>>>>>> + int busnum;
>>>>>
>>>>> The busnum should already be implied through the bus resource.
>>>>
>>>> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks!
>>>>
>>>> Thanks!
>>>> Yijing.
>>>>
>>>>>
>>>>> Arnd
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks!
>>>> Yijing
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2] powerpc: Remove more traces of bootmem
From: Tony Breeds @ 2014-11-20 2:33 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, David.Laight
In-Reply-To: <1416442058-19544-1-git-send-email-mpe@ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]
On Thu, Nov 20, 2014 at 11:07:38AM +1100, Michael Ellerman wrote:
> Although we are now selecting NO_BOOTMEM, we still have some traces of
> bootmem lying around. That is because even with NO_BOOTMEM there is
> still a shim that converts bootmem calls into memblock calls, but
> ultimately we want to remove all traces of bootmem.
>
> Most of the patch is conversions from alloc_bootmem() to
> memblock_virt_alloc(). In general a call such as:
>
> p = (struct foo *)alloc_bootmem(x);
>
> Becomes:
>
> p = memblock_virt_alloc(x, 0);
>
> We don't need the cast because memblock_virt_alloc() returns a void *.
> The alignment value of zero tells memblock to use the default alignment,
> which is SMP_CACHE_BYTES, the same value alloc_bootmem() uses.
>
> We remove a number of NULL checks on the result of
> memblock_virt_alloc(). That is because memblock_virt_alloc() will panic
> if it can't allocate, in exactly the same way as alloc_bootmem(), so the
> NULL checks are and always have been redundant.
>
> The memory returned by memblock_virt_alloc() is already zeroed, so we
> remove several memsets of the result of memblock_virt_alloc().
>
> Finally we convert a few uses of __alloc_bootmem(x, y, MAX_DMA_ADDRESS)
> to just plain memblock_virt_alloc(). We don't use memblock_alloc_base()
> because MAX_DMA_ADDRESS is ~0ul on powerpc, so limiting the allocation
> to that is pointless, 16XB ought to be enough for anyone.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
<snip>
> --- a/arch/powerpc/lib/alloc.c
> +++ b/arch/powerpc/lib/alloc.c
> @@ -13,9 +13,7 @@ void * __init_refok zalloc_maybe_bootmem(size_t size, gfp_t mask)
> if (mem_init_done)
> p = kzalloc(size, mask);
> else {
> - p = alloc_bootmem(size);
> - if (p)
> - memset(p, 0, size);
> + p = memblock_virt_alloc(size, 0);
> }
You knew someone would ask but ...
Do you want to remove the {} form the else clause so that the style matches the if()
Yours Tony.
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/3] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled
From: Mike Turquette @ 2014-11-20 2:38 UTC (permalink / raw)
To: Scott Wood, Kevin Hao; +Cc: Gerhard Sittig, linuxppc-dev, Jingchang Lu
In-Reply-To: <1415333263.23458.417.camel@snotra.buserror.net>
Quoting Scott Wood (2014-11-06 20:07:43)
> On Sun, 2014-10-19 at 14:11 +0800, Kevin Hao wrote:
> > Hi,
> > =
> > I have done a boot test on p2014rdb and t4240qds boards. I don't have a=
n access
> > to mpc512x board, so only build test for that.
> > =
> > v2:
> > - Revert the commit da788acb2838 first.
> > - Invoke of_clk_init() from a common place.
> > =
> > v1
> > This tries to fix a kernel panic introduced by commit da788acb2838
> > ("clk: ppc-corenet: Fix Section mismatch warning").
> > =
> > Kevin Hao (3):
> > Revert "clk: ppc-corenet: Fix Section mismatch warning"
> > powerpc: call of_clk_init() from time_init()
> > clk: ppc-corenet: fix section mismatch warning
> > =
> > arch/powerpc/kernel/time.c | 5 ++++
> > arch/powerpc/platforms/512x/clock-commonclk.c | 11 ++++---
> > drivers/clk/clk-ppc-corenet.c | 43 ++++---------------=
--------
> > 3 files changed, 16 insertions(+), 43 deletions(-)
> > =
> =
> Acked-by: Scott Wood <scottwood@freescale.com>
I'm fine with it going through the ppc tree.
Acked-by: Michael Turquette <mturquette@linaro.org>
> =
> Whose tree should this go through?
> =
> -Scott
> =
>=20
^ permalink raw reply
* Re: Fan management PowerMac7,2
From: Michel Dänzer @ 2014-11-20 2:37 UTC (permalink / raw)
To: Martin Kukač; +Cc: linuxppc-dev
In-Reply-To: <546C8763.6020209@yahoo.com>
On 19.11.2014 21:04, Martin Kukač wrote:
> Hello,
>
> I was directed to this list on #ppc64/freenode IRC channel. I have
> PowerMac7,2 (AGP, 2xPowerPC 970(2.2) 2GHz) and I wanted to try kernel
> supporting kvm-pr module, which is not in 3.2.63 kernel provided with
> Debian Wheezy. I downloaded 3.14.24, compiled it using original config
> from 3.2.63 and after booting fan management does not work. I tried
> therm_pm72 (which is deprecated, but works well with Debian kernel) and
> windfarm_pm72 modules, but neither worked - after about five minutes
> from boot all fans go to maximum and stay this way no matter the CPU
> load or frequency.
>
> I downloaded vanilla 3.2.64, compiled it the same way and there fans
> work with therm_p72 well. This kernel does not have kvm-pr, so I
> reverted to original kernel provided with Wheezy.
>
> I looked to logs, i tried to search the web whether anyone had already
> the same problem, but it seems that nobody else described not to say
> solved this. Can anyone please point me to anything that would help? I
> can provide whatever log files required.
Might be the same problem as discussed in
https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-August/109700.html
and the following posts: The i2c_powermac module doesn't get loaded
automatically with recent kernels.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Yijing Wang @ 2014-11-20 2:47 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141119163726.GW12037@e106497-lin.cambridge.arm.com>
>>>> +static void pci_release_host_bridge_dev(struct device *dev)
>>>> +{
>>>> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev);
>>>> +
>>>> + if (bridge->release_fn)
>>>> + bridge->release_fn(bridge);
>>>> + pci_free_resource_list(&bridge->windows);
>>>> + kfree(bridge);
>>>> +}
>>>> +
>>>> +struct pci_host_bridge *pci_create_host_bridge(
>>>> + struct device *parent, u32 db,
>>>> + struct pci_ops *ops, void *sysdata,
>>>
>>> I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It
>>> might be more useful to have pci_host_bridge specific ops here.
>>
>> Because we want to create pci_host_bridge before pci root bus creation,
>> so when we scan the root bus and child buses, we use pci_host_bridge as
>> the only argument, and another pci_host_info will be addes in later patch,
>> which one support carry the pci_host_bridge ops.
>
> But pci_create_root_bus() already has a pci_ops argument, I don't see the reason
> to drop that.
>
> pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets
> the bus ops. For find out the MSI controller, the domain number and any other HB
> specific stuff, you use the HB ops. For config R/W acceses you use bus ops.
>
I want to unexport pci_create_root_bus() if we have pci_create_host_bridge().
>>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>>>> - struct pci_ops *ops, void *sysdata, struct list_head *resources)
>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge)
>>>> {
>>>> int error;
>>>> - struct pci_host_bridge *bridge;
>>>> struct pci_bus *b, *b2;
>>>> - struct pci_host_bridge_window *window, *n;
>>>> + struct pci_host_bridge_window *window;
>>>> struct resource *res;
>>>> resource_size_t offset;
>>>> char bus_addr[64];
>>>> char *fmt;
>>>> - u8 bus = PCI_BUSNUM(db);
>>>> + struct device *parent = bridge->dev.parent;
>>>>
>>>> b = pci_alloc_bus(NULL);
>>>> if (!b)
>>>> return NULL;
>>>>
>>>> - b->sysdata = sysdata;
>>>> - b->ops = ops;
>>>> - b->number = b->busn_res.start = bus;
>>>> + b->sysdata = bridge->sysdata;
>>>
>>> I think bridge should be the b->sysdata here.
>>
>> ? what's the meaning?
>
> Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed
> in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver
> structure around the pci_host_bridge, then we will always have a way of retrieving that
> information; but for backwards compatibility we could set bus->sysdata to be the bridge.
> Then existing macros that convert sysdata to pci_controller can be reused after being
> updated.
I think wrapping the host bridge driver structure around the pci_host_bridge could help
us to find the private pci_controller, but in this case, pci_host_bridge is not a pointer
so we put all pci_host_bridge init stuff in host drivers ? And if we still use arch spec
macros convert ssydata to pci_controller, so what's the greatest advantage ?
If we move domain and msi_chip out of sysdata, what's other things in sysdata still need us
to convert in PCI core ?
>
> Best regards,
> Liviu
>
>>
>>>
>>>> + b->ops = bridge->ops;
>>>
>>> See comment above why I don't think this is necessary.
>>>
>>>> + b->number = b->busn_res.start = bridge->busnum;
>>>> pci_bus_assign_domain_nr(b, parent);
>>>> - b2 = pci_find_bus(pci_domain_nr(b), bus);
>>>> + bridge->domain = pci_domain_nr(b);
>>>
>>> Do you really want to overwrite the bridge's domain with the one from a bus that
>>> could possibly be rejected a couple of lines further down?
>>>
>>> As an asside: if we are doing the split of pci_host_bridge from root bus creation
>>> it is worth in my opinion to move the domain setup in pci_create_host_bridge()
>>> and stop fiddling with it here.
>>
>>
>> Hi Liviu, these lines just temporary, I will remove it after all host drivers
>> save its domain in pci_host_bridge.
>>
>>
>>>
>>> Otherwise it looks to me like you are heading in the right direction.
>>
>> Thanks!
>> Yijing.
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum);
>>>> if (b2) {
>>>> /* If we already got to this bus through a different bridge, ignore it */
>>>> dev_dbg(&b2->dev, "bus already known\n");
>>>> goto err_out;
>>>> }
>>>>
>>>> - bridge = pci_alloc_host_bridge(b);
>>>> - if (!bridge)
>>>> - goto err_out;
>>>> -
>>>> - bridge->dev.parent = parent;
>>>> - bridge->dev.release = pci_release_host_bridge_dev;
>>>> - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>>>> - error = pcibios_root_bridge_prepare(bridge);
>>>> - if (error) {
>>>> - kfree(bridge);
>>>> - goto err_out;
>>>> - }
>>>> -
>>>> - error = device_register(&bridge->dev);
>>>> - if (error) {
>>>> - put_device(&bridge->dev);
>>>> - goto err_out;
>>>> - }
>>>> b->bridge = get_device(&bridge->dev);
>>>> device_enable_async_suspend(b->bridge);
>>>> pci_set_bus_of_node(b);
>>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>>>>
>>>> b->dev.class = &pcibus_class;
>>>> b->dev.parent = b->bridge;
>>>> - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
>>>> + dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), b->number);
>>>> error = device_register(&b->dev);
>>>> if (error)
>>>> goto class_dev_reg_err;
>>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
>>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
>>>>
>>>> /* Add initial resources to the bus */
>>>> - list_for_each_entry_safe(window, n, resources, list) {
>>>> - list_move_tail(&window->list, &bridge->windows);
>>>> + list_for_each_entry(window, &bridge->windows, list) {
>>>> res = window->res;
>>>> offset = window->offset;
>>>> if (res->flags & IORESOURCE_BUS)
>>>> - pci_bus_insert_busn_res(b, bus, res->end);
>>>> + pci_bus_insert_busn_res(b, b->number, res->end);
>>>> else
>>>> pci_bus_add_resource(b, res, 0);
>>>> if (offset) {
>>>> @@ -2001,6 +1949,25 @@ err_out:
>>>> return NULL;
>>>> }
>>>>
>>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
>>>> + struct pci_ops *ops, void *sysdata, struct list_head *resources)
>>>> +{
>>>> + struct pci_host_bridge *host;
>>>> +
>>>> + host = pci_create_host_bridge(parent, bus, ops,
>>>> + sysdata ,resources);
>>>> + if (!host)
>>>> + return NULL;
>>>> +
>>>> + host->bus = __pci_create_root_bus(host);
>>>> + if (!host->bus) {
>>>> + pci_free_host_bridge(host);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + return host->bus;
>>>> +}
>>>> +
>>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
>>>> {
>>>> struct resource *res = &b->busn_res;
>>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>>>> {
>>>> struct pci_host_bridge_window *window;
>>>> bool found = false;
>>>> - struct pci_bus *b;
>>>> - LIST_HEAD(default_res);
>>>> + struct pci_host_bridge *host;
>>>> int max;
>>>>
>>>> - if (!resources) {
>>>> - pci_add_resource(&default_res, &ioport_resource);
>>>> - pci_add_resource(&default_res, &iomem_resource);
>>>> - pci_add_resource(&default_res, &busn_resource);
>>>> - } else {
>>>> - list_for_each_entry(window, resources, list)
>>>> - if (window->res->flags & IORESOURCE_BUS) {
>>>> - found = true;
>>>> - break;
>>>> - }
>>>> - }
>>>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources);
>>>> + if (!host)
>>>> + return NULL;
>>>>
>>>> - b = pci_create_root_bus(parent, db, ops, sysdata,
>>>> - resources ? resources : &default_res);
>>>> - if (!b)
>>>> + list_for_each_entry(window, &host->windows, list)
>>>> + if (window->res->flags & IORESOURCE_BUS) {
>>>> + found = true;
>>>> + break;
>>>> + }
>>>> +
>>>> + host->bus = __pci_create_root_bus(host);
>>>> + if (!host->bus) {
>>>> + pci_free_host_bridge(host);
>>>> return NULL;
>>>> + }
>>>>
>>>> if (!found) {
>>>> - dev_info(&b->dev,
>>>> + dev_info(&host->bus->dev,
>>>> "No busn resource found for root bus, will use [bus %02x-ff]\n",
>>>> PCI_BUSNUM(db));
>>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255);
>>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255);
>>>> }
>>>>
>>>> - max = pci_scan_child_bus(b);
>>>> -
>>>> + max = pci_scan_child_bus(host->bus);
>>>> if (!found)
>>>> - pci_bus_update_busn_res_end(b, max);
>>>> + pci_bus_update_busn_res_end(host->bus, max);
>>>>
>>>> - return b;
>>>> + return host->bus;
>>>> }
>>>> EXPORT_SYMBOL(pci_scan_root_bus);
>>>>
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 8b11b38..daa7f40 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
>>>> struct pci_host_bridge {
>>>> struct device dev;
>>>> struct pci_bus *bus; /* root bus */
>>>> + struct list_head list;
>>>> struct list_head windows; /* pci_host_bridge_windows */
>>>> + int busnum;
>>>> + int domain;
>>>> + void *sysdata;
>>>> + struct pci_ops *ops;
>>>> void (*release_fn)(struct pci_host_bridge *);
>>>> void *release_data;
>>>> };
>>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>>> void *release_data);
>>>>
>>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
>>>> -
>>>> +struct pci_host_bridge *pci_create_host_bridge(
>>>> + struct device *parent, u32 db, struct pci_ops *ops,
>>>> + void *sys, struct list_head *resources);
>>>> /*
>>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for
>>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus);
>>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
>>>> struct pci_ops *ops, void *sysdata,
>>>> struct list_head *resources);
>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host);
>>>> +void pci_free_host_bridge(struct pci_host_bridge *host);
>>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>>>> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>>>> void pci_bus_release_busn_res(struct pci_bus *b);
>>>> --
>>>> 1.7.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [RFC PATCH 08/16] PCI: Introduce pci_scan_host_bridge() and pci_host_info
From: Yijing Wang @ 2014-11-20 2:54 UTC (permalink / raw)
To: Liviu Dudau
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, Yijing Wang, suravee.suthikulpanit@amd.com,
Bjorn Helgaas, linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141119164128.GX12037@e106497-lin.cambridge.arm.com>
>> No, in this patch, host drivers pass a pci host bridge resources init hook
>> in pci_host_info *info, and we call this info->init_res() in pci_create_host_bridge().
>>
>> +struct pci_host_info {
>> + u8 res_type;
>> + void *arg;
>> + struct list_head *resources; /*just for build, will clean up later */
>> + int (*init_res)(struct pci_host_bridge *host,
>> + struct pci_host_info *info);
>> +};
>> +
>
> That's not what I've asked! Your code does:
>
> if (info->res_type != PCI_HOST_RES_DEFAULT)
> ....
> else /* info->res_type == PCI_HOST_RES_DEFAULT)
> info->res_type = PCI_HOST_RES_DEFAULT;
>
> info->res_type is already == PCI_HOST_RES_DEFAULT in the else side, assignment is a NOP?
Hmmm, I wanted pci_create_host_bridge() to process the default res later(add default res),
It's ugly code, I will rework it.
>
>
>>
>>>
>>>> +
>>>> + return 0;
>>>> +}
>> ...
>>>> @@ -2038,8 +2057,13 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>>>> bool found = false;
>>>> struct pci_host_bridge *host;
>>>> int max;
>>>> + struct pci_host_info info;
>>>> +
>>>> + info.arg = sysdata;
>>>> + info.resources = resources;
>>>> + info.init_res = pci_default_init_res;
>>>
>>> I have mixed feelings about this patch. While it is heading in the right direction
>>> of moving pci_host_bridge relevant information towards the right user, I don't think
>>> you picked up the right set to move. The resource list is going to be copied into
>>> internal pci_host_bridge list anyway, keeping another copy is not helpful *and*
>>> you have increased the code size.
>>>
>>> I think for now we should aim to get the *missing* data into pci_host_bridge: MSI
>>> controllers and PCI domain/segment. Then we can do more cleanup.
>>
>> Hi Liviu, I agree with you here, the changes to resources stuff seems not a perfect
>> solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(domain, bus) argument,
>> and store it in pci_host_bridge. For msi controller, we couldn't save the msi_controller
>> in pci_host_bridge. Before we assume one pci host bridge only had one msi_controller,
>> but now something changes, Jiang introduce hierarchy irq domain in x86, and now
>> one pci host bridge may has more than one msi_controller. So I prefer to add a
>> function to pci_host_bridge something like
>>
>> struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
>
> Yes, good idea.
>
>>
>>>
>>>>
>>>> - host = pci_create_host_bridge(parent, db, ops, sysdata, resources);
>>>> + host = pci_create_host_bridge(parent, db, ops, &info);
>>>> if (!host)
>>>> return NULL;
>>>>
>>>> @@ -2070,6 +2094,47 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db,
>>>> }
>>>> EXPORT_SYMBOL(pci_scan_root_bus);
>>>>
>>>> +struct pci_host_bridge *pci_scan_host_bridge(
>>>> + struct device *parent, u32 db, struct pci_ops *ops,
>>>> + struct pci_host_info *info)
>>>> +{
>>>> + struct pci_host_bridge_window *window;
>>>> + bool found = false;
>>>> + struct pci_host_bridge *host;
>>>> + int max;
>>>> +
>>>> + host = pci_create_host_bridge(parent, db, ops, info);
>>>> + if (!host)
>>>> + return NULL;
>>>> +
>>>> + list_for_each_entry(window, &host->windows, list)
>>>> + if (window->res->flags & IORESOURCE_BUS) {
>>>> + found = true;
>>>> + break;
>>>> + }
>>>> +
>>>> + host->bus = __pci_create_root_bus(host);
>>>> + if (!host->bus) {
>>>> + pci_free_host_bridge(host);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + if (!found) {
>>>> + dev_info(&host->bus->dev,
>>>> + "No busn resource found for root bus, will use [bus %02x-ff]\n",
>>>> + host->busnum);
>>>> + pci_bus_insert_busn_res(host->bus, host->busnum, 255);
>>>> + }
>>>> +
>>>> + max = pci_scan_child_bus(host->bus);
>>>> + if (!found)
>>>> + pci_bus_update_busn_res_end(host->bus, max);
>>>> +
>>>> + return host;
>>>> +
>>>> +}
>>>> +EXPORT_SYMBOL(pci_scan_host_bridge);
>>>> +
>>>> /**
>>>> * pci_rescan_bus_bridge_resize - scan a PCI bus for devices.
>>>> * @bridge: PCI bridge for the bus to scan
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index daa7f40..a51f5f5 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -412,6 +412,21 @@ struct pci_host_bridge {
>>>> void *release_data;
>>>> };
>>>>
>>>> +struct pci_host_info {
>>>> + u8 res_type;
>>>> + void *arg;
>>>> + struct list_head *resources; /*just for build, will clean up later */
>>>> + int (*init_res)(struct pci_host_bridge *host,
>>>> + struct pci_host_info *info);
>>>> +};
>>>> +
>>>> +static inline void init_pci_host_info(struct pci_host_info *info)
>>>> +{
>>>> + memset(info, 0 , sizeof(*info));
>>>> +}
>>>
>>> Where is this used?
>>
>> Host driver uses it to init pci_host_info.
>
> Might be worth adding it that patch rather than here.
OK
>
> Best regards,
> Liviu
>
>>
>>>
>>>> +
>>>> +#define PCI_HOST_RES_DEFAULT 0x2
>>>> +
>>>
>>> Magic number?
>>
>> Hmmm, I will rework pci host bridge resources stuff.
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>> #define to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
>>>> void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>>> void (*release_fn)(struct pci_host_bridge *),
>>>> @@ -420,7 +435,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
>>>> struct pci_host_bridge *pci_create_host_bridge(
>>>> struct device *parent, u32 db, struct pci_ops *ops,
>>>> - void *sys, struct list_head *resources);
>>>> + struct pci_host_info *info);
>>>> /*
>>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
>>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for
>>>> @@ -785,6 +800,9 @@ void pci_bus_release_busn_res(struct pci_bus *b);
>>>> struct pci_bus *pci_scan_root_bus(struct device *parent, u32 bus,
>>>> struct pci_ops *ops, void *sysdata,
>>>> struct list_head *resources);
>>>> +struct pci_host_bridge *pci_scan_host_bridge(struct device *parent,
>>>> + u32 db, struct pci_ops *ops,
>>>> + struct pci_host_info *info);
>>>> struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
>>>> int busnr);
>>>> void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
>>>> --
>>>> 1.7.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Thanks!
Yijing
^ permalink raw reply
* [PATCH] powerpc/powernv: Fix the hmi event version check.
From: Mahesh J Salgaonkar @ 2014-11-20 4:14 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, Benjamin Herrenschmidt
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
The current HMI event structure is an ABI and carries a version field to
accommodate future changes without affecting/rearranging current structure
members that are valid for previous versions. The current version check
"if (hmi_evt->version != OpalHMIEvt_V1)" seems to consider that version
will always be V1 which may not be true in future. If we start supporting
HMI event > V1, this check would fail without printing anything on older
kernels. This patch fixes this issue.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/opal-hmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal-hmi.c b/arch/powerpc/platforms/powernv/opal-hmi.c
index 5e1ed15..b322bfb 100644
--- a/arch/powerpc/platforms/powernv/opal-hmi.c
+++ b/arch/powerpc/platforms/powernv/opal-hmi.c
@@ -57,7 +57,7 @@ static void print_hmi_event_info(struct OpalHMIEvent *hmi_evt)
};
/* Print things out */
- if (hmi_evt->version != OpalHMIEvt_V1) {
+ if (hmi_evt->version < OpalHMIEvt_V1) {
pr_err("HMI Interrupt, Unknown event version %d !\n",
hmi_evt->version);
return;
^ permalink raw reply related
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Wei Yang @ 2014-11-20 5:39 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Wei Yang, Benjamin Herrenschmidt, Myron Stowe, linux-pci, gwshan,
Donald Dutile, linuxppc-dev
In-Reply-To: <20141119172350.GC23467@google.com>
On Wed, Nov 19, 2014 at 10:23:50AM -0700, Bjorn Helgaas wrote:
>On Wed, Nov 19, 2014 at 05:27:40PM +0800, Wei Yang wrote:
>> On Tue, Nov 18, 2014 at 09:26:01PM -0700, Bjorn Helgaas wrote:
>> >On Wed, Nov 19, 2014 at 11:21:00AM +0800, Wei Yang wrote:
>> >> On Wed, Nov 19, 2014 at 01:15:32PM +1100, Benjamin Herrenschmidt wrote:
>> >> >On Tue, 2014-11-18 at 18:12 -0700, Bjorn Helgaas wrote:
>
>> >> But the HW
>> >> must map 256 segments with the same size. This will lead a situation like
>> >> this.
>> >>
>> >> +------+------+ +------+------+------+------+
>> >> |VF#0 |VF#1 | ... | |VF#N-1|PF#A |PF#B |
>> >> +------+------+ +------+------+------+------+
>> >>
>> >> Suppose N = 254 and the HW map these 256 segments to their corresponding PE#.
>> >
>> >I guess these 256 segments are regions of CPU physical address space, and
>> >they are being mapped to bus address space? Is there some relationship
>> >between a PE and part of the bus address space?
>> >
>>
>> PE is an entity for EEH, which may include a whole bus or one pci device.
>
>Yes, I've read that many times. What's missing is the connection between a
>PE and the things in the PCI specs (buses, devices, functions, MMIO address
>space, DMA, MSI, etc.) Presumably the PE structure imposes constraints on
>how the core uses the standard PCI elements, but we don't really have a
>clear description of those constraints yet.
>
>> When some device got some error, we need to identify which PE it belongs to.
>> So we have some HW to map between PE# and MMIO/DMA/MSI address.
>>
>> The HW mentioned in previous letter is the one to map MMIO address to a PE#.
>> While this HW must map a range with 256 equal segments. And yes, this is
>> mapped to bus address space.
>> ...
>
>> >> The difference after our expanding is the IOV BAR size is 256*4KB instead of
>> >> 16KB. So it will look like this:
>> >>
>> >> PF pci_dev->resource[7] = [mem 0x00000000-0x000fffff] (1024KB)
>> >
>> >Is the idea that you want this resource to be big enough to cover all 256
>> >segments? I think I'm OK with increasing the size of the PF resources to
>> >prevent overlap. That part shouldn't be too ugly.
>> >
>>
>> Yes, big enough to cover all 256 segments.
>>
>> Sorry for making it ugly :-(
>
>I didn't mean that what you did was ugly. I meant that increasing the size
>of the PF resource can be done cleanly.
>
>By the way, when you do this, it would be nice if the dmesg showed the
>standard PF IOV BAR sizing, and then a separate line showing the resource
>expansion to deal with the PE constraints. I don't think even the standard
>output is very clear -- I think we currently get something like this:
>
> pci 0000:00:00.0 reg 0x174: [mem 0x00000000-0x00000fff]
>
>But that is only the size of a single VF BAR aperture. Then sriov_init()
>multiplies that by the number of possible VFs, but I don't think we print
>the overall size of that PF resource. I think we should, because it's
>misleading to print only the smaller piece. Maybe something like this:
>
> pci 0000:00:00.0 VF BAR0: [mem 0x00000000-0x00003fff] (for 4 VFs)
>
>And then you could do something like:
>
> pci 0000:00:00.0 VF BAR0: [mem 0x00000000-0x000fffff] (expanded for PE alignment)
>
Got it, will add message to reflect it.
>> >> VF1 pci_dev->resource[0] = [mem 0x00000000-0x00000fff]
>> >> VF2 pci_dev->resource[0] = [mem 0x00001000-0x00001fff]
>> >> VF3 pci_dev->resource[0] = [mem 0x00002000-0x00002fff]
>> >> VF4 pci_dev->resource[0] = [mem 0x00003000-0x00003fff]
>> >> ...
>> >> and 252 4KB space leave not used.
>> >>
>> >> So the start address and the size of VF will not change, but the PF's IOV BAR
>> >> will be expanded.
>> >
>> >I'm really dubious about this change to use pci_iov_resource_size(). I
>> >think you might be doing that because if you increase the PF resource size,
>> >dividing that increased size by total_VFs will give you garbage. E.g., in
>> >the example above, you would compute "size = 1024KB / 4", which would make
>> >the VF BARs appear to be 256KB instead of 4KB as they should be.
>>
>> Yes, your understanding is correct.
>>
>> >I think it would be better to solve that problem by decoupling the PF
>> >resource size and the VF BAR size. For example, we could keep track of the
>> >VF BAR size explicitly in struct pci_sriov, instead of computing it from
>> >the PF resource size and total_VFs. This would keep the VF BAR size
>> >completely platform-independent.
>>
>> Hmm... this is another solution.
>>
>> If you prefer this one, I will make a change accordingly.
>
>Yes, I definitely prefer to track the VF BAR size explicitly. I think that
>will make the code much clearer.
Got it.
>
>Bjorn
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH V9 03/18] PCI: Add weak pcibios_iov_resource_size() interface
From: Wei Yang @ 2014-11-20 5:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Wei Yang, Myron Stowe, linux-pci, gwshan, Donald Dutile,
Bjorn Helgaas, linuxppc-dev
In-Reply-To: <1416430300.5704.35.camel@au1.ibm.com>
On Thu, Nov 20, 2014 at 07:51:40AM +1100, Benjamin Herrenschmidt wrote:
>On Wed, 2014-11-19 at 10:23 -0700, Bjorn Helgaas wrote:
>>
>> Yes, I've read that many times. What's missing is the connection between a
>> PE and the things in the PCI specs (buses, devices, functions, MMIO address
>> space, DMA, MSI, etc.) Presumably the PE structure imposes constraints on
>> how the core uses the standard PCI elements, but we don't really have a
>> clear description of those constraints yet.
>
>Right, a "PE" is a HW concept in fact in our bridges, that essentially is
>a shared isolation state between DMA, MMIO, MSIs, PCIe error messages,...
>for a given "domain" or set of PCI functions.
>
>The techniques of how the HW resources are mapped to PE and associated
>constraints are slightly different from one generation of our chips to
>the next. In general, P7 follows an architecture known as "IODA" and P8
>"IODA2". I'm trying to get that spec made available via OpenPower but
>that hasn't happened yet.
>
>In this case we mostly care about IODA2 (P8), so I'll give a quick
>description here. Wei, feel free to copy/paste that into a bit of doco
>to throw into Documentation/powerpc/ along with your next spin of the patch.
>
Got it.
I will add more description in powerpc directory.
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH V9 08/18] powrepc/pci: Refactor pci_dn
From: Wei Yang @ 2014-11-20 7:20 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Wei Yang, benh, linuxppc-dev, gwshan
In-Reply-To: <20141119233024.GF23467@google.com>
On Wed, Nov 19, 2014 at 04:30:24PM -0700, Bjorn Helgaas wrote:
>On Sun, Nov 02, 2014 at 11:41:24PM +0800, Wei Yang wrote:
>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>
>> pci_dn is the extension of PCI device node and it's created from
>> device node. Unfortunately, VFs that are enabled dynamically by
>> PF's driver and they don't have corresponding device nodes, and
>> pci_dn. The patch refactors pci_dn to support VFs:
>>
>> * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
>> to the child list of pci_dn of PF's bridge. pci_dn of other
>> device put to the child list of pci_dn of its upstream bridge.
>>
>> * VF's pci_dn is expected to be created dynamically when applying
>> final fixup to PF. VF's pci_dn will be destroyed when releasing
>> PF's pci_dev instance. pci_dn of other device is still created
>> from device node as before.
>>
>> * For one particular PCI device (VF or not), its pci_dn can be
>> found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
>> or parent's list. The fast path (fetching pci_dn through PCI
>> device instance) is populated during early fixup time.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> ...
>
>> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> + struct pci_dn *parent, *pdn;
>> + int i;
>> +
>> + /* Only support IOV for now */
>> + if (!pdev->is_physfn)
>> + return pci_get_pdn(pdev);
>> +
>> + /* Check if VFs have been populated */
>> + pdn = pci_get_pdn(pdev);
>> + if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
>> + return NULL;
>> +
>> + pdn->flags |= PCI_DN_FLAG_IOV_VF;
>> + parent = pci_bus_to_pdn(pdev->bus);
>> + if (!parent)
>> + return NULL;
>> +
>> + for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
>> + pdn = add_one_dev_pci_info(parent, NULL,
>> + pci_iov_virtfn_bus(pdev, i),
>> + pci_iov_virtfn_devfn(pdev, i));
>
>I'm not sure this makes sense, but I certainly don't know this code, so
>maybe I'm missing something.
>
>pci_iov_virtfn_bus() and pci_iov_virtfn_devfn() depend on
>pdev->sriov->stride and pdev->sriov->offset. These are read from VF Stride
>and First VF Offset in the SR-IOV capability by sriov_init(), which is
>called before add_dev_pci_info():
>
> pci_scan_child_bus
> pci_scan_slot
> pci_scan_single_device
> pci_device_add
> pci_init_capabilities
> pci_iov_init(PF)
> sriov_init(PF, pos)
> pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0)
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset)
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride)
> iov->offset = offset
> iov->stride = stride
>
> pci_bus_add_devices
> pci_bus_add_device
> pci_fixup_device(pci_fixup_final)
> add_dev_pci_info
> pci_iov_virtfn_bus
> return ... + sriov->offset + (sriov->stride * id) ...
>
>But both First VF Offset and VF Stride change when ARI Capable Hierarchy or
>NumVFs changes (SR-IOV spec sec 3.3.9, 3.3.10). We set NumVFs to zero in
>sriov_init() above. We will change NumVFs to something different when a
>driver calls pci_enable_sriov():
>
> pci_enable_sriov
> sriov_enable
> pci_write_config_word(dev, iov->pos + PCI_SRIOV_NUM_VF, nr_virtfn)
>
>Now First VF Offset and VF Stride have changed from what they were when we
>called pci_iov_virtfn_bus() above.
>
Oops, I see the ARI would affect those value, while missed the NumVFs also
would.
Let's look at the problem one by one.
1. The ARI capability.
===============================================================================
The kernel initialize the capability like this:
pci_init_capabilities()
pci_configure_ari()
pci_iov_init()
iov->offset = offset
iov->stride = stride
When offset/stride is retrieved at this point, the ARI capability is taken
into consideration.
2. The PF's NumVFs field
===============================================================================
2.1 Potential problem in current code
===============================================================================
First, is current pci code has some potential problem?
sriov_enable()
pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_OFFSET, &offset);
pci_read_config_word(dev, iov->pos + PCI_SRIOV_VF_STRIDE, &stride);
iov->offset = offset;
iov->stride = stride;
pci_write_config_word(dev, iov->pos + PCI_SRIOV_NUM_VF, nr_virtfn);
virtfn_add()
...
virtfn->devfn = pci_iov_virtfn_devfn(dev, id);
The sriov_enable() retrieve the offset/stride then write the NumVFs. According
to the SPEC, at this moment the offset/stride may change. While I don't see
some code to retrieve and store those value again. And these fields will be
used in virtfn_add().
If my understanding is correct, I suggest to move the retrieve and store
operation after NumVFs is written.
2.2 The IOV bus range may not be correct in pci_scan_child_bus()?
===============================================================================
In current pci core, when enumerating the pci tree, we do like this:
pci_scan_child_bus()
pci_scan_slot()
pci_scan_single_device()
pci_device_add()
pci_init_capabilities()
pci_iov_init()
max += pci_iov_bus_range(bus);
busnr = pci_iov_virtfn_bus(dev, dev->sriov->total_VFs - 1);
max = pci_scan_bridge(bus, dev, max, pass);
>From this point, we see pci core reserve some bus range for VFs. This
calculation is based on the offset/stride at this moment. And do the
enumeration with the new bus number.
sriov_enable() could be called several times from driver to enable SRIOV, and
with different nr_virtfn. If each time the NumVFs written, the offset/stride
will change. This means we may try to address an extra bus we didn't reserve?
Or this means it is out of control?
Do I miss something?
2.3 How can I reserve bus range in FW?
===============================================================================
This question comes from the previous one.
Based on my understanding, current pci core will rely on the bus number in HW
if pcibios_assign_all_busses() is not set. If we want to support those VFs
sits on different bus with PF, we need to reserve bus range and write the
correct secondary/subordinate in bridge. Otherwise, those VFs on different bus
may not be addressed.
Currently I am writing the code in FW to reserve the range with the same
mechanism in pci core. While as you mentioned the offset/stride may change
after sriov_enable(), I am confused whether this is the correct way.
2.4 The potential problem for [Patch 08/18]
===============================================================================
According to the SPEC, the offset/stride will change after each
sriov_enable(). This means the bus/devfn will change after each
sriov_enable().
My current thought is to fix it up in virtfn_add(). If the total VF number
will not change, we could create those pci_dn at the beginning and fix the
bus/devfn at each time the VF is truely created.
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Re: [PATCH V9 08/18] powrepc/pci: Refactor pci_dn
From: Wei Yang @ 2014-11-20 7:25 UTC (permalink / raw)
To: Gavin Shan; +Cc: Bjorn Helgaas, linux-pci, Wei Yang, benh, linuxppc-dev
In-Reply-To: <20141120010213.GA11893@shangw>
On Thu, Nov 20, 2014 at 12:02:13PM +1100, Gavin Shan wrote:
>On Wed, Nov 19, 2014 at 04:30:24PM -0700, Bjorn Helgaas wrote:
>>On Sun, Nov 02, 2014 at 11:41:24PM +0800, Wei Yang wrote:
>>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>
>>> pci_dn is the extension of PCI device node and it's created from
>>> device node. Unfortunately, VFs that are enabled dynamically by
>>> PF's driver and they don't have corresponding device nodes, and
>>> pci_dn. The patch refactors pci_dn to support VFs:
>>>
>>> * pci_dn is organized as a hierarchy tree. VF's pci_dn is put
>>> to the child list of pci_dn of PF's bridge. pci_dn of other
>>> device put to the child list of pci_dn of its upstream bridge.
>>>
>>> * VF's pci_dn is expected to be created dynamically when applying
>>> final fixup to PF. VF's pci_dn will be destroyed when releasing
>>> PF's pci_dev instance. pci_dn of other device is still created
>>> from device node as before.
>>>
>>> * For one particular PCI device (VF or not), its pci_dn can be
>>> found from pdev->dev.archdata.firmware_data, PCI_DN(devnode),
>>> or parent's list. The fast path (fetching pci_dn through PCI
>>> device instance) is populated during early fixup time.
>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>> ...
>>
>>> +struct pci_dn *add_dev_pci_info(struct pci_dev *pdev)
>>> +{
>>> +#ifdef CONFIG_PCI_IOV
>>> + struct pci_dn *parent, *pdn;
>>> + int i;
>>> +
>>> + /* Only support IOV for now */
>>> + if (!pdev->is_physfn)
>>> + return pci_get_pdn(pdev);
>>> +
>>> + /* Check if VFs have been populated */
>>> + pdn = pci_get_pdn(pdev);
>>> + if (!pdn || (pdn->flags & PCI_DN_FLAG_IOV_VF))
>>> + return NULL;
>>> +
>>> + pdn->flags |= PCI_DN_FLAG_IOV_VF;
>>> + parent = pci_bus_to_pdn(pdev->bus);
>>> + if (!parent)
>>> + return NULL;
>>> +
>>> + for (i = 0; i < pci_sriov_get_totalvfs(pdev); i++) {
>>> + pdn = add_one_dev_pci_info(parent, NULL,
>>> + pci_iov_virtfn_bus(pdev, i),
>>> + pci_iov_virtfn_devfn(pdev, i));
>>
>>I'm not sure this makes sense, but I certainly don't know this code, so
>>maybe I'm missing something.
>>
>
>For ARI, Richard had some patches to fix the issue from firmware side.
>
>>pci_iov_virtfn_bus() and pci_iov_virtfn_devfn() depend on
>>pdev->sriov->stride and pdev->sriov->offset. These are read from VF Stride
>>and First VF Offset in the SR-IOV capability by sriov_init(), which is
>>called before add_dev_pci_info():
>>
>> pci_scan_child_bus
>> pci_scan_slot
>> pci_scan_single_device
>> pci_device_add
>> pci_init_capabilities
>> pci_iov_init(PF)
>> sriov_init(PF, pos)
>> pci_write_config_word(dev, pos + PCI_SRIOV_NUM_VF, 0)
>> pci_read_config_word(dev, pos + PCI_SRIOV_VF_OFFSET, &offset)
>> pci_read_config_word(dev, pos + PCI_SRIOV_VF_STRIDE, &stride)
>> iov->offset = offset
>> iov->stride = stride
>>
>> pci_bus_add_devices
>> pci_bus_add_device
>> pci_fixup_device(pci_fixup_final)
>> add_dev_pci_info
>> pci_iov_virtfn_bus
>> return ... + sriov->offset + (sriov->stride * id) ...
>>
>>But both First VF Offset and VF Stride change when ARI Capable Hierarchy or
>>NumVFs changes (SR-IOV spec sec 3.3.9, 3.3.10). We set NumVFs to zero in
>>sriov_init() above. We will change NumVFs to something different when a
>>driver calls pci_enable_sriov():
>>
>> pci_enable_sriov
>> sriov_enable
>> pci_write_config_word(dev, iov->pos + PCI_SRIOV_NUM_VF, nr_virtfn)
>>
>>Now First VF Offset and VF Stride have changed from what they were when we
>>called pci_iov_virtfn_bus() above.
>>
>
>It's the case we missed: First VF Offset and VF Stride can change when
>PF's number of VFs is changed. It means the BDFN (Bus/Device/Function
>number) for one VF can't be determined until PF's number of VFs is
>populated and updated to HW (before calling to virtfn_add()).
>
>The dynamically created pci_dn is used in PCI config accessors currently.
>That means we have to get it ready before first PCI config request to the
>VF in pci_setup_device(). In the code of old revision, we had some weak
>function called in pci_alloc_dev(), which gave platform chance to create
>pci_dn. I think we have to switch back to the old way in order to fix
>the problem you catched. However, the old way is implemented with cost
>of more weak function, which you're probably unhappy to see.
>
> sriov_enable()
> virtfn_add()
> virtfn_add_bus()
> pci_alloc_dev()
> pci_setup_device()
Ok, sounds my solution in previous reply can't work. We need the pci_dn ready
before access the configuration space of VFs.
--
Richard Yang
Help you, Help me
^ permalink raw reply
* Re: Fan management PowerMac7,2
From: Martin Kukač @ 2014-11-20 9:11 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <546D53F5.4030205@daenzer.net>
On 11/20/2014 03:37 AM, Michel Dänzer wrote:
> On 19.11.2014 21:04, Martin Kukač wrote:
>> Hello,
>>
>> I was directed to this list on #ppc64/freenode IRC channel. I have
>> PowerMac7,2 (AGP, 2xPowerPC 970(2.2) 2GHz) and I wanted to try kernel
>> supporting kvm-pr module, which is not in 3.2.63 kernel provided with
>> Debian Wheezy. I downloaded 3.14.24, compiled it using original config
>> from 3.2.63 and after booting fan management does not work. I tried
>> therm_pm72 (which is deprecated, but works well with Debian kernel) and
>> windfarm_pm72 modules, but neither worked - after about five minutes
>> from boot all fans go to maximum and stay this way no matter the CPU
>> load or frequency.
>>
>> I downloaded vanilla 3.2.64, compiled it the same way and there fans
>> work with therm_p72 well. This kernel does not have kvm-pr, so I
>> reverted to original kernel provided with Wheezy.
>>
>> I looked to logs, i tried to search the web whether anyone had already
>> the same problem, but it seems that nobody else described not to say
>> solved this. Can anyone please point me to anything that would help? I
>> can provide whatever log files required.
>
> Might be the same problem as discussed in
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-August/109700.html
> and the following posts: The i2c_powermac module doesn't get loaded
> automatically with recent kernels.
>
>
Thanks! That helped for therm_pm72 module, it works now as expected.
(It didn't help for windfarm_pm72 though, if I put it in /etc/modules
with i2c_powermac and remove therm_pm72, fans still go to full speed in
a few minutes. It's not problem for now, but if I understand it
correctly, therm_pm72 is deprecated and will be removed one day, so it
could be problem after that.)
Martin
^ permalink raw reply
* Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
From: Liviu Dudau @ 2014-11-20 9:47 UTC (permalink / raw)
To: Yijing Wang
Cc: Liviu Dudau, Tony Luck, Russell King, linux-pci@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org, huxinwei@huawei.com,
Thierry Reding, suravee.suthikulpanit@amd.com, Bjorn Helgaas,
linux-ia64@vger.kernel.org, Thomas Gleixner, Wuyun,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <546D5647.8030401@huawei.com>
On Thu, Nov 20, 2014 at 02:47:35AM +0000, Yijing Wang wrote:
> >>>> +static void pci_release_host_bridge_dev(struct device *dev)
> >>>> +{
> >>>> + struct pci_host_bridge *bridge =3D to_pci_host_bridge(dev);
> >>>> +
> >>>> + if (bridge->release_fn)
> >>>> + bridge->release_fn(bridge);
> >>>> + pci_free_resource_list(&bridge->windows);
> >>>> + kfree(bridge);
> >>>> +}
> >>>> +
> >>>> +struct pci_host_bridge *pci_create_host_bridge(
> >>>> + struct device *parent, u32 db,
> >>>> + struct pci_ops *ops, void *sysdata,
> >>>
> >>> I don't thinks it is worth moving the buses' pci_ops into pci_host_br=
idge. It
> >>> might be more useful to have pci_host_bridge specific ops here.
> >>
> >> Because we want to create pci_host_bridge before pci root bus creation=
,
> >> so when we scan the root bus and child buses, we use pci_host_bridge a=
s
> >> the only argument, and another pci_host_info will be addes in later pa=
tch,
> >> which one support carry the pci_host_bridge ops.
> >
> > But pci_create_root_bus() already has a pci_ops argument, I don't see t=
he reason
> > to drop that.
> >
> > pci_create_host_bridge() can get pci_host_bridge ops while pci_create_r=
oot_bus() gets
> > the bus ops. For find out the MSI controller, the domain number and any=
other HB
> > specific stuff, you use the HB ops. For config R/W acceses you use bus =
ops.
> >
>=20
> I want to unexport pci_create_root_bus() if we have pci_create_host_bridg=
e().
That's fine, but the function signature doesn't change, right? And I think =
we should keep
pci_scan_root_bus() with the current signature as well (except change sysda=
ta into a
struct pci_host_bridge *). We create the host bridge in one step, stuff it =
with all the
data that we need for scanning the root and associated busses, and then cal=
l pci_scan_root_bus().
pci_host_bridge has the ops from pci_create_host_bridge() and the root bus =
from pci_scan_root_bus().
Hope this makes sense.
>=20
>=20
> >>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db,
> >>>> - struct pci_ops *ops, void *sysdata, struct list_head=
*resources)
> >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridg=
e)
> >>>> {
> >>>> int error;
> >>>> - struct pci_host_bridge *bridge;
> >>>> struct pci_bus *b, *b2;
> >>>> - struct pci_host_bridge_window *window, *n;
> >>>> + struct pci_host_bridge_window *window;
> >>>> struct resource *res;
> >>>> resource_size_t offset;
> >>>> char bus_addr[64];
> >>>> char *fmt;
> >>>> - u8 bus =3D PCI_BUSNUM(db);
> >>>> + struct device *parent =3D bridge->dev.parent;
> >>>>
> >>>> b =3D pci_alloc_bus(NULL);
> >>>> if (!b)
> >>>> return NULL;
> >>>>
> >>>> - b->sysdata =3D sysdata;
> >>>> - b->ops =3D ops;
> >>>> - b->number =3D b->busn_res.start =3D bus;
> >>>> + b->sysdata =3D bridge->sysdata;
> >>>
> >>> I think bridge should be the b->sysdata here.
> >>
> >> ? what's the meaning?
> >
> > Currently, bus->sysdata holds a pointer to the arch/driver host bridge =
structure, as passed
> > in pci_create_root_bus(). If you agree with my idea of wrapping the hos=
t bridge driver
> > structure around the pci_host_bridge, then we will always have a way of=
retrieving that
> > information; but for backwards compatibility we could set bus->sysdata =
to be the bridge.
> > Then existing macros that convert sysdata to pci_controller can be reus=
ed after being
> > updated.
>=20
> I think wrapping the host bridge driver structure around the pci_host_bri=
dge could help
> us to find the private pci_controller, but in this case, pci_host_bridge =
is not a pointer
> so we put all pci_host_bridge init stuff in host drivers ?=20
Of course we put pci_host_bridge init *calls* in the host drivers because w=
e need to do host
driver specific initialisation there anyway.=20
> And if we still use arch spec
> macros convert ssydata to pci_controller, so what's the greatest advantag=
e ?
Agree, the macro is not useful unless we have a common pci_controller struc=
ture. Sorry about
the noise.
>=20
> If we move domain and msi_chip out of sysdata, what's other things in sys=
data still need us
> to convert in PCI core ?
I've never said we need to keep sysdata. From my discussion with Bjorn a ye=
ar ago the idea was
to put the sysdata members into pci_host_bridge structure and remove sysdat=
a member from pci_bus.
pci_scan_root_bus() can still get a parameter called sysdata if we want, bu=
t it will be a
struct pci_host_bridge * type.
Best regards,
Liviu
>=20
>=20
>=20
> >
> > Best regards,
> > Liviu
> >
> >>
> >>>
> >>>> + b->ops =3D bridge->ops;
> >>>
> >>> See comment above why I don't think this is necessary.
> >>>
> >>>> + b->number =3D b->busn_res.start =3D bridge->busnum;
> >>>> pci_bus_assign_domain_nr(b, parent);
> >>>> - b2 =3D pci_find_bus(pci_domain_nr(b), bus);
> >>>> + bridge->domain =3D pci_domain_nr(b);
> >>>
> >>> Do you really want to overwrite the bridge's domain with the one from=
a bus that
> >>> could possibly be rejected a couple of lines further down?
> >>>
> >>> As an asside: if we are doing the split of pci_host_bridge from root =
bus creation
> >>> it is worth in my opinion to move the domain setup in pci_create_host=
_bridge()
> >>> and stop fiddling with it here.
> >>
> >>
> >> Hi Liviu, these lines just temporary, I will remove it after all host =
drivers
> >> save its domain in pci_host_bridge.
> >>
> >>
> >>>
> >>> Otherwise it looks to me like you are heading in the right direction.
> >>
> >> Thanks!
> >> Yijing.
> >>
> >>>
> >>> Best regards,
> >>> Liviu
> >>>
> >>>> + b2 =3D pci_find_bus(pci_domain_nr(b), bridge->busnum);
> >>>> if (b2) {
> >>>> /* If we already got to this bus through a different=
bridge, ignore it */
> >>>> dev_dbg(&b2->dev, "bus already known\n");
> >>>> goto err_out;
> >>>> }
> >>>>
> >>>> - bridge =3D pci_alloc_host_bridge(b);
> >>>> - if (!bridge)
> >>>> - goto err_out;
> >>>> -
> >>>> - bridge->dev.parent =3D parent;
> >>>> - bridge->dev.release =3D pci_release_host_bridge_dev;
> >>>> - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b),=
bus);
> >>>> - error =3D pcibios_root_bridge_prepare(bridge);
> >>>> - if (error) {
> >>>> - kfree(bridge);
> >>>> - goto err_out;
> >>>> - }
> >>>> -
> >>>> - error =3D device_register(&bridge->dev);
> >>>> - if (error) {
> >>>> - put_device(&bridge->dev);
> >>>> - goto err_out;
> >>>> - }
> >>>> b->bridge =3D get_device(&bridge->dev);
> >>>> device_enable_async_suspend(b->bridge);
> >>>> pci_set_bus_of_node(b);
> >>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct dev=
ice *parent, u32 db,
> >>>>
> >>>> b->dev.class =3D &pcibus_class;
> >>>> b->dev.parent =3D b->bridge;
> >>>> - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> >>>> + dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), b->numb=
er);
> >>>> error =3D device_register(&b->dev);
> >>>> if (error)
> >>>> goto class_dev_reg_err;
> >>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct d=
evice *parent, u32 db,
> >>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_=
name(&b->dev));
> >>>>
> >>>> /* Add initial resources to the bus */
> >>>> - list_for_each_entry_safe(window, n, resources, list) {
> >>>> - list_move_tail(&window->list, &bridge->windows);
> >>>> + list_for_each_entry(window, &bridge->windows, list) {
> >>>> res =3D window->res;
> >>>> offset =3D window->offset;
> >>>> if (res->flags & IORESOURCE_BUS)
> >>>> - pci_bus_insert_busn_res(b, bus, res->end);
> >>>> + pci_bus_insert_busn_res(b, b->number, res->e=
nd);
> >>>> else
> >>>> pci_bus_add_resource(b, res, 0);
> >>>> if (offset) {
> >>>> @@ -2001,6 +1949,25 @@ err_out:
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
> >>>> + struct pci_ops *ops, void *sysdata, struct list_head=
*resources)
> >>>> +{
> >>>> + struct pci_host_bridge *host;
> >>>> +
> >>>> + host =3D pci_create_host_bridge(parent, bus, ops,
> >>>> + sysdata ,resources);
> >>>> + if (!host)
> >>>> + return NULL;
> >>>> +
> >>>> + host->bus =3D __pci_create_root_bus(host);
> >>>> + if (!host->bus) {
> >>>> + pci_free_host_bridge(host);
> >>>> + return NULL;
> >>>> + }
> >>>> +
> >>>> + return host->bus;
> >>>> +}
> >>>> +
> >>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max=
)
> >>>> {
> >>>> struct resource *res =3D &b->busn_res;
> >>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct dev=
ice *parent, u32 db,
> >>>> {
> >>>> struct pci_host_bridge_window *window;
> >>>> bool found =3D false;
> >>>> - struct pci_bus *b;
> >>>> - LIST_HEAD(default_res);
> >>>> + struct pci_host_bridge *host;
> >>>> int max;
> >>>>
> >>>> - if (!resources) {
> >>>> - pci_add_resource(&default_res, &ioport_resource);
> >>>> - pci_add_resource(&default_res, &iomem_resource);
> >>>> - pci_add_resource(&default_res, &busn_resource);
> >>>> - } else {
> >>>> - list_for_each_entry(window, resources, list)
> >>>> - if (window->res->flags & IORESOURCE_BUS) {
> >>>> - found =3D true;
> >>>> - break;
> >>>> - }
> >>>> - }
> >>>> + host =3D pci_create_host_bridge(parent, db, ops, sysdata, re=
sources);
> >>>> + if (!host)
> >>>> + return NULL;
> >>>>
> >>>> - b =3D pci_create_root_bus(parent, db, ops, sysdata,
> >>>> - resources ? resources : &default_res);
> >>>> - if (!b)
> >>>> + list_for_each_entry(window, &host->windows, list)
> >>>> + if (window->res->flags & IORESOURCE_BUS) {
> >>>> + found =3D true;
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + host->bus =3D __pci_create_root_bus(host);
> >>>> + if (!host->bus) {
> >>>> + pci_free_host_bridge(host);
> >>>> return NULL;
> >>>> + }
> >>>>
> >>>> if (!found) {
> >>>> - dev_info(&b->dev,
> >>>> + dev_info(&host->bus->dev,
> >>>> "No busn resource found for root bus, will use [bus=
%02x-ff]\n",
> >>>> PCI_BUSNUM(db));
> >>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255);
> >>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 2=
55);
> >>>> }
> >>>>
> >>>> - max =3D pci_scan_child_bus(b);
> >>>> -
> >>>> + max =3D pci_scan_child_bus(host->bus);
> >>>> if (!found)
> >>>> - pci_bus_update_busn_res_end(b, max);
> >>>> + pci_bus_update_busn_res_end(host->bus, max);
> >>>>
> >>>> - return b;
> >>>> + return host->bus;
> >>>> }
> >>>> EXPORT_SYMBOL(pci_scan_root_bus);
> >>>>
> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>>> index 8b11b38..daa7f40 100644
> >>>> --- a/include/linux/pci.h
> >>>> +++ b/include/linux/pci.h
> >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window {
> >>>> struct pci_host_bridge {
> >>>> struct device dev;
> >>>> struct pci_bus *bus; /* root bus */
> >>>> + struct list_head list;
> >>>> struct list_head windows; /* pci_host_bridge_windows *=
/
> >>>> + int busnum;
> >>>> + int domain;
> >>>> + void *sysdata;
> >>>> + struct pci_ops *ops;
> >>>> void (*release_fn)(struct pci_host_bridge *);
> >>>> void *release_data;
> >>>> };
> >>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host=
_bridge *bridge,
> >>>> void *release_data);
> >>>>
> >>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
> >>>> -
> >>>> +struct pci_host_bridge *pci_create_host_bridge(
> >>>> + struct device *parent, u32 db, struct pci_ops *ops,
> >>>> + void *sys, struct list_head *resources);
> >>>> /*
> >>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that =
correspond
> >>>> * to P2P or CardBus bridge windows) go in a table. Additional one=
s (for
> >>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *b=
us);
> >>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus,
> >>>> struct pci_ops *ops, void *sysda=
ta,
> >>>> struct list_head *resources);
> >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host)=
;
> >>>> +void pci_free_host_bridge(struct pci_host_bridge *host);
> >>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax)=
;
> >>>> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >>>> void pci_bus_release_busn_res(struct pci_bus *b);
> >>>> --
> >>>> 1.7.1
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe linux-pci"=
in
> >>>> the body of a message to majordomo@vger.kernel.org
> >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>>
> >>>
> >>
> >>
> >> --
> >> Thanks!
> >> Yijing
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-pci" i=
n
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
>=20
>=20
> --
> Thanks!
> Yijing
>=20
>=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
^ permalink raw reply
* [PATCH 01/10] mm: numa: Do not dereference pmd outside of the lock during NUMA hinting fault
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
A transhuge NUMA hinting fault may find the page is migrating and should
wait until migration completes. The check is race-prone because the pmd
is deferenced outside of the page lock and while the race is tiny, it'll
be larger if the PMD is cleared while marking PMDs for hinting fault.
This patch closes the race.
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
include/linux/migrate.h | 4 ----
mm/huge_memory.c | 3 ++-
mm/migrate.c | 6 ------
3 files changed, 2 insertions(+), 11 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 01aad3e..a3edcdf 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -77,7 +77,6 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
#ifdef CONFIG_NUMA_BALANCING
extern bool pmd_trans_migrating(pmd_t pmd);
-extern void wait_migrate_huge_page(struct anon_vma *anon_vma, pmd_t *pmd);
extern int migrate_misplaced_page(struct page *page,
struct vm_area_struct *vma, int node);
extern bool migrate_ratelimited(int node);
@@ -86,9 +85,6 @@ static inline bool pmd_trans_migrating(pmd_t pmd)
{
return false;
}
-static inline void wait_migrate_huge_page(struct anon_vma *anon_vma, pmd_t *pmd)
-{
-}
static inline int migrate_misplaced_page(struct page *page,
struct vm_area_struct *vma, int node)
{
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index de98415..38fa6cc 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1284,8 +1284,9 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
* check_same as the page may no longer be mapped.
*/
if (unlikely(pmd_trans_migrating(*pmdp))) {
+ page = pmd_page(*pmdp);
spin_unlock(ptl);
- wait_migrate_huge_page(vma->anon_vma, pmdp);
+ wait_on_page_locked(page);
goto out;
}
diff --git a/mm/migrate.c b/mm/migrate.c
index 0143995..baaf80e 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1698,12 +1698,6 @@ bool pmd_trans_migrating(pmd_t pmd)
return PageLocked(page);
}
-void wait_migrate_huge_page(struct anon_vma *anon_vma, pmd_t *pmd)
-{
- struct page *page = pmd_page(*pmd);
- wait_on_page_locked(page);
-}
-
/*
* Attempt to migrate a misplaced page to the specified destination
* node. Caller is expected to have an elevated reference count on
--
2.1.2
^ permalink raw reply related
* [PATCH 0/10] Replace _PAGE_NUMA with PAGE_NONE protections v2
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
V1 failed while running under kvm-tools very quickly and a second report
indicated that it happens on bare metal as well. This version survived
an overnight run of trinity running under kvm-tools here but verification
from Sasha would be appreciated.
Changelog since V1
o ppc64 paranoia checks and clarifications (aneesh)
o Fix trinity regression (hopefully)
o Reduce unnecessary TLB flushes (mel)
Automatic NUMA balancing depends on being able to protect PTEs to trap a
fault and gather reference locality information. Very broadly speaking it
would mark PTEs as not present and use another bit to distinguish between
NUMA hinting faults and other types of faults. It was universally loved
by everybody and caused no problems whatsoever. That last sentence might
be a lie.
This series is very heavily based on patches from Linus and Aneesh to
replace the existing PTE/PMD NUMA helper functions with normal change
protections. I did alter and add parts of it but I consider them relatively
minor contributions. At their suggestion, acked-bys are in there but I've
no problem converting them to Signed-off-by if requested.
AFAIK, this has received no testing on ppc64 and I'm depending on Aneesh for
that. I tested trinity under kvm-tool and passed and ran a few other basic
tests. In most cases I'm leaving out detail as it's not that interesting.
specjbb single JVM: There was negligible performance difference in the
benchmark itself for short and long runs. However, system activity
is higher and interrupts are much higher over time -- possibly
TLB flushes. Migrations are also higher. Overall, this is more
overhead but considering the problems faced with the old approach
I think we just have to suck it up and find another way of reducing
the overhead.
specjbb multi JVM: Negligible performance difference to the actual benchmarm
but like the single JVM case, the system overhead is noticably
higher. Again, interrupts are a major factor.
autonumabench: This was all over the place and about all that can be
reasonably concluded is that it's different but not necessarily
better or worse.
autonumabench
3.18.0-rc4 3.18.0-rc4
vanilla protnone-v2r5
User NUMA01 32806.01 ( 0.00%) 20250.67 ( 38.27%)
User NUMA01_THEADLOCAL 23910.28 ( 0.00%) 22734.37 ( 4.92%)
User NUMA02 3176.85 ( 0.00%) 3082.68 ( 2.96%)
User NUMA02_SMT 1600.06 ( 0.00%) 1547.08 ( 3.31%)
System NUMA01 719.07 ( 0.00%) 1344.39 (-86.96%)
System NUMA01_THEADLOCAL 916.26 ( 0.00%) 180.90 ( 80.26%)
System NUMA02 20.92 ( 0.00%) 17.34 ( 17.11%)
System NUMA02_SMT 8.76 ( 0.00%) 7.24 ( 17.35%)
Elapsed NUMA01 728.27 ( 0.00%) 519.28 ( 28.70%)
Elapsed NUMA01_THEADLOCAL 589.15 ( 0.00%) 554.73 ( 5.84%)
Elapsed NUMA02 81.20 ( 0.00%) 81.72 ( -0.64%)
Elapsed NUMA02_SMT 80.49 ( 0.00%) 79.58 ( 1.13%)
CPU NUMA01 4603.00 ( 0.00%) 4158.00 ( 9.67%)
CPU NUMA01_THEADLOCAL 4213.00 ( 0.00%) 4130.00 ( 1.97%)
CPU NUMA02 3937.00 ( 0.00%) 3793.00 ( 3.66%)
CPU NUMA02_SMT 1998.00 ( 0.00%) 1952.00 ( 2.30%)
System CPU usage of NUMA01 is worse but it's an adverse workload on this
machine so I'm reluctant to conclude that it's a problem that matters. On
the other workloads that are sensible on this machine, system CPU usage
is great. Overall time to complete the benchmark is comparable
3.18.0-rc4 3.18.0-rc4
vanillaprotnone-v2r5
User 61493.38 47615.01
System 1665.17 1550.07
Elapsed 1480.79 1236.74
NUMA alloc hit 4739774 5328362
NUMA alloc miss 0 0
NUMA interleave hit 0 0
NUMA alloc local 4664980 5328351
NUMA base PTE updates 556489407 444119981
NUMA huge PMD updates 1086000 866680
NUMA page range updates 1112521407 887860141
NUMA hint faults 1538964 1242142
NUMA hint local faults 835871 814313
NUMA hint local percent 54 65
NUMA pages migrated 7329212 59883854
The NUMA pages migrated look terrible but when I looked at a graph of the
activity over time I see that the massive spike in migration activity was
during NUMA01. This correlates with high system CPU usage and could be simply
down to bad luck but any modifications that affect that workload would be
related to scan rates and migrations, not the protection mechanism. For
all other workloads, migration activity was comparable.
Overall, headline performance figures are comparable but the overhead
is higher, mostly in interrupts. To some extent, higher overhead from
this approach was anticipated but not to this degree. It's going to be
necessary to reduce this again with a separate series in the future. It's
still worth going ahead with this series though as it's likely to avoid
constant headaches with Xen and is probably easier to maintain.
arch/powerpc/include/asm/pgtable.h | 53 ++----------
arch/powerpc/include/asm/pte-common.h | 5 --
arch/powerpc/include/asm/pte-hash64.h | 6 --
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
arch/powerpc/mm/copro_fault.c | 8 +-
arch/powerpc/mm/fault.c | 25 ++----
arch/powerpc/mm/gup.c | 4 +-
arch/powerpc/mm/pgtable.c | 8 +-
arch/powerpc/mm/pgtable_64.c | 3 +-
arch/x86/include/asm/pgtable.h | 46 +++++-----
arch/x86/include/asm/pgtable_64.h | 5 --
arch/x86/include/asm/pgtable_types.h | 41 +--------
arch/x86/mm/gup.c | 4 +-
include/asm-generic/pgtable.h | 152 ++--------------------------------
include/linux/migrate.h | 4 -
include/linux/swapops.h | 2 +-
include/uapi/linux/mempolicy.h | 2 +-
mm/gup.c | 8 +-
mm/huge_memory.c | 50 ++++++-----
mm/memory.c | 18 ++--
mm/mempolicy.c | 2 +-
mm/migrate.c | 8 +-
mm/mprotect.c | 48 +++++------
mm/pgtable-generic.c | 2 -
24 files changed, 131 insertions(+), 375 deletions(-)
--
2.1.2
^ permalink raw reply
* [PATCH 02/10] mm: Add p[te|md] protnone helpers for use by NUMA balancing
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
This is a preparatory patch that introduces protnone helpers for automatic
NUMA balancing.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/pgtable.h | 11 +++++++++++
arch/x86/include/asm/pgtable.h | 16 ++++++++++++++++
include/asm-generic/pgtable.h | 19 +++++++++++++++++++
3 files changed, 46 insertions(+)
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 316f9a5..452c3b4 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -39,6 +39,17 @@ static inline int pte_none(pte_t pte) { return (pte_val(pte) & ~_PTE_NONE_MASK)
static inline pgprot_t pte_pgprot(pte_t pte) { return __pgprot(pte_val(pte) & PAGE_PROT_BITS); }
#ifdef CONFIG_NUMA_BALANCING
+static inline int pte_protnone_numa(pte_t pte)
+{
+ return (pte_val(pte) &
+ (_PAGE_PRESENT | _PAGE_USER)) == _PAGE_PRESENT;
+}
+
+static inline int pmd_protnone_numa(pmd_t pmd)
+{
+ return pte_protnone_numa(pmd_pte(pmd));
+}
+
static inline int pte_present(pte_t pte)
{
return pte_val(pte) & _PAGE_NUMA_MASK;
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index aa97a07..613cd00 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -497,6 +497,22 @@ static inline int pmd_present(pmd_t pmd)
_PAGE_NUMA);
}
+#ifdef CONFIG_NUMA_BALANCING
+/*
+ * These work without NUMA balancing but the kernel does not care. See the
+ * comment in include/asm-generic/pgtable.h
+ */
+static inline int pte_protnone_numa(pte_t pte)
+{
+ return pte_flags(pte) & _PAGE_PROTNONE;
+}
+
+static inline int pmd_protnone_numa(pmd_t pmd)
+{
+ return pmd_flags(pmd) & _PAGE_PROTNONE;
+}
+#endif /* CONFIG_NUMA_BALANCING */
+
static inline int pmd_none(pmd_t pmd)
{
/* Only check low word on 32-bit platforms, since it might be
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 752e30d..7e74122 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -677,6 +677,25 @@ static inline int pmd_trans_unstable(pmd_t *pmd)
#endif
}
+#ifndef CONFIG_NUMA_BALANCING
+/*
+ * Technically a PTE can be PROTNONE even when not doing NUMA balancing but
+ * the only case the kernel cares is for NUMA balancing. By default,
+ * implement the helper as "always no". Note that this does not check VMA
+ * protection bits meaning that it is up to the caller to distinguish between
+ * PROT_NONE protections and NUMA hinting fault protections.
+ */
+static inline int pte_protnone_numa(pte_t pte)
+{
+ return 0;
+}
+
+static inline int pmd_protnone_numa(pmd_t pmd)
+{
+ return 0;
+}
+#endif /* CONFIG_NUMA_BALANCING */
+
#ifdef CONFIG_NUMA_BALANCING
/*
* _PAGE_NUMA distinguishes between an unmapped page table entry, an entry that
--
2.1.2
^ permalink raw reply related
* [PATCH 03/10] mm: Convert p[te|md]_numa users to p[te|md]_protnone_numa
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
Convert existing users of pte_numa and friends to the new helper. Note
that the kernel is broken after this patch is applied until the other
page table modifiers are also altered. This patch layout is to make
review easier.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Aneesh Kumar <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
arch/powerpc/mm/fault.c | 5 -----
arch/powerpc/mm/gup.c | 4 ++--
arch/powerpc/mm/pgtable.c | 8 +++++++-
arch/powerpc/mm/pgtable_64.c | 3 ++-
arch/x86/mm/gup.c | 4 ++--
include/uapi/linux/mempolicy.h | 2 +-
mm/gup.c | 8 ++++----
mm/huge_memory.c | 16 +++++++--------
mm/memory.c | 4 ++--
mm/mprotect.c | 39 ++++++++++---------------------------
mm/pgtable-generic.c | 2 +-
12 files changed, 40 insertions(+), 57 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 084ad54..bbd8499 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -235,7 +235,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
pte_size = psize;
pte = lookup_linux_pte_and_update(pgdir, hva, writing,
&pte_size);
- if (pte_present(pte) && !pte_numa(pte)) {
+ if (pte_present(pte) && !pte_protnone_numa(pte)) {
if (writing && !pte_write(pte))
/* make the actual HPTE be read-only */
ptel = hpte_make_readonly(ptel);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 08d659a..5007497 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -405,8 +405,6 @@ good_area:
* processors use the same I/D cache coherency mechanism
* as embedded.
*/
- if (error_code & DSISR_PROTFAULT)
- goto bad_area;
#endif /* CONFIG_PPC_STD_MMU */
/*
@@ -430,9 +428,6 @@ good_area:
flags |= FAULT_FLAG_WRITE;
/* a read */
} else {
- /* protection fault */
- if (error_code & 0x08000000)
- goto bad_area;
if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
goto bad_area;
}
diff --git a/arch/powerpc/mm/gup.c b/arch/powerpc/mm/gup.c
index d874668..d870d93 100644
--- a/arch/powerpc/mm/gup.c
+++ b/arch/powerpc/mm/gup.c
@@ -39,7 +39,7 @@ static noinline int gup_pte_range(pmd_t pmd, unsigned long addr,
/*
* Similar to the PMD case, NUMA hinting must take slow path
*/
- if (pte_numa(pte))
+ if (pte_protnone_numa(pte))
return 0;
if ((pte_val(pte) & mask) != result)
@@ -85,7 +85,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
* slowpath for accounting purposes and so that they
* can be serialised against THP migration.
*/
- if (pmd_numa(pmd))
+ if (pmd_protnone_numa(pmd))
return 0;
if (!gup_hugepte((pte_t *)pmdp, PMD_SIZE, addr, next,
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index c90e602..b5d58d3 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -173,7 +173,13 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
pte_t pte)
{
#ifdef CONFIG_DEBUG_VM
- WARN_ON(pte_val(*ptep) & _PAGE_PRESENT);
+ /*
+ * When handling numa faults, we already have the pte marked
+ * _PAGE_PRESENT, but we can be sure that it is not in hpte.
+ * Hence we can use set_pte_at for them.
+ */
+ WARN_ON((pte_val(*ptep) & (_PAGE_PRESENT | _PAGE_USER)) ==
+ (_PAGE_PRESENT | _PAGE_USER));
#endif
/* Note: mm->context.id might not yet have been assigned as
* this context might not have been activated yet when this
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index c8d709a..c721c5e 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -710,7 +710,8 @@ void set_pmd_at(struct mm_struct *mm, unsigned long addr,
pmd_t *pmdp, pmd_t pmd)
{
#ifdef CONFIG_DEBUG_VM
- WARN_ON(pmd_val(*pmdp) & _PAGE_PRESENT);
+ WARN_ON((pmd_val(*pmdp) & (_PAGE_PRESENT | _PAGE_USER)) ==
+ (_PAGE_PRESENT | _PAGE_USER));
assert_spin_locked(&mm->page_table_lock);
WARN_ON(!pmd_trans_huge(pmd));
#endif
diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
index 207d9aef..47ce479 100644
--- a/arch/x86/mm/gup.c
+++ b/arch/x86/mm/gup.c
@@ -84,7 +84,7 @@ static noinline int gup_pte_range(pmd_t pmd, unsigned long addr,
struct page *page;
/* Similar to the PMD case, NUMA hinting must take slow path */
- if (pte_numa(pte)) {
+ if (pte_protnone_numa(pte)) {
pte_unmap(ptep);
return 0;
}
@@ -178,7 +178,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
* slowpath for accounting purposes and so that they
* can be serialised against THP migration.
*/
- if (pmd_numa(pmd))
+ if (pmd_protnone_numa(pmd))
return 0;
if (!gup_huge_pmd(pmd, addr, next, write, pages, nr))
return 0;
diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
index 0d11c3d..e52379b 100644
--- a/include/uapi/linux/mempolicy.h
+++ b/include/uapi/linux/mempolicy.h
@@ -67,7 +67,7 @@ enum mpol_rebind_step {
#define MPOL_F_LOCAL (1 << 1) /* preferred local allocation */
#define MPOL_F_REBINDING (1 << 2) /* identify policies in rebinding */
#define MPOL_F_MOF (1 << 3) /* this policy wants migrate on fault */
-#define MPOL_F_MORON (1 << 4) /* Migrate On pte_numa Reference On Node */
+#define MPOL_F_MORON (1 << 4) /* Migrate On pte_protnone_numa Reference On Node */
#endif /* _UAPI_LINUX_MEMPOLICY_H */
diff --git a/mm/gup.c b/mm/gup.c
index cd62c8c..aec34cb 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -64,7 +64,7 @@ retry:
migration_entry_wait(mm, pmd, address);
goto retry;
}
- if ((flags & FOLL_NUMA) && pte_numa(pte))
+ if ((flags & FOLL_NUMA) && pte_protnone_numa(pte))
goto no_page;
if ((flags & FOLL_WRITE) && !pte_write(pte)) {
pte_unmap_unlock(ptep, ptl);
@@ -193,7 +193,7 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
}
return page;
}
- if ((flags & FOLL_NUMA) && pmd_numa(*pmd))
+ if ((flags & FOLL_NUMA) && pmd_protnone_numa(*pmd))
return no_page_table(vma, flags);
if (pmd_trans_huge(*pmd)) {
if (flags & FOLL_SPLIT) {
@@ -743,7 +743,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
* path
*/
if (!pte_present(pte) || pte_special(pte) ||
- pte_numa(pte) || (write && !pte_write(pte)))
+ pte_protnone_numa(pte) || (write && !pte_write(pte)))
goto pte_unmap;
VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
@@ -895,7 +895,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
* slowpath for accounting purposes and so that they
* can be serialised against THP migration.
*/
- if (pmd_numa(pmd))
+ if (pmd_protnone_numa(pmd))
return 0;
if (!gup_huge_pmd(pmd, pmdp, addr, next, write,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 38fa6cc..ec0b424 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1223,7 +1223,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
return ERR_PTR(-EFAULT);
/* Full NUMA hinting faults to serialise migration in fault paths */
- if ((flags & FOLL_NUMA) && pmd_numa(*pmd))
+ if ((flags & FOLL_NUMA) && pmd_protnone_numa(*pmd))
goto out;
page = pmd_page(*pmd);
@@ -1354,7 +1354,7 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
/*
* Migrate the THP to the requested node, returns with page unlocked
- * and pmd_numa cleared.
+ * and access rights restored.
*/
spin_unlock(ptl);
migrated = migrate_misplaced_transhuge_page(mm, vma,
@@ -1369,7 +1369,7 @@ clear_pmdnuma:
BUG_ON(!PageLocked(page));
pmd = pmd_mknonnuma(pmd);
set_pmd_at(mm, haddr, pmdp, pmd);
- VM_BUG_ON(pmd_numa(*pmdp));
+ VM_BUG_ON(pmd_protnone_numa(*pmdp));
update_mmu_cache_pmd(vma, addr, pmdp);
unlock_page(page);
out_unlock:
@@ -1514,7 +1514,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
ret = 1;
if (!prot_numa) {
entry = pmdp_get_and_clear(mm, addr, pmd);
- if (pmd_numa(entry))
+ if (pmd_protnone_numa(entry))
entry = pmd_mknonnuma(entry);
entry = pmd_modify(entry, newprot);
ret = HPAGE_PMD_NR;
@@ -1530,7 +1530,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
* local vs remote hits on the zero page.
*/
if (!is_huge_zero_page(page) &&
- !pmd_numa(*pmd)) {
+ !pmd_protnone_numa(*pmd)) {
pmdp_set_numa(mm, addr, pmd);
ret = HPAGE_PMD_NR;
}
@@ -1797,9 +1797,9 @@ static int __split_huge_page_map(struct page *page,
pte_t *pte, entry;
BUG_ON(PageCompound(page+i));
/*
- * Note that pmd_numa is not transferred deliberately
- * to avoid any possibility that pte_numa leaks to
- * a PROT_NONE VMA by accident.
+ * Note that NUMA hinting access restrictions are not
+ * transferred to avoid any possibility of altering
+ * permissions across VMAs.
*/
entry = mk_pte(page + i, vma->vm_page_prot);
entry = maybe_mkwrite(pte_mkdirty(entry), vma);
diff --git a/mm/memory.c b/mm/memory.c
index 3e50383..96ceb0a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3220,7 +3220,7 @@ static int handle_pte_fault(struct mm_struct *mm,
pte, pmd, flags, entry);
}
- if (pte_numa(entry))
+ if (pte_protnone_numa(entry))
return do_numa_page(mm, vma, address, entry, pte, pmd);
ptl = pte_lockptr(mm, pmd);
@@ -3298,7 +3298,7 @@ static int __handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
if (pmd_trans_splitting(orig_pmd))
return 0;
- if (pmd_numa(orig_pmd))
+ if (pmd_protnone_numa(orig_pmd))
return do_huge_pmd_numa_page(mm, vma, address,
orig_pmd, pmd);
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ace9345..e93ddac 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -75,36 +75,17 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
oldpte = *pte;
if (pte_present(oldpte)) {
pte_t ptent;
- bool updated = false;
-
- if (!prot_numa) {
- ptent = ptep_modify_prot_start(mm, addr, pte);
- if (pte_numa(ptent))
- ptent = pte_mknonnuma(ptent);
- ptent = pte_modify(ptent, newprot);
- /*
- * Avoid taking write faults for pages we
- * know to be dirty.
- */
- if (dirty_accountable && pte_dirty(ptent) &&
- (pte_soft_dirty(ptent) ||
- !(vma->vm_flags & VM_SOFTDIRTY)))
- ptent = pte_mkwrite(ptent);
- ptep_modify_prot_commit(mm, addr, pte, ptent);
- updated = true;
- } else {
- struct page *page;
-
- page = vm_normal_page(vma, addr, oldpte);
- if (page && !PageKsm(page)) {
- if (!pte_numa(oldpte)) {
- ptep_set_numa(mm, addr, pte);
- updated = true;
- }
- }
+ ptent = ptep_modify_prot_start(mm, addr, pte);
+ ptent = pte_modify(ptent, newprot);
+
+ /* Avoid taking write faults for known dirty pages */
+ if (dirty_accountable && pte_dirty(ptent) &&
+ (pte_soft_dirty(ptent) ||
+ !(vma->vm_flags & VM_SOFTDIRTY))) {
+ ptent = pte_mkwrite(ptent);
}
- if (updated)
- pages++;
+ ptep_modify_prot_commit(mm, addr, pte, ptent);
+ pages++;
} else if (IS_ENABLED(CONFIG_MIGRATION) && !pte_file(oldpte)) {
swp_entry_t entry = pte_to_swp_entry(oldpte);
diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index dfb79e0..a2d8587 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -193,7 +193,7 @@ void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
pmd_t entry = *pmdp;
- if (pmd_numa(entry))
+ if (pmd_protnone_numa(entry))
entry = pmd_mknonnuma(entry);
set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
--
2.1.2
^ permalink raw reply related
* [PATCH 04/10] ppc64: Add paranoid warnings for unexpected DSISR_PROTFAULT
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
ppc64 should not be depending on DSISR_PROTFAULT and it's unexpected
if they are triggered. This patch adds warnings just in case they
are being accidentally depended upon.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/mm/copro_fault.c | 8 ++++++--
arch/powerpc/mm/fault.c | 20 +++++++++-----------
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/mm/copro_fault.c b/arch/powerpc/mm/copro_fault.c
index 5a236f0..0450d68 100644
--- a/arch/powerpc/mm/copro_fault.c
+++ b/arch/powerpc/mm/copro_fault.c
@@ -64,10 +64,14 @@ int copro_handle_mm_fault(struct mm_struct *mm, unsigned long ea,
if (!(vma->vm_flags & VM_WRITE))
goto out_unlock;
} else {
- if (dsisr & DSISR_PROTFAULT)
- goto out_unlock;
if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
goto out_unlock;
+ /*
+ * protfault should only happen due to us
+ * mapping a region readonly temporarily. PROT_NONE
+ * is also covered by the VMA check above.
+ */
+ WARN_ON_ONCE(dsisr & DSISR_PROTFAULT);
}
ret = 0;
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 5007497..9d6e0b3 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -396,17 +396,6 @@ good_area:
#endif /* CONFIG_8xx */
if (is_exec) {
-#ifdef CONFIG_PPC_STD_MMU
- /* Protection fault on exec go straight to failure on
- * Hash based MMUs as they either don't support per-page
- * execute permission, or if they do, it's handled already
- * at the hash level. This test would probably have to
- * be removed if we change the way this works to make hash
- * processors use the same I/D cache coherency mechanism
- * as embedded.
- */
-#endif /* CONFIG_PPC_STD_MMU */
-
/*
* Allow execution from readable areas if the MMU does not
* provide separate controls over reading and executing.
@@ -421,6 +410,14 @@ good_area:
(cpu_has_feature(CPU_FTR_NOEXECUTE) ||
!(vma->vm_flags & (VM_READ | VM_WRITE))))
goto bad_area;
+#ifdef CONFIG_PPC_STD_MMU
+ /*
+ * protfault should only happen due to us
+ * mapping a region readonly temporarily. PROT_NONE
+ * is also covered by the VMA check above.
+ */
+ WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
+#endif /* CONFIG_PPC_STD_MMU */
/* a write */
} else if (is_write) {
if (!(vma->vm_flags & VM_WRITE))
@@ -430,6 +427,7 @@ good_area:
} else {
if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
goto bad_area;
+ WARN_ON_ONCE(error_code & DSISR_PROTFAULT);
}
/*
--
2.1.2
^ permalink raw reply related
* [PATCH 05/10] mm: Convert p[te|md]_mknonnuma and remaining page table manipulations
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
With PROT_NONE, the traditional page table manipulation functions are
sufficient.
Signed-off-by: Mel Gorman <mgorman@suse.de>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Aneesh Kumar <aneesh.kumar@linux.vnet.ibm.com>
---
include/linux/huge_mm.h | 3 +--
mm/huge_memory.c | 33 +++++++--------------------------
mm/memory.c | 10 ++++++----
mm/mempolicy.c | 2 +-
mm/migrate.c | 2 +-
mm/mprotect.c | 2 +-
mm/pgtable-generic.c | 2 --
7 files changed, 17 insertions(+), 37 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index ad9051b..554bbe3 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -31,8 +31,7 @@ extern int move_huge_pmd(struct vm_area_struct *vma,
unsigned long new_addr, unsigned long old_end,
pmd_t *old_pmd, pmd_t *new_pmd);
extern int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
- unsigned long addr, pgprot_t newprot,
- int prot_numa);
+ unsigned long addr, pgprot_t newprot);
enum transparent_hugepage_flag {
TRANSPARENT_HUGEPAGE_FLAG,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index ec0b424..668f1a3 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1367,9 +1367,8 @@ int do_huge_pmd_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
goto out;
clear_pmdnuma:
BUG_ON(!PageLocked(page));
- pmd = pmd_mknonnuma(pmd);
+ pmd = pmd_modify(pmd, vma->vm_page_prot);
set_pmd_at(mm, haddr, pmdp, pmd);
- VM_BUG_ON(pmd_protnone_numa(*pmdp));
update_mmu_cache_pmd(vma, addr, pmdp);
unlock_page(page);
out_unlock:
@@ -1503,7 +1502,7 @@ out:
* - HPAGE_PMD_NR is protections changed and TLB flush necessary
*/
int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
- unsigned long addr, pgprot_t newprot, int prot_numa)
+ unsigned long addr, pgprot_t newprot)
{
struct mm_struct *mm = vma->vm_mm;
spinlock_t *ptl;
@@ -1512,29 +1511,11 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
if (__pmd_trans_huge_lock(pmd, vma, &ptl) == 1) {
pmd_t entry;
ret = 1;
- if (!prot_numa) {
- entry = pmdp_get_and_clear(mm, addr, pmd);
- if (pmd_protnone_numa(entry))
- entry = pmd_mknonnuma(entry);
- entry = pmd_modify(entry, newprot);
- ret = HPAGE_PMD_NR;
- set_pmd_at(mm, addr, pmd, entry);
- BUG_ON(pmd_write(entry));
- } else {
- struct page *page = pmd_page(*pmd);
-
- /*
- * Do not trap faults against the zero page. The
- * read-only data is likely to be read-cached on the
- * local CPU cache and it is less useful to know about
- * local vs remote hits on the zero page.
- */
- if (!is_huge_zero_page(page) &&
- !pmd_protnone_numa(*pmd)) {
- pmdp_set_numa(mm, addr, pmd);
- ret = HPAGE_PMD_NR;
- }
- }
+ entry = pmdp_get_and_clear(mm, addr, pmd);
+ entry = pmd_modify(entry, newprot);
+ ret = HPAGE_PMD_NR;
+ set_pmd_at(mm, addr, pmd, entry);
+ BUG_ON(pmd_write(entry));
spin_unlock(ptl);
}
diff --git a/mm/memory.c b/mm/memory.c
index 96ceb0a..900127b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3120,9 +3120,9 @@ static int do_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
* validation through pte_unmap_same(). It's of NUMA type but
* the pfn may be screwed if the read is non atomic.
*
- * ptep_modify_prot_start is not called as this is clearing
- * the _PAGE_NUMA bit and it is not really expected that there
- * would be concurrent hardware modifications to the PTE.
+ * We can safely just do a "set_pte_at()", because the old
+ * page table entry is not accessible, so there would be no
+ * concurrent hardware modifications to the PTE.
*/
ptl = pte_lockptr(mm, pmd);
spin_lock(ptl);
@@ -3131,7 +3131,9 @@ static int do_numa_page(struct mm_struct *mm, struct vm_area_struct *vma,
goto out;
}
- pte = pte_mknonnuma(pte);
+ /* Make it present again */
+ pte = pte_modify(pte, vma->vm_page_prot);
+ pte = pte_mkyoung(pte);
set_pte_at(mm, addr, ptep, pte);
update_mmu_cache(vma, addr, ptep);
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e58725a..9d61dce 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -633,7 +633,7 @@ unsigned long change_prot_numa(struct vm_area_struct *vma,
{
int nr_updated;
- nr_updated = change_protection(vma, addr, end, vma->vm_page_prot, 0, 1);
+ nr_updated = change_protection(vma, addr, end, PAGE_NONE, 0, 1);
if (nr_updated)
count_vm_numa_events(NUMA_PTE_UPDATES, nr_updated);
diff --git a/mm/migrate.c b/mm/migrate.c
index baaf80e..e3282d4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1890,7 +1890,7 @@ out_fail:
out_dropref:
ptl = pmd_lock(mm, pmd);
if (pmd_same(*pmd, entry)) {
- entry = pmd_mknonnuma(entry);
+ entry = pmd_modify(entry, vma->vm_page_prot);
set_pmd_at(mm, mmun_start, pmd, entry);
update_mmu_cache_pmd(vma, address, &entry);
}
diff --git a/mm/mprotect.c b/mm/mprotect.c
index e93ddac..dc65c0f 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -141,7 +141,7 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
split_huge_page_pmd(vma, addr, pmd);
else {
int nr_ptes = change_huge_pmd(vma, pmd, addr,
- newprot, prot_numa);
+ newprot);
if (nr_ptes) {
if (nr_ptes == HPAGE_PMD_NR) {
diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index a2d8587..c25f94b 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -193,8 +193,6 @@ void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
pmd_t entry = *pmdp;
- if (pmd_protnone_numa(entry))
- entry = pmd_mknonnuma(entry);
set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
}
--
2.1.2
^ permalink raw reply related
* [PATCH 06/10] mm: Remove remaining references to NUMA hinting bits and helpers
From: Mel Gorman @ 2014-11-20 10:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Rik van Riel, Linus Torvalds, Hugh Dickins, Linux-MM, Ingo Molnar,
Paul Mackerras, Aneesh Kumar, Sasha Levin, Dave Jones,
LinuxPPC-dev, Kirill Shutemov, Mel Gorman
In-Reply-To: <1416478790-27522-1-git-send-email-mgorman@suse.de>
This patch removes the NUMA PTE bits and associated helpers. As a side-effect
it increases the maximum possible swap space on x86-64.
One potential source of problems is races between the marking of PTEs
PROT_NONE, NUMA hinting faults and migration. It must be guaranteed that
a PTE being protected is not faulted in parallel, seen as a pte_none and
corrupting memory. The base case is safe but transhuge has problems in the
past due to an different migration mechanism and a dependance on page lock
to serialise migrations and warrants a closer look.
task_work hinting update parallel fault
------------------------ --------------
change_pmd_range
change_huge_pmd
__pmd_trans_huge_lock
pmdp_get_and_clear
__handle_mm_fault
pmd_none
do_huge_pmd_anonymous_page
read? pmd_lock blocks until hinting complete, fail !pmd_none test
write? __do_huge_pmd_anonymous_page acquires pmd_lock, checks pmd_none
pmd_modify
set_pmd_at
task_work hinting update parallel migration
------------------------ ------------------
change_pmd_range
change_huge_pmd
__pmd_trans_huge_lock
pmdp_get_and_clear
__handle_mm_fault
do_huge_pmd_numa_page
migrate_misplaced_transhuge_page
pmd_lock waits for updates to complete, recheck pmd_same
pmd_modify
set_pmd_at
Both of those are safe and the case where a transhuge page is inserted
during a protection update is unchanged. The case where two processes try
migrating at the same time is unchanged by this series so should still be
ok. I could not find a case where we are accidentally depending on the
PTE not being cleared and flushed. If one is missed, it'll manifest as
corruption problems that start triggering shortly after this series is
merged and only happen when NUMA balancing is enabled.
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
arch/powerpc/include/asm/pgtable.h | 54 +-----------
arch/powerpc/include/asm/pte-common.h | 5 --
arch/powerpc/include/asm/pte-hash64.h | 6 --
arch/x86/include/asm/pgtable.h | 22 +----
arch/x86/include/asm/pgtable_64.h | 5 --
arch/x86/include/asm/pgtable_types.h | 41 +--------
include/asm-generic/pgtable.h | 155 ----------------------------------
include/linux/swapops.h | 2 +-
8 files changed, 7 insertions(+), 283 deletions(-)
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 452c3b4..2e074e7 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -49,64 +49,12 @@ static inline int pmd_protnone_numa(pmd_t pmd)
{
return pte_protnone_numa(pmd_pte(pmd));
}
-
-static inline int pte_present(pte_t pte)
-{
- return pte_val(pte) & _PAGE_NUMA_MASK;
-}
-
-#define pte_present_nonuma pte_present_nonuma
-static inline int pte_present_nonuma(pte_t pte)
-{
- return pte_val(pte) & (_PAGE_PRESENT);
-}
-
-#define ptep_set_numa ptep_set_numa
-static inline void ptep_set_numa(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep)
-{
- if ((pte_val(*ptep) & _PAGE_PRESENT) == 0)
- VM_BUG_ON(1);
-
- pte_update(mm, addr, ptep, _PAGE_PRESENT, _PAGE_NUMA, 0);
- return;
-}
-
-#define pmdp_set_numa pmdp_set_numa
-static inline void pmdp_set_numa(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp)
-{
- if ((pmd_val(*pmdp) & _PAGE_PRESENT) == 0)
- VM_BUG_ON(1);
-
- pmd_hugepage_update(mm, addr, pmdp, _PAGE_PRESENT, _PAGE_NUMA);
- return;
-}
-
-/*
- * Generic NUMA pte helpers expect pteval_t and pmdval_t types to exist
- * which was inherited from x86. For the purposes of powerpc pte_basic_t and
- * pmd_t are equivalent
- */
-#define pteval_t pte_basic_t
-#define pmdval_t pmd_t
-static inline pteval_t ptenuma_flags(pte_t pte)
-{
- return pte_val(pte) & _PAGE_NUMA_MASK;
-}
-
-static inline pmdval_t pmdnuma_flags(pmd_t pmd)
-{
- return pmd_val(pmd) & _PAGE_NUMA_MASK;
-}
-
-# else
+#endif /* CONFIG_NUMA_BALANCING */
static inline int pte_present(pte_t pte)
{
return pte_val(pte) & _PAGE_PRESENT;
}
-#endif /* CONFIG_NUMA_BALANCING */
/* Conversion functions: convert a page and protection to a page entry,
* and a page entry and page directory to the page they refer to.
diff --git a/arch/powerpc/include/asm/pte-common.h b/arch/powerpc/include/asm/pte-common.h
index e040c35..8d1569c 100644
--- a/arch/powerpc/include/asm/pte-common.h
+++ b/arch/powerpc/include/asm/pte-common.h
@@ -98,11 +98,6 @@ extern unsigned long bad_call_to_PMD_PAGE_SIZE(void);
_PAGE_USER | _PAGE_ACCESSED | \
_PAGE_RW | _PAGE_HWWRITE | _PAGE_DIRTY | _PAGE_EXEC)
-#ifdef CONFIG_NUMA_BALANCING
-/* Mask of bits that distinguish present and numa ptes */
-#define _PAGE_NUMA_MASK (_PAGE_NUMA|_PAGE_PRESENT)
-#endif
-
/*
* We define 2 sets of base prot bits, one for basic pages (ie,
* cacheable kernel and user pages) and one for non cacheable
diff --git a/arch/powerpc/include/asm/pte-hash64.h b/arch/powerpc/include/asm/pte-hash64.h
index 2505d8e..55aea0c 100644
--- a/arch/powerpc/include/asm/pte-hash64.h
+++ b/arch/powerpc/include/asm/pte-hash64.h
@@ -27,12 +27,6 @@
#define _PAGE_RW 0x0200 /* software: user write access allowed */
#define _PAGE_BUSY 0x0800 /* software: PTE & hash are busy */
-/*
- * Used for tracking numa faults
- */
-#define _PAGE_NUMA 0x00000010 /* Gather numa placement stats */
-
-
/* No separate kernel read-only */
#define _PAGE_KERNEL_RW (_PAGE_RW | _PAGE_DIRTY) /* user access blocked by key */
#define _PAGE_KERNEL_RO _PAGE_KERNEL_RW
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 613cd00..f8799e0 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -299,7 +299,7 @@ static inline pmd_t pmd_mkwrite(pmd_t pmd)
static inline pmd_t pmd_mknotpresent(pmd_t pmd)
{
- return pmd_clear_flags(pmd, _PAGE_PRESENT);
+ return pmd_clear_flags(pmd, _PAGE_PRESENT | _PAGE_PROTNONE);
}
#ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
@@ -457,13 +457,6 @@ static inline int pte_same(pte_t a, pte_t b)
static inline int pte_present(pte_t a)
{
- return pte_flags(a) & (_PAGE_PRESENT | _PAGE_PROTNONE |
- _PAGE_NUMA);
-}
-
-#define pte_present_nonuma pte_present_nonuma
-static inline int pte_present_nonuma(pte_t a)
-{
return pte_flags(a) & (_PAGE_PRESENT | _PAGE_PROTNONE);
}
@@ -473,7 +466,7 @@ static inline bool pte_accessible(struct mm_struct *mm, pte_t a)
if (pte_flags(a) & _PAGE_PRESENT)
return true;
- if ((pte_flags(a) & (_PAGE_PROTNONE | _PAGE_NUMA)) &&
+ if ((pte_flags(a) & _PAGE_PROTNONE) &&
mm_tlb_flush_pending(mm))
return true;
@@ -493,8 +486,7 @@ static inline int pmd_present(pmd_t pmd)
* the _PAGE_PSE flag will remain set at all times while the
* _PAGE_PRESENT bit is clear).
*/
- return pmd_flags(pmd) & (_PAGE_PRESENT | _PAGE_PROTNONE | _PAGE_PSE |
- _PAGE_NUMA);
+ return pmd_flags(pmd) & (_PAGE_PRESENT | _PAGE_PROTNONE | _PAGE_PSE);
}
#ifdef CONFIG_NUMA_BALANCING
@@ -569,11 +561,6 @@ static inline pte_t *pte_offset_kernel(pmd_t *pmd, unsigned long address)
static inline int pmd_bad(pmd_t pmd)
{
-#ifdef CONFIG_NUMA_BALANCING
- /* pmd_numa check */
- if ((pmd_flags(pmd) & (_PAGE_NUMA|_PAGE_PRESENT)) == _PAGE_NUMA)
- return 0;
-#endif
return (pmd_flags(pmd) & ~_PAGE_USER) != _KERNPG_TABLE;
}
@@ -892,19 +879,16 @@ static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
#ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
static inline pte_t pte_swp_mksoft_dirty(pte_t pte)
{
- VM_BUG_ON(pte_present_nonuma(pte));
return pte_set_flags(pte, _PAGE_SWP_SOFT_DIRTY);
}
static inline int pte_swp_soft_dirty(pte_t pte)
{
- VM_BUG_ON(pte_present_nonuma(pte));
return pte_flags(pte) & _PAGE_SWP_SOFT_DIRTY;
}
static inline pte_t pte_swp_clear_soft_dirty(pte_t pte)
{
- VM_BUG_ON(pte_present_nonuma(pte));
return pte_clear_flags(pte, _PAGE_SWP_SOFT_DIRTY);
}
#endif
diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
index 4572b2f..06ffca8 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -146,12 +146,7 @@ static inline int pgd_large(pgd_t pgd) { return 0; }
/* Encode and de-code a swap entry */
#define SWP_TYPE_BITS (_PAGE_BIT_FILE - _PAGE_BIT_PRESENT - 1)
-#ifdef CONFIG_NUMA_BALANCING
-/* Automatic NUMA balancing needs to be distinguishable from swap entries */
-#define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 2)
-#else
#define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 1)
-#endif
#define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 0778964..d299cdd 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -27,14 +27,6 @@
#define _PAGE_BIT_SOFT_DIRTY _PAGE_BIT_SOFTW3 /* software dirty tracking */
#define _PAGE_BIT_NX 63 /* No execute: only valid after cpuid check */
-/*
- * Swap offsets on configurations that allow automatic NUMA balancing use the
- * bits after _PAGE_BIT_GLOBAL. To uniquely distinguish NUMA hinting PTEs from
- * swap entries, we use the first bit after _PAGE_BIT_GLOBAL and shrink the
- * maximum possible swap space from 16TB to 8TB.
- */
-#define _PAGE_BIT_NUMA (_PAGE_BIT_GLOBAL+1)
-
/* If _PAGE_BIT_PRESENT is clear, we use these: */
/* - if the user mapped it with PROT_NONE; pte_present gives true */
#define _PAGE_BIT_PROTNONE _PAGE_BIT_GLOBAL
@@ -78,21 +70,6 @@
#endif
/*
- * _PAGE_NUMA distinguishes between a numa hinting minor fault and a page
- * that is not present. The hinting fault gathers numa placement statistics
- * (see pte_numa()). The bit is always zero when the PTE is not present.
- *
- * The bit picked must be always zero when the pmd is present and not
- * present, so that we don't lose information when we set it while
- * atomically clearing the present bit.
- */
-#ifdef CONFIG_NUMA_BALANCING
-#define _PAGE_NUMA (_AT(pteval_t, 1) << _PAGE_BIT_NUMA)
-#else
-#define _PAGE_NUMA (_AT(pteval_t, 0))
-#endif
-
-/*
* Tracking soft dirty bit when a page goes to a swap is tricky.
* We need a bit which can be stored in pte _and_ not conflict
* with swap entry format. On x86 bits 6 and 7 are *not* involved
@@ -125,8 +102,8 @@
/* Set of bits not changed in pte_modify */
#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \
_PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY | \
- _PAGE_SOFT_DIRTY | _PAGE_NUMA)
-#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE | _PAGE_NUMA)
+ _PAGE_SOFT_DIRTY)
+#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
#define _PAGE_CACHE_MASK (_PAGE_PCD | _PAGE_PWT)
#define _PAGE_CACHE_WB (0)
@@ -324,20 +301,6 @@ static inline pteval_t pte_flags(pte_t pte)
return native_pte_val(pte) & PTE_FLAGS_MASK;
}
-#ifdef CONFIG_NUMA_BALANCING
-/* Set of bits that distinguishes present, prot_none and numa ptes */
-#define _PAGE_NUMA_MASK (_PAGE_NUMA|_PAGE_PROTNONE|_PAGE_PRESENT)
-static inline pteval_t ptenuma_flags(pte_t pte)
-{
- return pte_flags(pte) & _PAGE_NUMA_MASK;
-}
-
-static inline pmdval_t pmdnuma_flags(pmd_t pmd)
-{
- return pmd_flags(pmd) & _PAGE_NUMA_MASK;
-}
-#endif /* CONFIG_NUMA_BALANCING */
-
#define pgprot_val(x) ((x).pgprot)
#define __pgprot(x) ((pgprot_t) { (x) } )
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 7e74122..323e914 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -233,10 +233,6 @@ static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b)
# define pte_accessible(mm, pte) ((void)(pte), 1)
#endif
-#ifndef pte_present_nonuma
-#define pte_present_nonuma(pte) pte_present(pte)
-#endif
-
#ifndef flush_tlb_fix_spurious_fault
#define flush_tlb_fix_spurious_fault(vma, address) flush_tlb_page(vma, address)
#endif
@@ -696,157 +692,6 @@ static inline int pmd_protnone_numa(pmd_t pmd)
}
#endif /* CONFIG_NUMA_BALANCING */
-#ifdef CONFIG_NUMA_BALANCING
-/*
- * _PAGE_NUMA distinguishes between an unmapped page table entry, an entry that
- * is protected for PROT_NONE and a NUMA hinting fault entry. If the
- * architecture defines __PAGE_PROTNONE then it should take that into account
- * but those that do not can rely on the fact that the NUMA hinting scanner
- * skips inaccessible VMAs.
- *
- * pte/pmd_present() returns true if pte/pmd_numa returns true. Page
- * fault triggers on those regions if pte/pmd_numa returns true
- * (because _PAGE_PRESENT is not set).
- */
-#ifndef pte_numa
-static inline int pte_numa(pte_t pte)
-{
- return ptenuma_flags(pte) == _PAGE_NUMA;
-}
-#endif
-
-#ifndef pmd_numa
-static inline int pmd_numa(pmd_t pmd)
-{
- return pmdnuma_flags(pmd) == _PAGE_NUMA;
-}
-#endif
-
-/*
- * pte/pmd_mknuma sets the _PAGE_ACCESSED bitflag automatically
- * because they're called by the NUMA hinting minor page fault. If we
- * wouldn't set the _PAGE_ACCESSED bitflag here, the TLB miss handler
- * would be forced to set it later while filling the TLB after we
- * return to userland. That would trigger a second write to memory
- * that we optimize away by setting _PAGE_ACCESSED here.
- */
-#ifndef pte_mknonnuma
-static inline pte_t pte_mknonnuma(pte_t pte)
-{
- pteval_t val = pte_val(pte);
-
- val &= ~_PAGE_NUMA;
- val |= (_PAGE_PRESENT|_PAGE_ACCESSED);
- return __pte(val);
-}
-#endif
-
-#ifndef pmd_mknonnuma
-static inline pmd_t pmd_mknonnuma(pmd_t pmd)
-{
- pmdval_t val = pmd_val(pmd);
-
- val &= ~_PAGE_NUMA;
- val |= (_PAGE_PRESENT|_PAGE_ACCESSED);
-
- return __pmd(val);
-}
-#endif
-
-#ifndef pte_mknuma
-static inline pte_t pte_mknuma(pte_t pte)
-{
- pteval_t val = pte_val(pte);
-
- VM_BUG_ON(!(val & _PAGE_PRESENT));
-
- val &= ~_PAGE_PRESENT;
- val |= _PAGE_NUMA;
-
- return __pte(val);
-}
-#endif
-
-#ifndef ptep_set_numa
-static inline void ptep_set_numa(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep)
-{
- pte_t ptent = *ptep;
-
- ptent = pte_mknuma(ptent);
- set_pte_at(mm, addr, ptep, ptent);
- return;
-}
-#endif
-
-#ifndef pmd_mknuma
-static inline pmd_t pmd_mknuma(pmd_t pmd)
-{
- pmdval_t val = pmd_val(pmd);
-
- val &= ~_PAGE_PRESENT;
- val |= _PAGE_NUMA;
-
- return __pmd(val);
-}
-#endif
-
-#ifndef pmdp_set_numa
-static inline void pmdp_set_numa(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp)
-{
- pmd_t pmd = *pmdp;
-
- pmd = pmd_mknuma(pmd);
- set_pmd_at(mm, addr, pmdp, pmd);
- return;
-}
-#endif
-#else
-static inline int pmd_numa(pmd_t pmd)
-{
- return 0;
-}
-
-static inline int pte_numa(pte_t pte)
-{
- return 0;
-}
-
-static inline pte_t pte_mknonnuma(pte_t pte)
-{
- return pte;
-}
-
-static inline pmd_t pmd_mknonnuma(pmd_t pmd)
-{
- return pmd;
-}
-
-static inline pte_t pte_mknuma(pte_t pte)
-{
- return pte;
-}
-
-static inline void ptep_set_numa(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep)
-{
- return;
-}
-
-
-static inline pmd_t pmd_mknuma(pmd_t pmd)
-{
- return pmd;
-}
-
-static inline void pmdp_set_numa(struct mm_struct *mm, unsigned long addr,
- pmd_t *pmdp)
-{
- return ;
-}
-#endif /* CONFIG_NUMA_BALANCING */
-
#endif /* CONFIG_MMU */
#endif /* !__ASSEMBLY__ */
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 6adfb7b..2b1fa56 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -54,7 +54,7 @@ static inline pgoff_t swp_offset(swp_entry_t entry)
/* check whether a pte points to a swap entry */
static inline int is_swap_pte(pte_t pte)
{
- return !pte_none(pte) && !pte_present_nonuma(pte) && !pte_file(pte);
+ return !pte_none(pte) && !pte_file(pte);
}
#endif
--
2.1.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox