* Re: [PATCH v2 09/10] tools/perf: Add perf tools support for extended register capability in powerpc
From: Michael Ellerman @ 2020-07-08 12:04 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-10-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> From: Anju T Sudhakar <anju@linux.vnet.ibm.com>
>
> Add extended regs to sample_reg_mask in the tool side to use
> with `-I?` option. Perf tools side uses extended mask to display
> the platform supported register names (with -I? option) to the user
> and also send this mask to the kernel to capture the extended registers
> in each sample. Hence decide the mask value based on the processor
> version.
>
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> [Decide extended mask at run time based on platform]
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Will need an ack from perf tools folks, who are not on Cc by the looks.
> diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> index f599064..485b1d5 100644
> --- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> +++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> @@ -48,6 +48,18 @@ enum perf_event_powerpc_regs {
> PERF_REG_POWERPC_DSISR,
> PERF_REG_POWERPC_SIER,
> PERF_REG_POWERPC_MMCRA,
> - PERF_REG_POWERPC_MAX,
> + /* Extended registers */
> + PERF_REG_POWERPC_MMCR0,
> + PERF_REG_POWERPC_MMCR1,
> + PERF_REG_POWERPC_MMCR2,
> + /* Max regs without the extended regs */
> + PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
I don't really understand this idea of a max that's not the max.
> };
> +
> +#define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1)
> +
> +/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300 */
> +#define PERF_REG_PMU_MASK_300 (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
> + - PERF_REG_PMU_MASK)
> +
> #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
> diff --git a/tools/perf/arch/powerpc/include/perf_regs.h b/tools/perf/arch/powerpc/include/perf_regs.h
> index e18a355..46ed00d 100644
> --- a/tools/perf/arch/powerpc/include/perf_regs.h
> +++ b/tools/perf/arch/powerpc/include/perf_regs.h
> @@ -64,7 +64,10 @@
> [PERF_REG_POWERPC_DAR] = "dar",
> [PERF_REG_POWERPC_DSISR] = "dsisr",
> [PERF_REG_POWERPC_SIER] = "sier",
> - [PERF_REG_POWERPC_MMCRA] = "mmcra"
> + [PERF_REG_POWERPC_MMCRA] = "mmcra",
> + [PERF_REG_POWERPC_MMCR0] = "mmcr0",
> + [PERF_REG_POWERPC_MMCR1] = "mmcr1",
> + [PERF_REG_POWERPC_MMCR2] = "mmcr2",
> };
>
> static inline const char *perf_reg_name(int id)
> diff --git a/tools/perf/arch/powerpc/util/perf_regs.c b/tools/perf/arch/powerpc/util/perf_regs.c
> index 0a52429..9179230 100644
> --- a/tools/perf/arch/powerpc/util/perf_regs.c
> +++ b/tools/perf/arch/powerpc/util/perf_regs.c
> @@ -6,9 +6,14 @@
>
> #include "../../../util/perf_regs.h"
> #include "../../../util/debug.h"
> +#include "../../../util/event.h"
> +#include "../../../util/header.h"
> +#include "../../../perf-sys.h"
>
> #include <linux/kernel.h>
>
> +#define PVR_POWER9 0x004E
> +
> const struct sample_reg sample_reg_masks[] = {
> SMPL_REG(r0, PERF_REG_POWERPC_R0),
> SMPL_REG(r1, PERF_REG_POWERPC_R1),
> @@ -55,6 +60,9 @@
> SMPL_REG(dsisr, PERF_REG_POWERPC_DSISR),
> SMPL_REG(sier, PERF_REG_POWERPC_SIER),
> SMPL_REG(mmcra, PERF_REG_POWERPC_MMCRA),
> + SMPL_REG(mmcr0, PERF_REG_POWERPC_MMCR0),
> + SMPL_REG(mmcr1, PERF_REG_POWERPC_MMCR1),
> + SMPL_REG(mmcr2, PERF_REG_POWERPC_MMCR2),
> SMPL_REG_END
> };
>
> @@ -163,3 +171,50 @@ int arch_sdt_arg_parse_op(char *old_op, char **new_op)
>
> return SDT_ARG_VALID;
> }
> +
> +uint64_t arch__intr_reg_mask(void)
> +{
> + struct perf_event_attr attr = {
> + .type = PERF_TYPE_HARDWARE,
> + .config = PERF_COUNT_HW_CPU_CYCLES,
> + .sample_type = PERF_SAMPLE_REGS_INTR,
> + .precise_ip = 1,
> + .disabled = 1,
> + .exclude_kernel = 1,
> + };
> + int fd, ret;
> + char buffer[64];
> + u32 version;
> + u64 extended_mask = 0;
> +
> + /* Get the PVR value to set the extended
> + * mask specific to platform
Comment format is wrong, and punctuation please.
> + */
> + get_cpuid(buffer, sizeof(buffer));
> + ret = sscanf(buffer, "%u,", &version);
This is powerpc specific code, why not just use mfspr(SPRN_PVR), rather
than redirecting via printf/sscanf.
> +
> + if (ret != 1) {
> + pr_debug("Failed to get the processor version, unable to output extended registers\n");
> + return PERF_REGS_MASK;
> + }
> +
> + if (version == PVR_POWER9)
> + extended_mask = PERF_REG_PMU_MASK_300;
> + else
> + return PERF_REGS_MASK;
> +
> + attr.sample_regs_intr = extended_mask;
> + attr.sample_period = 1;
> + event_attr_init(&attr);
> +
> + /*
> + * check if the pmu supports perf extended regs, before
> + * returning the register mask to sample.
> + */
> + fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> + if (fd != -1) {
> + close(fd);
> + return (extended_mask | PERF_REGS_MASK);
> + }
> + return PERF_REGS_MASK;
I think this would read a bit better like:
mask = PERF_REGS_MASK;
if (version == PVR_POWER9)
extended_mask = PERF_REG_PMU_MASK_300;
else
return mask;
attr.sample_regs_intr = extended_mask;
attr.sample_period = 1;
event_attr_init(&attr);
/*
* check if the pmu supports perf extended regs, before
* returning the register mask to sample.
*/
fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
if (fd != -1) {
close(fd);
mask |= extended_mask;
}
return mask;
cheers
^ permalink raw reply
* Re: [PATCH v2 10/10] powerpc/perf: Add extended regs support for power10 platform
From: Michael Ellerman @ 2020-07-08 12:04 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-11-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> Include capability flag `PERF_PMU_CAP_EXTENDED_REGS` for power10
> and expose MMCR3, SIER2, SIER3 registers as part of extended regs.
> Also introduce `PERF_REG_PMU_MASK_31` to define extended mask
> value at runtime for power10
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/uapi/asm/perf_regs.h | 6 ++++++
> arch/powerpc/perf/perf_regs.c | 10 +++++++++-
> arch/powerpc/perf/power10-pmu.c | 6 ++++++
> tools/arch/powerpc/include/uapi/asm/perf_regs.h | 6 ++++++
> tools/perf/arch/powerpc/include/perf_regs.h | 3 +++
> tools/perf/arch/powerpc/util/perf_regs.c | 6 ++++++
Please split into a kernel patch and a tools patch. And cc the tools people.
> 6 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
> index 485b1d5..020b51c 100644
> --- a/arch/powerpc/include/uapi/asm/perf_regs.h
> +++ b/arch/powerpc/include/uapi/asm/perf_regs.h
> @@ -52,6 +52,9 @@ enum perf_event_powerpc_regs {
> PERF_REG_POWERPC_MMCR0,
> PERF_REG_POWERPC_MMCR1,
> PERF_REG_POWERPC_MMCR2,
> + PERF_REG_POWERPC_MMCR3,
> + PERF_REG_POWERPC_SIER2,
> + PERF_REG_POWERPC_SIER3,
> /* Max regs without the extended regs */
> PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
> };
> @@ -62,4 +65,7 @@ enum perf_event_powerpc_regs {
> #define PERF_REG_PMU_MASK_300 (((1ULL << (PERF_REG_POWERPC_MMCR2 + 1)) - 1) \
> - PERF_REG_PMU_MASK)
>
> +/* PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31 */
> +#define PERF_REG_PMU_MASK_31 (((1ULL << (PERF_REG_POWERPC_SIER3 + 1)) - 1) \
> + - PERF_REG_PMU_MASK)
Wrapping that provides no benefit, just let it be long.
> #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
> index c8a7e8c..c969935 100644
> --- a/arch/powerpc/perf/perf_regs.c
> +++ b/arch/powerpc/perf/perf_regs.c
> @@ -81,6 +81,12 @@ static u64 get_ext_regs_value(int idx)
> return mfspr(SPRN_MMCR1);
> case PERF_REG_POWERPC_MMCR2:
> return mfspr(SPRN_MMCR2);
> + case PERF_REG_POWERPC_MMCR3:
> + return mfspr(SPRN_MMCR3);
> + case PERF_REG_POWERPC_SIER2:
> + return mfspr(SPRN_SIER2);
> + case PERF_REG_POWERPC_SIER3:
> + return mfspr(SPRN_SIER3);
Indentation is wrong.
> default: return 0;
> }
> }
> @@ -89,7 +95,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> {
> u64 PERF_REG_EXTENDED_MAX;
>
> - if (cpu_has_feature(CPU_FTR_ARCH_300))
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_SIER3 + 1;
There's no way to know if that's correct other than going back to the
header to look at the list of values.
So instead you should define it in the header, next to the other values,
with a meaningful name, like PERF_REG_MAX_ISA_31 or something.
> + else if (cpu_has_feature(CPU_FTR_ARCH_300))
> PERF_REG_EXTENDED_MAX = PERF_REG_POWERPC_MMCR2 + 1;
Same.
> if (idx == PERF_REG_POWERPC_SIER &&
> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
> index 07fb919..51082d6 100644
> --- a/arch/powerpc/perf/power10-pmu.c
> +++ b/arch/powerpc/perf/power10-pmu.c
> @@ -86,6 +86,8 @@
> #define POWER10_MMCRA_IFM3 0x00000000C0000000UL
> #define POWER10_MMCRA_BHRB_MASK 0x00000000C0000000UL
>
> +extern u64 mask_var;
Why is it extern? Also not a good name for a global.
Hang on, it's not even used? Is there some macro magic somewhere?
> /* Table of alternatives, sorted by column 0 */
> static const unsigned int power10_event_alternatives[][MAX_ALT] = {
> { PM_RUN_CYC_ALT, PM_RUN_CYC },
> @@ -397,6 +399,7 @@ static void power10_config_bhrb(u64 pmu_bhrb_filter)
> .cache_events = &power10_cache_events,
> .attr_groups = power10_pmu_attr_groups,
> .bhrb_nr = 32,
> + .capabilities = PERF_PMU_CAP_EXTENDED_REGS,
> };
>
> int init_power10_pmu(void)
> @@ -408,6 +411,9 @@ int init_power10_pmu(void)
> strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power10"))
> return -ENODEV;
>
> + /* Set the PERF_REG_EXTENDED_MASK here */
> + mask_var = PERF_REG_PMU_MASK_31;
> +
> rc = register_power_pmu(&power10_pmu);
> if (rc)
> return rc;
cheers
^ permalink raw reply
* Re: [PATCH v2 07/10] powerpc/perf: support BHRB disable bit and new filtering modes
From: Michael Ellerman @ 2020-07-08 11:42 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-8-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> PowerISA v3.1 has few updates for the Branch History Rolling Buffer(BHRB).
^
a
> First is the addition of BHRB disable bit and second new filtering
^
is
> modes for BHRB.
>
> BHRB disable is controlled via Monitor Mode Control Register A (MMCRA)
> bit 26, namely "BHRB Recording Disable (BHRBRD)". This field controls
Most people call that bit 37.
> whether BHRB entries are written when BHRB recording is enabled by other
> bits. Patch implements support for this BHRB disable bit.
^
This
> Secondly PowerISA v3.1 introduce filtering support for
.. that should be in a separate patch please.
> PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter support
^
This
> for "ind_call" and "cond" in power10_bhrb_filter_map().
>
> 'commit bb19af816025 ("powerpc/perf: Prevent kernel address leak to userspace via BHRB buffer")'
That doesn't need single quotes, and should be wrapped at 72 columns
like the rest of the text.
> added a check in bhrb_read() to filter the kernel address from BHRB buffer. Patch here modified
> it to avoid that check for PowerISA v3.1 based processors, since PowerISA v3.1 allows
> only MSR[PR]=1 address to be written to BHRB buffer.
And that should be a separate patch again please.
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> arch/powerpc/perf/core-book3s.c | 27 +++++++++++++++++++++------
> arch/powerpc/perf/isa207-common.c | 13 +++++++++++++
> arch/powerpc/perf/power10-pmu.c | 13 +++++++++++--
> arch/powerpc/platforms/powernv/idle.c | 14 ++++++++++++++
> 4 files changed, 59 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index fad5159..9709606 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -466,9 +466,13 @@ static void power_pmu_bhrb_read(struct perf_event *event, struct cpu_hw_events *
> * addresses at this point. Check the privileges before
> * exporting it to userspace (avoid exposure of regions
> * where we could have speculative execution)
> + * Incase of ISA 310, BHRB will capture only user-space
^
In case of ISA v3.1,
> + * address,hence include a check before filtering code
^ ^
addresses, hence .
> */
> - if (is_kernel_addr(addr) && perf_allow_kernel(&event->attr) != 0)
> - continue;
> + if (!(ppmu->flags & PPMU_ARCH_310S))
> + if (is_kernel_addr(addr) &&
> + perf_allow_kernel(&event->attr) != 0)
> + continue;
The indentation is weird. You should just check all three conditions
with &&.
>
> /* Branches are read most recent first (ie. mfbhrb 0 is
> * the most recent branch).
> @@ -1212,7 +1216,7 @@ static void write_mmcr0(struct cpu_hw_events *cpuhw, unsigned long mmcr0)
> static void power_pmu_disable(struct pmu *pmu)
> {
> struct cpu_hw_events *cpuhw;
> - unsigned long flags, mmcr0, val;
> + unsigned long flags, mmcr0, val, mmcra = 0;
You initialise it below.
> if (!ppmu)
> return;
> @@ -1245,12 +1249,23 @@ static void power_pmu_disable(struct pmu *pmu)
> mb();
> isync();
>
> + val = mmcra = cpuhw->mmcr[2];
> +
For mmcr0 (above), val is the variable we mutate and mmcr0 is the
original value. But here you've done the reverse, which is confusing.
> /*
> * Disable instruction sampling if it was enabled
> */
> - if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE) {
> - mtspr(SPRN_MMCRA,
> - cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
> + if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE)
> + mmcra = cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE;
You just loaded cpuhw->mmcr[2] into mmcra, use it rather than referring
back to cpuhw->mmcr[2] over and over.
> +
> + /* Disable BHRB via mmcra [:26] for p10 if needed */
> + if (!(cpuhw->mmcr[2] & MMCRA_BHRB_DISABLE))
You don't need to check that it's clear AFAICS. Just always set disable
and the check against val below will catch the nop case.
> + mmcra |= MMCRA_BHRB_DISABLE;
> +
> + /* Write SPRN_MMCRA if mmcra has either disabled
Comment format is wrong.
> + * instruction sampling or BHRB
Full stop please.
> + */
> + if (val != mmcra) {
> + mtspr(SPRN_MMCRA, mmcra);
> mb();
> isync();
> }
> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
> index 7d4839e..463d925 100644
> --- a/arch/powerpc/perf/isa207-common.c
> +++ b/arch/powerpc/perf/isa207-common.c
> @@ -404,6 +404,12 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
>
> mmcra = mmcr1 = mmcr2 = mmcr3 = 0;
>
> + /* Disable bhrb unless explicitly requested
> + * by setting MMCRA [:26] bit.
> + */
Comment format again.
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + mmcra |= MMCRA_BHRB_DISABLE;
Here we do a feature check before setting MMCRA_BHRB_DISABLE, but you
didn't above?
> +
> /* Second pass: assign PMCs, set all MMCR1 fields */
> for (i = 0; i < n_ev; ++i) {
> pmc = (event[i] >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK;
> @@ -475,10 +481,17 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
> }
>
> if (event[i] & EVENT_WANTS_BHRB) {
> + /* set MMCRA[:26] to 0 for Power10 to enable BHRB */
"set MMCRA[:26] to 0" == "clear MMCRA[:26]"
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + mmcra &= ~MMCRA_BHRB_DISABLE;
Newline please.
> val = (event[i] >> EVENT_IFM_SHIFT) & EVENT_IFM_MASK;
> mmcra |= val << MMCRA_IFM_SHIFT;
> }
>
> + /* set MMCRA[:26] to 0 if there is user request for BHRB */
> + if (cpu_has_feature(CPU_FTR_ARCH_31) && has_branch_stack(pevents[i]))
> + mmcra &= ~MMCRA_BHRB_DISABLE;
> +
I think it would be cleaner if you did a single test, eg:
if (cpu_has_feature(CPU_FTR_ARCH_31) &&
(has_branch_stack(pevents[i]) || (event[i] & EVENT_WANTS_BHRB)))
mmcra &= ~MMCRA_BHRB_DISABLE;
> if (pevents[i]->attr.exclude_user)
> mmcr2 |= MMCR2_FCP(pmc);
>
> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
> index d64d69d..07fb919 100644
> --- a/arch/powerpc/perf/power10-pmu.c
> +++ b/arch/powerpc/perf/power10-pmu.c
> @@ -82,6 +82,8 @@
>
> /* MMCRA IFM bits - POWER10 */
> #define POWER10_MMCRA_IFM1 0x0000000040000000UL
> +#define POWER10_MMCRA_IFM2 0x0000000080000000UL
> +#define POWER10_MMCRA_IFM3 0x00000000C0000000UL
> #define POWER10_MMCRA_BHRB_MASK 0x00000000C0000000UL
>
> /* Table of alternatives, sorted by column 0 */
> @@ -233,8 +235,15 @@ static u64 power10_bhrb_filter_map(u64 branch_sample_type)
> if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
> return -1;
>
> - if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
> - return -1;
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
> + pmu_bhrb_filter |= POWER10_MMCRA_IFM2;
> + return pmu_bhrb_filter;
> + }
> +
> + if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
> + pmu_bhrb_filter |= POWER10_MMCRA_IFM3;
> + return pmu_bhrb_filter;
> + }
>
> if (branch_sample_type & PERF_SAMPLE_BRANCH_CALL)
> return -1;
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 2dd4673..7db99c7 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -611,6 +611,7 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> unsigned long srr1;
> unsigned long pls;
> unsigned long mmcr0 = 0;
> + unsigned long mmcra_bhrb = 0;
> struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
> bool sprs_saved = false;
>
> @@ -657,6 +658,15 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> */
> mmcr0 = mfspr(SPRN_MMCR0);
> }
> +
> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> + /* POWER10 uses MMCRA[:26] as BHRB disable bit
Comment format.
> + * to disable BHRB logic when not used. Hence Save and
> + * restore MMCRA after a state-loss idle.
> + */
> + mmcra_bhrb = mfspr(SPRN_MMCRA);
> + }
It's the whole mmcra it should be called mmcra?
> +
> if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
> sprs.lpcr = mfspr(SPRN_LPCR);
> sprs.hfscr = mfspr(SPRN_HFSCR);
> @@ -721,6 +731,10 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> mtspr(SPRN_MMCR0, mmcr0);
> }
>
> + /* Reload MMCRA to restore BHRB disable bit for POWER10 */
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + mtspr(SPRN_MMCRA, mmcra_bhrb);
> +
> /*
> * DD2.2 and earlier need to set then clear bit 60 in MMCRA
> * to ensure the PMU starts running.
cheers
^ permalink raw reply
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: rework secure mem slot dropping
From: Bharata B Rao @ 2020-07-08 11:25 UTC (permalink / raw)
To: Laurent Dufour
Cc: Ram Pai, linux-kernel, kvm-ppc, paulus, sathnaga, sukadev,
linuxppc-dev, bauerman
In-Reply-To: <20200703155914.40262-3-ldufour@linux.ibm.com>
On Fri, Jul 03, 2020 at 05:59:14PM +0200, Laurent Dufour wrote:
> When a secure memslot is dropped, all the pages backed in the secure device
> (aka really backed by secure memory by the Ultravisor) should be paged out
> to a normal page. Previously, this was achieved by triggering the page
> fault mechanism which is calling kvmppc_svm_page_out() on each pages.
>
> This can't work when hot unplugging a memory slot because the memory slot
> is flagged as invalid and gfn_to_pfn() is then not trying to access the
> page, so the page fault mechanism is not triggered.
>
> Since the final goal is to make a call to kvmppc_svm_page_out() it seems
> simpler to directly calling it instead of triggering such a mechanism. This
> way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a
> memslot.
Yes, this appears much simpler.
>
> Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock,
> the call to __kvmppc_svm_page_out() is made.
> As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the
> VMA is fetched in a lazy way, to not trigger find_vma() all the time. In
> addition, the mmap_sem is help in read mode during that time, not in write
> mode since the virual memory layout is not impacted, and
> kvm->arch.uvmem_lock prevents concurrent operation on the secure device.
>
> Cc: Ram Pai <linuxram@us.ibm.com>
> Cc: Bharata B Rao <bharata@linux.ibm.com>
> Cc: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 852cc9ae6a0b..479ddf16d18c 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma,
> * fault on them, do fault time migration to replace the device PTEs in
> * QEMU page table with normal PTEs from newly allocated pages.
> */
> -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
> +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot,
> struct kvm *kvm, bool skip_page_out)
> {
> int i;
> struct kvmppc_uvmem_page_pvt *pvt;
> - unsigned long pfn, uvmem_pfn;
> - unsigned long gfn = free->base_gfn;
> + struct page *uvmem_page;
> + struct vm_area_struct *vma = NULL;
> + unsigned long uvmem_pfn, gfn;
> + unsigned long addr, end;
> +
> + down_read(&kvm->mm->mmap_sem);
You should be using mmap_read_lock(kvm->mm) with recent kernels.
> +
> + addr = slot->userspace_addr;
> + end = addr + (slot->npages * PAGE_SIZE);
>
> - for (i = free->npages; i; --i, ++gfn) {
> - struct page *uvmem_page;
> + gfn = slot->base_gfn;
> + for (i = slot->npages; i; --i, ++gfn, addr += PAGE_SIZE) {
> +
> + /* Fetch the VMA if addr is not in the latest fetched one */
> + if (!vma || (addr < vma->vm_start || addr >= vma->vm_end)) {
> + vma = find_vma_intersection(kvm->mm, addr, end);
> + if (!vma ||
> + vma->vm_start > addr || vma->vm_end < end) {
> + pr_err("Can't find VMA for gfn:0x%lx\n", gfn);
> + break;
> + }
> + }
The first find_vma_intersection() was called for the range spanning the
entire memslot, but you have code to check if vma remains valid for the
new addr in each iteration. Guess you wanted to get vma for one page at
a time and use it for subsequent pages until it covers the range?
Regards,
Bharata.
^ permalink raw reply
* Re: [PATCH v2 07/10] powerpc/perf: support BHRB disable bit and new filtering modes
From: Athira Rajeev @ 2020-07-08 7:41 UTC (permalink / raw)
To: Michael Neuling; +Cc: Vaidyanathan Srinivasan, maddy, linuxppc-dev, ego
In-Reply-To: <0cf26e42a3b190d5ea69d1ba61ae71bcaeee1973.camel@neuling.org>
[-- Attachment #1: Type: text/plain, Size: 8382 bytes --]
> On 07-Jul-2020, at 12:47 PM, Michael Neuling <mikey@neuling.org> wrote:
>
> On Wed, 2020-07-01 at 05:20 -0400, Athira Rajeev wrote:
>> PowerISA v3.1 has few updates for the Branch History Rolling Buffer(BHRB).
>> First is the addition of BHRB disable bit and second new filtering
>> modes for BHRB.
>>
>> BHRB disable is controlled via Monitor Mode Control Register A (MMCRA)
>> bit 26, namely "BHRB Recording Disable (BHRBRD)". This field controls
>> whether BHRB entries are written when BHRB recording is enabled by other
>> bits. Patch implements support for this BHRB disable bit.
>
> Probably good to note here that this is backwards compatible. So if you have a
> kernel that doesn't know about this bit, it'll clear it and hence you still get
> BHRB.
>
> You should also note why you'd want to do disable this (ie. the core will run
> faster).
>
Sure Mikey, will add these information in commit message
Thanks
Athira
>> Secondly PowerISA v3.1 introduce filtering support for
>> PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter support
>> for "ind_call" and "cond" in power10_bhrb_filter_map().
>>
>> 'commit bb19af816025 ("powerpc/perf: Prevent kernel address leak to userspace
>> via BHRB buffer")'
>> added a check in bhrb_read() to filter the kernel address from BHRB buffer.
>> Patch here modified
>> it to avoid that check for PowerISA v3.1 based processors, since PowerISA v3.1
>> allows
>> only MSR[PR]=1 address to be written to BHRB buffer.
>>
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/perf/core-book3s.c | 27 +++++++++++++++++++++------
>> arch/powerpc/perf/isa207-common.c | 13 +++++++++++++
>> arch/powerpc/perf/power10-pmu.c | 13 +++++++++++--
>> arch/powerpc/platforms/powernv/idle.c | 14 ++++++++++++++
>
> This touches the idle code so we should get those guys on CC (adding Vaidy and
> Ego).
>
>> 4 files changed, 59 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index fad5159..9709606 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -466,9 +466,13 @@ static void power_pmu_bhrb_read(struct perf_event *event,
>> struct cpu_hw_events *
>> * addresses at this point. Check the privileges before
>> * exporting it to userspace (avoid exposure of regions
>> * where we could have speculative execution)
>> + * Incase of ISA 310, BHRB will capture only user-space
>> + * address,hence include a check before filtering code
>> */
>> - if (is_kernel_addr(addr) && perf_allow_kernel(&event-
>>> attr) != 0)
>> - continue;
>> + if (!(ppmu->flags & PPMU_ARCH_310S))
>> + if (is_kernel_addr(addr) &&
>> + perf_allow_kernel(&event->attr) != 0)
>> + continue;
>>
>> /* Branches are read most recent first (ie. mfbhrb 0 is
>> * the most recent branch).
>> @@ -1212,7 +1216,7 @@ static void write_mmcr0(struct cpu_hw_events *cpuhw,
>> unsigned long mmcr0)
>> static void power_pmu_disable(struct pmu *pmu)
>> {
>> struct cpu_hw_events *cpuhw;
>> - unsigned long flags, mmcr0, val;
>> + unsigned long flags, mmcr0, val, mmcra = 0;
>>
>> if (!ppmu)
>> return;
>> @@ -1245,12 +1249,23 @@ static void power_pmu_disable(struct pmu *pmu)
>> mb();
>> isync();
>>
>> + val = mmcra = cpuhw->mmcr[2];
>> +
>> /*
>> * Disable instruction sampling if it was enabled
>> */
>> - if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE) {
>> - mtspr(SPRN_MMCRA,
>> - cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
>> + if (cpuhw->mmcr[2] & MMCRA_SAMPLE_ENABLE)
>> + mmcra = cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE;
>> +
>> + /* Disable BHRB via mmcra [:26] for p10 if needed */
>> + if (!(cpuhw->mmcr[2] & MMCRA_BHRB_DISABLE))
>> + mmcra |= MMCRA_BHRB_DISABLE;
>> +
>> + /* Write SPRN_MMCRA if mmcra has either disabled
>> + * instruction sampling or BHRB
>> + */
>> + if (val != mmcra) {
>> + mtspr(SPRN_MMCRA, mmcra);
>> mb();
>> isync();
>> }
>> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-
>> common.c
>> index 7d4839e..463d925 100644
>> --- a/arch/powerpc/perf/isa207-common.c
>> +++ b/arch/powerpc/perf/isa207-common.c
>> @@ -404,6 +404,12 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
>>
>> mmcra = mmcr1 = mmcr2 = mmcr3 = 0;
>>
>> + /* Disable bhrb unless explicitly requested
>> + * by setting MMCRA [:26] bit.
>> + */
>> + if (cpu_has_feature(CPU_FTR_ARCH_31))
>> + mmcra |= MMCRA_BHRB_DISABLE;
>> +
>> /* Second pass: assign PMCs, set all MMCR1 fields */
>> for (i = 0; i < n_ev; ++i) {
>> pmc = (event[i] >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK;
>> @@ -475,10 +481,17 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
>> }
>>
>> if (event[i] & EVENT_WANTS_BHRB) {
>> + /* set MMCRA[:26] to 0 for Power10 to enable BHRB */
>> + if (cpu_has_feature(CPU_FTR_ARCH_31))
>> + mmcra &= ~MMCRA_BHRB_DISABLE;
>> val = (event[i] >> EVENT_IFM_SHIFT) & EVENT_IFM_MASK;
>> mmcra |= val << MMCRA_IFM_SHIFT;
>> }
>>
>> + /* set MMCRA[:26] to 0 if there is user request for BHRB */
>> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
>> has_branch_stack(pevents[i]))
>> + mmcra &= ~MMCRA_BHRB_DISABLE;
>> +
>> if (pevents[i]->attr.exclude_user)
>> mmcr2 |= MMCR2_FCP(pmc);
>>
>> diff --git a/arch/powerpc/perf/power10-pmu.c b/arch/powerpc/perf/power10-pmu.c
>> index d64d69d..07fb919 100644
>> --- a/arch/powerpc/perf/power10-pmu.c
>> +++ b/arch/powerpc/perf/power10-pmu.c
>> @@ -82,6 +82,8 @@
>>
>> /* MMCRA IFM bits - POWER10 */
>> #define POWER10_MMCRA_IFM1 0x0000000040000000UL
>> +#define POWER10_MMCRA_IFM2 0x0000000080000000UL
>> +#define POWER10_MMCRA_IFM3 0x00000000C0000000UL
>> #define POWER10_MMCRA_BHRB_MASK 0x00000000C0000000UL
>>
>> /* Table of alternatives, sorted by column 0 */
>> @@ -233,8 +235,15 @@ static u64 power10_bhrb_filter_map(u64
>> branch_sample_type)
>> if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
>> return -1;
>>
>> - if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
>> - return -1;
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL) {
>> + pmu_bhrb_filter |= POWER10_MMCRA_IFM2;
>> + return pmu_bhrb_filter;
>> + }
>> +
>> + if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
>> + pmu_bhrb_filter |= POWER10_MMCRA_IFM3;
>> + return pmu_bhrb_filter;
>> + }
>>
>> if (branch_sample_type & PERF_SAMPLE_BRANCH_CALL)
>> return -1;
>> diff --git a/arch/powerpc/platforms/powernv/idle.c
>> b/arch/powerpc/platforms/powernv/idle.c
>> index 2dd4673..7db99c7 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -611,6 +611,7 @@ static unsigned long power9_idle_stop(unsigned long psscr,
>> bool mmu_on)
>> unsigned long srr1;
>> unsigned long pls;
>> unsigned long mmcr0 = 0;
>> + unsigned long mmcra_bhrb = 0;
>> struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
>> bool sprs_saved = false;
>>
>> @@ -657,6 +658,15 @@ static unsigned long power9_idle_stop(unsigned long
>> psscr, bool mmu_on)
>> */
>> mmcr0 = mfspr(SPRN_MMCR0);
>> }
>> +
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + /* POWER10 uses MMCRA[:26] as BHRB disable bit
>> + * to disable BHRB logic when not used. Hence Save and
>> + * restore MMCRA after a state-loss idle.
>> + */
>> + mmcra_bhrb = mfspr(SPRN_MMCRA);
>
>
> Why is the bhrb bit of mmcra special here?
This to save/restore BHRB disable bit in state-loss idle state to make sure
we keep BHRB disabled if it was not enabled on request at runtime.
>
>> + }
>> +
>> if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
>> sprs.lpcr = mfspr(SPRN_LPCR);
>> sprs.hfscr = mfspr(SPRN_HFSCR);
>> @@ -721,6 +731,10 @@ static unsigned long power9_idle_stop(unsigned long
>> psscr, bool mmu_on)
>> mtspr(SPRN_MMCR0, mmcr0);
>> }
>>
>> + /* Reload MMCRA to restore BHRB disable bit for POWER10 */
>> + if (cpu_has_feature(CPU_FTR_ARCH_31))
>> + mtspr(SPRN_MMCRA, mmcra_bhrb);
>> +
>> /*
>> * DD2.2 and earlier need to set then clear bit 60 in MMCRA
>> * to ensure the PMU starts running.
[-- Attachment #2: Type: text/html, Size: 37042 bytes --]
^ permalink raw reply
* [PATCH] arch: powerpc: Remove unnecessary cast in kfree()
From: Xu Wang @ 2020-07-08 7:22 UTC (permalink / raw)
To: mpe, benh, paulus, vulab, linuxppc-dev; +Cc: linux-kernel
Remove unnecassary casts in the argument to kfree.
Signed-off-by: Xu Wang <vulab@iscas.ac.cn>
---
arch/powerpc/platforms/pseries/dlpar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 16e86ba8aa20..1f3d26806295 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -379,7 +379,7 @@ static void pseries_hp_work_fn(struct work_struct *work)
handle_dlpar_errorlog(hp_work->errlog);
kfree(hp_work->errlog);
- kfree((void *)work);
+ kfree(work);
}
void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 04/10] powerpc/perf: Add power10_feat to dt_cpu_ftrs
From: Michael Ellerman @ 2020-07-08 11:15 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-5-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> From: Madhavan Srinivasan <maddy@linux.ibm.com>
>
> Add power10 feature function to dt_cpu_ftrs.c along
> with a power10 specific init() to initialize pmu sprs.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
> arch/powerpc/include/asm/reg.h | 3 +++
> arch/powerpc/kernel/cpu_setup_power.S | 7 +++++++
> arch/powerpc/kernel/dt_cpu_ftrs.c | 26 ++++++++++++++++++++++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 21a1b2d..900ada1 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1068,6 +1068,9 @@
> #define MMCR0_PMC2_LOADMISSTIME 0x5
> #endif
>
> +/* BHRB disable bit for PowerISA v3.10 */
> +#define MMCRA_BHRB_DISABLE 0x0000002000000000
> +
> /*
> * SPRG usage:
> *
> diff --git a/arch/powerpc/kernel/cpu_setup_power.S b/arch/powerpc/kernel/cpu_setup_power.S
> index efdcfa7..e8b3370c 100644
> --- a/arch/powerpc/kernel/cpu_setup_power.S
> +++ b/arch/powerpc/kernel/cpu_setup_power.S
> @@ -233,3 +233,10 @@ __init_PMU_ISA207:
> li r5,0
> mtspr SPRN_MMCRS,r5
> blr
> +
> +__init_PMU_ISA31:
> + li r5,0
> + mtspr SPRN_MMCR3,r5
> + LOAD_REG_IMMEDIATE(r5, MMCRA_BHRB_DISABLE)
> + mtspr SPRN_MMCRA,r5
> + blr
This doesn't seem like it belongs in this patch. It's not called?
cheers
^ permalink raw reply
* Re: [PATCH v2 03/10] powerpc/xmon: Add PowerISA v3.1 PMU SPRs
From: Michael Ellerman @ 2020-07-08 11:04 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-4-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> From: Madhavan Srinivasan <maddy@linux.ibm.com>
>
> PowerISA v3.1 added three new perfromance
> monitoring unit (PMU) speical purpose register (SPR).
> They are Monitor Mode Control Register 3 (MMCR3),
> Sampled Instruction Event Register 2 (SIER2),
> Sampled Instruction Event Register 3 (SIER3).
>
> Patch here adds a new dump function dump_310_sprs
> to print these SPR values.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> ---
> arch/powerpc/xmon/xmon.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 7efe4bc..8917fe8 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -2022,6 +2022,20 @@ static void dump_300_sprs(void)
> #endif
> }
>
> +static void dump_310_sprs(void)
> +{
> +#ifdef CONFIG_PPC64
> + if (!cpu_has_feature(CPU_FTR_ARCH_31))
> + return;
> +
> + printf("mmcr3 = %.16lx\n",
> + mfspr(SPRN_MMCR3));
> +
> + printf("sier2 = %.16lx sier3 = %.16lx\n",
> + mfspr(SPRN_SIER2), mfspr(SPRN_SIER3));
Why not all on one line like many of the others?
cheers
^ permalink raw reply
* Re: [PATCH v2 01/10] powerpc/perf: Add support for ISA3.1 PMU SPRs
From: Michael Ellerman @ 2020-07-08 11:02 UTC (permalink / raw)
To: Athira Rajeev; +Cc: mikey, maddy, linuxppc-dev
In-Reply-To: <1593595262-1433-2-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
...
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index cd6a742..5c64bd3 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -39,10 +39,10 @@ struct cpu_hw_events {
> unsigned int flags[MAX_HWEVENTS];
> /*
> * The order of the MMCR array is:
> - * - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
> + * - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
> * - 32-bit, MMCR0, MMCR1, MMCR2
> */
> - unsigned long mmcr[4];
> + unsigned long mmcr[5];
> struct perf_event *limited_counter[MAX_LIMITED_HWCOUNTERS];
> u8 limited_hwidx[MAX_LIMITED_HWCOUNTERS];
> u64 alternatives[MAX_HWEVENTS][MAX_EVENT_ALTERNATIVES];
...
> @@ -1310,6 +1326,10 @@ static void power_pmu_enable(struct pmu *pmu)
> if (!cpuhw->n_added) {
> mtspr(SPRN_MMCRA, cpuhw->mmcr[2] & ~MMCRA_SAMPLE_ENABLE);
> mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
> +#ifdef CONFIG_PPC64
> + if (ppmu->flags & PPMU_ARCH_310S)
> + mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
> +#endif /* CONFIG_PPC64 */
> goto out_enable;
> }
>
> @@ -1353,6 +1373,11 @@ static void power_pmu_enable(struct pmu *pmu)
> if (ppmu->flags & PPMU_ARCH_207S)
> mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
>
> +#ifdef CONFIG_PPC64
> + if (ppmu->flags & PPMU_ARCH_310S)
> + mtspr(SPRN_MMCR3, cpuhw->mmcr[4]);
> +#endif /* CONFIG_PPC64 */
I don't think you need the #ifdef CONFIG_PPC64?
cheers
^ permalink raw reply
* Re: [PATCH v2 06/10] powerpc/perf: power10 Performance Monitoring support
From: Athira Rajeev @ 2020-07-08 10:56 UTC (permalink / raw)
To: Michael Neuling; +Cc: maddy, linuxppc-dev
In-Reply-To: <babc98c50100bb2cc925a6518bdb885909f0c473.camel@neuling.org>
> On 07-Jul-2020, at 12:20 PM, Michael Neuling <mikey@neuling.org> wrote:
>
>
>> @@ -480,6 +520,7 @@ int isa207_compute_mmcr(u64 event[], int n_ev,
>> mmcr[1] = mmcr1;
>> mmcr[2] = mmcra;
>> mmcr[3] = mmcr2;
>> + mmcr[4] = mmcr3;
>
> This is fragile like the kvm vcpu case I commented on before but it gets passed
> in via a function parameter?! Can you create a struct to store these in rather
> than this odd ball numbering?
Mikey,
Yes, it gets passed as cpuhw->mmcr array
I will check on these cleanup changes for the kvm vcpu case as well as cpu_hw_events mmcr array
Thanks
Athira
>
> The cleanup should start in patch 1/10 here:
>
> /*
> * The order of the MMCR array is:
> - * - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2
> + * - 64-bit, MMCR0, MMCR1, MMCRA, MMCR2, MMCR3
> * - 32-bit, MMCR0, MMCR1, MMCR2
> */
> - unsigned long mmcr[4];
> + unsigned long mmcr[5];
>
>
>
> mikey
^ permalink raw reply
* Re: [PATCH] selftests/powerpc: Purge extra count_pmc() calls of ebb selftests
From: Sachin Sant @ 2020-07-08 10:38 UTC (permalink / raw)
To: Desnes A. Nunes do Rosario; +Cc: shuah, linuxppc-dev
In-Reply-To: <20200626164737.21943-1-desnesn@linux.ibm.com>
> On 26-Jun-2020, at 10:17 PM, Desnes A. Nunes do Rosario <desnesn@linux.ibm.com> wrote:
>
> An extra count on ebb_state.stats.pmc_count[PMC_INDEX(pmc)] is being per-
> formed when count_pmc() is used to reset PMCs on a few selftests. This
> extra pmc_count can occasionally invalidate results, such as the ones from
> cycles_test shown hereafter. The ebb_check_count() failed with an above
> the upper limit error due to the extra value on ebb_state.stats.pmc_count.
>
> Furthermore, this extra count is also indicated by extra PMC1 trace_log on
> the output of the cycle test (as well as on pmc56_overflow_test):
>
> ==========
> ...
> [21]: counter = 8
> [22]: register SPRN_MMCR0 = 0x0000000080000080
> [23]: register SPRN_PMC1 = 0x0000000080000004
> [24]: counter = 9
> [25]: register SPRN_MMCR0 = 0x0000000080000080
> [26]: register SPRN_PMC1 = 0x0000000080000004
> [27]: counter = 10
> [28]: register SPRN_MMCR0 = 0x0000000080000080
> [29]: register SPRN_PMC1 = 0x0000000080000004
>>> [30]: register SPRN_PMC1 = 0x000000004000051e
> PMC1 count (0x280000546) above upper limit 0x2800003e8 (+0x15e)
> [FAIL] Test FAILED on line 52
> failure: cycles
> ==========
>
> Signed-off-by: Desnes A. Nunes do Rosario <desnesn@linux.ibm.com>
> ---
I too have run into similar failure with cycles_test. I will add that the failure
is inconsistent. I have run into this issue 1 out of 25 times. The failure always
happen at first instance. Subsequent tries work correctly.
With this patch applied the test completes successfully 25 out of 25 times.
# ./cycles_test
test: cycles
…..
…..
[25]: register SPRN_MMCR0 = 0x0000000080000080
[26]: register SPRN_PMC1 = 0x0000000080000004
[27]: counter = 10
[28]: register SPRN_MMCR0 = 0x0000000080000080
[29]: register SPRN_PMC1 = 0x0000000080000004
[30]: register SPRN_PMC1 = 0x000000004000048f
PMC1 count (0x2800004b7) above upper limit 0x2800003e8 (+0xcf)
[FAIL] Test FAILED on line 52
failure: cycles
With the patch
# ./cycles_test
test: cycles
…..
…..
[25]: register SPRN_MMCR0 = 0x0000000080000080
[26]: register SPRN_PMC1 = 0x0000000080000004
[27]: counter = 10
[28]: register SPRN_MMCR0 = 0x0000000080000080
[29]: register SPRN_PMC1 = 0x0000000080000004
PMC1 count (0x280000028) is between 0x27ffffc18 and 0x2800003e8 delta +0x410/-0x3c0
success: cycles
#
FWIW Tested-by : Sachin Sant <sachinp@linux.vnet.ibm.com>
Thanks
-Sachin
^ permalink raw reply
* Re: [PATCH v4 2/2] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show cpumask
From: Madhavan Srinivasan @ 2020-07-08 9:34 UTC (permalink / raw)
To: Kajol Jain, linuxppc-dev, mpe; +Cc: nathanl, ego, suka, maddy, anju
In-Reply-To: <20200708085955.655686-3-kjain@linux.ibm.com>
On 7/8/20 2:29 PM, Kajol Jain wrote:
> Patch here adds a cpumask attr to hv_24x7 pmu along with ABI documentation.
>
> Primary use to expose the cpumask is for the perf tool which has the
> capability to parse the driver sysfs folder and understand the
> cpumask file. Having cpumask file will reduce the number of perf command
> line parameters (will avoid "-C" option in the perf tool
> command line). It can also notify the user which is
> the current cpu used to retrieve the counter data.
>
> command:# cat /sys/devices/hv_24x7/cpumask
> 0
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> .../sysfs-bus-event_source-devices-hv_24x7 | 7 ++++
> arch/powerpc/perf/hv-24x7.c | 33 ++++++++++++++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7 b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
> index e8698afcd952..ee89d0e94602 100644
> --- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
> +++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
> @@ -43,6 +43,13 @@ Description: read only
> This sysfs interface exposes the number of cores per chip
> present in the system.
>
> +What: /sys/devices/hv_24x7/cpumask
> +Date: June 2020
> +Contact: Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
> +Description: read only
> + This sysfs file exposes the cpumask which is designated to make
> + HCALLs to retrieve hv-24x7 pmu event counter data.
> +
> What: /sys/bus/event_source/devices/hv_24x7/event_descs/<event-name>
> Date: February 2014
> Contact: Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 93b4700dcf8c..3f769bb2d06a 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -448,6 +448,12 @@ static ssize_t device_show_string(struct device *dev,
> return sprintf(buf, "%s\n", (char *)d->var);
> }
>
> +static ssize_t cpumask_get_attr(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return cpumap_print_to_pagebuf(true, buf, &hv_24x7_cpumask);
> +}
> +
> static ssize_t sockets_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -1116,6 +1122,17 @@ static DEVICE_ATTR_RO(sockets);
> static DEVICE_ATTR_RO(chipspersocket);
> static DEVICE_ATTR_RO(coresperchip);
>
> +static DEVICE_ATTR(cpumask, S_IRUGO, cpumask_get_attr, NULL);
> +
> +static struct attribute *cpumask_attrs[] = {
> + &dev_attr_cpumask.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cpumask_attr_group = {
> + .attrs = cpumask_attrs,
> +};
> +
> static struct bin_attribute *if_bin_attrs[] = {
> &bin_attr_catalog,
> NULL,
> @@ -1143,6 +1160,11 @@ static const struct attribute_group *attr_groups[] = {
> &event_desc_group,
> &event_long_desc_group,
> &if_group,
> + /*
> + * This NULL is a placeholder for the cpumask attr which will update
> + * onlyif cpuhotplug registration is successful
> + */
> + NULL,
> NULL,
> };
>
> @@ -1684,7 +1706,7 @@ static int hv_24x7_cpu_hotplug_init(void)
>
> static int hv_24x7_init(void)
> {
> - int r;
> + int r, i = -1;
> unsigned long hret;
> struct hv_perf_caps caps;
>
> @@ -1731,6 +1753,15 @@ static int hv_24x7_init(void)
> if (r)
> return r;
>
> + /*
> + * Cpu hotplug init is successful, add the
> + * cpumask file as part of pmu attr group and
> + * assign it to very first NULL location.
> + */
> + while (attr_groups[++i])
> + /* nothing */;
> + attr_groups[i] = &cpumask_attr_group;
> +
We can avoid this complex stuff right.
Now that if the cpuhotplug init fail, we fail the pmu
registration right, with that, we dont need this dance.
Cant we just add the cpumask_attr_group right next to if_group
in "attr_group"?
Maddy
> r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
> if (r)
> return r;
^ permalink raw reply
* [PATCH v4 0/2] Add cpu hotplug support for powerpc/perf/hv-24x7
From: Kajol Jain @ 2020-07-08 8:59 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: nathanl, ego, maddy, kjain, suka, anju
This patchset add cpu hotplug support for hv_24x7 driver by adding
online/offline cpu hotplug function. It also add sysfs file
"cpumask" to expose current online cpu that can be used for
hv_24x7 event count.
Changelog:
v3 -> v4
- Make PMU initialization fail incase hotplug init failed. Rather then
just printing error msg.
- Did some nits changes like removing extra comment and initialising
target value part as suggested by Michael Ellerman
- Retained Reviewd-by tag because the changes were fixes to some nits.
- Incase we sequentially offline multiple cpus, taking cpumask_first() may
add some latency in that scenario.
So, I was trying to test benchmark in power9 lpar with 16 cpu,
by off-lining cpu 0-14
With cpumask_last: This is what I got.
real 0m2.812s
user 0m0.002s
sys 0m0.003s
With cpulast_any:
real 0m3.690s
user 0m0.002s
sys 0m0.062s
That's why I just went with cpumask_last thing.
v2 -> v3
- Corrected some of the typo mistakes and update commit message
as suggested by Gautham R Shenoy.
- Added Reviewed-by tag for the first patch in the patchset.
v1 -> v2
- Changed function to pick active cpu incase of offline
from "cpumask_any_but" to "cpumask_last", as
cpumask_any_but function pick very next online cpu and incase where
we are sequentially off-lining multiple cpus, "pmu_migrate_context"
can add extra latency.
- Suggested by: Gautham R Shenoy.
- Change documentation for cpumask and rather then hardcode the
initialization for cpumask_attr_group, add loop to get very first
NULL as suggested by Gautham R Shenoy.
Kajol Jain (2):
powerpc/perf/hv-24x7: Add cpu hotplug support
powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show cpumask
.../sysfs-bus-event_source-devices-hv_24x7 | 7 ++
arch/powerpc/perf/hv-24x7.c | 79 ++++++++++++++++++-
include/linux/cpuhotplug.h | 1 +
3 files changed, 86 insertions(+), 1 deletion(-)
--
2.26.2
^ permalink raw reply
* [PATCH v4 2/2] powerpc/hv-24x7: Add sysfs files inside hv-24x7 device to show cpumask
From: Kajol Jain @ 2020-07-08 8:59 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: nathanl, ego, maddy, kjain, suka, anju
In-Reply-To: <20200708085955.655686-1-kjain@linux.ibm.com>
Patch here adds a cpumask attr to hv_24x7 pmu along with ABI documentation.
Primary use to expose the cpumask is for the perf tool which has the
capability to parse the driver sysfs folder and understand the
cpumask file. Having cpumask file will reduce the number of perf command
line parameters (will avoid "-C" option in the perf tool
command line). It can also notify the user which is
the current cpu used to retrieve the counter data.
command:# cat /sys/devices/hv_24x7/cpumask
0
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
.../sysfs-bus-event_source-devices-hv_24x7 | 7 ++++
arch/powerpc/perf/hv-24x7.c | 33 ++++++++++++++++++-
2 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7 b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
index e8698afcd952..ee89d0e94602 100644
--- a/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
+++ b/Documentation/ABI/testing/sysfs-bus-event_source-devices-hv_24x7
@@ -43,6 +43,13 @@ Description: read only
This sysfs interface exposes the number of cores per chip
present in the system.
+What: /sys/devices/hv_24x7/cpumask
+Date: June 2020
+Contact: Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
+Description: read only
+ This sysfs file exposes the cpumask which is designated to make
+ HCALLs to retrieve hv-24x7 pmu event counter data.
+
What: /sys/bus/event_source/devices/hv_24x7/event_descs/<event-name>
Date: February 2014
Contact: Linux on PowerPC Developer List <linuxppc-dev@lists.ozlabs.org>
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 93b4700dcf8c..3f769bb2d06a 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -448,6 +448,12 @@ static ssize_t device_show_string(struct device *dev,
return sprintf(buf, "%s\n", (char *)d->var);
}
+static ssize_t cpumask_get_attr(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return cpumap_print_to_pagebuf(true, buf, &hv_24x7_cpumask);
+}
+
static ssize_t sockets_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -1116,6 +1122,17 @@ static DEVICE_ATTR_RO(sockets);
static DEVICE_ATTR_RO(chipspersocket);
static DEVICE_ATTR_RO(coresperchip);
+static DEVICE_ATTR(cpumask, S_IRUGO, cpumask_get_attr, NULL);
+
+static struct attribute *cpumask_attrs[] = {
+ &dev_attr_cpumask.attr,
+ NULL,
+};
+
+static struct attribute_group cpumask_attr_group = {
+ .attrs = cpumask_attrs,
+};
+
static struct bin_attribute *if_bin_attrs[] = {
&bin_attr_catalog,
NULL,
@@ -1143,6 +1160,11 @@ static const struct attribute_group *attr_groups[] = {
&event_desc_group,
&event_long_desc_group,
&if_group,
+ /*
+ * This NULL is a placeholder for the cpumask attr which will update
+ * onlyif cpuhotplug registration is successful
+ */
+ NULL,
NULL,
};
@@ -1684,7 +1706,7 @@ static int hv_24x7_cpu_hotplug_init(void)
static int hv_24x7_init(void)
{
- int r;
+ int r, i = -1;
unsigned long hret;
struct hv_perf_caps caps;
@@ -1731,6 +1753,15 @@ static int hv_24x7_init(void)
if (r)
return r;
+ /*
+ * Cpu hotplug init is successful, add the
+ * cpumask file as part of pmu attr group and
+ * assign it to very first NULL location.
+ */
+ while (attr_groups[++i])
+ /* nothing */;
+ attr_groups[i] = &cpumask_attr_group;
+
r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
if (r)
return r;
--
2.26.2
^ permalink raw reply related
* [PATCH v4 1/2] powerpc/perf/hv-24x7: Add cpu hotplug support
From: Kajol Jain @ 2020-07-08 8:59 UTC (permalink / raw)
To: linuxppc-dev, mpe; +Cc: nathanl, ego, maddy, kjain, suka, anju
In-Reply-To: <20200708085955.655686-1-kjain@linux.ibm.com>
Patch here adds cpu hotplug functions to hv_24x7 pmu.
A new cpuhp_state "CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE" enum
is added.
The online callback function updates the cpumask only if its
empty. As the primary intention of adding hotplug support
is to designate a CPU to make HCALL to collect the
counter data.
The offline function test and clear corresponding cpu in a cpumask
and update cpumask to any other active cpu.
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
arch/powerpc/perf/hv-24x7.c | 46 +++++++++++++++++++++++++++++++++++++
include/linux/cpuhotplug.h | 1 +
2 files changed, 47 insertions(+)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index db213eb7cb02..93b4700dcf8c 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -31,6 +31,8 @@ static int interface_version;
/* Whether we have to aggregate result data for some domains. */
static bool aggregate_result_elements;
+static cpumask_t hv_24x7_cpumask;
+
static bool domain_is_valid(unsigned domain)
{
switch (domain) {
@@ -1641,6 +1643,45 @@ static struct pmu h_24x7_pmu = {
.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
};
+static int ppc_hv_24x7_cpu_online(unsigned int cpu)
+{
+ if (cpumask_empty(&hv_24x7_cpumask))
+ cpumask_set_cpu(cpu, &hv_24x7_cpumask);
+
+ return 0;
+}
+
+static int ppc_hv_24x7_cpu_offline(unsigned int cpu)
+{
+ int target;
+
+ /* Check if exiting cpu is used for collecting 24x7 events */
+ if (!cpumask_test_and_clear_cpu(cpu, &hv_24x7_cpumask))
+ return 0;
+
+ /* Find a new cpu to collect 24x7 events */
+ target = cpumask_last(cpu_active_mask);
+
+ if (target < 0 || target >= nr_cpu_ids) {
+ pr_err("hv_24x7: CPU hotplug init failed\n");
+ return -1;
+ }
+
+ /* Migrate 24x7 events to the new target */
+ cpumask_set_cpu(target, &hv_24x7_cpumask);
+ perf_pmu_migrate_context(&h_24x7_pmu, cpu, target);
+
+ return 0;
+}
+
+static int hv_24x7_cpu_hotplug_init(void)
+{
+ return cpuhp_setup_state(CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
+ "perf/powerpc/hv_24x7:online",
+ ppc_hv_24x7_cpu_online,
+ ppc_hv_24x7_cpu_offline);
+}
+
static int hv_24x7_init(void)
{
int r;
@@ -1685,6 +1726,11 @@ static int hv_24x7_init(void)
if (r)
return r;
+ /* init cpuhotplug */
+ r = hv_24x7_cpu_hotplug_init();
+ if (r)
+ return r;
+
r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1);
if (r)
return r;
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 191772d4a4d7..a2710e654b64 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -181,6 +181,7 @@ enum cpuhp_state {
CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE,
CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE,
CPUHP_AP_PERF_POWERPC_TRACE_IMC_ONLINE,
+ CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE,
CPUHP_AP_WATCHDOG_ONLINE,
CPUHP_AP_WORKQUEUE_ONLINE,
CPUHP_AP_RCUTREE_ONLINE,
--
2.26.2
^ permalink raw reply related
* Re: kernel since 5.6 do not boot anymore on Apple PowerBook
From: Giuseppe Sacco @ 2020-07-08 8:56 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <639a48d1-815b-33f1-3c9e-cd9ca8ec41b1@csgroup.eu>
Salut Cristophe,
Il giorno mer, 08/07/2020 alle 10.38 +0200, Christophe Leroy ha
scritto:
> Hi Giuseppe,
>
> Le 08/07/2020 à 09:59, Giuseppe Sacco a écrit :
> > Hello Cristophe,
> >
> > Il giorno mar, 07/07/2020 alle 17.34 +0200, Giuseppe Sacco ha
> > scritto:
[...]
> > > > And can you try without CONFIG_VMAP_STACK on 5.6.19
> > >
> > > Sure, I'll let you know.
> >
> > No, this change did not make the kernel boot. I only changed the
> > option
> > you proposed:
> >
> > $ grep VMAP .config
> > CONFIG_HAVE_ARCH_VMAP_STACK=y
> > # CONFIG_VMAP_STACK is not set
> >
>
> Ok, at least it is not that. I wanted to be sure because this is a
> huge
> change added recently that is selected by default.
>
> I tried on QEMU with your config, it boots properly.
>
> So I think the only way now will be to bisect. Hope you were able to
> setup a cross compiler on a speedy machine.
thank you for your time; indeed I may now crosscompile the kernel in 15
minutes instead of about 390... I hope to have a final result in a
couple of days.
Bye,
Giuseppe
^ permalink raw reply
* Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
From: Peter Zijlstra @ 2020-07-08 8:41 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Will Deacon, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, Waiman Long, linuxppc-dev
In-Reply-To: <1594101082.hfq9x5yact.astroid@bobo.none>
On Tue, Jul 07, 2020 at 03:57:06PM +1000, Nicholas Piggin wrote:
> Yes, powerpc could certainly get more performance out of the slow
> paths, and then there are a few parameters to tune.
Can you clarify? The slow path is already in use on ARM64 which is weak,
so I doubt there's superfluous serialization present. And Will spend a
fair amount of time on making that thing guarantee forward progressm, so
there just isn't too much room to play.
> We don't have a good alternate patching for function calls yet, but
> that would be something to do for native vs pv.
Going by your jump_label implementation, support for static_call should
be fairly straight forward too, no?
https://lkml.kernel.org/r/20200624153024.794671356@infradead.org
^ permalink raw reply
* Re: kernel since 5.6 do not boot anymore on Apple PowerBook
From: Christophe Leroy @ 2020-07-08 8:38 UTC (permalink / raw)
To: Giuseppe Sacco, linuxppc-dev
In-Reply-To: <211a35b02193ae79a201d4d567fe1d7a53a979f5.camel@sguazz.it>
Hi Giuseppe,
Le 08/07/2020 à 09:59, Giuseppe Sacco a écrit :
> Hello Cristophe,
>
> Il giorno mar, 07/07/2020 alle 17.34 +0200, Giuseppe Sacco ha scritto:
>> Il giorno mar, 07/07/2020 alle 16.52 +0200, Christophe Leroy ha
>> scritto:
>>> Le 07/07/2020 à 16:03, Giuseppe Sacco a écrit :
>>>> Hello Cristophe,
>>>>
>>>>> Can you tell which defconfig you use or provide your .config
>>>>
>>>> You may get the standard one from debian or a reduced one that I
>>>> made on purpose. The latter is here:
>>>> https://eppesuigoccas.homedns.org/~giuseppe/config-5.4.50.gz
>>>>
>>>> (boot)
>>>> https://eppesuigoccas.homedns.org/~giuseppe/config-5.6.19.gz
>>>>
>>>> (no boot)
>>>
>>> Thanks
>>>
>>> Can you provide the complete output when it works, so that I can
>>> see
>>> what is after the place it stops when it fails.
>>
>> Here it is:
>> https://eppesuigoccas.homedns.org/~giuseppe/dmesg-5.4.40-minimo.gz
>>
>>
>>> And can you try without CONFIG_VMAP_STACK on 5.6.19
>>
>> Sure, I'll let you know.
>
> No, this change did not make the kernel boot. I only changed the option
> you proposed:
>
> $ grep VMAP .config
> CONFIG_HAVE_ARCH_VMAP_STACK=y
> # CONFIG_VMAP_STACK is not set
>
Ok, at least it is not that. I wanted to be sure because this is a huge
change added recently that is selected by default.
I tried on QEMU with your config, it boots properly.
So I think the only way now will be to bisect. Hope you were able to
setup a cross compiler on a speedy machine.
Christophe
^ permalink raw reply
* Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
From: Peter Zijlstra @ 2020-07-08 8:32 UTC (permalink / raw)
To: Waiman Long
Cc: linux-arch, Will Deacon, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, Nicholas Piggin, linuxppc-dev
In-Reply-To: <de3ead58-7f81-8ebd-754d-244f6be24af4@redhat.com>
On Tue, Jul 07, 2020 at 11:33:45PM -0400, Waiman Long wrote:
> From 5d7941a498935fb225b2c7a3108cbf590114c3db Mon Sep 17 00:00:00 2001
> From: Waiman Long <longman@redhat.com>
> Date: Tue, 7 Jul 2020 22:29:16 -0400
> Subject: [PATCH 2/9] locking/pvqspinlock: Introduce
> CONFIG_PARAVIRT_QSPINLOCKS_LITE
>
> Add a new PARAVIRT_QSPINLOCKS_LITE config option that allows
> architectures to use the PV qspinlock code without the need to use or
> implement a pv_kick() function, thus eliminating the atomic unlock
> overhead. The non-atomic queued_spin_unlock() can be used instead.
> The pv_wait() function will still be needed, but it can be a dummy
> function.
>
> With that option set, the hybrid PV queued/unfair locking code should
> still be able to make it performant enough in a paravirtualized
How is this supposed to work? If there is no kick, you have no control
over who wakes up and fairness goes out the window entirely.
You don't even begin to explain...
^ permalink raw reply
* Re: kernel since 5.6 do not boot anymore on Apple PowerBook
From: Giuseppe Sacco @ 2020-07-08 7:59 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <990279c219476c4d513df52454adf583de32641a.camel@sguazz.it>
Hello Cristophe,
Il giorno mar, 07/07/2020 alle 17.34 +0200, Giuseppe Sacco ha scritto:
> Il giorno mar, 07/07/2020 alle 16.52 +0200, Christophe Leroy ha
> scritto:
> > Le 07/07/2020 à 16:03, Giuseppe Sacco a écrit :
> > > Hello Cristophe,
> > >
> > > > Can you tell which defconfig you use or provide your .config
> > >
> > > You may get the standard one from debian or a reduced one that I
> > > made on purpose. The latter is here:
> > > https://eppesuigoccas.homedns.org/~giuseppe/config-5.4.50.gz
> > >
> > > (boot)
> > > https://eppesuigoccas.homedns.org/~giuseppe/config-5.6.19.gz
> > >
> > > (no boot)
> >
> > Thanks
> >
> > Can you provide the complete output when it works, so that I can
> > see
> > what is after the place it stops when it fails.
>
> Here it is:
> https://eppesuigoccas.homedns.org/~giuseppe/dmesg-5.4.40-minimo.gz
>
>
> > And can you try without CONFIG_VMAP_STACK on 5.6.19
>
> Sure, I'll let you know.
No, this change did not make the kernel boot. I only changed the option
you proposed:
$ grep VMAP .config
CONFIG_HAVE_ARCH_VMAP_STACK=y
# CONFIG_VMAP_STACK is not set
Thanks,
Giuseppe
^ permalink raw reply
* Re: [PATCH v3 1/9] powerpc/watchpoint: Fix 512 byte boundary limit
From: Christophe Leroy @ 2020-07-08 7:52 UTC (permalink / raw)
To: Jordan Niethe, Ravi Bangoria
Cc: Christophe Leroy, apopple, mikey, miltonm, peterz, oleg,
Nicholas Piggin, linux-kernel, Paul Mackerras, jolsa, fweisbec,
pedromfc, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <CACzsE9qka0j7vKm186Z70fhNy9L4dNR3B97-jQ2oZZAwYduaRw@mail.gmail.com>
Le 08/07/2020 à 09:44, Jordan Niethe a écrit :
> On Wed, Jul 8, 2020 at 2:53 PM Ravi Bangoria
> <ravi.bangoria@linux.ibm.com> wrote:
>>
>> Milton Miller reported that we are aligning start and end address to
>> wrong size SZ_512M. It should be SZ_512. Fix that.
>>
>> While doing this change I also found a case where ALIGN() comparison
>> fails. Within a given aligned range, ALIGN() of two addresses does not
>> match when start address is pointing to the first byte and end address
>> is pointing to any other byte except the first one. But that's not true
>> for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range
>> will always point to the first byte. So use ALIGN_DOWN() instead of
>> ALIGN().
>>
>> Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros")
>> Reported-by: Milton Miller <miltonm@us.ibm.com>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>> arch/powerpc/kernel/hw_breakpoint.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
>> index 0000daf0e1da..031e6defc08e 100644
>> --- a/arch/powerpc/kernel/hw_breakpoint.c
>> +++ b/arch/powerpc/kernel/hw_breakpoint.c
>> @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
>> if (dawr_enabled()) {
>> max_len = DAWR_MAX_LEN;
>> /* DAWR region can't cross 512 bytes boundary */
>> - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M))
>> + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512))
> I wonder if you should use end_addr - 1, but rather end_addr. For example:
> 512 -> 1023, because of the -1, 1024 will now be included in this
> range meaning 513 bytes?
end_addr is not part of the range.
If you want the range [512;1023], it means addr 512 len 512, that is
end_addr = addr + len = 1024
Christophe
^ permalink raw reply
* [PATCH] powerpc/64s/exception: Fix 0x1500 interrupt handler crash
From: Nicholas Piggin @ 2020-07-08 7:49 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Paul Menzel, Nicholas Piggin
A typo caused the interrupt handler to branch immediately to the common
"unknown interrupt" handler and skip the special case test for denormal
cause.
This does not affect KVM softpatch handling (e.g., for POWER9 TM assist)
because the KVM test was moved to common code by commit 9600f261acaa
("powerpc/64s/exception: Move KVM test to common code") just before this
bug was introduced.
Fixes: 3f7fbd97d07d ("powerpc/64s/exception: Clean up SRR specifiers")
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 2 +-
tools/testing/selftests/powerpc/math/Makefile | 2 +-
.../selftests/powerpc/math/fpu_denormal.c | 38 +++++++++++++++++++
3 files changed, 40 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/powerpc/math/fpu_denormal.c
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index fa080694e581..0fc8bad878b2 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -2551,7 +2551,7 @@ EXC_VIRT_NONE(0x5400, 0x100)
INT_DEFINE_BEGIN(denorm_exception)
IVEC=0x1500
IHSRR=1
- IBRANCH_COMMON=0
+ IBRANCH_TO_COMMON=0
IKVM_REAL=1
INT_DEFINE_END(denorm_exception)
diff --git a/tools/testing/selftests/powerpc/math/Makefile b/tools/testing/selftests/powerpc/math/Makefile
index 11a10d7a2bbd..4e2049d2fd8d 100644
--- a/tools/testing/selftests/powerpc/math/Makefile
+++ b/tools/testing/selftests/powerpc/math/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-TEST_GEN_PROGS := fpu_syscall fpu_preempt fpu_signal vmx_syscall vmx_preempt vmx_signal vsx_preempt
+TEST_GEN_PROGS := fpu_syscall fpu_preempt fpu_signal fpu_denormal vmx_syscall vmx_preempt vmx_signal vsx_preempt
top_srcdir = ../../../../..
include ../../lib.mk
diff --git a/tools/testing/selftests/powerpc/math/fpu_denormal.c b/tools/testing/selftests/powerpc/math/fpu_denormal.c
new file mode 100644
index 000000000000..5f96682abaa8
--- /dev/null
+++ b/tools/testing/selftests/powerpc/math/fpu_denormal.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright IBM Corp. 2020
+ *
+ * This test attempts to cause a FP denormal exception on POWER8 CPUs. Unfortunately
+ * if the denormal handler is not configured or working properly, this can cause a bad
+ * crash in kernel mode when the kernel tries to save FP registers when the process
+ * exits.
+ */
+
+#include <stdio.h>
+#include <string.h>
+
+#include "utils.h"
+
+static int test_denormal_fpu(void)
+{
+ unsigned int m32;
+ unsigned long m64;
+ volatile float f;
+ volatile double d;
+
+ /* try to induce lfs <denormal> ; stfd */
+
+ m32 = 0x00715fcf; /* random denormal */
+ memcpy((float *)&f, &m32, sizeof(f));
+ d = f;
+ memcpy(&m64, (double *)&d, sizeof(d));
+
+ FAIL_IF((long)(m64 != 0x380c57f3c0000000)); /* renormalised value */
+
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ return test_harness(test_denormal_fpu, "fpu_denormal");
+}
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v3 1/9] powerpc/watchpoint: Fix 512 byte boundary limit
From: Jordan Niethe @ 2020-07-08 7:44 UTC (permalink / raw)
To: Ravi Bangoria
Cc: Christophe Leroy, apopple, mikey, miltonm, peterz, oleg,
Nicholas Piggin, linux-kernel, Paul Mackerras, jolsa, fweisbec,
pedromfc, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200708045046.135702-2-ravi.bangoria@linux.ibm.com>
On Wed, Jul 8, 2020 at 2:53 PM Ravi Bangoria
<ravi.bangoria@linux.ibm.com> wrote:
>
> Milton Miller reported that we are aligning start and end address to
> wrong size SZ_512M. It should be SZ_512. Fix that.
>
> While doing this change I also found a case where ALIGN() comparison
> fails. Within a given aligned range, ALIGN() of two addresses does not
> match when start address is pointing to the first byte and end address
> is pointing to any other byte except the first one. But that's not true
> for ALIGN_DOWN(). ALIGN_DOWN() of any two addresses within that range
> will always point to the first byte. So use ALIGN_DOWN() instead of
> ALIGN().
>
> Fixes: e68ef121c1f4 ("powerpc/watchpoint: Use builtin ALIGN*() macros")
> Reported-by: Milton Miller <miltonm@us.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> arch/powerpc/kernel/hw_breakpoint.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 0000daf0e1da..031e6defc08e 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -419,7 +419,7 @@ static int hw_breakpoint_validate_len(struct arch_hw_breakpoint *hw)
> if (dawr_enabled()) {
> max_len = DAWR_MAX_LEN;
> /* DAWR region can't cross 512 bytes boundary */
> - if (ALIGN(start_addr, SZ_512M) != ALIGN(end_addr - 1, SZ_512M))
> + if (ALIGN_DOWN(start_addr, SZ_512) != ALIGN_DOWN(end_addr - 1, SZ_512))
I wonder if you should use end_addr - 1, but rather end_addr. For example:
512 -> 1023, because of the -1, 1024 will now be included in this
range meaning 513 bytes?
> return -EINVAL;
> } else if (IS_ENABLED(CONFIG_PPC_8xx)) {
> /* 8xx can setup a range without limitation */
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH v2 07/10] powerpc/perf: support BHRB disable bit and new filtering modes
From: Gautham R Shenoy @ 2020-07-08 7:43 UTC (permalink / raw)
To: Michael Neuling
Cc: ego, Athira Rajeev, Vaidyanathan Srinivasan, maddy, linuxppc-dev
In-Reply-To: <0cf26e42a3b190d5ea69d1ba61ae71bcaeee1973.camel@neuling.org>
On Tue, Jul 07, 2020 at 05:17:55PM +1000, Michael Neuling wrote:
> On Wed, 2020-07-01 at 05:20 -0400, Athira Rajeev wrote:
> > PowerISA v3.1 has few updates for the Branch History Rolling Buffer(BHRB).
> > First is the addition of BHRB disable bit and second new filtering
> > modes for BHRB.
> >
> > BHRB disable is controlled via Monitor Mode Control Register A (MMCRA)
> > bit 26, namely "BHRB Recording Disable (BHRBRD)". This field controls
> > whether BHRB entries are written when BHRB recording is enabled by other
> > bits. Patch implements support for this BHRB disable bit.
>
> Probably good to note here that this is backwards compatible. So if you have a
> kernel that doesn't know about this bit, it'll clear it and hence you still get
> BHRB.
>
> You should also note why you'd want to do disable this (ie. the core will run
> faster).
>
> > Secondly PowerISA v3.1 introduce filtering support for
> > PERF_SAMPLE_BRANCH_IND_CALL/COND. The patch adds BHRB filter support
> > for "ind_call" and "cond" in power10_bhrb_filter_map().
> >
> > 'commit bb19af816025 ("powerpc/perf: Prevent kernel address leak to userspace
> > via BHRB buffer")'
> > added a check in bhrb_read() to filter the kernel address from BHRB buffer.
> > Patch here modified
> > it to avoid that check for PowerISA v3.1 based processors, since PowerISA v3.1
> > allows
> > only MSR[PR]=1 address to be written to BHRB buffer.
> >
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/perf/core-book3s.c | 27 +++++++++++++++++++++------
> > arch/powerpc/perf/isa207-common.c | 13 +++++++++++++
> > arch/powerpc/perf/power10-pmu.c | 13 +++++++++++--
> > arch/powerpc/platforms/powernv/idle.c | 14 ++++++++++++++
>
> This touches the idle code so we should get those guys on CC (adding Vaidy and
> Ego).
>
> > 4 files changed, 59 insertions(+), 8 deletions(-)
> >
[..snip..]
> > diff --git a/arch/powerpc/platforms/powernv/idle.c
> > b/arch/powerpc/platforms/powernv/idle.c
> > index 2dd4673..7db99c7 100644
> > --- a/arch/powerpc/platforms/powernv/idle.c
> > +++ b/arch/powerpc/platforms/powernv/idle.c
> > @@ -611,6 +611,7 @@ static unsigned long power9_idle_stop(unsigned long psscr,
> > bool mmu_on)
> > unsigned long srr1;
> > unsigned long pls;
> > unsigned long mmcr0 = 0;
> > + unsigned long mmcra_bhrb = 0;
We are saving the whole of MMCRA aren't we ? We might want to just
name it mmcra in that case.
> > struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
> > bool sprs_saved = false;
> >
> > @@ -657,6 +658,15 @@ static unsigned long power9_idle_stop(unsigned long
> > psscr, bool mmu_on)
> > */
> > mmcr0 = mfspr(SPRN_MMCR0);
> > }
> > +
> > + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> > + /* POWER10 uses MMCRA[:26] as BHRB disable bit
> > + * to disable BHRB logic when not used. Hence Save and
> > + * restore MMCRA after a state-loss idle.
> > + */
Multi-line comment usually has the first line blank.
/*
* Line 1
* Line 2
* .
* .
* .
* Line N
*/
> > + mmcra_bhrb = mfspr(SPRN_MMCRA);
>
>
> Why is the bhrb bit of mmcra special here?
The comment above could include the consequence of not saving and
restoring MMCRA i.e
- If the user hasn't asked for the BHRB to be
written the value of MMCRA[BHRBD] = 1.
- On wakeup from stop, MMCRA[BHRBD] will be 0, since MMCRA is not a
previleged resource and will be lost.
- Thus, if we do not save and restore the MMCRA[BHRBD], the hardware
will be needlessly writing to the BHRB in the problem mode.
>
> > + }
> > +
> > if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
> > sprs.lpcr = mfspr(SPRN_LPCR);
> > sprs.hfscr = mfspr(SPRN_HFSCR);
> > @@ -721,6 +731,10 @@ static unsigned long power9_idle_stop(unsigned long
> > psscr, bool mmu_on)
> > mtspr(SPRN_MMCR0, mmcr0);
> > }
> >
> > + /* Reload MMCRA to restore BHRB disable bit for POWER10 */
> > + if (cpu_has_feature(CPU_FTR_ARCH_31))
> > + mtspr(SPRN_MMCRA, mmcra_bhrb);
> > +
> > /*
> > * DD2.2 and earlier need to set then clear bit 60 in MMCRA
> > * to ensure the PMU starts running.
>
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH 16/20] Documentation: s390/vfio-ap: eliminate duplicated word
From: Pierre Morel @ 2020-07-08 6:57 UTC (permalink / raw)
To: Randy Dunlap, linux-kernel
Cc: kvm, linux-doc, David Airlie, kgdb-bugreport, linux-fpga,
Liviu Dudau, dri-devel, linux-mips, Paul Cercueil, keyrings,
Paul Mackerras, linux-i2c, Pavel Machek, Srinivas Pandruvada,
Mihail Atanassov, linux-leds, linux-s390, Daniel Thompson,
linux-scsi, Jonathan Corbet, Masahiro Yamada, Matthew Wilcox,
Halil Pasic, Jarkko Sakkinen, James Wang, linux-input,
Mali DP Maintainers, Derek Kiernan, Dragan Cvetic, Wu Hao,
Tony Krowiak, linux-kbuild, James E.J. Bottomley, Jiri Kosina,
Hannes Reinecke, linux-block, Thomas Bogendoerfer,
Jacek Anaszewski, linux-mm, Dan Williams, Andrew Morton,
Mimi Zohar, Jens Axboe, Michal Marek, Martin K. Petersen,
Douglas Anderson, Wolfram Sang, Daniel Vetter, Jason Wessel,
Paolo Bonzini, linux-integrity, linuxppc-dev, Mike Rapoport,
Dan Murphy
In-Reply-To: <20200707180414.10467-17-rdunlap@infradead.org>
On 2020-07-07 20:04, Randy Dunlap wrote:
> Drop the doubled word "the".
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-doc@vger.kernel.org
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Pierre Morel <pmorel@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
> Documentation/s390/vfio-ap.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-next-20200701.orig/Documentation/s390/vfio-ap.rst
> +++ linux-next-20200701/Documentation/s390/vfio-ap.rst
> @@ -361,7 +361,7 @@ matrix device.
> assign_domain / unassign_domain:
> Write-only attributes for assigning/unassigning an AP usage domain to/from
> the mediated matrix device. To assign/unassign a domain, the domain
> - number of the the usage domain is echoed to the respective attribute
> + number of the usage domain is echoed to the respective attribute
> file.
> matrix:
> A read-only file for displaying the APQNs derived from the cross product
>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Thanks Randy,
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
^ 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