Re: [PATCH v3,4/5] misc: sram_dynamic for user level SRAM access
>> diff --git a/include/linux/sram_dynamic.h b/include/linux/sram_dynamic.h
>> new file mode 100644
>> index ..c77e9e7b1151
>> --- /dev/null
>> +++ b/include/linux/sram_dynamic.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SRAM_DYNAMIC_H
>> +#define __SRAM_DYNAMIC_H
>> +
>> +struct sram_api {
>> +const char *name;
>> +struct sram_device *sdev;
>> +void *(*alloc)(__u32 size, phys_addr_t *phys, __u32 align);
>> +void (*free)(void *ptr);
>> +};
>> +
>> +extern int __must_check
>> +__sram_register_device(struct module *owner,
>> + struct device *parent,
>> + struct sram_api *sa);
>
>'extern' keyword is useless here, remove it (checkpatch --strict will
>likely tell you the same)
>
>> +
>> +/* Use a define to avoid include chaining to get THIS_MODULE */
>> +#define sram_register_device(parent, sa) \
>> +__sram_register_device(THIS_MODULE, parent, sa)
>> +
>> +extern void sram_unregister_device(struct sram_api *sa);
>
>Same, no 'extern' please.
>
Thanks, I will remove them in patch v4. And checkpatch with --strict will be
prefered.
Wenhu
Re: [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Hi Ravi,
Le 24/04/2020 à 05:32, Ravi Bangoria a écrit :
Hi Christophe,
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
*/
void arch_unregister_hw_breakpoint(struct perf_event *bp)
{
+ int i;
+
This declaration should be in the block using it.
/*
* If the breakpoint is unregistered between a
hw_breakpoint_handler()
* and the single_step_dabr_instruction(), then cleanup the
breakpoint
* restoration variables to prevent dangling pointers.
* FIXME, this should not be using bp->ctx at all! Sayeth peterz.
*/
- if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
- bp->ctx->task->thread.last_hit_ubp = NULL;
+ if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
Add declaration of 'int i' here.
How will that help? Keeping declaration at the start of function is also
common practice and I don't see any recommendation to move them inside
conditional block.
Reducing the scope of local variables increases readability, you don't
have to scroll all up to the top of the function to see the declaration
of the variable.
common practice ? Are you sure ? Just have a look at fs/io_uring.c or
many other files in the kernel to see how uncommon your practice is.
Christophe
Re: [PATCH v3,5/5] powerpc: sysdev: support userspace access of fsl_85xx_sram
Le 24/04/2020 à 04:45, Wang Wenhu a écrit :
New module which registers its memory allocation and free APIs to the
sram_dynamic module, which would create a device of struct sram_device
type to act as an interface for user level applications to access the
backend hardware device, fsl_85xx_cache_sram, which is drived by the
FSL_85XX_CACHE_SRAM module.
Signed-off-by: Wang Wenhu
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: linuxppc-dev@lists.ozlabs.org
---
.../powerpc/include/asm/fsl_85xx_cache_sram.h | 4 ++
arch/powerpc/platforms/85xx/Kconfig | 10 +
arch/powerpc/sysdev/Makefile | 1 +
arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h | 6 +++
arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 12 ++
arch/powerpc/sysdev/fsl_85xx_sram_uapi.c | 39 +++
6 files changed, 72 insertions(+)
create mode 100644 arch/powerpc/sysdev/fsl_85xx_sram_uapi.c
We shouldn't add more stuff in arch/powerpc/sysdev/
Either it is dedicated to 85xx, and it should go into
arch/powerpc/platform/85xx/ , or it is common to several
platforms/architectures and should be moved to drivers/soc/fsl/
diff --git a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
index 0235a0447baa..99cb7e202c38 100644
--- a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
+++ b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
@@ -26,6 +26,10 @@ struct mpc85xx_cache_sram {
unsigned int size;
rh_info_t *rh;
spinlock_t lock;
+
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+ struct device *dev;
+#endif
};
extern void mpc85xx_cache_sram_free(void *ptr);
diff --git a/arch/powerpc/platforms/85xx/Kconfig
b/arch/powerpc/platforms/85xx/Kconfig
index fa3d29dcb57e..3a6f6af973eb 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -16,6 +16,16 @@ if FSL_SOC_BOOKE
if PPC32
+config FSL_85XX_SRAM_UAPI
+ tristate "Freescale MPC85xx SRAM UAPI Support"
+ depends on FSL_SOC_BOOKE && SRAM_DYNAMIC
Is SRAM_DYNAMIC usefull on its own, without a driver like this one ? Is
that worth allowing tiny selection of both drivers ? Shouldn't one of
them imply the other one ?
+ select FSL_85XX_CACHE_SRAM
+ help
+ This registers a device of struct sram_device type which would act as
+ an interface for user level applications to access the Freescale 85xx
+ Cache-SRAM memory dynamically, meaning allocate on demand dynamically
+ while they are running.
+
config FSL_85XX_CACHE_SRAM
bool
select PPC_LIB_RHEAP
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index cb5a5bd2cef5..e71f82f0d2c3 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_FSL_CORENET_RCPM)+= fsl_rcpm.o
obj-$(CONFIG_FSL_LBC) += fsl_lbc.o
obj-$(CONFIG_FSL_GTM) += fsl_gtm.o
obj-$(CONFIG_FSL_85XX_CACHE_SRAM) += fsl_85xx_l2ctlr.o
fsl_85xx_cache_sram.o
+obj-$(CONFIG_FSL_85XX_SRAM_UAPI) += fsl_85xx_sram_uapi.o
obj-$(CONFIG_FSL_RIO) += fsl_rio.o fsl_rmu.o
obj-$(CONFIG_TSI108_BRIDGE) += tsi108_pci.o tsi108_dev.o
obj-$(CONFIG_RTC_DRV_CMOS)+= rtc_cmos_setup.o
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
b/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
index ce370749add9..4930784d9852 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
@@ -10,6 +10,8 @@
#ifndef __FSL_85XX_CACHE_CTLR_H__
#define __FSL_85XX_CACHE_CTLR_H__
+#include
+
#define L2CR_L2FI 0x4000 /* L2 flash invalidate */
#define L2CR_L2IO 0x0020 /* L2 instruction only */
#define L2CR_SRAM_ZERO0x /* L2SRAM zero size */
@@ -81,6 +83,10 @@ struct sram_parameters {
phys_addr_t sram_offset;
};
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+extern struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void);
'extern' keywork is meaningless here, remove it.
+#endif
+
extern int instantiate_cache_sram(struct platform_device *dev,
struct sram_parameters sram_params);
extern void remove_cache_sram(struct platform_device *dev);
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index 3de5ac8382c0..0156ea63a3a2 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -23,6 +23,14 @@
struct mpc85xx_cache_sram *cache_sram;
+
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void)
+{
+ return cache_sram;
+}
+#endif
This function is not worth the mess of an #ifdef in a .c file.
cache_sram is already globaly visible, so this function should go in
fsl_85xx_cache_ctlr.h as a 'static inline'
+
void *mpc85xx_cache_sra
Re: [PATCH v3,4/5] misc: sram_dynamic for user level SRAM access
Le 24/04/2020 à 04:45, Wang Wenhu a écrit :
A generic User-Kernel interface module that allows a misc device created
when a backend SRAM hardware device driver registers its APIs to support
file operations of ioctl and mmap for user space applications to allocate
SRAM memory, mmap it to process address space and free it then after.
It is extremely helpful for the user space applications that require
high performance memory accesses, such as embedded networking devices
that would process data in user space, and PowerPC e500 is one case.
Signed-off-by: Wang Wenhu
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org
---
Changes since v1: addressed comments from Arnd
* Changed the ioctl cmd definitions using _IO micros
* Export interfaces for HW-SRAM drivers to register apis to available list
* Modified allocation alignment to PAGE_SIZE
* Use phys_addr_t as type of SRAM resource size and offset
* Support compat_ioctl
* Misc device name:sram
* Use tristate for SRAM_UAPI
* Use postcore_initcall
Changes since v2: addressed comments from Arnd, greg and Scott
* Name the module with sram_dynamic in comparing with drivers/misc/sram.c
I tried to tie the sram_dynamic with the abstractions in sram.c as
Arnd suggested, and actually sram.c probes SRAM devices from devicetree
and manages them with different partitions and create memory pools which
are managed with genalloc functions.
Here sram_dynamic acts only as a interface to user space. A SRAM memory
pool is managed by the module that registers APIs to us, such as the
backend hardware driver of Freescale 85xx Cache-SRAM.
* Create one sram_device for each backend SRAM device(from Scott)
* Allow only one block of SRAM memory allocated to a file descriptor(from
Scott)
* Add sysfs files for every allocated SRAM memory block
* More documentations(As Greg commented)
* Make uapi and non-uapi components apart(from Arnd and Greg)
Links:
v1: https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.w...@vivo.com/
v2: https://lore.kernel.org/lkml/20200420030538.101696-1-wenhu.w...@vivo.com/
UIO version:
v5: https://lore.kernel.org/lkml/20200417071616.44598-5-wenhu.w...@vivo.com/
---
drivers/misc/Kconfig | 11 +
drivers/misc/Makefile| 1 +
drivers/misc/sram_dynamic.c | 580 +++
drivers/misc/sram_uapi.c | 351 +
include/linux/sram_dynamic.h | 23 ++
include/uapi/linux/sram.h| 11 +
6 files changed, 977 insertions(+)
create mode 100644 drivers/misc/sram_dynamic.c
create mode 100644 drivers/misc/sram_uapi.c
create mode 100644 include/linux/sram_dynamic.h
create mode 100644 include/uapi/linux/sram.h
diff --git a/include/linux/sram_dynamic.h b/include/linux/sram_dynamic.h
new file mode 100644
index ..c77e9e7b1151
--- /dev/null
+++ b/include/linux/sram_dynamic.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SRAM_DYNAMIC_H
+#define __SRAM_DYNAMIC_H
+
+struct sram_api {
+ const char *name;
+ struct sram_device *sdev;
+ void *(*alloc)(__u32 size, phys_addr_t *phys, __u32 align);
+ void (*free)(void *ptr);
+};
+
+extern int __must_check
+ __sram_register_device(struct module *owner,
+ struct device *parent,
+ struct sram_api *sa);
'extern' keyword is useless here, remove it (checkpatch --strict will
likely tell you the same)
+
+/* Use a define to avoid include chaining to get THIS_MODULE */
+#define sram_register_device(parent, sa) \
+ __sram_register_device(THIS_MODULE, parent, sa)
+
+extern void sram_unregister_device(struct sram_api *sa);
Same, no 'extern' please.
+
+#endif /* __SRAM_DYNAMIC_H */
diff --git a/include/uapi/linux/sram.h b/include/uapi/linux/sram.h
new file mode 100644
index ..9b4a2615dbfe
--- /dev/null
+++ b/include/uapi/linux/sram.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __SRAM_H
+#define __SRAM_H
+
+/* Allocate memory resource from SRAM */
+#define SRAM_UAPI_IOC_ALLOC_IOWR('S', 0, __be64)
+
+/* Free allocated memory resource to SRAM */
+#define SRAM_UAPI_IOC_FREE _IO('S', 1)
+
+#endif /* __SRAM_H */
Christophe
[PATCH v3 3/3] powerpc/eeh: Release EEH device state synchronously
EEH device state is currently removed (by eeh_remove_device()) during
the device release handler, which is invoked as the device's reference
count drops to zero. This may take some time, or forever, as other
threads may hold references.
However, the PCI device state is released synchronously by
pci_stop_and_remove_bus_device(). This mismatch causes problems, for
example the device may be re-discovered as a new device before the
release handler has been called, leaving the PCI and EEH state
mismatched.
So instead, call eeh_remove_device() from the bus device removal
handlers, which are called synchronously in the removal path.
Signed-off-by: Sam Bobroff
---
arch/powerpc/kernel/eeh.c | 31 +++
arch/powerpc/kernel/pci-hotplug.c | 2 --
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 17cb3e9b5697..64361311bc8e 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1106,6 +1106,37 @@ static int eeh_init(void)
core_initcall_sync(eeh_init);
+static int eeh_device_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+
+ switch (action) {
+ /*
+* Note: It's not possible to perform EEH device addition (i.e.
+* {pseries,pnv}_pcibios_bus_add_device()) here because it depends on
+* the device's resources, which have not yet been set up.
+*/
+ case BUS_NOTIFY_DEL_DEVICE:
+ eeh_remove_device(to_pci_dev(dev));
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block eeh_device_nb = {
+ .notifier_call = eeh_device_notifier,
+};
+
+static __init int eeh_set_bus_notifier(void)
+{
+ bus_register_notifier(&pci_bus_type, &eeh_device_nb);
+ return 0;
+}
+arch_initcall(eeh_set_bus_notifier);
+
/**
* eeh_add_device_early - Enable EEH for the indicated device node
* @pdn: PCI device node for which to set up EEH
diff --git a/arch/powerpc/kernel/pci-hotplug.c
b/arch/powerpc/kernel/pci-hotplug.c
index d6a67f814983..28e9aa274f64 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -57,8 +57,6 @@ void pcibios_release_device(struct pci_dev *dev)
struct pci_controller *phb = pci_bus_to_host(dev->bus);
struct pci_dn *pdn = pci_get_pdn(dev);
- eeh_remove_device(dev);
-
if (phb->controller_ops.release_device)
phb->controller_ops.release_device(dev);
--
2.22.0.216.g00a2a96fc9
[PATCH v3 2/3] powerpc/eeh: fix pseries_eeh_configure_bridge()
If a device is hot unplgged during EEH recovery, it's possible for the
RTAS call to ibm,configure-pe in pseries_eeh_configure() to return
parameter error (-3), however negative return values are not checked
for and this leads to an infinite loop.
Fix this by correctly bailing out on negative values.
Signed-off-by: Sam Bobroff
---
arch/powerpc/platforms/pseries/eeh_pseries.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c
b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 893ba3f562c4..9ea1c06a78cd 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -607,6 +607,8 @@ static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
if (!ret)
return ret;
+ if (ret < 0)
+ break;
/*
* If RTAS returns a delay value that's above 100ms, cut it
@@ -627,7 +629,7 @@ static int pseries_eeh_configure_bridge(struct eeh_pe *pe)
pr_warn("%s: Unable to configure bridge PHB#%x-PE#%x (%d)\n",
__func__, pe->phb->global_number, pe->addr, ret);
- return ret;
+ return rtas_error_rc(ret);
}
/**
--
2.22.0.216.g00a2a96fc9
[PATCH v3 0/3] powerpc/eeh: Release EEH device state synchronously
Hi everyone,
Here are some fixes and cleanups that have come from other work but that I
think stand on their own.
Only one patch ("Release EEH device state synchronously", suggested by Oliver
O'Halloran) is a significant change: it moves the cleanup of some EEH device
data out of the (possibly asynchronous) device release handler and into the
(synchronously called) bus notifier. This is useful for future work as it makes
it easier to reason about the lifetimes of EEH structures.
Note that I've left a few WARN_ON_ONCEs in the code because I'm paranoid, but I
have not been able to hit them during testing.
Cheers,
Sam.
Notes for v3:
I've tweaked the fix for pseries_eeh_configure_bridge() to return the correct
error code (even though it's not used) by calling an already present RTAS
function, rtas_error_rc(). However, I had to make another change to export that
function and while it does seem like the right thing to do, but I'm concerned
it's a bit out of scope for such a small fix.
Notes for v2:
I've dropped both cleanup patches (3/4, 4/4) because that type of cleanup
(replacing a call to eeh_rmv_from_parent_pe() with one to eeh_remove_device())
is incorrect: if called during recovery, it will cause edev->pe to remain set
when it would have been cleared previously. This would lead to stale
information in the edev. I think there should be a way to simplify the code
around EEH_PE_KEEP but I'll look at that separately.
Patch set changelog follows:
Patch set v3:
Patch 1/3 (new in this version): powerpc/rtas: Export rtas_error_rc
Patch 2/3 (was 1/2): powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 3/3 (was 2/2): powerpc/eeh: Release EEH device state synchronously
Patch set v2:
Patch 1/2: powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 2/2: powerpc/eeh: Release EEH device state synchronously
- Added comment explaining why the add case can't be handled similarly to the
remove case.
Dropped (was 3/4) powerpc/eeh: Remove workaround from eeh_add_device_late()
Dropped (was 4/4) powerpc/eeh: Clean up edev cleanup for VFs
Patch set v1:
Patch 1/4: powerpc/eeh: fix pseries_eeh_configure_bridge()
Patch 2/4: powerpc/eeh: Release EEH device state synchronously
Patch 3/4: powerpc/eeh: Remove workaround from eeh_add_device_late()
Patch 4/4: powerpc/eeh: Clean up edev cleanup for VFs
Sam Bobroff (3):
powerpc/rtas: Export rtas_error_rc
powerpc/eeh: fix pseries_eeh_configure_bridge()
powerpc/eeh: Release EEH device state synchronously
arch/powerpc/include/asm/rtas.h | 1 +
arch/powerpc/kernel/eeh.c| 31
arch/powerpc/kernel/pci-hotplug.c| 2 --
arch/powerpc/kernel/rtas.c | 3 +-
arch/powerpc/platforms/pseries/eeh_pseries.c | 4 ++-
5 files changed, 37 insertions(+), 4 deletions(-)
--
2.22.0.216.g00a2a96fc9
[PATCH v3 1/3] powerpc/rtas: Export rtas_error_rc
Export rtas_error_rc() so that it can be used by other users of
rtas_call() (which is already exported).
Signed-off-by: Sam Bobroff
---
v3 * New in this version.
arch/powerpc/include/asm/rtas.h | 1 +
arch/powerpc/kernel/rtas.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 3c1887351c71..7c9e4d3635cf 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -379,6 +379,7 @@ extern int rtas_set_rtc_time(struct rtc_time *rtc_time);
extern unsigned int rtas_busy_delay_time(int status);
extern unsigned int rtas_busy_delay(int status);
+extern int rtas_error_rc(int rtas_rc);
extern int early_init_dt_scan_rtas(unsigned long node,
const char *uname, int depth, void *data);
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index c5fa251b8950..238bf112d29a 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -518,7 +518,7 @@ unsigned int rtas_busy_delay(int status)
}
EXPORT_SYMBOL(rtas_busy_delay);
-static int rtas_error_rc(int rtas_rc)
+int rtas_error_rc(int rtas_rc)
{
int rc;
@@ -546,6 +546,7 @@ static int rtas_error_rc(int rtas_rc)
}
return rc;
}
+EXPORT_SYMBOL(rtas_error_rc);
int rtas_get_power_level(int powerdomain, int *level)
{
--
2.22.0.216.g00a2a96fc9
Re: [PATCH v3 13/16] powerpc/watchpoint: Prepare handler to handle more than one watcnhpoint
Hi Christophe,
@@ -101,14 +129,20 @@ static bool is_ptrace_bp(struct perf_event *bp)
*/
void arch_unregister_hw_breakpoint(struct perf_event *bp)
{
+ int i;
+
This declaration should be in the block using it.
/*
* If the breakpoint is unregistered between a hw_breakpoint_handler()
* and the single_step_dabr_instruction(), then cleanup the breakpoint
* restoration variables to prevent dangling pointers.
* FIXME, this should not be using bp->ctx at all! Sayeth peterz.
*/
- if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L))
- bp->ctx->task->thread.last_hit_ubp = NULL;
+ if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) {
Add declaration of 'int i' here.
How will that help? Keeping declaration at the start of function is also
common practice and I don't see any recommendation to move them inside
conditional block.
Thanks,
Ravi
Re: [PATCH v3 12/16] powerpc/watchpoint: Use builtin ALIGN*() macros
Hi Christophe,
max_len = DAWR_MAX_LEN;
/* DAWR region can't cross 512 bytes boundary */
- if ((start_addr >> 9) != (end_addr >> 9))
+ if ((start_addr >> 9) != ((end_addr - 1) >> 9))
What about:
if (ALIGN(start_addr, SZ_512M) != ALIGN(end - 1, SZ_512M))
ok.
return -EINVAL;
} else if (IS_ENABLED(CONFIG_PPC_8xx)) {
/* 8xx can setup a range without limitation */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index aab82ab80dfa..06679adac447 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -800,12 +800,12 @@ static inline int set_breakpoint_8xx(struct
arch_hw_breakpoint *brk)
unsigned long lctrl1 = LCTRL1_CTE_GT | LCTRL1_CTF_LT | LCTRL1_CRWE_RW |
LCTRL1_CRWF_RW;
unsigned long lctrl2 = LCTRL2_LW0EN | LCTRL2_LW0LADC | LCTRL2_SLW0EN;
- unsigned long start_addr = brk->address & ~HW_BREAKPOINT_ALIGN;
- unsigned long end_addr = (brk->address + brk->len - 1) |
HW_BREAKPOINT_ALIGN;
+ unsigned long start_addr = ALIGN_DOWN(brk->address, HW_BREAKPOINT_SIZE);
+ unsigned long end_addr = ALIGN(brk->address + brk->len,
HW_BREAKPOINT_SIZE);
if (start_addr == 0)
lctrl2 |= LCTRL2_LW0LA_F;
- else if (end_addr == ~0U)
+ else if (end_addr - 1 == ~0U)
What about:
else if (end_addr == 0)
That's better.
Thanks,
Ravi
Re: [PATCH] lib/mpi: Fix building for powerpc with clang
Nathan Chancellor writes:
> On Tue, Apr 14, 2020 at 11:57:31PM +1000, Herbert Xu wrote:
>> On Mon, Apr 13, 2020 at 12:50:42PM -0700, Nathan Chancellor wrote:
>> > 0day reports over and over on an powerpc randconfig with clang:
>> >
>> > lib/mpi/generic_mpih-mul1.c:37:13: error: invalid use of a cast in a
>> > inline asm context requiring an l-value: remove the cast or build with
>> > -fheinous-gnu-extensions
>> >
>> > Remove the superfluous casts, which have been done previously for x86
>> > and arm32 in commit dea632cadd12 ("lib/mpi: fix build with clang") and
>> > commit 7b7c1df2883d ("lib/mpi/longlong.h: fix building with 32-bit
>> > x86").
>> >
>> > Reported-by: kbuild test robot
>> > Link: https://github.com/ClangBuiltLinux/linux/issues/991
>> > Signed-off-by: Nathan Chancellor
>>
>> Acked-by: Herbert Xu
>> --
>> Email: Herbert Xu
>> Home Page: http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
> Might be better for you to take this instead. 0day just tripped over
> this again.
Sorry I missed the ack. Will pick it up today.
cheers
[Bug 199471] [Bisected][Regression] windfarm_pm* no longer gets automatically loaded when CONFIG_I2C_POWERMAC=y is set
https://bugzilla.kernel.org/show_bug.cgi?id=199471 --- Comment #23 from Michael Ellerman (mich...@ellerman.id.au) --- The memory leak is a separate issue, see bug #206695. Can anyone verify that bcf3588d8ed fixes the original issue? -- You are receiving this mail because: You are watching the assignee of the bug.
[PATCH v3, 5/5] powerpc: sysdev: support userspace access of fsl_85xx_sram
New module which registers its memory allocation and free APIs to the
sram_dynamic module, which would create a device of struct sram_device
type to act as an interface for user level applications to access the
backend hardware device, fsl_85xx_cache_sram, which is drived by the
FSL_85XX_CACHE_SRAM module.
Signed-off-by: Wang Wenhu
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: linuxppc-dev@lists.ozlabs.org
---
.../powerpc/include/asm/fsl_85xx_cache_sram.h | 4 ++
arch/powerpc/platforms/85xx/Kconfig | 10 +
arch/powerpc/sysdev/Makefile | 1 +
arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h | 6 +++
arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 12 ++
arch/powerpc/sysdev/fsl_85xx_sram_uapi.c | 39 +++
6 files changed, 72 insertions(+)
create mode 100644 arch/powerpc/sysdev/fsl_85xx_sram_uapi.c
diff --git a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
index 0235a0447baa..99cb7e202c38 100644
--- a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
+++ b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h
@@ -26,6 +26,10 @@ struct mpc85xx_cache_sram {
unsigned int size;
rh_info_t *rh;
spinlock_t lock;
+
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+ struct device *dev;
+#endif
};
extern void mpc85xx_cache_sram_free(void *ptr);
diff --git a/arch/powerpc/platforms/85xx/Kconfig
b/arch/powerpc/platforms/85xx/Kconfig
index fa3d29dcb57e..3a6f6af973eb 100644
--- a/arch/powerpc/platforms/85xx/Kconfig
+++ b/arch/powerpc/platforms/85xx/Kconfig
@@ -16,6 +16,16 @@ if FSL_SOC_BOOKE
if PPC32
+config FSL_85XX_SRAM_UAPI
+ tristate "Freescale MPC85xx SRAM UAPI Support"
+ depends on FSL_SOC_BOOKE && SRAM_DYNAMIC
+ select FSL_85XX_CACHE_SRAM
+ help
+ This registers a device of struct sram_device type which would act as
+ an interface for user level applications to access the Freescale 85xx
+ Cache-SRAM memory dynamically, meaning allocate on demand dynamically
+ while they are running.
+
config FSL_85XX_CACHE_SRAM
bool
select PPC_LIB_RHEAP
diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
index cb5a5bd2cef5..e71f82f0d2c3 100644
--- a/arch/powerpc/sysdev/Makefile
+++ b/arch/powerpc/sysdev/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_FSL_CORENET_RCPM)+= fsl_rcpm.o
obj-$(CONFIG_FSL_LBC) += fsl_lbc.o
obj-$(CONFIG_FSL_GTM) += fsl_gtm.o
obj-$(CONFIG_FSL_85XX_CACHE_SRAM) += fsl_85xx_l2ctlr.o
fsl_85xx_cache_sram.o
+obj-$(CONFIG_FSL_85XX_SRAM_UAPI) += fsl_85xx_sram_uapi.o
obj-$(CONFIG_FSL_RIO) += fsl_rio.o fsl_rmu.o
obj-$(CONFIG_TSI108_BRIDGE)+= tsi108_pci.o tsi108_dev.o
obj-$(CONFIG_RTC_DRV_CMOS) += rtc_cmos_setup.o
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
b/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
index ce370749add9..4930784d9852 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h
@@ -10,6 +10,8 @@
#ifndef __FSL_85XX_CACHE_CTLR_H__
#define __FSL_85XX_CACHE_CTLR_H__
+#include
+
#define L2CR_L2FI 0x4000 /* L2 flash invalidate */
#define L2CR_L2IO 0x0020 /* L2 instruction only */
#define L2CR_SRAM_ZERO 0x /* L2SRAM zero size */
@@ -81,6 +83,10 @@ struct sram_parameters {
phys_addr_t sram_offset;
};
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+extern struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void);
+#endif
+
extern int instantiate_cache_sram(struct platform_device *dev,
struct sram_parameters sram_params);
extern void remove_cache_sram(struct platform_device *dev);
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index 3de5ac8382c0..0156ea63a3a2 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -23,6 +23,14 @@
struct mpc85xx_cache_sram *cache_sram;
+
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void)
+{
+ return cache_sram;
+}
+#endif
+
void *mpc85xx_cache_sram_alloc(unsigned int size,
phys_addr_t *phys, unsigned int align)
{
@@ -115,6 +123,10 @@ int instantiate_cache_sram(struct platform_device *dev,
rh_attach_region(cache_sram->rh, 0, cache_sram->size);
spin_lock_init(&cache_sram->lock);
+#ifdef CONFIG_FSL_85XX_SRAM_UAPI
+ cache_sram->dev = &dev->dev;
+#endif
+
dev_info(&dev->dev, "[base:0x%llx, size:0x%x] configured and loaded\n",
(unsigned long long)cache_sram->base_phys, cache_sram->size);
diff --git a/arch/powerpc/sysdev/fsl_85xx_sram_uapi.c
b/arch/powerpc/sysdev/fsl_85xx_sram_uapi.c
new file mode 100644
index ..60190bf3c8e9
--- /dev/null
+++ b/arch/powerpc/sysdev/fsl_85xx_sram_
[PATCH v3,4/5] misc: sram_dynamic for user level SRAM access
A generic User-Kernel interface module that allows a misc device created when a backend SRAM hardware device driver registers its APIs to support file operations of ioctl and mmap for user space applications to allocate SRAM memory, mmap it to process address space and free it then after. It is extremely helpful for the user space applications that require high performance memory accesses, such as embedded networking devices that would process data in user space, and PowerPC e500 is one case. Signed-off-by: Wang Wenhu Cc: Greg Kroah-Hartman Cc: Arnd Bergmann Cc: Christophe Leroy Cc: Scott Wood Cc: Michael Ellerman Cc: linuxppc-dev@lists.ozlabs.org --- Changes since v1: addressed comments from Arnd * Changed the ioctl cmd definitions using _IO micros * Export interfaces for HW-SRAM drivers to register apis to available list * Modified allocation alignment to PAGE_SIZE * Use phys_addr_t as type of SRAM resource size and offset * Support compat_ioctl * Misc device name:sram * Use tristate for SRAM_UAPI * Use postcore_initcall Changes since v2: addressed comments from Arnd, greg and Scott * Name the module with sram_dynamic in comparing with drivers/misc/sram.c I tried to tie the sram_dynamic with the abstractions in sram.c as Arnd suggested, and actually sram.c probes SRAM devices from devicetree and manages them with different partitions and create memory pools which are managed with genalloc functions. Here sram_dynamic acts only as a interface to user space. A SRAM memory pool is managed by the module that registers APIs to us, such as the backend hardware driver of Freescale 85xx Cache-SRAM. * Create one sram_device for each backend SRAM device(from Scott) * Allow only one block of SRAM memory allocated to a file descriptor(from Scott) * Add sysfs files for every allocated SRAM memory block * More documentations(As Greg commented) * Make uapi and non-uapi components apart(from Arnd and Greg) Links: v1: https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.w...@vivo.com/ v2: https://lore.kernel.org/lkml/20200420030538.101696-1-wenhu.w...@vivo.com/ UIO version: v5: https://lore.kernel.org/lkml/20200417071616.44598-5-wenhu.w...@vivo.com/ --- drivers/misc/Kconfig | 11 + drivers/misc/Makefile| 1 + drivers/misc/sram_dynamic.c | 580 +++ drivers/misc/sram_uapi.c | 351 + include/linux/sram_dynamic.h | 23 ++ include/uapi/linux/sram.h| 11 + 6 files changed, 977 insertions(+) create mode 100644 drivers/misc/sram_dynamic.c create mode 100644 drivers/misc/sram_uapi.c create mode 100644 include/linux/sram_dynamic.h create mode 100644 include/uapi/linux/sram.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 99e151475d8f..b7ad84e93855 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -465,6 +465,17 @@ config PVPANIC a paravirtualized device provided by QEMU; it lets a virtual machine (guest) communicate panic events to the host. +config SRAM_DYNAMIC + tristate "Generic SRAM User Level Dynamic Access API support" + help + This driver allows you to create a misc device which could be used + as an interface to allocate SRAM memory from user level dynamically. + + It is extremely helpful for some user space applications that require + high performance memory accesses. + + If unsure, say N. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 9abf2923d831..c32085026d30 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -46,6 +46,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/ obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o obj-$(CONFIG_SRAM) += sram.o obj-$(CONFIG_SRAM_EXEC)+= sram-exec.o +obj-$(CONFIG_SRAM_DYNAMIC) += sram_dynamic.o obj-y += mic/ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ diff --git a/drivers/misc/sram_dynamic.c b/drivers/misc/sram_dynamic.c new file mode 100644 index ..ea2d4d92cccf --- /dev/null +++ b/drivers/misc/sram_dynamic.c @@ -0,0 +1,580 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd. + * Copyright (C) 2020 Wang Wenhu + * All rights reserved. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define SRAM_MAX_DEVICES (1U << MINORBITS) + +/** + * struct sram_res - allocated SRAM memory resource description. + * + * @virt: virtual memory address of the SRAM memory resource + * @phys: physical memory address of the SRAM memory resource + * @size: size of the SRAM memory resource + * @sdev: sram_device the resource belongs to + * @map: sysf
[PATCH v3, 3/5] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
Function instantiate_cache_sram should not be linked into the init
section for its caller mpc85xx_l2ctlr_of_probe is none-__init.
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy
Signed-off-by: Wang Wenhu
---
No changes
---
arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index be3aef4229d7..3de5ac8382c0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
}
EXPORT_SYMBOL(mpc85xx_cache_sram_free);
-int __init instantiate_cache_sram(struct platform_device *dev,
+int instantiate_cache_sram(struct platform_device *dev,
struct sram_parameters sram_params)
{
int ret = 0;
--
2.17.1
[PATCH v3, 2/5] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
Include linux/io.h into fsl_85xx_cache_sram.c to fix the
implicit-declaration compile error when building Cache-Sram.
arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function ‘instantiate_cache_sram’:
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit declaration of
function ‘ioremap_coherent’; did you mean ‘bitmap_complement’?
[-Werror=implicit-function-declaration]
cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
^~~~
bitmap_complement
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes
pointer from integer without a cast [-Werror=int-conversion]
cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
^
arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit declaration of
function ‘iounmap’; did you mean ‘roundup’?
[-Werror=implicit-function-declaration]
iounmap(cache_sram->base_virt);
^~~
roundup
cc1: all warnings being treated as errors
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy
Signed-off-by: Wang Wenhu
---
No changes
---
arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index f6c665dac725..be3aef4229d7 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include "fsl_85xx_cache_ctlr.h"
--
2.17.1
[PATCH v3,0/5] misc: generic user level sram dynamic access support
This series add a new misc module that act as an interface for user level applications to access SRAM memory dynamically. Freescale 85xx Cache-SRAM is exact an example. This is extremely helpful for the user level applications that require high performance memory accesses, such as some embedded networking devices that need to process data in user space. The series also fix the compile errors and warning of the freescale 85xx Cache-SRAM driver, and implement a module to register the SRAM device to sram_dynamic module, which enables its access for users in user space. Changes since v1: addressed comments from Arnd * Changed the ioctl cmd definitions using _IO micros * Export interfaces for HW-SRAM drivers to register apis to available list * Modified allocation alignment to PAGE_SIZE * Use phys_addr_t as type of SRAM resource size and offset * Support compat_ioctl * Misc device name:sram * Use tristate for SRAM_UAPI * Use postcore_initcall Changes since v2: addressed comments from Arnd, greg and Scott * Name the module with sram_dynamic in comparing with drivers/misc/sram.c I tried to tie the sram_dynamic with the abstractions in sram.c as Arnd suggested, and actually sram.c probes SRAM devices from devicetree and manages them with different partitions and create memory pools which are managed with genalloc functions. Here sram_dynamic acts only as a interface to user space. A SRAM memory pool is managed by the module that registers APIs to us, such as the backend hardware driver of Freescale 85xx Cache-SRAM. * Create one sram_device for each backend SRAM device(from Scott) * Allow only one block of SRAM memory allocated to a file descriptor(from Scott) * Add sysfs files for every allocated SRAM memory block * More documentations(As Greg commented) * Make uapi and non-uapi components apart(from Arnd and Greg) * Add a new module to register freescale 85xx Cache-SRAM APIs to the sram_dynamic module Wang Wenhu (5): powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr powerpc: sysdev: fix compile error for fsl_85xx_cache_sram powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram misc: sram_dynamic for user level SRAM access powerpc: sysdev: support userspace access of fsl 85xx sram .../powerpc/include/asm/fsl_85xx_cache_sram.h | 4 + arch/powerpc/platforms/85xx/Kconfig | 10 + arch/powerpc/sysdev/Makefile | 1 + arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h | 6 + arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 15 +- arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 + arch/powerpc/sysdev/fsl_85xx_sram_uapi.c | 39 ++ drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/sram_dynamic.c | 580 ++ drivers/misc/sram_uapi.c | 351 +++ include/linux/sram_dynamic.h | 23 + include/uapi/linux/sram.h | 11 + 13 files changed, 1052 insertions(+), 1 deletion(-) create mode 100644 arch/powerpc/sysdev/fsl_85xx_sram_uapi.c create mode 100644 drivers/misc/sram_dynamic.c create mode 100644 drivers/misc/sram_uapi.c create mode 100644 include/linux/sram_dynamic.h create mode 100644 include/uapi/linux/sram.h -- 2.17.1
[PATCH v3, 1/5] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
Include "linux/of_address.h" to fix the compile error for
mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
CC arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of
function ‘of_iomap’; did you mean ‘pci_iomap’?
[-Werror=implicit-function-declaration]
l2ctlr = of_iomap(dev->dev.of_node, 0);
^~~~
pci_iomap
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer
from integer without a cast [-Werror=int-conversion]
l2ctlr = of_iomap(dev->dev.of_node, 0);
^
cc1: all warnings being treated as errors
scripts/Makefile.build:267: recipe for target
'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
Cc: Greg Kroah-Hartman
Cc: Arnd Bergmann
Cc: Christophe Leroy
Cc: Scott Wood
Cc: Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy
Signed-off-by: Wang Wenhu
---
No changes
---
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include "fsl_85xx_cache_ctlr.h"
--
2.17.1
Re: [PATCH 3/3] powerpc/module_64: Use special stub for _mcount() with -mprofile-kernel
> On Apr 21, 2020, at 1:35 PM, Naveen N. Rao
> wrote:
>
> Since commit c55d7b5e64265f ("powerpc: Remove STRICT_KERNEL_RWX
> incompatibility with RELOCATABLE"), powerpc kernels with
> -mprofile-kernel can crash in certain scenarios with a trace like below:
>
>BUG: Unable to handle kernel instruction fetch (NULL pointer?)
>Faulting instruction address: 0x
>Oops: Kernel access of bad area, sig: 11 [#1]
>LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
>
>NIP [] 0x0
>LR [c008102c0048] ext4_iomap_end+0x8/0x30 [ext4]
>Call Trace:
> iomap_apply+0x20c/0x920 (unreliable)
> iomap_bmap+0xfc/0x160
> ext4_bmap+0xa4/0x180 [ext4]
> bmap+0x4c/0x80
> jbd2_journal_init_inode+0x44/0x1a0 [jbd2]
> ext4_load_journal+0x440/0x860 [ext4]
> ext4_fill_super+0x342c/0x3ab0 [ext4]
> mount_bdev+0x25c/0x290
> ext4_mount+0x28/0x50 [ext4]
> legacy_get_tree+0x4c/0xb0
> vfs_get_tree+0x4c/0x130
> do_mount+0xa18/0xc50
> sys_mount+0x158/0x180
> system_call+0x5c/0x68
>
> The NIP points to NULL, or a random location (data even), while the LR
> always points to the LEP of a function (with an offset of 8), indicating
> that something went wrong with ftrace. However, ftrace is not
> necessarily active when such crashes occur.
>
> The kernel OOPS sometimes follows a warning from ftrace indicating that
> some module functions could not be patched with a nop. Other times, if a
> module is loaded early during boot, instruction patching can fail due to
> a separate bug, but the error is not reported due to missing error
> reporting.
>
> In all the above cases when instruction patching fails, ftrace will be
> disabled but certain kernel module functions will be left with default
> calls to _mcount(). This is not a problem with ELFv1. However, with
> -mprofile-kernel, the default stub is problematic since it depends on a
> valid module TOC in r2. If the kernel (or a different module) calls into
> a function that does not use the TOC, the function won't have a prologue
> to setup the module TOC. When that function calls into _mcount(), we
> will end up in the relocation stub that will use the previous TOC, and
> end up trying to jump into a random location. From the above trace:
>
> iomap_apply+0x20c/0x920 [kernel TOC]
> |
> V
> ext4_iomap_end+0x8/0x30 [no GEP == kernel TOC]
> |
> V
> _mcount() stub
> [uses kernel TOC -> random entry]
>
> To address this, let's change over to using the special stub that is
> used for ftrace_[regs_]caller() for _mcount(). This ensures that we are
> not dependent on a valid module TOC in r2 for default _mcount()
> handling.
>
> Reported-by: Qian Cai
> Signed-off-by: Naveen N. Rao
Feel free to add,
Tested-by: Qian Cai
Re: [PATCH 17/21] mm: free_area_init: allow defining max_zone_pfn in descending order
On 04/23/20 at 08:55am, Mike Rapoport wrote: > On Thu, Apr 23, 2020 at 10:57:20AM +0800, Baoquan He wrote: > > On 04/23/20 at 10:53am, Baoquan He wrote: > > > On 04/12/20 at 10:48pm, Mike Rapoport wrote: > > > > From: Mike Rapoport > > > > > > > > Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the > > > > ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it > > > > is > > > > sorted in descending order allows using free_area_init() on such > > > > architectures. > > > > > > > > Add top -> down traversal of max_zone_pfn array in free_area_init() and > > > > use > > > > the latter in ARC node/zone initialization. > > > > > > Or maybe leave ARC as is. The change in this patchset doesn't impact > > > ARC's handling about zone initialization, leaving it as is can reduce > > > the complication in implementation of free_area_init(), which is a > > > common function. So I personally don't see a strong motivation to have > > > this patch. > > > > OK, seems this patch is prepared to simplify free_area_init_node(), so > > take back what I said at above. > > > > Then this looks necessary, even though it introduces special case into > > common function free_area_init(). > > The idea is to have a single free_area_init() for all architectures > without keeping two completely different ways of calculating the zone > extents. > Another thing, is that with this we could eventually switch ARC from > DISCONTIGMEM. Yeah, I think uniting them into a single free_area_init() is a great idea. Even though I had been through this patchset, when looked into each of them, still may forget the detail in later patch :)
[Bug 206695] kmemleak reports leaks in drivers/macintosh/windfarm
https://bugzilla.kernel.org/show_bug.cgi?id=206695 Dennis Clarke (dcla...@blastwave.org) changed: What|Removed |Added CC||dcla...@blastwave.org --- Comment #8 from Dennis Clarke (dcla...@blastwave.org) --- 123456789+123456789+123456789+123456789+123456789+123456789+123456789+ I will apply the patch and try with Linux 5.7-rc2 and post any results seen. Also this does close off : https://bugzilla.kernel.org/show_bug.cgi?id=199471 I see Wolfram Sang has commented there. OKay ... good stuff. Dennis Clarke -- You are receiving this mail because: You are watching the assignee of the bug.
[powerpc:fixes-test] BUILD SUCCESS feb8e960d780e170e992a70491eec9dd68f4dbf2
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes-test branch HEAD: feb8e960d780e170e992a70491eec9dd68f4dbf2 powerpc/mm: Fix CONFIG_PPC_KUAP_DEBUG on PPC32 elapsed time: 1303m configs tested: 221 configs skipped: 167 The following configs have been built successfully. More configs may be tested in the coming days. arm efm32_defconfig arm at91_dt_defconfig armshmobile_defconfig arm64 defconfig arm exynos_defconfig armmulti_v5_defconfig arm sunxi_defconfig armmulti_v7_defconfig arm64allyesconfig arm allyesconfig arm64allmodconfig arm allmodconfig arm64 allnoconfig arm allnoconfig sparcallyesconfig mipsar7_defconfig um i386_defconfig ia64 tiger_defconfig i386defconfig riscvallmodconfig i386 allnoconfig i386 allyesconfig i386 alldefconfig i386 debian-10.3 ia64 allmodconfig ia64defconfig ia64 allnoconfig ia64generic_defconfig ia64 bigsur_defconfig ia64 allyesconfig ia64 alldefconfig nios2 3c120_defconfig nios2 10m50_defconfig c6xevmc6678_defconfig xtensa iss_defconfig c6x allyesconfig xtensa common_defconfig openrisc simple_smp_defconfig openriscor1ksim_defconfig h8300 h8s-sim_defconfig h8300 edosk2674_defconfig m68k m5475evb_defconfig m68k allmodconfig h8300h8300h-sim_defconfig m68k sun3_defconfig m68k multi_defconfig powerpc defconfig powerpc ppc64_defconfig powerpc rhel-kconfig powerpc allnoconfig arc defconfig arc allyesconfig microblaze mmu_defconfig microblazenommu_defconfig mips fuloong2e_defconfig mips malta_kvm_defconfig mips allyesconfig mips 64r6el_defconfig mips allnoconfig mips 32r2_defconfig mips allmodconfig mipsmalta_kvm_guest_defconfig mips tb0287_defconfig mips capcella_defconfig mips ip32_defconfig mips decstation_64_defconfig mips loongson3_defconfig mips ath79_defconfig mipsbcm63xx_defconfig pariscallnoconfig pariscgeneric-64bit_defconfig pariscgeneric-32bit_defconfig parisc allyesconfig parisc allmodconfig parisc randconfig-a001-20200423 alpharandconfig-a001-20200423 mips randconfig-a001-20200423 m68k randconfig-a001-20200423 riscvrandconfig-a001-20200423 nds32randconfig-a001-20200423 parisc randconfig-a001-20200422 mips randconfig-a001-20200422 alpharandconfig-a001-20200422 m68k randconfig-a001-20200422 riscvrandconfig-a001-20200422 nds32randconfig-a001-20200422 nios2randconfig-a001-20200423 h8300randconfig-a001-20200423 c6x randconfig-a001-20200423 sparc64 randconfig-a001-20200423 microblaze randconfig-a001-20200423 nios2randconfig-a001-20200422 h8300randconfig-a001-20200422 c6x randconfig-a001-20200422 sparc64 randconfig-a001-20200422 microblaze randconfig-a001-20200422 sh randconfig-a001-20200423 csky randconfig-a001-20200423 xtensa randconfig-a001-20200423 openrisc randconfig-a001-20200423 sh randconfig-a001
Re: [PATCH v8 1/1] powerpc/powernv: Introduce support and parsing for self-save API
On Thu, Apr 23, 2020 at 04:25:57PM +0530, Pratik Rajesh Sampat wrote:
> This commit introduces and leverages the Self save API. The difference
> between self-save and self-restore is that the value to be saved for the
> SPR does not need to be passed to the call.
>
> Add the new Self Save OPAL API call in the list of OPAL calls.
>
> The device tree is parsed looking for the property "ibm,opal-self-save"
> If self-save is supported then for all SPRs self-save is invoked for all
> P9 supported registers. In the case self-save fails corresponding
> self-restore call is invoked as a fallback.
>
> Signed-off-by: Pratik Rajesh Sampat
A suggestion from the bisectability point of view though.
Since in this patch you are also invoking self_save API for a new SPR,
namely PTCR which was previously not present, I would suggest that you
move the PTCR changes to a different patch.
Otherwise, the patchset looks good to me
Reviewed-by: Gautham R. Shenoy
> ---
> arch/powerpc/include/asm/opal-api.h| 3 +-
> arch/powerpc/include/asm/opal.h| 1 +
> arch/powerpc/platforms/powernv/idle.c | 73 ++
> arch/powerpc/platforms/powernv/opal-call.c | 1 +
> 4 files changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/opal-api.h
> b/arch/powerpc/include/asm/opal-api.h
> index 1dffa3cb16ba..7ba698369083 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -214,7 +214,8 @@
> #define OPAL_SECVAR_GET 176
> #define OPAL_SECVAR_GET_NEXT 177
> #define OPAL_SECVAR_ENQUEUE_UPDATE 178
> -#define OPAL_LAST178
> +#define OPAL_SLW_SELF_SAVE_REG 181
> +#define OPAL_LAST181
>
> #define QUIESCE_HOLD 1 /* Spin all calls at entry */
> #define QUIESCE_REJECT 2 /* Fail all calls with
> OPAL_BUSY */
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 9986ac34b8e2..a370b0e8d899 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -204,6 +204,7 @@ int64_t opal_handle_hmi2(__be64 *out_flags);
> int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
> int64_t opal_unregister_dump_region(uint32_t id);
> int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
> +int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
> int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
> int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t
> pe_number);
> int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
> diff --git a/arch/powerpc/platforms/powernv/idle.c
> b/arch/powerpc/platforms/powernv/idle.c
> index 78599bca66c2..ada7ece24521 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -32,6 +32,11 @@
> #define P9_STOP_SPR_MSR 2000
> #define P9_STOP_SPR_PSSCR 855
>
> +/* Caching the self-save functionality, lpcr, ptcr support */
> +DEFINE_STATIC_KEY_FALSE(self_save_available);
> +DEFINE_STATIC_KEY_FALSE(is_lpcr_self_save);
> +DEFINE_STATIC_KEY_FALSE(is_ptcr_self_save);
> +
> static u32 supported_cpuidle_states;
> struct pnv_idle_states_t *pnv_idle_states;
> int nr_pnv_idle_states;
> @@ -61,6 +66,35 @@ static bool deepest_stop_found;
>
> static unsigned long power7_offline_type;
>
> +/*
> + * Cache support for SPRs that support self-save as well as kernel save
> restore
> + * so that kernel does not duplicate efforts in saving and restoring SPRs
> + */
> +static void cache_spr_self_save_support(u64 sprn)
> +{
> + switch (sprn) {
> + case SPRN_LPCR:
> + static_branch_enable(&is_lpcr_self_save);
> + break;
> + case SPRN_PTCR:
> + static_branch_enable(&is_ptcr_self_save);
> + break;
> + }
> +}
> +
> +static int pnv_save_one_spr(u64 pir, u64 sprn, u64 val)
> +{
> + if (static_branch_likely(&self_save_available)) {
> + int rc = opal_slw_self_save_reg(pir, sprn);
> +
> + if (!rc) {
> + cache_spr_self_save_support(sprn);
> + return rc;
> + }
> + }
> + return opal_slw_set_reg(pir, sprn, val);
> +}
> +
> static int pnv_save_sprs_for_deep_states(void)
> {
> int cpu;
> @@ -72,6 +106,7 @@ static int pnv_save_sprs_for_deep_states(void)
>* same across all cpus.
>*/
> uint64_t lpcr_val = mfspr(SPRN_LPCR);
> + uint64_t ptcr_val = mfspr(SPRN_PTCR);
> uint64_t hid0_val = mfspr(SPRN_HID0);
> uint64_t hid1_val = mfspr(SPRN_HID1);
> uint64_t hid4_val = mfspr(SPRN_HID4);
> @@ -84,30 +119,34 @@ static int pnv_save_sprs_for_deep_states(void)
> uint64_t pir = get_hard_smp_processor_id(cpu);
>
Re: [PATCH v8 3/3] Self save API integration
On Thu, Apr 23, 2020 at 04:24:38PM +0530, Pratik Rajesh Sampat wrote: > The commit makes the self save API available outside the firmware by defining > an OPAL wrapper. > This wrapper has a similar interface to that of self restore and expects the > cpu pir, SPR number, minus the value of that SPR to be passed in its > paramters and returns OPAL_SUCCESS on success. It adds a device-tree > node signifying support for self-save after verifying the stop API > version compatibility. > > The commit also documents both the self-save and the self-restore API > calls along with their working and usage. > > Signed-off-by: Pratik Rajesh Sampat LGTM. Reviewed-by: Gautham R. Shenoy > --- > doc/opal-api/opal-slw-self-save-reg-181.rst | 51 ++ > doc/opal-api/opal-slw-set-reg-100.rst | 5 + > doc/power-management.rst| 48 + > hw/slw.c| 106 > include/opal-api.h | 3 +- > include/p9_stop_api.H | 18 > include/skiboot.h | 3 + > 7 files changed, 233 insertions(+), 1 deletion(-) > create mode 100644 doc/opal-api/opal-slw-self-save-reg-181.rst > > diff --git a/doc/opal-api/opal-slw-self-save-reg-181.rst > b/doc/opal-api/opal-slw-self-save-reg-181.rst > new file mode 100644 > index ..f20e9b81 > --- /dev/null > +++ b/doc/opal-api/opal-slw-self-save-reg-181.rst > @@ -0,0 +1,51 @@ > +.. OPAL_SLW_SELF_SAVE_REG: > + > +OPAL_SLW_SELF_SAVE_REG > +== > + > +.. code-block:: c > + > + #define OPAL_SLW_SELF_SAVE_REG181 > + > + int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn); > + > +:ref:`OPAL_SLW_SELF_SAVE_REG` is used to inform low-level firmware to save > +the current contents of the SPR before entering a state of loss and > +also restore the content back on waking up from a deep stop state. > + > +An OPAL call `OPAL_SLW_SET_REG` exists which is similar in function as > +saving and restoring the SPR, with one difference being that the value of the > +SPR must also be supplied in the parameters. > +Complete reference: doc/opal-api/opal-slw-set-reg-100.rst > + > +Parameters > +-- > + > +``uint64_t cpu_pir`` > + This parameter specifies the pir of the cpu for which the call is being > made. > +``uint64_t sprn`` > + This parameter specifies the spr number as mentioned in p9_stop_api.H > + The list of SPRs supported is as follows. > + P9_STOP_SPR_DAWR, > + P9_STOP_SPR_HSPRG0, > + P9_STOP_SPR_LDBAR, > + P9_STOP_SPR_LPCR, > + P9_STOP_SPR_PSSCR, > + P9_STOP_SPR_MSR, > + P9_STOP_SPR_HRMOR, > + P9_STOP_SPR_HMEER, > + P9_STOP_SPR_PMCR, > + P9_STOP_SPR_PTCR > + > + The property "ibm,opal-self-save" is supplied to the device tree to > advterise > + support. > + > +Returns > +--- > + > +:ref:`OPAL_UNSUPPORTED` > + If spr restore is not supported by pore engine. > +:ref:`OPAL_PARAMETER` > + Invalid handle for the pir/chip > +:ref:`OPAL_SUCCESS` > + On success > diff --git a/doc/opal-api/opal-slw-set-reg-100.rst > b/doc/opal-api/opal-slw-set-reg-100.rst > index 2e8f1bd6..ee3e68ce 100644 > --- a/doc/opal-api/opal-slw-set-reg-100.rst > +++ b/doc/opal-api/opal-slw-set-reg-100.rst > @@ -21,6 +21,11 @@ In Power 9, it uses p9_stop_save_cpureg(), api provided by > self restore code, > to inform the spr with their corresponding values with which they > must be restored. > > +An OPAL call `OPAL_SLW_SELF_SAVE_REG` exists which is similar in function > +saving and restoring the SPR, with one difference being that the value of the > +SPR doesn't need to be passed in the parameters, only with the SPR number > +the firmware can identify, save and restore the values for the same. > +Complete reference: doc/opal-api/opal-slw-self-save-reg-181.rst > > Parameters > -- > diff --git a/doc/power-management.rst b/doc/power-management.rst > index 76491a71..d6bd5358 100644 > --- a/doc/power-management.rst > +++ b/doc/power-management.rst > @@ -15,3 +15,51 @@ On boot, specific stop states can be disabled via setting > a mask. For example, > to disable all but stop 0,1,2, use ~0xE000. :: > >nvram -p ibm,skiboot --update-config > opal-stop-state-disable-mask=0x1FFF > + > +Saving and restoring Special Purpose Registers(SPRs) > + > + > +When a CPU wakes up from a deep stop state which can result in > +hypervisor state loss, all the SPRs are lost. The Linux Kernel expects > +a small set of SPRs to contain an expected value when the CPU wakes up > +from such a deep stop state. The microcode firmware provides the > +following two APIs, collectively known as the stop-APIs, to allow the > +kernel/OPAL to specify this set of SPRs and the value that they need > +to be restored with on waking up from a deep stop state. > + > +Self-restore: > +int64_t opal_slw_set_reg(uint64_t cpu_pir
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On Thu, Apr 23, 2020 at 02:15:58PM -0300, Adhemerval Zanella wrote: > > > On 23/04/2020 13:43, Rich Felker wrote: > > On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote: > >> > >> > >> On 23/04/2020 13:18, Rich Felker wrote: > >>> On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote: > > > On 22/04/2020 23:36, Rich Felker wrote: > > On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote: > >> Yeah I had a bit of a play around with musl (which is very nice code I > >> must say). The powerpc64 syscall asm is missing ctr clobber by the > >> way. > >> Fortunately adding it doesn't change code generation for me, but it > >> should be fixed. glibc had the same bug at one point I think (probably > >> due to syscall ABI documentation not existing -- something now lives > >> in > >> linux/Documentation/powerpc/syscall64-abi.rst). > > > > Do you know anywhere I can read about the ctr issue, possibly the > > relevant glibc bug report? I'm not particularly familiar with ppc > > register file (at least I have to refamiliarize myself every time I > > work on this stuff) so it'd be nice to understand what's > > potentially-wrong now. > > My understanding is the ctr issue only happens for vDSO calls where it > fallback to a syscall in case an error (invalid argument, etc. and > assuming if vDSO does not fallback to a syscall it always succeed). > This makes the vDSO call on powerpc to have same same ABI constraint > as a syscall, where it clobbers CR0. > >>> > >>> I think you mean "vsyscall", the old thing glibc used where there are > >>> in-userspace implementations of some syscalls with call interfaces > >>> roughly equivalent to a syscall. musl has never used this. It only > >>> uses the actual exported functions from the vdso which have normal > >>> external function call ABI. > >> > >> I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing. > >> The issue is indeed when calling the powerpc provided functions in > >> vDSO, which musl might want to do eventually. > > > > AIUI (at least this is true for all other archs) the functions have > > normal external function call ABI and calling them has nothing to do > > with syscall mechanisms. > > My point is powerpc specifically does not follow it, since it issues a > syscall in fallback and its semantic follow kernel syscalls (error > signalled in cr0, r3 being always a positive value): Oh, then I think we'll just ignore these unless the kernel can make ones with a reasonable ABI. It's not worth having ppc-specific code for this... It would be really nice if ones that actually behave like functions could be added though. > -- > V_FUNCTION_BEGIN(__kernel_clock_gettime) > .cfi_startproc > [...] > /* > * syscall fallback > */ > 99: > li r0,__NR_clock_gettime > .cfi_restore lr > sc > blr > .cfi_endproc > V_FUNCTION_END(__kernel_clock_gettime) > > > > > > It looks like we're not using them right now and I'm not sure why. It > > could be that there are ABI mismatch issues (are 32-bit ones > > compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or > > just that nobody proposed adding them. Also as of 5.4 32-bit ppc > > lacked time64 versions of them; not sure if this is fixed yet. > > For 64-bit it also have an issue where vDSO does not provide an OPD > for ELFv1, which has bitten glibc while trying to implement an ifunc > optimization. I don't recall any issue for ELFv2. > > For 32-bit I am not sure secure-plt will change anything, at least not > on powerpc where we use the same strategy for 64-bit and use a > mtctr/bctr directly. Indeed, I don't think there's a secure-plt distinction unless you're making outgoing calls to possibly-cross-DSO functions. Rich
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On 23/04/2020 13:43, Rich Felker wrote: > On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote: >> >> >> On 23/04/2020 13:18, Rich Felker wrote: >>> On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote: On 22/04/2020 23:36, Rich Felker wrote: > On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote: >> Yeah I had a bit of a play around with musl (which is very nice code I >> must say). The powerpc64 syscall asm is missing ctr clobber by the way. >> Fortunately adding it doesn't change code generation for me, but it >> should be fixed. glibc had the same bug at one point I think (probably >> due to syscall ABI documentation not existing -- something now lives in >> linux/Documentation/powerpc/syscall64-abi.rst). > > Do you know anywhere I can read about the ctr issue, possibly the > relevant glibc bug report? I'm not particularly familiar with ppc > register file (at least I have to refamiliarize myself every time I > work on this stuff) so it'd be nice to understand what's > potentially-wrong now. My understanding is the ctr issue only happens for vDSO calls where it fallback to a syscall in case an error (invalid argument, etc. and assuming if vDSO does not fallback to a syscall it always succeed). This makes the vDSO call on powerpc to have same same ABI constraint as a syscall, where it clobbers CR0. >>> >>> I think you mean "vsyscall", the old thing glibc used where there are >>> in-userspace implementations of some syscalls with call interfaces >>> roughly equivalent to a syscall. musl has never used this. It only >>> uses the actual exported functions from the vdso which have normal >>> external function call ABI. >> >> I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing. >> The issue is indeed when calling the powerpc provided functions in >> vDSO, which musl might want to do eventually. > > AIUI (at least this is true for all other archs) the functions have > normal external function call ABI and calling them has nothing to do > with syscall mechanisms. My point is powerpc specifically does not follow it, since it issues a syscall in fallback and its semantic follow kernel syscalls (error signalled in cr0, r3 being always a positive value): -- V_FUNCTION_BEGIN(__kernel_clock_gettime) .cfi_startproc [...] /* * syscall fallback */ 99: li r0,__NR_clock_gettime .cfi_restore lr sc blr .cfi_endproc V_FUNCTION_END(__kernel_clock_gettime) > > It looks like we're not using them right now and I'm not sure why. It > could be that there are ABI mismatch issues (are 32-bit ones > compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or > just that nobody proposed adding them. Also as of 5.4 32-bit ppc > lacked time64 versions of them; not sure if this is fixed yet. For 64-bit it also have an issue where vDSO does not provide an OPD for ELFv1, which has bitten glibc while trying to implement an ifunc optimization. I don't recall any issue for ELFv2. For 32-bit I am not sure secure-plt will change anything, at least not on powerpc where we use the same strategy for 64-bit and use a mtctr/bctr directly.
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote: > > > On 23/04/2020 13:18, Rich Felker wrote: > > On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote: > >> > >> > >> On 22/04/2020 23:36, Rich Felker wrote: > >>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote: > Yeah I had a bit of a play around with musl (which is very nice code I > must say). The powerpc64 syscall asm is missing ctr clobber by the way. > Fortunately adding it doesn't change code generation for me, but it > should be fixed. glibc had the same bug at one point I think (probably > due to syscall ABI documentation not existing -- something now lives in > linux/Documentation/powerpc/syscall64-abi.rst). > >>> > >>> Do you know anywhere I can read about the ctr issue, possibly the > >>> relevant glibc bug report? I'm not particularly familiar with ppc > >>> register file (at least I have to refamiliarize myself every time I > >>> work on this stuff) so it'd be nice to understand what's > >>> potentially-wrong now. > >> > >> My understanding is the ctr issue only happens for vDSO calls where it > >> fallback to a syscall in case an error (invalid argument, etc. and > >> assuming if vDSO does not fallback to a syscall it always succeed). > >> This makes the vDSO call on powerpc to have same same ABI constraint > >> as a syscall, where it clobbers CR0. > > > > I think you mean "vsyscall", the old thing glibc used where there are > > in-userspace implementations of some syscalls with call interfaces > > roughly equivalent to a syscall. musl has never used this. It only > > uses the actual exported functions from the vdso which have normal > > external function call ABI. > > I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing. > The issue is indeed when calling the powerpc provided functions in > vDSO, which musl might want to do eventually. AIUI (at least this is true for all other archs) the functions have normal external function call ABI and calling them has nothing to do with syscall mechanisms. It looks like we're not using them right now and I'm not sure why. It could be that there are ABI mismatch issues (are 32-bit ones compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or just that nobody proposed adding them. Also as of 5.4 32-bit ppc lacked time64 versions of them; not sure if this is fixed yet. Rich
Re: [PATCH] lib/mpi: Fix building for powerpc with clang
On Tue, Apr 14, 2020 at 11:57:31PM +1000, Herbert Xu wrote:
> On Mon, Apr 13, 2020 at 12:50:42PM -0700, Nathan Chancellor wrote:
> > 0day reports over and over on an powerpc randconfig with clang:
> >
> > lib/mpi/generic_mpih-mul1.c:37:13: error: invalid use of a cast in a
> > inline asm context requiring an l-value: remove the cast or build with
> > -fheinous-gnu-extensions
> >
> > Remove the superfluous casts, which have been done previously for x86
> > and arm32 in commit dea632cadd12 ("lib/mpi: fix build with clang") and
> > commit 7b7c1df2883d ("lib/mpi/longlong.h: fix building with 32-bit
> > x86").
> >
> > Reported-by: kbuild test robot
> > Link: https://github.com/ClangBuiltLinux/linux/issues/991
> > Signed-off-by: Nathan Chancellor
>
> Acked-by: Herbert Xu
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Might be better for you to take this instead. 0day just tripped over
this again.
Cheers,
Nathan
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On 23/04/2020 13:18, Rich Felker wrote: > On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote: >> >> >> On 22/04/2020 23:36, Rich Felker wrote: >>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote: Yeah I had a bit of a play around with musl (which is very nice code I must say). The powerpc64 syscall asm is missing ctr clobber by the way. Fortunately adding it doesn't change code generation for me, but it should be fixed. glibc had the same bug at one point I think (probably due to syscall ABI documentation not existing -- something now lives in linux/Documentation/powerpc/syscall64-abi.rst). >>> >>> Do you know anywhere I can read about the ctr issue, possibly the >>> relevant glibc bug report? I'm not particularly familiar with ppc >>> register file (at least I have to refamiliarize myself every time I >>> work on this stuff) so it'd be nice to understand what's >>> potentially-wrong now. >> >> My understanding is the ctr issue only happens for vDSO calls where it >> fallback to a syscall in case an error (invalid argument, etc. and >> assuming if vDSO does not fallback to a syscall it always succeed). >> This makes the vDSO call on powerpc to have same same ABI constraint >> as a syscall, where it clobbers CR0. > > I think you mean "vsyscall", the old thing glibc used where there are > in-userspace implementations of some syscalls with call interfaces > roughly equivalent to a syscall. musl has never used this. It only > uses the actual exported functions from the vdso which have normal > external function call ABI. I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing. The issue is indeed when calling the powerpc provided functions in vDSO, which musl might want to do eventually.
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote: > > > On 22/04/2020 23:36, Rich Felker wrote: > > On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote: > >> Yeah I had a bit of a play around with musl (which is very nice code I > >> must say). The powerpc64 syscall asm is missing ctr clobber by the way. > >> Fortunately adding it doesn't change code generation for me, but it > >> should be fixed. glibc had the same bug at one point I think (probably > >> due to syscall ABI documentation not existing -- something now lives in > >> linux/Documentation/powerpc/syscall64-abi.rst). > > > > Do you know anywhere I can read about the ctr issue, possibly the > > relevant glibc bug report? I'm not particularly familiar with ppc > > register file (at least I have to refamiliarize myself every time I > > work on this stuff) so it'd be nice to understand what's > > potentially-wrong now. > > My understanding is the ctr issue only happens for vDSO calls where it > fallback to a syscall in case an error (invalid argument, etc. and > assuming if vDSO does not fallback to a syscall it always succeed). > This makes the vDSO call on powerpc to have same same ABI constraint > as a syscall, where it clobbers CR0. I think you mean "vsyscall", the old thing glibc used where there are in-userspace implementations of some syscalls with call interfaces roughly equivalent to a syscall. musl has never used this. It only uses the actual exported functions from the vdso which have normal external function call ABI. Rich
Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
With STRICT_KERNEL_RWX, we are currently ignoring return value from
__patch_instruction() in do_patch_instruction(), resulting in the error
not being propagated back. Fix the same.
Good patch.
Be aware that there is ongoing work which tend to wanting to replace
error reporting by BUG_ON() . See
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for
patch_instruction()")
Signed-off-by: Naveen N. Rao
---
arch/powerpc/lib/code-patching.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 3345f039a876..5c713a6c0bd8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
static int do_patch_instruction(unsigned int *addr, unsigned int instr)
{
- int err;
+ int err, rc = 0;
unsigned int *patch_addr = NULL;
unsigned long flags;
unsigned long text_poke_addr;
@@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr,
unsigned int instr)
patch_addr = (unsigned int *)(text_poke_addr) +
((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
- __patch_instruction(addr, instr, patch_addr);
+ rc = __patch_instruction(addr, instr, patch_addr);
err = unmap_patch_area(text_poke_addr);
if (err)
@@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr,
unsigned int instr)
out:
local_irq_restore(flags);
- return err;
+ return rc ? rc : err;
That's not really consistent. __patch_instruction() and
unmap_patch_area() return a valid minus errno, while in case of
map_patch_area() failure, err has value -1
}
#else /* !CONFIG_STRICT_KERNEL_RWX */
Christophe
Re: [PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit : Introduce a macro PATCH_INSN() to simplify instruction patching, and to make the error messages more uniform and useful: - print an error message that includes the original return value - print the function name and line numbers, so that the offending location is clear - always return -EPERM, which ftrace_bug() expects for proper error handling Also eliminate use of patch_branch() since most such uses already call create_branch() for error checking before patching. Instead, use the return value from create_branch() with PATCH_INSN(). I have the same comment here as for patch 3, this kind of macro hides the return action and can be dangerous. What about implementing a macro that takes an explicit label as third argument and jump to that label in case of error ? On the same model as unsafe_put_user() ? Christophe
Re: [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
patch_instruction() can fail in some scenarios. Add appropriate error
checking so that such failures are caught and logged, and suitable error
code is returned.
Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to
patch_instruction()")
Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/kprobes.c | 10 +++-
arch/powerpc/kernel/optprobes.c | 99 ++---
2 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 81efb605113e..4a297ae2bd87 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
void arch_arm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+ int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+
+ if (rc)
+ WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr,
rc);
}
NOKPROBE_SYMBOL(arch_arm_kprobe);
void arch_disarm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, p->opcode);
+ int rc = patch_instruction(p->addr, p->opcode);
+
+ if (rc)
+ WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr,
rc);
}
NOKPROBE_SYMBOL(arch_disarm_kprobe);
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..046485bb0a52 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe
*op)
}
}
+#define PATCH_INSN(addr, instr) \
+do {\
+ int rc = patch_instruction((unsigned int *)(addr), instr); \
+ if (rc) {\
+ pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n",
\
+ __func__, __LINE__, \
+ (void *)(addr), (void *)(addr), rc); \
+ return rc; \
+ }\
+} while (0)
+
I hate this kind of macro which hides the "return".
What about keeping the return action in the caller ?
Otherwise, what about implementing something based on the use of goto,
on the same model as unsafe_put_user() for instance ?
Christophe
[PATCH 3/3] powerpc/hw_bkpt: Update printk format specifiers for kernel addresses
Change prinkt format specifier from %lx to %pK to indicate kernel
pointer, and to hide the addresses from unprivileged users.
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/hw_breakpoint.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
index 72f461bd70fb..93a303cf0c67 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -257,7 +257,8 @@ static bool stepping_handler(struct pt_regs *regs, struct
perf_event *bp,
if (!ret && (type == LARX || type == STCX)) {
printk_ratelimited("Breakpoint hit on instruction that can't be
emulated."
- " Breakpoint at 0x%lx will be disabled.\n",
addr);
+ " Breakpoint at 0x%pK will be disabled.\n",
+ (void *)addr);
goto disable;
}
@@ -286,7 +287,7 @@ static bool stepping_handler(struct pt_regs *regs, struct
perf_event *bp,
* it and throw a warning message to let the user know about it.
*/
WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
- "0x%lx will be disabled.", addr);
+ "0x%pK will be disabled.", (void *)addr);
disable:
perf_event_disable_inatomic(bp);
--
2.25.1
[PATCH 1/3] powerpc/kprobes: Use appropriate format specifier for printing kernel address
From: Balamuruhan S
Change use of %p to %pK when printing address of the instruction slot so
that the actual kernel address is visible for privileged users.
Signed-off-by: Balamuruhan S
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/optprobes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index ef0924b0809d..d5f8c25b7cac 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe
*op, struct kprobe *p)
/* Setup template */
/* We can optimize this via patch_instruction_window later */
size = (TMPL_END_IDX * sizeof(kprobe_opcode_t)) / sizeof(int);
- pr_devel("Copying template to %p, size %lu\n", buff, size);
+ pr_devel("Copying template to %pK, size %lu\n", (void *)buff, size);
for (i = 0; i < size; i++) {
rc = patch_instruction(buff + i, *(optprobe_template_entry +
i));
if (rc) {
--
2.25.1
[PATCH 2/3] powerpc/ftrace: Use appropriate format specifier for printing kernel addresses
Update use of printk format specifiers in ftrace code, so that addresses
are made visible for privileged users, or always for pr_devel() code:
- change %lx to use %px or %pK
- change %p to %px or %pK
- add %pS in certain places to show the symbol as well
Signed-off-by: Balamuruhan S
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/trace/ftrace.c | 74 +-
1 file changed, 41 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c
b/arch/powerpc/kernel/trace/ftrace.c
index 679d5249b002..29b77204f46d 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -83,8 +83,8 @@ ftrace_modify_code(unsigned long ip, unsigned int old,
unsigned int new)
/* Make sure it is what we expect it to be */
if (replaced != old) {
- pr_err("%p: replaced (%#x) != old (%#x)",
- (void *)ip, replaced, old);
+ pr_err("%pK (%pS): replaced (%#x) != old (%#x)",
+ (void *)ip, (void *)ip, replaced, old);
return -EINVAL;
}
@@ -152,19 +152,20 @@ __ftrace_make_nop(struct module *mod,
/* lets find where the pointer goes */
tramp = find_bl_target(ip, op);
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
if (module_trampoline_target(mod, tramp, &ptr)) {
pr_err("Failed to get trampoline target\n");
return -EFAULT;
}
- pr_devel("trampoline target %lx", ptr);
+ pr_devel("trampoline target 0x%px", (void *)ptr);
entry = ppc_global_function_entry((void *)addr);
/* This should match what was called */
if (ptr != entry) {
- pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ pr_err("addr 0x%pK does not match expected 0x%pK\n",
+ (void *)ptr, (void *)entry);
return -EINVAL;
}
@@ -173,7 +174,8 @@ __ftrace_make_nop(struct module *mod,
pop = PPC_INST_NOP;
if (probe_kernel_read(&op, (void *)(ip - 4), 4)) {
- pr_err("Fetching instruction at %lx failed.\n", ip - 4);
+ pr_err("Fetching instruction at 0x%pK (%pS) failed.\n",
+ (void *)(ip - 4), (void *)(ip - 4));
return -EFAULT;
}
@@ -249,11 +251,11 @@ __ftrace_make_nop(struct module *mod,
* 0x4e, 0x80, 0x04, 0x20 bctr
*/
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
/* Find where the trampoline jumps to */
if (probe_kernel_read(jmp, (void *)tramp, sizeof(jmp))) {
- pr_err("Failed to read %lx\n", tramp);
+ pr_err("Failed to read 0x%pK\n", (void *)tramp);
return -EFAULT;
}
@@ -273,11 +275,11 @@ __ftrace_make_nop(struct module *mod,
if (tramp & 0x8000)
tramp -= 0x1;
- pr_devel(" %lx ", tramp);
+ pr_devel(" 0x%px ", (void *)tramp);
if (tramp != addr) {
- pr_err("Trampoline location %08lx does not match addr\n",
- tramp);
+ pr_err("Trampoline location 0x%pK does not match addr\n",
+ (void *)tramp);
return -EINVAL;
}
@@ -362,7 +364,8 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
ptr = find_bl_target(tramp, op);
if (ptr != ppc_global_function_entry((void *)_mcount)) {
- pr_debug("Trampoline target %p is not _mcount\n", (void *)ptr);
+ pr_debug("Trampoline target 0x%px (%pS) is not _mcount\n",
+ (void *)ptr, (void *)ptr);
return -1;
}
@@ -374,8 +377,8 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
#endif
op = create_branch((void *)tramp, ptr, 0);
if (!op) {
- pr_debug("%ps is not reachable from existing mcount tramp\n",
- (void *)ptr);
+ pr_debug("0x%px (%ps) is not reachable from existing mcount
tramp\n",
+ (void *)ptr, (void *)ptr);
return -1;
}
@@ -409,13 +412,13 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace
*rec, unsigned long addr)
/* Let's find where the pointer goes */
tramp = find_bl_target(ip, op);
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
if (setup_mcount_compiler_tramp(tramp)) {
/* Are other trampolines reachable? */
if (!find_ftrace_tramp(ip)) {
- pr_err("No ftrace trampolines reachable from %ps\n",
- (void *)ip);
+ pr_err("No
[PATCH 0/3] powerpc: Use proper printk format specifiers
This series changes printk format specifiers from bare %p to %px/%pK in ftrace, kprobes and hw bkpts code. In addition, use of %lx is also changed over to conform to the recommended practice. This series applies on top of the below patch series: https://lore.kernel.org/r/cover.1587654213.git.naveen.n@linux.vnet.ibm.com - Naveen Balamuruhan S (1): powerpc/kprobes: Use appropriate format specifier for printing kernel address Naveen N. Rao (2): powerpc/ftrace: Use appropriate format specifier for printing kernel addresses powerpc/hw_bkpt: Update printk format specifiers for kernel addresses arch/powerpc/kernel/hw_breakpoint.c | 5 +- arch/powerpc/kernel/optprobes.c | 2 +- arch/powerpc/kernel/trace/ftrace.c | 74 - 3 files changed, 45 insertions(+), 36 deletions(-) base-commit: 8299da600ad05b8aa0f15ec0f5f03bd40e37d6f0 -- 2.25.1
[PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
With STRICT_KERNEL_RWX, we are currently ignoring return value from
__patch_instruction() in do_patch_instruction(), resulting in the error
not being propagated back. Fix the same.
Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for
patch_instruction()")
Signed-off-by: Naveen N. Rao
---
arch/powerpc/lib/code-patching.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 3345f039a876..5c713a6c0bd8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
static int do_patch_instruction(unsigned int *addr, unsigned int instr)
{
- int err;
+ int err, rc = 0;
unsigned int *patch_addr = NULL;
unsigned long flags;
unsigned long text_poke_addr;
@@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr,
unsigned int instr)
patch_addr = (unsigned int *)(text_poke_addr) +
((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
- __patch_instruction(addr, instr, patch_addr);
+ rc = __patch_instruction(addr, instr, patch_addr);
err = unmap_patch_area(text_poke_addr);
if (err)
@@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr,
unsigned int instr)
out:
local_irq_restore(flags);
- return err;
+ return rc ? rc : err;
}
#else /* !CONFIG_STRICT_KERNEL_RWX */
--
2.25.1
Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC
Hi Sathyanarayanan, On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote: > > On 4/20/20 2:37 PM, Jon Derrick wrote: > > The existing portdrv model prevents DPC services without either OS > > control (_OSC) granted to AER services, a Host Bridge requesting Native > > AER, or using one of the 'pcie_ports=' parameters of 'native' or > > 'dpc-native'. > > > > The DPC port service driver itself will also fail to probe if the kernel > > assumes the port is using Firmware-First AER. It's a reasonable > > expectation that a port using Firmware-First AER will also be using > > Firmware-First DPC, however if a Host Bridge requests Native DPC, the > > DPC driver should allow it and not fail to bind due to AER capability > > settings. > > > > Host Bridges which request Native DPC port services will also likely > > request Native AER, however it shouldn't be a requirement. This patch > > allows ports on those Host Bridges to have DPC port services. > > > > This will avoid the unlikely situation where the port is Firmware-First > > AER and Native DPC, and a BIOS or switch firmware preconfiguration of > > the DPC trigger could result in unhandled DPC events. > > > > Signed-off-by: Jon Derrick > > --- > > drivers/pci/pcie/dpc.c | 3 ++- > > drivers/pci/pcie/portdrv_core.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > > index 7621704..3f3106f 100644 > > --- a/drivers/pci/pcie/dpc.c > > +++ b/drivers/pci/pcie/dpc.c > > @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) > > int status; > > u16 ctl, cap; > > > > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && > > + !pci_find_host_bridge(pdev->bus)->native_dpc) > Why do it in probe as well ? if host->native_dpc is not set then the > device DPC probe it self won't happen right ? Portdrv only enables the interrupt and allows the probe to occur. The probe itself will still fail if there's a mixed-mode _OSC negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1 for AER and no check for DPC. I don't know if such a platform will exist, but the kernel is already wired for 'dpc-native' so it makes sense to extend it for this.. This transform might be more readable: if (pcie_aer_get_firmware_first(pdev) && !(pcie_ports_dpc_native || hb->native_dpc)) > > return -ENOTSUPP; > > > > status = devm_request_threaded_irq(device, dev->irq, dpc_irq, > > diff --git a/drivers/pci/pcie/portdrv_core.c > > b/drivers/pci/pcie/portdrv_core.c > > index 50a9522..f2139a1 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev > > *dev) > > */ > > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && > > pci_aer_available() && > > - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) > > + (pcie_ports_dpc_native || host->native_dpc || > > +(services & PCIE_PORT_SERVICE_AER))) > > services |= PCIE_PORT_SERVICE_DPC; > > > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || > >
Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER
Hi Sathyanarayanan, On Wed, 2020-04-22 at 15:48 -0700, Kuppuswamy, Sathyanarayanan wrote: > > On 4/20/20 2:37 PM, Jon Derrick wrote: > > Some platforms have a mix of ports whose capabilities can be negotiated > > by _OSC, and some ports which are not described by ACPI and instead > > managed by Native drivers. The existing Firmware-First HEST model can > > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > > ports by advertising the HEST Global Flag and matching the type and > > class of the port (aer_hest_parse). > Is there a real use case for mixed mode (one host bridge in FF mode and > another in native)? Intel's VMD exposes PCIe segments containing Root Ports and Bridges and other DPC consumers. These extra PCIe domains aren't described by ACPI. There have been a few versions where DPC won't bind due to platform's HEST configuration. > > If the port requests Native AER through the Host Bridge's capability > > settings, the AER driver should honor those settings and allow the port > > to bind. This patch changes the definition of Firmware-First to exclude > > ports whose Host Bridges request Native AER. > > > > Signed-off-by: Jon Derrick > > --- > > drivers/pci/pcie/aer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index f4274d3..30fbd1f 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > > if (pcie_ports_native) > > return 0; > > > > + if (pci_find_host_bridge(dev->bus)->native_aer) > > + return 0; > > + > > if (!dev->__aer_firmware_first_valid) > > aer_set_firmware_first(dev); > > return dev->__aer_firmware_first; > >
[PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions
Introduce a macro PATCH_INSN() to simplify instruction patching, and to
make the error messages more uniform and useful:
- print an error message that includes the original return value
- print the function name and line numbers, so that the offending
location is clear
- always return -EPERM, which ftrace_bug() expects for proper error
handling
Also eliminate use of patch_branch() since most such uses already call
create_branch() for error checking before patching. Instead, use the
return value from create_branch() with PATCH_INSN().
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/trace/ftrace.c | 69 ++
1 file changed, 33 insertions(+), 36 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c
b/arch/powerpc/kernel/trace/ftrace.c
index 7ea0ca044b65..5cf84c0c64cb 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -31,6 +31,17 @@
#ifdef CONFIG_DYNAMIC_FTRACE
+#define PATCH_INSN(addr, instr)
\
+do {\
+ int rc = patch_instruction((unsigned int *)(addr), instr); \
+ if (rc) {\
+ pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n",
\
+ __func__, __LINE__, \
+ (void *)(addr), (void *)(addr), rc); \
+ return -EPERM; \
+ }\
+} while (0)
+
/*
* We generally only have a single long_branch tramp and at most 2 or 3 plt
* tramps generated. But, we don't use the plt tramps currently. We also allot
@@ -78,8 +89,7 @@ ftrace_modify_code(unsigned long ip, unsigned int old,
unsigned int new)
}
/* replace the text with the new text */
- if (patch_instruction((unsigned int *)ip, new))
- return -EPERM;
+ PATCH_INSN(ip, new);
return 0;
}
@@ -204,10 +214,7 @@ __ftrace_make_nop(struct module *mod,
}
#endif /* CONFIG_MPROFILE_KERNEL */
- if (patch_instruction((unsigned int *)ip, pop)) {
- pr_err("Patching NOP failed.\n");
- return -EPERM;
- }
+ PATCH_INSN(ip, pop);
return 0;
}
@@ -276,8 +283,7 @@ __ftrace_make_nop(struct module *mod,
op = PPC_INST_NOP;
- if (patch_instruction((unsigned int *)ip, op))
- return -EPERM;
+ PATCH_INSN(ip, op);
return 0;
}
@@ -322,7 +328,7 @@ static int add_ftrace_tramp(unsigned long tramp)
*/
static int setup_mcount_compiler_tramp(unsigned long tramp)
{
- int i, op;
+ unsigned int i, op;
unsigned long ptr;
static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];
@@ -366,16 +372,14 @@ static int setup_mcount_compiler_tramp(unsigned long
tramp)
#else
ptr = ppc_global_function_entry((void *)ftrace_caller);
#endif
- if (!create_branch((void *)tramp, ptr, 0)) {
+ op = create_branch((void *)tramp, ptr, 0);
+ if (!op) {
pr_debug("%ps is not reachable from existing mcount tramp\n",
(void *)ptr);
return -1;
}
- if (patch_branch((unsigned int *)tramp, ptr, 0)) {
- pr_debug("REL24 out of range!\n");
- return -1;
- }
+ PATCH_INSN(tramp, op);
if (add_ftrace_tramp(tramp)) {
pr_debug("No tramp locations left\n");
@@ -416,10 +420,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace
*rec, unsigned long addr)
}
}
- if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
- pr_err("Patching NOP failed.\n");
- return -EPERM;
- }
+ PATCH_INSN(ip, PPC_INST_NOP);
return 0;
}
@@ -557,15 +558,13 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long
addr)
}
/* Ensure branch is within 24 bits */
- if (!create_branch(ip, tramp, BRANCH_SET_LINK)) {
+ op[0] = create_branch(ip, tramp, BRANCH_SET_LINK);
+ if (!op[0]) {
pr_err("Branch out of range\n");
return -EINVAL;
}
- if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
- pr_err("REL24 out of range!\n");
- return -EINVAL;
- }
+ PATCH_INSN(ip, op[0]);
return 0;
}
@@ -603,8 +602,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long
addr)
pr_devel("write to %lx\n", rec->ip);
- if (patch_instruction((unsigned int *)ip, op))
- return -EPERM;
+ PATCH_INSN(ip, op);
return 0;
}
@@ -650,11 +648,14 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace
*rec, unsigned long addr)
[PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
patch_instruction() can fail in some scenarios. Add appropriate error
checking so that such failures are caught and logged, and suitable error
code is returned.
Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to
patch_instruction()")
Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/kprobes.c | 10 +++-
arch/powerpc/kernel/optprobes.c | 99 ++---
2 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 81efb605113e..4a297ae2bd87 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
void arch_arm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+ int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+
+ if (rc)
+ WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr,
rc);
}
NOKPROBE_SYMBOL(arch_arm_kprobe);
void arch_disarm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, p->opcode);
+ int rc = patch_instruction(p->addr, p->opcode);
+
+ if (rc)
+ WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr,
rc);
}
NOKPROBE_SYMBOL(arch_disarm_kprobe);
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..046485bb0a52 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe
*op)
}
}
+#define PATCH_INSN(addr, instr)
\
+do {\
+ int rc = patch_instruction((unsigned int *)(addr), instr); \
+ if (rc) {\
+ pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n",
\
+ __func__, __LINE__, \
+ (void *)(addr), (void *)(addr), rc); \
+ return rc; \
+ }\
+} while (0)
+
/*
* emulate_step() requires insn to be emulated as
* second parameter. Load register 'r4' with the
* instruction.
*/
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static int patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
{
/* addis r4,0,(insn)@h */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+ PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
((val >> 16) & 0x));
addr++;
/* ori r4,r4,(insn)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(4) |
___PPC_RS(4) | (val & 0x));
+
+ return 0;
}
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'r3' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static int patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
+ PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
((val >> 48) & 0x));
addr++;
/* ori r3,r3,(op)@higher */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
___PPC_RS(3) | ((val >> 32) & 0x));
addr++;
/* rldicr r3,r3,32,31 */
- patch_instruction(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31));
addr++;
/* oris r3,r3,(op)@h */
- patch_instruction(addr, PPC_INST_ORIS | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORIS | ___PPC_RA(3) |
___PPC_RS(3) | ((val >> 16) & 0x));
addr++;
/* ori r3,r3,(op)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
___PPC_RS(3) | (val & 0x));
+
+ return 0;
}
int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe
*p)
@@ -216,14 +231,18 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe
*op, struct kprobe *p)
* be within 32MB on either side of the current instruction.
*/
b_offset = (unsigned long)buff - (unsigned long)p->addr;
- if (!is_offset_in_branch_range(b_offset))
+
[PATCH 0/3] powerpc: Enhance error handling with patch_instruction()
This patchset updates error handling with patch_instruction(). The first patch fixes an issue with do_patch_instruction() with STRICT_KERNEL_RWX wherein errors were not being returned back. The second and third patches update users of patch_instruction() in ftrace and kprobes code to properly validate return value from patch_instruction() and to notify errors. - Naveen Naveen N. Rao (3): powerpc: Properly return error code from do_patch_instruction() powerpc/ftrace: Simplify error checking when patching instructions powerpc/kprobes: Check return value of patch_instruction() arch/powerpc/kernel/kprobes.c | 10 ++- arch/powerpc/kernel/optprobes.c| 99 -- arch/powerpc/kernel/trace/ftrace.c | 69 ++--- arch/powerpc/lib/code-patching.c | 6 +- 4 files changed, 123 insertions(+), 61 deletions(-) base-commit: 8299da600ad05b8aa0f15ec0f5f03bd40e37d6f0 -- 2.25.1
Re: [PATCH][next] ASoC: fsl_easrc: fix spelling mistake "prefitler" -> "prefilter"
On Thu, 23 Apr 2020 09:39:22 +0100, Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in a deb_dbg message, fix it. > > Signed-off-by: Colin Ian King > --- > sound/soc/fsl/fsl_easrc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.8 Thanks! [1/1] ASoC: fsl_easrc: fix spelling mistake "prefitler" -> "prefilter" commit: 76ec4aea9fd8117f064caa63ee6f7fbcb70eeb2c 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 04/29] staging: media: ipu3: use vmap instead of reimplementing it
On Tue, Apr 14, 2020 at 03:13:23PM +0200, Christoph Hellwig wrote: > Just use vmap instead of messing with vmalloc internals. > > Signed-off-by: Christoph Hellwig > Acked-by: Peter Zijlstra (Intel) Thanks! Acked-by: Sakari Ailus -- Sakari Ailus
Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
On 22/04/2020 23:36, Rich Felker wrote:
> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
>> Yeah I had a bit of a play around with musl (which is very nice code I
>> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
>> Fortunately adding it doesn't change code generation for me, but it
>> should be fixed. glibc had the same bug at one point I think (probably
>> due to syscall ABI documentation not existing -- something now lives in
>> linux/Documentation/powerpc/syscall64-abi.rst).
>
> Do you know anywhere I can read about the ctr issue, possibly the
> relevant glibc bug report? I'm not particularly familiar with ppc
> register file (at least I have to refamiliarize myself every time I
> work on this stuff) so it'd be nice to understand what's
> potentially-wrong now.
My understanding is the ctr issue only happens for vDSO calls where it
fallback to a syscall in case an error (invalid argument, etc. and
assuming if vDSO does not fallback to a syscall it always succeed).
This makes the vDSO call on powerpc to have same same ABI constraint
as a syscall, where it clobbers CR0.
On glibc we handle by simulating a function call and analysing the CR0
result:
__asm__ __volatile__
("mtctr %0\n\t"
"bctrl\n\t"
"mfcr %0\n\t"
"0:"
: "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5), "+r" (r6),
"+r" (r7), "+r" (r8)
: : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");
__asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));
On musl you don't have this issue because it does not enable vDSO
support on powerpc. And if it eventually does it with the VDSO_*
macros the only issue I see is on when vDSO fallbacks to the syscall
and it also fails (the return code won't be negated since on musl it
uses a default C function pointer issue which does not model the CR0
kernel abi).
So I think the extra ctr constraint on glibc powerpc syscall code is
not really required. I think I have some patches to optimize this a
bit based on previous discussions.
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/23 19:00, Christian Borntraeger wrote:
On 23.04.20 12:58, Tianjia Zhang wrote:
On 2020/4/23 18:39, Cornelia Huck wrote:
On Thu, 23 Apr 2020 11:01:43 +0800
Tianjia Zhang wrote:
On 2020/4/23 0:04, Cornelia Huck wrote:
On Wed, 22 Apr 2020 17:58:04 +0200
Christian Borntraeger wrote:
On 22.04.20 15:45, Cornelia Huck wrote:
On Wed, 22 Apr 2020 20:58:04 +0800
Tianjia Zhang wrote:
In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu'
structure. Earlier than historical reasons, many kvm-related function
s/Earlier than/For/ ?
parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time.
This patch does a unified cleanup of these remaining redundant parameters.
Signed-off-by: Tianjia Zhang
---
arch/s390/kvm/kvm-s390.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e335a7e5ead7..d7bb2e7a07ff 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
return rc;
}
-static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+static void sync_regs_fmt2(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *kvm_run = vcpu->run;
struct runtime_instr_cb *riccb;
struct gs_cb *gscb;
@@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu,
struct kvm_run *kvm_run)
}
if (vcpu->arch.gs_enabled) {
current->thread.gs_cb = (struct gs_cb *)
- &vcpu->run->s.regs.gscb;
+ &kvm_run->s.regs.gscb;
Not sure if these changes (vcpu->run-> => kvm_run->) are really worth
it. (It seems they amount to at least as much as the changes advertised
in the patch description.)
Other opinions?
Agreed. It feels kind of random. Maybe just do the first line (move kvm_run
from the
function parameter list into the variable declaration)? Not sure if this is
better.
There's more in this patch that I cut... but I think just moving
kvm_run from the parameter list would be much less disruptive.
I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but
there will be more disruptive, not less.
I just fail to see the benefit; sure, kvm_run-> is convenient, but the
current code is just fine, and any rework should be balanced against
the cost (e.g. cluttering git annotate).
cluttering git annotate ? Does it mean Fix ("comment"). Is it possible to
solve this problem by splitting this patch?
No its about breaking git blame (and bugfix backports) for just a cosmetic
improvement.
And I agree with Conny: the cost is higher than the benefit.
I will make a fix in the v3 version. Help to see if there are problems
with the next few patches.
Thanks,
Tianjia
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 23.04.20 12:58, Tianjia Zhang wrote:
>
>
> On 2020/4/23 18:39, Cornelia Huck wrote:
>> On Thu, 23 Apr 2020 11:01:43 +0800
>> Tianjia Zhang wrote:
>>
>>> On 2020/4/23 0:04, Cornelia Huck wrote:
On Wed, 22 Apr 2020 17:58:04 +0200
Christian Borntraeger wrote:
> On 22.04.20 15:45, Cornelia Huck wrote:
>> On Wed, 22 Apr 2020 20:58:04 +0800
>> Tianjia Zhang wrote:
>>
>>> In the current kvm version, 'kvm_run' has been included in the
>>> 'kvm_vcpu'
>>> structure. Earlier than historical reasons, many kvm-related function
>>
>> s/Earlier than/For/ ?
>>
>>> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same
>>> time.
>>> This patch does a unified cleanup of these remaining redundant
>>> parameters.
>>>
>>> Signed-off-by: Tianjia Zhang
>>> ---
>>> arch/s390/kvm/kvm-s390.c | 37 ++---
>>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index e335a7e5ead7..d7bb2e7a07ff 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>>> return rc;
>>> }
>>> -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run
>>> *kvm_run)
>>> +static void sync_regs_fmt2(struct kvm_vcpu *vcpu)
>>> {
>>> + struct kvm_run *kvm_run = vcpu->run;
>>> struct runtime_instr_cb *riccb;
>>> struct gs_cb *gscb;
>>> @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu
>>> *vcpu, struct kvm_run *kvm_run)
>>> }
>>> if (vcpu->arch.gs_enabled) {
>>> current->thread.gs_cb = (struct gs_cb *)
>>> - &vcpu->run->s.regs.gscb;
>>> + &kvm_run->s.regs.gscb;
>>
>> Not sure if these changes (vcpu->run-> => kvm_run->) are really worth
>> it. (It seems they amount to at least as much as the changes advertised
>> in the patch description.)
>>
>> Other opinions?
>
> Agreed. It feels kind of random. Maybe just do the first line (move
> kvm_run from the
> function parameter list into the variable declaration)? Not sure if this
> is better.
>
There's more in this patch that I cut... but I think just moving
kvm_run from the parameter list would be much less disruptive.
>>>
>>> I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but
>>> there will be more disruptive, not less.
>>
>> I just fail to see the benefit; sure, kvm_run-> is convenient, but the
>> current code is just fine, and any rework should be balanced against
>> the cost (e.g. cluttering git annotate).
>>
>
> cluttering git annotate ? Does it mean Fix ("comment"). Is it possible
> to solve this problem by splitting this patch?
No its about breaking git blame (and bugfix backports) for just a cosmetic
improvement.
And I agree with Conny: the cost is higher than the benefit.
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On 2020/4/23 18:39, Cornelia Huck wrote:
On Thu, 23 Apr 2020 11:01:43 +0800
Tianjia Zhang wrote:
On 2020/4/23 0:04, Cornelia Huck wrote:
On Wed, 22 Apr 2020 17:58:04 +0200
Christian Borntraeger wrote:
On 22.04.20 15:45, Cornelia Huck wrote:
On Wed, 22 Apr 2020 20:58:04 +0800
Tianjia Zhang wrote:
In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu'
structure. Earlier than historical reasons, many kvm-related function
s/Earlier than/For/ ?
parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time.
This patch does a unified cleanup of these remaining redundant parameters.
Signed-off-by: Tianjia Zhang
---
arch/s390/kvm/kvm-s390.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e335a7e5ead7..d7bb2e7a07ff 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
return rc;
}
-static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+static void sync_regs_fmt2(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *kvm_run = vcpu->run;
struct runtime_instr_cb *riccb;
struct gs_cb *gscb;
@@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
}
if (vcpu->arch.gs_enabled) {
current->thread.gs_cb = (struct gs_cb *)
- &vcpu->run->s.regs.gscb;
+ &kvm_run->s.regs.gscb;
Not sure if these changes (vcpu->run-> => kvm_run->) are really worth
it. (It seems they amount to at least as much as the changes advertised
in the patch description.)
Other opinions?
Agreed. It feels kind of random. Maybe just do the first line (move kvm_run
from the
function parameter list into the variable declaration)? Not sure if this is
better.
There's more in this patch that I cut... but I think just moving
kvm_run from the parameter list would be much less disruptive.
I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but
there will be more disruptive, not less.
I just fail to see the benefit; sure, kvm_run-> is convenient, but the
current code is just fine, and any rework should be balanced against
the cost (e.g. cluttering git annotate).
cluttering git annotate ? Does it mean Fix ("comment"). Is it
possible to solve this problem by splitting this patch?
[PATCH v8 3/3] Self save API integration
The commit makes the self save API available outside the firmware by defining an OPAL wrapper. This wrapper has a similar interface to that of self restore and expects the cpu pir, SPR number, minus the value of that SPR to be passed in its paramters and returns OPAL_SUCCESS on success. It adds a device-tree node signifying support for self-save after verifying the stop API version compatibility. The commit also documents both the self-save and the self-restore API calls along with their working and usage. Signed-off-by: Pratik Rajesh Sampat --- doc/opal-api/opal-slw-self-save-reg-181.rst | 51 ++ doc/opal-api/opal-slw-set-reg-100.rst | 5 + doc/power-management.rst| 48 + hw/slw.c| 106 include/opal-api.h | 3 +- include/p9_stop_api.H | 18 include/skiboot.h | 3 + 7 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 doc/opal-api/opal-slw-self-save-reg-181.rst diff --git a/doc/opal-api/opal-slw-self-save-reg-181.rst b/doc/opal-api/opal-slw-self-save-reg-181.rst new file mode 100644 index ..f20e9b81 --- /dev/null +++ b/doc/opal-api/opal-slw-self-save-reg-181.rst @@ -0,0 +1,51 @@ +.. OPAL_SLW_SELF_SAVE_REG: + +OPAL_SLW_SELF_SAVE_REG +== + +.. code-block:: c + + #define OPAL_SLW_SELF_SAVE_REG 181 + + int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn); + +:ref:`OPAL_SLW_SELF_SAVE_REG` is used to inform low-level firmware to save +the current contents of the SPR before entering a state of loss and +also restore the content back on waking up from a deep stop state. + +An OPAL call `OPAL_SLW_SET_REG` exists which is similar in function as +saving and restoring the SPR, with one difference being that the value of the +SPR must also be supplied in the parameters. +Complete reference: doc/opal-api/opal-slw-set-reg-100.rst + +Parameters +-- + +``uint64_t cpu_pir`` + This parameter specifies the pir of the cpu for which the call is being made. +``uint64_t sprn`` + This parameter specifies the spr number as mentioned in p9_stop_api.H + The list of SPRs supported is as follows. + P9_STOP_SPR_DAWR, + P9_STOP_SPR_HSPRG0, + P9_STOP_SPR_LDBAR, + P9_STOP_SPR_LPCR, + P9_STOP_SPR_PSSCR, + P9_STOP_SPR_MSR, + P9_STOP_SPR_HRMOR, + P9_STOP_SPR_HMEER, + P9_STOP_SPR_PMCR, + P9_STOP_SPR_PTCR + + The property "ibm,opal-self-save" is supplied to the device tree to advterise + support. + +Returns +--- + +:ref:`OPAL_UNSUPPORTED` + If spr restore is not supported by pore engine. +:ref:`OPAL_PARAMETER` + Invalid handle for the pir/chip +:ref:`OPAL_SUCCESS` + On success diff --git a/doc/opal-api/opal-slw-set-reg-100.rst b/doc/opal-api/opal-slw-set-reg-100.rst index 2e8f1bd6..ee3e68ce 100644 --- a/doc/opal-api/opal-slw-set-reg-100.rst +++ b/doc/opal-api/opal-slw-set-reg-100.rst @@ -21,6 +21,11 @@ In Power 9, it uses p9_stop_save_cpureg(), api provided by self restore code, to inform the spr with their corresponding values with which they must be restored. +An OPAL call `OPAL_SLW_SELF_SAVE_REG` exists which is similar in function +saving and restoring the SPR, with one difference being that the value of the +SPR doesn't need to be passed in the parameters, only with the SPR number +the firmware can identify, save and restore the values for the same. +Complete reference: doc/opal-api/opal-slw-self-save-reg-181.rst Parameters -- diff --git a/doc/power-management.rst b/doc/power-management.rst index 76491a71..d6bd5358 100644 --- a/doc/power-management.rst +++ b/doc/power-management.rst @@ -15,3 +15,51 @@ On boot, specific stop states can be disabled via setting a mask. For example, to disable all but stop 0,1,2, use ~0xE000. :: nvram -p ibm,skiboot --update-config opal-stop-state-disable-mask=0x1FFF + +Saving and restoring Special Purpose Registers(SPRs) + + +When a CPU wakes up from a deep stop state which can result in +hypervisor state loss, all the SPRs are lost. The Linux Kernel expects +a small set of SPRs to contain an expected value when the CPU wakes up +from such a deep stop state. The microcode firmware provides the +following two APIs, collectively known as the stop-APIs, to allow the +kernel/OPAL to specify this set of SPRs and the value that they need +to be restored with on waking up from a deep stop state. + +Self-restore: +int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val); +The SPR number and the value of the that SPR must be restored with on +wakeup from the deep-stop state must be specified. When this call is +made, the microcode inserts instruction into the HOMER region to +restore the content of the SPR to the specified value on wakeup from a +deep-stop state. These inst
[PATCH v8 1/1] powerpc/powernv: Introduce support and parsing for self-save API
This commit introduces and leverages the Self save API. The difference
between self-save and self-restore is that the value to be saved for the
SPR does not need to be passed to the call.
Add the new Self Save OPAL API call in the list of OPAL calls.
The device tree is parsed looking for the property "ibm,opal-self-save"
If self-save is supported then for all SPRs self-save is invoked for all
P9 supported registers. In the case self-save fails corresponding
self-restore call is invoked as a fallback.
Signed-off-by: Pratik Rajesh Sampat
---
arch/powerpc/include/asm/opal-api.h| 3 +-
arch/powerpc/include/asm/opal.h| 1 +
arch/powerpc/platforms/powernv/idle.c | 73 ++
arch/powerpc/platforms/powernv/opal-call.c | 1 +
4 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/opal-api.h
b/arch/powerpc/include/asm/opal-api.h
index 1dffa3cb16ba..7ba698369083 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -214,7 +214,8 @@
#define OPAL_SECVAR_GET176
#define OPAL_SECVAR_GET_NEXT 177
#define OPAL_SECVAR_ENQUEUE_UPDATE 178
-#define OPAL_LAST 178
+#define OPAL_SLW_SELF_SAVE_REG 181
+#define OPAL_LAST 181
#define QUIESCE_HOLD 1 /* Spin all calls at entry */
#define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 9986ac34b8e2..a370b0e8d899 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -204,6 +204,7 @@ int64_t opal_handle_hmi2(__be64 *out_flags);
int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
int64_t opal_unregister_dump_region(uint32_t id);
int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
+int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t
pe_number);
int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
diff --git a/arch/powerpc/platforms/powernv/idle.c
b/arch/powerpc/platforms/powernv/idle.c
index 78599bca66c2..ada7ece24521 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -32,6 +32,11 @@
#define P9_STOP_SPR_MSR 2000
#define P9_STOP_SPR_PSSCR 855
+/* Caching the self-save functionality, lpcr, ptcr support */
+DEFINE_STATIC_KEY_FALSE(self_save_available);
+DEFINE_STATIC_KEY_FALSE(is_lpcr_self_save);
+DEFINE_STATIC_KEY_FALSE(is_ptcr_self_save);
+
static u32 supported_cpuidle_states;
struct pnv_idle_states_t *pnv_idle_states;
int nr_pnv_idle_states;
@@ -61,6 +66,35 @@ static bool deepest_stop_found;
static unsigned long power7_offline_type;
+/*
+ * Cache support for SPRs that support self-save as well as kernel save restore
+ * so that kernel does not duplicate efforts in saving and restoring SPRs
+ */
+static void cache_spr_self_save_support(u64 sprn)
+{
+ switch (sprn) {
+ case SPRN_LPCR:
+ static_branch_enable(&is_lpcr_self_save);
+ break;
+ case SPRN_PTCR:
+ static_branch_enable(&is_ptcr_self_save);
+ break;
+ }
+}
+
+static int pnv_save_one_spr(u64 pir, u64 sprn, u64 val)
+{
+ if (static_branch_likely(&self_save_available)) {
+ int rc = opal_slw_self_save_reg(pir, sprn);
+
+ if (!rc) {
+ cache_spr_self_save_support(sprn);
+ return rc;
+ }
+ }
+ return opal_slw_set_reg(pir, sprn, val);
+}
+
static int pnv_save_sprs_for_deep_states(void)
{
int cpu;
@@ -72,6 +106,7 @@ static int pnv_save_sprs_for_deep_states(void)
* same across all cpus.
*/
uint64_t lpcr_val = mfspr(SPRN_LPCR);
+ uint64_t ptcr_val = mfspr(SPRN_PTCR);
uint64_t hid0_val = mfspr(SPRN_HID0);
uint64_t hid1_val = mfspr(SPRN_HID1);
uint64_t hid4_val = mfspr(SPRN_HID4);
@@ -84,30 +119,34 @@ static int pnv_save_sprs_for_deep_states(void)
uint64_t pir = get_hard_smp_processor_id(cpu);
uint64_t hsprg0_val = (uint64_t)paca_ptrs[cpu];
- rc = opal_slw_set_reg(pir, SPRN_HSPRG0, hsprg0_val);
+ rc = pnv_save_one_spr(pir, SPRN_HSPRG0, hsprg0_val);
if (rc != 0)
return rc;
- rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
+ rc = pnv_save_one_spr(pir, SPRN_LPCR, lpcr_val);
if (rc != 0)
return rc;
+ /*
+* No need to check for failure, if firmware fails to save then
+
[PATCH v8 0/1] powerpc/powernv: Introduce support and parsing for self-save API
v7: https://lkml.org/lkml/2020/4/16/247 Changelog v7 --> v8 Simplified kernel design. Introducing an approach which eliminates the need for having support and preference for SPRs that need to be saved. Instead a simple self-save property is advertised and if it exists then self-save is called otherwise self-restore is resorted to. Complete specification of the approach is described below in the cover-letter. Background == The power management framework on POWER systems include core idle states that lose context. Deep idle states namely "winkle" on POWER8 and "stop4" and "stop5" on POWER9 can be entered by a CPU to save different levels of power, as a consequence of which all the hypervisor resources such as SPRs and SCOMs are lost. For most SPRs, saving and restoration of content for SPRs and SCOMs is handled by the hypervisor kernel prior to entering an post exit from an idle state respectively. However, there is a small set of critical SPRs and XSCOMs that are expected to contain sane values even before the control is transferred to the hypervisor kernel at system reset vector. For this purpose, microcode firmware provides a mechanism to restore values on certain SPRs. The communication mechanism between the hypervisor kernel and the microcode is a standard interface called sleep-winkle-engine (SLW) on Power8 and Stop-API on Power9 which is abstracted by OPAL calls from the hypervisor kernel. The Stop-API provides an interface known as the self-restore API, to which the SPR number and a predefined value to be restored on wake-up from a deep stop state is supplied. Motivation to introduce a new Stop-API == The self-restore API expects not just the SPR number but also the value with which the SPR is restored. This is good for those SPRs such as HSPRG0 whose values do not change at runtime, since for them, the kernel can invoke the self-restore API at boot time once the values of these SPRs are determined. However, there are use-cases where-in the value to be saved cannot be known or cannot be updated in the layer it currently is. The shortcomings and the new use-cases which cannot be served by the existing self-restore API, serves as motivation for a new API: Shortcoming1: In a special wakeup scenario, SPRs such as PSSCR, whose values can change at runtime, are compelled to make the self-restore API call every time before entering a deep-idle state rendering it to be prohibitively expensive Shortcoming2: The value of LPCR is dynamic based on if the CPU is entered a stop state during cpu idle versus cpu hotplug. Today, an additional self-restore call is made before entering CPU-Hotplug to clear the PECE1 bit in stop-API so that if we are woken up by a special wakeup on an offlined CPU, we go back to stop with the the bit cleared. There is a overhead of an extra call New Use-case: - In the case where the hypervisor is running on an ultravisor environment, the boot time is too late in the cycle to make the self-restore API calls, as these cannot be invoked from an non-secure context anymore To address these shortcomings, the firmware provides another API known as the self-save API. The self-save API only takes the SPR number as a parameter and will ensure that on wakeup from a deep-stop state the SPR is restored with the value that it contained prior to entering the deep-stop. Contrast between self-save and self-restore APIs Before entering deep idle |---| > | HCODE A | | |---| |-|| | CPU || |-|| | |---| |>| HCODE B | On waking up |---| from deep idle When a self-restore API is invoked, the HCODE inserts instructions into "HCODE B" region of the above figure to restore the content of the SPR to the said value. The "HCODE B" region gets executed soon after the CPU wakes up from a deep idle state, thus executing the inserted instructions, thereby restoring the contents of the SPRs to the required values. When a self-save API is invoked, the HCODE inserts instructions into the "HCODE A" region of the above figure to save the content of the SPR into some location in memory. It also inserts instructions into the "HCODE B" region to restore the content of the SPR to the corresponding value saved in the memory by the instructions in "HCODE A" region. Thus, in contrast with self-restore, the self-save API *does not* need a value to be passed to it, since it ensures that the value of SPR before entering deep stop is saved, and subsequently the same value is restored. Self-save and self-restore are complementary features since, self-restore can help in restoring a different value in
[PATCH v8 1/3] Self Save: Introducing Support for SPR Self Save
From: Prem Shanker Jha The commit is a merger of commits that makes the following changes: 1. Commit fixes some issues with code found during integration test - replacement of addi with xor instruction during self save API. - fixing instruction generation for MFMSR during self save - data struct updates in STOP API - error RC updates for hcode image build - HOMER parser updates. - removed self save support for URMOR and HRMOR - code changes for compilation with OPAL - populating CME Image header with unsecure HOMER address. Key_Cronus_Test=PM_REGRESS Change-Id: I7cedcc466267c4245255d8d75c01ed695e316720 Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66580 Tested-by: FSP CI Jenkins Tested-by: HWSV CI Tested-by: PPE CI Tested-by: Jenkins Server Tested-by: Cronus HW CI Tested-by: Hostboot CI Reviewed-by: Gregory S. Still Reviewed-by: RAHUL BATRA Reviewed-by: Jennifer A. Stofer Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/66587 Reviewed-by: Christian R. Geddes Signed-off-by: Prem Shanker Jha Signed-off-by: Akshay Adiga Signed-off-by: Pratik Rajesh Sampat 2. The commit also incorporates changes that make STOP API project agnostic changes include defining wrapper functions which call legacy API. It also adds duplicate enum members which start with prefix PROC instead of P9. Key_Cronus_Test=PM_REGRESS Change-Id: If87970f3e8cf9b507f33eb1be249e03eb3836a5e RTC: 201128 Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71307 Tested-by: FSP CI Jenkins Tested-by: Jenkins Server Tested-by: Hostboot CI Tested-by: Cronus HW CI Reviewed-by: RANGANATHPRASAD G. BRAHMASAMUDRA Reviewed-by: Gregory S. Still Reviewed-by: Jennifer A Stofer Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/71314 Tested-by: Jenkins OP Build CI Tested-by: Jenkins OP HW Reviewed-by: Daniel M. Crowell Signed-off-by: Prem Shanker Jha Signed-off-by: Pratik Rajesh Sampat --- include/p9_stop_api.H| 79 +- libpore/p9_cpu_reg_restore_instruction.H | 4 + libpore/p9_stop_api.C| 954 +-- libpore/p9_stop_api.H| 115 ++- libpore/p9_stop_data_struct.H| 4 +- libpore/p9_stop_util.H | 7 +- 6 files changed, 721 insertions(+), 442 deletions(-) diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H index 79abd000..9d3bc1e5 100644 --- a/include/p9_stop_api.H +++ b/include/p9_stop_api.H @@ -63,6 +63,26 @@ typedef enum P9_STOP_SPR_PMCR=884, // core register P9_STOP_SPR_HID = 1008, // core register P9_STOP_SPR_MSR = 2000, // thread register + +//enum members which are project agnostic +PROC_STOP_SPR_DAWR=180, // thread register +PROC_STOP_SPR_CIABR =187, // thread register +PROC_STOP_SPR_DAWRX =188, // thread register +PROC_STOP_SPR_HSPRG0 =304, // thread register +PROC_STOP_SPR_HRMOR =313, // core register +PROC_STOP_SPR_LPCR=318, // thread register +PROC_STOP_SPR_HMEER =337, // core register +PROC_STOP_SPR_PTCR=464, // core register +PROC_STOP_SPR_USPRG0 =496, // thread register +PROC_STOP_SPR_USPRG1 =497, // thread register +PROC_STOP_SPR_URMOR =505, // core register +PROC_STOP_SPR_SMFCTRL =511, // thread register +PROC_STOP_SPR_LDBAR =850, // thread register +PROC_STOP_SPR_PSSCR =855, // thread register +PROC_STOP_SPR_PMCR=884, // core register +PROC_STOP_SPR_HID = 1008, // core register +PROC_STOP_SPR_MSR = 2000, // thread register + } CpuReg_t; /** @@ -85,6 +105,8 @@ typedef enum STOP_SAVE_SCOM_ENTRY_UPDATE_FAILED = 12, STOP_SAVE_INVALID_FUSED_CORE_STATUS = 13, STOP_SAVE_FAIL = 14, // for internal failure within firmware. +STOP_SAVE_SPR_ENTRY_MISSING = 15, +STOP_SAVE_SPR_BIT_POS_RESERVE= 16, } StopReturnCode_t; /** @@ -101,7 +123,20 @@ typedef enum P9_STOP_SCOM_RESET = 6, P9_STOP_SCOM_OR_APPEND = 7, P9_STOP_SCOM_AND_APPEND = 8, -P9_STOP_SCOM_OP_MAX = 9 +P9_STOP_SCOM_OP_MAX = 9, + +//enum members which are project agnostic +PROC_STOP_SCOM_OP_MIN = 0, +PROC_STOP_SCOM_APPEND = 1, +PROC_STOP_SCOM_REPLACE= 2, +PROC_STOP_SCOM_OR = 3, +PROC_STOP_SCOM_AND= 4, +PROC_STOP_SCOM_NOOP = 5, +PROC_STOP_SCOM_RESET = 6, +PROC_STOP_SCOM_OR_APPEND = 7, +PROC_STOP_SCOM_AND_APPEND = 8, +PROC_STOP_SCOM_OP_MAX = 9, + } ScomOperation_t; /** @@ -114,9 +149,49 @@ typedef enum P9_STOP_SECTION_EQ_SCOM = 2, P9_STOP_SECTION_L2 = 3, P9_STOP_SECTION_L3 = 4, -P9_STOP_SECTION_MAX = 5 +P9_STOP_SECTION_MAX = 5, + +//enum members which are project agnostic +PROC_STOP_SEC
[PATCH v8 2/3] API to verify the STOP API and image compatibility
From: Prem Shanker Jha
Commit defines a new API primarily intended for OPAL to determine
cpu register save API's compatibility with HOMER layout and
self save restore. It can help OPAL determine if version of
API integrated with OPAL is different from hostboot.
Change-Id: Ic0de45a336cfb8b6b6096a10ac1cd3ffbaa44fc0
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/77612
Tested-by: FSP CI Jenkins
Tested-by: Jenkins Server
Tested-by: Hostboot CI
Reviewed-by: RANGANATHPRASAD G. BRAHMASAMUDRA
Reviewed-by: Gregory S Still
Reviewed-by: Jennifer A Stofer
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/77614
Tested-by: Jenkins OP Build CI
Tested-by: Jenkins OP HW
Reviewed-by: Daniel M Crowell
Signed-off-by: Pratik Rajesh Sampat
---
include/p9_stop_api.H| 25 ++
libpore/p9_cpu_reg_restore_instruction.H | 7 ++-
libpore/p9_hcd_memmap_base.H | 7 +++
libpore/p9_stop_api.C| 58 +++-
libpore/p9_stop_api.H| 26 ++-
libpore/p9_stop_util.H | 20
6 files changed, 130 insertions(+), 13 deletions(-)
diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H
index 9d3bc1e5..cb5ffd6f 100644
--- a/include/p9_stop_api.H
+++ b/include/p9_stop_api.H
@@ -107,6 +107,7 @@ typedef enum
STOP_SAVE_FAIL = 14, // for internal failure within
firmware.
STOP_SAVE_SPR_ENTRY_MISSING = 15,
STOP_SAVE_SPR_BIT_POS_RESERVE= 16,
+STOP_SAVE_API_IMG_INCOMPATIBLE = 18,
} StopReturnCode_t;
/**
@@ -161,6 +162,14 @@ typedef enum
} ScomSection_t;
+/**
+ * @brief versions pertaining relvant to STOP API.
+ */
+typedef enum
+{
+STOP_API_VER= 0x00,
+STOP_API_VER_CONTROL= 0x02,
+} VersionList_t;
/**
@@ -192,6 +201,14 @@ typedef enum
BIT_POS_USPRG1 = 30,
} SprBitPositionList_t;
+typedef enum
+{
+SMF_SUPPORT_MISSING_IN_HOMER = 0x01,
+SELF_SUPPORT_MISSING_FOR_LE_HYP = 0x02,
+IPL_RUNTIME_CPU_SAVE_VER_MISMATCH= 0x04,
+SELF_RESTORE_VER_MISMATCH= 0x08,
+} VersionIncompList_t;
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -230,6 +247,14 @@ StopReturnCode_t p9_stop_save_scom( void* const i_pImage,
const ScomOperation_t i_operation,
const ScomSection_t i_section );
+/**
+ * @brief verifies if API is compatible of current HOMER image.
+ * @param[in] i_pImagepoints to the start of HOMER image of P9 chip.
+ * @param[out] o_inCompVector list of incompatibilities found.
+ * @return STOP_SAVE_SUCCESS if if API succeeds, error code otherwise.
+ */
+StopReturnCode_t proc_stop_api_discover_capability( void* const i_pImage,
uint64_t* o_inCompVector );
+
#ifdef __cplusplus
} // extern "C"
}; // namespace stopImageSection ends
diff --git a/libpore/p9_cpu_reg_restore_instruction.H
b/libpore/p9_cpu_reg_restore_instruction.H
index d69a4212..5f168855 100644
--- a/libpore/p9_cpu_reg_restore_instruction.H
+++ b/libpore/p9_cpu_reg_restore_instruction.H
@@ -5,7 +5,7 @@
/**/
/* OpenPOWER HostBoot Project */
/**/
-/* Contributors Listed Below - COPYRIGHT 2015,2018*/
+/* Contributors Listed Below - COPYRIGHT 2015,2020*/
/* [+] International Business Machines Corp. */
/**/
/**/
@@ -69,6 +69,11 @@ enum
OPCODE_18 = 18,
SELF_SAVE_FUNC_ADD = 0x2300,
SELF_SAVE_OFFSET= 0x180,
+SKIP_SPR_REST_INST = 0x481c, //b . +0x01c
+MFLR_R30= 0x7fc802a6,
+SKIP_SPR_SELF_SAVE = 0x3bff0020, //addi r31 r31, 0x20
+MTLR_INST = 0x7fc803a6, //mtlr r30
+BRANCH_BE_INST = 0x4820,
};
#define MR_R0_TO_R100x7c0a0378UL //mr r10 r0
diff --git a/libpore/p9_hcd_memmap_base.H b/libpore/p9_hcd_memmap_base.H
index 000fafef..ddb56728 100644
--- a/libpore/p9_hcd_memmap_base.H
+++ b/libpore/p9_hcd_memmap_base.H
@@ -444,6 +444,13 @@ HCD_CONST(CME_QUAD_PSTATE_SIZE, HALF_KB)
HCD_CONST(CME_REGION_SIZE, (64 * ONE_KB))
+
+// HOMER compatibility
+
+HCD_CONST(STOP_API_CPU_SAVE_VER,0x02)
+HCD_CONST(SELF_SAVE_RESTORE_VER,0x02)
+HCD_CONST(SMF_SUPPORT_SIGNATURE_OFFSET, 0x1300)
+HCD_CONST(SMF_SELF_SIGNATURE, (0x5f534d46))
// Debug
HCD_CONST(CPMR_TRACE_REGION_OFFSET, (512 * ONE_KB))
diff --git a/libpore/p9_stop_api.C b/libpore/p9_stop_api.C
index 2d9bb549..10e050a1 100644
--
[PATCH v8 0/3] Support for Self Save API in OPAL
v7: https://lists.ozlabs.org/pipermail/skiboot/2020-April/016763.html Changelog v6 --> v7 1. Simplified approach. Instead of advertising support bitmask for each SPR, advertising support only for the presence of the new self-save API Complete design specification is detailed below in the cover-leter. 2. Patch re-organization, move introducing self-save [2] after STOP API versioning [3]. Background == The power management framework on POWER systems include core idle states that lose context. Deep idle states namely "winkle" on POWER8 and "stop4" and "stop5" on POWER9 can be entered by a CPU to save different levels of power, as a consequence of which all the hypervisor resources such as SPRs and SCOMs are lost. For most SPRs, saving and restoration of content for SPRs and SCOMs is handled by the hypervisor kernel prior to entering an post exit from an idle state respectively. However, there is a small set of critical SPRs and XSCOMs that are expected to contain sane values even before the control is transferred to the hypervisor kernel at system reset vector. For this purpose, microcode firmware provides a mechanism to restore values on certain SPRs. The communication mechanism between the hypervisor kernel and the microcode is a standard interface called sleep-winkle-engine (SLW) on Power8 and Stop-API on Power9 which is abstracted by OPAL calls from the hypervisor kernel. The Stop-API provides an interface known as the self-restore API, to which the SPR number and a predefined value to be restored on wake-up from a deep stop state is supplied. Motivation to introduce a new Stop-API == The self-restore API expects not just the SPR number but also the value with which the SPR is restored. This is good for those SPRs such as HSPRG0 whose values do not change at runtime, since for them, the kernel can invoke the self-restore API at boot time once the values of these SPRs are determined. However, there are use-cases where-in the value to be saved cannot be known or cannot be updated in the layer it currently is. The shortcomings and the new use-cases which cannot be served by the existing self-restore API, serves as motivation for a new API: Shortcoming1: In a special wakeup scenario when a CPU is woken up in stop4/5 and after the task is done, the HCODE puts it back to stop. The value of PSSCR is passed to the HCODE via the self-restore API. The kernel currently provides the value of the deepest stop state due to being conservative. Thus if a core that was in stop4 was woken up due to special wakeup, the HCODE will now put it back to stop5 thus increasing the subsequent wakeup latency to ~200us. Shortcoming2: The value of LPCR is dynamic based on if the CPU is entered a stop state during cpu idle versus cpu hotplug. Today, an additional self-restore call is made before entering CPU-Hotplug to clear the PECE1 bit in stop-API so that if we are woken up by a special wakeup on an offlined CPU, we go back to stop with the the bit cleared. There is a overhead of an extra call New Use-case: - In the case where the hypervisor is running on an ultravisor environment, the boot time is too late in the cycle to make the self-restore API calls, as these cannot be invoked from an non-secure context anymore To address these shortcomings, the firmware provides another API known as the self-save API. The self-save API only takes the SPR number as a parameter and will ensure that on wakeup from a deep-stop state the SPR is restored with the value that it contained prior to entering the deep-stop. Contrast between self-save and self-restore APIs Before entering deep idle |---| > | HCODE A | | |---| |-|| | CPU || |-|| | |---| |>| HCODE B | On waking up |---| from deep idle When a self-restore API is invoked, the HCODE inserts instructions into "HCODE B" region of the above figure to restore the content of the SPR to the said value. The "HCODE B" region gets executed soon after the CPU wakes up from a deep idle state, thus executing the inserted instructions, thereby restoring the contents of the SPRs to the required values. When a self-save API is invoked, the HCODE inserts instructions into the "HCODE A" region of the above figure to save the content of the SPR into some location in memory. It also inserts instructions into the "HCODE B" region to restore the content of the SPR to the corresponding value saved in the memory by the instructions in "HCODE A" region. Thus, in contrast with self-restore, the self-save API *does not* need a value to be pass
Re: [PATCH v2 1/7] KVM: s390: clean up redundant 'kvm_run' parameters
On Thu, 23 Apr 2020 11:01:43 +0800
Tianjia Zhang wrote:
> On 2020/4/23 0:04, Cornelia Huck wrote:
> > On Wed, 22 Apr 2020 17:58:04 +0200
> > Christian Borntraeger wrote:
> >
> >> On 22.04.20 15:45, Cornelia Huck wrote:
> >>> On Wed, 22 Apr 2020 20:58:04 +0800
> >>> Tianjia Zhang wrote:
> >>>
> In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu'
> structure. Earlier than historical reasons, many kvm-related function
> >>>
> >>> s/Earlier than/For/ ?
> >>>
> parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same
> time.
> This patch does a unified cleanup of these remaining redundant
> parameters.
>
> Signed-off-by: Tianjia Zhang
> ---
> arch/s390/kvm/kvm-s390.c | 37 ++---
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e335a7e5ead7..d7bb2e7a07ff 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
> return rc;
> }
>
> -static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run
> *kvm_run)
> +static void sync_regs_fmt2(struct kvm_vcpu *vcpu)
> {
> +struct kvm_run *kvm_run = vcpu->run;
> struct runtime_instr_cb *riccb;
> struct gs_cb *gscb;
>
> @@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu,
> struct kvm_run *kvm_run)
> }
> if (vcpu->arch.gs_enabled) {
> current->thread.gs_cb = (struct gs_cb *)
> -&vcpu->run->s.regs.gscb;
> +&kvm_run->s.regs.gscb;
> >>>
> >>> Not sure if these changes (vcpu->run-> => kvm_run->) are really worth
> >>> it. (It seems they amount to at least as much as the changes advertised
> >>> in the patch description.)
> >>>
> >>> Other opinions?
> >>
> >> Agreed. It feels kind of random. Maybe just do the first line (move
> >> kvm_run from the
> >> function parameter list into the variable declaration)? Not sure if this
> >> is better.
> >>
> >
> > There's more in this patch that I cut... but I think just moving
> > kvm_run from the parameter list would be much less disruptive.
> >
>
> I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but
> there will be more disruptive, not less.
I just fail to see the benefit; sure, kvm_run-> is convenient, but the
current code is just fine, and any rework should be balanced against
the cost (e.g. cluttering git annotate).
Re: [PATCH v5 0/5] Track and expose idle PURR and SPURR ticks
On Mon, Apr 20, 2020 at 03:46:35PM -0700, Tyrel Datwyler wrote: > On 4/7/20 1:47 AM, Gautham R. Shenoy wrote: > > From: "Gautham R. Shenoy" > > > > Hi, > > > > This is the fifth version of the patches to track and expose idle PURR > > and SPURR ticks. These patches are required by tools such as lparstat > > to compute system utilization for capacity planning purposes. > > > > The previous versions can be found here: > > v4: https://lkml.org/lkml/2020/3/27/323 > > v3: https://lkml.org/lkml/2020/3/11/331 > > v2: https://lkml.org/lkml/2020/2/21/21 > > v1: https://lore.kernel.org/patchwork/cover/1159341/ > > > > They changes from v4 are: > > > >- As suggested by Naveen, moved the functions read_this_idle_purr() > > and read_this_idle_spurr() from Patch 2 and Patch 3 respectively > > to Patch 4 where it is invoked. > > > >- Dropped Patch 6 which cached the values of purr, spurr, > > idle_purr, idle_spurr in order to minimize the number of IPIs > > sent. > > > >- Updated the dates for the idle_purr, idle_spurr in the > > Documentation Patch 5. > > > > Motivation: > > === > > On PSeries LPARs, the data centers planners desire a more accurate > > view of system utilization per resource such as CPU to plan the system > > capacity requirements better. Such accuracy can be obtained by reading > > PURR/SPURR registers for CPU resource utilization. > > > > Tools such as lparstat which are used to compute the utilization need > > to know [S]PURR ticks when the cpu was busy or idle. The [S]PURR > > counters are already exposed through sysfs. We already account for > > PURR ticks when we go to idle so that we can update the VPA area. This > > patchset extends support to account for SPURR ticks when idle, and > > expose both via per-cpu sysfs files. > > > > These patches are required for enhancement to the lparstat utility > > that compute the CPU utilization based on PURR and SPURR which can be > > found here : > > https://groups.google.com/forum/#!topic/powerpc-utils-devel/fYRo69xO9r4 > > > > > > With the patches, when lparstat is run on a LPAR running CPU-Hogs, > > = > > sudo ./src/lparstat -E 1 3 > > > > System Configuration > > type=Dedicated mode=Capped smt=8 lcpu=2 mem=4834112 kB cpus=0 ent=2.00 > > > > ---Actual--- -Normalized- > > %busy %idle Frequency %busy %idle > > -- -- - -- -- > > 1 99.99 0.00 3.35GHz[111%] 110.99 0.00 > > 2 100.00 0.00 3.35GHz[111%] 111.01 0.00 > > 3 100.00 0.00 3.35GHz[111%] 111.00 0.00 > > > > With patches, when lparstat is run on and idle LPAR > > = > > System Configuration > > type=Dedicated mode=Capped smt=8 lcpu=2 mem=4834112 kB cpus=0 ent=2.00 > > ---Actual--- -Normalized- > > %busy %idle Frequency %busy %idle > > -- -- - -- -- > > 1 0.15 99.84 2.17GHz[ 72%] 0.11 71.89 > > 2 0.24 99.76 2.11GHz[ 70%] 0.18 69.82 > > 3 0.24 99.75 2.11GHz[ 70%] 0.18 69.81 > > > > Gautham R. Shenoy (5): > > powerpc: Move idle_loop_prolog()/epilog() functions to header file > > powerpc/idle: Store PURR snapshot in a per-cpu global variable > > powerpc/pseries: Account for SPURR ticks on idle CPUs > > powerpc/sysfs: Show idle_purr and idle_spurr for every CPU > > Documentation: Document sysfs interfaces purr, spurr, idle_purr, > > idle_spurr > > > > Documentation/ABI/testing/sysfs-devices-system-cpu | 39 + > > arch/powerpc/include/asm/idle.h| 93 > > ++ > > arch/powerpc/kernel/sysfs.c| 82 ++- > > arch/powerpc/platforms/pseries/setup.c | 8 +- > > drivers/cpuidle/cpuidle-pseries.c | 39 ++--- > > 5 files changed, 224 insertions(+), 37 deletions(-) > > create mode 100644 arch/powerpc/include/asm/idle.h > > > > Reviewed-by: Tyrel Datwyler Thanks for reviewing the patches. > > Any chance this is going to be merged in the near future? There is a patchset > to > update lparstat in the powerpc-utils package to calculate PURR/SPURR cpu > utilization that I would like to merge, but have been holding off to make sure > we are synced with this proposed patchset. Michael, could you please consider this for 5.8 ? -- Thanks and Regards gautham.
[PATCH][next] ASoC: fsl_easrc: fix spelling mistake "prefitler" -> "prefilter"
From: Colin Ian King There is a spelling mistake in a deb_dbg message, fix it. Signed-off-by: Colin Ian King --- sound/soc/fsl/fsl_easrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c index 233f26ff885c..97658e1f4989 100644 --- a/sound/soc/fsl/fsl_easrc.c +++ b/sound/soc/fsl/fsl_easrc.c @@ -1769,7 +1769,7 @@ static void fsl_easrc_dump_firmware(struct fsl_asrc *easrc) } dev_dbg(dev, "Firmware v%u dump:\n", firm->firmware_version); - dev_dbg(dev, "Num prefitler scenarios: %u\n", firm->prefil_scen); + dev_dbg(dev, "Num prefilter scenarios: %u\n", firm->prefil_scen); dev_dbg(dev, "Num interpolation scenarios: %u\n", firm->interp_scen); dev_dbg(dev, "\nInterpolation scenarios:\n"); -- 2.25.1
