* Re: [PATCH v3 3/3] powerpc: Define and use MSR_RI only on non booke/40x
From: Christophe Leroy @ 2021-08-25 6:21 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, npiggin
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <8735qyceev.fsf@mpe.ellerman.id.au>
Le 25/08/2021 à 06:54, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> 40x and BOOKE don't have MSR_RI.
>>
>> Define MSR_RI only for platforms where it exists. For the other ones,
>> defines it as BUILD_BUG for C and do not define it for ASM.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v3: Fixes kvm_emul.S and include <linux/bug.h> in <asm/reg.h>
>> ---
>> arch/powerpc/include/asm/reg.h | 5 +++++
>> arch/powerpc/include/asm/reg_booke.h | 6 +++---
>> arch/powerpc/kernel/head_32.h | 4 ++++
>> arch/powerpc/kernel/kvm_emul.S | 13 +++++++++++++
>> arch/powerpc/kernel/process.c | 2 +-
>> arch/powerpc/lib/sstep.c | 2 +-
>> 6 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> index be85cf156a1f..b270b570fb51 100644
>> --- a/arch/powerpc/include/asm/reg.h
>> +++ b/arch/powerpc/include/asm/reg.h
>> @@ -109,7 +109,12 @@
>> #ifndef MSR_PMM
>> #define MSR_PMM __MASK(MSR_PMM_LG) /* Performance monitor */
>> #endif
>> +#if !defined(CONFIG_BOOKE) && !defined(CONFIG_40x)
>> #define MSR_RI __MASK(MSR_RI_LG) /* Recoverable Exception */
>
> This breaks 64-bit BookE, which is using MSR_RI in bookehv_interrupts.S.
>
> eg. ppc64_book3e_allmodconfig gives:
>
> arch/powerpc/kvm/bookehv_interrupts.S: Assembler messages:
> arch/powerpc/kvm/bookehv_interrupts.S:221: Error: invalid operands (*ABS* and *UND* sections) for `|'
> etc.
Oops, I missed that one. Should be easy to fix though as this file is dedicated to BOOKE we can just
remove MSR_RI from there.
>
> ISA v2.07B says MSR_RI is Book3S only, but looking at the e500mc manual
> it does have bit 62 defined as RI.
Oh !
So it makes the story different, even different than what we have in kernel today where everything
is more or less based on CONFIG_BOOKE or CONFIG_40x.
>
> I can fix it with:
>
> +#if !(defined(CONFIG_BOOKE) && !defined(CONFIG_PPC_BOOK3E)) && !defined(CONFIG_40x)
> #define MSR_RI __MASK(MSR_RI_LG) /* Recoverable Exception */
Why CONFIG_PPC_BOOK3E ? Shouldn't it be CONFIG_PPC_E500MC instead ?
>
>
> But that's getting really ugly, and we'd have to repeat it elsewhere.
>
> I think we need a kconfig symbol that captures which platforms should
> use MSR_RI, something like:
>
> CONFIG PPC_MSR_RI
> def_bool y
> depends on !40x && (!BOOKE || PPC_BOOK3E)
Yes I think that would be the best.
>
>
> Or maybe we should just define MSR_RI to 0 for platforms that don't use
> it, to avoid so much ifdefing?
>
Well, I tried to take the same approach as Ben in the original patch
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/61ad3646674e6bf541a8f7fb420cdf556d0b2152.camel@kernel.crashing.org/
but with a smoother approach for C files to avoid ifdefs there.
However for ASM it is different, we can't use tricks like BUILD_BUG. When we had MSR_RI completely
undefined on all booke it was rather easy, only a few #ifdefs needed in parts common to 3S and 3E.
But if we bring back MSR_RI on the e500mc that becomes more complex, and I agree with you too many
#ifdefs will be unfriendly.
On the other hand, setting MSR_RI to 0 will make all tests that check msr & MSR_RI fails during run.
Isn't it worse than what we have today ?
Maybe the way out is to carrefully look at the situations where e500mc uses MSR_RI, because
according to the reference manual it is limited to a few situations:
4.4 Recoverability and MSR[RI]
MSR[RI] is an MSR (and save/restore register) storage bit for compatibility with pre-Book E PowerPC
processors. When an interrupt occurs, the recoverable interrupt bit, MSR[RI] is unchanged by the
interrupt
mechanism when a new MSR is established; however, when a machine check, error report or NMI
interrupt occurs, MSR[RI] is cleared.
If used properly, RI determines whether an interrupt that is taken at the machine check interrupt
vector can
be safely returned from (that is, that architected state set by the interrupt mechanism has been
safely stored
by software). RI should be set by software when all MSR values are first established. When an interrupt
occurs that is taken at the machine check interrupt vector, software should set RI when it has
safely stored
MCSRR0 and MCSRR1. The associated MCSRR1 bit should be checked to determine whether the
interrupt occurred when another machine check interrupt was being processed and before state was
successfully saved. If MCSRR1[RI] is set, it is safe to return when processing is complete.
So, let's think about it.
By the way patch 2 of the series could go now, it doesn't introduces anything new, it only cleans up
things a bit.
Christophe
^ permalink raw reply
* [PATCH linux-next] powerpc:security: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
From: CGEL @ 2021-08-25 6:40 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jing Yangyang, Alexey Kardashevskiy, Zeal Robot, Nicholas Piggin,
linux-kernel, Li Huafei, Paul Mackerras, linuxppc-dev
From: Jing Yangyang <jing.yangyang@zte.com.cn>
Fix the following coccicheck warning:
./arch/powerpc/kernel/security.c:807:0-23: WARNING:
fops_entry_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/powerpc/kernel/security.c:781:0-23:WARNING:
fops_rfi_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/powerpc/kernel/security.c:833:0-23:WARNING:
fops_uaccess_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Jing Yangyang <jing.yangyang@zte.com.cn>
---
arch/powerpc/kernel/security.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 1a99849..cf8ce24 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -778,7 +778,7 @@ static int rfi_flush_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
static int entry_flush_set(void *data, u64 val)
{
@@ -804,7 +804,7 @@ static int entry_flush_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_entry_flush, entry_flush_get, entry_flush_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_entry_flush, entry_flush_get, entry_flush_set, "%llu\n");
static int uaccess_flush_set(void *data, u64 val)
{
@@ -830,7 +830,7 @@ static int uaccess_flush_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_uaccess_flush, uaccess_flush_get, uaccess_flush_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_uaccess_flush, uaccess_flush_get, uaccess_flush_set, "%llu\n");
static __init int rfi_flush_debugfs_init(void)
{
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH linux-next] powerpc:security: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
From: Christophe Leroy @ 2021-08-25 6:42 UTC (permalink / raw)
To: CGEL, Michael Ellerman
Cc: Jing Yangyang, Alexey Kardashevskiy, Zeal Robot, linux-kernel,
Li Huafei, Paul Mackerras, Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210825064016.70421-1-deng.changcheng@zte.com.cn>
Hi,
Le 25/08/2021 à 08:40, CGEL a écrit :
> From: Jing Yangyang <jing.yangyang@zte.com.cn>
>
> Fix the following coccicheck warning:
> ./arch/powerpc/kernel/security.c:807:0-23: WARNING:
> fops_entry_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/powerpc/kernel/security.c:781:0-23:WARNING:
> fops_rfi_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/powerpc/kernel/security.c:833:0-23:WARNING:
> fops_uaccess_flush should be defined with DEFINE_DEBUGFS_ATTRIBUTE
Can you give a few more details why the suggestion from coccicheck is a good suggestion ?
Thanks
Christophe
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Jing Yangyang <jing.yangyang@zte.com.cn>
> ---
> arch/powerpc/kernel/security.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 1a99849..cf8ce24 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -778,7 +778,7 @@ static int rfi_flush_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
>
> static int entry_flush_set(void *data, u64 val)
> {
> @@ -804,7 +804,7 @@ static int entry_flush_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(fops_entry_flush, entry_flush_get, entry_flush_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_entry_flush, entry_flush_get, entry_flush_set, "%llu\n");
>
> static int uaccess_flush_set(void *data, u64 val)
> {
> @@ -830,7 +830,7 @@ static int uaccess_flush_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(fops_uaccess_flush, uaccess_flush_get, uaccess_flush_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_uaccess_flush, uaccess_flush_get, uaccess_flush_set, "%llu\n");
>
> static __init int rfi_flush_debugfs_init(void)
> {
>
^ permalink raw reply
* [PATCH linux-next] power:pkeys: fix bugon.cocci warnings
From: CGEL @ 2021-08-25 6:42 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Sandipan Das, linuxppc-dev, linux-kernel
Cc: Zeal Robot, Jing Yangyang
From: Jing Yangyang <jing.yangyang@zte.com.cn>
Use BUG_ON instead of a if condition followed by BUG.
./arch/powerpc/include/asm/book3s/64/pkeys.h:21:2-5:WARNING
Use BUG_ON instead of if condition followed by BUG.
./arch/powerpc/include/asm/book3s/64/pkeys.h:14:2-5:WARNING
Use BUG_ON instead of if condition followed by BUG.
Generated by: scripts/coccinelle/misc/bugon.cocci
Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Jing Yangyang <jing.yangyang@zte.com.cn>
---
arch/powerpc/include/asm/book3s/64/pkeys.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pkeys.h b/arch/powerpc/include/asm/book3s/64/pkeys.h
index 5b17813..5f74f0c 100644
--- a/arch/powerpc/include/asm/book3s/64/pkeys.h
+++ b/arch/powerpc/include/asm/book3s/64/pkeys.h
@@ -10,15 +10,13 @@ static inline u64 vmflag_to_pte_pkey_bits(u64 vm_flags)
if (!mmu_has_feature(MMU_FTR_PKEY))
return 0x0UL;
- if (radix_enabled())
- BUG();
+ BUG_ON(radix_enabled());
return hash__vmflag_to_pte_pkey_bits(vm_flags);
}
static inline u16 pte_to_pkey_bits(u64 pteflags)
{
- if (radix_enabled())
- BUG();
+ BUG_ON(radix_enabled());
return hash__pte_to_pkey_bits(pteflags);
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3 1/3] powerpc: Remove MSR_PR check in interrupt_exit_{user/kernel}_prepare()
From: Michael Ellerman @ 2021-08-25 6:56 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, npiggin
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f5f598a5-3830-ee21-aff5-cba06deeb959@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 25/08/2021 à 07:27, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> In those hot functions that are called at every interrupt, any saved
>>> cycle is worth it.
>>>
>>> interrupt_exit_user_prepare() and interrupt_exit_kernel_prepare() are
>>> called from three places:
>>> - From entry_32.S
>>> - From interrupt_64.S
>>> - From interrupt_exit_user_restart() and interrupt_exit_kernel_restart()
>>>
>>> In entry_32.S, there are inambiguously called based on MSR_PR:
>>>
>>> interrupt_return:
>>> lwz r4,_MSR(r1)
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> andi. r0,r4,MSR_PR
>>> beq .Lkernel_interrupt_return
>>> bl interrupt_exit_user_prepare
>>> ...
>>> .Lkernel_interrupt_return:
>>> bl interrupt_exit_kernel_prepare
>>>
>>> In interrupt_64.S, that's similar:
>>>
>>> interrupt_return_\srr\():
>>> ld r4,_MSR(r1)
>>> andi. r0,r4,MSR_PR
>>> beq interrupt_return_\srr\()_kernel
>>> interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> bl interrupt_exit_user_prepare
>>> ...
>>> interrupt_return_\srr\()_kernel:
>>> addi r3,r1,STACK_FRAME_OVERHEAD
>>> bl interrupt_exit_kernel_prepare
>>>
>>> In interrupt_exit_user_restart() and interrupt_exit_kernel_restart(),
>>> MSR_PR is verified respectively by BUG_ON(!user_mode(regs)) and
>>> BUG_ON(user_mode(regs)) prior to calling interrupt_exit_user_prepare()
>>> and interrupt_exit_kernel_prepare().
>>>
>>> The verification in interrupt_exit_user_prepare() and
>>> interrupt_exit_kernel_prepare() are therefore useless and can be removed.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Acked-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/kernel/interrupt.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>
>> I'll pick this one up independent of the other two patches.
>
> Second patch should be ok as well, no ?
Yeah I guess.
I'm not sure if we'll want to keep cpu_has_msr_ri() if we have a
CONFIG_PPC_MSR_RI, but that's a pretty minor detail.
So yeah I'll take patch 2 as well.
cheers
^ permalink raw reply
* [PATCH] ASoC: imx-rpmsg: change dev_err to dev_err_probe for -EPROBE_DEFER
From: Shengjiu Wang @ 2021-08-25 7:14 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
Change dev_err to dev_err_probe for no need print error message
when defer probe happens.
Fixes: 39f8405c3e50 ("ASoC: imx-rpmsg: Add machine driver for audio base on rpmsg")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/imx-rpmsg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/imx-rpmsg.c b/sound/soc/fsl/imx-rpmsg.c
index f0cae8c59d54..f96fe4ff8425 100644
--- a/sound/soc/fsl/imx-rpmsg.c
+++ b/sound/soc/fsl/imx-rpmsg.c
@@ -125,7 +125,7 @@ static int imx_rpmsg_probe(struct platform_device *pdev)
snd_soc_card_set_drvdata(&data->card, data);
ret = devm_snd_soc_register_card(&pdev->dev, &data->card);
if (ret) {
- dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
+ dev_err_probe(&pdev->dev, ret, "snd_soc_register_card failed\n");
goto fail;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v4 1/2] powerpc/perf: Use PVR rather than oprofile field to determine CPU version
From: Christophe Leroy @ 2021-08-25 7:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
From: Rashmica Gupta <rashmica.g@gmail.com>
Currently the perf CPU backend drivers detect what CPU they're on using
cur_cpu_spec->oprofile_cpu_type.
Although that works, it's a bit crufty to be using oprofile related fields,
especially seeing as oprofile is more or less unused these days.
It also means perf is reliant on the fragile logic in setup_cpu_spec()
which detects when we're using a logical PVR and copies back the PMU
related fields from the raw CPU entry. So lets check the PVR directly.
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
[chleroy: Added power10 and fixed checkpatch issues]
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-and-tested-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reviewed-and-tested-By: Kajol Jain <kjain@linux.ibm.com> [For 24x7 side changes]
---
v4:
- No change to the series
- Added additional Reviewed-by/Tested-by.
- Rebased
- Resending to get some CI result this time
---
arch/powerpc/perf/e500-pmu.c | 9 +++++----
arch/powerpc/perf/e6500-pmu.c | 5 +++--
arch/powerpc/perf/hv-24x7.c | 6 +++---
arch/powerpc/perf/mpc7450-pmu.c | 5 +++--
arch/powerpc/perf/power10-pmu.c | 6 ++----
arch/powerpc/perf/power5+-pmu.c | 6 +++---
arch/powerpc/perf/power5-pmu.c | 5 +++--
arch/powerpc/perf/power6-pmu.c | 5 +++--
arch/powerpc/perf/power7-pmu.c | 7 ++++---
arch/powerpc/perf/power8-pmu.c | 5 +++--
arch/powerpc/perf/power9-pmu.c | 4 +---
arch/powerpc/perf/ppc970-pmu.c | 7 ++++---
12 files changed, 37 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/perf/e500-pmu.c b/arch/powerpc/perf/e500-pmu.c
index a59c33bed32a..e3e1a68eb1d5 100644
--- a/arch/powerpc/perf/e500-pmu.c
+++ b/arch/powerpc/perf/e500-pmu.c
@@ -118,12 +118,13 @@ static struct fsl_emb_pmu e500_pmu = {
static int init_e500_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type)
- return -ENODEV;
+ unsigned int pvr = mfspr(SPRN_PVR);
- if (!strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc/e500mc"))
+ /* ec500mc */
+ if (PVR_VER(pvr) == PVR_VER_E500MC || PVR_VER(pvr) == PVR_VER_E5500)
num_events = 256;
- else if (strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc/e500"))
+ /* e500 */
+ else if (PVR_VER(pvr) != PVR_VER_E500V1 && PVR_VER(pvr) != PVR_VER_E500V2)
return -ENODEV;
return register_fsl_emb_pmu(&e500_pmu);
diff --git a/arch/powerpc/perf/e6500-pmu.c b/arch/powerpc/perf/e6500-pmu.c
index 44ad65da82ed..bd779a2338f8 100644
--- a/arch/powerpc/perf/e6500-pmu.c
+++ b/arch/powerpc/perf/e6500-pmu.c
@@ -107,8 +107,9 @@ static struct fsl_emb_pmu e6500_pmu = {
static int init_e6500_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc/e6500"))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_VER_E6500)
return -ENODEV;
return register_fsl_emb_pmu(&e6500_pmu);
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 1816f560a465..63e84075fd64 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1718,16 +1718,16 @@ static int hv_24x7_init(void)
{
int r;
unsigned long hret;
+ unsigned int pvr = mfspr(SPRN_PVR);
struct hv_perf_caps caps;
if (!firmware_has_feature(FW_FEATURE_LPAR)) {
pr_debug("not a virtualized system, not enabling\n");
return -ENODEV;
- } else if (!cur_cpu_spec->oprofile_cpu_type)
- return -ENODEV;
+ }
/* POWER8 only supports v1, while POWER9 only supports v2. */
- if (!strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8"))
+ if (PVR_VER(pvr) == PVR_POWER8)
interface_version = 1;
else {
interface_version = 2;
diff --git a/arch/powerpc/perf/mpc7450-pmu.c b/arch/powerpc/perf/mpc7450-pmu.c
index e39b15b79a83..552d51a925d3 100644
--- a/arch/powerpc/perf/mpc7450-pmu.c
+++ b/arch/powerpc/perf/mpc7450-pmu.c
@@ -417,8 +417,9 @@ struct power_pmu mpc7450_pmu = {
static int __init init_mpc7450_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc/7450"))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_7450)
return -ENODEV;
return register_power_pmu(&mpc7450_pmu);
diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
index f9d64c63bb4a..53b14deda088 100644
--- a/arch/powerpc/perf/power10-pmu.c
+++ b/arch/powerpc/perf/power10-pmu.c
@@ -579,12 +579,10 @@ int init_power10_pmu(void)
unsigned int pvr;
int rc;
- /* Comes from cpu_specs[] */
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
+ pvr = mfspr(SPRN_PVR);
+ if (PVR_VER(pvr) != PVR_POWER10)
return -ENODEV;
- pvr = mfspr(SPRN_PVR);
/* Add the ppmu flag for power10 DD1 */
if ((PVR_CFG(pvr) == 1))
power10_pmu.flags |= PPMU_P10_DD1;
diff --git a/arch/powerpc/perf/power5+-pmu.c b/arch/powerpc/perf/power5+-pmu.c
index 18732267993a..a79eae40ef6d 100644
--- a/arch/powerpc/perf/power5+-pmu.c
+++ b/arch/powerpc/perf/power5+-pmu.c
@@ -679,9 +679,9 @@ static struct power_pmu power5p_pmu = {
int init_power5p_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- (strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power5+")
- && strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power5++")))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_POWER5p)
return -ENODEV;
return register_power_pmu(&power5p_pmu);
diff --git a/arch/powerpc/perf/power5-pmu.c b/arch/powerpc/perf/power5-pmu.c
index cb611c1e7abe..35a9d7f3b4b9 100644
--- a/arch/powerpc/perf/power5-pmu.c
+++ b/arch/powerpc/perf/power5-pmu.c
@@ -620,8 +620,9 @@ static struct power_pmu power5_pmu = {
int init_power5_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power5"))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_POWER5)
return -ENODEV;
return register_power_pmu(&power5_pmu);
diff --git a/arch/powerpc/perf/power6-pmu.c b/arch/powerpc/perf/power6-pmu.c
index 69ef38216418..8aa220c712a7 100644
--- a/arch/powerpc/perf/power6-pmu.c
+++ b/arch/powerpc/perf/power6-pmu.c
@@ -541,8 +541,9 @@ static struct power_pmu power6_pmu = {
int init_power6_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power6"))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_POWER6)
return -ENODEV;
return register_power_pmu(&power6_pmu);
diff --git a/arch/powerpc/perf/power7-pmu.c b/arch/powerpc/perf/power7-pmu.c
index 894c17f9a762..ca7373143b02 100644
--- a/arch/powerpc/perf/power7-pmu.c
+++ b/arch/powerpc/perf/power7-pmu.c
@@ -447,11 +447,12 @@ static struct power_pmu power7_pmu = {
int init_power7_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power7"))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_POWER7 && PVR_VER(pvr) != PVR_POWER7p)
return -ENODEV;
- if (pvr_version_is(PVR_POWER7p))
+ if (PVR_VER(pvr) == PVR_POWER7p)
power7_pmu.flags |= PPMU_SIAR_VALID;
return register_power_pmu(&power7_pmu);
diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index 5282e8415ddf..5a396ba8bf58 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -381,9 +381,10 @@ static struct power_pmu power8_pmu = {
int init_power8_pmu(void)
{
int rc;
+ unsigned int pvr = mfspr(SPRN_PVR);
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8"))
+ if (PVR_VER(pvr) != PVR_POWER8E && PVR_VER(pvr) != PVR_POWER8NVL &&
+ PVR_VER(pvr) != PVR_POWER8)
return -ENODEV;
rc = register_power_pmu(&power8_pmu);
diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
index ff3382140d7e..147767ca0039 100644
--- a/arch/powerpc/perf/power9-pmu.c
+++ b/arch/powerpc/perf/power9-pmu.c
@@ -457,9 +457,7 @@ int init_power9_pmu(void)
int rc = 0;
unsigned int pvr = mfspr(SPRN_PVR);
- /* Comes from cpu_specs[] */
- if (!cur_cpu_spec->oprofile_cpu_type ||
- strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power9"))
+ if (PVR_VER(pvr) != PVR_POWER9)
return -ENODEV;
/* Blacklist events */
diff --git a/arch/powerpc/perf/ppc970-pmu.c b/arch/powerpc/perf/ppc970-pmu.c
index 1f8263785286..39a0a4d7841c 100644
--- a/arch/powerpc/perf/ppc970-pmu.c
+++ b/arch/powerpc/perf/ppc970-pmu.c
@@ -491,9 +491,10 @@ static struct power_pmu ppc970_pmu = {
int init_ppc970_pmu(void)
{
- if (!cur_cpu_spec->oprofile_cpu_type ||
- (strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/970")
- && strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/970MP")))
+ unsigned int pvr = mfspr(SPRN_PVR);
+
+ if (PVR_VER(pvr) != PVR_970 && PVR_VER(pvr) != PVR_970MP &&
+ PVR_VER(pvr) != PVR_970FX && PVR_VER(pvr) != PVR_970GX)
return -ENODEV;
return register_power_pmu(&ppc970_pmu);
--
2.25.0
^ permalink raw reply related
* [PATCH v4 2/2] powerpc: Remove remaining parts of oprofile
From: Christophe Leroy @ 2021-08-25 7:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <8ea2d4065ec5a0137e5ad9b75773ed27c03bfc91.1629877436.git.christophe.leroy@csgroup.eu>
Commit 9850b6c69356 ("arch: powerpc: Remove oprofile") removed
oprofile.
Remove all remaining parts of it.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
arch/powerpc/include/asm/cputable.h | 3 --
arch/powerpc/kernel/cputable.c | 66 +----------------------
arch/powerpc/kernel/dt_cpu_ftrs.c | 4 --
arch/powerpc/platforms/cell/spufs/spufs.h | 2 +-
4 files changed, 3 insertions(+), 72 deletions(-)
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index e85c849214a2..850129d13d46 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -70,9 +70,6 @@ struct cpu_spec {
/* Used to restore cpu setup on secondary processors and at resume */
cpu_restore_t cpu_restore;
- /* Used by oprofile userspace to select the right counters */
- char *oprofile_cpu_type;
-
/* Name of processor class, for the ELF AT_PLATFORM entry */
char *platform;
diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index ae0fdef0ac11..0fc812ac7519 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -149,7 +149,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.pmc_type = PPC_PMC_IBM,
.cpu_setup = __setup_cpu_ppc970,
.cpu_restore = __restore_cpu_ppc970,
- .oprofile_cpu_type = "ppc64/970",
.platform = "ppc970",
},
{ /* PPC970FX */
@@ -166,7 +165,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.pmc_type = PPC_PMC_IBM,
.cpu_setup = __setup_cpu_ppc970,
.cpu_restore = __restore_cpu_ppc970,
- .oprofile_cpu_type = "ppc64/970",
.platform = "ppc970",
},
{ /* PPC970MP DD1.0 - no DEEPNAP, use regular 970 init */
@@ -183,7 +181,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.pmc_type = PPC_PMC_IBM,
.cpu_setup = __setup_cpu_ppc970,
.cpu_restore = __restore_cpu_ppc970,
- .oprofile_cpu_type = "ppc64/970MP",
.platform = "ppc970",
},
{ /* PPC970MP */
@@ -200,7 +197,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.pmc_type = PPC_PMC_IBM,
.cpu_setup = __setup_cpu_ppc970MP,
.cpu_restore = __restore_cpu_ppc970,
- .oprofile_cpu_type = "ppc64/970MP",
.platform = "ppc970",
},
{ /* PPC970GX */
@@ -216,7 +212,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 8,
.pmc_type = PPC_PMC_IBM,
.cpu_setup = __setup_cpu_ppc970,
- .oprofile_cpu_type = "ppc64/970",
.platform = "ppc970",
},
{ /* Power5 GR */
@@ -230,7 +225,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power5",
.platform = "power5",
},
{ /* Power5++ */
@@ -243,7 +237,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 128,
.dcache_bsize = 128,
.num_pmcs = 6,
- .oprofile_cpu_type = "ppc64/power5++",
.platform = "power5+",
},
{ /* Power5 GS */
@@ -257,7 +250,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power5+",
.platform = "power5+",
},
{ /* POWER6 in P5+ mode; 2.04-compliant processor */
@@ -269,7 +261,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER5,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.platform = "power5+",
},
{ /* Power6 */
@@ -284,7 +275,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power6",
.platform = "power6x",
},
{ /* 2.05-compliant processor, i.e. Power6 "architected" mode */
@@ -296,7 +286,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER6,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.platform = "power6",
},
{ /* 2.06-compliant processor, i.e. Power7 "architected" mode */
@@ -309,7 +298,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER7,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.cpu_setup = __setup_cpu_power7,
.cpu_restore = __restore_cpu_power7,
.machine_check_early = __machine_check_early_realmode_p7,
@@ -325,7 +313,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER8,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.cpu_setup = __setup_cpu_power8,
.cpu_restore = __restore_cpu_power8,
.machine_check_early = __machine_check_early_realmode_p8,
@@ -341,7 +328,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER9,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.cpu_setup = __setup_cpu_power9,
.cpu_restore = __restore_cpu_power9,
.platform = "power9",
@@ -356,7 +342,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.mmu_features = MMU_FTRS_POWER10,
.icache_bsize = 128,
.dcache_bsize = 128,
- .oprofile_cpu_type = "ppc64/ibm-compat-v1",
.cpu_setup = __setup_cpu_power10,
.cpu_restore = __restore_cpu_power10,
.platform = "power10",
@@ -373,7 +358,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power7",
.cpu_setup = __setup_cpu_power7,
.cpu_restore = __restore_cpu_power7,
.machine_check_early = __machine_check_early_realmode_p7,
@@ -391,7 +375,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power7",
.cpu_setup = __setup_cpu_power7,
.cpu_restore = __restore_cpu_power7,
.machine_check_early = __machine_check_early_realmode_p7,
@@ -409,7 +392,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power8",
.cpu_setup = __setup_cpu_power8,
.cpu_restore = __restore_cpu_power8,
.machine_check_early = __machine_check_early_realmode_p8,
@@ -427,7 +409,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power8",
.cpu_setup = __setup_cpu_power8,
.cpu_restore = __restore_cpu_power8,
.machine_check_early = __machine_check_early_realmode_p8,
@@ -445,7 +426,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power8",
.cpu_setup = __setup_cpu_power8,
.cpu_restore = __restore_cpu_power8,
.machine_check_early = __machine_check_early_realmode_p8,
@@ -463,7 +443,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power9",
.cpu_setup = __setup_cpu_power9,
.cpu_restore = __restore_cpu_power9,
.machine_check_early = __machine_check_early_realmode_p9,
@@ -481,7 +460,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power9",
.cpu_setup = __setup_cpu_power9,
.cpu_restore = __restore_cpu_power9,
.machine_check_early = __machine_check_early_realmode_p9,
@@ -499,7 +477,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power9",
.cpu_setup = __setup_cpu_power9,
.cpu_restore = __restore_cpu_power9,
.machine_check_early = __machine_check_early_realmode_p9,
@@ -517,7 +494,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 6,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/power10",
.cpu_setup = __setup_cpu_power10,
.cpu_restore = __restore_cpu_power10,
.machine_check_early = __machine_check_early_realmode_p10,
@@ -536,7 +512,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.dcache_bsize = 128,
.num_pmcs = 4,
.pmc_type = PPC_PMC_IBM,
- .oprofile_cpu_type = "ppc64/cell-be",
.platform = "ppc-cell-be",
},
{ /* PA Semi PA6T */
@@ -552,7 +527,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.pmc_type = PPC_PMC_PA6T,
.cpu_setup = __setup_cpu_pa6t,
.cpu_restore = __restore_cpu_pa6t,
- .oprofile_cpu_type = "ppc64/pa6t",
.platform = "pa6t",
},
{ /* default match */
@@ -716,7 +690,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_750,
.machine_check = machine_check_generic,
.platform = "ppc750",
- .oprofile_cpu_type = "ppc/750",
},
{ /* 745/755 */
.pvr_mask = 0xfffff000,
@@ -747,7 +720,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_750,
.machine_check = machine_check_generic,
.platform = "ppc750",
- .oprofile_cpu_type = "ppc/750",
},
{ /* 750FX rev 2.0 must disable HID0[DPM] */
.pvr_mask = 0xffffffff,
@@ -763,7 +735,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_750,
.machine_check = machine_check_generic,
.platform = "ppc750",
- .oprofile_cpu_type = "ppc/750",
},
{ /* 750FX (All revs except 2.0) */
.pvr_mask = 0xffff0000,
@@ -779,7 +750,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_750fx,
.machine_check = machine_check_generic,
.platform = "ppc750",
- .oprofile_cpu_type = "ppc/750",
},
{ /* 750GX */
.pvr_mask = 0xffff0000,
@@ -795,7 +765,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_750fx,
.machine_check = machine_check_generic,
.platform = "ppc750",
- .oprofile_cpu_type = "ppc/750",
},
{ /* 740/750 (L2CR bit need fixup for 740) */
.pvr_mask = 0xffff0000,
@@ -873,7 +842,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -890,7 +858,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -907,7 +874,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -924,7 +890,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -941,7 +906,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -958,7 +922,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -975,7 +938,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -992,7 +954,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -1008,7 +969,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -1025,7 +985,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -1042,7 +1001,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.num_pmcs = 6,
.pmc_type = PPC_PMC_G4,
.cpu_setup = __setup_cpu_745x,
- .oprofile_cpu_type = "ppc/7450",
.machine_check = machine_check_generic,
.platform = "ppc7450",
},
@@ -1154,7 +1112,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_603,
.machine_check = machine_check_83xx,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e300",
.platform = "ppc603",
},
{ /* e300c4 (e300c1, plus one IU) */
@@ -1170,7 +1127,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.cpu_setup = __setup_cpu_603,
.machine_check = machine_check_83xx,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e300",
.platform = "ppc603",
},
#endif
@@ -1866,7 +1822,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 32,
.dcache_bsize = 32,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e500",
.cpu_setup = __setup_cpu_e500v1,
.machine_check = machine_check_e500,
.platform = "ppc8540",
@@ -1885,7 +1840,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 32,
.dcache_bsize = 32,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e500",
.cpu_setup = __setup_cpu_e500v2,
.machine_check = machine_check_e500,
.platform = "ppc8548",
@@ -1904,7 +1858,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 64,
.dcache_bsize = 64,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e500mc",
.cpu_setup = __setup_cpu_e500mc,
.machine_check = machine_check_e500mc,
.platform = "ppce500mc",
@@ -1925,7 +1878,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 64,
.dcache_bsize = 64,
.num_pmcs = 4,
- .oprofile_cpu_type = "ppc/e500mc",
.cpu_setup = __setup_cpu_e5500,
#ifndef CONFIG_PPC32
.cpu_restore = __restore_cpu_e5500,
@@ -1947,7 +1899,6 @@ static struct cpu_spec __initdata cpu_specs[] = {
.icache_bsize = 64,
.dcache_bsize = 64,
.num_pmcs = 6,
- .oprofile_cpu_type = "ppc/e6500",
.cpu_setup = __setup_cpu_e6500,
#ifndef CONFIG_PPC32
.cpu_restore = __restore_cpu_e6500,
@@ -2015,23 +1966,10 @@ static struct cpu_spec * __init setup_cpu_spec(unsigned long offset,
t->pmc_type = old.pmc_type;
/*
- * If we have passed through this logic once before and
- * have pulled the default case because the real PVR was
- * not found inside cpu_specs[], then we are possibly
- * running in compatibility mode. In that case, let the
- * oprofiler know which set of compatibility counters to
- * pull from by making sure the oprofile_cpu_type string
- * is set to that of compatibility mode. If the
- * oprofile_cpu_type already has a value, then we are
- * possibly overriding a real PVR with a logical one,
- * and, in that case, keep the current value for
- * oprofile_cpu_type. Futhermore, let's ensure that the
+ * Let's ensure that the
* fix for the PMAO bug is enabled on compatibility mode.
*/
- if (old.oprofile_cpu_type != NULL) {
- t->oprofile_cpu_type = old.oprofile_cpu_type;
- t->cpu_features |= old.cpu_features & CPU_FTR_PMAO_BUG;
- }
+ t->cpu_features |= old.cpu_features & CPU_FTR_PMAO_BUG;
}
*PTRRELOC(&cur_cpu_spec) = &the_cpu_spec;
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 358aee7c2d79..2bb1a5ca2cff 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -101,7 +101,6 @@ static struct cpu_spec __initdata base_cpu_spec = {
.dcache_bsize = 32, /* cache info init. */
.num_pmcs = 0,
.pmc_type = PPC_PMC_DEFAULT,
- .oprofile_cpu_type = NULL,
.cpu_setup = NULL,
.cpu_restore = __restore_cpu_cpufeatures,
.machine_check_early = NULL,
@@ -379,7 +378,6 @@ static int __init feat_enable_pmu_power8(struct dt_cpu_feature *f)
cur_cpu_spec->num_pmcs = 6;
cur_cpu_spec->pmc_type = PPC_PMC_IBM;
- cur_cpu_spec->oprofile_cpu_type = "ppc64/power8";
return 1;
}
@@ -415,7 +413,6 @@ static int __init feat_enable_pmu_power9(struct dt_cpu_feature *f)
cur_cpu_spec->num_pmcs = 6;
cur_cpu_spec->pmc_type = PPC_PMC_IBM;
- cur_cpu_spec->oprofile_cpu_type = "ppc64/power9";
return 1;
}
@@ -441,7 +438,6 @@ static int __init feat_enable_pmu_power10(struct dt_cpu_feature *f)
cur_cpu_spec->num_pmcs = 6;
cur_cpu_spec->pmc_type = PPC_PMC_IBM;
- cur_cpu_spec->oprofile_cpu_type = "ppc64/power10";
return 1;
}
diff --git a/arch/powerpc/platforms/cell/spufs/spufs.h b/arch/powerpc/platforms/cell/spufs/spufs.h
index afc1d6604d12..23c6799cfa5a 100644
--- a/arch/powerpc/platforms/cell/spufs/spufs.h
+++ b/arch/powerpc/platforms/cell/spufs/spufs.h
@@ -76,7 +76,7 @@ struct spu_context {
struct address_space *mss; /* 'mss' area mappings. */
struct address_space *psmap; /* 'psmap' area mappings. */
struct mutex mapping_lock;
- u64 object_id; /* user space pointer for oprofile */
+ u64 object_id; /* user space pointer for GNU Debugger */
enum { SPU_STATE_RUNNABLE, SPU_STATE_SAVED } state;
struct mutex state_mutex;
--
2.25.0
^ permalink raw reply related
* RE: [PATCH] powerpc/32: Don't use lmw/stmw for saving/restoring non volatile regs
From: David Laight @ 2021-08-25 8:42 UTC (permalink / raw)
To: 'Segher Boessenkool', Christophe Leroy
Cc: Paul Mackerras, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20210824152813.GG1583@gate.crashing.org>
From: Segher Boessenkool
> Sent: 24 August 2021 16:28
>
> On Tue, Aug 24, 2021 at 08:16:00AM -0500, Segher Boessenkool wrote:
> > On Tue, Aug 24, 2021 at 07:54:22AM +0200, Christophe Leroy wrote:
> > > >On mpccore both lmw and stmw are only N+1 btw. But the serialization
> > > >might cost another cycle here?
> > >
> > > That coherent on MPC8xx, that's only 2 cycles.
> > > But on the mpc832x which has a e300c2 core, it looks like I have 10 cycles
> > > difference. Is anything wrong ?
> >
> > I don't know that core very well, I'll have a look.
>
> So, I don't see any difference between e300c2 and e300c1 (which is 603
> basically, for this) that is significant here. The e300c2 has two
> integer units instead of just one, but it still has only one load/store
> unit, and I don't see anything else that could matter either. Huh.
Is the cpu as brain-damaged as the (old) strongarm (SA1100 etc)
where ldm/stm always took 1 clock to check each register bit
regardless of the number of registers to copy?
(IIRC it also took the same length of time when conditionally not
executed.)
If x86 had ever had ldm/stm then it would end up being a microcoded
instruction and take forever to decode.
Intel never managed to optimise 'loop' (dec %cx and jump nz).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] powerpc/32: Don't use lmw/stmw for saving/restoring non volatile regs
From: Christophe Leroy @ 2021-08-25 9:39 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20210824131600.GF1583@gate.crashing.org>
Le 24/08/2021 à 15:16, Segher Boessenkool a écrit :
> Hi!
>
> On Tue, Aug 24, 2021 at 07:54:22AM +0200, Christophe Leroy wrote:
>> Le 23/08/2021 à 20:46, Segher Boessenkool a écrit :
>>> On Mon, Aug 23, 2021 at 03:29:12PM +0000, Christophe Leroy wrote:
>>>> Instructions lmw/stmw are interesting for functions that are rarely
>>>> used and not in the cache, because only one instruction is to be
>>>> copied into the instruction cache instead of 19. However those
>>>> instruction are less performant than 19x raw lwz/stw as they require
>>>> synchronisation plus one additional cycle.
>>>
>>> lmw takes N+2 cycles for loading N words on 603/604/750/7400, and N+3 on
>>> 7450. stmw takes N+1 cycles for storing N words on 603, N+2 on 604/750/
>>> 7400, and N+3 on 7450 (load latency is 3 instead of 2 on 7450).
>>>
>>> There is no synchronisation needed, although there is some serialisation,
>>> which of course doesn't mean much since there can be only 6 or 8 or so
>>> insns executing at once anyway.
>>
>> Yes I meant serialisation, isn't it the same as synchronisation ?
>
> Ha no, synchronisation are insns like sync and eieio :-) Synchronisation
> is architectural, serialisation is (mostly) not, it is a feature of the
> specific core.
>
>>> So, these insns are almost never slower, they can easily win cycles back
>>> because of the smaller code, too.
>>>
>>> What 32-bit core do you see where load/store multiple are more than a
>>> fraction of a cycle (per memory access) slower?
>>>
>>>> SAVE_NVGPRS / REST_NVGPRS are used in only a few places which are
>>>> mostly in interrupts entries/exits and in task switch so they are
>>>> likely already in the cache.
>>>
>>> Nothing is likely in the cache on the older cores (except in
>>> microbenchmarks), the caches are not big enough for that!
>>
>> Even syscall entries/exit pathes and/or most frequent interrupts entries
>> and interrupt exit ?
>
> It has to be measured. You are probably right for programs that use a
> lot of system calls, and (unmeasurably :-) ) wrong for those that don't.
>
> So that is a good argument: it speeds up some scenarios, and does not
> make any real impact on anything else.
>
> This also does not replace all {l,st}mw in the kernel, only those on
> interrupt paths. So it is not necessarily bad :-)
Yes exactly, I wanted to focus on interrupt paths which are the bottle neck.
So I take it that you finally don't disagree with the change.
By the way, it has to be noted that later versions of GCC do less and less use of lmw/stmw. See for
exemple show_user_instructions():
c0007114 <show_user_instructions>:
c0007114: 94 21 ff 50 stwu r1,-176(r1)
c0007118: 7d 80 00 26 mfcr r12
c000711c: 7c 08 02 a6 mflr r0
c0007120: 93 01 00 90 stw r24,144(r1)
c0007124: 93 21 00 94 stw r25,148(r1)
c0007128: 93 41 00 98 stw r26,152(r1)
c000712c: 93 61 00 9c stw r27,156(r1)
c0007130: 93 81 00 a0 stw r28,160(r1)
c0007134: 93 c1 00 a8 stw r30,168(r1)
c0007138: 91 81 00 8c stw r12,140(r1)
c000713c: 90 01 00 b4 stw r0,180(r1)
c0007140: 93 a1 00 a4 stw r29,164(r1)
c0007144: 93 e1 00 ac stw r31,172(r1)
...
c0007244: 80 01 00 b4 lwz r0,180(r1)
c0007248: 81 81 00 8c lwz r12,140(r1)
c000724c: 83 01 00 90 lwz r24,144(r1)
c0007250: 83 21 00 94 lwz r25,148(r1)
c0007254: 83 41 00 98 lwz r26,152(r1)
c0007258: 83 61 00 9c lwz r27,156(r1)
c000725c: 83 81 00 a0 lwz r28,160(r1)
c0007260: 83 a1 00 a4 lwz r29,164(r1)
c0007264: 83 c1 00 a8 lwz r30,168(r1)
c0007268: 83 e1 00 ac lwz r31,172(r1)
c000726c: 7c 08 03 a6 mtlr r0
c0007270: 7d 80 81 20 mtcrf 8,r12
c0007274: 38 21 00 b0 addi r1,r1,176
c0007278: 4e 80 00 20 blr
On older version (GCC 5.5 here) it used to be:
00000408 <show_user_instructions>:
408: 7c 08 02 a6 mflr r0
40c: 94 21 ff 40 stwu r1,-192(r1)
410: 7d 80 00 26 mfcr r12
414: be a1 00 94 stmw r21,148(r1)
418: 91 81 00 90 stw r12,144(r1)
41c: 90 01 00 c4 stw r0,196(r1)
...
504: 80 01 00 c4 lwz r0,196(r1)
508: 81 81 00 90 lwz r12,144(r1)
50c: 7c 08 03 a6 mtlr r0
510: ba a1 00 94 lmw r21,148(r1)
514: 7d 80 81 20 mtcrf 8,r12
518: 38 21 00 c0 addi r1,r1,192
51c: 4e 80 00 20 blr
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/doc: Fix htmldocs errors
From: Stephen Rothwell @ 2021-08-25 9:46 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, Jonathan Corbet
In-Reply-To: <20210825042447.106219-1-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 784 bytes --]
Hi Aneesh,
On Wed, 25 Aug 2021 09:54:47 +0530 "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> wrote:
>
> Fix make htmldocs related errors with the newly added associativity.rst
> doc file.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Documentation/powerpc/associativity.rst | 29 +++++++++++++------------
> Documentation/powerpc/index.rst | 1 +
> 2 files changed, 16 insertions(+), 14 deletions(-)
This fixes all the warnings related to this file for me.
Tested-by: Stephen Rothwell <sfr@canb.auug.org.au> # build test
Michael, it would be good if this could go in soon as without this
patch my "make htmldocs" run livelocks. :-(
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2 0/4] powerpc/64s: interrupt speedups
From: Nicholas Piggin @ 2021-08-25 12:37 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Here's a few stragglers. The first patch was submitted already but had
some bugs with unrecoverable exceptions on HPT (current->blah being
accessed before MSR[RI] was enabled). Those should be fixed now.
The others are generally for helping asynch interrupts, which are a bit
harder to measure well but important for IO and IPIs.
After this series, the SPR accesses of the interrupt handlers for radix
are becoming pretty optimal except for PPR which we could improve on,
and virt CPU accounting which is very costly -- we might disable that
by default unless someone comes up with a good reason to keep it.
Since v1:
- Compile fixes for 64e.
- Fixed a SOFT_MASK_DEBUG false positive.
- Improve function name and comments explaining why patch 2 does not
need to hard enable when PMU is enabled via sysfs.
Thanks,
Nick
Nicholas Piggin (4):
powerpc/64: handle MSR EE and RI in interrupt entry wrapper
powerpc/64s/perf: add power_pmu_wants_prompt_pmi to say whether perf
wants PMIs to be soft-NMI
powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless
perf is in use
powerpc/64s/interrupt: avoid saving CFAR in some asynchronous
interrupts
arch/powerpc/include/asm/hw_irq.h | 57 ++++++++++++++---
arch/powerpc/include/asm/interrupt.h | 31 ++++++++--
arch/powerpc/kernel/dbell.c | 3 +-
arch/powerpc/kernel/exceptions-64s.S | 93 +++++++++++++++++++---------
arch/powerpc/kernel/fpu.S | 5 ++
arch/powerpc/kernel/irq.c | 3 +-
arch/powerpc/kernel/time.c | 31 +++++-----
arch/powerpc/kernel/vector.S | 8 +++
arch/powerpc/perf/core-book3s.c | 28 +++++++++
9 files changed, 199 insertions(+), 60 deletions(-)
--
2.23.0
^ permalink raw reply
* [PATCH v2 1/4] powerpc/64: handle MSR EE and RI in interrupt entry wrapper
From: Nicholas Piggin @ 2021-08-25 12:37 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20210825123714.706201-1-npiggin@gmail.com>
Similarly to the system call change in the previous patch, the mtmsrd to
enable RI can be combined with the mtmsrd to enable EE for interrupts
which enable the latter, which tends to be the important synchronous
interrupts (i.e., page faults).
Do this by enabling EE and RI together at the beginning of the entry
wrapper if PACA_IRQ_HARD_DIS is clear, and just enabling RI if it is set
(which means something wanted EE=0).
Asynchronous interrupts set PACA_IRQ_HARD_DIS, but synchronous ones
leave it unchanged, so by default they always get EE=1 unless they
interrupt a caller that has hard disabled. When the sync interrupt
later calls interrupt_cond_local_irq_enable(), that will not require
another mtmsrd because we already enabled here.
64e is conceptually unchanged, but it also sets MSR[EE]=1 now in the
interrupt wrapper for synchronous interrupts with the same code.
On 64s, saves one mtmsrd L=1 for synchronous interrupts on 64s, which
saves about 20 cycles. For kernel-mode interrupts, both synchronous and
asynchronous, this saves an additional ~40 cycles due to the mtmsrd
being moved ahead of mfspr SPRN_AMR, which prevents a SPR scoreboard
stall (on POWER9).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/interrupt.h | 31 ++++++++++++++++++++++++----
arch/powerpc/kernel/exceptions-64s.S | 30 ---------------------------
arch/powerpc/kernel/fpu.S | 5 +++++
arch/powerpc/kernel/vector.S | 8 +++++++
4 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 6b800d3e2681..e3228a911b35 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -148,9 +148,21 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
#endif
#ifdef CONFIG_PPC64
- if (irq_soft_mask_set_return(IRQS_ALL_DISABLED) == IRQS_ENABLED)
+ bool trace_enable = false;
+
+ if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS)) {
+ if (irq_soft_mask_set_return(IRQS_DISABLED) == IRQS_ENABLED)
+ trace_enable = true;
+ } else {
+ irq_soft_mask_set(IRQS_DISABLED);
+ }
+ /* If the interrupt was taken with HARD_DIS set, don't enable MSR[EE] */
+ if (local_paca->irq_happened & PACA_IRQ_HARD_DIS)
+ __hard_RI_enable();
+ else
+ __hard_irq_enable();
+ if (trace_enable)
trace_hardirqs_off();
- local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
if (user_mode(regs)) {
CT_WARN_ON(ct_state() != CONTEXT_USER);
@@ -200,13 +212,20 @@ static inline void interrupt_exit_prepare(struct pt_regs *regs, struct interrupt
static inline void interrupt_async_enter_prepare(struct pt_regs *regs, struct interrupt_state *state)
{
+#ifdef CONFIG_PPC64
+ /* Ensure interrupt_enter_prepare does not enable MSR[EE] */
+ local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
+#endif
+ interrupt_enter_prepare(regs, state);
#ifdef CONFIG_PPC_BOOK3S_64
+ /*
+ * MSR[RI] is only enabled after interrupt_enter_prepare, so this
+ * thread flags access has to come afterward.
+ */
if (cpu_has_feature(CPU_FTR_CTRL) &&
!test_thread_local_flags(_TLF_RUNLATCH))
__ppc64_runlatch_on();
#endif
-
- interrupt_enter_prepare(regs, state);
irq_enter();
}
@@ -273,6 +292,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
BUG_ON(!arch_irq_disabled_regs(regs) && !(regs->msr & MSR_EE));
+ __hard_RI_enable();
+
/* Don't do any per-CPU operations until interrupt state is fixed */
if (nmi_disables_ftrace(regs)) {
@@ -370,6 +391,8 @@ interrupt_handler long func(struct pt_regs *regs) \
{ \
long ret; \
\
+ __hard_RI_enable(); \
+ \
ret = ____##func (regs); \
\
return ret; \
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 4aec59a77d4c..69a472c38f62 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -113,7 +113,6 @@ name:
#define IISIDE .L_IISIDE_\name\() /* Uses SRR0/1 not DAR/DSISR */
#define IDAR .L_IDAR_\name\() /* Uses DAR (or SRR0) */
#define IDSISR .L_IDSISR_\name\() /* Uses DSISR (or SRR1) */
-#define ISET_RI .L_ISET_RI_\name\() /* Run common code w/ MSR[RI]=1 */
#define IBRANCH_TO_COMMON .L_IBRANCH_TO_COMMON_\name\() /* ENTRY branch to common */
#define IREALMODE_COMMON .L_IREALMODE_COMMON_\name\() /* Common runs in realmode */
#define IMASK .L_IMASK_\name\() /* IRQ soft-mask bit */
@@ -157,9 +156,6 @@ do_define_int n
.ifndef IDSISR
IDSISR=0
.endif
- .ifndef ISET_RI
- ISET_RI=1
- .endif
.ifndef IBRANCH_TO_COMMON
IBRANCH_TO_COMMON=1
.endif
@@ -512,11 +508,6 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real)
stb r10,PACASRR_VALID(r13)
.endif
- .if ISET_RI
- li r10,MSR_RI
- mtmsrd r10,1 /* Set MSR_RI */
- .endif
-
.if ISTACK
.if IKUAP
kuap_save_amr_and_lock r9, r10, cr1, cr0
@@ -901,11 +892,6 @@ INT_DEFINE_BEGIN(system_reset)
IVEC=0x100
IAREA=PACA_EXNMI
IVIRT=0 /* no virt entry point */
- /*
- * MSR_RI is not enabled, because PACA_EXNMI and nmi stack is
- * being used, so a nested NMI exception would corrupt it.
- */
- ISET_RI=0
ISTACK=0
IKVM_REAL=1
INT_DEFINE_END(system_reset)
@@ -986,8 +972,6 @@ EXC_COMMON_BEGIN(system_reset_common)
lhz r10,PACA_IN_NMI(r13)
addi r10,r10,1
sth r10,PACA_IN_NMI(r13)
- li r10,MSR_RI
- mtmsrd r10,1
mr r10,r1
ld r1,PACA_NMI_EMERG_SP(r13)
@@ -1061,12 +1045,6 @@ INT_DEFINE_BEGIN(machine_check_early)
IAREA=PACA_EXMC
IVIRT=0 /* no virt entry point */
IREALMODE_COMMON=1
- /*
- * MSR_RI is not enabled, because PACA_EXMC is being used, so a
- * nested machine check corrupts it. machine_check_common enables
- * MSR_RI.
- */
- ISET_RI=0
ISTACK=0
IDAR=1
IDSISR=1
@@ -1077,7 +1055,6 @@ INT_DEFINE_BEGIN(machine_check)
IVEC=0x200
IAREA=PACA_EXMC
IVIRT=0 /* no virt entry point */
- ISET_RI=0
IDAR=1
IDSISR=1
IKVM_REAL=1
@@ -1147,9 +1124,6 @@ EXC_COMMON_BEGIN(machine_check_early_common)
BEGIN_FTR_SECTION
bl enable_machine_check
END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
- li r10,MSR_RI
- mtmsrd r10,1
-
addi r3,r1,STACK_FRAME_OVERHEAD
bl machine_check_early
std r3,RESULT(r1) /* Save result */
@@ -1237,10 +1211,6 @@ EXC_COMMON_BEGIN(machine_check_common)
* save area: PACA_EXMC instead of PACA_EXGEN.
*/
GEN_COMMON machine_check
-
- /* Enable MSR_RI when finished with PACA_EXMC */
- li r10,MSR_RI
- mtmsrd r10,1
addi r3,r1,STACK_FRAME_OVERHEAD
bl machine_check_exception
b interrupt_return_srr
diff --git a/arch/powerpc/kernel/fpu.S b/arch/powerpc/kernel/fpu.S
index 6010adcee16e..eabd578cb772 100644
--- a/arch/powerpc/kernel/fpu.S
+++ b/arch/powerpc/kernel/fpu.S
@@ -81,7 +81,12 @@ EXPORT_SYMBOL(store_fp_state)
*/
_GLOBAL(load_up_fpu)
mfmsr r5
+#ifdef CONFIG_PPC_BOOK3S_64
+ /* interrupt doesn't set MSR[RI] and HPT can fault on current access */
+ ori r5,r5,MSR_FP|MSR_RI
+#else
ori r5,r5,MSR_FP
+#endif
#ifdef CONFIG_VSX
BEGIN_FTR_SECTION
oris r5,r5,MSR_VSX@h
diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S
index fc120fac1910..ead2900d9bb0 100644
--- a/arch/powerpc/kernel/vector.S
+++ b/arch/powerpc/kernel/vector.S
@@ -47,6 +47,10 @@ EXPORT_SYMBOL(store_vr_state)
*/
_GLOBAL(load_up_altivec)
mfmsr r5 /* grab the current MSR */
+#ifdef CONFIG_PPC_BOOK3S_64
+ /* interrupt doesn't set MSR[RI] and HPT can fault on current access */
+ ori r5,r5,MSR_RI
+#endif
oris r5,r5,MSR_VEC@h
MTMSRD(r5) /* enable use of AltiVec now */
isync
@@ -128,6 +132,10 @@ _GLOBAL(load_up_vsx)
andis. r5,r12,MSR_VEC@h
beql+ load_up_altivec /* skip if already loaded */
+ /* interrupt doesn't set MSR[RI] and HPT can fault on current access */
+ li r5,MSR_RI
+ mtmsrd r5,1
+
ld r4,PACACURRENT(r13)
addi r4,r4,THREAD /* Get THREAD */
li r6,1
--
2.23.0
^ permalink raw reply related
* [PATCH v2 2/4] powerpc/64s/perf: add power_pmu_wants_prompt_pmi to say whether perf wants PMIs to be soft-NMI
From: Nicholas Piggin @ 2021-08-25 12:37 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20210825123714.706201-1-npiggin@gmail.com>
Interrupt code enables MSR[EE] in some irq handlers while keeping local
irqs disabled via soft-mask, allowing PMI interrupts to be taken as
soft-NMI to improve profiling of irq handlers.
When perf is not enabled, there is no point to doing this, it's
additional overhead. So provide a function that can say if PMIs should
be taken promptly if possible.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 2 ++
arch/powerpc/perf/core-book3s.c | 28 ++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index 21cc571ea9c2..b987822e552e 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -306,6 +306,8 @@ static inline bool lazy_irq_pending_nocheck(void)
return __lazy_irq_pending(local_paca->irq_happened);
}
+bool power_pmu_wants_prompt_pmi(void);
+
/*
* This is called by asynchronous interrupts to conditionally
* re-enable hard interrupts after having cleared the source
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index bb0ee716de91..771f49aea8f4 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -17,6 +17,7 @@
#include <asm/firmware.h>
#include <asm/ptrace.h>
#include <asm/code-patching.h>
+#include <asm/hw_irq.h>
#include <asm/interrupt.h>
#ifdef CONFIG_PPC64
@@ -2380,6 +2381,33 @@ static void perf_event_interrupt(struct pt_regs *regs)
perf_sample_event_took(sched_clock() - start_clock);
}
+/*
+ * If the perf subsystem wants performance monitor interrupts as soon as
+ * possible (e.g., to sample the instruction address and stack chain),
+ * this should return true. The IRQ masking code can then enable MSR[EE]
+ * in some places (e.g., interrupt handlers) that allows PMI interrupts
+ * though to improve accuracy of profiles, at the cost of some performance.
+ *
+ * The PMU counters can be enabled by other means (e.g., sysfs raw SPR
+ * access), but in that case there is no need for prompt PMI handling.
+ *
+ * This currently returns true if any perf counter is being used. It
+ * could possibly return false if only events are being counted rather than
+ * samples being taken, but for now this is good enough.
+ */
+bool power_pmu_wants_prompt_pmi(void)
+{
+ struct cpu_hw_events *cpuhw;
+
+ /* Could this simply test local_paca->pmcregs_in_use? */
+
+ if (!ppmu)
+ return false;
+
+ cpuhw = this_cpu_ptr(&cpu_hw_events);
+ return cpuhw->n_events;
+}
+
static int power_pmu_prepare_cpu(unsigned int cpu)
{
struct cpu_hw_events *cpuhw = &per_cpu(cpu_hw_events, cpu);
--
2.23.0
^ permalink raw reply related
* [PATCH v2 3/4] powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless perf is in use
From: Nicholas Piggin @ 2021-08-25 12:37 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20210825123714.706201-1-npiggin@gmail.com>
Enabling MSR[EE] in interrupt handlers while interrupts are still soft
masked allows PMIs to profile interrupt handlers to some degree, beyond
what SIAR latching allows.
When perf is not being used, this is almost useless work. It requires an
extra mtmsrd in the irq handler, and it also opens the door to masked
interrupts hitting and requiring replay, which is more expensive than
just taking them directly. This effect can be noticable in high IRQ
workloads.
Avoid enabling MSR[EE] unless perf is currently in use. This saves about
60 cycles (or 8%) on a simple decrementer interrupt microbenchmark.
Replayed interrupts drop from 1.4% of interrupts to 0.003%.
This does prevent the soft-nmi interrupt being taken in these handlers,
but that's not too reliable anyway. The SMP watchdog will continue to be
the reliable way to catch lockups.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 55 ++++++++++++++++++++++++++-----
arch/powerpc/kernel/dbell.c | 3 +-
arch/powerpc/kernel/irq.c | 3 +-
arch/powerpc/kernel/time.c | 31 ++++++++---------
4 files changed, 66 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index b987822e552e..8c78c40c006e 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -309,17 +309,54 @@ static inline bool lazy_irq_pending_nocheck(void)
bool power_pmu_wants_prompt_pmi(void);
/*
- * This is called by asynchronous interrupts to conditionally
- * re-enable hard interrupts after having cleared the source
- * of the interrupt. They are kept disabled if there is a different
- * soft-masked interrupt pending that requires hard masking.
+ * This is called by asynchronous interrupts to check whether to
+ * conditionally re-enable hard interrupts after having cleared
+ * the source of the interrupt. They are kept disabled if there
+ * is a different soft-masked interrupt pending that requires hard
+ * masking.
*/
-static inline void may_hard_irq_enable(void)
+static inline bool should_hard_irq_enable(void)
{
- if (!(get_paca()->irq_happened & PACA_IRQ_MUST_HARD_MASK)) {
- get_paca()->irq_happened &= ~PACA_IRQ_HARD_DIS;
- __hard_irq_enable();
- }
+#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
+ WARN_ON(irq_soft_mask_return() == IRQS_ENABLED);
+ WARN_ON(mfmsr() & MSR_EE);
+#endif
+#ifdef CONFIG_PERF_EVENTS
+ /*
+ * If the PMU is not running, there is not much reason to enable
+ * MSR[EE] in irq handlers because any interrupts would just be
+ * soft-masked.
+ *
+ * TODO: Add test for 64e
+ */
+ if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !power_pmu_wants_prompt_pmi())
+ return false;
+
+ if (get_paca()->irq_happened & PACA_IRQ_MUST_HARD_MASK)
+ return false;
+
+ return true;
+#else
+ return false;
+#endif
+}
+
+/*
+ * Do the hard enabling, only call this if should_hard_irq_enable is true.
+ */
+static inline void do_hard_irq_enable(void)
+{
+#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG
+ WARN_ON(irq_soft_mask_return() == IRQS_ENABLED);
+ WARN_ON(get_paca()->irq_happened & PACA_IRQ_MUST_HARD_MASK);
+ WARN_ON(mfmsr() & MSR_EE);
+#endif
+ /*
+ * This allows PMI interrupts (and watchdog soft-NMIs) through.
+ * There is no other reason to enable this way.
+ */
+ get_paca()->irq_happened &= ~PACA_IRQ_HARD_DIS;
+ __hard_irq_enable();
}
static inline bool arch_irq_disabled_regs(struct pt_regs *regs)
diff --git a/arch/powerpc/kernel/dbell.c b/arch/powerpc/kernel/dbell.c
index 5545c9cd17c1..f55c6fb34a3a 100644
--- a/arch/powerpc/kernel/dbell.c
+++ b/arch/powerpc/kernel/dbell.c
@@ -27,7 +27,8 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(doorbell_exception)
ppc_msgsync();
- may_hard_irq_enable();
+ if (should_hard_irq_enable())
+ do_hard_irq_enable();
kvmppc_clear_host_ipi(smp_processor_id());
__this_cpu_inc(irq_stat.doorbell_irqs);
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 551b653228c4..f658aa22a21e 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -739,7 +739,8 @@ void __do_irq(struct pt_regs *regs)
irq = ppc_md.get_irq();
/* We can hard enable interrupts now to allow perf interrupts */
- may_hard_irq_enable();
+ if (should_hard_irq_enable())
+ do_hard_irq_enable();
/* And finally process it */
if (unlikely(!irq))
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index c487ba5a6e11..e7aab5540d09 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -567,22 +567,23 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
return;
}
- /* Ensure a positive value is written to the decrementer, or else
- * some CPUs will continue to take decrementer exceptions. When the
- * PPC_WATCHDOG (decrementer based) is configured, keep this at most
- * 31 bits, which is about 4 seconds on most systems, which gives
- * the watchdog a chance of catching timer interrupt hard lockups.
- */
- if (IS_ENABLED(CONFIG_PPC_WATCHDOG))
- set_dec(0x7fffffff);
- else
- set_dec(decrementer_max);
-
- /* Conditionally hard-enable interrupts now that the DEC has been
- * bumped to its maximum value
- */
- may_hard_irq_enable();
+ /* Conditionally hard-enable interrupts. */
+ if (should_hard_irq_enable()) {
+ /*
+ * Ensure a positive value is written to the decrementer, or
+ * else some CPUs will continue to take decrementer exceptions.
+ * When the PPC_WATCHDOG (decrementer based) is configured,
+ * keep this at most 31 bits, which is about 4 seconds on most
+ * systems, which gives the watchdog a chance of catching timer
+ * interrupt hard lockups.
+ */
+ if (IS_ENABLED(CONFIG_PPC_WATCHDOG))
+ set_dec(0x7fffffff);
+ else
+ set_dec(decrementer_max);
+ do_hard_irq_enable();
+ }
#if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
if (atomic_read(&ppc_n_lost_interrupts) != 0)
--
2.23.0
^ permalink raw reply related
* [PATCH v2 4/4] powerpc/64s/interrupt: avoid saving CFAR in some asynchronous interrupts
From: Nicholas Piggin @ 2021-08-25 12:37 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20210825123714.706201-1-npiggin@gmail.com>
Reading the CFAR register is quite costly (~20 cycles on POWER9). It is
a good idea to have for most synchronous interrupts, but for async ones
it is much less important.
Doorbell, external, and decrementer interrupts are the important
asynchronous ones. HV interrupts can't skip CFAR if KVM HV is possible,
because it might be a guest exit that requires CFAR preserved. But for
now the important pseries interrupts can avoid loading CFAR.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 63 ++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 69a472c38f62..42badd7beaf0 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -111,6 +111,8 @@ name:
#define IAREA .L_IAREA_\name\() /* PACA save area */
#define IVIRT .L_IVIRT_\name\() /* Has virt mode entry point */
#define IISIDE .L_IISIDE_\name\() /* Uses SRR0/1 not DAR/DSISR */
+#define ICFAR .L_ICFAR_\name\() /* Uses CFAR */
+#define ICFAR_IF_HVMODE .L_ICFAR_IF_HVMODE_\name\() /* Uses CFAR if HV */
#define IDAR .L_IDAR_\name\() /* Uses DAR (or SRR0) */
#define IDSISR .L_IDSISR_\name\() /* Uses DSISR (or SRR1) */
#define IBRANCH_TO_COMMON .L_IBRANCH_TO_COMMON_\name\() /* ENTRY branch to common */
@@ -150,6 +152,12 @@ do_define_int n
.ifndef IISIDE
IISIDE=0
.endif
+ .ifndef ICFAR
+ ICFAR=1
+ .endif
+ .ifndef ICFAR_IF_HVMODE
+ ICFAR_IF_HVMODE=0
+ .endif
.ifndef IDAR
IDAR=0
.endif
@@ -287,9 +295,21 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
HMT_MEDIUM
std r10,IAREA+EX_R10(r13) /* save r10 - r12 */
+ .if ICFAR
BEGIN_FTR_SECTION
mfspr r10,SPRN_CFAR
END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
+ .elseif ICFAR_IF_HVMODE
+BEGIN_FTR_SECTION
+ BEGIN_FTR_SECTION_NESTED(69)
+ mfspr r10,SPRN_CFAR
+ END_FTR_SECTION_NESTED(CPU_FTR_CFAR, CPU_FTR_CFAR, 69)
+FTR_SECTION_ELSE
+ BEGIN_FTR_SECTION_NESTED(69)
+ li r10,0
+ END_FTR_SECTION_NESTED(CPU_FTR_CFAR, CPU_FTR_CFAR, 69)
+ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE | CPU_FTR_ARCH_206)
+ .endif
.if \ool
.if !\virt
b tramp_real_\name
@@ -305,9 +325,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
BEGIN_FTR_SECTION
std r9,IAREA+EX_PPR(r13)
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
+ .if ICFAR || ICFAR_IF_HVMODE
BEGIN_FTR_SECTION
std r10,IAREA+EX_CFAR(r13)
END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
+ .endif
INTERRUPT_TO_KERNEL
mfctr r10
std r10,IAREA+EX_CTR(r13)
@@ -559,7 +581,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
.endif
BEGIN_FTR_SECTION
+ .if ICFAR || ICFAR_IF_HVMODE
ld r10,IAREA+EX_CFAR(r13)
+ .else
+ li r10,0
+ .endif
std r10,ORIG_GPR3(r1)
END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
ld r10,IAREA+EX_CTR(r13)
@@ -1501,6 +1527,12 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
*
* If soft masked, the masked handler will note the pending interrupt for
* replay, and clear MSR[EE] in the interrupted context.
+ *
+ * CFAR is not required because this is an asynchronous interrupt that in
+ * general won't have much bearing on the state of the CPU, with the possible
+ * exception of crash/debug IPIs, but those are generally moving to use SRESET
+ * IPIs. Unless this is an HV interrupt and KVM HV is possible, in which case
+ * it may be exiting the guest and need CFAR to be saved.
*/
INT_DEFINE_BEGIN(hardware_interrupt)
IVEC=0x500
@@ -1508,6 +1540,10 @@ INT_DEFINE_BEGIN(hardware_interrupt)
IMASK=IRQS_DISABLED
IKVM_REAL=1
IKVM_VIRT=1
+ ICFAR=0
+#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+ ICFAR_IF_HVMODE=1
+#endif
INT_DEFINE_END(hardware_interrupt)
EXC_REAL_BEGIN(hardware_interrupt, 0x500, 0x100)
@@ -1726,6 +1762,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_TM)
* If PPC_WATCHDOG is configured, the soft masked handler will actually set
* things back up to run soft_nmi_interrupt as a regular interrupt handler
* on the emergency stack.
+ *
+ * CFAR is not required because this is asynchronous (see hardware_interrupt).
+ * A watchdog interrupt may like to have CFAR, but usually the interesting
+ * branch is long gone by that point (e.g., infinite loop).
*/
INT_DEFINE_BEGIN(decrementer)
IVEC=0x900
@@ -1733,6 +1773,7 @@ INT_DEFINE_BEGIN(decrementer)
#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
IKVM_REAL=1
#endif
+ ICFAR=0
INT_DEFINE_END(decrementer)
EXC_REAL_BEGIN(decrementer, 0x900, 0x80)
@@ -1808,6 +1849,8 @@ EXC_COMMON_BEGIN(hdecrementer_common)
* If soft masked, the masked handler will note the pending interrupt for
* replay, leaving MSR[EE] enabled in the interrupted context because the
* doorbells are edge triggered.
+ *
+ * CFAR is not required, similarly to hardware_interrupt.
*/
INT_DEFINE_BEGIN(doorbell_super)
IVEC=0xa00
@@ -1815,6 +1858,7 @@ INT_DEFINE_BEGIN(doorbell_super)
#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
IKVM_REAL=1
#endif
+ ICFAR=0
INT_DEFINE_END(doorbell_super)
EXC_REAL_BEGIN(doorbell_super, 0xa00, 0x100)
@@ -1866,6 +1910,7 @@ INT_DEFINE_BEGIN(system_call)
IVEC=0xc00
IKVM_REAL=1
IKVM_VIRT=1
+ ICFAR=0
INT_DEFINE_END(system_call)
.macro SYSTEM_CALL virt
@@ -2164,6 +2209,11 @@ EXC_COMMON_BEGIN(hmi_exception_common)
* Interrupt 0xe80 - Directed Hypervisor Doorbell Interrupt.
* This is an asynchronous interrupt in response to a msgsnd doorbell.
* Similar to the 0xa00 doorbell but for host rather than guest.
+ *
+ * CFAR is not required (similar to doorbell_interrupt), unless KVM HV
+ * is enabled, in which case it may be a guest exit. Most PowerNV kernels
+ * include KVM support so it would be nice if this could be dynamically
+ * patched out if KVM was not currently running any guests.
*/
INT_DEFINE_BEGIN(h_doorbell)
IVEC=0xe80
@@ -2171,6 +2221,9 @@ INT_DEFINE_BEGIN(h_doorbell)
IMASK=IRQS_DISABLED
IKVM_REAL=1
IKVM_VIRT=1
+#ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+ ICFAR=0
+#endif
INT_DEFINE_END(h_doorbell)
EXC_REAL_BEGIN(h_doorbell, 0xe80, 0x20)
@@ -2194,6 +2247,9 @@ EXC_COMMON_BEGIN(h_doorbell_common)
* Interrupt 0xea0 - Hypervisor Virtualization Interrupt.
* This is an asynchronous interrupt in response to an "external exception".
* Similar to 0x500 but for host only.
+ *
+ * Like h_doorbell, CFAR is only required for KVM HV because this can be
+ * a guest exit.
*/
INT_DEFINE_BEGIN(h_virt_irq)
IVEC=0xea0
@@ -2201,6 +2257,9 @@ INT_DEFINE_BEGIN(h_virt_irq)
IMASK=IRQS_DISABLED
IKVM_REAL=1
IKVM_VIRT=1
+#ifndef CONFIG_KVM_BOOK3S_HV_POSSIBLE
+ ICFAR=0
+#endif
INT_DEFINE_END(h_virt_irq)
EXC_REAL_BEGIN(h_virt_irq, 0xea0, 0x20)
@@ -2237,6 +2296,8 @@ EXC_VIRT_NONE(0x4ee0, 0x20)
*
* If soft masked, the masked handler will note the pending interrupt for
* replay, and clear MSR[EE] in the interrupted context.
+ *
+ * CFAR is not used by perf interrupts so not required.
*/
INT_DEFINE_BEGIN(performance_monitor)
IVEC=0xf00
@@ -2244,6 +2305,7 @@ INT_DEFINE_BEGIN(performance_monitor)
#ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
IKVM_REAL=1
#endif
+ ICFAR=0
INT_DEFINE_END(performance_monitor)
EXC_REAL_BEGIN(performance_monitor, 0xf00, 0x20)
@@ -2668,6 +2730,7 @@ EXC_VIRT_NONE(0x5800, 0x100)
INT_DEFINE_BEGIN(soft_nmi)
IVEC=0x900
ISTACK=0
+ ICFAR=0
INT_DEFINE_END(soft_nmi)
/*
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] powerpc/pseries: Parse control memory access error
From: Ganesh @ 2021-08-25 11:03 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, mahesh, npiggin
In-Reply-To: <87eeajcpmq.fsf@mpe.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]
On 8/24/21 12:09 PM, Michael Ellerman wrote:
> Hi Ganesh,
>
> Some comments below ...
>
> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>> Add support to parse and log control memory access
>> error for pseries.
>>
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>> v2: No changes in this patch.
>> ---
>> arch/powerpc/platforms/pseries/ras.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
>> index 167f2e1b8d39..608c35cad0c3 100644
>> --- a/arch/powerpc/platforms/pseries/ras.c
>> +++ b/arch/powerpc/platforms/pseries/ras.c
>> @@ -80,6 +80,7 @@ struct pseries_mc_errorlog {
>> #define MC_ERROR_TYPE_TLB 0x04
>> #define MC_ERROR_TYPE_D_CACHE 0x05
>> #define MC_ERROR_TYPE_I_CACHE 0x07
>> +#define MC_ERROR_TYPE_CTRL_MEM_ACCESS 0x08
> ...
>>
>> +#define MC_ERROR_CTRL_MEM_ACCESS_PTABLE_WALK 0
>> +#define MC_ERROR_CTRL_MEM_ACCESS_OP_ACCESS 1
>
> Where do the above values come from?
It is from latest PAPR that added support for control memory error.
>> +
>> static inline u8 rtas_mc_error_sub_type(const struct pseries_mc_errorlog *mlog)
>> {
>> switch (mlog->error_type) {
>> @@ -112,6 +116,8 @@ static inline u8 rtas_mc_error_sub_type(const struct pseries_mc_errorlog *mlog)
>> case MC_ERROR_TYPE_ERAT:
>> case MC_ERROR_TYPE_TLB:
>> return (mlog->sub_err_type & 0x03);
>> + case MC_ERROR_TYPE_CTRL_MEM_ACCESS:
>> + return (mlog->sub_err_type & 0x70) >> 4;
> Can you add to the comment above sub_err_type explaining what these bits are.
Sure, for other errors it is explained in pseries_mc_errorlog definition, ill add it there.
>> default:
>> return 0;
>> }
>> @@ -699,6 +705,21 @@ static int mce_handle_err_virtmode(struct pt_regs *regs,
>> case MC_ERROR_TYPE_I_CACHE:
>> mce_err.error_type = MCE_ERROR_TYPE_ICACHE;
>> break;
>> + case MC_ERROR_TYPE_CTRL_MEM_ACCESS:
>> + mce_err.error_type = MCE_ERROR_TYPE_RA;
>> + if (mce_log->sub_err_type & 0x80)
> This appears many times in the file.
>
> Can we add eg. MC_EFFECTIVE_ADDR_PROVIDED?
ok, thanks.
>> + eaddr = be64_to_cpu(mce_log->effective_address);
>> + switch (err_sub_type) {
>> + case MC_ERROR_CTRL_MEM_ACCESS_PTABLE_WALK:
>> + mce_err.u.ra_error_type =
>> + MCE_RA_ERROR_PAGE_TABLE_WALK_LOAD_STORE_FOREIGN;
> That name is ridiculously long, but I guess that's not your fault :)
> We can fix it up in a later patch.
>
>> + break;
>> + case MC_ERROR_CTRL_MEM_ACCESS_OP_ACCESS:
>> + mce_err.u.ra_error_type =
>> + MCE_RA_ERROR_LOAD_STORE_FOREIGN;
>> + break;
>> + }
>> + break;
> cheers
[-- Attachment #2: Type: text/html, Size: 4268 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/3] selftests/powerpc: Add test for real address error handling
From: Ganesh @ 2021-08-25 11:31 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, mahesh, npiggin
In-Reply-To: <87a6l7c8ku.fsf@mpe.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]
On 8/24/21 6:18 PM, Michael Ellerman wrote:
> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>> Add test for real address or control memory address access
>> error handling, using NX-GZIP engine.
>>
>> The error is injected by accessing the control memory address
>> using illegal instruction, on successful handling the process
>> attempting to access control memory address using illegal
>> instruction receives SIGBUS.
> ...
>
>> diff --git a/tools/testing/selftests/powerpc/mce/inject-ra-err.sh b/tools/testing/selftests/powerpc/mce/inject-ra-err.sh
>> new file mode 100755
>> index 000000000000..3633cdc651a1
>> --- /dev/null
>> +++ b/tools/testing/selftests/powerpc/mce/inject-ra-err.sh
>> @@ -0,0 +1,18 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +if [[ ! -w /dev/crypto/nx-gzip ]]; then
>> + echo "WARN: Can't access /dev/crypto/nx-gzip, skipping"
>> + exit 0
>> +fi
>> +
>> +timeout 5 ./inject-ra-err
>> +
>> +# 128 + 7 (SIGBUS) = 135, 128 is a exit code with special meaning.
>> +if [ $? -ne 135 ]; then
>> + echo "FAILED: Real address or Control memory access error not handled"
>> + exit $?
>> +fi
>> +
>> +echo "OK: Real address or Control memory access error is handled"
>> +exit 0
> I don't think we really need the shell script, we should be able to do
> all that in the C code.
>
> Can you try this?
it works!, We need to set timeout, with 120 sec timeout we may flood the dmesg.
Thanks.
>
> cheers
>
> diff --git a/tools/testing/selftests/powerpc/mce/Makefile b/tools/testing/selftests/powerpc/mce/Makefile
> new file mode 100644
> index 000000000000..2424513982d9
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mce/Makefile
> @@ -0,0 +1,7 @@
> +#SPDX-License-Identifier: GPL-2.0-or-later
> +
> +TEST_GEN_PROGS := inject-ra-err
> +
> +include ../../lib.mk
> +
> +$(TEST_GEN_PROGS): ../harness.c
> diff --git a/tools/testing/selftests/powerpc/mce/inject-ra-err.c b/tools/testing/selftests/powerpc/mce/inject-ra-err.c
> new file mode 100644
> index 000000000000..ba0f9c28f786
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mce/inject-ra-err.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "vas-api.h"
> +#include "utils.h"
> +
> +static bool faulted;
> +
> +static void sigbus_handler(int n, siginfo_t *info, void *ctxt_v)
> +{
> + ucontext_t *ctxt = (ucontext_t *)ctxt_v;
> + struct pt_regs *regs = ctxt->uc_mcontext.regs;
> +
> + faulted = true;
> + regs->nip += 4;
> +}
> +
> +static int test_ra_error(void)
> +{
> + struct vas_tx_win_open_attr attr;
> + int fd, *paste_addr;
> + char *devname = "/dev/crypto/nx-gzip";
> + struct sigaction act = {
> + .sa_sigaction = sigbus_handler,
> + .sa_flags = SA_SIGINFO,
> + };
> +
> + memset(&attr, 0, sizeof(attr));
> + attr.version = 1;
> + attr.vas_id = 0;
> +
> + SKIP_IF(!access(devname, F_OK));
> +
> + fd = open(devname, O_RDWR);
> + FAIL_IF(fd < 0);
> + FAIL_IF(ioctl(fd, VAS_TX_WIN_OPEN, &attr) < 0);
> + FAIL_IF(sigaction(SIGBUS, &act, NULL) != 0);
> +
> + paste_addr = mmap(NULL, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0ULL);
> +
> + /* The following assignment triggers exception */
> + mb();
> + *paste_addr = 1;
> + mb();
> +
> + FAIL_IF(!faulted);
> +
> + return 0;
> +}
> +
> +int main(void)
> +{
> + return test_harness(test_ra_error, "inject-ra-err");
> +}
[-- Attachment #2: Type: text/html, Size: 4197 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc/pseries: Parse control memory access error
From: Ganesh @ 2021-08-25 11:36 UTC (permalink / raw)
To: Segher Boessenkool, Michael Ellerman; +Cc: mikey, linuxppc-dev, mahesh, npiggin
In-Reply-To: <20210824212420.GL1583@gate.crashing.org>
[-- Attachment #1: Type: text/plain, Size: 591 bytes --]
On 8/25/21 2:54 AM, Segher Boessenkool wrote:
> On Tue, Aug 24, 2021 at 04:39:57PM +1000, Michael Ellerman wrote:
>>> + case MC_ERROR_CTRL_MEM_ACCESS_PTABLE_WALK:
>>> + mce_err.u.ra_error_type =
>>> + MCE_RA_ERROR_PAGE_TABLE_WALK_LOAD_STORE_FOREIGN;
>> That name is ridiculously long, but I guess that's not your fault :)
>> We can fix it up in a later patch.
> It also has surprisingly little information content for the 47 chars
> length it has :-) What does this even mean?!
It means control memory access error/real address error is detected during page
table walk.
>
> Segher
[-- Attachment #2: Type: text/html, Size: 1340 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc/numa: Print debug statements only when required
From: Michael Ellerman @ 2021-08-25 13:01 UTC (permalink / raw)
To: Srikar Dronamraju, Laurent Dufour
Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Peter Zijlstra,
Geetika Moolchandani, Valentin Schneider, linuxppc-dev,
Ingo Molnar
In-Reply-To: <20210823093801.GK21942@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Laurent Dufour <ldufour@linux.ibm.com> [2021-08-23 11:21:33]:
>> Le 21/08/2021 à 12:25, Srikar Dronamraju a écrit :
>> > Currently, a debug message gets printed every time an attempt to
>> > add(remove) a CPU. However this is redundant if the CPU is already added
>> > (removed) from the node.
>> >
>> > Cc: linuxppc-dev@lists.ozlabs.org
>> > Cc: Nathan Lynch <nathanl@linux.ibm.com>
>> > Cc: Michael Ellerman <mpe@ellerman.id.au>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Valentin Schneider <valentin.schneider@arm.com>
>> > Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
>> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
>> > Cc: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
>> > Cc: Laurent Dufour <ldufour@linux.ibm.com>
>> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> > ---
>> > arch/powerpc/mm/numa.c | 11 +++++------
>> > 1 file changed, 5 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> > index f2bf98bdcea2..fbe03f6840e0 100644
>> > --- a/arch/powerpc/mm/numa.c
>> > +++ b/arch/powerpc/mm/numa.c
>> > @@ -141,10 +141,11 @@ static void map_cpu_to_node(int cpu, int node)
>> > {
>> > update_numa_cpu_lookup_table(cpu, node);
>> > - dbg("adding cpu %d to node %d\n", cpu, node);
>> > - if (!(cpumask_test_cpu(cpu, node_to_cpumask_map[node])))
>> > + if (!(cpumask_test_cpu(cpu, node_to_cpumask_map[node]))) {
>> > + dbg("adding cpu %d to node %d\n", cpu, node);
>> > cpumask_set_cpu(cpu, node_to_cpumask_map[node]);
>> > + }
>> > }
>> > #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PPC_SPLPAR)
>> > @@ -152,13 +153,11 @@ static void unmap_cpu_from_node(unsigned long cpu)
>> > {
>> > int node = numa_cpu_lookup_table[cpu];
>> > - dbg("removing cpu %lu from node %d\n", cpu, node);
>> > -
>> > if (cpumask_test_cpu(cpu, node_to_cpumask_map[node])) {
>> > cpumask_clear_cpu(cpu, node_to_cpumask_map[node]);
>> > + dbg("removing cpu %lu from node %d\n", cpu, node);
>> > } else {
>> > - printk(KERN_ERR "WARNING: cpu %lu not found in node %d\n",
>> > - cpu, node);
>> > + pr_err("WARNING: cpu %lu not found in node %d\n", cpu, node);
>>
>> Would pr_warn() be more appropriate here (or removing the "WARNING" statement)?
>
> Its a fair point.
>
> Michael,
>
> Do you want me to resend this patch with s/pr_err/pr_warn for the above
> line?
I think what I'd prefer is if we stopped using this custom dbg() stuff
in numa.c, and cleaned up all the messages to use pr_xxx().
Those debug statements only appear if you boot with numa=debug, which is
not documented anywhere and I had completely forgotten existed TBH.
These days there's CONFIG_DYNAMIC_DEBUG for turning on/off messages,
which is much more flexible.
So can we drop the numa=debug bits, and convert all the dbg()s to
pr_debug().
And then do a pass converting all the printk("NUMA: ") to pr_xxx() which
will get "numa:" from pr_fmt().
cheers
^ permalink raw reply
* [PATCH] powerpc: Redefine HMT_xxx macros as empty on PPC32
From: Christophe Leroy @ 2021-08-25 13:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
HMT_xxx macros are macros for adjusting thread priority
(hardware multi-threading) are macros inherited from PPC64
via commit 5f7c690728ac ("[PATCH] powerpc: Merged ppc_asm.h")
Those instructions are pointless on PPC32, but some common
fonctions like arch_cpu_idle() use them.
So make them empty on PPC32 to avoid those instructions.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/vdso/processor.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/powerpc/include/asm/vdso/processor.h b/arch/powerpc/include/asm/vdso/processor.h
index e072577bc7c0..8d79f994b4aa 100644
--- a/arch/powerpc/include/asm/vdso/processor.h
+++ b/arch/powerpc/include/asm/vdso/processor.h
@@ -5,12 +5,21 @@
#ifndef __ASSEMBLY__
/* Macros for adjusting thread priority (hardware multi-threading) */
+#ifdef CONFIG_PPC64
#define HMT_very_low() asm volatile("or 31, 31, 31 # very low priority")
#define HMT_low() asm volatile("or 1, 1, 1 # low priority")
#define HMT_medium_low() asm volatile("or 6, 6, 6 # medium low priority")
#define HMT_medium() asm volatile("or 2, 2, 2 # medium priority")
#define HMT_medium_high() asm volatile("or 5, 5, 5 # medium high priority")
#define HMT_high() asm volatile("or 3, 3, 3 # high priority")
+#else
+#define HMT_very_low()
+#define HMT_low()
+#define HMT_medium_low()
+#define HMT_medium()
+#define HMT_medium_high()
+#define HMT_high()
+#endif
#ifdef CONFIG_PPC64
#define cpu_relax() do { HMT_low(); HMT_medium(); barrier(); } while (0)
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc: Make set_endian() return EINVAL when not supporting little endian
From: Christophe Leroy @ 2021-08-25 13:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
There is no point in modifying MSR_LE bit on CPUs not supporting
little endian.
Just like done for get_endian(), make set_endian() return
EINVAL in that case.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/process.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 185beb290580..b2b9919795a2 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1995,6 +1995,10 @@ int set_endian(struct task_struct *tsk, unsigned int val)
{
struct pt_regs *regs = tsk->thread.regs;
+ if (!cpu_has_feature(CPU_FTR_PPC_LE) &&
+ !cpu_has_feature(CPU_FTR_REAL_LE))
+ return -EINVAL;
+
if ((val == PR_ENDIAN_LITTLE && !cpu_has_feature(CPU_FTR_REAL_LE)) ||
(val == PR_ENDIAN_PPC_LITTLE && !cpu_has_feature(CPU_FTR_PPC_LE)))
return -EINVAL;
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] powerpc/pseries: Parse control memory access error
From: Segher Boessenkool @ 2021-08-25 14:47 UTC (permalink / raw)
To: Ganesh; +Cc: mikey, linuxppc-dev, mahesh, npiggin
In-Reply-To: <92790955-ce8d-1826-2317-c889038545fc@linux.ibm.com>
On Wed, Aug 25, 2021 at 05:06:29PM +0530, Ganesh wrote:
>
> On 8/25/21 2:54 AM, Segher Boessenkool wrote:
> >On Tue, Aug 24, 2021 at 04:39:57PM +1000, Michael Ellerman wrote:
> >>>+ case MC_ERROR_CTRL_MEM_ACCESS_PTABLE_WALK:
> >>>+ mce_err.u.ra_error_type =
> >>>+ MCE_RA_ERROR_PAGE_TABLE_WALK_LOAD_STORE_FOREIGN;
> >>That name is ridiculously long, but I guess that's not your fault :)
> >>We can fix it up in a later patch.
> >It also has surprisingly little information content for the 47 chars
> >length it has :-) What does this even mean?!
>
> It means control memory access error/real address error is detected during
> page
> table walk.
This isn't obvious from the name. The name contains some words your
explanation does not, as well: LOAD, STORE, FOREIGN. Most importantly,
the name is just a jumble of words, with no apparent connection between
them.
I didn't ask for an explanation, sorry if you misunderstood. I was just
exploring the many ways this name is baffling :-)
Segher
^ permalink raw reply
* Re: [PATCH] ASoC: imx-rpmsg: change dev_err to dev_err_probe for -EPROBE_DEFER
From: Mark Brown @ 2021-08-25 15:06 UTC (permalink / raw)
To: nicoleotsuka, festevam, tiwai, Xiubo.Lee, timur, alsa-devel,
Shengjiu Wang, perex
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1629875681-16373-1-git-send-email-shengjiu.wang@nxp.com>
On Wed, 25 Aug 2021 15:14:41 +0800, Shengjiu Wang wrote:
> Change dev_err to dev_err_probe for no need print error message
> when defer probe happens.
>
>
>
>
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: imx-rpmsg: change dev_err to dev_err_probe for -EPROBE_DEFER
commit: a8946f032eeace6eeb4e51e518275010e5528660
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
^ permalink raw reply
* Re: [PATCH v3] PCI: Move pci_dev_is/assign_added() to pci.h
From: Bjorn Helgaas @ 2021-08-25 19:04 UTC (permalink / raw)
To: Niklas Schnelle
Cc: linux-arch, linux-s390, linux-kernel, Paul Mackerras, linux-pci,
Bjorn Helgaas, linuxppc-dev
In-Reply-To: <7595397d6c32ae8745201085956696866cc400b6.camel@linux.ibm.com>
On Mon, Aug 23, 2021 at 12:53:39PM +0200, Niklas Schnelle wrote:
> On Fri, 2021-08-20 at 17:37 -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 20, 2021 at 05:01:45PM +0200, Niklas Schnelle wrote:
> > > The helper function pci_dev_is_added() from drivers/pci/pci.h is used in
> > > PCI arch code of both s390 and powerpc leading to awkward relative
> > > includes. Move it to the global include/linux/pci.h and get rid of these
> > > includes just for that one function.
> >
> > I agree the includes are awkward.
> >
> > But the arch code *using* pci_dev_is_added() seems awkward, too.
>
> See below for my interpretation why s390 has some driver like
> functionality in its arch code which isn't necessarily awkward.
>
> Independent from that I have found pci_dev_is_added() as the only way
> deal with the case that one might be looking at a struct pci_dev
> reference that has been removed via pci_stop_and_remove_bus_device() or
> has never been fully scanned. This is quite useful when handling error
> events which on s390 are part of the adapter event mechanism shared
> with channel I/O devices.
>
> > AFAICS, in powerpc, pci_dev_is_added() is only used by
> > pnv_pci_ioda_fixup_iov() and pseries_pci_fixup_iov_resources(). Those
> > are only called from pcibios_add_device(), which is only called from
> > pci_device_add().
> >
> > Is it even possible for pci_dev_is_added() to be true in that path?
If the pci_dev_is_added() in powerpc is unreachable, we can remove it
and at least reduce this to an s390-only problem.
> > s390 uses pci_dev_is_added() in recover_store()
>
> I'm actually looking into this as I'm working on an s390 implementation
> of the PCI recovery flow described in Documentation/PCI/pci-error-
> recovery.rst that would also call pci_dev_is_added() because when we
> get a platform notification of a PCI reset done by firmware it may be
> that the struct pci_dev is going away i.e. we still have a ref count
> but it is not added to the PCI bus anymore. And pci_dev_is_added() is
> the only way I've found to check for this state.
>
> > , but I don't know what
> > that is (looks like a sysfs file, but it's not documented) or why s390
> > is the only arch that does this.
>
> Good point about this not being documented, I'll look into adding docs.
>
> This is a sysfs attribute that basically removes the pci_dev and re-
> adds it. This has the complication that since the attribute sits at
> /sys/bus/pci/devices/<dev>/recover it deletes its own parent directory
> which requires extra caution and means concurrent accesses block on
> pci_lock_rescan_remove() instead of a kernfs lock.
> Long story short when concurrently triggering the attribute one thread
> proceeds into the pci_lock_rescan_remove() section and does the
> removal, while others would block on pci_lock_rescan_remove(). Now when
> the threads unblock the removal is done. In this case there is a new
> struct pci_dev found in the rescan but the previously blocked threads
> still have references to the old struct pci_dev which was removed and
> as far as I could tell can only be distinguished by checking
> pci_dev_is_added().
Is this locking issue different from concurrently writing to
/sys/.../remove on other architectures?
> > Maybe we should make powerpc and s390 less special?
>
> On s390, as I see it, the reason for this is that all of the PCI
> functionality is directly defined in the Architecture as special CPU
> instructions which are kind of hypercalls but also an ISA extension.
>
> These instructions range from the basic PCI memory accesses (no real
> MMIO) to enumeration of the devices and on to reporting of hot-plug and
> and resets/recovery events. Importantly we do not have any kind of
> direct access to a real or virtual PCI controller and the architecture
> has no concept of a comparable entity.
>
> So in my opinion while there is some of the functionality of a PCI
> controller in arch/s390/pci the cut off between controller
> functionality and arch support isn't clear at all and exposing PCI
> support as CPU instructions doesn't map well to the controller concept.
>
> That said, in principle I'm open to moving some of that into
> drivers/pci/controller/ if you think that would improve things and we
> can find a good argument what should go where. One possible cut off
> would be to have arch/s390/pci/ provide wrappers to the PCI
> instructions but move all their uses to e.g.
> drivers/pci/controller/s390/. This would of course be a major
> refactoring and none of that code would be useful on any other
> architecture but it would move a lot the accesses to PCI common code
> functionality out of the arch code.
Looks like hotplug is already in drivers/pci/hotplug/s390_pci_hpc.c.
Might be worth considering putting the other PCI core-ish code in
drivers/pci as well, though it doesn't feel urgent to me. Maybe a
good internship or mentoring project.
I'm not sure this juggling around is worth it basically to just clean
up the include path. The downside to me is exposing
pci_dev_is_added() to outside the PCI core, because I don't want
to encourage any other users.
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > ---
> > > Since v1 (and bad v2):
> > > - Fixed accidental removal of PCI_DPC_RECOVERED, PCI_DPC_RECOVERING
> > > defines and also move these to include/linux/pci.h
> > >
> > > arch/powerpc/platforms/powernv/pci-sriov.c | 3 ---
> > > arch/powerpc/platforms/pseries/setup.c | 1 -
> > > arch/s390/pci/pci_sysfs.c | 2 --
> > > drivers/pci/hotplug/acpiphp_glue.c | 1 -
> > > drivers/pci/pci.h | 15 ---------------
> > > include/linux/pci.h | 15 +++++++++++++++
> > > 6 files changed, 15 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> > > index 28aac933a439..2e0ca5451e85 100644
> > > --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> > > +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> > > @@ -9,9 +9,6 @@
> > >
> > > #include "pci.h"
> > >
> > > -/* for pci_dev_is_added() */
> > > -#include "../../../../drivers/pci/pci.h"
> > >
> .. snip ..
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox