* [RFC v2 2/2] drm/amd/display: Use PPC FPU functions
From: Anson Jacob @ 2021-07-21 4:48 UTC (permalink / raw)
To: mpe, benh, paulus, christophe.leroy, linuxppc-dev, amd-gfx,
linux-kernel
Cc: Sunpeng.Li, Harry Wentland, qingqing.zhuo, Rodrigo.Siqueira,
roman.li, Christoph Hellwig, Anson.Jacob, Aurabindo.Pillai,
Bhawanpreet.Lakha, Christian König, bindu.r
In-Reply-To: <20210721044801.840501-1-Anson.Jacob@amd.com>
Use kernel_fpu_begin & kernel_fpu_end for PPC
Depends on "ppc/fpu: Add generic FPU api similar to x86"
v2:
- Got rid of macro switch for PPC as header file with same
name as x86 is added by previous patch in the series
Signed-off-by: Anson Jacob <Anson.Jacob@amd.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
CC: Harry Wentland <harry.wentland@amd.com>
CC: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/display/dc/os_types.h | 29 -----------------------
1 file changed, 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
index 126c2f3a4dd3..47ef434f93d8 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -51,38 +51,9 @@
#define dm_error(fmt, ...) DRM_ERROR(fmt, ##__VA_ARGS__)
#if defined(CONFIG_DRM_AMD_DC_DCN)
-#if defined(CONFIG_X86)
#include <asm/fpu/api.h>
#define DC_FP_START() kernel_fpu_begin()
#define DC_FP_END() kernel_fpu_end()
-#elif defined(CONFIG_PPC64)
-#include <asm/switch_to.h>
-#include <asm/cputable.h>
-#define DC_FP_START() { \
- if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
- preempt_disable(); \
- enable_kernel_vsx(); \
- } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
- preempt_disable(); \
- enable_kernel_altivec(); \
- } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
- preempt_disable(); \
- enable_kernel_fp(); \
- } \
-}
-#define DC_FP_END() { \
- if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
- disable_kernel_vsx(); \
- preempt_enable(); \
- } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
- disable_kernel_altivec(); \
- preempt_enable(); \
- } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
- disable_kernel_fp(); \
- preempt_enable(); \
- } \
-}
-#endif
#endif
/*
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] powerpc/64s/perf: Always use SIAR for kernel interrupts
From: Athira Rajeev @ 2021-07-21 5:36 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Madhavan Srinivasan, linuxppc-dev
In-Reply-To: <20210720141504.420110-1-npiggin@gmail.com>
> On 20-Jul-2021, at 7:45 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> If an interrupt is taken in kernel mode, always use SIAR for it rather than
> looking at regs_sipr. This prevents samples piling up around interrupt
> enable (hard enable or interrupt replay via soft enable) in PMUs / modes
> where the PR sample indication is not in synch with SIAR.
>
> This results in better sampling of interrupt entry and exit in particular.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/perf/core-book3s.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index bb0ee716de91..91203ed9d0ff 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -340,6 +340,13 @@ static inline void perf_read_regs(struct pt_regs *regs)
> * If the PMU doesn't update the SIAR for non marked events use
> * pt_regs.
> *
> + * If regs is a kernel interrupt, always use SIAR. Some PMUs have an
> + * issue with regs_sipr not being in synch with SIAR in interrupt entry
> + * and return sequences, which can result in regs_sipr being true for
> + * kernel interrupts and SIAR, which has the effect of causing samples
> + * to pile up at mtmsrd MSR[EE] 0->1 or pending irq replay around
> + * interrupt entry/exit.
> + *
> * If the PMU has HV/PR flags then check to see if they
> * place the exception in userspace. If so, use pt_regs. In
> * continuous sampling mode the SIAR and the PMU exception are
> @@ -356,6 +363,8 @@ static inline void perf_read_regs(struct pt_regs *regs)
> use_siar = 1;
> else if ((ppmu->flags & PPMU_NO_CONT_SAMPLING))
> use_siar = 0;
> + else if (!user_mode(regs))
> + use_siar = 1;
Tested the change.
Workload used: Testcase that runs a loop of “scv” syscalls
Before the patch:
# perf record <test>
# perf report|grep replay
3.90% scvonly [kernel.vmlinux] [k] replay_soft_interrupts
Samples were present around interrupt replay code.
After the fix, perf report didn’t had samples pointing to replay code.
Tested-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Thanks
Athira
> else if (!(ppmu->flags & PPMU_NO_SIPR) && regs_sipr(regs))
> use_siar = 0;
> else
> --
> 2.23.0
>
^ permalink raw reply
* [PATCH V4 1/1] powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting an overflown PMC
From: Athira Rajeev @ 2021-07-21 5:48 UTC (permalink / raw)
To: mpe; +Cc: maddy, linuxppc-dev, npiggin, rnsastry
In-Reply-To: <1626846509-1350-1-git-send-email-atrajeev@linux.vnet.ibm.com>
Running perf fuzzer showed below in dmesg logs:
"Can't find PMC that caused IRQ"
This means a PMU exception happened, but none of the PMC's (Performance
Monitor Counter) were found to be overflown. There are some corner cases
that clears the PMCs after PMI gets masked. In such cases, the perf
interrupt handler will not find the active PMC values that had caused
the overflow and thus leads to this message while replaying.
Case 1: PMU Interrupt happens during replay of other interrupts and
counter values gets cleared by PMU callbacks before replay:
During replay of interrupts like timer, __do_irq and doorbell exception, we
conditionally enable interrupts via may_hard_irq_enable(). This could
potentially create a window to generate a PMI. Since irq soft mask is set
to ALL_DISABLED, the PMI will get masked here. We could get IPIs run before
perf interrupt is replayed and the PMU events could deleted or stopped.
This will change the PMU SPR values and resets the counters. Snippet of
ftrace log showing PMU callbacks invoked in "__do_irq":
<idle>-0 [051] dns. 132025441306354: __do_irq <-call_do_irq
<idle>-0 [051] dns. 132025441306430: irq_enter <-__do_irq
<idle>-0 [051] dns. 132025441306503: irq_enter_rcu <-__do_irq
<idle>-0 [051] dnH. 132025441306599: xive_get_irq <-__do_irq
<<>>
<idle>-0 [051] dnH. 132025441307770: generic_smp_call_function_single_interrupt <-smp_ipi_demux_relaxed
<idle>-0 [051] dnH. 132025441307839: flush_smp_call_function_queue <-smp_ipi_demux_relaxed
<idle>-0 [051] dnH. 132025441308057: _raw_spin_lock <-event_function
<idle>-0 [051] dnH. 132025441308206: power_pmu_disable <-perf_pmu_disable
<idle>-0 [051] dnH. 132025441308337: power_pmu_del <-event_sched_out
<idle>-0 [051] dnH. 132025441308407: power_pmu_read <-power_pmu_del
<idle>-0 [051] dnH. 132025441308477: read_pmc <-power_pmu_read
<idle>-0 [051] dnH. 132025441308590: isa207_disable_pmc <-power_pmu_del
<idle>-0 [051] dnH. 132025441308663: write_pmc <-power_pmu_del
<idle>-0 [051] dnH. 132025441308787: power_pmu_event_idx <-perf_event_update_userpage
<idle>-0 [051] dnH. 132025441308859: rcu_read_unlock_strict <-perf_event_update_userpage
<idle>-0 [051] dnH. 132025441308975: power_pmu_enable <-perf_pmu_enable
<<>>
<idle>-0 [051] dnH. 132025441311108: irq_exit <-__do_irq
<idle>-0 [051] dns. 132025441311319: performance_monitor_exception <-replay_soft_interrupts
Case 2: PMI's masked during local_* operations, example local_add.
If the local_add operation happens within a local_irq_save, replay of
PMI will be during local_irq_restore. Similar to case 1, this could
also create a window before replay where PMU events gets deleted or
stopped.
Patch adds a fix to update the PMU callback function 'power_pmu_disable' to
check for pending perf interrupt. If there is an overflown PMC and pending
perf interrupt indicated in Paca, clear the PMI bit in paca to drop that
sample. Clearing of PMI bit is done in 'power_pmu_disable' since disable is
invoked before any event gets deleted/stopped. With this fix, if there are
more than one event running in the PMU, there is a chance that we clear the
PMI bit for the event which is not getting deleted/stopped. The other
events may still remain active. Hence to make sure we don't drop valid
sample in such cases, another check is added in power_pmu_enable. This
checks if there is an overflown PMC found among the active events and if
so enable back the PMI bit. Two new helper functions are introduced to
clear/set the PMI, ie 'clear_pmi_irq_pending' and 'set_pmi_irq_pending'.
Helper function 'pmi_irq_pending' is introduced to give a warning if
there is pending PMI bit in paca, but no PMC is overflown.
Also there are corner cases which results in performance monitor interrupts
getting triggered during power_pmu_disable. This happens since PMXE bit is
not cleared along with disabling of other MMCR0 bits in the pmu_disable.
Such PMI's could leave the PMU running and could trigger PMI again which
will set MMCR0 PMAO bit. This could lead to spurious interrupts in some
corner cases. Example, a timer after power_pmu_del which will re-enable
interrupts and triggers a PMI again since PMAO bit is still set. But fails
to find valid overflow since PMC get cleared in power_pmu_del. Patch
fixes this by disabling PMXE along with disabling of other MMCR0 bits
in power_pmu_disable.
We can't just replay PMI any time. Hence this approach is preferred rather
than replaying PMI before resetting overflown PMC. Patch also documents
core-book3s on a race condition which can trigger these PMC messages during
idle path in PowerNV.
Fixes: f442d004806e ("powerpc/64s: Add support to mask perf interrupts and replay them")
Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
Suggested-by: Nicholas Piggin <npiggin@gmail.com>
Suggested-by: Madhavan Srinivasan <maddy@linux.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 38 +++++++++++++++++++++++++
arch/powerpc/perf/core-book3s.c | 59 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index 21cc571..d4e2d74 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -224,6 +224,40 @@ static inline bool arch_irqs_disabled(void)
return arch_irqs_disabled_flags(arch_local_save_flags());
}
+static inline void set_pmi_irq_pending(void)
+{
+ /*
+ * Invoked from PMU callback functions to set
+ * PMI bit in Paca. This has to be called with
+ * irq's disabled ( via hard_irq_disable ).
+ */
+ if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
+ WARN_ON_ONCE(mfmsr() & MSR_EE);
+ get_paca()->irq_happened |= PACA_IRQ_PMI;
+}
+
+static inline void clear_pmi_irq_pending(void)
+{
+ /*
+ * Invoked from PMU callback functions to clear
+ * the pending PMI bit in Paca.
+ */
+ if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
+ WARN_ON_ONCE(mfmsr() & MSR_EE);
+ get_paca()->irq_happened &= ~PACA_IRQ_PMI;
+}
+
+static inline int pmi_irq_pending(void)
+{
+ /*
+ * Invoked from PMU callback functions to check
+ * if there is a pending PMI bit in Paca.
+ */
+ if (get_paca()->irq_happened & PACA_IRQ_PMI)
+ return 1;
+ return 0;
+}
+
#ifdef CONFIG_PPC_BOOK3S
/*
* To support disabling and enabling of irq with PMI, set of
@@ -408,6 +442,10 @@ static inline void do_hard_irq_enable(void)
BUILD_BUG();
}
+static inline void clear_pmi_irq_pending(void) { }
+static inline void set_pmi_irq_pending(void) { }
+static inline int pmi_irq_pending(void) { return 0; }
+
static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs, unsigned long val)
{
}
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index bb0ee71..ad29220 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -848,6 +848,19 @@ static void write_pmc(int idx, unsigned long val)
}
}
+static int any_pmc_overflown(struct cpu_hw_events *cpuhw)
+{
+ int i, idx;
+
+ for (i = 0; i < cpuhw->n_events; i++) {
+ idx = cpuhw->event[i]->hw.idx;
+ if ((idx) && ((int)read_pmc(idx) < 0))
+ return idx;
+ }
+
+ return 0;
+}
+
/* Called from sysrq_handle_showregs() */
void perf_event_print_debug(void)
{
@@ -1272,11 +1285,13 @@ static void power_pmu_disable(struct pmu *pmu)
/*
* Set the 'freeze counters' bit, clear EBE/BHRBA/PMCC/PMAO/FC56
+ * Also clear PMXE to disable PMI's getting triggered in some
+ * corner cases during PMU disable.
*/
val = mmcr0 = mfspr(SPRN_MMCR0);
val |= MMCR0_FC;
val &= ~(MMCR0_EBE | MMCR0_BHRBA | MMCR0_PMCC | MMCR0_PMAO |
- MMCR0_FC56);
+ MMCR0_PMXE | MMCR0_FC56);
/* Set mmcr0 PMCCEXT for p10 */
if (ppmu->flags & PPMU_ARCH_31)
val |= MMCR0_PMCCEXT;
@@ -1290,6 +1305,24 @@ static void power_pmu_disable(struct pmu *pmu)
mb();
isync();
+ /*
+ * Some corner cases could clear the PMU counter overflow
+ * while a masked PMI is pending. One of such case is
+ * when a PMI happens during interrupt replay and perf
+ * counter values gets cleared by PMU callbacks before
+ * replay.
+ *
+ * If any of PMC corresponding to the active PMU events is
+ * overflown, disable the interrupt by clearing the paca
+ * bit for PMI since we are disabling the PMU now.
+ * Otherwise provide a warning if there is PMI pending, but
+ * no counter is found overflown.
+ */
+ if (any_pmc_overflown(cpuhw))
+ clear_pmi_irq_pending();
+ else
+ WARN_ON(pmi_irq_pending());
+
val = mmcra = cpuhw->mmcr.mmcra;
/*
@@ -1381,6 +1414,15 @@ static void power_pmu_enable(struct pmu *pmu)
* (possibly updated for removal of events).
*/
if (!cpuhw->n_added) {
+ /*
+ * If there is any active event with an overflown PMC
+ * value, Set back PACA_IRQ_PMI which would have got
+ * cleared in power_pmu_disable.
+ */
+ hard_irq_disable();
+ if (any_pmc_overflown(cpuhw))
+ set_pmi_irq_pending();
+
mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
if (ppmu->flags & PPMU_ARCH_31)
@@ -2336,6 +2378,12 @@ static void __perf_event_interrupt(struct pt_regs *regs)
break;
}
}
+ /*
+ * Clear PACA_IRQ_PMI in case it was set by
+ * set_pmi_irq_pending() when PMU was enabled
+ * after accounting for interrupts.
+ */
+ clear_pmi_irq_pending();
if (!active)
/* reset non active counters that have overflowed */
write_pmc(i + 1, 0);
@@ -2355,6 +2403,15 @@ static void __perf_event_interrupt(struct pt_regs *regs)
}
}
}
+
+ /*
+ * During system wide profling or while specific CPU
+ * is monitored for an event, some corner cases could
+ * cause PMC to overflow in idle path. This will trigger
+ * a PMI after waking up from idle. Since counter values
+ * are _not_ saved/restored in idle path, can lead to
+ * below "Can't find PMC" message.
+ */
if (unlikely(!found) && !arch_irq_disabled_regs(regs))
printk_ratelimited(KERN_WARNING "Can't find PMC that caused IRQ\n");
--
1.8.3.1
^ permalink raw reply related
* [PATCH V4 0/1] powerpc/perf: Clear pending PMI in ppmu callbacks
From: Athira Rajeev @ 2021-07-21 5:48 UTC (permalink / raw)
To: mpe; +Cc: maddy, linuxppc-dev, npiggin, rnsastry
Running perf fuzzer testsuite popped up below messages
in the dmesg logs:
"Can't find PMC that caused IRQ"
This means a PMU exception happened, but none of the PMC's (Performance
Monitor Counter) were found to be overflown. Perf interrupt handler checks
the PMC's to see which PMC has overflown and if none of the PMCs are
overflown ( counter value not >= 0x80000000 ), it throws warning:
"Can't find PMC that caused IRQ".
Powerpc has capability to mask and replay a performance monitoring
interrupt (PMI). In case of replayed PMI, there are some corner cases
that clears the PMCs after masking. In such cases, the perf interrupt
handler will not find the active PMC values that had caused the overflow
and thus leading to this message. This patchset attempts to fix those
corner cases.
However there is one more case in PowerNV where these messages are
emitted during system wide profiling or when a specific CPU is monitored
for an event. That is, when a counter overflow just before entering idle
and a PMI gets triggered after wakeup from idle. Since PMCs
are not saved in the idle path, perf interrupt handler will not
find overflown counter value and emits the "Can't find PMC" messages.
This patch documents this race condition in powerpc core-book3s.
Patch fixes the ppmu callbacks to disable pending interrupt before clearing
the overflown PMC and documents the race condition in idle path.
Changelog:
changes from v3 -> v4
Addressed review comments from Nicholas Piggin
- Added comment explaining the need to clear MMCR0 PMXE bit in
pmu disable callback.
- Added a check to display warning if there is a PMI pending
bit set in Paca without any overflown PMC.
- Removed the condition check before clearing pending PMI
in 'clear_pmi_irq_pending' function.
- Added reviewed by from Nicholas Piggin.
Changes from v2 -> v3
Addressed review comments from Nicholas Piggin
- Moved the clearing of PMI bit to power_pmu_disable.
In previous versions, this was done in power_pmu_del,
power_pmu_stop/enable callbacks before clearing of PMC's.
- power_pmu_disable is called before any event gets deleted
or stopped. If more than one event is running in the PMU,
we may clear the PMI bit for an event which is not going
to be deleted/stopped. Hence introduced check in
power_pmu_enable to set back PMI to avoid dropping of valid
samples in such cases.
- Disable MMCR0 PMXE bit in pmu disable callback which otherwise
could trigger PMI when PMU is getting disabled.
Changes from v1 -> v2
Addressed review comments from Nicholas Piggin
- Moved the PMI pending check and clearing function
to arch/powerpc/include/asm/hw_irq.h and renamed
function to "get_clear_pmi_irq_pending"
- Along with checking for pending PMI bit in Paca,
look for PMAO bit in MMCR0 register to decide on
pending PMI interrupt.
Athira Rajeev (1):
powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting
an overflown PMC
arch/powerpc/include/asm/hw_irq.h | 38 +++++++++++++++++++++++++
arch/powerpc/perf/core-book3s.c | 59 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 96 insertions(+), 1 deletion(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH V4 1/1] powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting an overflown PMC
From: Nageswara Sastry @ 2021-07-21 5:50 UTC (permalink / raw)
To: Athira Rajeev, mpe; +Cc: maddy, linuxppc-dev, npiggin
In-Reply-To: <1626846509-1350-2-git-send-email-atrajeev@linux.vnet.ibm.com>
On 21/07/21 11:18 am, Athira Rajeev wrote:
> Running perf fuzzer showed below in dmesg logs:
> "Can't find PMC that caused IRQ"
>
> This means a PMU exception happened, but none of the PMC's (Performance
> Monitor Counter) were found to be overflown. There are some corner cases
> that clears the PMCs after PMI gets masked. In such cases, the perf
> interrupt handler will not find the active PMC values that had caused
> the overflow and thus leads to this message while replaying.
>
> Case 1: PMU Interrupt happens during replay of other interrupts and
> counter values gets cleared by PMU callbacks before replay:
>
> During replay of interrupts like timer, __do_irq and doorbell exception, we
> conditionally enable interrupts via may_hard_irq_enable(). This could
> potentially create a window to generate a PMI. Since irq soft mask is set
> to ALL_DISABLED, the PMI will get masked here. We could get IPIs run before
> perf interrupt is replayed and the PMU events could deleted or stopped.
> This will change the PMU SPR values and resets the counters. Snippet of
> ftrace log showing PMU callbacks invoked in "__do_irq":
>
> <idle>-0 [051] dns. 132025441306354: __do_irq <-call_do_irq
> <idle>-0 [051] dns. 132025441306430: irq_enter <-__do_irq
> <idle>-0 [051] dns. 132025441306503: irq_enter_rcu <-__do_irq
> <idle>-0 [051] dnH. 132025441306599: xive_get_irq <-__do_irq
> <<>>
> <idle>-0 [051] dnH. 132025441307770: generic_smp_call_function_single_interrupt <-smp_ipi_demux_relaxed
> <idle>-0 [051] dnH. 132025441307839: flush_smp_call_function_queue <-smp_ipi_demux_relaxed
> <idle>-0 [051] dnH. 132025441308057: _raw_spin_lock <-event_function
> <idle>-0 [051] dnH. 132025441308206: power_pmu_disable <-perf_pmu_disable
> <idle>-0 [051] dnH. 132025441308337: power_pmu_del <-event_sched_out
> <idle>-0 [051] dnH. 132025441308407: power_pmu_read <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308477: read_pmc <-power_pmu_read
> <idle>-0 [051] dnH. 132025441308590: isa207_disable_pmc <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308663: write_pmc <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308787: power_pmu_event_idx <-perf_event_update_userpage
> <idle>-0 [051] dnH. 132025441308859: rcu_read_unlock_strict <-perf_event_update_userpage
> <idle>-0 [051] dnH. 132025441308975: power_pmu_enable <-perf_pmu_enable
> <<>>
> <idle>-0 [051] dnH. 132025441311108: irq_exit <-__do_irq
> <idle>-0 [051] dns. 132025441311319: performance_monitor_exception <-replay_soft_interrupts
>
> Case 2: PMI's masked during local_* operations, example local_add.
> If the local_add operation happens within a local_irq_save, replay of
> PMI will be during local_irq_restore. Similar to case 1, this could
> also create a window before replay where PMU events gets deleted or
> stopped.
>
> Patch adds a fix to update the PMU callback function 'power_pmu_disable' to
> check for pending perf interrupt. If there is an overflown PMC and pending
> perf interrupt indicated in Paca, clear the PMI bit in paca to drop that
> sample. Clearing of PMI bit is done in 'power_pmu_disable' since disable is
> invoked before any event gets deleted/stopped. With this fix, if there are
> more than one event running in the PMU, there is a chance that we clear the
> PMI bit for the event which is not getting deleted/stopped. The other
> events may still remain active. Hence to make sure we don't drop valid
> sample in such cases, another check is added in power_pmu_enable. This
> checks if there is an overflown PMC found among the active events and if
> so enable back the PMI bit. Two new helper functions are introduced to
> clear/set the PMI, ie 'clear_pmi_irq_pending' and 'set_pmi_irq_pending'.
> Helper function 'pmi_irq_pending' is introduced to give a warning if
> there is pending PMI bit in paca, but no PMC is overflown.
>
> Also there are corner cases which results in performance monitor interrupts
> getting triggered during power_pmu_disable. This happens since PMXE bit is
> not cleared along with disabling of other MMCR0 bits in the pmu_disable.
> Such PMI's could leave the PMU running and could trigger PMI again which
> will set MMCR0 PMAO bit. This could lead to spurious interrupts in some
> corner cases. Example, a timer after power_pmu_del which will re-enable
> interrupts and triggers a PMI again since PMAO bit is still set. But fails
> to find valid overflow since PMC get cleared in power_pmu_del. Patch
> fixes this by disabling PMXE along with disabling of other MMCR0 bits
> in power_pmu_disable.
>
> We can't just replay PMI any time. Hence this approach is preferred rather
> than replaying PMI before resetting overflown PMC. Patch also documents
> core-book3s on a race condition which can trigger these PMC messages during
> idle path in PowerNV.
>
> Fixes: f442d004806e ("powerpc/64s: Add support to mask perf interrupts and replay them")
> Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Suggested-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_irq.h | 38 +++++++++++++++++++++++++
> arch/powerpc/perf/core-book3s.c | 59 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 21cc571..d4e2d74 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -224,6 +224,40 @@ static inline bool arch_irqs_disabled(void)
> return arch_irqs_disabled_flags(arch_local_save_flags());
> }
>
> +static inline void set_pmi_irq_pending(void)
> +{
> + /*
> + * Invoked from PMU callback functions to set
> + * PMI bit in Paca. This has to be called with
> + * irq's disabled ( via hard_irq_disable ).
> + */
> + if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> + get_paca()->irq_happened |= PACA_IRQ_PMI;
> +}
> +
> +static inline void clear_pmi_irq_pending(void)
> +{
> + /*
> + * Invoked from PMU callback functions to clear
> + * the pending PMI bit in Paca.
> + */
> + if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> + get_paca()->irq_happened &= ~PACA_IRQ_PMI;
> +}
> +
> +static inline int pmi_irq_pending(void)
> +{
> + /*
> + * Invoked from PMU callback functions to check
> + * if there is a pending PMI bit in Paca.
> + */
> + if (get_paca()->irq_happened & PACA_IRQ_PMI)
> + return 1;
> + return 0;
> +}
> +
> #ifdef CONFIG_PPC_BOOK3S
> /*
> * To support disabling and enabling of irq with PMI, set of
> @@ -408,6 +442,10 @@ static inline void do_hard_irq_enable(void)
> BUILD_BUG();
> }
>
> +static inline void clear_pmi_irq_pending(void) { }
> +static inline void set_pmi_irq_pending(void) { }
> +static inline int pmi_irq_pending(void) { return 0; }
> +
> static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs, unsigned long val)
> {
> }
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index bb0ee71..ad29220 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -848,6 +848,19 @@ static void write_pmc(int idx, unsigned long val)
> }
> }
>
> +static int any_pmc_overflown(struct cpu_hw_events *cpuhw)
> +{
> + int i, idx;
> +
> + for (i = 0; i < cpuhw->n_events; i++) {
> + idx = cpuhw->event[i]->hw.idx;
> + if ((idx) && ((int)read_pmc(idx) < 0))
> + return idx;
> + }
> +
> + return 0;
> +}
> +
> /* Called from sysrq_handle_showregs() */
> void perf_event_print_debug(void)
> {
> @@ -1272,11 +1285,13 @@ static void power_pmu_disable(struct pmu *pmu)
>
> /*
> * Set the 'freeze counters' bit, clear EBE/BHRBA/PMCC/PMAO/FC56
> + * Also clear PMXE to disable PMI's getting triggered in some
> + * corner cases during PMU disable.
> */
> val = mmcr0 = mfspr(SPRN_MMCR0);
> val |= MMCR0_FC;
> val &= ~(MMCR0_EBE | MMCR0_BHRBA | MMCR0_PMCC | MMCR0_PMAO |
> - MMCR0_FC56);
> + MMCR0_PMXE | MMCR0_FC56);
> /* Set mmcr0 PMCCEXT for p10 */
> if (ppmu->flags & PPMU_ARCH_31)
> val |= MMCR0_PMCCEXT;
> @@ -1290,6 +1305,24 @@ static void power_pmu_disable(struct pmu *pmu)
> mb();
> isync();
>
> + /*
> + * Some corner cases could clear the PMU counter overflow
> + * while a masked PMI is pending. One of such case is
> + * when a PMI happens during interrupt replay and perf
> + * counter values gets cleared by PMU callbacks before
> + * replay.
> + *
> + * If any of PMC corresponding to the active PMU events is
> + * overflown, disable the interrupt by clearing the paca
> + * bit for PMI since we are disabling the PMU now.
> + * Otherwise provide a warning if there is PMI pending, but
> + * no counter is found overflown.
> + */
> + if (any_pmc_overflown(cpuhw))
> + clear_pmi_irq_pending();
> + else
> + WARN_ON(pmi_irq_pending());
> +
> val = mmcra = cpuhw->mmcr.mmcra;
>
> /*
> @@ -1381,6 +1414,15 @@ static void power_pmu_enable(struct pmu *pmu)
> * (possibly updated for removal of events).
> */
> if (!cpuhw->n_added) {
> + /*
> + * If there is any active event with an overflown PMC
> + * value, Set back PACA_IRQ_PMI which would have got
> + * cleared in power_pmu_disable.
> + */
> + hard_irq_disable();
> + if (any_pmc_overflown(cpuhw))
> + set_pmi_irq_pending();
> +
> mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
> mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
> if (ppmu->flags & PPMU_ARCH_31)
> @@ -2336,6 +2378,12 @@ static void __perf_event_interrupt(struct pt_regs *regs)
> break;
> }
> }
> + /*
> + * Clear PACA_IRQ_PMI in case it was set by
> + * set_pmi_irq_pending() when PMU was enabled
> + * after accounting for interrupts.
> + */
> + clear_pmi_irq_pending();
> if (!active)
> /* reset non active counters that have overflowed */
> write_pmc(i + 1, 0);
> @@ -2355,6 +2403,15 @@ static void __perf_event_interrupt(struct pt_regs *regs)
> }
> }
> }
> +
> + /*
> + * During system wide profling or while specific CPU
> + * is monitored for an event, some corner cases could
> + * cause PMC to overflow in idle path. This will trigger
> + * a PMI after waking up from idle. Since counter values
> + * are _not_ saved/restored in idle path, can lead to
> + * below "Can't find PMC" message.
> + */
> if (unlikely(!found) && !arch_irq_disabled_regs(regs))
> printk_ratelimited(KERN_WARNING "Can't find PMC that caused IRQ\n");
>
>
--
Thanks and Regards
R.Nageswara Sastry
^ permalink raw reply
* Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions
From: Christian König @ 2021-07-21 6:29 UTC (permalink / raw)
To: Anson Jacob, mpe, benh, paulus, christophe.leroy, linuxppc-dev,
amd-gfx, linux-kernel
Cc: Sunpeng.Li, Harry.Wentland, qingqing.zhuo, Rodrigo.Siqueira,
roman.li, Christoph Hellwig, Aurabindo.Pillai, Bhawanpreet.Lakha,
bindu.r
In-Reply-To: <20210721044801.840501-3-Anson.Jacob@amd.com>
Am 21.07.21 um 06:48 schrieb Anson Jacob:
> Use kernel_fpu_begin & kernel_fpu_end for PPC
>
> Depends on "ppc/fpu: Add generic FPU api similar to x86"
>
> v2:
> - Got rid of macro switch for PPC as header file with same
> name as x86 is added by previous patch in the series
>
> Signed-off-by: Anson Jacob <Anson.Jacob@amd.com>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> CC: Harry Wentland <harry.wentland@amd.com>
> CC: Christian König <christian.koenig@amd.com>
Looks good in general, but question is what about other architectures
like ARM?
Regards,
Christian.
> ---
> drivers/gpu/drm/amd/display/dc/os_types.h | 29 -----------------------
> 1 file changed, 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 126c2f3a4dd3..47ef434f93d8 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -51,38 +51,9 @@
> #define dm_error(fmt, ...) DRM_ERROR(fmt, ##__VA_ARGS__)
>
> #if defined(CONFIG_DRM_AMD_DC_DCN)
> -#if defined(CONFIG_X86)
> #include <asm/fpu/api.h>
> #define DC_FP_START() kernel_fpu_begin()
> #define DC_FP_END() kernel_fpu_end()
> -#elif defined(CONFIG_PPC64)
> -#include <asm/switch_to.h>
> -#include <asm/cputable.h>
> -#define DC_FP_START() { \
> - if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
> - preempt_disable(); \
> - enable_kernel_vsx(); \
> - } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
> - preempt_disable(); \
> - enable_kernel_altivec(); \
> - } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
> - preempt_disable(); \
> - enable_kernel_fp(); \
> - } \
> -}
> -#define DC_FP_END() { \
> - if (cpu_has_feature(CPU_FTR_VSX_COMP)) { \
> - disable_kernel_vsx(); \
> - preempt_enable(); \
> - } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) { \
> - disable_kernel_altivec(); \
> - preempt_enable(); \
> - } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) { \
> - disable_kernel_fp(); \
> - preempt_enable(); \
> - } \
> -}
> -#endif
> #endif
>
> /*
^ permalink raw reply
* Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions
From: Christoph Hellwig @ 2021-07-21 6:51 UTC (permalink / raw)
To: Christian K??nig
Cc: Bhawanpreet.Lakha, Harry.Wentland, Anson Jacob, qingqing.zhuo,
Rodrigo.Siqueira, linux-kernel, roman.li, Christoph Hellwig,
Sunpeng.Li, Aurabindo.Pillai, paulus, amd-gfx, linuxppc-dev,
bindu.r
In-Reply-To: <9094cea1-602e-6e71-4ff7-c06fd04af6c5@amd.com>
On Wed, Jul 21, 2021 at 08:29:43AM +0200, Christian K??nig wrote:
> Looks good in general, but question is what about other architectures like
> ARM?
DRM_AMD_DC_DCN currently requires X86 || PPC64.
Maybe a good think would be to add a new KERNEL_FPU_API Kconfig symbol,
selected by x86 and powerpc (I think ppc32 should be fine too now) so
that we get these arch dependencies out of the driver.
^ permalink raw reply
* Re: [RFC v2 1/2] ppc/fpu: Add generic FPU api similar to x86
From: Christoph Hellwig @ 2021-07-21 6:58 UTC (permalink / raw)
To: Anson Jacob
Cc: Bhawanpreet.Lakha, Harry Wentland, Sunpeng.Li, qingqing.zhuo,
Rodrigo.Siqueira, linux-kernel, amd-gfx, roman.li,
Christoph Hellwig, Aurabindo.Pillai, paulus, linuxppc-dev,
Christian K??nig, bindu.r
In-Reply-To: <20210721044801.840501-2-Anson.Jacob@amd.com>
> +
> +/*
> + * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> + * disables preemption so be careful if you intend to use it for long periods
> + * of time.
> + * TODO: If you intend to use the FPU in irq/softirq you need to check first with
> + * irq_fpu_usable() if it is possible.
Please avoid the overly lone lines comments.
> +extern bool kernel_fpu_enabled(void);
> +extern void kernel_fpu_begin(void);
> +extern void kernel_fpu_end(void);
No need for the externs.
> +/*
> + * Track whether the kernel is using the FPU state
> + * currently.
This all fits on a single line.
> +static bool fpu_support(void)
> +{
> + if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
> + return true;
> + } else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
> + return true;
> + } else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
> + return true;
> + }
No need for the braces, or else after a return. In fact this could
be simplified down to:
return cpu_has_feature(CPU_FTR_VSX_COMP) ||
cpu_has_feature(CPU_FTR_ALTIVEC_COMP) ||
cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE));
> + preempt_disable();
> +
> +#ifdef CONFIG_VSX
> + if (cpu_has_feature(CPU_FTR_VSX_COMP)) {
> + enable_kernel_vsx();
> + return;
> + }
> +#endif
> +
> +#ifdef CONFIG_ALTIVEC
> + if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP)) {
> + enable_kernel_altivec();
> + return;
> + }
> +#endif
> +
> +#ifdef CONFIG_PPC_FPU
> + if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE)) {
> + enable_kernel_fp();
> + return;
> + }
> +#endif
All the features are defined away if not supported (and we already rely
on that in fpu_support()). So this could become:
if (cpu_has_feature(CPU_FTR_VSX_COMP))
enable_kernel_vsx();
else if (cpu_has_feature(CPU_FTR_ALTIVEC_COMP))
enable_kernel_altivec();
else if (!cpu_has_feature(CPU_FTR_FPU_UNAVAILABLE))
enable_kernel_fp();
Same for the disable path.
^ permalink raw reply
* [PATCH v2] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"
From: Michael Ellerman @ 2021-07-21 7:02 UTC (permalink / raw)
To: Christophe Leroy, Will Deacon
Cc: Mark Rutland, Jonathan Marek, Marc Zyngier, linux-kernel,
Nicholas Piggin, Ard Biesheuvel, Geert Uytterhoeven,
linux-arm-kernel, Catalin Marinas, Paul Mackerras, Andrew Morton,
linuxppc-dev, Thomas Gleixner, Mike Rapoport
In-Reply-To: <20210720202627.Horde.vlszNhxkKrLIg0-3Sn2ucw5@messagerie.c-s.fr>
From: Jonathan Marek <jonathan@marek.ca>
This reverts commit c742199a014de23ee92055c2473d91fe5561ffdf.
c742199a014d ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
breaks arm64 in at least two ways for configurations where PUD or PMD
folding occur:
1. We no longer install huge-vmap mappings and silently fall back to
page-granular entries, despite being able to install block entries
at what is effectively the PGD level.
2. If the linear map is backed with block mappings, these will now
silently fail to be created in alloc_init_pud(), causing a panic
early during boot.
The pgtable selftests caught this, although a fix has not been
forthcoming and Christophe is AWOL at the moment, so just revert the
change for now to get a working -rc3 on which we can queue patches for
5.15.
A simple revert breaks the build for 32-bit PowerPC 8xx machines, which
rely on the default function definitions when the corresponding
page-table levels are folded, since commit a6a8f7c4aa7e ("powerpc/8xx:
add support for huge pages on VMAP and VMALLOC"), eg:
powerpc64-linux-ld: mm/vmalloc.o: in function `vunmap_pud_range':
linux/mm/vmalloc.c:362: undefined reference to `pud_clear_huge'
To avoid that, add stubs for pud_clear_huge() and pmd_clear_huge() in
arch/powerpc/mm/nohash/8xx.c as suggested by Christophe.
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: c742199a014d ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
Signed-off-by: Will Deacon <will@kernel.org>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Acked-by: Marc Zyngier <maz@kernel.org>
[mpe: Fold in 8xx.c changes from Christophe and mention in change log]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/linux-arm-kernel/CAMuHMdXShORDox-xxaeUfDW3wx2PeggFSqhVSHVZNKCGK-y_vQ@mail.gmail.com/
Link: https://lore.kernel.org/r/20210717160118.9855-1-jonathan@marek.ca
---
arch/arm64/mm/mmu.c | 20 ++++++++------------
arch/powerpc/mm/nohash/8xx.c | 10 ++++++++++
arch/x86/mm/pgtable.c | 34 +++++++++++++++-------------------
include/linux/pgtable.h | 26 +-------------------------
4 files changed, 34 insertions(+), 56 deletions(-)
v2: Fold in suggestion from Christophe to add stubs for 8xx.
I kept the reviewed-by/acked-by tags from arm64 folks as the patch is
unchanged as far as arm64 is concerned.
Please take this via the arm64 tree.
cheers
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index d74586508448..9ff0de1b2b93 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1339,7 +1339,6 @@ void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
return dt_virt;
}
-#if CONFIG_PGTABLE_LEVELS > 3
int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
{
pud_t new_pud = pfn_pud(__phys_to_pfn(phys), mk_pud_sect_prot(prot));
@@ -1354,16 +1353,6 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot)
return 1;
}
-int pud_clear_huge(pud_t *pudp)
-{
- if (!pud_sect(READ_ONCE(*pudp)))
- return 0;
- pud_clear(pudp);
- return 1;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
{
pmd_t new_pmd = pfn_pmd(__phys_to_pfn(phys), mk_pmd_sect_prot(prot));
@@ -1378,6 +1367,14 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot)
return 1;
}
+int pud_clear_huge(pud_t *pudp)
+{
+ if (!pud_sect(READ_ONCE(*pudp)))
+ return 0;
+ pud_clear(pudp);
+ return 1;
+}
+
int pmd_clear_huge(pmd_t *pmdp)
{
if (!pmd_sect(READ_ONCE(*pmdp)))
@@ -1385,7 +1382,6 @@ int pmd_clear_huge(pmd_t *pmdp)
pmd_clear(pmdp);
return 1;
}
-#endif
int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
{
diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c
index 60780e089118..0df9fe29dd56 100644
--- a/arch/powerpc/mm/nohash/8xx.c
+++ b/arch/powerpc/mm/nohash/8xx.c
@@ -240,3 +240,13 @@ void __init setup_kuap(bool disabled)
mtspr(SPRN_MD_AP, MD_APG_KUAP);
}
#endif
+
+int pud_clear_huge(pud_t *pud)
+{
+ return 0;
+}
+
+int pmd_clear_huge(pmd_t *pmd)
+{
+ return 0;
+}
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 3364fe62b903..3481b35cb4ec 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -682,7 +682,6 @@ int p4d_clear_huge(p4d_t *p4d)
}
#endif
-#if CONFIG_PGTABLE_LEVELS > 3
/**
* pud_set_huge - setup kernel PUD mapping
*
@@ -721,23 +720,6 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
return 1;
}
-/**
- * pud_clear_huge - clear kernel PUD mapping when it is set
- *
- * Returns 1 on success and 0 on failure (no PUD map is found).
- */
-int pud_clear_huge(pud_t *pud)
-{
- if (pud_large(*pud)) {
- pud_clear(pud);
- return 1;
- }
-
- return 0;
-}
-#endif
-
-#if CONFIG_PGTABLE_LEVELS > 2
/**
* pmd_set_huge - setup kernel PMD mapping
*
@@ -768,6 +750,21 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
return 1;
}
+/**
+ * pud_clear_huge - clear kernel PUD mapping when it is set
+ *
+ * Returns 1 on success and 0 on failure (no PUD map is found).
+ */
+int pud_clear_huge(pud_t *pud)
+{
+ if (pud_large(*pud)) {
+ pud_clear(pud);
+ return 1;
+ }
+
+ return 0;
+}
+
/**
* pmd_clear_huge - clear kernel PMD mapping when it is set
*
@@ -782,7 +779,6 @@ int pmd_clear_huge(pmd_t *pmd)
return 0;
}
-#endif
#ifdef CONFIG_X86_64
/**
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index d147480cdefc..e24d2c992b11 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1397,34 +1397,10 @@ static inline int p4d_clear_huge(p4d_t *p4d)
}
#endif /* !__PAGETABLE_P4D_FOLDED */
-#ifndef __PAGETABLE_PUD_FOLDED
int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot);
-int pud_clear_huge(pud_t *pud);
-#else
-static inline int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
-{
- return 0;
-}
-static inline int pud_clear_huge(pud_t *pud)
-{
- return 0;
-}
-#endif /* !__PAGETABLE_PUD_FOLDED */
-
-#ifndef __PAGETABLE_PMD_FOLDED
int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot);
+int pud_clear_huge(pud_t *pud);
int pmd_clear_huge(pmd_t *pmd);
-#else
-static inline int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
-{
- return 0;
-}
-static inline int pmd_clear_huge(pmd_t *pmd)
-{
- return 0;
-}
-#endif /* !__PAGETABLE_PMD_FOLDED */
-
int p4d_free_pud_page(p4d_t *p4d, unsigned long addr);
int pud_free_pmd_page(pud_t *pud, unsigned long addr);
int pmd_free_pte_page(pmd_t *pmd, unsigned long addr);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 1/2] KVM: PPC: Book3S: Fix CONFIG_TRANSACTIONAL_MEM=n crash
From: Michael Ellerman @ 2021-07-21 7:38 UTC (permalink / raw)
To: kvm-ppc, Nicholas Piggin; +Cc: Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <20210716024310.164448-1-npiggin@gmail.com>
On Fri, 16 Jul 2021 12:43:09 +1000, Nicholas Piggin wrote:
> When running CPU_FTR_P9_TM_HV_ASSIST, HFSCR[TM] is set for the guest
> even if the host has CONFIG_TRANSACTIONAL_MEM=n, which causes it to be
> unprepared to handle guest exits while transactional.
>
> Normal guests don't have a problem because the HTM capability will not
> be advertised, but a rogue or buggy one could crash the host.
Applied to powerpc/fixes.
[1/2] KVM: PPC: Book3S: Fix CONFIG_TRANSACTIONAL_MEM=n crash
https://git.kernel.org/powerpc/c/bd31ecf44b8e18ccb1e5f6b50f85de6922a60de3
[2/2] KVM: PPC: Fix kvm_arch_vcpu_ioctl vcpu_load leak
https://git.kernel.org/powerpc/c/bc4188a2f56e821ea057aca6bf444e138d06c252
cheers
^ permalink raw reply
* Re: [RFC v2 2/2] drm/amd/display: Use PPC FPU functions
From: Christian König @ 2021-07-21 9:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Harry.Wentland, Anson Jacob, qingqing.zhuo, Rodrigo.Siqueira,
linux-kernel, roman.li, Bhawanpreet.Lakha, Sunpeng.Li,
Aurabindo.Pillai, paulus, amd-gfx, linuxppc-dev, bindu.r
In-Reply-To: <YPfECUtD0SRCjrSm@infradead.org>
Am 21.07.21 um 08:51 schrieb Christoph Hellwig:
> On Wed, Jul 21, 2021 at 08:29:43AM +0200, Christian K??nig wrote:
>> Looks good in general, but question is what about other architectures like
>> ARM?
> DRM_AMD_DC_DCN currently requires X86 || PPC64.
And exactly that's the problem I'm noting here. At least officially AMD
claims that we support ARM and some very brave still use the hardware
together with MIPS as well.
> Maybe a good think would be to add a new KERNEL_FPU_API Kconfig symbol,
> selected by x86 and powerpc (I think ppc32 should be fine too now) so
> that we get these arch dependencies out of the driver.
Good idea.
Christian.
^ permalink raw reply
* Re: [PATCH v2] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"
From: Will Deacon @ 2021-07-21 10:47 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman
Cc: Mark Rutland, Marc Zyngier, linux-kernel, Jonathan Marek,
Will Deacon, catalin.marinas, linuxppc-dev, Nicholas Piggin,
Ard Biesheuvel, Geert Uytterhoeven, linux-arm-kernel,
Paul Mackerras, Thomas Gleixner, kernel-team, Andrew Morton,
Mike Rapoport
In-Reply-To: <87r1fs1762.fsf@mpe.ellerman.id.au>
On Wed, 21 Jul 2021 17:02:13 +1000, Michael Ellerman wrote:
> This reverts commit c742199a014de23ee92055c2473d91fe5561ffdf.
>
> c742199a014d ("mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge")
> breaks arm64 in at least two ways for configurations where PUD or PMD
> folding occur:
>
> 1. We no longer install huge-vmap mappings and silently fall back to
> page-granular entries, despite being able to install block entries
> at what is effectively the PGD level.
>
> [...]
Thank you Michael! I owe you a beer next time I see you, if we don't go
extinct before then.
Applied to arm64 (for-next/fixes), thanks!
[1/1] Revert "mm/pgtable: add stubs for {pmd/pub}_{set/clear}_huge"
https://git.kernel.org/arm64/c/d8a719059b9d
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
^ permalink raw reply
* Re: [PATCH printk v4 3/6] printk: remove safe buffers
From: Petr Mladek @ 2021-07-21 11:25 UTC (permalink / raw)
To: John Ogness
Cc: Kees Cook, Paul E. McKenney, Alexey Kardashevskiy,
Nicholas Piggin, linux-kernel, Steven Rostedt, kexec,
Sergey Senozhatsky, Yue Hu, Paul Mackerras, Eric Biederman,
Thomas Gleixner, linuxppc-dev, Andrew Morton, Tiezhu Yang,
Cédric Le Goater
In-Reply-To: <20210715193359.25946-4-john.ogness@linutronix.de>
On Thu 2021-07-15 21:39:56, John Ogness wrote:
> With @logbuf_lock removed, the high level printk functions for
> storing messages are lockless. Messages can be stored from any
> context, so there is no need for the NMI and safe buffers anymore.
> Remove the NMI and safe buffers.
>
> Although the safe buffers are removed, the NMI and safe context
> tracking is still in place. In these contexts, store the message
> immediately but still use irq_work to defer the console printing.
>
> Since printk recursion tracking is in place, safe context tracking
> for most of printk is not needed. Remove it. Only safe context
> tracking relating to the console and console_owner locks is left
> in place. This is because the console and console_owner locks are
> needed for the actual printing.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH printk v4 4/6] printk: remove NMI tracking
From: Petr Mladek @ 2021-07-21 12:00 UTC (permalink / raw)
To: John Ogness
Cc: Peter Zijlstra, Rasmus Villemoes, Wolfram Sang (Renesas),
Paul Mackerras, Daniel Borkmann, Marc Zyngier, Masahiro Yamada,
Russell King, Ingo Molnar, Geert Uytterhoeven, linux-arm-kernel,
Valentin Schneider, Kees Cook, Anshuman Khandual, Vipin Sharma,
Frederic Weisbecker, Steven Rostedt, Nathan Chancellor,
Nick Terrell, Thomas Gleixner, Vlastimil Babka, Xiongwei Song,
Nick Desaulniers, linux-kernel, Sergey Senozhatsky, Andrew Morton,
linuxppc-dev, Mike Rapoport
In-Reply-To: <20210715193359.25946-5-john.ogness@linutronix.de>
On Thu 2021-07-15 21:39:57, John Ogness wrote:
> All NMI contexts are handled the same as the safe context: store the
> message and defer printing. There is no need to have special NMI
> context tracking for this. Using in_nmi() is enough.
>
> There are several parts of the kernel that are manually calling into
> the printk NMI context tracking in order to cause general printk
> deferred printing:
>
> arch/arm/kernel/smp.c
> arch/powerpc/kexec/crash.c
> kernel/trace/trace.c
>
> For these users, provide a new function pair
> printk_deferred_enter/exit that explicitly achieves the same
> objective.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> arch/arm/kernel/smp.c | 4 ++--
> arch/powerpc/kexec/crash.c | 2 +-
> include/linux/hardirq.h | 2 --
> include/linux/printk.h | 31 +++++++++++++++++++------------
> init/Kconfig | 5 -----
> kernel/printk/internal.h | 8 --------
> kernel/printk/printk_safe.c | 37 +------------------------------------
> kernel/trace/trace.c | 4 ++--
> 8 files changed, 25 insertions(+), 68 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index c7bb168b0d97..842427ff2b3c 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -667,9 +667,9 @@ static void do_handle_IPI(int ipinr)
> break;
>
> case IPI_CPU_BACKTRACE:
> - printk_nmi_enter();
> + printk_deferred_enter();
> nmi_cpu_backtrace(get_irq_regs());
> - printk_nmi_exit();
> + printk_deferred_exit();
> break;
>
> default:
> diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
> index 0196d0c211ac..1070378c8e35 100644
> --- a/arch/powerpc/kexec/crash.c
> +++ b/arch/powerpc/kexec/crash.c
> @@ -313,7 +313,7 @@ void default_machine_crash_shutdown(struct pt_regs *regs)
> int (*old_handler)(struct pt_regs *regs);
>
> /* Avoid hardlocking with irresponsive CPU holding logbuf_lock */
> - printk_nmi_enter();
> + printk_deferred_enter();
>
> /*
> * This function is only called after the system
> diff --git a/include/linux/hardirq.h b/include/linux/hardirq.h
> index 69bc86ea382c..76878b357ffa 100644
> --- a/include/linux/hardirq.h
> +++ b/include/linux/hardirq.h
> @@ -116,7 +116,6 @@ extern void rcu_nmi_exit(void);
> do { \
> lockdep_off(); \
> arch_nmi_enter(); \
> - printk_nmi_enter(); \
> BUG_ON(in_nmi() == NMI_MASK); \
> __preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET); \
> } while (0)
> @@ -135,7 +134,6 @@ extern void rcu_nmi_exit(void);
> do { \
> BUG_ON(!in_nmi()); \
> __preempt_count_sub(NMI_OFFSET + HARDIRQ_OFFSET); \
> - printk_nmi_exit(); \
> arch_nmi_exit(); \
> lockdep_on(); \
> } while (0)
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 664612f75dac..03f7ccedaf18 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -149,18 +149,6 @@ static inline __printf(1, 2) __cold
> void early_printk(const char *s, ...) { }
> #endif
>
> -#ifdef CONFIG_PRINTK_NMI
> -extern void printk_nmi_enter(void);
> -extern void printk_nmi_exit(void);
> -extern void printk_nmi_direct_enter(void);
> -extern void printk_nmi_direct_exit(void);
> -#else
> -static inline void printk_nmi_enter(void) { }
> -static inline void printk_nmi_exit(void) { }
> -static inline void printk_nmi_direct_enter(void) { }
> -static inline void printk_nmi_direct_exit(void) { }
> -#endif /* PRINTK_NMI */
> -
> struct dev_printk_info;
>
> #ifdef CONFIG_PRINTK
> @@ -180,6 +168,16 @@ int printk(const char *fmt, ...);
> */
> __printf(1, 2) __cold int printk_deferred(const char *fmt, ...);
>
> +extern void __printk_safe_enter(void);
> +extern void __printk_safe_exit(void);
> +/*
> + * The printk_deferred_enter/exit macros are available only as a hack for
> + * some code paths that need to defer all printk console printing. Interrupts
> + * must be disabled for the deferred duration.
> + */
> +#define printk_deferred_enter __printk_safe_enter
> +#define printk_deferred_exit __printk_safe_exit
> +
> /*
> * Please don't use printk_ratelimit(), because it shares ratelimiting state
> * with all other unrelated printk_ratelimit() callsites. Instead use
> @@ -223,6 +221,15 @@ int printk_deferred(const char *s, ...)
> {
> return 0;
> }
> +
> +static inline void printk_deferred_enter(void)
> +{
> +}
> +
> +static inline void printk_deferred_exit(void)
> +{
> +}
> +
> static inline int printk_ratelimit(void)
> {
> return 0;
> diff --git a/init/Kconfig b/init/Kconfig
> index a61c92066c2e..9c0510693543 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1506,11 +1506,6 @@ config PRINTK
> very difficult to diagnose system problems, saying N here is
> strongly discouraged.
>
> -config PRINTK_NMI
> - def_bool y
> - depends on PRINTK
> - depends on HAVE_NMI
> -
> config BUG
> bool "BUG() support" if EXPERT
> default y
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 6cc35c5de890..b6d310c72fc9 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -6,12 +6,6 @@
>
> #ifdef CONFIG_PRINTK
>
> -#define PRINTK_SAFE_CONTEXT_MASK 0x007ffffff
> -#define PRINTK_NMI_DIRECT_CONTEXT_MASK 0x008000000
> -#define PRINTK_NMI_CONTEXT_MASK 0xff0000000
> -
> -#define PRINTK_NMI_CONTEXT_OFFSET 0x010000000
> -
> __printf(4, 0)
> int vprintk_store(int facility, int level,
> const struct dev_printk_info *dev_info,
> @@ -19,8 +13,6 @@ int vprintk_store(int facility, int level,
>
> __printf(1, 0) int vprintk_default(const char *fmt, va_list args);
> __printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
> -void __printk_safe_enter(void);
> -void __printk_safe_exit(void);
>
> bool printk_percpu_data_ready(void);
>
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index 29c580dac93d..ef0f9a2044da 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -4,12 +4,9 @@
> */
>
> #include <linux/preempt.h>
> -#include <linux/spinlock.h>
> -#include <linux/debug_locks.h>
> #include <linux/kdb.h>
> #include <linux/smp.h>
> #include <linux/cpumask.h>
> -#include <linux/irq_work.h>
> #include <linux/printk.h>
> #include <linux/kprobes.h>
>
> @@ -17,35 +14,6 @@
>
> static DEFINE_PER_CPU(int, printk_context);
>
> -#ifdef CONFIG_PRINTK_NMI
> -void noinstr printk_nmi_enter(void)
> -{
> - this_cpu_add(printk_context, PRINTK_NMI_CONTEXT_OFFSET);
> -}
> -
> -void noinstr printk_nmi_exit(void)
> -{
> - this_cpu_sub(printk_context, PRINTK_NMI_CONTEXT_OFFSET);
> -}
> -
> -/*
> - * Marks a code that might produce many messages in NMI context
> - * and the risk of losing them is more critical than eventual
> - * reordering.
> - */
> -void printk_nmi_direct_enter(void)
> -{
> - if (this_cpu_read(printk_context) & PRINTK_NMI_CONTEXT_MASK)
> - this_cpu_or(printk_context, PRINTK_NMI_DIRECT_CONTEXT_MASK);
> -}
> -
> -void printk_nmi_direct_exit(void)
> -{
> - this_cpu_and(printk_context, ~PRINTK_NMI_DIRECT_CONTEXT_MASK);
> -}
> -
> -#endif /* CONFIG_PRINTK_NMI */
> -
> /* Can be preempted by NMI. */
> void __printk_safe_enter(void)
> {
> @@ -70,10 +38,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
> * Use the main logbuf even in NMI. But avoid calling console
> * drivers that might have their own locks.
> */
> - if (this_cpu_read(printk_context) &
> - (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> - PRINTK_NMI_CONTEXT_MASK |
> - PRINTK_SAFE_CONTEXT_MASK)) {
> + if (this_cpu_read(printk_context) || in_nmi()) {
> int len;
>
> len = vprintk_store(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d23a09d3eb37..b30ad20d251f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9647,7 +9647,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
> tracing_off();
>
> local_irq_save(flags);
> - printk_nmi_direct_enter();
> + printk_deferred_enter();
I would prefer to do not manipulate the printk context here anymore,
as it was done in v3.
printk_nmi_direct_enter() was added here by the commit the commit
03fc7f9c99c1e7ae2925d4 ("printk/nmi: Prevent deadlock when
accessing the main log buffer in NMI"). It was _not_ about console
handling. The reason was to modify the default behavior under NMI
and store the messages directly into the main log buffer.
When I think about it. The original fix was not correct. We should have
modified the context only when ftrace_dump() was really called under NMI:
if (in_nmi())
printk_nmi_direct_enter();
By other words. We should try to show the messages on the console
when ftrace_dump()/panic() is not called from NMI. It will help
to see all messages even when the ftrace buffers are bigger
than printk() ones.
And we do not need any special handling here for NMI. vprintk()
in printk/printk_safe.c will do the right thing for us.
Best Regards,
Petr
PS: There is no need to re-send the entire patchset if this was
the only problem. Feel free to send v4.1 version of this
patch only.
That said, I still have to look at 5th and 6th patch.
^ permalink raw reply
* Re: [PATCH printk v4 4/6] printk: remove NMI tracking
From: John Ogness @ 2021-07-21 12:46 UTC (permalink / raw)
To: Petr Mladek
Cc: Peter Zijlstra, Rasmus Villemoes, Wolfram Sang (Renesas),
Paul Mackerras, Daniel Borkmann, Marc Zyngier, Masahiro Yamada,
Russell King, Ingo Molnar, Geert Uytterhoeven, linux-arm-kernel,
Valentin Schneider, Kees Cook, Anshuman Khandual, Vipin Sharma,
Frederic Weisbecker, Steven Rostedt, Nathan Chancellor,
Nick Terrell, Thomas Gleixner, Vlastimil Babka, Xiongwei Song,
Nick Desaulniers, linux-kernel, Sergey Senozhatsky, Andrew Morton,
linuxppc-dev, Mike Rapoport
In-Reply-To: <20210721120026.y3dqno24ahw4sazy@pathway.suse.cz>
On 2021-07-21, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -9647,7 +9647,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>> tracing_off();
>>
>> local_irq_save(flags);
>> - printk_nmi_direct_enter();
>> + printk_deferred_enter();
>
> I would prefer to do not manipulate the printk context here anymore,
> as it was done in v3.
>
> printk_nmi_direct_enter() was added here by the commit the commit
> 03fc7f9c99c1e7ae2925d4 ("printk/nmi: Prevent deadlock when
> accessing the main log buffer in NMI"). It was _not_ about console
> handling. The reason was to modify the default behavior under NMI
> and store the messages directly into the main log buffer.
>
> When I think about it. The original fix was not correct. We should have
> modified the context only when ftrace_dump() was really called under NMI:
>
> if (in_nmi())
> printk_nmi_direct_enter();
>
> By other words. We should try to show the messages on the console
> when ftrace_dump()/panic() is not called from NMI. It will help
> to see all messages even when the ftrace buffers are bigger
> than printk() ones.
>
> And we do not need any special handling here for NMI. vprintk()
> in printk/printk_safe.c will do the right thing for us.
Agreed. We need to mention this behavior change in the commit
message. Perhaps this as the commit message:
All NMI contexts are handled the same as the safe context: store the
message and defer printing. There is no need to have special NMI
context tracking for this. Using in_nmi() is enough.
There are several parts of the kernel that are manually calling into
the printk NMI context tracking in order to cause general printk
deferred printing:
arch/arm/kernel/smp.c
arch/powerpc/kexec/crash.c
kernel/trace/trace.c
For arm/kernel/smp.c and powerpc/kexec/crash.c, provide a new
function pair printk_deferred_enter/exit that explicitly achieves the
same objective.
For ftrace, remove general printk deferring. This general deferrment
was added in commit 03fc7f9c99c1 ("printk/nmi: Prevent deadlock when
accessing the main log buffer in NMI"), but really should have only
been deferred when in NMI context. Since vprintk() now checks for
NMI context when deciding to defer, ftrace does not need any special
handling.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
^ permalink raw reply
* Re: [PATCH v4 0/5] bus: Make remove callback return void
From: Greg Kroah-Hartman @ 2021-07-21 10:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: nvdimm, linux-sh, Samuel Iglesias Gonsalvez, Jens Taprogge,
Ulf Hansson, Jaroslav Kysela, Benjamin Tissoires, Paul Mackerras,
Srinivas Pandruvada, K. Y. Srinivasan, Mike Christie, Wei Liu,
Maxim Levitsky, Samuel Holland, Halil Pasic, linux-acpi,
Geert Uytterhoeven, linux-pci, xen-devel, Tomas Winkler,
Julien Grall, Ohad Ben-Cohen, Yufen Yu, Alex Williamson,
Alex Elder, linux-parisc, Finn Thain, Geoff Levand, linux-fpga,
linux-usb, Rafael J. Wysocki, linux-kernel, linux-spi,
Kai-Heng Feng, kernel, Jon Mason, linux-ntb, Wu Hao,
David Woodhouse, Krzysztof Wilczyński, Alexandre Belloni,
Manohar Vanga, Matthew Rosato, linux-wireless, Dominik Brodowski,
virtualization, James E.J. Bottomley, target-devel,
Srinivas Kandagatla, linux-i2c, linux-s390, Stefano Stabellini,
Stephen Hemminger, Yoshinori Sato, Jiri Slaby, Helge Deller,
Rafał Miłecki, YueHaibing, industrypack-devel,
linux-mips, Len Brown, Eric Farman, Vasily Gorbik, linux-arm-msm,
linux-media, Maxime Ripard, William Breathitt Gray, greybus-dev,
linux-m68k, Florian Fainelli, Rikard Falkeborn, Frank Li,
Mark Gross, Boris Ostrovsky, Mika Westerberg, linux-arm-kernel,
Johannes Thumshirn, Mathieu Poirier, Stephen Boyd, Cornelia Huck,
Peter Oberparleiter, Wolfram Sang, Joey Pabalan, Yehezkel Bernat,
Pali Rohár, Bodo Stroesser, Alison Schofield,
Heikki Krogerus, Tyrel Datwyler, Alexander Shishkin, Tom Rix,
Jason Wang, SeongJae Park, alsa-devel, platform-driver-x86,
Bjorn Helgaas, netdev, Vineeth Vijayan, Ira Weiny, Rob Herring,
Lorenzo Pieralisi, Dave Jiang, linux-staging, Dexuan Cui,
Jernej Skrabec, Kishon Vijay Abraham I, Christian Borntraeger,
Chen-Yu Tsai, linux-input, Allen Hubbe, Alex Dubov, Haiyang Zhang,
Jiri Kosina, Russell King (Oracle), Ben Widawsky,
Harald Freudenberger, linux-cxl, Michael Buesch, Dan Williams,
Mauro Carvalho Chehab, Cristian Marussi, Thomas Bogendoerfer,
Martin K. Petersen, Martyn Welch, Dmitry Torokhov, linux-mmc,
linux-sunxi, Stefan Richter, Sudeep Holla, David S. Miller,
Sven Van Asbroeck, Rich Felker, kvm, Michael S. Tsirkin,
linux-remoteproc, Bjorn Andersson, sparclinux, Kirti Wankhede,
Andreas Noever, linux-i3c, linux1394-devel, Lee Jones,
Arnd Bergmann, linux-scsi, Marc Zyngier, Russell King,
Thorsten Scherer, Andy Gross, linux-serial, Jakub Kicinski,
linux-hyperv, Michael Jamet, Heiko Carstens, Johan Hovold,
Hans de Goede, Hannes Reinecke, Juergen Gross, linuxppc-dev,
Takashi Iwai, Alexandre Bounine, Vinod Koul, Mark Brown,
Vishal Verma, dmaengine, Moritz Fischer, Johannes Berg,
Maximilian Luz
In-Reply-To: <20210713193522.1770306-1-u.kleine-koenig@pengutronix.de>
On Tue, Jul 13, 2021 at 09:35:17PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> this is v4 of the final patch set for my effort to make struct
> bus_type::remove return void.
>
> The first four patches contain cleanups that make some of these
> callbacks (more obviously) always return 0. They are acked by the
> respective maintainers. Bjorn Helgaas explicitly asked to include the
> pci patch (#1) into this series, so Greg taking this is fine. I assume
> the s390 people are fine with Greg taking patches #2 to #4, too, they
> didn't explicitly said so though.
>
> The last patch actually changes the prototype and so touches quite some
> drivers and has the potential to conflict with future developments, so I
> consider it beneficial to put these patches into next soon. I expect
> that it will be Greg who takes the complete series, he already confirmed
> via irc (for v2) to look into this series.
>
> The only change compared to v3 is in the fourth patch where I modified a
> few more drivers to fix build failures. Some of them were found by build
> bots (thanks!), some of them I found myself using a regular expression
> search. The newly modified files are:
>
> arch/sparc/kernel/vio.c
> drivers/nubus/bus.c
> drivers/sh/superhyway/superhyway.c
> drivers/vlynq/vlynq.c
> drivers/zorro/zorro-driver.c
> sound/ac97/bus.c
>
> Best regards
> Uwe
Now queued up. I can go make a git tag that people can pull from after
0-day is finished testing this to verify all is good, if others need it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH printk v4 4/6] printk: remove NMI tracking
From: Petr Mladek @ 2021-07-21 13:08 UTC (permalink / raw)
To: John Ogness
Cc: Peter Zijlstra, Rasmus Villemoes, Wolfram Sang (Renesas),
Paul Mackerras, Daniel Borkmann, Marc Zyngier, Masahiro Yamada,
Russell King, Ingo Molnar, Geert Uytterhoeven, linux-arm-kernel,
Valentin Schneider, Kees Cook, Anshuman Khandual, Vipin Sharma,
Frederic Weisbecker, Steven Rostedt, Nathan Chancellor,
Nick Terrell, Thomas Gleixner, Vlastimil Babka, Xiongwei Song,
Nick Desaulniers, linux-kernel, Sergey Senozhatsky, Andrew Morton,
linuxppc-dev, Mike Rapoport
In-Reply-To: <877dhjygvc.fsf@jogness.linutronix.de>
On Wed 2021-07-21 14:52:15, John Ogness wrote:
> On 2021-07-21, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/trace/trace.c
> >> +++ b/kernel/trace/trace.c
> >> @@ -9647,7 +9647,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
> >> tracing_off();
> >>
> >> local_irq_save(flags);
> >> - printk_nmi_direct_enter();
> >> + printk_deferred_enter();
> >
> > I would prefer to do not manipulate the printk context here anymore,
> > as it was done in v3.
> >
> > printk_nmi_direct_enter() was added here by the commit the commit
> > 03fc7f9c99c1e7ae2925d4 ("printk/nmi: Prevent deadlock when
> > accessing the main log buffer in NMI"). It was _not_ about console
> > handling. The reason was to modify the default behavior under NMI
> > and store the messages directly into the main log buffer.
> >
> > When I think about it. The original fix was not correct. We should have
> > modified the context only when ftrace_dump() was really called under NMI:
> >
> > if (in_nmi())
> > printk_nmi_direct_enter();
> >
> > By other words. We should try to show the messages on the console
> > when ftrace_dump()/panic() is not called from NMI. It will help
> > to see all messages even when the ftrace buffers are bigger
> > than printk() ones.
> >
> > And we do not need any special handling here for NMI. vprintk()
> > in printk/printk_safe.c will do the right thing for us.
>
> Agreed. We need to mention this behavior change in the commit
> message. Perhaps this as the commit message:
>
> All NMI contexts are handled the same as the safe context: store the
> message and defer printing. There is no need to have special NMI
> context tracking for this. Using in_nmi() is enough.
>
> There are several parts of the kernel that are manually calling into
> the printk NMI context tracking in order to cause general printk
> deferred printing:
>
> arch/arm/kernel/smp.c
> arch/powerpc/kexec/crash.c
> kernel/trace/trace.c
>
> For arm/kernel/smp.c and powerpc/kexec/crash.c, provide a new
> function pair printk_deferred_enter/exit that explicitly achieves the
> same objective.
>
> For ftrace, remove general printk deferring. This general deferrment
> was added in commit 03fc7f9c99c1 ("printk/nmi: Prevent deadlock when
> accessing the main log buffer in NMI"), but really should have only
> been deferred when in NMI context. Since vprintk() now checks for
> NMI context when deciding to defer, ftrace does not need any special
> handling.
I would make it less focused on the deferring part and try to explain
the original purpose here, something like:
"For ftrace, remove the printk context manipulation completely. It was
added in commit 03fc7f9c99c1 ("printk/nmi: Prevent deadlock when
accessing the main log buffer in NMI"). The purpose was to enforce
storing messages directly into the ring buffer even in NMI context.
It really should have only modified the behavior in NMI context.
There is no need for a special behavior any longer. All messages are
always stored directly now. The console deferring is handled
transparently in vprintk()."
But I do not resist on it. And feel free to make it shorter.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH printk v4 4/6] printk: remove NMI tracking
From: John Ogness @ 2021-07-21 13:23 UTC (permalink / raw)
To: Petr Mladek
Cc: Peter Zijlstra, Rasmus Villemoes, Wolfram Sang (Renesas),
Paul Mackerras, Daniel Borkmann, Marc Zyngier, Masahiro Yamada,
Russell King, Ingo Molnar, Geert Uytterhoeven, linux-arm-kernel,
Valentin Schneider, Kees Cook, Anshuman Khandual, Vipin Sharma,
Frederic Weisbecker, Steven Rostedt, Nathan Chancellor,
Nick Terrell, Thomas Gleixner, Vlastimil Babka, Xiongwei Song,
Nick Desaulniers, linux-kernel, Sergey Senozhatsky, Andrew Morton,
linuxppc-dev, Mike Rapoport
In-Reply-To: <20210721130852.zrjnti6b3fwjgdzj@pathway.suse.cz>
On 2021-07-21, Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2021-07-21 14:52:15, John Ogness wrote:
>> On 2021-07-21, Petr Mladek <pmladek@suse.com> wrote:
>> >> --- a/kernel/trace/trace.c
>> >> +++ b/kernel/trace/trace.c
>> >> @@ -9647,7 +9647,7 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>> >> tracing_off();
>> >>
>> >> local_irq_save(flags);
>> >> - printk_nmi_direct_enter();
>> >> + printk_deferred_enter();
>> >
>> > I would prefer to do not manipulate the printk context here anymore,
>> > as it was done in v3.
>> >
>> > printk_nmi_direct_enter() was added here by the commit the commit
>> > 03fc7f9c99c1e7ae2925d4 ("printk/nmi: Prevent deadlock when
>> > accessing the main log buffer in NMI"). It was _not_ about console
>> > handling. The reason was to modify the default behavior under NMI
>> > and store the messages directly into the main log buffer.
>> >
>> > When I think about it. The original fix was not correct. We should have
>> > modified the context only when ftrace_dump() was really called under NMI:
>> >
>> > if (in_nmi())
>> > printk_nmi_direct_enter();
>> >
>> > By other words. We should try to show the messages on the console
>> > when ftrace_dump()/panic() is not called from NMI. It will help
>> > to see all messages even when the ftrace buffers are bigger
>> > than printk() ones.
>> >
>> > And we do not need any special handling here for NMI. vprintk()
>> > in printk/printk_safe.c will do the right thing for us.
>>
>> Agreed. We need to mention this behavior change in the commit
>> message. Perhaps this as the commit message:
>>
>> All NMI contexts are handled the same as the safe context: store the
>> message and defer printing. There is no need to have special NMI
>> context tracking for this. Using in_nmi() is enough.
>>
>> There are several parts of the kernel that are manually calling into
>> the printk NMI context tracking in order to cause general printk
>> deferred printing:
>>
>> arch/arm/kernel/smp.c
>> arch/powerpc/kexec/crash.c
>> kernel/trace/trace.c
>>
>> For arm/kernel/smp.c and powerpc/kexec/crash.c, provide a new
>> function pair printk_deferred_enter/exit that explicitly achieves the
>> same objective.
>>
>> For ftrace, remove general printk deferring. This general deferrment
>> was added in commit 03fc7f9c99c1 ("printk/nmi: Prevent deadlock when
>> accessing the main log buffer in NMI"), but really should have only
>> been deferred when in NMI context. Since vprintk() now checks for
>> NMI context when deciding to defer, ftrace does not need any special
>> handling.
>
> I would make it less focused on the deferring part and try to explain
> the original purpose here, something like:
>
> "For ftrace, remove the printk context manipulation completely. It was
> added in commit 03fc7f9c99c1 ("printk/nmi: Prevent deadlock when
> accessing the main log buffer in NMI"). The purpose was to enforce
> storing messages directly into the ring buffer even in NMI context.
> It really should have only modified the behavior in NMI context.
> There is no need for a special behavior any longer. All messages are
> always stored directly now. The console deferring is handled
> transparently in vprintk()."
Your wording is OK for me.
John Ogness
^ permalink raw reply
* Re: [PATCH v5 10/11] powerpc/pseries/iommu: Make use of DDW for indirect mapping
From: Frederic Barrat @ 2021-07-21 15:04 UTC (permalink / raw)
To: Alexey Kardashevskiy, Leonardo Bras, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <8dfb28d5-b654-746c-03d8-aeee3d438240@ozlabs.ru>
On 21/07/2021 05:32, Alexey Kardashevskiy wrote:
>>> + struct iommu_table *newtbl;
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(pci->phb->mem_resources); i++) {
>>> + const unsigned long mask = IORESOURCE_MEM_64 |
>>> IORESOURCE_MEM;
>>> +
>>> + /* Look for MMIO32 */
>>> + if ((pci->phb->mem_resources[i].flags & mask) ==
>>> IORESOURCE_MEM)
>>> + break;
>>> + }
>>> +
>>> + if (i == ARRAY_SIZE(pci->phb->mem_resources))
>>> + goto out_del_list;
>>
>>
>> So we exit and do nothing if there's no MMIO32 bar?
>> Isn't the intent just to figure out the MMIO32 area to reserve it when
>> init'ing the table? In which case we could default to 0,0
>>
>> I'm actually not clear why we are reserving this area on pseries.
>
>
>
> If we do not reserve it, then the iommu code will allocate DMA pages
> from there and these addresses are MMIO32 from the kernel pov at least.
> I saw crashes when (I think) a device tried DMAing to the top 2GB of the
> bus space which happened to be a some other device's BAR.
hmmm... then figuring out the correct range needs more work. We could
have more than one MMIO32 bar. And they don't have to be adjacent. I
don't see that we are reserving any range on the initial table though
(on pseries).
Fred
^ permalink raw reply
* [powerpc:fixes-test] BUILD SUCCESS bc4188a2f56e821ea057aca6bf444e138d06c252
From: kernel test robot @ 2021-07-21 21:15 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes-test
branch HEAD: bc4188a2f56e821ea057aca6bf444e138d06c252 KVM: PPC: Fix kvm_arch_vcpu_ioctl vcpu_load leak
elapsed time: 730m
configs tested: 147
configs skipped: 94
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
powerpc mpc836x_rdk_defconfig
powerpc mpc885_ads_defconfig
h8300 h8s-sim_defconfig
sh ecovec24-romimage_defconfig
parisc generic-64bit_defconfig
arm exynos_defconfig
s390 zfcpdump_defconfig
arm ep93xx_defconfig
mips maltasmvp_defconfig
arm corgi_defconfig
powerpc linkstation_defconfig
m68k m5407c3_defconfig
sh secureedge5410_defconfig
powerpc sequoia_defconfig
powerpc mpc5200_defconfig
powerpc mpc834x_itxgp_defconfig
sh apsh4a3a_defconfig
arm mxs_defconfig
arm pcm027_defconfig
arm tegra_defconfig
powerpc tqm8xx_defconfig
powerpc ge_imp3a_defconfig
csky alldefconfig
h8300 defconfig
sh sh7785lcr_32bit_defconfig
arm integrator_defconfig
s390 debug_defconfig
powerpc sbc8548_defconfig
powerpc storcenter_defconfig
powerpc ppc6xx_defconfig
arm lpc32xx_defconfig
arm bcm2835_defconfig
powerpc mgcoge_defconfig
ia64 defconfig
riscv alldefconfig
microblaze defconfig
m68k defconfig
sh rsk7269_defconfig
arm stm32_defconfig
arm vf610m4_defconfig
x86_64 allnoconfig
arm multi_v5_defconfig
mips cavium_octeon_defconfig
powerpc chrp32_defconfig
m68k mvme16x_defconfig
powerpc holly_defconfig
powerpc katmai_defconfig
mips mpc30x_defconfig
mips tb0287_defconfig
sh landisk_defconfig
arm socfpga_defconfig
powerpc64 defconfig
powerpc cm5200_defconfig
arm palmz72_defconfig
ia64 allmodconfig
ia64 allyesconfig
m68k allmodconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
s390 allmodconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
x86_64 randconfig-a003-20210720
x86_64 randconfig-a006-20210720
x86_64 randconfig-a001-20210720
x86_64 randconfig-a005-20210720
x86_64 randconfig-a004-20210720
x86_64 randconfig-a002-20210720
i386 randconfig-a005-20210719
i386 randconfig-a004-20210719
i386 randconfig-a006-20210719
i386 randconfig-a001-20210719
i386 randconfig-a003-20210719
i386 randconfig-a002-20210719
i386 randconfig-a005-20210720
i386 randconfig-a003-20210720
i386 randconfig-a004-20210720
i386 randconfig-a002-20210720
i386 randconfig-a001-20210720
i386 randconfig-a006-20210720
i386 randconfig-a016-20210720
i386 randconfig-a013-20210720
i386 randconfig-a012-20210720
i386 randconfig-a014-20210720
i386 randconfig-a011-20210720
i386 randconfig-a015-20210720
i386 randconfig-a016-20210721
i386 randconfig-a013-20210721
i386 randconfig-a012-20210721
i386 randconfig-a014-20210721
i386 randconfig-a011-20210721
i386 randconfig-a015-20210721
riscv nommu_k210_defconfig
riscv allyesconfig
riscv nommu_virt_defconfig
riscv allnoconfig
riscv defconfig
riscv rv32_defconfig
riscv allmodconfig
x86_64 rhel-8.3-kselftests
um x86_64_defconfig
um i386_defconfig
x86_64 allyesconfig
x86_64 defconfig
x86_64 rhel-8.3
x86_64 rhel-8.3-kbuiltin
x86_64 kexec
clang tested configs:
x86_64 randconfig-b001-20210720
x86_64 randconfig-a011-20210720
x86_64 randconfig-a016-20210720
x86_64 randconfig-a013-20210720
x86_64 randconfig-a014-20210720
x86_64 randconfig-a012-20210720
x86_64 randconfig-a015-20210720
x86_64 randconfig-a003-20210721
x86_64 randconfig-a006-20210721
x86_64 randconfig-a001-20210721
x86_64 randconfig-a005-20210721
x86_64 randconfig-a004-20210721
x86_64 randconfig-a002-20210721
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH v5 2/6] powerpc/pseries: rename distance_ref_points_depth to max_associativity_domain_index
From: David Gibson @ 2021-07-22 1:19 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <YPjC3W9BFbD+wMZz@yekko>
[-- Attachment #1: Type: text/plain, Size: 4789 bytes --]
On Thu, Jul 22, 2021 at 10:59:09AM +1000, David Gibson wrote:
> On Mon, Jun 28, 2021 at 08:41:13PM +0530, Aneesh Kumar K.V wrote:
> > No functional change in this patch
> >
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
No... wait, I take that back. This change makes the code *more*
confusing.
"distance_ref_points_depth" is accurate - it's the length of the
distance_ref_points array.
"max_associativity_domain_index" is not. That implies it's the
maximum value that a domain index can have - which it isn't. You
could have 15 entries in every associativity array, but if only 2 of
them are referenced in distance_ref_points, then
"max_associativity_domain_index" would only be 2.
>
> > ---
> > arch/powerpc/mm/numa.c | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > index 8365b298ec48..132813dd1a6c 100644
> > --- a/arch/powerpc/mm/numa.c
> > +++ b/arch/powerpc/mm/numa.c
> > @@ -56,7 +56,7 @@ static int n_mem_addr_cells, n_mem_size_cells;
> > static int form1_affinity;
> >
> > #define MAX_DISTANCE_REF_POINTS 4
> > -static int distance_ref_points_depth;
> > +static int max_associativity_domain_index;
> > static const __be32 *distance_ref_points;
> > static int distance_lookup_table[MAX_NUMNODES][MAX_DISTANCE_REF_POINTS];
> >
> > @@ -169,7 +169,7 @@ int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> >
> > int i, index;
> >
> > - for (i = 0; i < distance_ref_points_depth; i++) {
> > + for (i = 0; i < max_associativity_domain_index; i++) {
> > index = be32_to_cpu(distance_ref_points[i]);
> > if (cpu1_assoc[index] == cpu2_assoc[index])
> > break;
> > @@ -193,7 +193,7 @@ int __node_distance(int a, int b)
> > if (!form1_affinity)
> > return ((a == b) ? LOCAL_DISTANCE : REMOTE_DISTANCE);
> >
> > - for (i = 0; i < distance_ref_points_depth; i++) {
> > + for (i = 0; i < max_associativity_domain_index; i++) {
> > if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
> > break;
> >
> > @@ -213,7 +213,7 @@ static void initialize_distance_lookup_table(int nid,
> > if (!form1_affinity)
> > return;
> >
> > - for (i = 0; i < distance_ref_points_depth; i++) {
> > + for (i = 0; i < max_associativity_domain_index; i++) {
> > const __be32 *entry;
> >
> > entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> > @@ -240,7 +240,7 @@ static int associativity_to_nid(const __be32 *associativity)
> > nid = NUMA_NO_NODE;
> >
> > if (nid > 0 &&
> > - of_read_number(associativity, 1) >= distance_ref_points_depth) {
> > + of_read_number(associativity, 1) >= max_associativity_domain_index) {
> > /*
> > * Skip the length field and send start of associativity array
> > */
> > @@ -310,14 +310,14 @@ static int __init find_primary_domain_index(void)
> > */
> > distance_ref_points = of_get_property(root,
> > "ibm,associativity-reference-points",
> > - &distance_ref_points_depth);
> > + &max_associativity_domain_index);
> >
> > if (!distance_ref_points) {
> > dbg("NUMA: ibm,associativity-reference-points not found.\n");
> > goto err;
> > }
> >
> > - distance_ref_points_depth /= sizeof(int);
> > + max_associativity_domain_index /= sizeof(int);
> >
> > if (firmware_has_feature(FW_FEATURE_OPAL) ||
> > firmware_has_feature(FW_FEATURE_TYPE1_AFFINITY)) {
> > @@ -328,7 +328,7 @@ static int __init find_primary_domain_index(void)
> > if (form1_affinity) {
> > index = of_read_number(distance_ref_points, 1);
> > } else {
> > - if (distance_ref_points_depth < 2) {
> > + if (max_associativity_domain_index < 2) {
> > printk(KERN_WARNING "NUMA: "
> > "short ibm,associativity-reference-points\n");
> > goto err;
> > @@ -341,10 +341,10 @@ static int __init find_primary_domain_index(void)
> > * Warn and cap if the hardware supports more than
> > * MAX_DISTANCE_REF_POINTS domains.
> > */
> > - if (distance_ref_points_depth > MAX_DISTANCE_REF_POINTS) {
> > + if (max_associativity_domain_index > MAX_DISTANCE_REF_POINTS) {
> > printk(KERN_WARNING "NUMA: distance array capped at "
> > "%d entries\n", MAX_DISTANCE_REF_POINTS);
> > - distance_ref_points_depth = MAX_DISTANCE_REF_POINTS;
> > + max_associativity_domain_index = MAX_DISTANCE_REF_POINTS;
> > }
> >
> > of_node_put(root);
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 6/6] powerpc/pseries: Add support for FORM2 associativity
From: David Gibson @ 2021-07-22 2:28 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <20210628151117.545935-7-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 18059 bytes --]
On Mon, Jun 28, 2021 at 08:41:17PM +0530, Aneesh Kumar K.V wrote:
> PAPR interface currently supports two different ways of communicating resource
> grouping details to the OS. These are referred to as Form 0 and Form 1
> associativity grouping. Form 0 is the older format and is now considered
> deprecated. This patch adds another resource grouping named FORM2.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> Documentation/powerpc/associativity.rst | 103 ++++++++++++++
> arch/powerpc/include/asm/firmware.h | 3 +-
> arch/powerpc/include/asm/prom.h | 1 +
> arch/powerpc/kernel/prom_init.c | 3 +-
> arch/powerpc/mm/numa.c | 157 ++++++++++++++++++----
> arch/powerpc/platforms/pseries/firmware.c | 1 +
> 6 files changed, 242 insertions(+), 26 deletions(-)
> create mode 100644 Documentation/powerpc/associativity.rst
>
> diff --git a/Documentation/powerpc/associativity.rst b/Documentation/powerpc/associativity.rst
> new file mode 100644
> index 000000000000..31cc7da2c7a6
> --- /dev/null
> +++ b/Documentation/powerpc/associativity.rst
> @@ -0,0 +1,103 @@
> +============================
> +NUMA resource associativity
> +=============================
> +
> +Associativity represents the groupings of the various platform resources into
> +domains of substantially similar mean performance relative to resources outside
> +of that domain. Resources subsets of a given domain that exhibit better
> +performance relative to each other than relative to other resources subsets
> +are represented as being members of a sub-grouping domain. This performance
> +characteristic is presented in terms of NUMA node distance within the Linux kernel.
> +From the platform view, these groups are also referred to as domains.
Pretty hard to decipher, but that's typical for PAPR.
> +PAPR interface currently supports different ways of communicating these resource
> +grouping details to the OS. These are referred to as Form 0, Form 1 and Form2
> +associativity grouping. Form 0 is the older format and is now considered deprecated.
Nit: s/older/oldest/ since there are now >2 forms.
> +Hypervisor indicates the type/form of associativity used via "ibm,architecture-vec-5 property".
> +Bit 0 of byte 5 in the "ibm,architecture-vec-5" property indicates usage of Form 0 or Form 1.
> +A value of 1 indicates the usage of Form 1 associativity. For Form 2 associativity
> +bit 2 of byte 5 in the "ibm,architecture-vec-5" property is used.
> +
> +Form 0
> +-----
> +Form 0 associativity supports only two NUMA distances (LOCAL and REMOTE).
> +
> +Form 1
> +-----
> +With Form 1 a combination of ibm,associativity-reference-points, and ibm,associativity
> +device tree properties are used to determine the NUMA distance between resource groups/domains.
> +
> +The “ibm,associativity” property contains a list of one or more numbers (domainID)
> +representing the resource’s platform grouping domains.
> +
> +The “ibm,associativity-reference-points” property contains a list of one or more numbers
> +(domainID index) that represents the 1 based ordinal in the associativity lists.
> +The list of domainID indexes represents an increasing hierarchy of resource grouping.
> +
> +ex:
> +{ primary domainID index, secondary domainID index, tertiary domainID index.. }
> +
> +Linux kernel uses the domainID at the primary domainID index as the NUMA node id.
> +Linux kernel computes NUMA distance between two domains by recursively comparing
> +if they belong to the same higher-level domains. For mismatch at every higher
> +level of the resource group, the kernel doubles the NUMA distance between the
> +comparing domains.
> +
> +Form 2
> +-------
> +Form 2 associativity format adds separate device tree properties representing NUMA node distance
> +thereby making the node distance computation flexible. Form 2 also allows flexible primary
> +domain numbering. With numa distance computation now detached from the index value in
> +"ibm,associativity-reference-points" property, Form 2 allows a large number of primary domain
> +ids at the same domainID index representing resource groups of different performance/latency
> +characteristics.
> +
> +Hypervisor indicates the usage of FORM2 associativity using bit 2 of byte 5 in the
> +"ibm,architecture-vec-5" property.
> +
> +"ibm,numa-lookup-index-table" property contains a list of one or more numbers representing
> +the domainIDs present in the system. The offset of the domainID in this property is
> +used as an index while computing numa distance information via "ibm,numa-distance-table".
> +
> +prop-encoded-array: The number N of the domainIDs encoded as with encode-int, followed by
> +N domainID encoded as with encode-int
> +
> +For ex:
> +"ibm,numa-lookup-index-table" = {4, 0, 8, 250, 252}. The offset of domainID 8 (2) is used when
> +computing the distance of domain 8 from other domains present in the system. For the rest of
> +this document, this offset will be referred to as domain distance offset.
> +
> +"ibm,numa-distance-table" property contains a list of one or more numbers representing the NUMA
> +distance between resource groups/domains present in the system.
> +
> +prop-encoded-array: The number N of the distance values encoded as with encode-int, followed by
> +N distance values encoded as with encode-bytes. The max distance value we could encode is 255.
> +The number N must be equal to the square of m where m is the number of domainIDs in the
> +numa-lookup-index-table.
> +
> +For ex:
> +ibm,numa-lookup-index-table = {3, 0, 8, 40}
> +ibm,numa-distance-table = {9, 10, 20, 80, 20, 10, 160, 80, 160, 10}
This representation doesn't make it clear that the 9 is a u32, but the
rest are u8s.
> +
> + | 0 8 40
> +--|------------
> + |
> +0 | 10 20 80
> + |
> +8 | 20 10 160
> + |
> +40| 80 160 10
> +
> +A possible "ibm,associativity" property for resources in node 0, 8 and 40
> +
> +{ 3, 6, 7, 0 }
> +{ 3, 6, 9, 8 }
> +{ 3, 6, 7, 40}
> +
> +With "ibm,associativity-reference-points" { 0x3 }
You haven't actually described how ibm,associativity-reference-points
operates in Form2.
> +"ibm,lookup-index-table" helps in having a compact representation of distance matrix.
> +Since domainID can be sparse, the matrix of distances can also be effectively sparse.
> +With "ibm,lookup-index-table" we can achieve a compact representation of
> +distance information.
> diff --git a/arch/powerpc/include/asm/firmware.h b/arch/powerpc/include/asm/firmware.h
> index 60b631161360..97a3bd9ffeb9 100644
> --- a/arch/powerpc/include/asm/firmware.h
> +++ b/arch/powerpc/include/asm/firmware.h
> @@ -53,6 +53,7 @@
> #define FW_FEATURE_ULTRAVISOR ASM_CONST(0x0000004000000000)
> #define FW_FEATURE_STUFF_TCE ASM_CONST(0x0000008000000000)
> #define FW_FEATURE_RPT_INVALIDATE ASM_CONST(0x0000010000000000)
> +#define FW_FEATURE_FORM2_AFFINITY ASM_CONST(0x0000020000000000)
>
> #ifndef __ASSEMBLY__
>
> @@ -73,7 +74,7 @@ enum {
> FW_FEATURE_HPT_RESIZE | FW_FEATURE_DRMEM_V2 |
> FW_FEATURE_DRC_INFO | FW_FEATURE_BLOCK_REMOVE |
> FW_FEATURE_PAPR_SCM | FW_FEATURE_ULTRAVISOR |
> - FW_FEATURE_RPT_INVALIDATE,
> + FW_FEATURE_RPT_INVALIDATE | FW_FEATURE_FORM2_AFFINITY,
> FW_FEATURE_PSERIES_ALWAYS = 0,
> FW_FEATURE_POWERNV_POSSIBLE = FW_FEATURE_OPAL | FW_FEATURE_ULTRAVISOR,
> FW_FEATURE_POWERNV_ALWAYS = 0,
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index df9fec9d232c..5c80152e8f18 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -149,6 +149,7 @@ extern int of_read_drc_info_cell(struct property **prop,
> #define OV5_XCMO 0x0440 /* Page Coalescing */
> #define OV5_FORM1_AFFINITY 0x0580 /* FORM1 NUMA affinity */
> #define OV5_PRRN 0x0540 /* Platform Resource Reassignment */
> +#define OV5_FORM2_AFFINITY 0x0520 /* Form2 NUMA affinity */
> #define OV5_HP_EVT 0x0604 /* Hot Plug Event support */
> #define OV5_RESIZE_HPT 0x0601 /* Hash Page Table resizing */
> #define OV5_PFO_HW_RNG 0x1180 /* PFO Random Number Generator */
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 5d9ea059594f..c483df6c9393 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1069,7 +1069,8 @@ static const struct ibm_arch_vec ibm_architecture_vec_template __initconst = {
> #else
> 0,
> #endif
> - .associativity = OV5_FEAT(OV5_FORM1_AFFINITY) | OV5_FEAT(OV5_PRRN),
> + .associativity = OV5_FEAT(OV5_FORM1_AFFINITY) | OV5_FEAT(OV5_PRRN) |
> + OV5_FEAT(OV5_FORM2_AFFINITY),
> .bin_opts = OV5_FEAT(OV5_RESIZE_HPT) | OV5_FEAT(OV5_HP_EVT),
> .micro_checkpoint = 0,
> .reserved0 = 0,
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index c6293037a103..c68846fc9550 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -56,12 +56,17 @@ static int n_mem_addr_cells, n_mem_size_cells;
>
> #define FORM0_AFFINITY 0
> #define FORM1_AFFINITY 1
> +#define FORM2_AFFINITY 2
> static int affinity_form;
>
> #define MAX_DISTANCE_REF_POINTS 4
> static int max_associativity_domain_index;
> static const __be32 *distance_ref_points;
> static int distance_lookup_table[MAX_NUMNODES][MAX_DISTANCE_REF_POINTS];
> +static int numa_distance_table[MAX_NUMNODES][MAX_NUMNODES] = {
> + [0 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] = -1 }
> +};
> +static int numa_id_index_table[MAX_NUMNODES] = { [0 ... MAX_NUMNODES - 1] = NUMA_NO_NODE };
>
> /*
> * Allocate node_to_cpumask_map based on number of available nodes
> @@ -166,6 +171,44 @@ static void unmap_cpu_from_node(unsigned long cpu)
> }
> #endif /* CONFIG_HOTPLUG_CPU || CONFIG_PPC_SPLPAR */
>
> +/*
> + * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> + * info is found.
> + */
> +static int associativity_to_nid(const __be32 *associativity)
> +{
> + int nid = NUMA_NO_NODE;
> +
> + if (!numa_enabled)
> + goto out;
> +
> + if (of_read_number(associativity, 1) >= primary_domain_index)
> + nid = of_read_number(&associativity[primary_domain_index], 1);
> +
> + /* POWER4 LPAR uses 0xffff as invalid node */
> + if (nid == 0xffff || nid >= nr_node_ids)
> + nid = NUMA_NO_NODE;
> +out:
> + return nid;
> +}
> +
> +static int __cpu_form2_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> +{
> + int dist;
> + int node1, node2;
> +
> + node1 = associativity_to_nid(cpu1_assoc);
> + node2 = associativity_to_nid(cpu2_assoc);
> +
> + dist = numa_distance_table[node1][node2];
> + if (dist <= LOCAL_DISTANCE)
> + return 0;
> + else if (dist <= REMOTE_DISTANCE)
> + return 1;
> + else
> + return 2;
Squashing the full range of distances into just 0, 1 or 2 seems odd.
But then, this whole cpu_distance() thing being distinct from
node_distance() seems odd.
> +}
> +
> static int __cpu_form1_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> int dist = 0;
> @@ -186,8 +229,9 @@ int cpu_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> /* We should not get called with FORM0 */
> VM_WARN_ON(affinity_form == FORM0_AFFINITY);
> -
> - return __cpu_form1_relative_distance(cpu1_assoc, cpu2_assoc);
> + if (affinity_form == FORM1_AFFINITY)
> + return __cpu_form1_relative_distance(cpu1_assoc, cpu2_assoc);
> + return __cpu_form2_relative_distance(cpu1_assoc, cpu2_assoc);
> }
>
> /* must hold reference to node during call */
> @@ -201,7 +245,9 @@ int __node_distance(int a, int b)
> int i;
> int distance = LOCAL_DISTANCE;
>
> - if (affinity_form == FORM0_AFFINITY)
> + if (affinity_form == FORM2_AFFINITY)
> + return numa_distance_table[a][b];
> + else if (affinity_form == FORM0_AFFINITY)
> return ((a == b) ? LOCAL_DISTANCE : REMOTE_DISTANCE);
>
> for (i = 0; i < max_associativity_domain_index; i++) {
Hmm.. couldn't we simplify this whole __node_distance function, if we
just update numa_distance_table[][] appropriately for Form0 and Form1
as well?
> @@ -216,27 +262,6 @@ int __node_distance(int a, int b)
> }
> EXPORT_SYMBOL(__node_distance);
>
> -/*
> - * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> - * info is found.
> - */
> -static int associativity_to_nid(const __be32 *associativity)
> -{
> - int nid = NUMA_NO_NODE;
> -
> - if (!numa_enabled)
> - goto out;
> -
> - if (of_read_number(associativity, 1) >= primary_domain_index)
> - nid = of_read_number(&associativity[primary_domain_index], 1);
> -
> - /* POWER4 LPAR uses 0xffff as invalid node */
> - if (nid == 0xffff || nid >= nr_node_ids)
> - nid = NUMA_NO_NODE;
> -out:
> - return nid;
> -}
> -
> /* Returns the nid associated with the given device tree node,
> * or -1 if not found.
> */
> @@ -305,12 +330,84 @@ static void initialize_form1_numa_distance(struct device_node *node)
> */
> void update_numa_distance(struct device_node *node)
> {
> + int nid;
> +
> if (affinity_form == FORM0_AFFINITY)
> return;
> else if (affinity_form == FORM1_AFFINITY) {
> initialize_form1_numa_distance(node);
> return;
> }
> +
> + /* FORM2 affinity */
> + nid = of_node_to_nid_single(node);
> + if (nid == NUMA_NO_NODE)
> + return;
> +
> + /*
> + * With FORM2 we expect NUMA distance of all possible NUMA
> + * nodes to be provided during boot.
> + */
> + WARN(numa_distance_table[nid][nid] == -1,
> + "NUMA distance details for node %d not provided\n", nid);
> +}
> +
> +/*
> + * ibm,numa-lookup-index-table= {N, domainid1, domainid2, ..... domainidN}
> + * ibm,numa-distance-table = { N, 1, 2, 4, 5, 1, 6, .... N elements}
> + */
> +static void initialize_form2_numa_distance_lookup_table(struct device_node *root)
> +{
> + int i, j;
> + const __u8 *numa_dist_table;
> + const __be32 *numa_lookup_index;
> + int numa_dist_table_length;
> + int max_numa_index, distance_index;
> +
> + numa_lookup_index = of_get_property(root, "ibm,numa-lookup-index-table", NULL);
> + max_numa_index = of_read_number(&numa_lookup_index[0], 1);
> +
> + /* first element of the array is the size and is encode-int */
> + numa_dist_table = of_get_property(root, "ibm,numa-distance-table", NULL);
> + numa_dist_table_length = of_read_number((const __be32 *)&numa_dist_table[0], 1);
> + /* Skip the size which is encoded int */
> + numa_dist_table += sizeof(__be32);
> +
> + pr_debug("numa_dist_table_len = %d, numa_dist_indexes_len = %d\n",
> + numa_dist_table_length, max_numa_index);
> +
> + for (i = 0; i < max_numa_index; i++)
> + /* +1 skip the max_numa_index in the property */
> + numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
> +
> +
> + if (numa_dist_table_length != max_numa_index * max_numa_index) {
> +
> + WARN(1, "Wrong NUMA distance information\n");
> + /* consider everybody else just remote. */
> + for (i = 0; i < max_numa_index; i++) {
> + for (j = 0; j < max_numa_index; j++) {
> + int nodeA = numa_id_index_table[i];
> + int nodeB = numa_id_index_table[j];
> +
> + if (nodeA == nodeB)
> + numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
> + else
> + numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
> + }
> + }
> + }
> +
> + distance_index = 0;
> + for (i = 0; i < max_numa_index; i++) {
> + for (j = 0; j < max_numa_index; j++) {
> + int nodeA = numa_id_index_table[i];
> + int nodeB = numa_id_index_table[j];
> +
> + numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
> + }
> + }
> }
>
> static int __init find_primary_domain_index(void)
> @@ -323,6 +420,9 @@ static int __init find_primary_domain_index(void)
> */
> if (firmware_has_feature(FW_FEATURE_OPAL)) {
> affinity_form = FORM1_AFFINITY;
> + } else if (firmware_has_feature(FW_FEATURE_FORM2_AFFINITY)) {
> + dbg("Using form 2 affinity\n");
> + affinity_form = FORM2_AFFINITY;
> } else if (firmware_has_feature(FW_FEATURE_FORM1_AFFINITY)) {
> dbg("Using form 1 affinity\n");
> affinity_form = FORM1_AFFINITY;
> @@ -367,8 +467,17 @@ static int __init find_primary_domain_index(void)
>
> index = of_read_number(&distance_ref_points[1], 1);
> } else {
> + /*
> + * Both FORM1 and FORM2 affinity find the primary domain details
> + * at the same offset.
> + */
> index = of_read_number(distance_ref_points, 1);
> }
> + /*
> + * If it is FORM2 also initialize the distance table here.
> + */
> + if (affinity_form == FORM2_AFFINITY)
> + initialize_form2_numa_distance_lookup_table(root);
Ew. Calling a function called "find_primary_domain_index" to also
initialize the main distance table is needlessly counterintuitive.
Move this call to parse_numa_properties().
>
> /*
> * Warn and cap if the hardware supports more than
> diff --git a/arch/powerpc/platforms/pseries/firmware.c b/arch/powerpc/platforms/pseries/firmware.c
> index 5d4c2bc20bba..f162156b7b68 100644
> --- a/arch/powerpc/platforms/pseries/firmware.c
> +++ b/arch/powerpc/platforms/pseries/firmware.c
> @@ -123,6 +123,7 @@ vec5_fw_features_table[] = {
> {FW_FEATURE_PRRN, OV5_PRRN},
> {FW_FEATURE_DRMEM_V2, OV5_DRMEM_V2},
> {FW_FEATURE_DRC_INFO, OV5_DRC_INFO},
> + {FW_FEATURE_FORM2_AFFINITY, OV5_FORM2_AFFINITY},
> };
>
> static void __init fw_vec5_feature_init(const char *vec5, unsigned long len)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/6] powerpc/pseries: Add a helper for form1 cpu distance
From: David Gibson @ 2021-07-22 1:42 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <20210628151117.545935-6-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3860 bytes --]
On Mon, Jun 28, 2021 at 08:41:16PM +0530, Aneesh Kumar K.V wrote:
> This helper is only used with the dispatch trace log collection.
> A later patch will add Form2 affinity support and this change helps
> in keeping that simpler. Also add a comment explaining we don't expect
> the code to be called with FORM0
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
What makes it a "relative_distance" rather than just a "distance"?
> ---
> arch/powerpc/include/asm/topology.h | 4 ++--
> arch/powerpc/mm/numa.c | 10 +++++++++-
> arch/powerpc/platforms/pseries/lpar.c | 4 ++--
> 3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index e4db64c0e184..ac8b5ed79832 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -36,7 +36,7 @@ static inline int pcibus_to_node(struct pci_bus *bus)
> cpu_all_mask : \
> cpumask_of_node(pcibus_to_node(bus)))
>
> -extern int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc);
> +int cpu_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc);
> extern int __node_distance(int, int);
> #define node_distance(a, b) __node_distance(a, b)
>
> @@ -83,7 +83,7 @@ static inline void sysfs_remove_device_from_node(struct device *dev,
>
> static inline void update_numa_cpu_lookup_table(unsigned int cpu, int node) {}
>
> -static inline int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> +static inline int cpu_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> return 0;
> }
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 7b142f79d600..c6293037a103 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -166,7 +166,7 @@ static void unmap_cpu_from_node(unsigned long cpu)
> }
> #endif /* CONFIG_HOTPLUG_CPU || CONFIG_PPC_SPLPAR */
>
> -int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> +static int __cpu_form1_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> {
> int dist = 0;
>
> @@ -182,6 +182,14 @@ int cpu_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> return dist;
> }
>
> +int cpu_relative_distance(__be32 *cpu1_assoc, __be32 *cpu2_assoc)
> +{
> + /* We should not get called with FORM0 */
> + VM_WARN_ON(affinity_form == FORM0_AFFINITY);
> +
> + return __cpu_form1_relative_distance(cpu1_assoc, cpu2_assoc);
> +}
> +
> /* must hold reference to node during call */
> static const __be32 *of_get_associativity(struct device_node *dev)
> {
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index dab356e3ff87..afefbdfe768d 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -261,7 +261,7 @@ static int cpu_relative_dispatch_distance(int last_disp_cpu, int cur_disp_cpu)
> if (!last_disp_cpu_assoc || !cur_disp_cpu_assoc)
> return -EIO;
>
> - return cpu_distance(last_disp_cpu_assoc, cur_disp_cpu_assoc);
> + return cpu_relative_distance(last_disp_cpu_assoc, cur_disp_cpu_assoc);
> }
>
> static int cpu_home_node_dispatch_distance(int disp_cpu)
> @@ -281,7 +281,7 @@ static int cpu_home_node_dispatch_distance(int disp_cpu)
> if (!disp_cpu_assoc || !vcpu_assoc)
> return -EIO;
>
> - return cpu_distance(disp_cpu_assoc, vcpu_assoc);
> + return cpu_relative_distance(disp_cpu_assoc, vcpu_assoc);
> }
>
> static void update_vcpu_disp_stat(int disp_cpu)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 4/6] powerpc/pseries: Consolidate different NUMA distance update code paths
From: David Gibson @ 2021-07-22 1:40 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <20210628151117.545935-5-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 11408 bytes --]
On Mon, Jun 28, 2021 at 08:41:15PM +0530, Aneesh Kumar K.V wrote:
> The associativity details of the newly added resourced are collected from
> the hypervisor via "ibm,configure-connector" rtas call. Update the numa
> distance details of the newly added numa node after the above call.
>
> Instead of updating NUMA distance every time we lookup a node id
> from the associativity property, add helpers that can be used
> during boot which does this only once. Also remove the distance
> update from node id lookup helpers.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/mm/numa.c | 173 +++++++++++++-----
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 +
> .../platforms/pseries/hotplug-memory.c | 2 +
> arch/powerpc/platforms/pseries/pseries.h | 1 +
> 4 files changed, 132 insertions(+), 46 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 0ec16999beef..7b142f79d600 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -208,22 +208,6 @@ int __node_distance(int a, int b)
> }
> EXPORT_SYMBOL(__node_distance);
>
> -static void initialize_distance_lookup_table(int nid,
> - const __be32 *associativity)
> -{
> - int i;
> -
> - if (affinity_form != FORM1_AFFINITY)
> - return;
> -
> - for (i = 0; i < max_associativity_domain_index; i++) {
> - const __be32 *entry;
> -
> - entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
> - distance_lookup_table[nid][i] = of_read_number(entry, 1);
> - }
> -}
> -
> /*
> * Returns nid in the range [0..nr_node_ids], or -1 if no useful NUMA
> * info is found.
> @@ -241,15 +225,6 @@ static int associativity_to_nid(const __be32 *associativity)
> /* POWER4 LPAR uses 0xffff as invalid node */
> if (nid == 0xffff || nid >= nr_node_ids)
> nid = NUMA_NO_NODE;
> -
> - if (nid > 0 &&
> - of_read_number(associativity, 1) >= max_associativity_domain_index) {
> - /*
> - * Skip the length field and send start of associativity array
> - */
> - initialize_distance_lookup_table(nid, associativity + 1);
> - }
> -
> out:
> return nid;
> }
> @@ -287,6 +262,49 @@ int of_node_to_nid(struct device_node *device)
> }
> EXPORT_SYMBOL(of_node_to_nid);
>
> +static void __initialize_form1_numa_distance(const __be32 *associativity)
> +{
> + int i, nid;
> +
> + if (affinity_form != FORM1_AFFINITY)
Since this shouldn't be called on a !form1 system, this could be a WARN_ON().
> + return;
> +
> + if (of_read_number(associativity, 1) >= primary_domain_index) {
> + nid = of_read_number(&associativity[primary_domain_index], 1);
This computes the nid from the assoc array independently of
associativity_to_nid, which doesn't seem like a good idea. Wouldn't
it be better to call assocaitivity_to_nid(), then make the next bit
conditional on nid !== NUMA_NO_NODE?
> +
> + for (i = 0; i < max_associativity_domain_index; i++) {
> + const __be32 *entry;
> +
> + entry = &associativity[be32_to_cpu(distance_ref_points[i])];
> + distance_lookup_table[nid][i] = of_read_number(entry, 1);
> + }
> + }
> +}
> +
> +static void initialize_form1_numa_distance(struct device_node *node)
> +{
> + const __be32 *associativity;
> +
> + associativity = of_get_associativity(node);
> + if (!associativity)
> + return;
> +
> + __initialize_form1_numa_distance(associativity);
> +}
> +
> +/*
> + * Used to update distance information w.r.t newly added node.
> + */
> +void update_numa_distance(struct device_node *node)
> +{
> + if (affinity_form == FORM0_AFFINITY)
> + return;
> + else if (affinity_form == FORM1_AFFINITY) {
> + initialize_form1_numa_distance(node);
> + return;
> + }
> +}
> +
> static int __init find_primary_domain_index(void)
> {
> int index;
> @@ -433,6 +451,48 @@ static int of_get_assoc_arrays(struct assoc_arrays *aa)
> return 0;
> }
>
> +static int get_nid_and_numa_distance(struct drmem_lmb *lmb)
> +{
> + struct assoc_arrays aa = { .arrays = NULL };
> + int default_nid = NUMA_NO_NODE;
> + int nid = default_nid;
> + int rc, index;
> +
> + if ((primary_domain_index < 0) || !numa_enabled)
Under what circumstances could you get primary_domain_index < 0?
> + return default_nid;
> +
> + rc = of_get_assoc_arrays(&aa);
> + if (rc)
> + return default_nid;
> +
> + if (primary_domain_index <= aa.array_sz &&
> + !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) {
> + index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
Does anywhere verify that primary_domain_index <= aa.array_sz?
> + nid = of_read_number(&aa.arrays[index], 1);
> +
> + if (nid == 0xffff || nid >= nr_node_ids)
> + nid = default_nid;
> + if (nid > 0 && affinity_form == FORM1_AFFINITY) {
> + int i;
> + const __be32 *associativity;
> +
> + index = lmb->aa_index * aa.array_sz;
> + associativity = &aa.arrays[index];
> + /*
> + * lookup array associativity entries have different format
> + * There is no length of the array as the first element.
The difference it very small, and this is not a hot path. Couldn't
you reduce a chunk of code by prepending aa.array_sz, then re-using
__initialize_form1_numa_distance. Or even making
__initialize_form1_numa_distance() take the length as a parameter.
> + */
> + for (i = 0; i < max_associativity_domain_index; i++) {
> + const __be32 *entry;
> +
> + entry = &associativity[be32_to_cpu(distance_ref_points[i]) - 1];
Does anywhere verify that distance_ref_points[i] <= aa.array_size for
every i?
> + distance_lookup_table[nid][i] = of_read_number(entry, 1);
> + }
> + }
> + }
> + return nid;
> +}
> +
> /*
> * This is like of_node_to_nid_single() for memory represented in the
> * ibm,dynamic-reconfiguration-memory node.
> @@ -458,21 +518,14 @@ int of_drconf_to_nid_single(struct drmem_lmb *lmb)
>
> if (nid == 0xffff || nid >= nr_node_ids)
> nid = default_nid;
> -
> - if (nid > 0) {
> - index = lmb->aa_index * aa.array_sz;
> - initialize_distance_lookup_table(nid,
> - &aa.arrays[index]);
> - }
> }
> -
> return nid;
> }
>
> #ifdef CONFIG_PPC_SPLPAR
> -static int vphn_get_nid(long lcpu)
> +
> +static int __vphn_get_associativity(long lcpu, __be32 *associativity)
> {
> - __be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
> long rc, hwid;
>
> /*
> @@ -492,10 +545,22 @@ static int vphn_get_nid(long lcpu)
>
> rc = hcall_vphn(hwid, VPHN_FLAG_VCPU, associativity);
> if (rc == H_SUCCESS)
> - return associativity_to_nid(associativity);
> + return 0;
> }
>
> + return -1;
> +}
> +
> +static int vphn_get_nid(long lcpu)
> +{
> + __be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
> +
> +
> + if (!__vphn_get_associativity(lcpu, associativity))
> + return associativity_to_nid(associativity);
> +
> return NUMA_NO_NODE;
> +
> }
> #else
> static int vphn_get_nid(long unused)
> @@ -692,7 +757,7 @@ static int __init numa_setup_drmem_lmb(struct drmem_lmb *lmb,
> size = read_n_cells(n_mem_size_cells, usm);
> }
>
> - nid = of_drconf_to_nid_single(lmb);
> + nid = get_nid_and_numa_distance(lmb);
> fake_numa_create_new_node(((base + size) >> PAGE_SHIFT),
> &nid);
> node_set_online(nid);
> @@ -709,6 +774,7 @@ static int __init parse_numa_properties(void)
> struct device_node *memory;
> int default_nid = 0;
> unsigned long i;
> + const __be32 *associativity;
>
> if (numa_enabled == 0) {
> printk(KERN_WARNING "NUMA disabled by user\n");
> @@ -734,18 +800,30 @@ static int __init parse_numa_properties(void)
> * each node to be onlined must have NODE_DATA etc backing it.
> */
> for_each_present_cpu(i) {
> + __be32 vphn_assoc[VPHN_ASSOC_BUFSIZE];
> struct device_node *cpu;
> - int nid = vphn_get_nid(i);
> + int nid = NUMA_NO_NODE;
>
> - /*
> - * Don't fall back to default_nid yet -- we will plug
> - * cpus into nodes once the memory scan has discovered
> - * the topology.
> - */
> - if (nid == NUMA_NO_NODE) {
> + memset(vphn_assoc, 0, VPHN_ASSOC_BUFSIZE * sizeof(__be32));
What's the memset() for? AFAICT you only look at vphn_assoc in the
branch where __vphn_get_associativity() succeeds.
> +
> + if (__vphn_get_associativity(i, vphn_assoc) == 0) {
> + nid = associativity_to_nid(vphn_assoc);
> + __initialize_form1_numa_distance(vphn_assoc);
> + } else {
> +
> + /*
> + * Don't fall back to default_nid yet -- we will plug
> + * cpus into nodes once the memory scan has discovered
> + * the topology.
> + */
> cpu = of_get_cpu_node(i, NULL);
> BUG_ON(!cpu);
> - nid = of_node_to_nid_single(cpu);
> +
> + associativity = of_get_associativity(cpu);
> + if (associativity) {
> + nid = associativity_to_nid(associativity);
> + __initialize_form1_numa_distance(associativity);
> + }
> of_node_put(cpu);
> }
>
> @@ -781,8 +859,11 @@ static int __init parse_numa_properties(void)
> * have associativity properties. If none, then
> * everything goes to default_nid.
> */
> - nid = of_node_to_nid_single(memory);
> - if (nid < 0)
> + associativity = of_get_associativity(memory);
> + if (associativity) {
> + nid = associativity_to_nid(associativity);
> + __initialize_form1_numa_distance(associativity);
> + } else
> nid = default_nid;
>
> fake_numa_create_new_node(((start + size) >> PAGE_SHIFT), &nid);
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 7e970f81d8ff..778b6ab35f0d 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -498,6 +498,8 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
> return saved_rc;
> }
>
> + update_numa_distance(dn);
> +
> rc = dlpar_online_cpu(dn);
> if (rc) {
> saved_rc = rc;
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 36f66556a7c6..40d350f31a34 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -180,6 +180,8 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
> return -ENODEV;
> }
>
> + update_numa_distance(lmb_node);
> +
> dr_node = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> if (!dr_node) {
> dlpar_free_cc_nodes(lmb_node);
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 1f051a786fb3..663a0859cf13 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -113,4 +113,5 @@ extern u32 pseries_security_flavor;
> void pseries_setup_security_mitigations(void);
> void pseries_lpar_read_hblkrm_characteristics(void);
>
> +void update_numa_distance(struct device_node *node);
> #endif /* _PSERIES_PSERIES_H */
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/6] powerpc/pseries: rename min_common_depth to primary_domain_index
From: David Gibson @ 2021-07-22 1:59 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <20210628151117.545935-2-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 5234 bytes --]
On Mon, Jun 28, 2021 at 08:41:12PM +0530, Aneesh Kumar K.V wrote:
> No functional change in this patch.
The new name does not match how you describe "primary domain index" in
the documentation from patch 6/6. There it comes from the values in
associativity-reference-points, but here it simply comes from the
lengths of all the associativity properties.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> arch/powerpc/mm/numa.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..8365b298ec48 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -51,7 +51,7 @@ EXPORT_SYMBOL(numa_cpu_lookup_table);
> EXPORT_SYMBOL(node_to_cpumask_map);
> EXPORT_SYMBOL(node_data);
>
> -static int min_common_depth;
> +static int primary_domain_index;
> static int n_mem_addr_cells, n_mem_size_cells;
> static int form1_affinity;
>
> @@ -232,8 +232,8 @@ static int associativity_to_nid(const __be32 *associativity)
> if (!numa_enabled)
> goto out;
>
> - if (of_read_number(associativity, 1) >= min_common_depth)
> - nid = of_read_number(&associativity[min_common_depth], 1);
> + if (of_read_number(associativity, 1) >= primary_domain_index)
> + nid = of_read_number(&associativity[primary_domain_index], 1);
>
> /* POWER4 LPAR uses 0xffff as invalid node */
> if (nid == 0xffff || nid >= nr_node_ids)
> @@ -284,9 +284,9 @@ int of_node_to_nid(struct device_node *device)
> }
> EXPORT_SYMBOL(of_node_to_nid);
>
> -static int __init find_min_common_depth(void)
> +static int __init find_primary_domain_index(void)
> {
> - int depth;
> + int index;
> struct device_node *root;
>
> if (firmware_has_feature(FW_FEATURE_OPAL))
> @@ -326,7 +326,7 @@ static int __init find_min_common_depth(void)
> }
>
> if (form1_affinity) {
> - depth = of_read_number(distance_ref_points, 1);
> + index = of_read_number(distance_ref_points, 1);
> } else {
> if (distance_ref_points_depth < 2) {
> printk(KERN_WARNING "NUMA: "
> @@ -334,7 +334,7 @@ static int __init find_min_common_depth(void)
> goto err;
> }
>
> - depth = of_read_number(&distance_ref_points[1], 1);
> + index = of_read_number(&distance_ref_points[1], 1);
> }
>
> /*
> @@ -348,7 +348,7 @@ static int __init find_min_common_depth(void)
> }
>
> of_node_put(root);
> - return depth;
> + return index;
>
> err:
> of_node_put(root);
> @@ -437,16 +437,16 @@ int of_drconf_to_nid_single(struct drmem_lmb *lmb)
> int nid = default_nid;
> int rc, index;
>
> - if ((min_common_depth < 0) || !numa_enabled)
> + if ((primary_domain_index < 0) || !numa_enabled)
> return default_nid;
>
> rc = of_get_assoc_arrays(&aa);
> if (rc)
> return default_nid;
>
> - if (min_common_depth <= aa.array_sz &&
> + if (primary_domain_index <= aa.array_sz &&
> !(lmb->flags & DRCONF_MEM_AI_INVALID) && lmb->aa_index < aa.n_arrays) {
> - index = lmb->aa_index * aa.array_sz + min_common_depth - 1;
> + index = lmb->aa_index * aa.array_sz + primary_domain_index - 1;
> nid = of_read_number(&aa.arrays[index], 1);
>
> if (nid == 0xffff || nid >= nr_node_ids)
> @@ -708,18 +708,18 @@ static int __init parse_numa_properties(void)
> return -1;
> }
>
> - min_common_depth = find_min_common_depth();
> + primary_domain_index = find_primary_domain_index();
>
> - if (min_common_depth < 0) {
> + if (primary_domain_index < 0) {
> /*
> - * if we fail to parse min_common_depth from device tree
> + * if we fail to parse primary_domain_index from device tree
> * mark the numa disabled, boot with numa disabled.
> */
> numa_enabled = false;
> - return min_common_depth;
> + return primary_domain_index;
> }
>
> - dbg("NUMA associativity depth for CPU/Memory: %d\n", min_common_depth);
> + dbg("NUMA associativity depth for CPU/Memory: %d\n", primary_domain_index);
>
> /*
> * Even though we connect cpus to numa domains later in SMP
> @@ -919,14 +919,14 @@ static void __init find_possible_nodes(void)
> goto out;
> }
>
> - max_nodes = of_read_number(&domains[min_common_depth], 1);
> + max_nodes = of_read_number(&domains[primary_domain_index], 1);
> for (i = 0; i < max_nodes; i++) {
> if (!node_possible(i))
> node_set(i, node_possible_map);
> }
>
> prop_length /= sizeof(int);
> - if (prop_length > min_common_depth + 2)
> + if (prop_length > primary_domain_index + 2)
> coregroup_enabled = 1;
>
> out:
> @@ -1259,7 +1259,7 @@ int cpu_to_coregroup_id(int cpu)
> goto out;
>
> index = of_read_number(associativity, 1);
> - if (index > min_common_depth + 1)
> + if (index > primary_domain_index + 1)
> return of_read_number(&associativity[index - 1], 1);
>
> out:
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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