Re: [PATCH v2 2/4] powerpc/sstep: support emulation for vsx vector paired storage access instructions
Hi Bala,
@@ -709,6 +722,8 @@ void emulate_vsx_load(struct instruction_op *op, union
vsx_reg *reg,
reg->d[0] = reg->d[1] = 0;
switch (op->element_size) {
+ case 32:
+ /* [p]lxvp[x] or [p]stxvp[x] */
This function does not emulate stvxp
case 16:
/* whole vector; lxv[x] or lxvl[l] */
if (size == 0)
@@ -717,7 +732,7 @@ void emulate_vsx_load(struct instruction_op *op, union
vsx_reg *reg,
if (IS_LE && (op->vsx_flags & VSX_LDLEFT))
rev = !rev;
if (rev)
- do_byte_reverse(reg, 16);
+ do_byte_reverse(reg, size);
break;
case 8:
/* scalar loads, lxvd2x, lxvdsx */
@@ -793,6 +808,22 @@ void emulate_vsx_store(struct instruction_op *op, const
union vsx_reg *reg,
size = GETSIZE(op->type);
switch (op->element_size) {
+ case 32:
+ /* [p]lxvp[x] or [p]stxvp[x] */
This function does not emulate lxvp
+ if (size == 0)
+ break;
+ if (IS_LE && (op->vsx_flags & VSX_LDLEFT))
+ rev = !rev;
Why this if condition ?
+ if (rev) {
+ /* reverse 32 bytes */
+ buf.d[0] = byterev_8(reg->d[3]);
+ buf.d[1] = byterev_8(reg->d[2]);
+ buf.d[2] = byterev_8(reg->d[1]);
+ buf.d[3] = byterev_8(reg->d[0]);
+ reg = &buf;
+ }
+ memcpy(mem, reg, size);
+ break;
case 16:
/* stxv, stxvx, stxvl, stxvll */
if (size == 0)
@@ -861,28 +892,33 @@ static nokprobe_inline int do_vsx_load(struct
instruction_op *op,
bool cross_endian)
{
int reg = op->reg;
- u8 mem[16];
+ int i, nr_vsx_regs;
+ u8 mem[32];
union vsx_reg buf;
int size = GETSIZE(op->type);
if (!address_ok(regs, ea, size) || copy_mem_in(mem, ea, size, regs))
return -EFAULT;
+ nr_vsx_regs = size / sizeof(__vector128);
emulate_vsx_load(op, &buf, mem, cross_endian);
preempt_disable();
if (reg < 32) {
/* FP regs + extensions */
if (regs->msr & MSR_FP) {
- load_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ load_vsrn(reg + i, &buf.v[i]);
} else {
current->thread.fp_state.fpr[reg][0] = buf.d[0];
current->thread.fp_state.fpr[reg][1] = buf.d[1];
Should we change else part as well?
}
} else {
if (regs->msr & MSR_VEC)
- load_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ load_vsrn(reg + i, &buf.v[i]);
+
Unnecessary line.
else
- current->thread.vr_state.vr[reg - 32] = buf.v;
+ current->thread.vr_state.vr[reg - 32] = buf.v[0];
Same here. else part, should we add:
if (vsx 32 byte)
current->thread.vr_state.vr[reg - 32 + 1] = buf.v[1];
}
preempt_enable();
return 0;
@@ -893,27 +929,31 @@ static nokprobe_inline int do_vsx_store(struct
instruction_op *op,
bool cross_endian)
{
int reg = op->reg;
- u8 mem[16];
+ int i, nr_vsx_regs;
+ u8 mem[32];
union vsx_reg buf;
int size = GETSIZE(op->type);
if (!address_ok(regs, ea, size))
return -EFAULT;
+ nr_vsx_regs = size / sizeof(__vector128);
preempt_disable();
if (reg < 32) {
/* FP regs + extensions */
if (regs->msr & MSR_FP) {
- store_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ store_vsrn(reg + i, &buf.v[i]);
} else {
buf.d[0] = current->thread.fp_state.fpr[reg][0];
buf.d[1] = current->thread.fp_state.fpr[reg][1];
}
} else {
if (regs->msr & MSR_VEC)
- store_vsrn(reg, &buf);
+ for (i = 0; i < nr_vsx_regs; i++)
+ store_vsrn(reg + i, &buf.v[i]);
else
- buf.v = current->thread.vr_state.vr[reg - 32];
+ buf.v[0] = current->thread.vr_state.vr[reg - 32];
}
preempt_enable();
emulate_vsx_store(op, &buf, mem, cross_endian);
Ravi
[PATCH v4 0/7] Remove default DMA window before creating DDW
There are some devices in which a hypervisor may only allow 1 DMA window to exist at a time, and in those cases, a DDW is never created to them, since the default DMA window keeps using this resource. LoPAR recommends this procedure: 1. Remove the default DMA window, 2. Query for which configs the DDW can be created, 3. Create a DDW. Patch #1: Create defines for outputs of ibm,ddw-applicable, so it's easier to identify them. Patch #2: - After LoPAR level 2.8, there is an extension that can make ibm,query-pe-dma-windows to have 6 outputs instead of 5. This changes the order of the outputs, and that can cause some trouble. - query_ddw() was updated to check how many outputs the ibm,query-pe-dma-windows is supposed to have, update the rtas_call() and deal correctly with the outputs in both cases. - This patch looks somehow unrelated to the series, but it can avoid future problems on DDW creation. Patch #3 moves the window-removing code from remove_ddw() to remove_dma_window(), creating a way to delete any DMA window, so it can be used to delete the default DMA window. Patch #4 makes use of the remove_dma_window() from patch #3 to remove the default DMA window before query_ddw(). It also implements a new rtas call to recover the default DMA window, in case anything fails after it was removed, and a DDW couldn't be created. Patch #5 moves the part of iommu_table_free() that does struct iommu_table cleaning into iommu_table_clean, so we can invoke it separately in patch #6. Patch #6: Instead of destroying the created DDW if it doesn't map the whole partition, make use of it instead of the default DMA window as it improves performance. Also, update the iommu_table and re-generate the pools. Patch #7: Does some renaming of 'direct window' to 'dma window', given the DDW created can now be also used in indirect mapping if direct mapping is not available. All patches were tested into an LPAR with an Ethernet VF: 4005:01:00.0 Ethernet controller: Mellanox Technologies MT27700 Family [ConnectX-4 Virtual Function] Patch #6 It was tested with a 64GB DDW which did not map the whole partition (128G). Performance improvement noticed by using the DDW instead of the default DMA window: 64 thread write throughput: +203.0% 64 thread read throughput: +17.5% 1 thread write throughput: +20.5% 1 thread read throughput: +3.43% Average write latency: -23.0% Average read latency: -2.26% --- Changes since v3: - Introduces new patch #5, to prepare for an important change in #6 - struct iommu_table was not being updated, so include a way to do this in patch #6. - Improved patch #4 based in a suggestion from Alexey, to make code more easily understandable - v3 link: http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=187348&state=%2A&archive=both Changes since v2: - Change the way ibm,ddw-extensions is accessed, using a proper function instead of doing this inline everytime it's used. - Remove previous patch #6, as it doesn't look like it would be useful. - Add new patch, for changing names from direct* to dma*, as indirect mapping can be used from now on. - Fix some typos, corrects some define usage. - v2 link: http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=185433&state=%2A&archive=both Changes since v1: - Add defines for ibm,ddw-applicable and ibm,ddw-extensions outputs - Merge aux function query_ddw_out_sz() into query_ddw() - Merge reset_dma_window() patch (prev. #2) into remove default DMA window patch (#4). - Keep device_node *np name instead of using pdn in remove_*() - Rename 'device_node *pdn' into 'parent' in new functions - Rename dfl_win to default_win - Only remove the default DMA window if there is no window available in first query. - Check if default DMA window can be restored before removing it. - Fix 'unitialized use' (found by travis mpe:ci-test) - New patches #5 and #6 - v1 link: http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=184420&state=%2A&archive=both Special thanks for Alexey Kardashevskiy, Brian King and Oliver O'Halloran for the feedback provided! Leonardo Bras (7): powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window powerpc/pseries/iommu: Remove default DMA window before creating DDW powerpc/iommu: Move iommu_table cleaning routine to iommu_table_clean powerpc/pseries/iommu: Make use of DDW even if it does not map the partition powerpc/pseries/iommu: Rename "direct window" to "dma window" arch/powerpc/include/asm/iommu.h | 3 + arch/powerpc/kernel/iommu.c| 45 ++- arch/powerpc/platforms/pseries/iommu.c | 380 ++--- 3 files changed, 313 insertions(+), 115 deletions(-) -- 2.25.4
[PATCH v4 1/7] powerpc/pseries/iommu: Create defines for operations in ibm, ddw-applicable
Create defines to help handling ibm,ddw-applicable values, avoiding
confusion about the index of given operations.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 43 --
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 6d47b4a3ce39..ac0d6376bdad 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -39,6 +39,14 @@
#include "pseries.h"
+enum {
+ DDW_QUERY_PE_DMA_WIN = 0,
+ DDW_CREATE_PE_DMA_WIN = 1,
+ DDW_REMOVE_PE_DMA_WIN = 2,
+
+ DDW_APPLICABLE_SIZE
+};
+
static struct iommu_table_group *iommu_pseries_alloc_group(int node)
{
struct iommu_table_group *table_group;
@@ -771,12 +779,12 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
{
struct dynamic_dma_window_prop *dwp;
struct property *win64;
- u32 ddw_avail[3];
+ u32 ddw_avail[DDW_APPLICABLE_SIZE];
u64 liobn;
int ret = 0;
ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
-&ddw_avail[0], 3);
+&ddw_avail[0], DDW_APPLICABLE_SIZE);
win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
if (!win64)
@@ -798,15 +806,15 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
pr_debug("%pOF successfully cleared tces in window.\n",
np);
- ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+ ret = rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN], 1, 1, NULL, liobn);
if (ret)
pr_warn("%pOF: failed to remove direct window: rtas returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
- np, ret, ddw_avail[2], liobn);
+ np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
else
pr_debug("%pOF: successfully removed direct window: rtas
returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
- np, ret, ddw_avail[2], liobn);
+ np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
delprop:
if (remove_prop)
@@ -889,11 +897,11 @@ static int query_ddw(struct pci_dev *dev, const u32
*ddw_avail,
buid = pdn->phb->buid;
cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
- ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
- cfg_addr, BUID_HI(buid), BUID_LO(buid));
+ ret = rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN], 3, 5, (u32 *)query,
+ cfg_addr, BUID_HI(buid), BUID_LO(buid));
dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
- " returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),
- BUID_LO(buid), ret);
+ " returned %d\n", ddw_avail[DDW_QUERY_PE_DMA_WIN], cfg_addr,
+BUID_HI(buid), BUID_LO(buid), ret);
return ret;
}
@@ -920,15 +928,16 @@ static int create_ddw(struct pci_dev *dev, const u32
*ddw_avail,
do {
/* extra outputs are LIOBN and dma-addr (hi, lo) */
- ret = rtas_call(ddw_avail[1], 5, 4, (u32 *)create,
- cfg_addr, BUID_HI(buid), BUID_LO(buid),
- page_shift, window_shift);
+ ret = rtas_call(ddw_avail[DDW_CREATE_PE_DMA_WIN], 5, 4,
+ (u32 *)create, cfg_addr, BUID_HI(buid),
+ BUID_LO(buid), page_shift, window_shift);
} while (rtas_busy_delay(ret));
dev_info(&dev->dev,
"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "
- "(liobn = 0x%x starting addr = %x %x)\n", ddw_avail[1],
-cfg_addr, BUID_HI(buid), BUID_LO(buid), page_shift,
-window_shift, ret, create->liobn, create->addr_hi,
create->addr_lo);
+ "(liobn = 0x%x starting addr = %x %x)\n",
+ddw_avail[DDW_CREATE_PE_DMA_WIN], cfg_addr, BUID_HI(buid),
+BUID_LO(buid), page_shift, window_shift, ret, create->liobn,
+create->addr_hi, create->addr_lo);
return ret;
}
@@ -996,7 +1005,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
int page_shift;
u64 dma_addr, max_addr;
struct device_node *dn;
- u32 ddw_avail[3];
+ u32 ddw_avail[DDW_APPLICABLE_SIZE];
struct direct_window *window;
struct property *win64;
struct dynamic_dma_window_prop *ddwprop;
@@ -1029,7 +1038,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
* the property is actually in the parent, not the PE
*/
ret = of_property_read_u32_array(pdn, "ibm,ddw-applicable",
-
[PATCH v4 2/7] powerpc/pseries/iommu: Update call to ibm, query-pe-dma-windows
>From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can make the number of
outputs from "ibm,query-pe-dma-windows" go from 5 to 6.
This change of output size is meant to expand the address size of
largest_available_block PE TCE from 32-bit to 64-bit, which ends up
shifting page_size and migration_capable.
This ends up requiring the update of
ddw_query_response->largest_available_block from u32 to u64, and manually
assigning the values from the buffer into this struct, according to
output size.
Also, a routine was created for helping reading the ddw extensions as
suggested by LoPAR: First reading the size of the extension array from
index 0, checking if the property exists, and then returning it's value.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 91 +++---
1 file changed, 81 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index ac0d6376bdad..1a933c4e8bba 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -47,6 +47,12 @@ enum {
DDW_APPLICABLE_SIZE
};
+enum {
+ DDW_EXT_SIZE = 0,
+ DDW_EXT_RESET_DMA_WIN = 1,
+ DDW_EXT_QUERY_OUT_SIZE = 2
+};
+
static struct iommu_table_group *iommu_pseries_alloc_group(int node)
{
struct iommu_table_group *table_group;
@@ -342,7 +348,7 @@ struct direct_window {
/* Dynamic DMA Window support */
struct ddw_query_response {
u32 windows_available;
- u32 largest_available_block;
+ u64 largest_available_block;
u32 page_size;
u32 migration_capable;
};
@@ -877,14 +883,62 @@ static int find_existing_ddw_windows(void)
}
machine_arch_initcall(pseries, find_existing_ddw_windows);
+/**
+ * ddw_read_ext - Get the value of an DDW extension
+ * @np:device node from which the extension value is to be
read.
+ * @extnum:index number of the extension.
+ * @value: pointer to return value, modified when extension is available.
+ *
+ * Checks if "ibm,ddw-extensions" exists for this node, and get the value
+ * on index 'extnum'.
+ * It can be used only to check if a property exists, passing value == NULL.
+ *
+ * Returns:
+ * 0 if extension successfully read
+ * -EINVAL if the "ibm,ddw-extensions" does not exist,
+ * -ENODATA if "ibm,ddw-extensions" does not have a value, and
+ * -EOVERFLOW if "ibm,ddw-extensions" does not contain this extension.
+ */
+static inline int ddw_read_ext(const struct device_node *np, int extnum,
+ u32 *value)
+{
+ static const char propname[] = "ibm,ddw-extensions";
+ u32 count;
+ int ret;
+
+ ret = of_property_read_u32_index(np, propname, DDW_EXT_SIZE, &count);
+ if (ret)
+ return ret;
+
+ if (count < extnum)
+ return -EOVERFLOW;
+
+ if (!value)
+ value = &count;
+
+ return of_property_read_u32_index(np, propname, extnum, value);
+}
+
static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
- struct ddw_query_response *query)
+struct ddw_query_response *query,
+struct device_node *parent)
{
struct device_node *dn;
struct pci_dn *pdn;
- u32 cfg_addr;
+ u32 cfg_addr, ext_query, query_out[5];
u64 buid;
- int ret;
+ int ret, out_sz;
+
+ /*
+* From LoPAR level 2.8, "ibm,ddw-extensions" index 3 can rule how many
+* output parameters ibm,query-pe-dma-windows will have, ranging from
+* 5 to 6.
+*/
+ ret = ddw_read_ext(parent, DDW_EXT_QUERY_OUT_SIZE, &ext_query);
+ if (!ret && ext_query == 1)
+ out_sz = 6;
+ else
+ out_sz = 5;
/*
* Get the config address and phb buid of the PE window.
@@ -897,11 +951,28 @@ static int query_ddw(struct pci_dev *dev, const u32
*ddw_avail,
buid = pdn->phb->buid;
cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
- ret = rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN], 3, 5, (u32 *)query,
+ ret = rtas_call(ddw_avail[DDW_QUERY_PE_DMA_WIN], 3, out_sz, query_out,
cfg_addr, BUID_HI(buid), BUID_LO(buid));
- dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
- " returned %d\n", ddw_avail[DDW_QUERY_PE_DMA_WIN], cfg_addr,
-BUID_HI(buid), BUID_LO(buid), ret);
+ dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x returned
%d\n",
+ddw_avail[DDW_QUERY_PE_DMA_WIN], cfg_addr, BUID_HI(buid),
+BUID_LO(buid), ret);
+
+ switch (out_sz) {
+ case 5:
+ query->windows_available = query_out[0];
+ query->largest_available_block = query_out[1];
+ query->page_size = query_out[2];
+ query->migration_capable = query_out[3];
+
[PATCH v4 3/7] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window
Move the window-removing part of remove_ddw into a new function
(remove_dma_window), so it can be used to remove other DMA windows.
It's useful for removing DMA windows that don't create DIRECT64_PROPNAME
property, like the default DMA window from the device, which uses
"ibm,dma-window".
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 45 +++---
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 1a933c4e8bba..4e33147825cc 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -781,25 +781,14 @@ static int __init disable_ddw_setup(char *str)
early_param("disable_ddw", disable_ddw_setup);
-static void remove_ddw(struct device_node *np, bool remove_prop)
+static void remove_dma_window(struct device_node *np, u32 *ddw_avail,
+ struct property *win)
{
struct dynamic_dma_window_prop *dwp;
- struct property *win64;
- u32 ddw_avail[DDW_APPLICABLE_SIZE];
u64 liobn;
- int ret = 0;
-
- ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
-&ddw_avail[0], DDW_APPLICABLE_SIZE);
-
- win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
- if (!win64)
- return;
-
- if (ret || win64->length < sizeof(*dwp))
- goto delprop;
+ int ret;
- dwp = win64->value;
+ dwp = win->value;
liobn = (u64)be32_to_cpu(dwp->liobn);
/* clear the whole window, note the arg is in kernel pages */
@@ -821,10 +810,30 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
pr_debug("%pOF: successfully removed direct window: rtas
returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
+}
+
+static void remove_ddw(struct device_node *np, bool remove_prop)
+{
+ struct property *win;
+ u32 ddw_avail[DDW_APPLICABLE_SIZE];
+ int ret = 0;
+
+ ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
+&ddw_avail[0], DDW_APPLICABLE_SIZE);
+ if (ret)
+ return;
+
+ win = of_find_property(np, DIRECT64_PROPNAME, NULL);
+ if (!win)
+ return;
+
+ if (win->length >= sizeof(struct dynamic_dma_window_prop))
+ remove_dma_window(np, ddw_avail, win);
+
+ if (!remove_prop)
+ return;
-delprop:
- if (remove_prop)
- ret = of_remove_property(np, win64);
+ ret = of_remove_property(np, win);
if (ret)
pr_warn("%pOF: failed to remove direct window property: %d\n",
np, ret);
--
2.25.4
[PATCH v4 4/7] powerpc/pseries/iommu: Remove default DMA window before creating DDW
On LoPAR "DMA Window Manipulation Calls", it's recommended to remove the
default DMA window for the device, before attempting to configure a DDW,
in order to make the maximum resources available for the next DDW to be
created.
This is a requirement for using DDW on devices in which hypervisor
allows only one DMA window.
If setting up a new DDW fails anywhere after the removal of this
default DMA window, it's needed to restore the default DMA window.
For this, an implementation of ibm,reset-pe-dma-windows rtas call is
needed:
Platforms supporting the DDW option starting with LoPAR level 2.7 implement
ibm,ddw-extensions. The first extension available (index 2) carries the
token for ibm,reset-pe-dma-windows rtas call, which is used to restore
the default DMA window for a device, if it has been deleted.
It does so by resetting the TCE table allocation for the PE to it's
boot time value, available in "ibm,dma-window" device tree node.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 73 +++---
1 file changed, 66 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 4e33147825cc..fc8d0555e2e9 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1066,6 +1066,38 @@ static phys_addr_t ddw_memory_hotplug_max(void)
return max_addr;
}
+/*
+ * Platforms supporting the DDW option starting with LoPAR level 2.7 implement
+ * ibm,ddw-extensions, which carries the rtas token for
+ * ibm,reset-pe-dma-windows.
+ * That rtas-call can be used to restore the default DMA window for the device.
+ */
+static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
+{
+ int ret;
+ u32 cfg_addr, reset_dma_win;
+ u64 buid;
+ struct device_node *dn;
+ struct pci_dn *pdn;
+
+ ret = ddw_read_ext(par_dn, DDW_EXT_RESET_DMA_WIN, &reset_dma_win);
+ if (ret)
+ return;
+
+ dn = pci_device_to_OF_node(dev);
+ pdn = PCI_DN(dn);
+ buid = pdn->phb->buid;
+ cfg_addr = ((pdn->busno << 16) | (pdn->devfn << 8));
+
+ ret = rtas_call(reset_dma_win, 3, 1, NULL, cfg_addr, BUID_HI(buid),
+ BUID_LO(buid));
+ if (ret)
+ dev_info(&dev->dev,
+"ibm,reset-pe-dma-windows(%x) %x %x %x returned %d ",
+reset_dma_win, cfg_addr, BUID_HI(buid), BUID_LO(buid),
+ret);
+}
+
/*
* If the PE supports dynamic dma windows, and there is space for a table
* that can map all pages in a linear offset, then setup such a table,
@@ -1090,6 +1122,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
struct property *win64;
struct dynamic_dma_window_prop *ddwprop;
struct failed_ddw_pdn *fpdn;
+ bool default_win_removed = false;
mutex_lock(&direct_window_init_mutex);
@@ -1133,14 +1166,38 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
if (ret != 0)
goto out_failed;
+ /*
+* If there is no window available, remove the default DMA window,
+* if it's present. This will make all the resources available to the
+* new DDW window.
+* If anything fails after this, we need to restore it, so also check
+* for extensions presence.
+*/
if (query.windows_available == 0) {
- /*
-* no additional windows are available for this device.
-* We might be able to reallocate the existing window,
-* trading in for a larger page size.
-*/
- dev_dbg(&dev->dev, "no free dynamic windows");
- goto out_failed;
+ struct property *default_win;
+ int reset_win_ext;
+
+ default_win = of_find_property(pdn, "ibm,dma-window", NULL);
+ if (!default_win)
+ goto out_failed;
+
+ reset_win_ext = ddw_read_ext(pdn, DDW_EXT_RESET_DMA_WIN, NULL);
+ if (reset_win_ext)
+ goto out_failed;
+
+ remove_dma_window(pdn, ddw_avail, default_win);
+ default_win_removed = true;
+
+ /* Query again, to check if the window is available */
+ ret = query_ddw(dev, ddw_avail, &query, pdn);
+ if (ret != 0)
+ goto out_failed;
+
+ if (query.windows_available == 0) {
+ /* no windows are available for this device. */
+ dev_dbg(&dev->dev, "no free dynamic windows");
+ goto out_failed;
+ }
}
if (query.page_size & 4) {
page_shift = 24; /* 16MB */
@@ -1231,6 +1288,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
kfr
[PATCH v4 5/7] powerpc/iommu: Move iommu_table cleaning routine to iommu_table_clean
Move the part of iommu_table_free() that does struct iommu_table cleaning
into iommu_table_clean, so we can invoke it separately.
This new function is useful for cleaning struct iommu_table before
initializing it again with a new DMA window, without having it freed and
allocated again.
Signed-off-by: Leonardo Bras
---
arch/powerpc/kernel/iommu.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 9704f3f76e63..c3242253a4e7 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -735,21 +735,10 @@ struct iommu_table *iommu_init_table(struct iommu_table
*tbl, int nid,
return tbl;
}
-static void iommu_table_free(struct kref *kref)
+static void iommu_table_clean(struct iommu_table *tbl)
{
unsigned long bitmap_sz;
unsigned int order;
- struct iommu_table *tbl;
-
- tbl = container_of(kref, struct iommu_table, it_kref);
-
- if (tbl->it_ops->free)
- tbl->it_ops->free(tbl);
-
- if (!tbl->it_map) {
- kfree(tbl);
- return;
- }
iommu_table_release_pages(tbl);
@@ -763,6 +752,23 @@ static void iommu_table_free(struct kref *kref)
/* free bitmap */
order = get_order(bitmap_sz);
free_pages((unsigned long) tbl->it_map, order);
+}
+
+static void iommu_table_free(struct kref *kref)
+{
+ struct iommu_table *tbl;
+
+ tbl = container_of(kref, struct iommu_table, it_kref);
+
+ if (tbl->it_ops->free)
+ tbl->it_ops->free(tbl);
+
+ if (!tbl->it_map) {
+ kfree(tbl);
+ return;
+ }
+
+ iommu_table_clean(tbl);
/* free table */
kfree(tbl);
--
2.25.4
[PATCH v4 6/7] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition
As of today, if the biggest DDW that can be created can't map the whole
partition, it's creation is skipped and the default DMA window
"ibm,dma-window" is used instead.
Usually this DDW is bigger than the default DMA window, and it performs
better, so it would be nice to use it instead.
The DDW created will be used for direct mapping by default.
If it's not available, indirect mapping will be used instead.
In this case, it's necessary to update the iommu_table so iommu_alloc()
can use the DDW created. For this, iommu_table_update() is called after a
enable_ddw() when direct DMA is not available.
As there will never have both direct and indirect mappings at the same
time, the same property name can be used for the created DDW.
So renaming
define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
to
define DMA64_PROPNAME "linux,dma64-ddr-window-info"
looks the right thing to do.
Signed-off-by: Leonardo Bras
---
arch/powerpc/include/asm/iommu.h | 3 ++
arch/powerpc/kernel/iommu.c| 15 +
arch/powerpc/platforms/pseries/iommu.c | 46 +++---
3 files changed, 52 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index 5032f1593299..dc4480a9d60d 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -154,6 +154,9 @@ extern int iommu_tce_table_put(struct iommu_table *tbl);
*/
extern struct iommu_table *iommu_init_table(struct iommu_table *tbl,
int nid, unsigned long res_start, unsigned long res_end);
+void iommu_table_update(struct iommu_table *tbl, int nid, unsigned long liobn,
+ unsigned long win_addr, unsigned long page_shift,
+ unsigned long window_shift);
#define IOMMU_TABLE_GROUP_MAX_TABLES 2
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index c3242253a4e7..cb0cb572eb0a 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -774,6 +774,21 @@ static void iommu_table_free(struct kref *kref)
kfree(tbl);
}
+void iommu_table_update(struct iommu_table *tbl, int nid, unsigned long liobn,
+ unsigned long win_addr, unsigned long page_shift,
+ unsigned long window_shift)
+{
+ iommu_table_clean(tbl);
+
+ /* Update tlb with values from ddw */
+ tbl->it_index = liobn;
+ tbl->it_offset = win_addr >> page_shift;
+ tbl->it_page_shift = page_shift;
+ tbl->it_size = 1 << (window_shift - page_shift);
+
+ iommu_init_table(tbl, nid, 0, 0);
+}
+
struct iommu_table *iommu_tce_table_get(struct iommu_table *tbl)
{
if (kref_get_unless_zero(&tbl->it_kref))
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index fc8d0555e2e9..6e1c9d1599d1 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -364,7 +364,7 @@ static LIST_HEAD(direct_window_list);
static DEFINE_SPINLOCK(direct_window_list_lock);
/* protects initializing window twice for same device */
static DEFINE_MUTEX(direct_window_init_mutex);
-#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
+#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
unsigned long num_pfn, const void *arg)
@@ -823,7 +823,7 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
if (ret)
return;
- win = of_find_property(np, DIRECT64_PROPNAME, NULL);
+ win = of_find_property(np, DMA64_PROPNAME, NULL);
if (!win)
return;
@@ -869,8 +869,8 @@ static int find_existing_ddw_windows(void)
if (!firmware_has_feature(FW_FEATURE_LPAR))
return 0;
- for_each_node_with_property(pdn, DIRECT64_PROPNAME) {
- direct64 = of_get_property(pdn, DIRECT64_PROPNAME, &len);
+ for_each_node_with_property(pdn, DMA64_PROPNAME) {
+ direct64 = of_get_property(pdn, DMA64_PROPNAME, &len);
if (!direct64)
continue;
@@ -1210,23 +1210,26 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
query.page_size);
goto out_failed;
}
+
/* verify the window * number of ptes will map the partition */
- /* check largest block * page size > max memory hotplug addr */
max_addr = ddw_memory_hotplug_max();
if (query.largest_available_block < (max_addr >> page_shift)) {
- dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
- "%llu-sized pages\n", max_addr,
query.largest_available_block,
- 1ULL << page_shift);
- goto out_failed;
+ dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu
%llu-sized page
[PATCH v4 7/7] powerpc/pseries/iommu: Rename "direct window" to "dma window"
A previous change introduced the usage of DDW as a bigger indirect DMA
mapping when the DDW available size does not map the whole partition.
As most of the code that manipulates direct mappings was reused for
indirect mappings, it's necessary to rename all names and debug/info
messages to reflect that it can be used for both kinds of mapping.
Also, defines DEFAULT_DMA_WIN as "ibm,dma-window" to document that
it's the name of the default DMA window.
Those changes are not supposed to change how the code works in any
way, just adjust naming.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 100 +
1 file changed, 52 insertions(+), 48 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 6e1c9d1599d1..5ca952d966a4 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -339,7 +339,7 @@ struct dynamic_dma_window_prop {
__be32 window_shift; /* ilog2(tce_window_size) */
};
-struct direct_window {
+struct dma_win {
struct device_node *device;
const struct dynamic_dma_window_prop *prop;
struct list_head list;
@@ -359,12 +359,13 @@ struct ddw_create_response {
u32 addr_lo;
};
-static LIST_HEAD(direct_window_list);
+static LIST_HEAD(dma_win_list);
/* prevents races between memory on/offline and window creation */
-static DEFINE_SPINLOCK(direct_window_list_lock);
+static DEFINE_SPINLOCK(dma_win_list_lock);
/* protects initializing window twice for same device */
-static DEFINE_MUTEX(direct_window_init_mutex);
+static DEFINE_MUTEX(dma_win_init_mutex);
#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
+#define DEFAULT_DMA_WIN "ibm,dma-window"
static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
unsigned long num_pfn, const void *arg)
@@ -697,15 +698,18 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus
*bus)
pr_debug("pci_dma_bus_setup_pSeriesLP: setting up bus %pOF\n",
dn);
- /* Find nearest ibm,dma-window, walking up the device tree */
+ /*
+* Find nearest ibm,dma-window (default DMA window), walking up the
+* device tree
+*/
for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
- dma_window = of_get_property(pdn, "ibm,dma-window", NULL);
+ dma_window = of_get_property(pdn, DEFAULT_DMA_WIN, NULL);
if (dma_window != NULL)
break;
}
if (dma_window == NULL) {
- pr_debug(" no ibm,dma-window property !\n");
+ pr_debug(" no %s property !\n", DEFAULT_DMA_WIN);
return;
}
@@ -803,11 +807,11 @@ static void remove_dma_window(struct device_node *np, u32
*ddw_avail,
ret = rtas_call(ddw_avail[DDW_REMOVE_PE_DMA_WIN], 1, 1, NULL, liobn);
if (ret)
- pr_warn("%pOF: failed to remove direct window: rtas returned "
+ pr_warn("%pOF: failed to remove dma window: rtas returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
else
- pr_debug("%pOF: successfully removed direct window: rtas
returned "
+ pr_debug("%pOF: successfully removed dma window: rtas returned "
"%d to ibm,remove-pe-dma-window(%x) %llx\n",
np, ret, ddw_avail[DDW_REMOVE_PE_DMA_WIN], liobn);
}
@@ -835,26 +839,26 @@ static void remove_ddw(struct device_node *np, bool
remove_prop)
ret = of_remove_property(np, win);
if (ret)
- pr_warn("%pOF: failed to remove direct window property: %d\n",
+ pr_warn("%pOF: failed to remove dma window property: %d\n",
np, ret);
}
static u64 find_existing_ddw(struct device_node *pdn)
{
- struct direct_window *window;
- const struct dynamic_dma_window_prop *direct64;
+ struct dma_win *window;
+ const struct dynamic_dma_window_prop *dma64;
u64 dma_addr = 0;
- spin_lock(&direct_window_list_lock);
+ spin_lock(&dma_win_list_lock);
/* check if we already created a window and dupe that config if so */
- list_for_each_entry(window, &direct_window_list, list) {
+ list_for_each_entry(window, &dma_win_list, list) {
if (window->device == pdn) {
- direct64 = window->prop;
- dma_addr = be64_to_cpu(direct64->dma_base);
+ dma64 = window->prop;
+ dma_addr = be64_to_cpu(dma64->dma_base);
break;
}
}
- spin_unlock(&direct_window_list_lock);
+ spin_unlock(&dma_win_list_lock);
return dma_addr;
}
@@ -863,15 +867,15 @@ static int find_existing_ddw_windows(voi
[powerpc:merge] BUILD SUCCESS 58a4eb09c4aebaaffa8b4517c71543a41539c096
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: 58a4eb09c4aebaaffa8b4517c71543a41539c096 Automatic merge of 'master', 'next' and 'fixes' (2020-07-15 23:12) elapsed time: 1031m configs tested: 94 configs skipped: 4 The following configs have been built successfully. More configs may be tested in the coming days. arm defconfig arm allyesconfig arm allmodconfig arm allnoconfig arm64allyesconfig arm64 defconfig arm64allmodconfig arm64 allnoconfig arc axs101_defconfig c6xevmc6457_defconfig sh kfr2r09-romimage_defconfig powerpcgamecube_defconfig arm lpd270_defconfig mips malta_defconfig riscv defconfig c6xevmc6474_defconfig armclps711x_defconfig arm corgi_defconfig riscvallyesconfig arm orion5x_defconfig arm moxart_defconfig powerpcamigaone_defconfig m68k apollo_defconfig shedosk7705_defconfig i386 allnoconfig i386 allyesconfig i386defconfig i386 debian-10.3 ia64 allmodconfig ia64defconfig ia64 allnoconfig ia64 allyesconfig m68k allmodconfig m68k allnoconfig m68k sun3_defconfig m68kdefconfig m68k allyesconfig nios2 defconfig nios2allyesconfig openriscdefconfig c6x allyesconfig c6x allnoconfig openrisc allyesconfig nds32 defconfig nds32 allnoconfig csky allyesconfig cskydefconfig alpha defconfig alphaallyesconfig xtensa allyesconfig h8300allyesconfig h8300allmodconfig xtensa defconfig arc defconfig arc allyesconfig sh allmodconfig shallnoconfig microblazeallnoconfig mips allyesconfig mips allnoconfig mips allmodconfig pariscallnoconfig parisc defconfig parisc allyesconfig parisc allmodconfig powerpc allyesconfig powerpc rhel-kconfig powerpc allmodconfig powerpc allnoconfig powerpc defconfig i386 randconfig-a016-20200715 i386 randconfig-a011-20200715 i386 randconfig-a015-20200715 i386 randconfig-a012-20200715 i386 randconfig-a013-20200715 i386 randconfig-a014-20200715 riscv allnoconfig riscvallmodconfig s390 allyesconfig s390 allnoconfig s390 allmodconfig s390defconfig sparcallyesconfig sparc defconfig sparc64 defconfig sparc64 allnoconfig sparc64 allyesconfig sparc64 allmodconfig x86_64rhel-7.6-kselftests x86_64 rhel-8.3 x86_64 kexec x86_64 rhel x86_64lkp x86_64 fedora-25 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
RE: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Benjamin Herrenschmidt > Sent: 15 July 2020 23:49 > On Wed, 2020-07-15 at 17:12 -0500, Bjorn Helgaas wrote: > > > I've 'played' with PCIe error handling - without much success. > > > What might be useful is for a driver that has just read ~0u to > > > be able to ask 'has there been an error signalled for this device?'. > > > > In many cases a driver will know that ~0 is not a valid value for the > > register it's reading. But if ~0 *could* be valid, an interface like > > you suggest could be useful. I don't think we have anything like that > > today, but maybe we could. It would certainly be nice if the PCI core > > noticed, logged, and cleared errors. We have some of that for AER, > > but that's an optional feature, and support for the error bits in the > > garden-variety PCI_STATUS register is pretty haphazard. As you note > > below, this sort of SERR/PERR reporting is frequently hard-wired in > > ways that takes it out of our purview. > > We do have pci_channel_state (via pci_channel_offline()) which covers > the cases where the underlying error handling (such as EEH or unplug) > results in the device being offlined though this tend to be > asynchronous so it might take a few ~0's before you get it. On one of my systems I don't think the error TLP from the target made its way past the first bridge - I could see the error in it's status registers. But I couldn't find any of the AER status registers in the root bridge. So I think you'd need a software poll of the bridge registers to find out (and clear) the error. The NMI on the dell system (which is supposed to meet some special NEBS? server requirements) is just stupid. Too late to be synchronous and impossible for the OS to handle. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v3] powerpc/pseries: detect secure and trusted boot state of the system.
On Wed, Jul 15, 2020 at 07:52:01AM -0400, Nayna Jain wrote:
> The device-tree property to check secure and trusted boot state is
> different for guests(pseries) compared to baremetal(powernv).
>
> This patch updates the existing is_ppc_secureboot_enabled() and
> is_ppc_trustedboot_enabled() functions to add support for pseries.
>
> The secureboot and trustedboot state are exposed via device-tree property:
> /proc/device-tree/ibm,secure-boot and /proc/device-tree/ibm,trusted-boot
>
> The values of ibm,secure-boot under pseries are interpreted as:
^^^
>
> 0 - Disabled
> 1 - Enabled in Log-only mode. This patch interprets this value as
> disabled, since audit mode is currently not supported for Linux.
> 2 - Enabled and enforced.
> 3-9 - Enabled and enforcing; requirements are at the discretion of the
> operating system.
>
> The values of ibm,trusted-boot under pseries are interpreted as:
^^^
These two should be different I suppose?
Thanks
Michal
> 0 - Disabled
> 1 - Enabled
>
> Signed-off-by: Nayna Jain
> Reviewed-by: Daniel Axtens
> ---
> v3:
> * fixed double check. Thanks Daniel for noticing it.
> * updated patch description.
>
> v2:
> * included Michael Ellerman's feedback.
> * added Daniel Axtens's Reviewed-by.
>
> arch/powerpc/kernel/secure_boot.c | 19 +--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/secure_boot.c
> b/arch/powerpc/kernel/secure_boot.c
> index 4b982324d368..118bcb5f79c4 100644
> --- a/arch/powerpc/kernel/secure_boot.c
> +++ b/arch/powerpc/kernel/secure_boot.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
>
> static struct device_node *get_ppc_fw_sb_node(void)
> {
> @@ -23,12 +24,19 @@ bool is_ppc_secureboot_enabled(void)
> {
> struct device_node *node;
> bool enabled = false;
> + u32 secureboot;
>
> node = get_ppc_fw_sb_node();
> enabled = of_property_read_bool(node, "os-secureboot-enforcing");
> -
> of_node_put(node);
>
> + if (enabled)
> + goto out;
> +
> + if (!of_property_read_u32(of_root, "ibm,secure-boot", &secureboot))
> + enabled = (secureboot > 1);
> +
> +out:
> pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
>
> return enabled;
> @@ -38,12 +46,19 @@ bool is_ppc_trustedboot_enabled(void)
> {
> struct device_node *node;
> bool enabled = false;
> + u32 trustedboot;
>
> node = get_ppc_fw_sb_node();
> enabled = of_property_read_bool(node, "trusted-enabled");
> -
> of_node_put(node);
>
> + if (enabled)
> + goto out;
> +
> + if (!of_property_read_u32(of_root, "ibm,trusted-boot", &trustedboot))
> + enabled = (trustedboot > 0);
> +
> +out:
> pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
>
> return enabled;
> --
> 2.26.2
>
RE: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
From: Bjorn Helgaas > Sent: 15 July 2020 23:02 > > On Wed, Jul 15, 2020 at 02:24:21PM +, David Laight wrote: > > From: Arnd Bergmann > > > Sent: 15 July 2020 07:47 > > > On Wed, Jul 15, 2020 at 1:46 AM Bjorn Helgaas wrote: > > > > > > So something like: > > > > > > > > void pci_read_config_word(struct pci_dev *dev, int where, u16 *val) > > > > > > > > and where we used to return anything non-zero, we just set *val = ~0 > > > > instead? I think we do that already in most, maybe all, cases. > > > > > > Right, this is what I had in mind. If we start by removing the handling > > > of the return code in all files that clearly don't need it, looking at > > > whatever remains will give a much better idea of what a good interface > > > should be. > > > > It would be best to get rid of that nasty 'u16 *' parameter. > > Do you mean nasty because it's basically a return value, but not > returned as the *function's* return value? I agree that if we were > starting from scratch it would nicer to have: > > u16 pci_read_config_word(struct pci_dev *dev, int where) > > but I don't think it's worth changing the thousands of callers just > for that. It'll shrink the kernel text size somewhat. It could also be 'fixed' with a static inline. Actually you don't even want the result to be u16. Even though the domain of the value is 0..65535 keeping the type as int (or unsigned int) will save the compiler having to generate lots of masking instructions. Code performance here will be overwhelmed by the time taken for the config space access. But more generally all local variables should really be the size of cpu registers. On x86-64 you need to use 'unsigned int' for anything used as array subscripts to avoid the 'sign extend' instructions. In some code paths it may matter... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[RFC PATCH] powerpc/pseries/svm: capture instruction faulting on MMIO access, in sprg0 register
An instruction accessing a mmio address, generates a HDSI fault. This fault is
appropriately handled by the Hypervisor. However in the case of secureVMs, the
fault is delivered to the ultravisor.
Unfortunately the Ultravisor has no correct-way to fetch the faulting
instruction. The PEF architecture does not allow Ultravisor to enable MMU
translation. Walking the two level page table to read the instruction can race
with other vcpus modifying the SVM's process scoped page table.
This problem can be correctly solved with some help from the kernel.
Capture the faulting instruction in SPRG0 register, before executing the
faulting instruction. This enables the ultravisor to easily procure the
faulting instruction and emulate it.
Signed-off-by: Ram Pai
---
arch/powerpc/include/asm/io.h | 85 ++-
1 file changed, 75 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 635969b..7ef663d 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#define SIO_CONFIG_RA 0x398
#define SIO_CONFIG_RD 0x399
@@ -105,34 +106,98 @@
static inline u##size name(const volatile u##size __iomem *addr) \
{ \
u##size ret;\
- __asm__ __volatile__("sync;"#insn" %0,%y1;twi 0,%0,0;isync" \
- : "=r" (ret) : "Z" (*addr) : "memory"); \
+ if (is_secure_guest()) {\
+ __asm__ __volatile__("mfsprg0 %3;" \
+ "lnia %2;" \
+ "ld %2,12(%2);" \
+ "mtsprg0 %2;" \
+ "sync;" \
+ #insn" %0,%y1;" \
+ "twi 0,%0,0;" \
+ "isync;"\
+ "mtsprg0 %3"\
+ : "=r" (ret)\
+ : "Z" (*addr), "r" (0), "r" (0) \
+ : "memory");\
+ } else {\
+ __asm__ __volatile__("sync;"\
+ #insn" %0,%y1;" \
+ "twi 0,%0,0;" \
+ "isync" \
+ : "=r" (ret) : "Z" (*addr) : "memory"); \
+ } \
return ret; \
}
#define DEF_MMIO_OUT_X(name, size, insn) \
static inline void name(volatile u##size __iomem *addr, u##size val) \
{ \
- __asm__ __volatile__("sync;"#insn" %1,%y0" \
- : "=Z" (*addr) : "r" (val) : "memory"); \
- mmiowb_set_pending(); \
+ if (is_secure_guest()) {\
+ __asm__ __volatile__("mfsprg0 %3;" \
+ "lnia %2;" \
+ "ld %2,12(%2);" \
+ "mtsprg0 %2;" \
+ "sync;" \
+ #insn" %1,%y0;" \
+ "mtsprg0 %3"\
+ : "=Z" (*addr) \
+ : "r" (val), "r" (0), "r" (0) \
+ : "memory");\
+ } else {\
+ __asm__ __volatile__("sync;"\
+ #insn" %1,%y0" \
+ : "=Z" (*addr) : "r" (val) : "memory"); \
+ mmiowb_set_pending(); \
+ } \
}
#define DEF_MMIO_IN_D(name, size, insn)\
static inline u##size name(const volatile u##size __iomem *addr)
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
On Wed, Jul 15, 2020 at 10:18:20PM -0700, Andy Lutomirski wrote: > > On Jul 15, 2020, at 9:15 PM, Nicholas Piggin wrote: > > CPU0 CPU1 > > 1. user stuff > > a. membarrier() 2. enter kernel > > b. read rq->curr 3. rq->curr switched to kthread > > c. is kthread, skip IPI 4. switch_to kthread > > d. return to user5. rq->curr switched to user thread > > 6. switch_to user thread > > 7. exit kernel > > 8. more user stuff > I find it hard to believe that this is x86 only. Why would thread > switch imply core sync on any architecture? Is x86 unique in having a > stupid expensive core sync that is heavier than smp_mb()? smp_mb() is nowhere near the most expensive barrier we have in Linux, mb() might qualify, since that has some completion requirements since it needs to serialize against external actors. On x86_64 things are rather murky, we have: LOCK prefix -- which implies smp_mb() before and after RmW LFENCE -- which used to be rmb like, until Spectre, and now it is ISYNC like. Since ISYNC ensures an empty pipeline, it also implies all loads are retired (and therefore complete) it implies rmb. MFENCE -- which is a memop completion barrier like, it makes sure all previously issued memops are complete. if you read that carefully, you'll note you'll have to use LFENCE + MFENCE to order against non-memops instructions. But none of them imply dumping the instruction decoder caches, that only happens on core serializing instructions like CR3 writes, IRET, CPUID and a few others, I think we recently got a SERIALIZE instruction to add to this list. On ARM64 there's something a whole different set of barriers, and again smp_mb() isn't nowhere near the top of the list. They have roughly 3 classes: ISB -- instruction sync barrier DMB(x) -- memory ordering in domain x DSB(x) -- memory completion in domain x And they have at least 3 domains (IIRC), system, outer, inner. The ARM64 __switch_to() includes a dsb(sy), just like PowerPC used to have a SYNC, but since PowerPC is rare for only having one rediculously heavy serializing instruction, we got to re-use the smp_mb() early in __schedule() instead, but ARM64 can't do that. So rather than say that x86 is special here, I'd say that PowerPC is special here. > But I’m wondering if all this deferred sync stuff is wrong. In the > brave new world of io_uring and such, perhaps kernel access matter > too. Heck, even: IIRC the membarrier SYNC_CORE use-case is about user-space self-modifying code. Userspace re-uses a text address and needs to SYNC_CORE before it can be sure the old text is forgotten. Nothing the kernel does matters there. I suppose the manpage could be more clear there.
Re: [PATCH] pseries: Fix 64 bit logical memory block panic
On Wed, Jul 15, 2020 at 06:12:25PM +0530, Aneesh Kumar K.V wrote:
> Anton Blanchard writes:
>
> > Booting with a 4GB LMB size causes us to panic:
> >
> > qemu-system-ppc64: OS terminated: OS panic:
> > Memory block size not suitable: 0x0
> >
> > Fix pseries_memory_block_size() to handle 64 bit LMBs.
> >
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Anton Blanchard
> > ---
> > arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
> > b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > index 5ace2f9a277e..6574ac33e887 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > @@ -27,7 +27,7 @@ static bool rtas_hp_event;
> > unsigned long pseries_memory_block_size(void)
> > {
> > struct device_node *np;
> > - unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
> > + uint64_t memblock_size = MIN_MEMORY_BLOCK_SIZE;
> > struct resource r;
> >
> > np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>
> We need similar changes at more places?
>
> modified arch/powerpc/include/asm/book3s/64/mmu.h
> @@ -85,7 +85,7 @@ extern unsigned int mmu_base_pid;
> /*
> * memory block size used with radix translation.
> */
> -extern unsigned int __ro_after_init radix_mem_block_size;
> +extern unsigned long __ro_after_init radix_mem_block_size;
>
> #define PRTB_SIZE_SHIFT (mmu_pid_bits + 4)
> #define PRTB_ENTRIES (1ul << mmu_pid_bits)
> modified arch/powerpc/include/asm/drmem.h
> @@ -21,7 +21,7 @@ struct drmem_lmb {
> struct drmem_lmb_info {
> struct drmem_lmb*lmbs;
> int n_lmbs;
> - u32 lmb_size;
> + u64 lmb_size;
> };
>
> extern struct drmem_lmb_info *drmem_info;
> modified arch/powerpc/mm/book3s64/radix_pgtable.c
> @@ -34,7 +34,7 @@
>
> unsigned int mmu_pid_bits;
> unsigned int mmu_base_pid;
> -unsigned int radix_mem_block_size __ro_after_init;
> +unsigned long radix_mem_block_size __ro_after_init;
These changes look fine.
> static __ref void *early_alloc_pgtable(unsigned long size, int nid,
> unsigned long region_start, unsigned long region_end)
> modified arch/powerpc/mm/drmem.c
> @@ -268,14 +268,15 @@ static void __init __walk_drmem_v2_lmbs(const __be32
> *prop, const __be32 *usm,
> void __init walk_drmem_lmbs_early(unsigned long node,
> void (*func)(struct drmem_lmb *, const __be32 **))
> {
> + const __be64 *lmb_prop;
> const __be32 *prop, *usm;
> int len;
>
> - prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
> - if (!prop || len < dt_root_size_cells * sizeof(__be32))
> + lmb_prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
> + if (!lmb_prop || len < sizeof(__be64))
> return;
>
> - drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
> + drmem_info->lmb_size = be64_to_cpup(lmb_prop);
This particular change shouldn't be necessary. We already have
dt_mem_next_cell() returning u64, and it knows how to combine two
cells to give a u64 (for dt_root_size_cells == 2).
> usm = of_get_flat_dt_prop(node, "linux,drconf-usable-memory", &len);
>
> @@ -296,19 +297,19 @@ void __init walk_drmem_lmbs_early(unsigned long node,
>
> static int __init init_drmem_lmb_size(struct device_node *dn)
> {
> - const __be32 *prop;
> + const __be64 *prop;
> int len;
>
> if (drmem_info->lmb_size)
> return 0;
>
> prop = of_get_property(dn, "ibm,lmb-size", &len);
> - if (!prop || len < dt_root_size_cells * sizeof(__be32)) {
> + if (!prop || len < sizeof(__be64)) {
> pr_info("Could not determine LMB size\n");
> return -1;
> }
>
> - drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
> + drmem_info->lmb_size = be64_to_cpup(prop);
Same comment here.
Paul.
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
Excerpts from Peter Zijlstra's message of July 16, 2020 6:50 pm: > On Wed, Jul 15, 2020 at 10:18:20PM -0700, Andy Lutomirski wrote: >> > On Jul 15, 2020, at 9:15 PM, Nicholas Piggin wrote: > >> > CPU0 CPU1 >> > 1. user stuff >> > a. membarrier() 2. enter kernel >> > b. read rq->curr 3. rq->curr switched to kthread >> > c. is kthread, skip IPI 4. switch_to kthread >> > d. return to user5. rq->curr switched to user thread >> > 6. switch_to user thread >> > 7. exit kernel >> > 8. more user stuff > >> I find it hard to believe that this is x86 only. Why would thread >> switch imply core sync on any architecture? Is x86 unique in having a >> stupid expensive core sync that is heavier than smp_mb()? > > smp_mb() is nowhere near the most expensive barrier we have in Linux, > mb() might qualify, since that has some completion requirements since it > needs to serialize against external actors. > > On x86_64 things are rather murky, we have: > > LOCK prefix -- which implies smp_mb() before and after RmW > LFENCE -- which used to be rmb like, until Spectre, and now it > is ISYNC like. Since ISYNC ensures an empty pipeline, > it also implies all loads are retired (and therefore > complete) it implies rmb. > MFENCE -- which is a memop completion barrier like, it makes > sure all previously issued memops are complete. > > if you read that carefully, you'll note you'll have to use LFENCE + > MFENCE to order against non-memops instructions. > > But none of them imply dumping the instruction decoder caches, that only > happens on core serializing instructions like CR3 writes, IRET, CPUID > and a few others, I think we recently got a SERIALIZE instruction to add > to this list. > > > On ARM64 there's something a whole different set of barriers, and again > smp_mb() isn't nowhere near the top of the list. They have roughly 3 > classes: > > ISB -- instruction sync barrier > DMB(x) -- memory ordering in domain x > DSB(x) -- memory completion in domain x > > And they have at least 3 domains (IIRC), system, outer, inner. > > The ARM64 __switch_to() includes a dsb(sy), just like PowerPC used to > have a SYNC, but since PowerPC is rare for only having one rediculously > heavy serializing instruction, we got to re-use the smp_mb() early in > __schedule() instead, but ARM64 can't do that. > > > So rather than say that x86 is special here, I'd say that PowerPC is > special here. PowerPC is "special", I'll agree with you there :) It does have a SYNC (HWSYNC) instruction that is mb(). It does not serialize the core. ISYNC is a nop. ICBI ; ISYNC does serialize the core. Difference between them is probably much the same as difference between MFENCE and CPUID on x86 CPUs. Serializing the core is almost always pretty expensive. HWSYNC/MFENCE can be expensive if you have a lot of or difficult (not exclusive in cache) outstanding with critical reads after the barrier, but it can also be somewhat cheap if there are few writes, and executed past, it only needs to hold up subsequent reads. That said... implementation details. powerpc CPUs have traditionally had fairly costly HWSYNC. >> But I’m wondering if all this deferred sync stuff is wrong. In the >> brave new world of io_uring and such, perhaps kernel access matter >> too. Heck, even: > > IIRC the membarrier SYNC_CORE use-case is about user-space > self-modifying code. > > Userspace re-uses a text address and needs to SYNC_CORE before it can be > sure the old text is forgotten. Nothing the kernel does matters there. > > I suppose the manpage could be more clear there. True, but memory ordering of kernel stores from kernel threads for regular mem barrier is the concern here. Does io_uring update completion queue from kernel thread or interrupt, for example? If it does, then membarrier will not order such stores with user memory accesses. Thanks, Nick
[PATCH -next] powerpc: Convert to DEFINE_SHOW_ATTRIBUTE
From: Chen Huang
Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
Signed-off-by: Chen Huang
---
arch/powerpc/kvm/book3s_xive_native.c | 12 +---
arch/powerpc/mm/ptdump/bats.c | 24 +++-
arch/powerpc/mm/ptdump/hashpagetable.c | 12 +---
arch/powerpc/mm/ptdump/ptdump.c| 13 +
arch/powerpc/mm/ptdump/segment_regs.c | 12 +---
5 files changed, 11 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_xive_native.c
b/arch/powerpc/kvm/book3s_xive_native.c
index 02e3cbbea..d0c2db0e0 100644
--- a/arch/powerpc/kvm/book3s_xive_native.c
+++ b/arch/powerpc/kvm/book3s_xive_native.c
@@ -1227,17 +1227,7 @@ static int xive_native_debug_show(struct seq_file *m,
void *private)
return 0;
}
-static int xive_native_debug_open(struct inode *inode, struct file *file)
-{
- return single_open(file, xive_native_debug_show, inode->i_private);
-}
-
-static const struct file_operations xive_native_debug_fops = {
- .open = xive_native_debug_open,
- .read_iter = seq_read_iter,
- .llseek = seq_lseek,
- .release = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(xive_native_debug);
static void xive_native_debugfs_init(struct kvmppc_xive *xive)
{
diff --git a/arch/powerpc/mm/ptdump/bats.c b/arch/powerpc/mm/ptdump/bats.c
index 7afcdac48..93771af72 100644
--- a/arch/powerpc/mm/ptdump/bats.c
+++ b/arch/powerpc/mm/ptdump/bats.c
@@ -56,7 +56,7 @@ static void bat_show_601(struct seq_file *m, int idx, u32
lower, u32 upper)
#define BAT_SHOW_601(_m, _n, _l, _u) bat_show_601(_m, _n, mfspr(_l), mfspr(_u))
-static int bats_show_601(struct seq_file *m, void *v)
+static int bats_601_show(struct seq_file *m, void *v)
{
seq_puts(m, "---[ Block Address Translation ]---\n");
@@ -113,7 +113,7 @@ static void bat_show_603(struct seq_file *m, int idx, u32
lower, u32 upper, bool
#define BAT_SHOW_603(_m, _n, _l, _u, _d) bat_show_603(_m, _n, mfspr(_l),
mfspr(_u), _d)
-static int bats_show_603(struct seq_file *m, void *v)
+static int bats_603_show(struct seq_file *m, void *v)
{
seq_puts(m, "---[ Instruction Block Address Translation ]---\n");
@@ -144,25 +144,15 @@ static int bats_show_603(struct seq_file *m, void *v)
return 0;
}
-static int bats_open(struct inode *inode, struct file *file)
-{
- if (IS_ENABLED(CONFIG_PPC_BOOK3S_601))
- return single_open(file, bats_show_601, NULL);
-
- return single_open(file, bats_show_603, NULL);
-}
-
-static const struct file_operations bats_fops = {
- .open = bats_open,
- .read_iter = seq_read_iter,
- .llseek = seq_lseek,
- .release= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(bats_601);
+DEFINE_SHOW_ATTRIBUTE(bats_603);
static int __init bats_init(void)
{
debugfs_create_file("block_address_translation", 0400,
- powerpc_debugfs_root, NULL, &bats_fops);
+ powerpc_debugfs_root, NULL,
+ IS_ENABLED(CONFIG_PPC_BOOK3S_601) ?
+ &bats_601_fops : &bats_603_fops);
return 0;
}
device_initcall(bats_init);
diff --git a/arch/powerpc/mm/ptdump/hashpagetable.c
b/arch/powerpc/mm/ptdump/hashpagetable.c
index 457fcee7e..c7f824d29 100644
--- a/arch/powerpc/mm/ptdump/hashpagetable.c
+++ b/arch/powerpc/mm/ptdump/hashpagetable.c
@@ -526,17 +526,7 @@ static int ptdump_show(struct seq_file *m, void *v)
return 0;
}
-static int ptdump_open(struct inode *inode, struct file *file)
-{
- return single_open(file, ptdump_show, NULL);
-}
-
-static const struct file_operations ptdump_fops = {
- .open = ptdump_open,
- .read_iter = seq_read_iter,
- .llseek = seq_lseek,
- .release= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump);
static int ptdump_init(void)
{
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index db17e84b5..58b062f1b 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -398,18 +398,7 @@ static int ptdump_show(struct seq_file *m, void *v)
return 0;
}
-
-static int ptdump_open(struct inode *inode, struct file *file)
-{
- return single_open(file, ptdump_show, NULL);
-}
-
-static const struct file_operations ptdump_fops = {
- .open = ptdump_open,
- .read_iter = seq_read_iter,
- .llseek = seq_lseek,
- .release= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(ptdump);
static void build_pgtable_complete_mask(void)
{
diff --git a/arch/powerpc/mm/ptdump/segment_regs.c
b/arch/powerpc/mm/ptdump/segment_regs.c
index 8b15bad5a..9e870d44c 100644
--- a/arch/powerpc/mm/ptdump/segment_regs.c
+++ b/arch/powerpc/mm/ptdump/segment_regs.c
@@ -41,17 +41,7 @@ static int sr_show(struct seq_file *m, void *v)
return 0;
}
-static int sr_open(stru
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
On Thu, Jul 16, 2020 at 08:03:36PM +1000, Nicholas Piggin wrote: > Excerpts from Peter Zijlstra's message of July 16, 2020 6:50 pm: > > On Wed, Jul 15, 2020 at 10:18:20PM -0700, Andy Lutomirski wrote: > >> > On Jul 15, 2020, at 9:15 PM, Nicholas Piggin wrote: > >> But I’m wondering if all this deferred sync stuff is wrong. In the > >> brave new world of io_uring and such, perhaps kernel access matter > >> too. Heck, even: > > > > IIRC the membarrier SYNC_CORE use-case is about user-space > > self-modifying code. > > > > Userspace re-uses a text address and needs to SYNC_CORE before it can be > > sure the old text is forgotten. Nothing the kernel does matters there. > > > > I suppose the manpage could be more clear there. > > True, but memory ordering of kernel stores from kernel threads for > regular mem barrier is the concern here. > > Does io_uring update completion queue from kernel thread or interrupt, > for example? If it does, then membarrier will not order such stores > with user memory accesses. So we're talking about regular membarrier() then? Not the SYNC_CORE variant per-se. Even there, I'll argue we don't care, but perhaps Mathieu has a different opinion. All we care about is that all other threads (or CPUs for GLOBAL) observe an smp_mb() before it returns. Any serialization against whatever those other threads/CPUs are running at the instant of the syscall is external to the syscall, we make no gauarantees about that. That is, we can fundamentally not say what another CPU is executing concurrently. Nor should we want to. So if you feel that your membarrier() ought to serialize against remote execution, you need to arrange a quiecent state on the remote side yourself. Now, normally membarrier() is used to implement userspace RCU like things, and there all that matters is that the remote CPUs observe the beginngin of the new grace-period, ie counter flip, and we observe their read-side critical sections, or smething like that, it's been a while since I looked at all that. It's always been the case that concurrent syscalls could change user memory, io_uring doesn't change that, it just makes it even less well defined when that would happen. If you want to serialize against that, you need to arrange that externally.
[PATCH 0/5] Improvements to pkey tests
Based on recent bugs found in the pkey infrastructure, this improves the test for execute-disabled pkeys and adds a new test for detecting inconsistencies with the pkey reported by the signal information upon getting a fault. Sandipan Das (5): selftests/powerpc: Move pkey helpers to headers selftests/powerpc: Add pkey helpers for rights selftests/powerpc: Harden test for execute-disabled pkeys selftests/powerpc: Add helper to exit on failure selftests/powerpc: Add test for pkey siginfo verification .../testing/selftests/powerpc/include/pkeys.h | 136 +++ .../testing/selftests/powerpc/include/utils.h | 13 + tools/testing/selftests/powerpc/mm/.gitignore | 1 + tools/testing/selftests/powerpc/mm/Makefile | 5 +- .../selftests/powerpc/mm/pkey_exec_prot.c | 210 +++ .../selftests/powerpc/mm/pkey_siginfo.c | 332 ++ 6 files changed, 544 insertions(+), 153 deletions(-) create mode 100644 tools/testing/selftests/powerpc/include/pkeys.h create mode 100644 tools/testing/selftests/powerpc/mm/pkey_siginfo.c -- 2.25.1
[PATCH 1/5] selftests/powerpc: Move pkey helpers to headers
This moves all the pkey-related helpers to a new header
file and also a helper to print error messages in signal
handlers to the existing utils header file.
Signed-off-by: Sandipan Das
---
.../testing/selftests/powerpc/include/pkeys.h | 108 ++
.../testing/selftests/powerpc/include/utils.h | 4 +
.../selftests/powerpc/mm/pkey_exec_prot.c | 100 +---
3 files changed, 114 insertions(+), 98 deletions(-)
create mode 100644 tools/testing/selftests/powerpc/include/pkeys.h
diff --git a/tools/testing/selftests/powerpc/include/pkeys.h
b/tools/testing/selftests/powerpc/include/pkeys.h
new file mode 100644
index 0..9b53a97e664ea
--- /dev/null
+++ b/tools/testing/selftests/powerpc/include/pkeys.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2020, Sandipan Das, IBM Corp.
+ */
+
+#ifndef _SELFTESTS_POWERPC_PKEYS_H
+#define _SELFTESTS_POWERPC_PKEYS_H
+
+#include
+
+#include "reg.h"
+#include "utils.h"
+
+/*
+ * Older versions of libc use the Intel-specific access rights.
+ * Hence, override the definitions as they might be incorrect.
+ */
+#undef PKEY_DISABLE_ACCESS
+#define PKEY_DISABLE_ACCESS0x3
+
+#undef PKEY_DISABLE_WRITE
+#define PKEY_DISABLE_WRITE 0x2
+
+#undef PKEY_DISABLE_EXECUTE
+#define PKEY_DISABLE_EXECUTE 0x4
+
+/* Older versions of libc do not not define this */
+#ifndef SEGV_PKUERR
+#define SEGV_PKUERR4
+#endif
+
+#define SI_PKEY_OFFSET 0x20
+
+#define SYS_pkey_mprotect 386
+#define SYS_pkey_alloc 384
+#define SYS_pkey_free 385
+
+#define PKEY_BITS_PER_PKEY 2
+#define NR_PKEYS 32
+#define PKEY_BITS_MASK ((1UL << PKEY_BITS_PER_PKEY) - 1)
+
+inline unsigned long pkeyreg_get(void)
+{
+ return mfspr(SPRN_AMR);
+}
+
+inline void pkeyreg_set(unsigned long amr)
+{
+ set_amr(amr);
+}
+
+void pkey_set_rights(int pkey, unsigned long rights)
+{
+ unsigned long amr, shift;
+
+ shift = (NR_PKEYS - pkey - 1) * PKEY_BITS_PER_PKEY;
+ amr = pkeyreg_get();
+ amr &= ~(PKEY_BITS_MASK << shift);
+ amr |= (rights & PKEY_BITS_MASK) << shift;
+ pkeyreg_set(amr);
+}
+
+int sys_pkey_mprotect(void *addr, size_t len, int prot, int pkey)
+{
+ return syscall(SYS_pkey_mprotect, addr, len, prot, pkey);
+}
+
+int sys_pkey_alloc(unsigned long flags, unsigned long rights)
+{
+ return syscall(SYS_pkey_alloc, flags, rights);
+}
+
+int sys_pkey_free(int pkey)
+{
+ return syscall(SYS_pkey_free, pkey);
+}
+
+int pkeys_unsupported(void)
+{
+ bool hash_mmu = false;
+ int pkey;
+
+ /* Protection keys are currently supported on Hash MMU only */
+ FAIL_IF(using_hash_mmu(&hash_mmu));
+ SKIP_IF(!hash_mmu);
+
+ /* Check if the system call is supported */
+ pkey = sys_pkey_alloc(0, 0);
+ SKIP_IF(pkey < 0);
+ sys_pkey_free(pkey);
+
+ return 0;
+}
+
+int siginfo_pkey(siginfo_t *si)
+{
+ /*
+* In older versions of libc, siginfo_t does not have si_pkey as
+* a member.
+*/
+#ifdef si_pkey
+ return si->si_pkey;
+#else
+ return *((int *)(((char *) si) + SI_PKEY_OFFSET));
+#endif
+}
+
+#endif /* _SELFTESTS_POWERPC_PKEYS_H */
diff --git a/tools/testing/selftests/powerpc/include/utils.h
b/tools/testing/selftests/powerpc/include/utils.h
index 9dbe607cc5ec3..7f259f36e23bc 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -97,6 +97,10 @@ do {
\
#define _str(s) #s
#define str(s) _str(s)
+#define sigsafe_err(msg) ({ \
+ ssize_t nbytes __attribute__((unused)); \
+ nbytes = write(STDERR_FILENO, msg, strlen(msg)); })
+
/* POWER9 feature */
#ifndef PPC_FEATURE2_ARCH_3_00
#define PPC_FEATURE2_ARCH_3_00 0x0080
diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
index 7c7c93425c5e9..1253ad6afba24 100644
--- a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
+++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
@@ -14,83 +14,13 @@
#include
#include
-#include
-#include "reg.h"
-#include "utils.h"
-
-/*
- * Older versions of libc use the Intel-specific access rights.
- * Hence, override the definitions as they might be incorrect.
- */
-#undef PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS0x3
-
-#undef PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE 0x2
-
-#undef PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE 0x4
-
-/* Older versions of libc do not not define this */
-#ifndef SEGV_PKUERR
-#define SEGV_PKUERR4
-#endif
-
-#define SI_PKEY_OFFSET 0x20
-
-#define SYS_pkey_mprotect 386
-#define SYS_pkey_alloc 384
-#define SYS_pkey_free 385
-
-#define PKEY_BITS_PER_PKEY 2
-#define NR_PKEYS 32
-#define PKEY_BITS_MASK ((1UL << PKEY_BITS_PER_PKEY) - 1)
+#include "pke
[PATCH 2/5] selftests/powerpc: Add pkey helpers for rights
This adds some new pkey-related helper to print
access rights of a pkey in the "rwx" format and
to generate different valid combinations of pkey
rights starting from a given combination.
Signed-off-by: Sandipan Das
---
.../testing/selftests/powerpc/include/pkeys.h | 28 +++
.../selftests/powerpc/mm/pkey_exec_prot.c | 36 ++-
2 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/powerpc/include/pkeys.h
b/tools/testing/selftests/powerpc/include/pkeys.h
index 9b53a97e664ea..6ba95039a0343 100644
--- a/tools/testing/selftests/powerpc/include/pkeys.h
+++ b/tools/testing/selftests/powerpc/include/pkeys.h
@@ -105,4 +105,32 @@ int siginfo_pkey(siginfo_t *si)
#endif
}
+#define pkey_rights(r) ({ \
+ static char buf[4] = "rwx"; \
+ unsigned int amr_bits; \
+ if ((r) & PKEY_DISABLE_EXECUTE) \
+ buf[2] = '-'; \
+ amr_bits = (r) & PKEY_BITS_MASK;\
+ if (amr_bits & PKEY_DISABLE_WRITE) \
+ buf[1] = '-'; \
+ if (amr_bits & PKEY_DISABLE_ACCESS & ~PKEY_DISABLE_WRITE) \
+ buf[0] = '-'; \
+ buf;\
+})
+
+unsigned long next_pkey_rights(unsigned long rights)
+{
+ if (rights == PKEY_DISABLE_ACCESS)
+ return PKEY_DISABLE_EXECUTE;
+ else if (rights == (PKEY_DISABLE_ACCESS | PKEY_DISABLE_EXECUTE))
+ return 0;
+
+ if ((rights & PKEY_BITS_MASK) == 0)
+ rights |= PKEY_DISABLE_WRITE;
+ else if ((rights & PKEY_BITS_MASK) == PKEY_DISABLE_WRITE)
+ rights |= PKEY_DISABLE_ACCESS;
+
+ return rights;
+}
+
#endif /* _SELFTESTS_POWERPC_PKEYS_H */
diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
index 1253ad6afba24..18ebfe6bae1c9 100644
--- a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
+++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
@@ -102,6 +102,7 @@ static void segv_handler(int signum, siginfo_t *sinfo, void
*ctx)
static int test(void)
{
struct sigaction segv_act, trap_act;
+ unsigned long rights;
int pkey, ret, i;
ret = pkeys_unsupported();
@@ -150,7 +151,8 @@ static int test(void)
insns[numinsns - 1] = PPC_INST_BLR;
/* Allocate a pkey that restricts execution */
- pkey = sys_pkey_alloc(0, PKEY_DISABLE_EXECUTE);
+ rights = PKEY_DISABLE_EXECUTE;
+ pkey = sys_pkey_alloc(0, rights);
FAIL_IF(pkey < 0);
/*
@@ -175,8 +177,8 @@ static int test(void)
*/
remaining_faults = 0;
FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- printf("read from %p, pkey is execute-disabled, access-enabled\n",
- (void *) fault_addr);
+ printf("read from %p, pkey permissions are %s\n", fault_addr,
+ pkey_rights(rights));
i = *fault_addr;
FAIL_IF(remaining_faults != 0);
@@ -192,12 +194,13 @@ static int test(void)
*/
remaining_faults = 1;
FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- printf("write to %p, pkey is execute-disabled, access-enabled\n",
- (void *) fault_addr);
+ printf("write to %p, pkey permissions are %s\n", fault_addr,
+ pkey_rights(rights));
*fault_addr = PPC_INST_TRAP;
FAIL_IF(remaining_faults != 0 || fault_code != SEGV_ACCERR);
/* The following three cases will generate SEGV_PKUERR */
+ rights |= PKEY_DISABLE_ACCESS;
fault_type = PKEY_DISABLE_ACCESS;
fault_pkey = pkey;
@@ -211,9 +214,9 @@ static int test(void)
*/
remaining_faults = 1;
FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- printf("read from %p, pkey is execute-disabled, access-disabled\n",
- (void *) fault_addr);
- pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
+ pkey_set_rights(pkey, rights);
+ printf("read from %p, pkey permissions are %s\n", fault_addr,
+ pkey_rights(rights));
i = *fault_addr;
FAIL_IF(remaining_faults != 0 || fault_code != SEGV_PKUERR);
@@ -228,9 +231,9 @@ static int test(void)
*/
remaining_faults = 2;
FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- printf("write to %p, pkey is execute-disabled, access-disabled\n",
- (void *) fault_addr);
- pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
+ pkey_set_rights(pkey, rights);
+ printf("write to %p, pkey permiss
[PATCH 3/5] selftests/powerpc: Harden test for execute-disabled pkeys
Commit 192b6a7805989 ("powerpc/book3s64/pkeys: Fix
pkey_access_permitted() for execute disable pkey") fixed a
bug that caused repetitive faults for pkeys with no execute
rights alongside some combination of read and write rights.
This removes the last two cases of the test, which check
the behaviour of pkeys with read, write but no execute
rights and all the rights, in favour of checking all the
possible combinations of read, write and execute rights
to be able to detect bugs like the one mentioned above.
Signed-off-by: Sandipan Das
---
.../selftests/powerpc/mm/pkey_exec_prot.c | 84 +--
1 file changed, 41 insertions(+), 43 deletions(-)
diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
index 18ebfe6bae1c9..9e5c7f3f498a7 100644
--- a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
+++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
@@ -237,55 +237,53 @@ static int test(void)
*fault_addr = PPC_INST_NOP;
FAIL_IF(remaining_faults != 0 || fault_code != SEGV_ACCERR);
- /*
-* Jump to the executable region when AMR bits are set i.e.
-* the pkey permits neither read nor write access.
-*
-* This should generate a pkey fault based on IAMR bits which
-* are set to not permit execution. AMR bits should not affect
-* execution.
-*
-* This also checks if the overwrite of the first instruction
-* word from a trap to a no-op succeeded.
-*/
- fault_addr = insns;
- fault_type = PKEY_DISABLE_EXECUTE;
- fault_pkey = pkey;
- remaining_faults = 1;
- FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
- printf("execute at %p, pkey permissions are %s\n", fault_addr,
- pkey_rights(rights));
- asm volatile("mtctr %0; bctrl" : : "r"(insns));
- FAIL_IF(remaining_faults != 0 || fault_code != SEGV_PKUERR);
-
- /*
-* Free the current pkey and allocate a new one that is
-* fully permissive.
-*/
+ /* Free the current pkey */
sys_pkey_free(pkey);
+
rights = 0;
- pkey = sys_pkey_alloc(0, rights);
+ do {
+ /*
+* Allocate pkeys with all valid combinations of read,
+* write and execute restrictions.
+*/
+ pkey = sys_pkey_alloc(0, rights);
+ FAIL_IF(pkey < 0);
+
+ /*
+* Jump to the executable region. AMR bits may or may not
+* be set but they should not affect execution.
+*
+* This should generate pkey faults based on IAMR bits which
+* may be set to restrict execution.
+*
+* The first iteration also checks if the overwrite of the
+* first instruction word from a trap to a no-op succeeded.
+*/
+ fault_pkey = pkey;
+ fault_type = -1;
+ remaining_faults = 0;
+ if (rights & PKEY_DISABLE_EXECUTE) {
+ fault_type = PKEY_DISABLE_EXECUTE;
+ remaining_faults = 1;
+ }
- /*
-* Jump to the executable region when AMR bits are not set
-* i.e. the pkey permits read and write access.
-*
-* This should not generate any faults as the IAMR bits are
-* also not set and hence will the pkey will not restrict
-* execution.
-*/
- fault_pkey = pkey;
- remaining_faults = 0;
- FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
- printf("execute at %p, pkey permissions are %s\n", fault_addr,
- pkey_rights(rights));
- asm volatile("mtctr %0; bctrl" : : "r"(insns));
- FAIL_IF(remaining_faults != 0);
+ FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
+ printf("execute at %p, pkey permissions are %s\n", fault_addr,
+ pkey_rights(rights));
+ asm volatile("mtctr %0; bctrl" : : "r"(insns));
+ FAIL_IF(remaining_faults != 0);
+ if (rights & PKEY_DISABLE_EXECUTE)
+ FAIL_IF(fault_code != SEGV_PKUERR);
+
+ /* Free the current pkey */
+ sys_pkey_free(pkey);
+
+ /* Find next valid combination of pkey rights */
+ rights = next_pkey_rights(rights);
+ } while (rights);
/* Cleanup */
munmap((void *) insns, pgsize);
- sys_pkey_free(pkey);
return 0;
}
--
2.25.1
[PATCH 4/5] selftests/powerpc: Add helper to exit on failure
This adds a helper similar to FAIL_IF() which lets a
program exit with code 1 (to indicate failure) when
the given condition is true.
Signed-off-by: Sandipan Das
---
tools/testing/selftests/powerpc/include/utils.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/tools/testing/selftests/powerpc/include/utils.h
b/tools/testing/selftests/powerpc/include/utils.h
index 7f259f36e23bc..69d16875802da 100644
--- a/tools/testing/selftests/powerpc/include/utils.h
+++ b/tools/testing/selftests/powerpc/include/utils.h
@@ -72,6 +72,15 @@ do {
\
} \
} while (0)
+#define FAIL_IF_EXIT(x)\
+do { \
+ if ((x)) { \
+ fprintf(stderr, \
+ "[FAIL] Test FAILED on line %d\n", __LINE__); \
+ _exit(1); \
+ } \
+} while (0)
+
/* The test harness uses this, yes it's gross */
#define MAGIC_SKIP_RETURN_VALUE99
--
2.25.1
[PATCH 5/5] selftests/powerpc: Add test for pkey siginfo verification
Commit c46241a370a61 ("powerpc/pkeys: Check vma before
returning key fault error to the user") fixes a bug which
causes the kernel to set the wrong pkey in siginfo when a
pkey fault occurs after two competing threads that have
allocated different pkeys, one fully permissive and the
other restrictive, attempt to protect a common page at the
same time. This adds a test to detect the bug.
Signed-off-by: Sandipan Das
---
tools/testing/selftests/powerpc/mm/.gitignore | 1 +
tools/testing/selftests/powerpc/mm/Makefile | 5 +-
.../selftests/powerpc/mm/pkey_siginfo.c | 332 ++
3 files changed, 337 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/mm/pkey_siginfo.c
diff --git a/tools/testing/selftests/powerpc/mm/.gitignore
b/tools/testing/selftests/powerpc/mm/.gitignore
index 8f841f925baa5..36ec2c4ccdea4 100644
--- a/tools/testing/selftests/powerpc/mm/.gitignore
+++ b/tools/testing/selftests/powerpc/mm/.gitignore
@@ -9,3 +9,4 @@ large_vm_fork_separation
bad_accesses
tlbie_test
pkey_exec_prot
+pkey_siginfo
diff --git a/tools/testing/selftests/powerpc/mm/Makefile
b/tools/testing/selftests/powerpc/mm/Makefile
index f9fa0ba7435c4..558b7ccc93932 100644
--- a/tools/testing/selftests/powerpc/mm/Makefile
+++ b/tools/testing/selftests/powerpc/mm/Makefile
@@ -3,7 +3,8 @@ noarg:
$(MAKE) -C ../
TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors
wild_bctr \
- large_vm_fork_separation bad_accesses pkey_exec_prot
+ large_vm_fork_separation bad_accesses pkey_exec_prot \
+ pkey_siginfo
TEST_GEN_PROGS_EXTENDED := tlbie_test
TEST_GEN_FILES := tempfile
@@ -18,8 +19,10 @@ $(OUTPUT)/wild_bctr: CFLAGS += -m64
$(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
$(OUTPUT)/bad_accesses: CFLAGS += -m64
$(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
+$(OUTPUT)/pkey_siginfo: CFLAGS += -m64
$(OUTPUT)/tempfile:
dd if=/dev/zero of=$@ bs=64k count=1
$(OUTPUT)/tlbie_test: LDLIBS += -lpthread
+$(OUTPUT)/pkey_siginfo: LDLIBS += -lpthread
diff --git a/tools/testing/selftests/powerpc/mm/pkey_siginfo.c
b/tools/testing/selftests/powerpc/mm/pkey_siginfo.c
new file mode 100644
index 0..58605c53d495d
--- /dev/null
+++ b/tools/testing/selftests/powerpc/mm/pkey_siginfo.c
@@ -0,0 +1,332 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2020, Sandipan Das, IBM Corp.
+ *
+ * Test if the signal information reports the correct memory protection
+ * key upon getting a key access violation fault for a page that was
+ * attempted to be protected by two different keys from two competing
+ * threads at the same time.
+ */
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "pkeys.h"
+
+#define PPC_INST_NOP 0x6000
+#define PPC_INST_BLR 0x4e800020
+#define PROT_RWX (PROT_READ | PROT_WRITE | PROT_EXEC)
+
+#define NUM_ITERATIONS 100
+
+static volatile sig_atomic_t perm_pkey, rest_pkey;
+static volatile sig_atomic_t rights, fault_count;
+static volatile unsigned int *volatile fault_addr;
+static pthread_barrier_t iteration_barrier;
+
+static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
+{
+ void *pgstart;
+ size_t pgsize;
+ int pkey;
+
+ pkey = siginfo_pkey(sinfo);
+
+ /* Check if this fault originated from a pkey access violation */
+ if (sinfo->si_code != SEGV_PKUERR) {
+ sigsafe_err("got a fault for an unexpected reason\n");
+ _exit(1);
+ }
+
+ /* Check if this fault originated from the expected address */
+ if (sinfo->si_addr != (void *) fault_addr) {
+ sigsafe_err("got a fault for an unexpected address\n");
+ _exit(1);
+ }
+
+ /* Check if this fault originated from the restrictive pkey */
+ if (pkey != rest_pkey) {
+ sigsafe_err("got a fault for an unexpected pkey\n");
+ _exit(1);
+ }
+
+ /* Check if too many faults have occurred for the same iteration */
+ if (fault_count > 0) {
+ sigsafe_err("got too many faults for the same address\n");
+ _exit(1);
+ }
+
+ pgsize = getpagesize();
+ pgstart = (void *) ((unsigned long) fault_addr & ~(pgsize - 1));
+
+ /*
+* If the current fault occurred due to lack of execute rights,
+* reassociate the page with the exec-only pkey since execute
+* rights cannot be changed directly for the faulting pkey as
+* IAMR is inaccessible from userspace.
+*
+* Otherwise, if the current fault occurred due to lack of
+* read-write rights, change the AMR permission bits for the
+* pkey.
+*
+* This will let the test continue.
+*/
+ if (rights == PKEY_DISABLE_EXECUTE &&
+ mprotect(pgstart, pgsize, PROT_EXEC))
+ _exit(1);
+ else
+
[PATCH] selftests/powerpc: Run per_event_excludes test on Power8 or later
The per_event_excludes test wants to run on Power8 or later. But
currently it checks that AT_BASE_PLATFORM *equals* power8, which means
it only runs on Power8.
Fix it to check for the ISA 2.07 feature, which will be set on Power8
and later CPUs.
Signed-off-by: Michael Ellerman
---
tools/testing/selftests/powerpc/pmu/per_event_excludes.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/powerpc/pmu/per_event_excludes.c
b/tools/testing/selftests/powerpc/pmu/per_event_excludes.c
index 2756fe2efdc5..2d37942bf72b 100644
--- a/tools/testing/selftests/powerpc/pmu/per_event_excludes.c
+++ b/tools/testing/selftests/powerpc/pmu/per_event_excludes.c
@@ -12,6 +12,8 @@
#include
#include
+#include
+
#include "event.h"
#include "lib.h"
#include "utils.h"
@@ -23,12 +25,9 @@
static int per_event_excludes(void)
{
struct event *e, events[4];
- char *platform;
int i;
- platform = (char *)get_auxv_entry(AT_BASE_PLATFORM);
- FAIL_IF(!platform);
- SKIP_IF(strcmp(platform, "power8") != 0);
+ SKIP_IF(!have_hwcap2(PPC_FEATURE2_ARCH_2_07));
/*
* We need to create the events disabled, otherwise the running/enabled
--
2.25.1
Re: [PATCH] powerpc/book3s64/pkeys: Fix pkey_access_permitted w.r.t execute disable pkey
On Sun, 12 Jul 2020 18:50:47 +0530, Aneesh Kumar K.V wrote: > Even if the IAMR value deny the execute access, current kernel return true > w.r.t pkey_access_permitted() for execute permission check if the AMR > read pkey bit is cleared. > > This results in repeated page fault loop with a test like below. > > #define _GNU_SOURCE > #include > #include > #include > #include > #include > > [...] Applied to powerpc/fixes. [1/1] powerpc/book3s64/pkeys: Fix pkey_access_permitted() for execute disable pkey https://git.kernel.org/powerpc/c/192b6a780598976feb7321ff007754f8511a4129 cheers
Re: [PATCH V2 1/2] powerpc/vas: Report proper error code for address translation failure
On Fri, 10 Jul 2020 16:47:19 -0700, Haren Myneni wrote: > P9 DD2 NX workbook (Table 4-36) says DMA controller uses CC=5 > internally for translation fault handling. NX reserves CC=250 for > OS to notify user space when NX encounters address translation > failure on the request buffer. Not an issue in earlier releases > as NX does not get faults on kernel addresses. > > This patch defines CSB_CC_FAULT_ADDRESS(250) and updates CSB.CC with > this proper error code for user space. > > [...] Applied to powerpc/fixes. [1/2] powerpc/vas: Report proper error code for address translation failure https://git.kernel.org/powerpc/c/6068e1a4427e88f5cc62f238d1baf94a8b824ef4 [2/2] selftests/powerpc: Use proper error code to check fault address https://git.kernel.org/powerpc/c/f0479c4bcbd92d1a457d4a43bcab79f29d11334a cheers
Re: [PATCH V2] powerpc/pseries/svm: Remove unwanted check for shared_lppaca_size
On Fri, 19 Jun 2020 12:31:13 +0530, Satheesh Rajendran wrote: > Early secure guest boot hits the below crash while booting with > vcpus numbers aligned with page boundary for PAGE size of 64k > and LPPACA size of 1k i.e 64, 128 etc, due to the BUG_ON assert > for shared_lppaca_total_size equal to shared_lppaca_size, > > [0.00] Partition configured for 64 cpus. > [0.00] CPU maps initialized for 1 thread per core > [0.00] [ cut here ] > [0.00] kernel BUG at arch/powerpc/kernel/paca.c:89! > [0.00] Oops: Exception in kernel mode, sig: 5 [#1] > [0.00] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries > > [...] Applied to powerpc/fixes. [1/1] powerpc/pseries/svm: Fix incorrect check for shared_lppaca_size https://git.kernel.org/powerpc/c/b710d27bf72068b15b2f0305d825988183e2ff28 cheers
Re: [PATCH v7 0/7] Support new pmem flush and sync instructions for POWER
On Wed, 1 Jul 2020 12:52:28 +0530, Aneesh Kumar K.V wrote: > This patch series enables the usage os new pmem flush and sync instructions > on POWER > architecture. POWER10 introduces two new variants of dcbf instructions > (dcbstps and dcbfps) > that can be used to write modified locations back to persistent storage. > Additionally, > POWER10 also introduce phwsync and plwsync which can be used to establish > order of these > writes to persistent storage. > > This series exposes these instructions to the rest of the kernel. The existing > dcbf and hwsync instructions in P8 and P9 are adequate to enable appropriate > synchronization with OpenCAPI-hosted persistent storage. Hence the new > instructions > are added as a variant of the old ones that old hardware won't differentiate. > > [...] Applied to powerpc/next. [1/7] powerpc/pmem: Restrict papr_scm to P8 and above. https://git.kernel.org/powerpc/c/c83040192f3763b243ece26073d61a895b4a230f [2/7] powerpc/pmem: Add new instructions for persistent storage and sync https://git.kernel.org/powerpc/c/32db09d992ddc7d145595cff49cccfe14e018266 [3/7] powerpc/pmem: Add flush routines using new pmem store and sync instruction https://git.kernel.org/powerpc/c/d358042793183a57094dac45a44116e1165ac593 [4/7] libnvdimm/nvdimm/flush: Allow architecture to override the flush barrier https://git.kernel.org/powerpc/c/3e79f082ebfc130360bcee23e4dd74729dcafdf4 [5/7] powerpc/pmem: Update ppc64 to use the new barrier instruction. https://git.kernel.org/powerpc/c/76e6c73f33d4e1cc4de4f25c0bf66d59e42113c4 [6/7] powerpc/pmem: Avoid the barrier in flush routines https://git.kernel.org/powerpc/c/436499ab868f1a9e497cfdbf641affe8a122c571 [7/7] powerpc/pmem: Initialize pmem device on newer hardware https://git.kernel.org/powerpc/c/8c26ab72663b4affc31e47cdf77d61d0172d1033 cheers
Re: [PATCH] powerpc: Add cputime_to_nsecs()
On Mon, 13 Jul 2020 18:36:01 +1000, Anton Blanchard wrote: > Generic code has a wrapper to implement cputime_to_nsecs() on top of > cputime_to_usecs() but we can easily return the full nanosecond > resolution directly. Applied to powerpc/next. [1/1] powerpc: Add cputime_to_nsecs() https://git.kernel.org/powerpc/c/ade7667a981be49af9310f7c682c226283ec833d cheers
Re: [PATCH] powerpc/vdso: Fix vdso cpu truncation
On Thu, 16 Jul 2020 09:37:04 +1000, Anton Blanchard wrote: > The code in vdso_cpu_init that exposes the cpu and numa node to > userspace via SPRG_VDSO incorrctly masks the cpu to 12 bits. This means > that any kernel running on a box with more than 4096 threads (NR_CPUS > advertises a limit of of 8192 cpus) would expose userspace to two cpu > contexts running at the same time with the same cpu number. > > Note: I'm not aware of any distro shipping a kernel with support for more > than 4096 threads today, nor of any system image that currently exceeds > 4096 threads. Found via code browsing. Applied to powerpc/next. [1/1] powerpc/vdso: Fix vdso cpu truncation https://git.kernel.org/powerpc/c/a9f675f950a07d5c1dbcbb97aabac56f5ed085e3 cheers
Re: [PATCH] xmon: Reset RCU and soft lockup watchdogs
On Tue, 30 Jun 2020 10:02:18 +1000, Anton Blanchard wrote: > I'm seeing RCU warnings when exiting xmon. xmon resets the NMI watchdog, > but does nothing with the RCU stall or soft lockup watchdogs. Add a > helper function that handles all three. Applied to powerpc/next. [1/1] powerpc/xmon: Reset RCU and soft lockup watchdogs https://git.kernel.org/powerpc/c/5c699396f5f6cf6d67055af7b82c270d31fd831a cheers
Re: [PATCH] pseries: Fix 64 bit logical memory block panic
On Wed, 15 Jul 2020 10:08:20 +1000, Anton Blanchard wrote: > Booting with a 4GB LMB size causes us to panic: > > qemu-system-ppc64: OS terminated: OS panic: > Memory block size not suitable: 0x0 > > Fix pseries_memory_block_size() to handle 64 bit LMBs. Applied to powerpc/next. [1/1] pseries: Fix 64 bit logical memory block panic https://git.kernel.org/powerpc/c/89c140bbaeee7a55ed0360a88f294ead2b95201b cheers
Re: [PATCH v2 0/6] consolidate PowerPC instruction encoding macros
On Wed, 24 Jun 2020 17:00:32 +0530, Balamuruhan S wrote: > ppc-opcode.h have base instruction encoding wrapped with stringify_in_c() > for raw encoding to have compatibility. But there are redundant macros for > base instruction encodings in bpf, instruction emulation test infrastructure > and powerpc selftests. > > Currently PPC_INST_* macros are used for encoding instruction opcode and PPC_* > for raw instuction encoding, this rfc patchset introduces PPC_RAW_* macros for > base instruction encoding and reuse it from elsewhere. With this change we can > avoid redundant macro definitions in multiple files and start adding new > instructions in ppc-opcode.h in future. > > [...] Applied to powerpc/next. [1/6] powerpc/ppc-opcode: Introduce PPC_RAW_* macros for base instruction encoding https://git.kernel.org/powerpc/c/db551f8cc6a33f79cd2d2a6cfd1903f044e828a8 [2/6] powerpc/ppc-opcode: Move ppc instruction encoding from test_emulate_step https://git.kernel.org/powerpc/c/1d33dd84080f4a430bde2fc363d9b70f0a010c19 [3/6] powerpc/bpf_jit: Reuse instruction macros from ppc-opcode.h https://git.kernel.org/powerpc/c/0654186510a40e7e1fa788cb941d1a156ba2dcb2 [4/6] powerpc/ppc-opcode: Consolidate powerpc instructions from bpf_jit.h https://git.kernel.org/powerpc/c/3a181237916310b2bbbad158d97933bb2b4e7552 [5/6] powerpc/ppc-opcode: Reuse raw instruction macros to stringify https://git.kernel.org/powerpc/c/357c572948310c88868cee00e64ca3f7fc933a74 [6/6] powerpc/ppc-opcode: Fold PPC_INST_* macros into PPC_RAW_* macros https://git.kernel.org/powerpc/c/e4208f1399b1bf7ed84ba359a6ba0979d1df4029 cheers
Re: [PATCH] powerpc/spufs: add CONFIG_COREDUMP dependency
On Mon, 6 Jul 2020 15:22:46 +0200, Arnd Bergmann wrote:
> The kernel test robot pointed out a slightly different error message
> after recent commit 5456ffdee666 ("powerpc/spufs: simplify spufs core
> dumping") to spufs for a configuration that never worked:
>
>powerpc64-linux-ld: arch/powerpc/platforms/cell/spufs/file.o: in function
> `.spufs_proxydma_info_dump':
> >> file.c:(.text+0x4c68): undefined reference to `.dump_emit'
>powerpc64-linux-ld: arch/powerpc/platforms/cell/spufs/file.o: in function
> `.spufs_dma_info_dump':
>file.c:(.text+0x4d70): undefined reference to `.dump_emit'
>powerpc64-linux-ld: arch/powerpc/platforms/cell/spufs/file.o: in function
> `.spufs_wbox_info_dump':
>file.c:(.text+0x4df4): undefined reference to `.dump_emit'
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/spufs: add CONFIG_COREDUMP dependency
https://git.kernel.org/powerpc/c/b648a5132ca3237a0f1ce5d871fff342b0efcf8a
cheers
Re: [PATCH v3 0/3] Off-load TLB invalidations to host for !GTSE
On Fri, 3 Jul 2020 11:06:05 +0530, Bharata B Rao wrote: > Hypervisor may choose not to enable Guest Translation Shootdown Enable > (GTSE) option for the guest. When GTSE isn't ON, the guest OS isn't > permitted to use instructions like tblie and tlbsync directly, but is > expected to make hypervisor calls to get the TLB flushed. > > This series enables the TLB flush routines in the radix code to > off-load TLB flushing to hypervisor via the newly proposed hcall > H_RPT_INVALIDATE. > > [...] Applied to powerpc/next. [1/3] powerpc/mm: Enable radix GTSE only if supported. https://git.kernel.org/powerpc/c/029ab30b4c0a7ec587eece1ec07c3981fdff2bed [2/3] powerpc/pseries: H_REGISTER_PROC_TBL should ask for GTSE only if enabled https://git.kernel.org/powerpc/c/b6c84175078ff022b343b7b0737aeb33001ca90c [3/3] powerpc/mm/book3s64/radix: Off-load TLB invalidations to host when !GTSE https://git.kernel.org/powerpc/c/dd3d9aa5589c52efaec12ffeb84f0f5f8208fbc3 cheers
Re: [PATCH v2] powerpc: Drop CONFIG_MTD_M25P80 in 85xx-hw.config
On Fri, 1 May 2020 21:44:54 -0700, Bin Meng wrote:
> Drop CONFIG_MTD_M25P80 that was removed in
> commit b35b9a10362d ("mtd: spi-nor: Move m25p80 code in spi-nor.c")
Applied to powerpc/next.
[1/1] powerpc: Drop CONFIG_MTD_M25P80 in 85xx-hw.config
https://git.kernel.org/powerpc/c/76f09371bc05d6eb8d5a01823c9eaab768d6e934
cheers
Re: [PATCH 1/2] powerpc/signal_32: Remove !FULL_REGS() special handling in PPC64 save_general_regs()
On Tue, 7 Jul 2020 12:33:35 + (UTC), Christophe Leroy wrote:
> Since commit ("1bd79336a426 powerpc: Fix various
> syscall/signal/swapcontext bugs"), getting save_general_regs() called
> without FULL_REGS() is very unlikely and generates a warning.
>
> The 32-bit version of save_general_regs() doesn't take care of it
> at all and copies all registers anyway since that commit.
>
> [...]
Applied to powerpc/next.
[1/2] powerpc/signal_32: Remove !FULL_REGS() special handling in PPC64
save_general_regs()
https://git.kernel.org/powerpc/c/667e3c413ecf20371692fd2dc37e06dc14d0b140
[2/2] powerpc/signal_32: Simplify loop in PPC64 save_general_regs()
https://git.kernel.org/powerpc/c/020c4831e01264f8b62af6ca9e669b7c51881a56
cheers
Re: [PATCH v8 0/8] powerpc: switch VDSO to C implementation
On Tue, 28 Apr 2020 13:16:46 + (UTC), Christophe Leroy wrote: > This is the seventh version of a series to switch powerpc VDSO to > generic C implementation. > > Main changes since v7 are: > - Added gettime64 on PPC32 > > This series applies on today's powerpc/merge branch. > > [...] Patch 1 applied to powerpc/next. [1/8] powerpc/vdso64: Switch from __get_datapage() to get_datapage inline macro https://git.kernel.org/powerpc/c/793d74a8c78e05d6833bfcf582e24e40bd92518f cheers
Re: [PATCH] docs: powerpc: Clarify book3s/32 MMU families
On Thu, 2 Jul 2020 14:09:21 + (UTC), Christophe Leroy wrote: > Documentation wrongly tells that book3s/32 CPU have hash MMU. > > 603 and e300 core only have software loaded TLB. > > 755, 7450 family and e600 core have both hash MMU and software loaded > TLB. This can be selected by setting a bit in HID2 (755) or > HID0 (others). At the time being this is not supported by the kernel. > > [...] Applied to powerpc/next. [1/1] docs: powerpc: Clarify book3s/32 MMU families https://git.kernel.org/powerpc/c/7d38f089731fe129a49e254028caec6f05420f18 cheers
Re: [PATCH 1/2] Revert "powerpc/kasan: Fix shadow pages allocation failure"
On Thu, 2 Jul 2020 11:52:02 + (UTC), Christophe Leroy wrote: > This reverts commit d2a91cef9bbdeb87b7449fdab1a6be6000930210. > > This commit moved too much work in kasan_init(). The allocation > of shadow pages has to be moved for the reason explained in that > patch, but the allocation of page tables still need to be done > before switching to the final hash table. > > [...] Applied to powerpc/next. [1/2] Revert "powerpc/kasan: Fix shadow pages allocation failure" https://git.kernel.org/powerpc/c/b506923ee44ae87fc9f4de16b53feb313623e146 [2/2] powerpc/kasan: Fix shadow pages allocation failure https://git.kernel.org/powerpc/c/41ea93cf7ba4e0f0cc46ebfdda8b6ff27c67bc91 cheers
Re: [PATCH] selftests/powerpc: Purge extra count_pmc() calls of ebb selftests
On Fri, 26 Jun 2020 13:47:37 -0300, Desnes A. Nunes do Rosario wrote: > An extra count on ebb_state.stats.pmc_count[PMC_INDEX(pmc)] is being per- > formed when count_pmc() is used to reset PMCs on a few selftests. This > extra pmc_count can occasionally invalidate results, such as the ones from > cycles_test shown hereafter. The ebb_check_count() failed with an above > the upper limit error due to the extra value on ebb_state.stats.pmc_count. > > Furthermore, this extra count is also indicated by extra PMC1 trace_log on > the output of the cycle test (as well as on pmc56_overflow_test): > > [...] Applied to powerpc/next. [1/1] selftests/powerpc: Purge extra count_pmc() calls of ebb selftests https://git.kernel.org/powerpc/c/3337bf41e0dd70b4064cdf60acdfcdc2d050066c cheers
Re: [PATCH] powerpc/signal64: Don't opencode page prefaulting
On Tue, 7 Jul 2020 18:32:25 + (UTC), Christophe Leroy wrote: > Instead of doing a __get_user() from the first and last location > into a tmp var which won't be used, use fault_in_pages_readable() Applied to powerpc/next. [1/1] powerpc/signal64: Don't opencode page prefaulting https://git.kernel.org/powerpc/c/96032f983ca32ad1d43c73da922dbc7022754c3c cheers
Re: [PATCH v5] ocxl: control via sysfs whether the FPGA is reloaded on a link reset
On Fri, 19 Jun 2020 16:04:39 +0200, Frederic Barrat wrote: > Some opencapi FPGA images allow to control if the FPGA should be reloaded > on the next adapter reset. If it is supported, the image specifies it > through a Vendor Specific DVSEC in the config space of function 0. Applied to powerpc/next. [1/1] ocxl: control via sysfs whether the FPGA is reloaded on a link reset https://git.kernel.org/powerpc/c/87db7579ebd5ded337056eb765542eb2608f16e3 cheers
Re: [PATCH] ocxl: Replace HTTP links with HTTPS ones
On Mon, 13 Jul 2020 19:55:06 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. Applied to powerpc/next. [1/1] ocxl: Replace HTTP links with HTTPS ones https://git.kernel.org/powerpc/c/07497137a5efa9b2628c18083e8b07b33160153d cheers
Re: [PATCH] powerpc/Kconfig: Replace HTTP links with HTTPS ones
On Mon, 13 Jul 2020 21:26:56 +0200, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. Applied to powerpc/next. [1/1] powerpc/Kconfig: Replace HTTP links with HTTPS ones https://git.kernel.org/powerpc/c/9a3e3dccbf4317d02d28f8f99a5d1ccce42f9922 cheers
Re: [PATCH] cpuidle/powernv : Remove dead code block
On Mon, 6 Jul 2020 00:32:58 -0500, Abhishek Goel wrote: > Commit 1961acad2f88559c2cdd2ef67c58c3627f1f6e54 removes usage of > function "validate_dt_prop_sizes". This patch removes this unused > function. Applied to powerpc/next. [1/1] cpuidle/powernv : Remove dead code block https://git.kernel.org/powerpc/c/c339f9be304c21da1c42899a824f84a2cc9ced30 cheers
Re: [PATCH v5 0/2] Add cpu hotplug support for powerpc/perf/hv-24x7
On Thu, 9 Jul 2020 10:48:34 +0530, Kajol Jain wrote: > This patchset add cpu hotplug support for hv_24x7 driver by adding > online/offline cpu hotplug function. It also add sysfs file > "cpumask" to expose current online cpu that can be used for > hv_24x7 event count. > > Changelog: > v4 -> v5 > - Since we are making PMU fail incase hotplug init failed, hence > directly adding cpumask attr inside if_attrs rather then creating > new attribute_group as suggested by Madhavan Srinivasan. > > [...] Applied to powerpc/next. [1/2] powerpc/perf/hv-24x7: Add cpu hotplug support https://git.kernel.org/powerpc/c/1a8f0886a6008c98a926bdeca49f2ef33015a491 [2/2] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show cpumask https://git.kernel.org/powerpc/c/792f73f747b82f6cb191a323e1f5755d33149b50 cheers
Re: [PATCH] powerpc/boot: Use address-of operator on section symbols
On Tue, 23 Jun 2020 20:59:20 -0700, Nathan Chancellor wrote:
> Clang warns:
>
> arch/powerpc/boot/main.c:107:18: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_initrd_end > _initrd_start) {
> ^
> arch/powerpc/boot/main.c:155:20: warning: array comparison always
> evaluates to a constant [-Wtautological-compare]
> if (_esm_blob_end <= _esm_blob_start)
> ^
> 2 warnings generated.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/boot: Use address-of operator on section symbols
https://git.kernel.org/powerpc/c/df4232d96e724d09e54a623362f9f610727f059f
cheers
Re: [PATCH v2] powerpc/perf: Add kernel support for new MSR[HV PR] bits in trace-imc
On Mon, 13 Jul 2020 20:16:23 +0530, Madhavan Srinivasan wrote: > IMC trace-mode record has MSR[HV PR] bits added in the third DW. > These bits can be used to set the cpumode for the instruction pointer > captured in each sample. > > Add support in kernel to use these bits to set the cpumode for > each sample. Applied to powerpc/next. [1/1] powerpc/perf: Add kernel support for new MSR[HV PR] bits in trace-imc https://git.kernel.org/powerpc/c/77ca3951cc37727ae8361d583a30da7a1b84e427 cheers
Re: [PATCH] powerpc/boot/dts: Fix dtc "pciex" warnings
On Tue, 23 Jun 2020 23:03:20 +1000, Michael Ellerman wrote: > With CONFIG_OF_ALL_DTBS=y, as set by eg. allmodconfig, we see lots of > warnings about our dts files, such as: > > arch/powerpc/boot/dts/glacier.dts:492.26-532.5: > Warning (pci_bridge): /plb/pciex@d: node name is not "pci" > or "pcie" > > [...] Applied to powerpc/next. [1/1] powerpc/boot/dts: Fix dtc "pciex" warnings https://git.kernel.org/powerpc/c/86bc917d2ac117ec922dbf8ed92ca989bf333281 cheers
Re: [PATCH 00/18] remove extended cede offline mode and bogus topology update code
On Fri, 12 Jun 2020 00:12:20 -0500, Nathan Lynch wrote: > Two major parts to this series: > > 1. Removal of the extended cede offline mode for CPUs as well as the >partition suspend code which accommodates it by temporarily >onlining all CPUs prior to suspending the LPAR. This solves some >accounting problems, simplifies the pseries CPU hotplug code, and >greatly uncomplicates the existing partition suspend code, easing >a much-needed transition to the Linux suspend framework. The two >patches which make up this part have been posted before: > > [...] Applied to powerpc/next. [01/18] powerpc/pseries: remove cede offline state for CPUs https://git.kernel.org/powerpc/c/48f6e7f6d948b56489da027bc3284c709b939d28 [02/18] powerpc/rtas: don't online CPUs for partition suspend https://git.kernel.org/powerpc/c/ec2fc2a9e9bbad9023aab65bc472ce7a3ca8608f [03/18] powerpc/numa: remove ability to enable topology updates https://git.kernel.org/powerpc/c/c30f931e891eb0a32885ecd79984e1e7366fceda [04/18] powerpc/numa: remove unreachable topology update code https://git.kernel.org/powerpc/c/7d35bef96a46f7e9e167bb25258c0bd389aeab1b [05/18] powerpc/numa: make vphn_enabled, prrn_enabled flags const https://git.kernel.org/powerpc/c/e6eacf8eb4dee7bc7021c837666e3ebf1b0ec3b5 [06/18] powerpc/numa: remove unreachable topology timer code https://git.kernel.org/powerpc/c/50e0cf3742a01e72f4ea4a8fe9221b152e22871b [07/18] powerpc/numa: remove unreachable topology workqueue code https://git.kernel.org/powerpc/c/6325cb4a4ea8f4af8515b923650dd8f709694b44 [08/18] powerpc/numa: remove vphn_enabled and prrn_enabled internal flags https://git.kernel.org/powerpc/c/9fb8b5fd1bf782a8257506ad5198237f4124d556 [09/18] powerpc/numa: stub out numa_update_cpu_topology() https://git.kernel.org/powerpc/c/893ec6461f46c91487d914e6d467d2e804b9a883 [10/18] powerpc/numa: remove timed_topology_update() https://git.kernel.org/powerpc/c/b1815aeac7fde2dc3412daf2efaededd21cd58e0 [11/18] powerpc/numa: remove start/stop_topology_update() https://git.kernel.org/powerpc/c/1835303e5690cbeef2c07a9a5416045475ddaa13 [12/18] powerpc/rtasd: simplify handle_rtas_event(), emit message on events https://git.kernel.org/powerpc/c/91713ac377859893a7798999cb2e3a388d8ae710 [13/18] powerpc/numa: remove prrn_is_enabled() https://git.kernel.org/powerpc/c/042ef7cc43f4571d8cbe44a7c735ab6622809142 [14/18] powerpc/numa: remove arch_update_cpu_topology https://git.kernel.org/powerpc/c/cdf082c4570f186d608aca688f2cc872b014558a [15/18] powerpc/pseries: remove prrn special case from DT update path https://git.kernel.org/powerpc/c/bb7c3d36e3b18aa02d34358ae75e1b91f69a968b [16/18] powerpc/pseries: remove memory "re-add" implementation https://git.kernel.org/powerpc/c/4abe60c6448bf1dba48689450ad1348e5fc6f7b7 [17/18] powerpc/pseries: remove dlpar_cpu_readd() https://git.kernel.org/powerpc/c/38c392cef19019457ddcfb197ff3d9c5267698e6 [18/18] powerpc/pseries: remove obsolete memory hotplug DT notifier code https://git.kernel.org/powerpc/c/e978a3ccaa714b5ff125857d2cbecbb6fdf6c094 cheers
Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
On Mon, 11 May 2020 20:19:52 +1000, Nicholas Piggin wrote: > Returning from an interrupt or syscall to a signal handler currently > begins execution directly at the handler's entry point, with LR set to > the address of the sigreturn trampoline. When the signal handler > function returns, it runs the trampoline. It looks like this: > > # interrupt at user address xyz > # kernel stuff... signal is raised > rfid > # void handler(int sig) > addis 2,12,.TOC.-.LCF0@ha > addi 2,2,.TOC.-.LCF0@l > mflr 0 > std 0,16(1) > stdu 1,-96(1) > # handler stuff > ld 0,16(1) > mtlr 0 > blr > # __kernel_sigtramp_rt64 > addir1,r1,__SIGNAL_FRAMESIZE > li r0,__NR_rt_sigreturn > sc > # kernel executes rt_sigreturn > rfid > # back to user address xyz > > [...] Applied to powerpc/next. [1/1] powerpc/64/signal: Balance return predictor stack in signal trampoline https://git.kernel.org/powerpc/c/0138ba5783ae0dcc799ad401a1e8ac8333790df9 cheers
Re: [PATCH 0/7] powerpc: branch cache flush changes
On Tue, 9 Jun 2020 17:06:03 +1000, Nicholas Piggin wrote: > This series allows the link stack to be flushed with the speical > bcctr 2,0,0 flush instruction that also flushes the count cache if > the processor supports it. > > Firmware does not support this at the moment, but I've tested it in > simulator with a patched firmware to advertise support. > > [...] Patches 1-6 applied to powerpc/next. [1/7] powerpc/security: re-name count cache flush to branch cache flush https://git.kernel.org/powerpc/c/1026798c644bfd3115fc4e32fd5e767cfc30ccf1 [2/7] powerpc/security: change link stack flush state to the flush type enum https://git.kernel.org/powerpc/c/c06ac2771070f465076e87bba262c64fb0b3aca3 [3/7] powerpc/security: make display of branch cache flush more consistent https://git.kernel.org/powerpc/c/1afe00c74ffe6d502bffa81c7d849cb4640d7ae5 [4/7] powerpc/security: split branch cache flush toggle from code patching https://git.kernel.org/powerpc/c/c0036549a9d9a060fa8bc24e31f85503ce08ad5e [5/7] powerpc/64s: Move branch cache flushing bcctr variant to ppc-ops.h https://git.kernel.org/powerpc/c/70d7cdaf0548ec95fa7204dcdd39cd8e63cee24d [6/7] powerpc/security: Allow for processors that flush the link stack using the special bcctr https://git.kernel.org/powerpc/c/4d24e21cc694e7253a532fe5a9bde12b284f1317 cheers
Re: [PATCH v3 0/3] selftests: powerpc: Fixes and execute-disable test for pkeys
On Thu, 4 Jun 2020 18:26:07 +0530, Sandipan Das wrote: > This fixes the way the Authority Mask Register (AMR) is updated > by the existing pkey tests and adds a new test to verify the > functionality of execute-disabled pkeys. > > Previous versions can be found at: > v2: > https://lore.kernel.org/linuxppc-dev/20200527030342.13712-1-sandi...@linux.ibm.com/ > v1: > https://lore.kernel.org/linuxppc-dev/20200508162332.65316-1-sandi...@linux.ibm.com/ > > [...] Applied to powerpc/next. [1/3] selftests/powerpc: Fix pkey access right updates https://git.kernel.org/powerpc/c/828ca4320d130bbe1d12866152600c49ff6a9f79 [2/3] selftests/powerpc: Move Hash MMU check to utilities https://git.kernel.org/powerpc/c/c405b738daf9d8e8a5aedfeb6be851681e65e54b [3/3] selftests/powerpc: Add test for execute-disabled pkeys https://git.kernel.org/powerpc/c/1addb6444791f9e87fce0eb9882ec96a4a76e615 cheers
Re: [PATCH v3] powerpc/pseries: detect secure and trusted boot state of the system.
On Wed, 15 Jul 2020 07:52:01 -0400, Nayna Jain wrote: > The device-tree property to check secure and trusted boot state is > different for guests(pseries) compared to baremetal(powernv). > > This patch updates the existing is_ppc_secureboot_enabled() and > is_ppc_trustedboot_enabled() functions to add support for pseries. > > The secureboot and trustedboot state are exposed via device-tree property: > /proc/device-tree/ibm,secure-boot and /proc/device-tree/ibm,trusted-boot > > [...] Applied to powerpc/next. [1/1] powerpc/pseries: Detect secure and trusted boot state of the system. https://git.kernel.org/powerpc/c/61f879d97ce4510dd29d676a20d67692e3b34806 cheers
Re: [PATCH 1/2] powerpc/powernv: Make pnv_pci_sriov_enable() and friends static
On Sun, 5 Jul 2020 23:35:56 +1000, Oliver O'Halloran wrote: > The kernel test robot noticed these are non-static which causes Clang to > print some warnings. These are called via ppc_md function pointers so > there's no need for them to be non-static. Applied to powerpc/next. [1/2] powerpc/powernv: Make pnv_pci_sriov_enable() and friends static https://git.kernel.org/powerpc/c/93eacd94e09db2b1bb0343f8115385e5c34abf0a [2/2] powerpc/powernv: Move pnv_ioda_setup_bus_dma under CONFIG_IOMMU_API https://git.kernel.org/powerpc/c/e3417faec526cbf97773dca691dcd743f5bfeb64 cheers
Re: [PATCH 1/3] powerpc/64s: restore_math remove TM test
On Wed, 24 Jun 2020 09:41:37 +1000, Nicholas Piggin wrote:
> The TM test in restore_math added by commit dc16b553c949e ("powerpc:
> Always restore FPU/VEC/VSX if hardware transactional memory in use") is
> no longer necessary after commit a8318c13e79ba ("powerpc/tm: Fix
> restoring FP/VMX facility incorrectly on interrupts"), which removed
> the cases where restore_math has to restore if TM is active.
Applied to powerpc/next.
[1/3] powerpc/64s: restore_math remove TM test
https://git.kernel.org/powerpc/c/891b4fe8fe3d09f20948b391f24c9fc5b7580a2b
[2/3] powerpc/64s: Fix restore_math unnecessarily changing MSR
https://git.kernel.org/powerpc/c/01eb01877f3386d4bd5de75909abdd0af45a5fa2
[3/3] powerpc: re-initialise lazy FPU/VEC counters on every fault
https://git.kernel.org/powerpc/c/b2b46304e9360f3dda49c9d8ba4a1478b9eecf1d
cheers
Re: [PATCH 1/1] MAINTAINERS: Remove self
On Tue, 30 Jun 2020 08:50:44 +1000, Sam Bobroff wrote: > I'm sorry to say I can no longer maintain this position. Applied to powerpc/next. [1/1] MAINTAINERS: Remove self from powerpc EEH https://git.kernel.org/powerpc/c/a984c1f2e49225b40f1d0d20d383ec27d4d0 cheers
Re: [PATCH v6] powerpc/fadump: fix race between pstore write and fadump crash trigger
On Mon, 13 Jul 2020 10:54:35 +0530, Sourabh Jain wrote: > When we enter into fadump crash path via system reset we fail to update > the pstore. > > On the system reset path we first update the pstore then we go for fadump > crash. But the problem here is when all the CPUs try to get the pstore > lock to initiate the pstore write, only one CPUs will acquire the lock > and proceed with the pstore write. Since it in NMI context CPUs that fail > to get lock do not wait for their turn to write to the pstore and simply > proceed with the next operation which is fadump crash. One of the CPU who > proceeded with fadump crash path triggers the crash and does not wait for > the CPU who gets the pstore lock to complete the pstore update. > > [...] Applied to powerpc/next. [1/1] powerpc/fadump: fix race between pstore write and fadump crash trigger https://git.kernel.org/powerpc/c/ba608c4fa12cfd0cab0e153249c29441f4dd3312 cheers
Re: [PATCH -next] powerpc/xive: Remove unused inline function xive_kexec_teardown_cpu()
On Wed, 15 Jul 2020 10:50:40 +0800, YueHaibing wrote:
> commit e27e0a94651e ("powerpc/xive: Remove xive_kexec_teardown_cpu()")
> left behind this, remove it.
Applied to powerpc/next.
[1/1] powerpc/xive: Remove unused inline function xive_kexec_teardown_cpu()
https://git.kernel.org/powerpc/c/29d9407e1037868b59d12948d42ad3ef58fc3a5a
cheers
Re: [PATCH -next] cpuidle/pseries: Make symbol 'pseries_idle_driver' static
On Tue, 14 Jul 2020 22:24:24 +0800, Wei Yongjun wrote: > The sparse tool complains as follows: > > drivers/cpuidle/cpuidle-pseries.c:25:23: warning: > symbol 'pseries_idle_driver' was not declared. Should it be static? > > 'pseries_idle_driver' is not used outside of this file, so marks > it static. Applied to powerpc/next. [1/1] cpuidle/pseries: Make symbol 'pseries_idle_driver' static https://git.kernel.org/powerpc/c/92fe8483b1660feaa602d8be6ca7efe95ae4789b cheers
Re: [PATCH 0/3] Implement shared_cpu_list for powerpc
On Mon, 29 Jun 2020 16:07:00 +0530, Srikar Dronamraju wrote: > shared_cpu_list sysfs file is missing in powerpc and shared_cpu_map gives an > extra newline character. > > Before this patchset > # ls /sys/devices/system/cpu0/cache/index1 > coherency_line_size number_of_sets size ways_of_associativity > levelshared_cpu_map type > # cat /sys/devices/system/cpu0/cache/index1/shared_cpu_map > 00ff > > [...] Applied to powerpc/next. [1/3] powerpc/cacheinfo: Use cpumap_print to print cpumap https://git.kernel.org/powerpc/c/5658cf085ba3c3f3c24ac0f7210f0473794df506 [2/3] powerpc/cacheinfo: Make cpumap_show code reusable https://git.kernel.org/powerpc/c/74b7492e417812ea0f5002e210e2ac07a5728d17 [3/3] powerpc/cacheinfo: Add per cpu per index shared_cpu_list https://git.kernel.org/powerpc/c/a87a77cb947cc9fc89f0dad51aeee66a61cc7fc4 cheers
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
"Aneesh Kumar K.V" writes:
> This is the next version of the fixes for memory unplug on radix.
> The issues and the fix are described in the actual patches.
I guess this isn't actually causing problems at runtime right now, but I
notice calls to resize_hpt_for_hotplug() from arch_add_memory() and
arch_remove_memory(), which ought to be mmu-agnostic:
int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)
{
unsigned long start_pfn = start >> PAGE_SHIFT;
unsigned long nr_pages = size >> PAGE_SHIFT;
int rc;
resize_hpt_for_hotplug(memblock_phys_mem_size());
start = (unsigned long)__va(start);
rc = create_section_mapping(start, start + size, nid,
params->pgprot);
...
Re: [PATCH] pseries: Fix 64 bit logical memory block panic
On 7/16/20 7:00 AM, Paul Mackerras wrote:
On Wed, Jul 15, 2020 at 06:12:25PM +0530, Aneesh Kumar K.V wrote:
Anton Blanchard writes:
Booting with a 4GB LMB size causes us to panic:
qemu-system-ppc64: OS terminated: OS panic:
Memory block size not suitable: 0x0
Fix pseries_memory_block_size() to handle 64 bit LMBs.
Cc: sta...@vger.kernel.org
Signed-off-by: Anton Blanchard
---
arch/powerpc/platforms/pseries/hotplug-memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 5ace2f9a277e..6574ac33e887 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -27,7 +27,7 @@ static bool rtas_hp_event;
unsigned long pseries_memory_block_size(void)
{
struct device_node *np;
- unsigned int memblock_size = MIN_MEMORY_BLOCK_SIZE;
+ uint64_t memblock_size = MIN_MEMORY_BLOCK_SIZE;
struct resource r;
np = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
We need similar changes at more places?
modified arch/powerpc/include/asm/book3s/64/mmu.h
@@ -85,7 +85,7 @@ extern unsigned int mmu_base_pid;
/*
* memory block size used with radix translation.
*/
-extern unsigned int __ro_after_init radix_mem_block_size;
+extern unsigned long __ro_after_init radix_mem_block_size;
#define PRTB_SIZE_SHIFT (mmu_pid_bits + 4)
#define PRTB_ENTRIES (1ul << mmu_pid_bits)
modified arch/powerpc/include/asm/drmem.h
@@ -21,7 +21,7 @@ struct drmem_lmb {
struct drmem_lmb_info {
struct drmem_lmb*lmbs;
int n_lmbs;
- u32 lmb_size;
+ u64 lmb_size;
};
extern struct drmem_lmb_info *drmem_info;
modified arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -34,7 +34,7 @@
unsigned int mmu_pid_bits;
unsigned int mmu_base_pid;
-unsigned int radix_mem_block_size __ro_after_init;
+unsigned long radix_mem_block_size __ro_after_init;
These changes look fine.
static __ref void *early_alloc_pgtable(unsigned long size, int nid,
unsigned long region_start, unsigned long region_end)
modified arch/powerpc/mm/drmem.c
@@ -268,14 +268,15 @@ static void __init __walk_drmem_v2_lmbs(const __be32
*prop, const __be32 *usm,
void __init walk_drmem_lmbs_early(unsigned long node,
void (*func)(struct drmem_lmb *, const __be32 **))
{
+ const __be64 *lmb_prop;
const __be32 *prop, *usm;
int len;
- prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
- if (!prop || len < dt_root_size_cells * sizeof(__be32))
+ lmb_prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
+ if (!lmb_prop || len < sizeof(__be64))
return;
- drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+ drmem_info->lmb_size = be64_to_cpup(lmb_prop);
This particular change shouldn't be necessary. We already have
dt_mem_next_cell() returning u64, and it knows how to combine two
cells to give a u64 (for dt_root_size_cells == 2).
agreed. I added it here because in another patch i was confused about
the usage of dt_root_size_cells. We don't generally use that in other
device tree parsing code. I will move that to a separate patch as cleanup.
usm = of_get_flat_dt_prop(node, "linux,drconf-usable-memory", &len);
@@ -296,19 +297,19 @@ void __init walk_drmem_lmbs_early(unsigned long node,
static int __init init_drmem_lmb_size(struct device_node *dn)
{
- const __be32 *prop;
+ const __be64 *prop;
int len;
if (drmem_info->lmb_size)
return 0;
prop = of_get_property(dn, "ibm,lmb-size", &len);
- if (!prop || len < dt_root_size_cells * sizeof(__be32)) {
+ if (!prop || len < sizeof(__be64)) {
pr_info("Could not determine LMB size\n");
return -1;
}
- drmem_info->lmb_size = dt_mem_next_cell(dt_root_size_cells, &prop);
+ drmem_info->lmb_size = be64_to_cpup(prop);
Same comment here.
-aneesh
Re: [PATCH V5 1/4] mm/debug_vm_pgtable: Add tests validating arch helpers for core MM features
On 13/07/2020 04:23, Anshuman Khandual wrote:
This adds new tests validating arch page table helpers for these following
core memory features. These tests create and test specific mapping types at
various page table levels.
1. SPECIAL mapping
2. PROTNONE mapping
3. DEVMAP mapping
4. SOFTDIRTY mapping
5. SWAP mapping
6. MIGRATION mapping
7. HUGETLB mapping
8. THP mapping
Cc: Andrew Morton
Cc: Gerald Schaefer
Cc: Christophe Leroy
Cc: Mike Rapoport
Cc: Vineet Gupta
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: Kirill A. Shutemov
Cc: Paul Walmsley
Cc: Palmer Dabbelt
Cc: linux-snps-...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s...@vger.kernel.org
Cc: linux-ri...@lists.infradead.org
Cc: x...@kernel.org
Cc: linux...@kvack.org
Cc: linux-a...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Tested-by: Vineet Gupta #arc
Reviewed-by: Zi Yan
Suggested-by: Catalin Marinas
Signed-off-by: Anshuman Khandual
---
mm/debug_vm_pgtable.c | 302 +-
1 file changed, 301 insertions(+), 1 deletion(-)
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 61ab16fb2e36..2fac47db3eb7 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
[...]
+
+static void __init pte_swap_tests(unsigned long pfn, pgprot_t prot)
+{
+ swp_entry_t swp;
+ pte_t pte;
+
+ pte = pfn_pte(pfn, prot);
+ swp = __pte_to_swp_entry(pte);
Minor issue: this doesn't look necessarily valid - there's no reason a
normal PTE can be turned into a swp_entry. In practise this is likely to
work on all architectures because there's no reason not to use (at
least) all the PFN bits for the swap entry, but it doesn't exactly seem
correct.
Can we start with a swp_entry_t (from __swp_entry()) and check the round
trip of that?
It would also seem sensible to have a check that
is_swap_pte(__swp_entry_to_pte(__swp_entry(x,y))) is true.
+ pte = __swp_entry_to_pte(swp);
+ WARN_ON(pfn != pte_pfn(pte));
+}
+
+#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
+static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
+{
+ swp_entry_t swp;
+ pmd_t pmd;
+
+ pmd = pfn_pmd(pfn, prot);
+ swp = __pmd_to_swp_entry(pmd);
+ pmd = __swp_entry_to_pmd(swp);
+ WARN_ON(pfn != pmd_pfn(pmd));
+}
+#else /* !CONFIG_ARCH_ENABLE_THP_MIGRATION */
+static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot) { }
+#endif /* CONFIG_ARCH_ENABLE_THP_MIGRATION */
+
+static void __init swap_migration_tests(void)
+{
+ struct page *page;
+ swp_entry_t swp;
+
+ if (!IS_ENABLED(CONFIG_MIGRATION))
+ return;
+ /*
+* swap_migration_tests() requires a dedicated page as it needs to
+* be locked before creating a migration entry from it. Locking the
+* page that actually maps kernel text ('start_kernel') can be real
+* problematic. Lets allocate a dedicated page explicitly for this
NIT: s/Lets/Let's
Otherwise looks good to me.
Steve
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
- On Jul 16, 2020, at 7:00 AM, Peter Zijlstra pet...@infradead.org wrote: > On Thu, Jul 16, 2020 at 08:03:36PM +1000, Nicholas Piggin wrote: >> Excerpts from Peter Zijlstra's message of July 16, 2020 6:50 pm: >> > On Wed, Jul 15, 2020 at 10:18:20PM -0700, Andy Lutomirski wrote: >> >> > On Jul 15, 2020, at 9:15 PM, Nicholas Piggin wrote: > >> >> But I’m wondering if all this deferred sync stuff is wrong. In the >> >> brave new world of io_uring and such, perhaps kernel access matter >> >> too. Heck, even: >> > >> > IIRC the membarrier SYNC_CORE use-case is about user-space >> > self-modifying code. >> > >> > Userspace re-uses a text address and needs to SYNC_CORE before it can be >> > sure the old text is forgotten. Nothing the kernel does matters there. >> > >> > I suppose the manpage could be more clear there. >> >> True, but memory ordering of kernel stores from kernel threads for >> regular mem barrier is the concern here. >> >> Does io_uring update completion queue from kernel thread or interrupt, >> for example? If it does, then membarrier will not order such stores >> with user memory accesses. > > So we're talking about regular membarrier() then? Not the SYNC_CORE > variant per-se. > > Even there, I'll argue we don't care, but perhaps Mathieu has a > different opinion. I agree with Peter that we don't care about accesses to user-space memory performed concurrently with membarrier. What we'd care about in terms of accesses to user-space memory from the kernel is something that would be clearly ordered as happening before or after the membarrier call, for instance a read(2) followed by membarrier(2) after the read returns, or a read(2) issued after return from membarrier(2). The other scenario we'd care about is with the compiler barrier paired with membarrier: e.g. read(2) returns, compiler barrier, followed by a store. Or load, compiler barrier, followed by write(2). All those scenarios imply before/after ordering wrt either membarrier or the compiler barrier. I notice that io_uring has a "completion" queue. Let's try to come up with realistic usage scenarios. So the dependency chain would be provided by e.g.: * Infrequent read / Frequent write, communicating read completion through variable X wait for io_uring read request completion -> membarrier -> store X=1 with matching load from X (waiting for X==1) -> asm volatile (::: "memory") -> submit io_uring write request or this other scenario: * Frequent read / Infrequent write, communicating read completion through variable X load from X (waiting for X==1) -> membarrier -> submit io_uring write request with matching wait for io_uring read request completion -> asm volatile (::: "memory") -> store X=1 Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
- On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com wrote: > I should be more complete here, especially since I was complaining > about unclear barrier comment :) > > > CPU0 CPU1 > a. user stuff1. user stuff > b. membarrier() 2. enter kernel > c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule > d. read rq->curr 4. rq->curr switched to kthread > e. is kthread, skip IPI 5. switch_to kthread > f. return to user6. rq->curr switched to user thread > g. user stuff7. switch_to user thread > 8. exit kernel > 9. more user stuff > > What you're really ordering is a, g vs 1, 9 right? > > In other words, 9 must see a if it sees g, g must see 1 if it saw 9, > etc. > > Userspace does not care where the barriers are exactly or what kernel > memory accesses might be being ordered by them, so long as there is a > mb somewhere between a and g, and 1 and 9. Right? This is correct. Note that the accesses to user-space memory can be done either by user-space code or kernel code, it doesn't matter. However, in order to be considered as happening before/after either membarrier or the matching compiler barrier, kernel code needs to have causality relationship with user-space execution, e.g. user-space does a system call, or returns from a system call. In the case of io_uring, submitting a request or returning from waiting on request completion appear to provide this causality relationship. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH net-next] ibmvnic: Increase driver logging
On 7/15/20 8:29 PM, David Miller wrote: From: Jakub Kicinski Date: Wed, 15 Jul 2020 17:06:32 -0700 On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote: free_netdev(netdev); dev_set_drvdata(&dev->dev, NULL); + netdev_info(netdev, "VNIC client device has been successfully removed.\n"); A step too far, perhaps. In general this patch looks a little questionable IMHO, this amount of logging output is not commonly seen in drivers. All the the info messages are just static text, not even carrying any extra information. In an era of ftrace, and bpftrace, do we really need this? Agreed, this is too much. This is debugging, and thus suitable for tracing facilities, at best. Thanks for your feedback. I see now that I was overly aggressive with this patch to be sure, but it would help with narrowing down problems at a first glance, should they arise. The driver in its current state logs very little of what is it doing without the use of additional debugging or tracing facilities. Would it be worth it to pursue a less aggressive version or would that be dead on arrival? What are acceptable driver operations to log at this level? Thanks, Tom
Re: [PATCH net-next] ibmvnic: Increase driver logging
On Thu, Jul 16, 2020 at 10:59:58AM -0500, Thomas Falcon wrote: > > On 7/15/20 8:29 PM, David Miller wrote: > > From: Jakub Kicinski > > Date: Wed, 15 Jul 2020 17:06:32 -0700 > > > > > On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote: > > > > free_netdev(netdev); > > > > dev_set_drvdata(&dev->dev, NULL); > > > > + netdev_info(netdev, "VNIC client device has been successfully > > > > removed.\n"); > > > A step too far, perhaps. > > > > > > In general this patch looks a little questionable IMHO, this amount of > > > logging output is not commonly seen in drivers. All the the info > > > messages are just static text, not even carrying any extra information. > > > In an era of ftrace, and bpftrace, do we really need this? > > Agreed, this is too much. This is debugging, and thus suitable for tracing > > facilities, at best. > > Thanks for your feedback. I see now that I was overly aggressive with this > patch to be sure, but it would help with narrowing down problems at a first > glance, should they arise. The driver in its current state logs very little > of what is it doing without the use of additional debugging or tracing > facilities. Would it be worth it to pursue a less aggressive version or > would that be dead on arrival? What are acceptable driver operations to log > at this level? Also would it be advisable to add the messages as pr_dbg to be enabled on demand? Thanks Michal
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
- On Jul 16, 2020, at 11:46 AM, Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: > - On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com wrote: >> I should be more complete here, especially since I was complaining >> about unclear barrier comment :) >> >> >> CPU0 CPU1 >> a. user stuff1. user stuff >> b. membarrier() 2. enter kernel >> c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule >> d. read rq->curr 4. rq->curr switched to kthread >> e. is kthread, skip IPI 5. switch_to kthread >> f. return to user6. rq->curr switched to user thread >> g. user stuff7. switch_to user thread >> 8. exit kernel >> 9. more user stuff >> >> What you're really ordering is a, g vs 1, 9 right? >> >> In other words, 9 must see a if it sees g, g must see 1 if it saw 9, >> etc. >> >> Userspace does not care where the barriers are exactly or what kernel >> memory accesses might be being ordered by them, so long as there is a >> mb somewhere between a and g, and 1 and 9. Right? > > This is correct. Actually, sorry, the above is not quite right. It's been a while since I looked into the details of membarrier. The smp_mb() at the beginning of membarrier() needs to be paired with a smp_mb() _after_ rq->curr is switched back to the user thread, so the memory barrier is between store to rq->curr and following user-space accesses. The smp_mb() at the end of membarrier() needs to be paired with the smp_mb__after_spinlock() at the beginning of schedule, which is between accesses to userspace memory and switching rq->curr to kthread. As to *why* this ordering is needed, I'd have to dig through additional scenarios from https://lwn.net/Articles/573436/. Or maybe Paul remembers ? Thanks, Mathieu > Note that the accesses to user-space memory can be > done either by user-space code or kernel code, it doesn't matter. > However, in order to be considered as happening before/after > either membarrier or the matching compiler barrier, kernel code > needs to have causality relationship with user-space execution, > e.g. user-space does a system call, or returns from a system call. > > In the case of io_uring, submitting a request or returning from waiting > on request completion appear to provide this causality relationship. > > Thanks, > > Mathieu > > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH v3 0/3] Off-load TLB invalidations to host for !GTSE
On Fri, Jul 03, 2020 at 11:06:05AM +0530, Bharata B Rao wrote: > Hypervisor may choose not to enable Guest Translation Shootdown Enable > (GTSE) option for the guest. When GTSE isn't ON, the guest OS isn't > permitted to use instructions like tblie and tlbsync directly, but is > expected to make hypervisor calls to get the TLB flushed. > > This series enables the TLB flush routines in the radix code to > off-load TLB flushing to hypervisor via the newly proposed hcall > H_RPT_INVALIDATE. > > To easily check the availability of GTSE, it is made an MMU feature. > The OV5 handling and H_REGISTER_PROC_TBL hcall are changed to > handle GTSE as an optionally available feature and to not assume GTSE > when radix support is available. > > The actual hcall implementation for KVM isn't included in this > patchset and will be posted separately. > > Changes in v3 > = > - Fixed a bug in the hcall wrapper code where we were missing setting > H_RPTI_TYPE_NESTED while retrying the failed flush request with > a full flush for the nested case. > - s/psize_to_h_rpti/psize_to_rpti_pgsize > > v2: > https://lore.kernel.org/linuxppc-dev/20200626131000.5207-1-bhar...@linux.ibm.com/T/#t > > Bharata B Rao (2): > powerpc/mm: Enable radix GTSE only if supported. > powerpc/pseries: H_REGISTER_PROC_TBL should ask for GTSE only if > enabled > > Nicholas Piggin (1): > powerpc/mm/book3s64/radix: Off-load TLB invalidations to host when > !GTSE Reverting the whole series fixed random memory corruptions during boot on POWER9 PowerNV systems below. IBM 8335-GTH (ibm,witherspoon) POWER9, altivec supported 262144 MB memory, 2000 GB disk space .config: https://gitlab.com/cailca/linux-mm/-/blob/master/powerpc.config [9.338996][ T925] BUG: Unable to handle kernel instruction fetch (NULL pointer?) [9.339026][ T925] Faulting instruction address: 0x [9.339051][ T925] Oops: Kernel access of bad area, sig: 11 [#1] [9.339064][ T925] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 NUMA PowerNV [9.339098][ T925] Modules linked in: dm_mirror dm_region_hash dm_log dm_mod [9.339150][ T925] CPU: 92 PID: 925 Comm: (md-udevd) Not tainted 5.8.0-rc5-next-20200716 #3 [9.339186][ T925] NIP: LR: c021f2cc CTR: [9.339210][ T925] REGS: c000201cb52d79b0 TRAP: 0400 Not tainted (5.8.0-rc5-next-20200716) [9.339244][ T925] MSR: 900040009033 CR: 2492 XER: [9.339278][ T925] CFAR: c021f2c8 IRQMASK: 0 [9.339278][ T925] GPR00: c021f2cc c000201cb52d7c40 c5901000 c000201cb52d7ca8 [9.339278][ T925] GPR04: c0080ea60038 7fff 7fff [9.339278][ T925] GPR08: c000201cb50bd500 0003 [9.339278][ T925] GPR12: c000201fff694500 7fffa4a8a940 7fffa4a8a6c8 [9.339278][ T925] GPR16: 7fffa4a8a8f8 7fffa4a8a650 7fffa4a8a488 [9.339278][ T925] GPR20: 00050001 7fffa4a8a984 7fff ca4545cc [9.339278][ T925] GPR24: c0affe28 0166 [9.339278][ T925] GPR28: c000201cb52d7ca8 c0080ea6 c000201cc3b72600 7fff [9.339493][ T925] NIP [] 0x0 [9.339516][ T925] LR [c021f2cc] __seccomp_filter+0xec/0x530 bpf_dispatcher_nop_func at include/linux/bpf.h:567 (inlined by) bpf_prog_run_pin_on_cpu at include/linux/filter.h:597 (inlined by) seccomp_run_filters at kernel/seccomp.c:324 (inlined by) __seccomp_filter at kernel/seccomp.c:937 [9.339538][ T925] Call Trace: [9.339548][ T925] [c000201cb52d7c40] [c021f2cc] __seccomp_filter+0xec/0x530 (unreliable) [9.339566][ T925] [c000201cb52d7d50] [c0025af8] do_syscall_trace_enter+0xb8/0x470 do_seccomp at arch/powerpc/kernel/ptrace/ptrace.c:252 (inlined by) do_syscall_trace_enter at arch/powerpc/kernel/ptrace/ptrace.c:327 [9.339600][ T925] [c000201cb52d7dc0] [c002c8f8] system_call_exception+0x138/0x180 [9.339625][ T925] [c000201cb52d7e20] [c000c9e8] system_call_common+0xe8/0x214 [9.339648][ T925] Instruction dump: [9.339667][ T925] [9.339706][ T925] [9.339748][ T925] ---[ end trace d89eb80f9a6bc141 ]--- [ OK ] Started Journal Service. [ 10.452364][ T925] Kernel panic - not syncing: Fatal exception [ 11.876655][ T925] ---[ end Kernel panic - not syncing: Fatal exception ]--- There could also be lots of random userspace segfault like, [ 16.463545][ T771] rngd[771]: segfault (11) at 0 nip 0 lr 0 code 1 in rngd[
Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
Hello Prakhar,
Prakhar Srivastava writes:
> On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
>>
>> Prakhar Srivastava writes:
>>
>>> Powerpc has support to carry over the IMA measurement logs. Refatoring the
>>> non-architecture specific code out of arch/powerpc and into security/ima.
>>>
>>> The code adds support for reserving and freeing up of memory for IMA
>>> measurement
>>> logs.
>>
>> Last week, Mimi provided this feedback:
>>
>> "From your patch description, this patch should be broken up. Moving
>> the non-architecture specific code out of powerpc should be one patch.
>> Additional support should be in another patch. After each patch, the
>> code should work properly."
>>
>> That's not what you do here. You move the code, but you also make other
>> changes at the same time. This has two problems:
>>
>> 1. It makes the patch harder to review, because it's very easy to miss a
>> change.
>>
>> 2. If in the future a git bisect later points to this patch, it's not
>> clear whether the problem is because of the code movement, or because
>> of the other changes.
>>
>> When you move code, ideally the patch should only make the changes
>> necessary to make the code work at its new location. The patch which
>> does code movement should not cause any change in behavior.
>>
>> Other changes should go in separate patches, either before or after the
>> one moving the code.
>>
>> More comments below.
>>
> Hi Thiago,
>
> Apologies for the delayed response i was away for a few days.
> I am working on breaking up the changes so that its easier to review and
> update
> as well.
No problem.
>
> Thanks,
> Prakhar Srivastava
>
>>>
>>> ---
>>> arch/powerpc/include/asm/ima.h | 10 ---
>>> arch/powerpc/kexec/ima.c | 126 ++---
>>> security/integrity/ima/ima_kexec.c | 116 ++
>>> 3 files changed, 124 insertions(+), 128 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>>> index ead488cf3981..c29ec86498f8 100644
>>> --- a/arch/powerpc/include/asm/ima.h
>>> +++ b/arch/powerpc/include/asm/ima.h
>>> @@ -4,15 +4,6 @@
>>>
>>> struct kimage;
>>>
>>> -int ima_get_kexec_buffer(void **addr, size_t *size);
>>> -int ima_free_kexec_buffer(void);
>>> -
>>> -#ifdef CONFIG_IMA
>>> -void remove_ima_buffer(void *fdt, int chosen_node);
>>> -#else
>>> -static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
>>> -#endif
>>> -
>>> #ifdef CONFIG_IMA_KEXEC
>>> int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long
>>> load_addr,
>>> size_t size);
>>> @@ -22,7 +13,6 @@ int setup_ima_buffer(const struct kimage *image, void
>>> *fdt, int chosen_node);
>>> static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>>>int chosen_node)
>>> {
>>> - remove_ima_buffer(fdt, chosen_node);
>>> return 0;
>>> }
>>
>> This is wrong. Even if the currently running kernel doesn't have
>> CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
>> reservation from the FDT that is being prepared for the next kernel.
>>
>> This is because the IMA kexec buffer is useless for the next kernel,
>> regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
>> not. Keeping it around would be a waste of memory.
>>
> I will keep it in my next revision.
> My understanding was the reserved memory is freed and property removed when
> IMA
> loads the logs on init.
If CONFIG_IMA_KEXEC is set, then yes. If it isn't then that needs to
happen in the function above.
> During setup_fdt in kexec, a duplicate copy of the dt is
> used, but memory still needs to be allocated, thus the property itself
> indicats
> presence of reserved memory.
>
>>> @@ -179,13 +64,18 @@ int setup_ima_buffer(const struct kimage *image, void
>>> *fdt, int chosen_node)
>>> int ret, addr_cells, size_cells, entry_size;
>>> u8 value[16];
>>>
>>> - remove_ima_buffer(fdt, chosen_node);
>>
>> This is wrong, for the same reason stated above.
>>
>>> if (!image->arch.ima_buffer_size)
>>> return 0;
>>>
>>> - ret = get_addr_size_cells(&addr_cells, &size_cells);
>>> - if (ret)
>>> + ret = fdt_address_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> + return ret;
>>> + addr_cells = ret;
>>> +
>>> + ret = fdt_size_cells(fdt, chosen_node);
>>> + if (ret < 0)
>>> return ret;
>>> + size_cells = ret;
>>>
>>> entry_size = 4 * (addr_cells + size_cells);
>>>
>>
>> I liked this change. Thanks! I agree it's better to use
>> fdt_address_cells() and fdt_size_cells() here.
>>
>> But it should be in a separate patch. Either before or after the one
>> moving the code.
>>
>>> diff --git a/security/integrity/ima/ima_kexec.c
>>> b/security/integrity/ima/ima_kexec.c
>>> index 121de3e04af2..e1e6d6154015 100644
>>> --- a/security/integrity/ima/ima_kexec.c
>>> +++ b/security/integrity/ima/ima_ke
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
- On Jul 16, 2020, at 12:03 PM, Mathieu Desnoyers mathieu.desnoy...@efficios.com wrote: > - On Jul 16, 2020, at 11:46 AM, Mathieu Desnoyers > mathieu.desnoy...@efficios.com wrote: > >> - On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com wrote: >>> I should be more complete here, especially since I was complaining >>> about unclear barrier comment :) >>> >>> >>> CPU0 CPU1 >>> a. user stuff1. user stuff >>> b. membarrier() 2. enter kernel >>> c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule >>> d. read rq->curr 4. rq->curr switched to kthread >>> e. is kthread, skip IPI 5. switch_to kthread >>> f. return to user6. rq->curr switched to user thread >>> g. user stuff7. switch_to user thread >>> 8. exit kernel >>> 9. more user stuff >>> >>> What you're really ordering is a, g vs 1, 9 right? >>> >>> In other words, 9 must see a if it sees g, g must see 1 if it saw 9, >>> etc. >>> >>> Userspace does not care where the barriers are exactly or what kernel >>> memory accesses might be being ordered by them, so long as there is a >>> mb somewhere between a and g, and 1 and 9. Right? >> >> This is correct. > > Actually, sorry, the above is not quite right. It's been a while > since I looked into the details of membarrier. > > The smp_mb() at the beginning of membarrier() needs to be paired with a > smp_mb() _after_ rq->curr is switched back to the user thread, so the > memory barrier is between store to rq->curr and following user-space > accesses. > > The smp_mb() at the end of membarrier() needs to be paired with the > smp_mb__after_spinlock() at the beginning of schedule, which is > between accesses to userspace memory and switching rq->curr to kthread. > > As to *why* this ordering is needed, I'd have to dig through additional > scenarios from https://lwn.net/Articles/573436/. Or maybe Paul remembers ? Thinking further about this, I'm beginning to consider that maybe we have been overly cautious by requiring memory barriers before and after store to rq->curr. If CPU0 observes a CPU1's rq->curr->mm which differs from its own process (current) while running the membarrier system call, it necessarily means that CPU1 had to issue smp_mb__after_spinlock when entering the scheduler, between any user-space loads/stores and update of rq->curr. Requiring a memory barrier between update of rq->curr (back to current process's thread) and following user-space memory accesses does not seem to guarantee anything more than what the initial barrier at the beginning of __schedule already provides, because the guarantees are only about accesses to user-space memory. Therefore, with the memory barrier at the beginning of __schedule, just observing that CPU1's rq->curr differs from current should guarantee that a memory barrier was issued between any sequentially consistent instructions belonging to the current process on CPU1. Or am I missing/misremembering an important point here ? Thanks, Mathieu > > Thanks, > > Mathieu > > >> Note that the accesses to user-space memory can be >> done either by user-space code or kernel code, it doesn't matter. >> However, in order to be considered as happening before/after >> either membarrier or the matching compiler barrier, kernel code >> needs to have causality relationship with user-space execution, >> e.g. user-space does a system call, or returns from a system call. >> >> In the case of io_uring, submitting a request or returning from waiting >> on request completion appear to provide this causality relationship. >> >> Thanks, >> >> Mathieu >> >> >> -- >> Mathieu Desnoyers >> EfficiOS Inc. >> http://www.efficios.com > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
[PATCH] powerpc/64: Fix an out of date comment about MMIO ordering
From: Palmer Dabbelt This primitive has been renamed, but because it was spelled incorrectly in the first place it must have escaped the fixup patch. As far as I can tell this logic is still correct: smp_mb__after_spinlock() uses the default smp_mb() implementation, which is "sync" rather than "hwsync" but those are the same (though I'm not that familiar with PowerPC). Signed-off-by: Palmer Dabbelt --- arch/powerpc/kernel/entry_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index b3c9f15089b6..7b38b4daca93 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -357,7 +357,7 @@ _GLOBAL(_switch) * kernel/sched/core.c). * * Uncacheable stores in the case of involuntary preemption must -* be taken care of. The smp_mb__before_spin_lock() in __schedule() +* be taken care of. The smp_mb__after_spinlock() in __schedule() * is implemented as hwsync on powerpc, which orders MMIO too. So * long as there is an hwsync in the context switch path, it will * be executed on the source CPU after the task has performed -- 2.28.0.rc0.105.gf9edc3c819-goog
Re: [PATCH net-next] ibmvnic: Increase driver logging
On Thu, 16 Jul 2020 18:07:37 +0200 Michal Suchánek wrote: > On Thu, Jul 16, 2020 at 10:59:58AM -0500, Thomas Falcon wrote: > > On 7/15/20 8:29 PM, David Miller wrote: > > > From: Jakub Kicinski > > > Date: Wed, 15 Jul 2020 17:06:32 -0700 > > > > > > > On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote: > > > > > free_netdev(netdev); > > > > > dev_set_drvdata(&dev->dev, NULL); > > > > > + netdev_info(netdev, "VNIC client device has been successfully > > > > > removed.\n"); > > > > A step too far, perhaps. > > > > > > > > In general this patch looks a little questionable IMHO, this amount of > > > > logging output is not commonly seen in drivers. All the the info > > > > messages are just static text, not even carrying any extra information. > > > > In an era of ftrace, and bpftrace, do we really need this? > > > Agreed, this is too much. This is debugging, and thus suitable for > > > tracing > > > facilities, at best. > > > > Thanks for your feedback. I see now that I was overly aggressive with this > > patch to be sure, but it would help with narrowing down problems at a first > > glance, should they arise. The driver in its current state logs very little > > of what is it doing without the use of additional debugging or tracing > > facilities. Would it be worth it to pursue a less aggressive version or > > would that be dead on arrival? What are acceptable driver operations to log > > at this level? Sadly it's much more of an art than hard science. Most networking drivers will print identifying information when they probe the device and then only about major config changes or when link comes up or goes down. And obviously when anything unexpected, like an error happens, that's key. You seem to be adding start / end information for each driver init / deinit stage. I'd say try to focus on the actual errors you're trying to catch. > Also would it be advisable to add the messages as pr_dbg to be enabled on > demand? I personally have had a pretty poor experience with pr_debug() because CONFIG_DYNAMIC_DEBUG is not always enabled. Since you're just printing static text there shouldn't be much difference between pr_debug and ftrace and/or bpftrace, honestly. Again, slightly hard to advise not knowing what you're trying to catch.
Re: [PATCH v3 10/12] ppc64/kexec_file: prepare elfcore header for crashing kernel
On 16/07/20 7:52 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini writes:
>
>> /**
>> + * get_crash_memory_ranges - Get crash memory ranges. This list includes
>> + * first/crashing kernel's memory regions that
>> + * would be exported via an elfcore.
>> + * @mem_ranges: Range list to add the memory ranges to.
>> + *
>> + * Returns 0 on success, negative errno on error.
>> + */
>> +static int get_crash_memory_ranges(struct crash_mem **mem_ranges)
>> +{
>> +struct memblock_region *reg;
>> +struct crash_mem *tmem;
>> +int ret;
>> +
>> +for_each_memblock(memory, reg) {
>> +u64 base, size;
>> +
>> +base = (u64)reg->base;
>> +size = (u64)reg->size;
>> +
>> +/* Skip backup memory region, which needs a separate entry */
>> +if (base == BACKUP_SRC_START) {
>> +if (size > BACKUP_SRC_SIZE) {
>> +base = BACKUP_SRC_END + 1;
>> +size -= BACKUP_SRC_SIZE;
>> +} else
>> +continue;
>> +}
>> +
>> +ret = add_mem_range(mem_ranges, base, size);
>> +if (ret)
>> +goto out;
>> +
>> +/* Try merging adjacent ranges before reallocation attempt */
>> +if ((*mem_ranges)->nr_ranges == (*mem_ranges)->max_nr_ranges)
>> +sort_memory_ranges(*mem_ranges, true);
>> +}
>> +
>> +/* Reallocate memory ranges if there is no space to split ranges */
>> +tmem = *mem_ranges;
>> +if (tmem && (tmem->nr_ranges == tmem->max_nr_ranges)) {
>> +tmem = realloc_mem_ranges(mem_ranges);
>> +if (!tmem)
>> +goto out;
>> +}
>> +
>> +/* Exclude crashkernel region */
>> +ret = crash_exclude_mem_range(tmem, crashk_res.start, crashk_res.end);
>> +if (ret)
>> +goto out;
>> +
>> +ret = add_rtas_mem_range(mem_ranges);
>> +if (ret)
>> +goto out;
>> +
>> +ret = add_opal_mem_range(mem_ranges);
>> +if (ret)
>> +goto out;
>
> Maybe I'm confused, but don't you add the RTAS and OPAL regions as
> usable memory for the crashkernel? In that case they shouldn't show up
> in the core file.
kexec-tools does the same thing. I am not endorsing it but I was trying to stay
in parity to avoid breaking any userspace tools/commands. But as you rightly
pointed, this is NOT right. The right thing to do, to get the rtas/opal data at
the time of crash, is to have a backup region for them just like we have for
the first 64K memory. I was hoping to do that later.
Will check how userspace tools respond to dropping these regions. If that makes
the tools unhappy, will retain the regions with a FIXME. Sorry about the
confusion.
Thanks
Hari
Re: [PATCH v3 03/12] powerpc/kexec_file: add helper functions for getting memory ranges
On 15/07/20 5:19 am, Thiago Jung Bauermann wrote:
>
>
>
>> +/**
>> + * get_mem_rngs_size - Get the allocated size of mrngs based on
>> + * max_nr_ranges and chunk size.
>> + * @mrngs: Memory ranges.
>> + *
>> + * Returns the maximum no. of ranges.
>
> This isn't correct. It returns the maximum size of @mrngs.
True. Will update..
>
>
>> +/**
>> + * add_tce_mem_ranges - Adds tce-table range to the given memory ranges
>> list.
>> + * @mem_ranges: Range list to add the memory range(s) to.
>> + *
>> + * Returns 0 on success, negative errno on error.
>> + */
>> +int add_tce_mem_ranges(struct crash_mem **mem_ranges)
>> +{
>> +struct device_node *dn;
>> +int ret;
>> +
>> +for_each_node_by_type(dn, "pci") {
>> +u64 base;
>> +u32 size;
>> +
>> +ret = of_property_read_u64(dn, "linux,tce-base", &base);
>> +ret |= of_property_read_u32(dn, "linux,tce-size", &size);
>> +if (!ret)
>
> Shouldn't the condition be `ret` instead of `!ret`?
Oops! Will fix it.
>> +/**
>> + * sort_memory_ranges - Sorts the given memory ranges list.
>> + * @mem_ranges: Range list to sort.
>> + * @merge: If true, merge the list after sorting.
>> + *
>> + * Returns nothing.
>> + */
>> +void sort_memory_ranges(struct crash_mem *mrngs, bool merge)
>> +{
>> +struct crash_mem_range *rngs;
>> +struct crash_mem_range rng;
>> +int i, j, idx;
>> +
>> +if (!mrngs)
>> +return;
>> +
>> +/* Sort the ranges in-place */
>> +rngs = &mrngs->ranges[0];
>> +for (i = 0; i < mrngs->nr_ranges; i++) {
>> +idx = i;
>> +for (j = (i + 1); j < mrngs->nr_ranges; j++) {
>> +if (rngs[idx].start > rngs[j].start)
>> +idx = j;
>> +}
>> +if (idx != i) {
>> +rng = rngs[idx];
>> +rngs[idx] = rngs[i];
>> +rngs[i] = rng;
>> +}
>> +}
>
> Would it work using sort() from lib/sort.c here?
Yeah. I think we could reuse it with a simple compare callback. Will do that.
Thanks
Hari
Re: [PATCH v3 04/12] ppc64/kexec_file: avoid stomping memory used by special regions
On 15/07/20 8:09 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini writes:
>
>> +/**
>> + * __locate_mem_hole_top_down - Looks top down for a large enough memory
>> hole
>> + * in the memory regions between buf_min &
>> buf_max
>> + * for the buffer. If found, sets kbuf->mem.
>> + * @kbuf: Buffer contents and memory parameters.
>> + * @buf_min:Minimum address for the buffer.
>> + * @buf_max:Maximum address for the buffer.
>> + *
>> + * Returns 0 on success, negative errno on error.
>> + */
>> +static int __locate_mem_hole_top_down(struct kexec_buf *kbuf,
>> + u64 buf_min, u64 buf_max)
>> +{
>> +int ret = -EADDRNOTAVAIL;
>> +phys_addr_t start, end;
>> +u64 i;
>> +
>> +for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
>> + MEMBLOCK_NONE, &start, &end, NULL) {
>> +if (start > buf_max)
>> +continue;
>> +
>> +/* Memory hole not found */
>> +if (end < buf_min)
>> +break;
>> +
>> +/* Adjust memory region based on the given range */
>> +if (start < buf_min)
>> +start = buf_min;
>> +if (end > buf_max)
>> +end = buf_max;
>> +
>> +start = ALIGN(start, kbuf->buf_align);
>> +if (start < end && (end - start + 1) >= kbuf->memsz) {
>
> This is why I dislike using start and end to express address ranges:
>
> While struct resource seems to use the [address, end] convention, my
struct crash_mem also uses [address, end] convention.
This off-by-one error did not cause any issues as the hole start and size we
try to find
are at least page aligned.
Nonetheless, I think fixing 'end' early in the loop with "end -= 1" would ensure
correctness while continuing to use the same convention for structs crash_mem &
resource.
Thanks
Hari
Re: [PATCH v3 05/12] powerpc/drmem: make lmb walk a bit more flexible
On 15/07/20 9:20 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini writes:
>
>> @@ -534,7 +537,7 @@ static int __init early_init_dt_scan_memory_ppc(unsigned
>> long node,
>> #ifdef CONFIG_PPC_PSERIES
>> if (depth == 1 &&
>> strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
>> -walk_drmem_lmbs_early(node, early_init_drmem_lmb);
>> +walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>
> walk_drmem_lmbs_early() can now fail. Should this failure be propagated
> as a return value of early_init_dt_scan_memory_ppc()?
>
>> return 0;
>> }
>> #endif
>
>
>> @@ -787,7 +790,7 @@ static int __init parse_numa_properties(void)
>> */
>> memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>> if (memory) {
>> -walk_drmem_lmbs(memory, numa_setup_drmem_lmb);
>> +walk_drmem_lmbs(memory, NULL, numa_setup_drmem_lmb);
>
> Similarly here. Now that this call can fail, should
> parse_numa_properties() handle or propagate the failure?
They would still not fail unless the callbacks early_init_drmem_lmb() &
numa_setup_drmem_lmb()
are updated to have failure scenarios. Also, these call sites always ignored
failure scenarios
even before walk_drmem_lmbs() was introduced. So, I prefer to keep them the way
they are?
Thanks
Hari
Re: [PATCH v3 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
On 16/07/20 4:22 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini writes:
>
>> +/**
>> + * get_node_path - Get the full path of the given node.
>> + * @dn:Node.
>> + * @path: Updated with the full path of the node.
>> + *
>> + * Returns nothing.
>> + */
>> +static void get_node_path(struct device_node *dn, char *path)
>> +{
>> +if (!dn)
>> +return;
>> +
>> +get_node_path(dn->parent, path);
>
> Is it ok to do recursion in the kernel? In this case I believe it's not
> problematic since the maximum call depth will be the maximum depth of a
> device tree node which shouldn't be too much. Also, there are no local
> variables in this function. But I thought it was worth mentioning.
You are right. We are better off avoiding the recursion here. Will
change it to an iterative version instead.
>> + * each representing a memory range.
>> + */
>> +ranges = (len >> 2) / (n_mem_addr_cells + n_mem_size_cells);
>> +
>> +for (i = 0; i < ranges; i++) {
>> +base = of_read_number(prop, n_mem_addr_cells);
>> +prop += n_mem_addr_cells;
>> +end = base + of_read_number(prop, n_mem_size_cells) - 1;
prop is not used after the above.
> You need to `prop += n_mem_size_cells` here.
But yeah, adding it would make it look complete in some sense..
Thanks
Hari
Re: [PATCH v3 09/12] ppc64/kexec_file: setup backup region for kdump kernel
On 16/07/20 7:08 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini writes:
>
>> @@ -968,7 +1040,7 @@ int setup_new_fdt_ppc64(const struct kimage *image,
>> void *fdt,
>>
>> /*
>> * Restrict memory usage for kdump kernel by setting up
>> - * usable memory ranges.
>> + * usable memory ranges and memory reserve map.
>> */
>> if (image->type == KEXEC_TYPE_CRASH) {
>> ret = get_usable_memory_ranges(&umem);
>> @@ -980,6 +1052,24 @@ int setup_new_fdt_ppc64(const struct kimage *image,
>> void *fdt,
>> pr_err("Error setting up usable-memory property for
>> kdump kernel\n");
>> goto out;
>> }
>> +
>> +ret = fdt_add_mem_rsv(fdt, BACKUP_SRC_START + BACKUP_SRC_SIZE,
>> + crashk_res.start - BACKUP_SRC_SIZE);
>
> I believe this answers my question from the other email about how the
> crashkernel is prevented from stomping in the crashed kernel's memory,
> right? I needed to think for a bit to understand what the above
> reservation was protecting. I think it's worth adding a comment.
Right. The reason to add it in the first place is, prom presses the panic
button if
it can't find low memory. Marking it reserved seems to keep it quiet though.
so..
Will add comment mentioning that..
>> +void purgatory(void)
>> +{
>> +void *dest, *src;
>> +
>> +src = (void *)BACKUP_SRC_START;
>> +if (backup_start) {
>> +dest = (void *)backup_start;
>> +__memcpy(dest, src, BACKUP_SRC_SIZE);
>> +}
>> +}
>
> In general I'm in favor of using C code over assembly, but having to
> bring in that relocation support just for the above makes me wonder if
> it's worth it in this case.
I am planning to build on purgatory later with "I'm in purgatory" print support
for pseries at least and also, sha256 digest check.
Thanks
Hari
Re: [PATCH v3 07/12] ppc64/kexec_file: add support to relocate purgatory
On 16/07/20 5:50 am, Thiago Jung Bauermann wrote: > > Hari Bathini writes: > >> Right now purgatory implementation is only minimal. But if purgatory >> code is to be enhanced to copy memory to the backup region and verify > > Can't the memcpy be done in asm? We have arch/powerpc/lib/memcpy_64.S > for example, perhaps it could be linked in with the purgatory? I wanted to avoid touching common code to make it work for purgatory for now. > >> sha256 digest, relocations may have to be applied to the purgatory. > > Do we want to do the sha256 verification? My original patch series for > kexec_file_load() had a purgatory in C from kexec-tools which did the > sha256 verification but Michael Ellerman thought it was unnecessary and > decided to use the simpler purgatory in asm from kexec-lite. kexec_file_load could as well be used without IMA or secureboot. With sha256 digest calculated anyway, verifying it would make sense to accommodate that case as well. > >> So, add support to relocate purgatory in kexec_file_load system call >> by setting up TOC pointer and applying RELA relocations as needed. > > If we do want to use a C purgatory, Michael Ellerman had suggested > building it as a Position Independent Executable, which greatly reduces > the number and types of relocations that are needed. See patches 4 and 9 > here: > > https://lore.kernel.org/linuxppc-dev/1478748449-3894-1-git-send-email-bauer...@linux.vnet.ibm.com/ > > In the series above I hadn't converted x86 to PIE. If I had done that, > possibly Dave Young's opinion would have been different. :-) > > If that's still not desirable, he suggested in that discussion lifting > some code from x86 to generic code, which I implemented and would > simplify this patch as well: > > https://lore.kernel.org/linuxppc-dev/5009580.5GxAkTrMYA@morokweng/ > Agreed. But I prefer to work on PIE and/or moving common relocation_add code for x86 & s390 to generic code later when I try to build on these purgatory changes. So, a separate series later to rework purgatory with the things you mentioned above sounds ok? Thanks Hari
Re: [PATCH v3 10/12] ppc64/kexec_file: prepare elfcore header for crashing kernel
Hari Bathini writes:
> On 16/07/20 7:52 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini writes:
>>
>>> /**
>>> + * get_crash_memory_ranges - Get crash memory ranges. This list includes
>>> + * first/crashing kernel's memory regions that
>>> + * would be exported via an elfcore.
>>> + * @mem_ranges: Range list to add the memory ranges to.
>>> + *
>>> + * Returns 0 on success, negative errno on error.
>>> + */
>>> +static int get_crash_memory_ranges(struct crash_mem **mem_ranges)
>>> +{
>>> + struct memblock_region *reg;
>>> + struct crash_mem *tmem;
>>> + int ret;
>>> +
>>> + for_each_memblock(memory, reg) {
>>> + u64 base, size;
>>> +
>>> + base = (u64)reg->base;
>>> + size = (u64)reg->size;
>>> +
>>> + /* Skip backup memory region, which needs a separate entry */
>>> + if (base == BACKUP_SRC_START) {
>>> + if (size > BACKUP_SRC_SIZE) {
>>> + base = BACKUP_SRC_END + 1;
>>> + size -= BACKUP_SRC_SIZE;
>>> + } else
>>> + continue;
>>> + }
>>> +
>>> + ret = add_mem_range(mem_ranges, base, size);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + /* Try merging adjacent ranges before reallocation attempt */
>>> + if ((*mem_ranges)->nr_ranges == (*mem_ranges)->max_nr_ranges)
>>> + sort_memory_ranges(*mem_ranges, true);
>>> + }
>>> +
>>> + /* Reallocate memory ranges if there is no space to split ranges */
>>> + tmem = *mem_ranges;
>>> + if (tmem && (tmem->nr_ranges == tmem->max_nr_ranges)) {
>>> + tmem = realloc_mem_ranges(mem_ranges);
>>> + if (!tmem)
>>> + goto out;
>>> + }
>>> +
>>> + /* Exclude crashkernel region */
>>> + ret = crash_exclude_mem_range(tmem, crashk_res.start, crashk_res.end);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + ret = add_rtas_mem_range(mem_ranges);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + ret = add_opal_mem_range(mem_ranges);
>>> + if (ret)
>>> + goto out;
>>
>> Maybe I'm confused, but don't you add the RTAS and OPAL regions as
>> usable memory for the crashkernel? In that case they shouldn't show up
>> in the core file.
>
> kexec-tools does the same thing. I am not endorsing it but I was trying to
> stay
> in parity to avoid breaking any userspace tools/commands. But as you rightly
> pointed, this is NOT right. The right thing to do, to get the rtas/opal data
> at
> the time of crash, is to have a backup region for them just like we have for
> the first 64K memory. I was hoping to do that later.
>
> Will check how userspace tools respond to dropping these regions. If that
> makes
> the tools unhappy, will retain the regions with a FIXME. Sorry about the
> confusion.
No problem, thanks for the clarification.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v3 04/12] ppc64/kexec_file: avoid stomping memory used by special regions
Hari Bathini writes:
> On 15/07/20 8:09 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini writes:
>>
>
>
>
>>> +/**
>>> + * __locate_mem_hole_top_down - Looks top down for a large enough memory
>>> hole
>>> + * in the memory regions between buf_min &
>>> buf_max
>>> + * for the buffer. If found, sets kbuf->mem.
>>> + * @kbuf: Buffer contents and memory parameters.
>>> + * @buf_min:Minimum address for the buffer.
>>> + * @buf_max:Maximum address for the buffer.
>>> + *
>>> + * Returns 0 on success, negative errno on error.
>>> + */
>>> +static int __locate_mem_hole_top_down(struct kexec_buf *kbuf,
>>> + u64 buf_min, u64 buf_max)
>>> +{
>>> + int ret = -EADDRNOTAVAIL;
>>> + phys_addr_t start, end;
>>> + u64 i;
>>> +
>>> + for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE,
>>> + MEMBLOCK_NONE, &start, &end, NULL) {
>>> + if (start > buf_max)
>>> + continue;
>>> +
>>> + /* Memory hole not found */
>>> + if (end < buf_min)
>>> + break;
>>> +
>>> + /* Adjust memory region based on the given range */
>>> + if (start < buf_min)
>>> + start = buf_min;
>>> + if (end > buf_max)
>>> + end = buf_max;
>>> +
>>> + start = ALIGN(start, kbuf->buf_align);
>>> + if (start < end && (end - start + 1) >= kbuf->memsz) {
>>
>> This is why I dislike using start and end to express address ranges:
>>
>> While struct resource seems to use the [address, end] convention, my
>
> struct crash_mem also uses [address, end] convention.
> This off-by-one error did not cause any issues as the hole start and size we
> try to find
> are at least page aligned.
>
> Nonetheless, I think fixing 'end' early in the loop with "end -= 1" would
> ensure
> correctness while continuing to use the same convention for structs crash_mem
> & resource.
Sounds good.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v3 05/12] powerpc/drmem: make lmb walk a bit more flexible
Hari Bathini writes:
> On 15/07/20 9:20 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini writes:
>>
>>> @@ -534,7 +537,7 @@ static int __init
>>> early_init_dt_scan_memory_ppc(unsigned long node,
>>> #ifdef CONFIG_PPC_PSERIES
>>> if (depth == 1 &&
>>> strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
>>> - walk_drmem_lmbs_early(node, early_init_drmem_lmb);
>>> + walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>>
>> walk_drmem_lmbs_early() can now fail. Should this failure be propagated
>> as a return value of early_init_dt_scan_memory_ppc()?
>
>>
>>> return 0;
>>> }
>>> #endif
>>
>>
>>> @@ -787,7 +790,7 @@ static int __init parse_numa_properties(void)
>>> */
>>> memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>>> if (memory) {
>>> - walk_drmem_lmbs(memory, numa_setup_drmem_lmb);
>>> + walk_drmem_lmbs(memory, NULL, numa_setup_drmem_lmb);
>>
>> Similarly here. Now that this call can fail, should
>> parse_numa_properties() handle or propagate the failure?
>
> They would still not fail unless the callbacks early_init_drmem_lmb() &
> numa_setup_drmem_lmb()
> are updated to have failure scenarios. Also, these call sites always ignored
> failure scenarios
> even before walk_drmem_lmbs() was introduced. So, I prefer to keep them the
> way they are?
Ok, makes sense. In this case:
Reviewed-by: Thiago Jung Bauermann
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v3 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
Hari Bathini writes:
> On 16/07/20 4:22 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini writes:
>>
>
>
>
>>> +/**
>>> + * get_node_path - Get the full path of the given node.
>>> + * @dn:Node.
>>> + * @path: Updated with the full path of the node.
>>> + *
>>> + * Returns nothing.
>>> + */
>>> +static void get_node_path(struct device_node *dn, char *path)
>>> +{
>>> + if (!dn)
>>> + return;
>>> +
>>> + get_node_path(dn->parent, path);
>>
>> Is it ok to do recursion in the kernel? In this case I believe it's not
>> problematic since the maximum call depth will be the maximum depth of a
>> device tree node which shouldn't be too much. Also, there are no local
>> variables in this function. But I thought it was worth mentioning.
>
> You are right. We are better off avoiding the recursion here. Will
> change it to an iterative version instead.
Ok.
>>> +* each representing a memory range.
>>> +*/
>>> + ranges = (len >> 2) / (n_mem_addr_cells + n_mem_size_cells);
>>> +
>>> + for (i = 0; i < ranges; i++) {
>>> + base = of_read_number(prop, n_mem_addr_cells);
>>> + prop += n_mem_addr_cells;
>>> + end = base + of_read_number(prop, n_mem_size_cells) - 1;
>
> prop is not used after the above.
>
>> You need to `prop += n_mem_size_cells` here.
>
> But yeah, adding it would make it look complete in some sense..
Isn't it used in the next iteration of the loop?
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v3 09/12] ppc64/kexec_file: setup backup region for kdump kernel
Hari Bathini writes:
> On 16/07/20 7:08 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini writes:
>>
>>> @@ -968,7 +1040,7 @@ int setup_new_fdt_ppc64(const struct kimage *image,
>>> void *fdt,
>>>
>>> /*
>>> * Restrict memory usage for kdump kernel by setting up
>>> -* usable memory ranges.
>>> +* usable memory ranges and memory reserve map.
>>> */
>>> if (image->type == KEXEC_TYPE_CRASH) {
>>> ret = get_usable_memory_ranges(&umem);
>>> @@ -980,6 +1052,24 @@ int setup_new_fdt_ppc64(const struct kimage *image,
>>> void *fdt,
>>> pr_err("Error setting up usable-memory property for
>>> kdump kernel\n");
>>> goto out;
>>> }
>>> +
>>> + ret = fdt_add_mem_rsv(fdt, BACKUP_SRC_START + BACKUP_SRC_SIZE,
>>> + crashk_res.start - BACKUP_SRC_SIZE);
>>
>> I believe this answers my question from the other email about how the
>> crashkernel is prevented from stomping in the crashed kernel's memory,
>> right? I needed to think for a bit to understand what the above
>> reservation was protecting. I think it's worth adding a comment.
>
> Right. The reason to add it in the first place is, prom presses the panic
> button if
> it can't find low memory. Marking it reserved seems to keep it quiet though.
> so..
>
> Will add comment mentioning that..
Ah, makes sense. Thanks for the explanation.
>>> +void purgatory(void)
>>> +{
>>> + void *dest, *src;
>>> +
>>> + src = (void *)BACKUP_SRC_START;
>>> + if (backup_start) {
>>> + dest = (void *)backup_start;
>>> + __memcpy(dest, src, BACKUP_SRC_SIZE);
>>> + }
>>> +}
>>
>> In general I'm in favor of using C code over assembly, but having to
>> bring in that relocation support just for the above makes me wonder if
>> it's worth it in this case.
>
> I am planning to build on purgatory later with "I'm in purgatory" print
> support
> for pseries at least and also, sha256 digest check.
Ok. In that case, my preference would be to convert both the powerpc and
x86 purgatories to PIE since this greatly reduces the types of
relocations that are emitted, but better ask Dave Young what he thinks
before going down that route.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v3 07/12] ppc64/kexec_file: add support to relocate purgatory
Hari Bathini writes: > On 16/07/20 5:50 am, Thiago Jung Bauermann wrote: >> >> Hari Bathini writes: >> >>> So, add support to relocate purgatory in kexec_file_load system call >>> by setting up TOC pointer and applying RELA relocations as needed. >> >> If we do want to use a C purgatory, Michael Ellerman had suggested >> building it as a Position Independent Executable, which greatly reduces >> the number and types of relocations that are needed. See patches 4 and 9 >> here: >> >> https://lore.kernel.org/linuxppc-dev/1478748449-3894-1-git-send-email-bauer...@linux.vnet.ibm.com/ >> >> In the series above I hadn't converted x86 to PIE. If I had done that, >> possibly Dave Young's opinion would have been different. :-) >> >> If that's still not desirable, he suggested in that discussion lifting >> some code from x86 to generic code, which I implemented and would >> simplify this patch as well: >> >> https://lore.kernel.org/linuxppc-dev/5009580.5GxAkTrMYA@morokweng/ >> > > Agreed. But I prefer to work on PIE and/or moving common relocation_add code > for x86 & s390 to generic code later when I try to build on these purgatory > changes. So, a separate series later to rework purgatory with the things you > mentioned above sounds ok? Sounds ok to me. Let's see what the maintainers think, then. -- Thiago Jung Bauermann IBM Linux Technology Center
Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
Jarkko Sakkinen a écrit :
Rename module_alloc() to text_alloc() and module_memfree() to
text_memfree(), and move them to kernel/text.c, which is unconditionally
compiled to the kernel proper. This allows kprobes, ftrace and bpf to
allocate space for executable code without requiring to compile the modules
support (CONFIG_MODULES=y) in.
You are not changing enough in powerpc to have this work.
On powerpc 32 bits (6xx), when STRICT_KERNEL_RWX is selected, the
vmalloc space is set to NX (no exec) at segment level (ie by 256Mbytes
zone) unless CONFIG_MODULES is selected.
Christophe
Cc: Andi Kleen
Suggested-by: Peter Zijlstra
Signed-off-by: Jarkko Sakkinen
---
arch/arm/kernel/Makefile | 3 +-
arch/arm/kernel/module.c | 21 ---
arch/arm/kernel/text.c | 33 ++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/module.c | 42 --
arch/arm64/kernel/text.c | 54
arch/mips/kernel/Makefile| 2 +-
arch/mips/kernel/module.c| 9 -
arch/mips/kernel/text.c | 19 ++
arch/mips/net/bpf_jit.c | 4 +--
arch/nds32/kernel/Makefile | 2 +-
arch/nds32/kernel/module.c | 7
arch/nds32/kernel/text.c | 12 +++
arch/nios2/kernel/Makefile | 1 +
arch/nios2/kernel/module.c | 19 --
arch/nios2/kernel/text.c | 34 ++
arch/parisc/kernel/Makefile | 2 +-
arch/parisc/kernel/module.c | 11 --
arch/parisc/kernel/text.c| 22
arch/powerpc/net/bpf_jit_comp.c | 4 +--
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/module.c | 12 ---
arch/riscv/kernel/text.c | 20 +++
arch/s390/kernel/Makefile| 2 +-
arch/s390/kernel/ftrace.c| 2 +-
arch/s390/kernel/module.c| 16 -
arch/s390/kernel/text.c | 23
arch/sparc/kernel/Makefile | 1 +
arch/sparc/kernel/module.c | 30
arch/sparc/kernel/text.c | 39 +
arch/sparc/net/bpf_jit_comp_32.c | 6 ++--
arch/unicore32/kernel/Makefile | 1 +
arch/unicore32/kernel/module.c | 7
arch/unicore32/kernel/text.c | 18 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/ftrace.c | 4 +--
arch/x86/kernel/kprobes/core.c | 4 +--
arch/x86/kernel/module.c | 49 --
arch/x86/kernel/text.c | 60
include/linux/moduleloader.h | 4 +--
kernel/Makefile | 2 +-
kernel/bpf/core.c| 4 +--
kernel/kprobes.c | 4 +--
kernel/module.c | 37 ++--
kernel/text.c| 25 +
45 files changed, 400 insertions(+), 275 deletions(-)
create mode 100644 arch/arm/kernel/text.c
create mode 100644 arch/arm64/kernel/text.c
create mode 100644 arch/mips/kernel/text.c
create mode 100644 arch/nds32/kernel/text.c
create mode 100644 arch/nios2/kernel/text.c
create mode 100644 arch/parisc/kernel/text.c
create mode 100644 arch/riscv/kernel/text.c
create mode 100644 arch/s390/kernel/text.c
create mode 100644 arch/sparc/kernel/text.c
create mode 100644 arch/unicore32/kernel/text.c
create mode 100644 arch/x86/kernel/text.c
create mode 100644 kernel/text.c
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 89e5d864e923..69bfacfd60ef 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -19,7 +19,8 @@ CFLAGS_REMOVE_return_address.o = -pg
obj-y := elf.o entry-common.o irq.o opcodes.o \
process.o ptrace.o reboot.o \
setup.o signal.o sigreturn_codes.o \
- stacktrace.o sys_arm.o time.o traps.o
+ stacktrace.o sys_arm.o time.o traps.o \
+ text.o
ifneq ($(CONFIG_ARM_UNWIND),y)
obj-$(CONFIG_FRAME_POINTER)+= return_address.o
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index e15444b25ca0..13e3442a6b9f 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -33,27 +33,6 @@
#define MODULES_VADDR (((unsigned long)_exiprom + ~PMD_MASK) & PMD_MASK)
#endif
-#ifdef CONFIG_MMU
-void *module_alloc(unsigned long size)
-{
- gfp_t gfp_mask = GFP_KERNEL;
- void *p;
-
- /* Silence the initial allocation */
- if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
- gfp_mask |= __GFP_NOWARN;
-
- p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
- gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
- __builtin_return_address(0));
- if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
- return p;
- return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
- GFP_KERNEL, PAGE_KERNE
Re: [PATCH] powerpc/64: Fix an out of date comment about MMIO ordering
On Thu, 2020-07-16 at 12:38 -0700, Palmer Dabbelt wrote: > From: Palmer Dabbelt > > This primitive has been renamed, but because it was spelled incorrectly in the > first place it must have escaped the fixup patch. As far as I can tell this > logic is still correct: smp_mb__after_spinlock() uses the default smp_mb() > implementation, which is "sync" rather than "hwsync" but those are the same > (though I'm not that familiar with PowerPC). Typo ? That must be me ... :) Looks fine. Yes, sync and hwsync are the same (by opposition to lwsync which is lighter weight and doesn't order cache inhibited). Cheers, Ben. > Signed-off-by: Palmer Dabbelt > --- > arch/powerpc/kernel/entry_64.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index b3c9f15089b6..7b38b4daca93 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -357,7 +357,7 @@ _GLOBAL(_switch) >* kernel/sched/core.c). >* >* Uncacheable stores in the case of involuntary preemption must > - * be taken care of. The smp_mb__before_spin_lock() in __schedule() > + * be taken care of. The smp_mb__after_spinlock() in __schedule() >* is implemented as hwsync on powerpc, which orders MMIO too. So >* long as there is an hwsync in the context switch path, it will >* be executed on the source CPU after the task has performed
Re: [PATCH v3 0/3] Off-load TLB invalidations to host for !GTSE
Hi all, On Thu, 16 Jul 2020 13:27:14 -0400 Qian Cai wrote: > > Reverting the whole series fixed random memory corruptions during boot on > POWER9 PowerNV systems below. I will revert those commits from linux-next today as well (they revert cleanly). -- Cheers, Stephen Rothwell pgpknsPCqmZPx.pgp Description: OpenPGP digital signature
Question about NUMA distance calculation in powerpc/mm/numa.c
Hello,
I didn't find an explanation about the 'double the distance' logic in
'git log' or anywhere in the kernel docs:
(arch/powerpc/mm/numa.c, __node_distance()):
for (i = 0; i < distance_ref_points_depth; i++) {
if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
break;
/* Double the distance for each NUMA level */
distance *= 2;
}
For reference, the commit that added it:
commit 41eab6f88f24124df89e38067b3766b7bef06ddb
Author: Anton Blanchard
Date: Sun May 16 20:22:31 2010 +
powerpc/numa: Use form 1 affinity to setup node distance
Is there a technical reason for the distance being calculated as the double
for each NUMA level?
The reason I'm asking is because of the QEMU/Libvirt capability to define NUMA
node distances in the VMs. For x86, an user is capable of setting any distance
values to the NUMA topology due to how ACPI SLIT works.
The user, of course, wants the pseries guest to behave the same way. The best
we can do for now is document why this will not happen. I'll document the
limitations imposed by the design itself (how ibm,associativity-reference-points
is capped to MAX_DISTANCE_REF_POINTS and so on). I also would like to document
that the pseries kernel will double the distance for each NUMA level, and for
that it would be nice to provide an actual reason for that to happen, if
there is any.
Thanks,
Daniel
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
Excerpts from pet...@infradead.org's message of July 16, 2020 9:00 pm: > On Thu, Jul 16, 2020 at 08:03:36PM +1000, Nicholas Piggin wrote: >> Excerpts from Peter Zijlstra's message of July 16, 2020 6:50 pm: >> > On Wed, Jul 15, 2020 at 10:18:20PM -0700, Andy Lutomirski wrote: >> >> > On Jul 15, 2020, at 9:15 PM, Nicholas Piggin wrote: > >> >> But I’m wondering if all this deferred sync stuff is wrong. In the >> >> brave new world of io_uring and such, perhaps kernel access matter >> >> too. Heck, even: >> > >> > IIRC the membarrier SYNC_CORE use-case is about user-space >> > self-modifying code. >> > >> > Userspace re-uses a text address and needs to SYNC_CORE before it can be >> > sure the old text is forgotten. Nothing the kernel does matters there. >> > >> > I suppose the manpage could be more clear there. >> >> True, but memory ordering of kernel stores from kernel threads for >> regular mem barrier is the concern here. >> >> Does io_uring update completion queue from kernel thread or interrupt, >> for example? If it does, then membarrier will not order such stores >> with user memory accesses. > > So we're talking about regular membarrier() then? Not the SYNC_CORE > variant per-se. Well, both but Andy in this case was wondering about kernel writes vs user. > > Even there, I'll argue we don't care, but perhaps Mathieu has a > different opinion. All we care about is that all other threads (or CPUs > for GLOBAL) observe an smp_mb() before it returns. > > Any serialization against whatever those other threads/CPUs are running > at the instant of the syscall is external to the syscall, we make no > gauarantees about that. That is, we can fundamentally not say what > another CPU is executing concurrently. Nor should we want to. > > So if you feel that your membarrier() ought to serialize against remote > execution, you need to arrange a quiecent state on the remote side > yourself. > > Now, normally membarrier() is used to implement userspace RCU like > things, and there all that matters is that the remote CPUs observe the > beginngin of the new grace-period, ie counter flip, and we observe their > read-side critical sections, or smething like that, it's been a while > since I looked at all that. > > It's always been the case that concurrent syscalls could change user > memory, io_uring doesn't change that, it just makes it even less well > defined when that would happen. If you want to serialize against that, > you need to arrange that externally. membarrier does replace barrier instructions on remote CPUs, which do order accesses performed by the kernel on the user address space. So membarrier should too I guess. Normal process context accesses like read(2) will do so because they don't get filtered out from IPIs, but kernel threads using the mm may not. Thanks, Nick
Re: [PATCH v2 1/1] ASoC: fsl: fsl-asoc-card: Trivial: Fix misspelling of 'exists'
On Wed, 15 Jul 2020 16:00:09 +0100, Lee Jones wrote: > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: fsl: fsl-asoc-card: Trivial: Fix misspelling of 'exists' commit: 1b58214113481616b74ee4d196e5b1cb683758ee All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Re: [PATCH 1/1] ASoC: fsl: fsl-asoc-card: Trivial: Fix misspelling of 'exists'
On Wed, 15 Jul 2020 10:44:47 +0100, Lee Jones wrote: > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: fsl: fsl-asoc-card: Trivial: Fix misspelling of 'exists' commit: 1b58214113481616b74ee4d196e5b1cb683758ee All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Re: [PATCH v8 5/8] powerpc/vdso: Prepare for switching VDSO to generic C implementation.
Christophe Leroy writes: > Michael Ellerman a écrit : > >> Christophe Leroy writes: >>> Prepare for switching VDSO to generic C implementation in following >>> patch. Here, we: >>> - Modify __get_datapage() to take an offset >>> - Prepare the helpers to call the C VDSO functions >>> - Prepare the required callbacks for the C VDSO functions >>> - Prepare the clocksource.h files to define VDSO_ARCH_CLOCKMODES >>> - Add the C trampolines to the generic C VDSO functions >>> >>> powerpc is a bit special for VDSO as well as system calls in the >>> way that it requires setting CR SO bit which cannot be done in C. >>> Therefore, entry/exit needs to be performed in ASM. >>> >>> Implementing __arch_get_vdso_data() would clobber the link register, >>> requiring the caller to save it. As the ASM calling function already >>> has to set a stack frame and saves the link register before calling >>> the C vdso function, retriving the vdso data pointer there is lighter. >> ... >> >>> diff --git a/arch/powerpc/include/asm/vdso/gettimeofday.h >>> b/arch/powerpc/include/asm/vdso/gettimeofday.h >>> new file mode 100644 >>> index ..4452897f9bd8 >>> --- /dev/null >>> +++ b/arch/powerpc/include/asm/vdso/gettimeofday.h >>> @@ -0,0 +1,175 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef __ASM_VDSO_GETTIMEOFDAY_H >>> +#define __ASM_VDSO_GETTIMEOFDAY_H >>> + >>> +#include >>> + >>> +#ifdef __ASSEMBLY__ >>> + >>> +.macro cvdso_call funct >>> + .cfi_startproc >>> + PPC_STLUr1, -STACK_FRAME_OVERHEAD(r1) >>> + mflrr0 >>> + .cfi_register lr, r0 >>> + PPC_STL r0, STACK_FRAME_OVERHEAD + PPC_LR_STKOFF(r1) >> >> This doesn't work for me on ppc64(le) with glibc. >> >> glibc doesn't create a stack frame before making the VDSO call, so the >> store of r0 (LR) goes into the caller's frame, corrupting the saved LR, >> leading to an infinite loop. > > Where should it be saved if it can't be saved in the standard location ? As Michael pointed out, userspace doesn't treat the VDSO as a normal function call. In order to keep compatibility with existent software, LR would need to be saved on another stack frame. -- Tulio Magno
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
Excerpts from Mathieu Desnoyers's message of July 17, 2020 4:58 am: > - On Jul 16, 2020, at 12:03 PM, Mathieu Desnoyers > mathieu.desnoy...@efficios.com wrote: > >> - On Jul 16, 2020, at 11:46 AM, Mathieu Desnoyers >> mathieu.desnoy...@efficios.com wrote: >> >>> - On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com wrote: I should be more complete here, especially since I was complaining about unclear barrier comment :) CPU0 CPU1 a. user stuff1. user stuff b. membarrier() 2. enter kernel c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule d. read rq->curr 4. rq->curr switched to kthread e. is kthread, skip IPI 5. switch_to kthread f. return to user6. rq->curr switched to user thread g. user stuff7. switch_to user thread 8. exit kernel 9. more user stuff What you're really ordering is a, g vs 1, 9 right? In other words, 9 must see a if it sees g, g must see 1 if it saw 9, etc. Userspace does not care where the barriers are exactly or what kernel memory accesses might be being ordered by them, so long as there is a mb somewhere between a and g, and 1 and 9. Right? >>> >>> This is correct. >> >> Actually, sorry, the above is not quite right. It's been a while >> since I looked into the details of membarrier. >> >> The smp_mb() at the beginning of membarrier() needs to be paired with a >> smp_mb() _after_ rq->curr is switched back to the user thread, so the >> memory barrier is between store to rq->curr and following user-space >> accesses. >> >> The smp_mb() at the end of membarrier() needs to be paired with the >> smp_mb__after_spinlock() at the beginning of schedule, which is >> between accesses to userspace memory and switching rq->curr to kthread. >> >> As to *why* this ordering is needed, I'd have to dig through additional >> scenarios from https://lwn.net/Articles/573436/. Or maybe Paul remembers ? > > Thinking further about this, I'm beginning to consider that maybe we have been > overly cautious by requiring memory barriers before and after store to > rq->curr. > > If CPU0 observes a CPU1's rq->curr->mm which differs from its own process > (current) > while running the membarrier system call, it necessarily means that CPU1 had > to issue smp_mb__after_spinlock when entering the scheduler, between any > user-space > loads/stores and update of rq->curr. > > Requiring a memory barrier between update of rq->curr (back to current > process's > thread) and following user-space memory accesses does not seem to guarantee > anything more than what the initial barrier at the beginning of __schedule > already > provides, because the guarantees are only about accesses to user-space memory. > > Therefore, with the memory barrier at the beginning of __schedule, just > observing that > CPU1's rq->curr differs from current should guarantee that a memory barrier > was issued > between any sequentially consistent instructions belonging to the current > process on > CPU1. > > Or am I missing/misremembering an important point here ? I might have mislead you. CPU0CPU1 r1=yx=1 membarrier()y=1 r2=x membarrier provides if r1==1 then r2==1 (right?) CPU0 r1=y membarrier() smp_mb(); t = cpu_rq(1)->curr; if (t->mm == mm) IPI(CPU1); smp_mb() r2=x vs CPU1 ... __schedule() smp_mb__after_spinlock() rq->curr = kthread ... __schedule() smp_mb__after_spinlock() rq->curr = user thread exit kernel x=1 y=1 Now these last 3 stores are not ordered, so CPU0 might see y==1 but rq->curr == kthread, right? Then it will skip the IPI and stores to x and y will not be ordered. So we do need a mb after rq->curr store when mm is switching. I believe for the global membarrier PF_KTHREAD optimisation, we also need a barrier when switching from a kernel thread to user, for the same reason. So I think I was wrong to say the barrier is not necessary. I haven't quite worked out why two mb()s are required in membarrier(), but at least that's less of a performance concern. Thanks, Nick
Re: [PATCH net-next] ibmvnic: Increase driver logging
On Thu, 16 Jul 2020 13:22:00 -0700 Jakub Kicinski wrote: > On Thu, 16 Jul 2020 18:07:37 +0200 Michal Suchánek wrote: > > On Thu, Jul 16, 2020 at 10:59:58AM -0500, Thomas Falcon wrote: > > > On 7/15/20 8:29 PM, David Miller wrote: > > > > From: Jakub Kicinski > > > > Date: Wed, 15 Jul 2020 17:06:32 -0700 > > > > > > > > > On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote: > > > > > > free_netdev(netdev); > > > > > > dev_set_drvdata(&dev->dev, NULL); > > > > > > + netdev_info(netdev, "VNIC client device has been successfully > > > > > > removed.\n"); > > > > > A step too far, perhaps. > > > > > > > > > > In general this patch looks a little questionable IMHO, this amount of > > > > > logging output is not commonly seen in drivers. All the the info > > > > > messages are just static text, not even carrying any extra > > > > > information. > > > > > In an era of ftrace, and bpftrace, do we really need this? > > > > Agreed, this is too much. This is debugging, and thus suitable for > > > > tracing > > > > facilities, at best. > > > > > > Thanks for your feedback. I see now that I was overly aggressive with this > > > patch to be sure, but it would help with narrowing down problems at a > > > first > > > glance, should they arise. The driver in its current state logs very > > > little > > > of what is it doing without the use of additional debugging or tracing > > > facilities. Would it be worth it to pursue a less aggressive version or > > > would that be dead on arrival? What are acceptable driver operations to > > > log > > > at this level? > > Sadly it's much more of an art than hard science. Most networking > drivers will print identifying information when they probe the device > and then only about major config changes or when link comes up or goes > down. And obviously when anything unexpected, like an error happens, > that's key. > > You seem to be adding start / end information for each driver init / > deinit stage. I'd say try to focus on the actual errors you're trying > to catch. > > > Also would it be advisable to add the messages as pr_dbg to be enabled on > > demand? > > I personally have had a pretty poor experience with pr_debug() because > CONFIG_DYNAMIC_DEBUG is not always enabled. Since you're just printing > static text there shouldn't be much difference between pr_debug and > ftrace and/or bpftrace, honestly. > > Again, slightly hard to advise not knowing what you're trying to catch. Linux drivers in general are far too noisy. In production it is not uncommon to set kernel to suppress all info messages.
