* Re: [RFC v2 2/3] postmigration/memory: Review assoc lookup array changes
From: Michael Bringmann @ 2018-04-24 21:33 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: John Allen, Thomas Falcon, Tyrel Datwyler
In-Reply-To: <69529aa2-aa7d-5107-0fa8-a0c9ad06420a@linux.vnet.ibm.com>
On 04/24/2018 12:01 PM, Nathan Fontenot wrote:
>> +};
>> +
>> +static int pseries_update_ala_memory_aai(int aa_index,
>> + struct property *dmprop)
>> +
>> +static int pseries_update_ala_memory(struct of_reconfig_data *pr)
> The two routines above should be updated to use the in-kernel drmem array instead
> of looking up the dynamic-memory property in the device tree.
Okay.
>
> -Nathan
Thanks.
Michael
>
>> +
>> static int pseries_memory_notifier(struct notifier_block *nb,
>> unsigned long action, void *data)
>> {
>> @@ -1067,6 +1184,9 @@ static int pseries_memory_notifier(struct notifier_block *nb,
>> case OF_RECONFIG_UPDATE_PROPERTY:
>> if (!strcmp(rd->prop->name, "ibm,dynamic-memory"))
>> err = pseries_update_drconf_memory(rd);
>> + if (!strcmp(rd->prop->name,
>> + "ibm,associativity-lookup-arrays"))
>> + err = pseries_update_ala_memory(rd);
>> break;
>> }
>> return notifier_from_errno(err);
>>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [RFC v2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2
From: Michael Bringmann @ 2018-04-24 21:35 UTC (permalink / raw)
To: Nathan Fontenot, linuxppc-dev; +Cc: John Allen, Thomas Falcon, Tyrel Datwyler
In-Reply-To: <5b82e3ba-a92f-2c05-0922-d7f8c31df534@linux.vnet.ibm.com>
See below.
On 04/24/2018 12:17 PM, Nathan Fontenot wrote:
> On 02/26/2018 02:53 PM, Michael Bringmann wrote:
>> postmigration/memory: Now apply changes to the associativity of memory
>> blocks described by the 'ibm,dynamic-memory-v2' property regarding
>> the topology of LPARS in Post Migration events.
>>
>> * Extend the previous work done for the 'ibm,associativity-lookup-array'
>> to apply to either property 'ibm,dynamic-memory' or
>> 'ibm,dynamic-memory-v2', whichever is present.
>> * Add new code to parse the 'ibm,dynamic-memory-v2' property looking
>> for differences in block 'assignment', associativity indexes per
>> block, and any other difference currently known.
>> * Rewrite some of the original code to parse the 'ibm,dynamic-memory'
>> property to take advantage of LMB parsing code.
>>
>> When block differences are recognized, the memory block may be removed,
>> added, or updated depending upon the state of the new device tree
>> property and differences from the migrated value of the property.
>>
>
> The only thing we need to check during LPM is affinity updates, memory
> is not added or removed as part of LPM.
>
> I think a slightly different approach to this may be worth considering.
> One of the goals of the drmem.c code was to remove the need to parse the
> device tree for memory directly. For this update, I think we could modify
> the code that builds the drmem_info data so that it can return a drmem_info
> struct instead of assuming to set the global one.
>
> This change would allow you to do a straight compare on the global vs. the
> new info from the updated device tree property. I think this would be cleaner
> and may be able to use the same routine for V1 and V2 properties.
The code dealing with the 'ibm,associativity' array updated cleanly to use
the same function to scan the LMBs regardless of the version of the properties.
The code dealing with changes to 'ibm,dynamic-memory-v2' is a mirror of the
code in 'pseries_update_drconf_memory' that deals with changes to the property
'ibm,dynamic-memory', so it should also be updated. On the other hand, do we
need to consider the memory requirements of creating/cloning the drmem_info
structure to provide a copy based on the new 'dynamic-memory' property?
Or is this not an issue?
>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> Changes in RFC v2:
>> -- Reuse existing parser code from 'drmem.c' in parsing property
>> 'imb,dynamic-memory-v2' for migration.
>> -- Fix crash during migration that occurs on non-VPHN systems
>> when attempting to reset topology timer.
>> -- Change section of a support function + variable from __init
>> to normal runtime to make them visible to migration code.
>> ---
>> arch/powerpc/include/asm/drmem.h | 8 +
>> arch/powerpc/mm/drmem.c | 23 ++-
>> arch/powerpc/mm/numa.c | 3
>> arch/powerpc/platforms/pseries/hotplug-memory.c | 175 +++++++++++++++++++----
>> drivers/of/fdt.c | 4 -
>> include/linux/of_fdt.h | 2
>> 6 files changed, 170 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
>> index 47a7012..e4773c9 100644
>> --- a/arch/powerpc/include/asm/drmem.h
>> +++ b/arch/powerpc/include/asm/drmem.h
>> @@ -92,6 +92,14 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>> void (*func)(struct drmem_lmb *, const __be32 **));
>> int drmem_update_dt(void);
>>
>> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>> + void (*func)(struct drmem_lmb *, const __be32 **));
>> +
>> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> + const __be32 **prop);
>> +void walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *data,
>> + void (*func)(struct drmem_lmb *, const __be32 **));
>> +
>> #ifdef CONFIG_PPC_PSERIES
>> void __init walk_drmem_lmbs_early(unsigned long node,
>> void (*func)(struct drmem_lmb *, const __be32 **));
>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>> index 31dbe14..e47a6e0 100644
>> --- a/arch/powerpc/mm/drmem.c
>> +++ b/arch/powerpc/mm/drmem.c
>> @@ -192,7 +192,7 @@ int drmem_update_dt(void)
>> return rc;
>> }
>>
>> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>> +static void read_drconf_v1_cell(struct drmem_lmb *lmb,
>> const __be32 **prop)
>> {
>> const __be32 *p = *prop;
>> @@ -208,7 +208,7 @@ static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>> *prop = p;
>> }
>>
>> -static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
>> +void walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *data,
>> void (*func)(struct drmem_lmb *, const __be32 **))
>> {
>> struct drmem_lmb lmb;
>> @@ -218,11 +218,12 @@ static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
>>
>> for (i = 0; i < n_lmbs; i++) {
>> read_drconf_v1_cell(&lmb, &prop);
>> - func(&lmb, &usm);
>> + func(&lmb, &data);
>> }
>> }
>> +EXPORT_SYMBOL(walk_drmem_v1_lmbs);
>>
>> -static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> +void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> const __be32 **prop)
>> {
>> const __be32 *p = *prop;
>> @@ -235,8 +236,9 @@ static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>>
>> *prop = p;
>> }
>> +EXPORT_SYMBOL(read_drconf_v2_cell);
>>
>> -static void __init __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
>> +void walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *data,
>> void (*func)(struct drmem_lmb *, const __be32 **))
>> {
>> struct of_drconf_cell_v2 dr_cell;
>> @@ -258,10 +260,11 @@ static void __init __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
>> lmb.aa_index = dr_cell.aa_index;
>> lmb.flags = dr_cell.flags;
>>
>> - func(&lmb, &usm);
>> + func(&lmb, &data);
>> }
>> }
>> }
>> +EXPORT_SYMBOL(walk_drmem_v2_lmbs);
>>
>> #ifdef CONFIG_PPC_PSERIES
>> void __init walk_drmem_lmbs_early(unsigned long node,
>> @@ -280,12 +283,12 @@ void __init walk_drmem_lmbs_early(unsigned long node,
>>
>> prop = of_get_flat_dt_prop(node, "ibm,dynamic-memory", &len);
>> if (prop) {
>> - __walk_drmem_v1_lmbs(prop, usm, func);
>> + walk_drmem_v1_lmbs(prop, usm, func);
>> } else {
>> prop = of_get_flat_dt_prop(node, "ibm,dynamic-memory-v2",
>> &len);
>> if (prop)
>> - __walk_drmem_v2_lmbs(prop, usm, func);
>> + walk_drmem_v2_lmbs(prop, usm, func);
>> }
>>
>> memblock_dump_all();
>> @@ -340,11 +343,11 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>>
>> prop = of_get_property(dn, "ibm,dynamic-memory", NULL);
>> if (prop) {
>> - __walk_drmem_v1_lmbs(prop, usm, func);
>> + walk_drmem_v1_lmbs(prop, usm, func);
>> } else {
>> prop = of_get_property(dn, "ibm,dynamic-memory-v2", NULL);
>> if (prop)
>> - __walk_drmem_v2_lmbs(prop, usm, func);
>> + walk_drmem_v2_lmbs(prop, usm, func);
>> }
>> }
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index 0e573f9..2545fea 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -1395,7 +1395,8 @@ static void topology_timer_fn(struct timer_list *unused)
>>
>> static void reset_topology_timer(void)
>> {
>> - mod_timer(&topology_timer, jiffies + topology_timer_secs * HZ);
>> + if (vphn_enabled)
>> + mod_timer(&topology_timer, jiffies + topology_timer_secs * HZ);
>
> This really looks like a bug fix that should be in a different patch.
Okay.
>
> -Nathan
Thanks.
Michael
>
>> }
>>
>> #ifdef CONFIG_SMP
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index b63181d..bf717e2 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -1051,49 +1051,155 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
>> return rc;
>> }
>>
>> -struct assoc_arrays {
>> - u32 n_arrays;
>> - u32 array_sz;
>> - const __be32 *arrays;
>> -};
>> +static inline int pseries_memory_v2_find_drc(u32 drc_index,
>> + u64 *base_addr, unsigned long memblock_size,
>> + struct of_drconf_cell_v2 *dm)
>> +{
>> + if ((dm->drc_index <= drc_index) &&
>> + (drc_index <= (dm->drc_index + dm->seq_lmbs - 1))) {
>> + int offset = drc_index - dm->drc_index;
>> +
>> + (*base_addr) = dm->base_addr +
>> + (offset * memblock_size);
>> + } else if (drc_index > (dm->drc_index +
>> + dm->seq_lmbs - 1)) {
>> + return -1;
>> + } else if (dm->drc_index > drc_index) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>>
>> -static int pseries_update_ala_memory_aai(int aa_index,
>> - struct property *dmprop)
>> +static int pseries_update_drconf_memory_v2(struct of_reconfig_data *pr)
>> {
>> - struct of_drconf_cell *drmem;
>> - u32 entries;
>> - __be32 *p;
>> - int i;
>> - int rc = 0;
>> + const __be32 *new_drmem, *old_drmem;
>> + unsigned long memblock_size;
>> + u32 new_lmb_sets, old_lmb_sets;
>> + u64 old_base_addr;
>> + int i, rc = 0;
>>
>> - p = (__be32 *) dmprop->value;
>> - if (!p)
>> + if (rtas_hp_event)
>> + return 0;
>> +
>> + memblock_size = pseries_memory_block_size();
>> + if (!memblock_size)
>> return -EINVAL;
>>
>> /* The first int of the property is the number of lmb's
>> * described by the property. This is followed by an array
>> - * of of_drconf_cell entries. Get the number of entries
>> - * and skip to the array of of_drconf_cell's.
>> + * of of_drconf_cell_v2 entries. Get the number of entries
>> + * and skip to the array of of_drconf_cell_v2's.
>> */
>> - entries = be32_to_cpu(*p++);
>> - drmem = (struct of_drconf_cell *)p;
>> + old_drmem = (__be32 *) pr->old_prop->value;
>> + if (!old_drmem)
>> + return -EINVAL;
>> + old_lmb_sets = of_read_number(old_drmem++, 1);
>>
>> - for (i = 0; i < entries; i++) {
>> - if ((be32_to_cpu(drmem[i].aa_index) != aa_index) &&
>> - (be32_to_cpu(drmem[i].flags) & DRCONF_MEM_ASSIGNED)) {
>> - rc = dlpar_memory_readd_by_index(
>> - be32_to_cpu(drmem[i].drc_index));
>> + new_drmem = (__be32 *)pr->prop->value;
>> + new_lmb_sets = of_read_number(new_drmem++, 1);
>> +
>> + for (i = 0; i < old_lmb_sets; i++) {
>> + int j;
>> + struct of_drconf_cell_v2 old_cell, new_cell;
>> +
>> + read_drconf_v2_cell(&old_cell, &old_drmem);
>> + read_drconf_v2_cell(&new_cell, &new_drmem);
>> +
>> + for (j = 0; j < new_cell.seq_lmbs; j++) {
>> + if (pseries_memory_v2_find_drc(
>> + new_cell.drc_index + j,
>> + &old_base_addr,
>> + memblock_size,
>> + &old_cell))
>> + continue;
>> +
>> + if ((old_cell.flags &
>> + DRCONF_MEM_ASSIGNED) &&
>> + (!(new_cell.flags &
>> + DRCONF_MEM_ASSIGNED))) {
>> + rc = pseries_remove_memblock(
>> + old_base_addr,
>> + memblock_size);
>> + } else if ((!(old_cell.flags &
>> + DRCONF_MEM_ASSIGNED)) &&
>> + (new_cell.flags &
>> + DRCONF_MEM_ASSIGNED)) {
>> + rc = memblock_add(
>> + old_base_addr, memblock_size);
>> + } else if ((old_cell.aa_index !=
>> + new_cell.aa_index) &&
>> + (new_cell.flags &
>> + DRCONF_MEM_ASSIGNED)) {
>> + dlpar_memory_readd_by_index(
>> + new_cell.drc_index + j);
>> + }
>> }
>> }
>>
>> - return rc;
>> + return 0;
>> +}
>> +
>> +struct assoc_arrays {
>> + u32 n_arrays;
>> + u32 array_sz;
>> + const __be32 *arrays;
>> +};
>> +
>> +struct update_ala_memory_aai_struct {
>> + int aa_index;
>> +};
>> +
>> +static void update_ala_memory_aai_cb(struct drmem_lmb *lmb,
>> + const __be32 **data)
>> +{
>> + struct update_ala_memory_aai_struct *updt =
>> + (struct update_ala_memory_aai_struct *)*data;
>> +
>> + if ((lmb->aa_index != updt->aa_index) &&
>> + (lmb->flags & DRCONF_MEM_ASSIGNED))
>> + dlpar_memory_readd_by_index(lmb->drc_index);
>> +}
>> +
>> +static int pseries_update_ala_memory_aai_v1(int aa_index,
>> + const __be32 *dmprop)
>> +{
>> + struct update_ala_memory_aai_struct data = {
>> + aa_index };
>> +
>> + walk_drmem_v1_lmbs(dmprop, (const __be32 *)&data,
>> + update_ala_memory_aai_cb);
>> +
>> + return 0;
>> +}
>> +
>> +static int pseries_update_ala_memory_aai_v2(int aa_index,
>> + const __be32 *dmprop)
>> +{
>> + struct update_ala_memory_aai_struct data = {
>> + aa_index };
>> +
>> + walk_drmem_v2_lmbs(dmprop, (const __be32 *)&data,
>> + update_ala_memory_aai_cb);
>> +
>> + return 0;
>> +}
>> +
>> +static int pseries_update_ala_memory_aai(int v1, int aa_index,
>> + const __be32 *dmprop)
>> +{
>> + if (v1)
>> + return pseries_update_ala_memory_aai_v1(aa_index, dmprop);
>> + else
>> + return pseries_update_ala_memory_aai_v2(aa_index, dmprop);
>> }
>>
>> static int pseries_update_ala_memory(struct of_reconfig_data *pr)
>> {
>> struct assoc_arrays new_ala, old_ala;
>> struct device_node *dn;
>> - struct property *dmprop;
>> + const __be32 *dmprop;
>> + bool v1 = true;
>> __be32 *p;
>> int i, lim;
>>
>> @@ -1104,10 +1210,15 @@ static int pseries_update_ala_memory(struct of_reconfig_data *pr)
>> if (!dn)
>> return -ENODEV;
>>
>> - dmprop = of_find_property(dn, "ibm,dynamic-memory", NULL);
>> + dmprop = of_get_property(dn, "ibm,dynamic-memory", NULL);
>> if (!dmprop) {
>> - of_node_put(dn);
>> - return -ENODEV;
>> + v1 = false;
>> + dmprop = of_get_property(dn, "ibm,dynamic-memory-v2",
>> + NULL);
>> + if (!dmprop) {
>> + of_node_put(dn);
>> + return -ENODEV;
>> + }
>> }
>>
>> /*
>> @@ -1149,11 +1260,11 @@ static int pseries_update_ala_memory(struct of_reconfig_data *pr)
>> new_ala.array_sz))
>> continue;
>>
>> - pseries_update_ala_memory_aai(i, dmprop);
>> + pseries_update_ala_memory_aai(v1, i, dmprop);
>> }
>>
>> for (i = lim; i < new_ala.n_arrays; i++)
>> - pseries_update_ala_memory_aai(i, dmprop);
>> + pseries_update_ala_memory_aai(v1, i, dmprop);
>>
>> } else {
>> /* Update all entries representing these rows;
>> @@ -1161,7 +1272,7 @@ static int pseries_update_ala_memory(struct of_reconfig_data *pr)
>> * have equivalent values.
>> */
>> for (i = 0; i < lim; i++)
>> - pseries_update_ala_memory_aai(i, dmprop);
>> + pseries_update_ala_memory_aai(v1, i, dmprop);
>> }
>>
>> of_node_put(dn);
>> @@ -1184,6 +1295,8 @@ static int pseries_memory_notifier(struct notifier_block *nb,
>> case OF_RECONFIG_UPDATE_PROPERTY:
>> if (!strcmp(rd->prop->name, "ibm,dynamic-memory"))
>> err = pseries_update_drconf_memory(rd);
>> + if (!strcmp(rd->prop->name, "ibm,dynamic-memory-v2"))
>> + err = pseries_update_drconf_memory_v2(rd);
>> if (!strcmp(rd->prop->name,
>> "ibm,associativity-lookup-arrays"))
>> err = pseries_update_ala_memory(rd);
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4675e5a..00df576 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -539,7 +539,7 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>> EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
>>
>> /* Everything below here references initial_boot_params directly. */
>> -int __initdata dt_root_addr_cells;
>> +int dt_root_addr_cells;
>> int __initdata dt_root_size_cells;
>>
>> void *initial_boot_params;
>> @@ -1013,7 +1013,7 @@ int __init early_init_dt_scan_root(unsigned long node, const char *uname,
>> return 1;
>> }
>>
>> -u64 __init dt_mem_next_cell(int s, const __be32 **cellp)
>> +u64 dt_mem_next_cell(int s, const __be32 **cellp)
>> {
>> const __be32 *p = *cellp;
>>
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index 013c541..14c8681 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -40,7 +40,7 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>> struct device_node **mynodes);
>>
>> /* TBD: Temporary export of fdt globals - remove when code fully merged */
>> -extern int __initdata dt_root_addr_cells;
>> +extern int dt_root_addr_cells;
>> extern int __initdata dt_root_size_cells;
>> extern void *initial_boot_params;
>>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* [PATCH] powerpc/numa: Handle unitialized timer reset
From: Michael Bringmann @ 2018-04-24 22:00 UTC (permalink / raw)
To: linuxppc-dev
Cc: Michael Bringmann, Nathan Fontenot, John Allen, Tyrel Datwyler,
Thomas Falcon
Some of the code in 'numa.c' may be reused when updating kernel
state after an LPAR migration event. One such common function
is 'reset_topology_timer' which was being invoked by the
'.notifier_call' function within the NUMA module by
'of_property_notify'. If this occurs after a previous call to
'stop_topology_update' or on a system without VPHN enabled, the
code runs into an unitialized timer structure and crashes. This
patch adds a safety check along this path toward the problem code.
Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
---
arch/powerpc/mm/numa.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 57a5029..8802e7d 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1457,7 +1457,8 @@ static void topology_timer_fn(struct timer_list *unused)
static void reset_topology_timer(void)
{
- mod_timer(&topology_timer, jiffies + topology_timer_secs * HZ);
+ if (vphn_enabled)
+ mod_timer(&topology_timer, jiffies + topology_timer_secs * HZ);
}
#ifdef CONFIG_SMP
^ permalink raw reply related
* [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
From: Nicholas Piggin @ 2018-04-25 2:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Abdul Haleem
The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
over the handler function call, which causes later smp_send_nmi_ipi()
callers to spin until the call is finished.
The smp_send_stop function never returns, so the busy count is never
decremeted, which can cause the system to hang in some cases. For
example panic() will call smp_send_stop early on, then later in the
reboot path, pnv_restart will call smp_send_stop again, which hangs.
Fix this by adding a special case to the smp_send_stop handler to
decrement the busy count, because it will never return.
Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/smp.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e16ec7b3b427..250fccf04c6e 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
#ifdef CONFIG_NMI_IPI
static void stop_this_cpu(struct pt_regs *regs)
+{
+ /*
+ * This is a special case because it never returns, so the NMI IPI
+ * handling would never mark it as done, which makes any later
+ * smp_send_nmi_ipi() call spin forever. Mark it done now.
+ */
+ nmi_ipi_lock();
+ nmi_ipi_busy_count--;
+ nmi_ipi_unlock();
#else
static void stop_this_cpu(void *dummy)
-#endif
{
+#endif
/* Remove this CPU */
set_cpu_online(smp_processor_id(), false);
--
2.17.0
^ permalink raw reply related
* [PATCH] powerpc: NMI IPI make NMI IPIs fully sychronous
From: Nicholas Piggin @ 2018-04-25 2:54 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Abdul Haleem
There is an asynchronous aspect to smp_send_nmi_ipi. The caller waits
for all CPUs to call in to the handler, but it does not wait for
completion of the handler. This is a needless complication, so remove
it and always wait synchronously.
The synchronous wait allows the caller to easily time out and clear
the wait for completion (zero nmi_ipi_busy_count) in the case of badly
behaved handlers. This would have prevented the recent smp_send_stop
NMI IPI bug from causing the system to hang.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/smp.h | 1 -
arch/powerpc/kernel/smp.c | 42 +++++++++++++++++-----------------
arch/powerpc/kernel/watchdog.c | 1 -
3 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index cfecfee1194b..8b844435cbd5 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -56,7 +56,6 @@ struct smp_ops_t {
int (*cpu_bootable)(unsigned int nr);
};
-extern void smp_flush_nmi_ipi(u64 delay_us);
extern int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
extern void smp_send_debugger_break(void);
extern void start_secondary_resume(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 250fccf04c6e..70cb9ceb8d6f 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -412,7 +412,8 @@ int smp_handle_nmi_ipi(struct pt_regs *regs)
fn(regs);
nmi_ipi_lock();
- nmi_ipi_busy_count--;
+ if (nmi_ipi_busy_count > 0) /* Can race with caller time-out */
+ nmi_ipi_busy_count--;
out:
nmi_ipi_unlock_end(&flags);
@@ -437,29 +438,11 @@ static void do_smp_send_nmi_ipi(int cpu)
}
}
-void smp_flush_nmi_ipi(u64 delay_us)
-{
- unsigned long flags;
-
- nmi_ipi_lock_start(&flags);
- while (nmi_ipi_busy_count) {
- nmi_ipi_unlock_end(&flags);
- udelay(1);
- if (delay_us) {
- delay_us--;
- if (!delay_us)
- return;
- }
- nmi_ipi_lock_start(&flags);
- }
- nmi_ipi_unlock_end(&flags);
-}
-
/*
* - cpu is the target CPU (must not be this CPU), or NMI_IPI_ALL_OTHERS.
* - fn is the target callback function.
* - delay_us > 0 is the delay before giving up waiting for targets to
- * enter the handler, == 0 specifies indefinite delay.
+ * complete executing the handler, == 0 specifies indefinite delay.
*/
int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us)
{
@@ -512,6 +495,22 @@ int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us)
cpumask_clear(&nmi_ipi_pending_mask);
}
nmi_ipi_busy_count--;
+
+ while (nmi_ipi_busy_count) {
+ nmi_ipi_unlock();
+ udelay(1);
+ if (delay_us) {
+ delay_us--;
+ if (!delay_us)
+ break;
+ }
+ nmi_ipi_lock();
+ }
+ if (nmi_ipi_busy_count) {
+ /* Could not execute on all CPUs */
+ ret = 0;
+ nmi_ipi_busy_count = 0;
+ }
nmi_ipi_unlock_end(&flags);
return ret;
@@ -574,7 +573,8 @@ static void stop_this_cpu(struct pt_regs *regs)
* smp_send_nmi_ipi() call spin forever. Mark it done now.
*/
nmi_ipi_lock();
- nmi_ipi_busy_count--;
+ if (nmi_ipi_busy_count > 0)
+ nmi_ipi_busy_count--;
nmi_ipi_unlock();
#else
static void stop_this_cpu(void *dummy)
diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
index 6256dc3b0087..8cf048c6c1eb 100644
--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -165,7 +165,6 @@ static void watchdog_smp_panic(int cpu, u64 tb)
continue;
smp_send_nmi_ipi(c, wd_lockup_ipi, 1000000);
}
- smp_flush_nmi_ipi(1000000);
}
/* Take the stuck CPUs out of the watch group */
--
2.17.0
^ permalink raw reply related
* Re: powerpc/mce: Fix a bug where mce loops on memory UE.
From: Michael Ellerman @ 2018-04-25 2:55 UTC (permalink / raw)
To: Mahesh J Salgaonkar, linuxppc-dev
In-Reply-To: <152445952887.3244.567606806755236868.stgit@jupiter.in.ibm.com>
On Mon, 2018-04-23 at 04:59:27 UTC, Mahesh J Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> The current code extracts the physical address for UE errors and then
> hooks it up into memory failure infrastructure. On successful extraction
> of physical address it wrongly sets "handled = 1" which means this UE error
> has been recovered. Since MCE handler gets return value as handled = 1, it
> assumes that error has been recovered and goes back to same NIP. This causes
> MCE interrupt again and again in a loop leading to hard lockup.
>
> Also, initialize phys_addr to ULONG_MAX so that we don't end up queuing
> undesired page to hwpoison.
>
> Without this patch we see:
> [ 1476.541984] Severe Machine check interrupt [Recovered]
> [ 1476.541985] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.541986] Initiator: CPU
> [ 1476.541987] Error type: UE [Load/Store]
> [ 1476.541988] Effective address: 00007fffd2755940
> [ 1476.541989] Physical address: 000020181a080000
> [...]
> [ 1476.542003] Severe Machine check interrupt [Recovered]
> [ 1476.542004] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.542005] Initiator: CPU
> [ 1476.542006] Error type: UE [Load/Store]
> [ 1476.542006] Effective address: 00007fffd2755940
> [ 1476.542007] Physical address: 000020181a080000
> [ 1476.542010] Severe Machine check interrupt [Recovered]
> [ 1476.542012] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.542013] Initiator: CPU
> [ 1476.542014] Error type: UE [Load/Store]
> [ 1476.542015] Effective address: 00007fffd2755940
> [ 1476.542016] Physical address: 000020181a080000
> [ 1476.542448] Memory failure: 0x20181a08: recovery action for dirty LRU page: Recovered
> [ 1476.542452] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542453] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542454] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542455] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542456] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542457] Memory failure: 0x20181a08: already hardware poisoned
> [...]
> [ 1490.972174] Watchdog CPU:38 Hard LOCKUP
>
> After this patch we see:
>
> [ 325.384336] Severe Machine check interrupt [Not recovered]
> [ 325.384338] NIP: [00007fffaae585f4] PID: 7168 Comm: find
> [ 325.384339] Initiator: CPU
> [ 325.384341] Error type: UE [Load/Store]
> [ 325.384343] Effective address: 00007fffaafe28ac
> [ 325.384345] Physical address: 00002017c0bd0000
> [ 325.384350] find[7168]: unhandled signal 7 at 00007fffaae585f4 nip 00007fffaae585f4 lr 00007fffaae585e0 code 4
> [ 325.388574] Memory failure: 0x2017c0bd: recovery action for dirty LRU page: Recovered
>
> Fixes: 01eaac2b0591 ("powerpc/mce: Hookup ierror (instruction) UE errors")
> Fixes: ba41e1e1ccb9 ("powerpc/mce: Hookup derror (load/store) UE errors")
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/75ecfb49516c53da00c57b9efe48fa
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
From: Michael Ellerman @ 2018-04-25 3:15 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Abdul Haleem, Nicholas Piggin
In-Reply-To: <20180425021753.26275-1-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
> over the handler function call, which causes later smp_send_nmi_ipi()
> callers to spin until the call is finished.
>
> The smp_send_stop function never returns, so the busy count is never
> decremeted, which can cause the system to hang in some cases. For
> example panic() will call smp_send_stop early on, then later in the
> reboot path, pnv_restart will call smp_send_stop again, which hangs.
>
> Fix this by adding a special case to the smp_send_stop handler to
> decrement the busy count, because it will never return.
>
> Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
> Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/smp.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index e16ec7b3b427..250fccf04c6e 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
>
> #ifdef CONFIG_NMI_IPI
> static void stop_this_cpu(struct pt_regs *regs)
> +{
> + /*
> + * This is a special case because it never returns, so the NMI IPI
> + * handling would never mark it as done, which makes any later
> + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> + */
> + nmi_ipi_lock();
> + nmi_ipi_busy_count--;
> + nmi_ipi_unlock();
> #else
> static void stop_this_cpu(void *dummy)
> -#endif
> {
> +#endif
I don't love this ifdef/endif business.
Can we do it this way instead?
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e16ec7b3b427..3582f30b60b7 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -565,11 +565,7 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
}
#endif
-#ifdef CONFIG_NMI_IPI
-static void stop_this_cpu(struct pt_regs *regs)
-#else
static void stop_this_cpu(void *dummy)
-#endif
{
/* Remove this CPU */
set_cpu_online(smp_processor_id(), false);
@@ -580,10 +576,26 @@ static void stop_this_cpu(void *dummy)
spin_cpu_relax();
}
+#ifdef CONFIG_NMI_IPI
+static void nmi_stop_this_cpu(struct pt_regs *regs)
+{
+ /*
+ * This is a special case because it never returns, so the NMI IPI
+ * handling would never mark it as done, which makes any later
+ * smp_send_nmi_ipi() call spin forever. Mark it done now.
+ */
+ nmi_ipi_lock();
+ nmi_ipi_busy_count--;
+ nmi_ipi_unlock();
+
+ stop_this_cpu(NULL);
+}
+#endif
+
void smp_send_stop(void)
{
#ifdef CONFIG_NMI_IPI
- smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
+ smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, nmi_stop_this_cpu, 1000000);
#else
smp_call_function(stop_this_cpu, NULL, 0);
#endif
cheers
^ permalink raw reply related
* Re: [PATCH 2/3] powerpc/powernv: Fix OPAL RTC driver OPAL_BUSY loops
From: Michael Ellerman @ 2018-04-25 3:28 UTC (permalink / raw)
To: Alexandre Belloni, Nicholas Piggin; +Cc: linux-rtc, linuxppc-dev
In-Reply-To: <20180424183918.GF19011@piout.net>
Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
> On 10/04/2018 23:01:36+1000, Nicholas Piggin wrote:
>> On Tue, 10 Apr 2018 14:07:28 +0200
>> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
>> > > Fixes ("powerpc/powernv: Add RTC and NVRAM support plus RTAS fallbacks"
>> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> > > Cc: linux-rtc@vger.kernel.org
>> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> > > ---
>> > > arch/powerpc/platforms/powernv/opal-rtc.c | 8 +++--
>> > > drivers/rtc/rtc-opal.c | 37 ++++++++++++++---------
>> >
>> > From what I understand, the changes in those files are fairly
>> > independent, they should probably be separated to ease merging.
>>
>> I'm happy to do that. It's using the same firmware call, so I thought
>> a single patch would be fine. But I guess the boot call can be
>> dropped from this patch because it does not not solve the problem
>> described in the changelog.
>>
>> Would you be happy for the driver change to be merged via the powerpc
>> tree? The code being fixed here came from the same original patch as
>> a similar issue being fixed in the OPAL NVRAM driver so it might be
>> easier that way.
>
> Ok then, just add my
>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> and let it go through the powerpc tree.
Thanks.
It's still mostly an rtc patch by lines changed, so I changed the
subject to:
rtc: opal: Fix OPAL RTC driver OPAL_BUSY loops
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
From: Nicholas Piggin @ 2018-04-25 3:51 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Abdul Haleem
In-Reply-To: <87fu3k563t.fsf@concordia.ellerman.id.au>
On Wed, 25 Apr 2018 13:15:34 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
> > over the handler function call, which causes later smp_send_nmi_ipi()
> > callers to spin until the call is finished.
> >
> > The smp_send_stop function never returns, so the busy count is never
> > decremeted, which can cause the system to hang in some cases. For
> > example panic() will call smp_send_stop early on, then later in the
> > reboot path, pnv_restart will call smp_send_stop again, which hangs.
> >
> > Fix this by adding a special case to the smp_send_stop handler to
> > decrement the busy count, because it will never return.
> >
> > Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
> > Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > arch/powerpc/kernel/smp.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index e16ec7b3b427..250fccf04c6e 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
> >
> > #ifdef CONFIG_NMI_IPI
> > static void stop_this_cpu(struct pt_regs *regs)
> > +{
> > + /*
> > + * This is a special case because it never returns, so the NMI IPI
> > + * handling would never mark it as done, which makes any later
> > + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> > + */
> > + nmi_ipi_lock();
> > + nmi_ipi_busy_count--;
> > + nmi_ipi_unlock();
> > #else
> > static void stop_this_cpu(void *dummy)
> > -#endif
> > {
> > +#endif
>
> I don't love this ifdef/endif business.
>
> Can we do it this way instead?
Yeah that's better. Does stop_this_cpu give you an unused function warning
if you compile with NMI though? I think we need an #if/#else
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index e16ec7b3b427..3582f30b60b7 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -565,11 +565,7 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
> }
> #endif
>
> -#ifdef CONFIG_NMI_IPI
> -static void stop_this_cpu(struct pt_regs *regs)
> -#else
> static void stop_this_cpu(void *dummy)
> -#endif
> {
> /* Remove this CPU */
> set_cpu_online(smp_processor_id(), false);
> @@ -580,10 +576,26 @@ static void stop_this_cpu(void *dummy)
> spin_cpu_relax();
> }
>
> +#ifdef CONFIG_NMI_IPI
> +static void nmi_stop_this_cpu(struct pt_regs *regs)
> +{
> + /*
> + * This is a special case because it never returns, so the NMI IPI
> + * handling would never mark it as done, which makes any later
> + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> + */
> + nmi_ipi_lock();
> + nmi_ipi_busy_count--;
> + nmi_ipi_unlock();
> +
> + stop_this_cpu(NULL);
> +}
> +#endif
> +
> void smp_send_stop(void)
> {
> #ifdef CONFIG_NMI_IPI
> - smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
> + smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, nmi_stop_this_cpu, 1000000);
> #else
> smp_call_function(stop_this_cpu, NULL, 0);
> #endif
>
>
> cheers
^ permalink raw reply
* [PATCH v2] powerpc: NMI IPI make NMI IPIs fully sychronous
From: Nicholas Piggin @ 2018-04-25 5:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
There is an asynchronous aspect to smp_send_nmi_ipi. The caller waits
for all CPUs to call in to the handler, but it does not wait for
completion of the handler. This is a needless complication, so remove
it and always wait synchronously.
The synchronous wait allows the caller to easily time out and clear
the wait for completion (zero nmi_ipi_busy_count) in the case of badly
behaved handlers. This would have prevented the recent smp_send_stop
NMI IPI bug from causing the system to hang.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Changes since v2:
- Fixed a locking bug in the timeout path
- Move all the shared memory accesses under the lock to avoid worrys
about memory ordering.
arch/powerpc/include/asm/smp.h | 1 -
arch/powerpc/kernel/smp.c | 52 ++++++++++++++++++----------------
arch/powerpc/kernel/watchdog.c | 1 -
3 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index cfecfee1194b..8b844435cbd5 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -56,7 +56,6 @@ struct smp_ops_t {
int (*cpu_bootable)(unsigned int nr);
};
-extern void smp_flush_nmi_ipi(u64 delay_us);
extern int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us);
extern void smp_send_debugger_break(void);
extern void start_secondary_resume(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 250fccf04c6e..8ad9288a72a0 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -412,7 +412,8 @@ int smp_handle_nmi_ipi(struct pt_regs *regs)
fn(regs);
nmi_ipi_lock();
- nmi_ipi_busy_count--;
+ if (nmi_ipi_busy_count > 1) /* Can race with caller time-out */
+ nmi_ipi_busy_count--;
out:
nmi_ipi_unlock_end(&flags);
@@ -437,29 +438,11 @@ static void do_smp_send_nmi_ipi(int cpu)
}
}
-void smp_flush_nmi_ipi(u64 delay_us)
-{
- unsigned long flags;
-
- nmi_ipi_lock_start(&flags);
- while (nmi_ipi_busy_count) {
- nmi_ipi_unlock_end(&flags);
- udelay(1);
- if (delay_us) {
- delay_us--;
- if (!delay_us)
- return;
- }
- nmi_ipi_lock_start(&flags);
- }
- nmi_ipi_unlock_end(&flags);
-}
-
/*
* - cpu is the target CPU (must not be this CPU), or NMI_IPI_ALL_OTHERS.
* - fn is the target callback function.
* - delay_us > 0 is the delay before giving up waiting for targets to
- * enter the handler, == 0 specifies indefinite delay.
+ * complete executing the handler, == 0 specifies indefinite delay.
*/
int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us)
{
@@ -496,8 +479,23 @@ int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us)
do_smp_send_nmi_ipi(cpu);
+ nmi_ipi_lock();
+ /* nmi_ipi_busy_count is held here, so unlock/lock is okay */
while (!cpumask_empty(&nmi_ipi_pending_mask)) {
+ nmi_ipi_unlock();
udelay(1);
+ nmi_ipi_lock();
+ if (delay_us) {
+ delay_us--;
+ if (!delay_us)
+ break;
+ }
+ }
+
+ while (nmi_ipi_busy_count > 1) {
+ nmi_ipi_unlock();
+ udelay(1);
+ nmi_ipi_lock();
if (delay_us) {
delay_us--;
if (!delay_us)
@@ -505,12 +503,17 @@ int smp_send_nmi_ipi(int cpu, void (*fn)(struct pt_regs *), u64 delay_us)
}
}
- nmi_ipi_lock();
if (!cpumask_empty(&nmi_ipi_pending_mask)) {
- /* Could not gather all CPUs */
+ /* Timeout waiting for CPUs to call smp_handle_nmi_ipi */
ret = 0;
cpumask_clear(&nmi_ipi_pending_mask);
}
+ if (nmi_ipi_busy_count > 1) {
+ /* Timeout waiting for CPUs to execute fn */
+ ret = 0;
+ nmi_ipi_busy_count = 1;
+ }
+
nmi_ipi_busy_count--;
nmi_ipi_unlock_end(&flags);
@@ -574,7 +577,8 @@ static void stop_this_cpu(struct pt_regs *regs)
* smp_send_nmi_ipi() call spin forever. Mark it done now.
*/
nmi_ipi_lock();
- nmi_ipi_busy_count--;
+ if (nmi_ipi_busy_count > 1)
+ nmi_ipi_busy_count--;
nmi_ipi_unlock();
#else
static void stop_this_cpu(void *dummy)
diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
index 6256dc3b0087..8cf048c6c1eb 100644
--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -165,7 +165,6 @@ static void watchdog_smp_panic(int cpu, u64 tb)
continue;
smp_send_nmi_ipi(c, wd_lockup_ipi, 1000000);
}
- smp_flush_nmi_ipi(1000000);
}
/* Take the stuck CPUs out of the watch group */
--
2.17.0
^ permalink raw reply related
* [PATCH] cpufreq: powernv: Remove global pstate ramp-down timer in POWER9
From: Shilpasri G Bhat @ 2018-04-25 6:14 UTC (permalink / raw)
To: rjw, viresh.kumar
Cc: benh, mpe, linux-pm, linuxppc-dev, linux-kernel, svaidy,
Shilpasri G Bhat
POWER9 doesnot support global pstate requests for the chip. So remove
the timer logic which slowly ramps down the global pstate in P9
platforms.
Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 0591874..71f8682 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -750,8 +750,13 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy,
cur_msec = jiffies_to_msecs(get_jiffies_64());
- spin_lock(&gpstates->gpstate_lock);
freq_data.pstate_id = idx_to_pstate(new_index);
+ if (!gpstates) {
+ freq_data.gpstate_id = freq_data.pstate_id;
+ goto no_gpstate;
+ }
+
+ spin_lock(&gpstates->gpstate_lock);
if (!gpstates->last_sampled_time) {
gpstate_idx = new_index;
@@ -801,6 +806,7 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy,
spin_unlock(&gpstates->gpstate_lock);
+no_gpstate:
/*
* Use smp_call_function to send IPI and execute the
* mtspr on target CPU. We could do that without IPI
@@ -835,6 +841,13 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
kernfs_put(kn);
}
+ policy->freq_table = powernv_freqs;
+ policy->fast_switch_possible = true;
+
+ if (pvr_version_is(PVR_POWER9))
+ return 0;
+
+ /* Initialise Gpstate ramp-down timer only on POWER8 */
gpstates = kzalloc(sizeof(*gpstates), GFP_KERNEL);
if (!gpstates)
return -ENOMEM;
@@ -849,15 +862,14 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
spin_lock_init(&gpstates->gpstate_lock);
- policy->freq_table = powernv_freqs;
- policy->fast_switch_possible = true;
return 0;
}
static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
{
/* timer is deleted in cpufreq_cpu_stop() */
- kfree(policy->driver_data);
+ if (policy->driver_data)
+ kfree(policy->driver_data);
return 0;
}
@@ -990,7 +1002,8 @@ static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
- del_timer_sync(&gpstates->timer);
+ if (gpstates)
+ del_timer_sync(&gpstates->timer);
}
static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] cpufreq: powernv: Remove global pstate ramp-down timer in POWER9
From: Viresh Kumar @ 2018-04-25 6:25 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, benh, mpe, linux-pm, linuxppc-dev, linux-kernel, svaidy
In-Reply-To: <1524636895-27185-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
On 25-04-18, 11:44, Shilpasri G Bhat wrote:
> POWER9 doesnot support global pstate requests for the chip. So remove
> the timer logic which slowly ramps down the global pstate in P9
> platforms.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 0591874..71f8682 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -750,8 +750,13 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy,
>
> cur_msec = jiffies_to_msecs(get_jiffies_64());
>
> - spin_lock(&gpstates->gpstate_lock);
> freq_data.pstate_id = idx_to_pstate(new_index);
> + if (!gpstates) {
> + freq_data.gpstate_id = freq_data.pstate_id;
> + goto no_gpstate;
> + }
> +
> + spin_lock(&gpstates->gpstate_lock);
>
> if (!gpstates->last_sampled_time) {
> gpstate_idx = new_index;
> @@ -801,6 +806,7 @@ static int powernv_cpufreq_target_index(struct cpufreq_policy *policy,
>
> spin_unlock(&gpstates->gpstate_lock);
>
> +no_gpstate:
> /*
> * Use smp_call_function to send IPI and execute the
> * mtspr on target CPU. We could do that without IPI
> @@ -835,6 +841,13 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> kernfs_put(kn);
> }
>
> + policy->freq_table = powernv_freqs;
> + policy->fast_switch_possible = true;
> +
> + if (pvr_version_is(PVR_POWER9))
> + return 0;
> +
> + /* Initialise Gpstate ramp-down timer only on POWER8 */
> gpstates = kzalloc(sizeof(*gpstates), GFP_KERNEL);
> if (!gpstates)
> return -ENOMEM;
> @@ -849,15 +862,14 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> msecs_to_jiffies(GPSTATE_TIMER_INTERVAL);
> spin_lock_init(&gpstates->gpstate_lock);
>
> - policy->freq_table = powernv_freqs;
> - policy->fast_switch_possible = true;
> return 0;
> }
>
> static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> {
> /* timer is deleted in cpufreq_cpu_stop() */
> - kfree(policy->driver_data);
> + if (policy->driver_data)
> + kfree(policy->driver_data);
>
> return 0;
> }
> @@ -990,7 +1002,8 @@ static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
> freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
> freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
> smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
> - del_timer_sync(&gpstates->timer);
> + if (gpstates)
> + del_timer_sync(&gpstates->timer);
> }
>
> static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Linus Walleij @ 2018-04-25 8:29 UTC (permalink / raw)
To: Bryant G. Ly, christoffer.dall
Cc: Benjamin Herrenschmidt, Michael Ellerman, Greg KH, Arnd Bergmann,
Jonathan Corbet, seroyer, mrochs, adreznec, fbarrat,
David S. Miller, Andrew Morton, Randy Dunlap, mikey,
Philippe Ombredanne, tlfalcon, msuchanek, linux-doc,
linuxppc-dev@lists.ozlabs.org list
In-Reply-To: <1524494812-60150-2-git-send-email-bryantly@linux.vnet.ibm.com>
I don't know why I'm included in review of this driver, but why
not :) it is good to include people.
Thanks for your patch! It looks exciting and complicated,
I like that kind of stuff.
On Mon, Apr 23, 2018 at 4:46 PM, Bryant G. Ly
<bryantly@linux.vnet.ibm.com> wrote:
> This driver is a logical device which provides an
> interface between the hypervisor and a management
> partition.
Please indicate what this management partition is to
be used for, i.e. the intended use case.
> This driver is to be used for the POWER Virtual
> Management Channel Virtual Adapter on the PowerVM
> platform.
Is that virtualization on the PowerPC platform?
> It provides both request/response and
> async message support through the /dev/ibmvmc node.
Is that a character or block device?
I guess block, but it's nice to have some specifics
in the commit message.
> diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-d=
evices/ibmvmc.txt
The document suffers a bit from internal lingo, so it's be great
if you improve it a bit with a style that will be welcoming for newcomers
who have no idea what you are talking about.
I will point out some items below.
> +Description
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> +
> +The Virtual Management Channel (VMC) is a logical device which provides =
an
> +interface between the hypervisor and a management partition.
What kind of interface? A bitstream? Message passing interface?
Marshalling of structs? Some specifics would be great.
> This
> +management partition is intended to provide an alternative to HMC-based
> +system management.
Please expand the acronym HMC for the newcomer so we do not need
to look it up, or it looks like the document is only for people who already
know everything about virtualization specifics.
> In the management partition, a Novalink application
> +exists
Is Novalink some kind of product name? In that case, can it be
instead described with some neutral technology name and state
Novalink as an examplf of this type of technology?
> which enables a system administrator to configure the system=E2=80=99s
> +partitioning characteristics
Exemplify "partitioning characteristics", with a sentence in this
vein: "partitioning characteristics such as FOO, BAR and BAZ".
> via a command line interface (CLI) or
> +Representational State Transfer Application (REST).
I understand what CLI is but what is a representational state
transfer application? Is that a fancy name for a GUI application
doing the same thing as a CLI program would do?
> You can also manage the
> +server by using PowerVC
There was PowerVM now there is PowerVC. What are those things
really? PowerVM I guess is virtual machine on PowerPC? What is
PowerVC? Virtual client (guest) in the PowerVM?
This kind of lingo makes the document a hard read for me.
> or other OpenStack solution.
I heard about OpenStack. I have a vague idea about what it is,
do you mean "other virtualization concepts"?
> Support for
> +conventional HMC management of the system may still be provided on a
> +system; however, when an HMC is attached to the system, the VMC
> +interface is disabled by the hypervisor.
The VMC acronym is expanded below, instead expand it at first
instance. The same for HMC (hypervisor management channel I guess?
it is never really explained.)
> +NovaLink runs on a Linux logical partition on a POWER8 or newer processo=
r-
> +based server that is virtualized by PowerVM.
So this information needs to be in the top of the document.
> System configuration,
> +maintenance, and control functions which traditionally require an HMC ca=
n
> +be implemented in the Novalink using a combination of HMC to hypervisor
> +interfaces and existing operating system methods. This tool provides
What tool? It is unclear what tool we are referring to here.
A command line tool? A technology? etc.
> a
> +subset of the functions implemented by the HMC and enables basic partiti=
on
> +configuration. The set of HMC to hypervisor messages
This makes me think that probably the "h" in HMC stands for "hypervisor"
so now I start to puzzle things together. But it's nicer if you just expand
the acronym the first time you use it.
> supported by the
> +Novalink component
I still haven't figured out what that is, in practice.
> are passed to the hypervisor over a VMC interface, which
> +is defined below.
Maybe some illustration of how HMC, Novalink and VMC work
together would be appropriate?
> +Virtual Management Channel (VMC)
> +A logical device, called the virtual management channel (VMC), is define=
d
> +for communicating between the Novalink application and the hypervisor.
So is "Novalink" some specific application, as in a proprietary
piece of software or something?
I'm still confused.
> +This device, similar to a VSCSI server device,
What is that acronym now, I get completely confused.
> is presented to a designated
> +management partition as a virtual device and is only presented when the
> +system is not HMC managed.
> +This communication device borrows aspects from both VSCSI and ILLAN devi=
ces
Now there is ILLAN too.
> +and is implemented using the CRQ and the RDMA interfaces.
CRQ and RDMA. The RDMA I heard about, I wonder what CRQ is.
At this point the document is referring to so much external a priori
knowledge that I don't have that I don't understand anything at all.
So if this document is meant as an introduction to the technology
for newcomers, it's not working at all, unfortunately.
If some specific domain knowledge is needed to read and
understand the document, you need to state these prerequisites
at the beginning of the document.
Something like: "if you don't know a lot about virtualization already,
then don't even bother trying to read this document".
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-25 9:02 UTC (permalink / raw)
To: rjw, viresh.kumar, npiggin
Cc: linux-pm, ppaidipe, linuxppc-dev, linux-kernel, svaidy,
Shilpasri G Bhat, stable
gpstate_timer_handler() uses synchronous smp_call to set the pstate
on the requested core. This causes the below hard lockup:
[c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
[c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
[c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
[c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
[c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
[c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
[c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
[c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
[c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
[c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
-- interrupt: 901 at doorbell_global_ipi+0x34/0x50
LR = arch_send_call_function_ipi_mask+0x120/0x130
[c000003fe566ba50] [c00000000004876c]
arch_send_call_function_ipi_mask+0x4c/0x130
[c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
[c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
[c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
[c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
[c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
[c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
[c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
One way to avoid this is removing the smp-call. We can ensure that the timer
always runs on one of the policy-cpus. If the timer gets migrated to a
cpu outside the policy then re-queue it back on the policy->cpus. This way
we can get rid of the smp-call which was being used to set the pstate
on the policy->cpus.
Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
Cc: <stable@vger.kernel.org> [4.8+]
Reported-by: Nicholas Piggin <npiggin@gmail.com>
Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
Changes from V1:
- Remove smp_call in the pstate handler.
drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 71f8682..dc8ffb5 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
if (!spin_trylock(&gpstates->gpstate_lock))
return;
+ /*
+ * If the timer has migrated to the different cpu then bring
+ * it back to one of the policy->cpus
+ */
+ if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
+ /*
+ * Timer should be deleted if policy is inactive.
+ * If policy is active then re-queue on one of the
+ * policy->cpus.
+ */
+ if (!cpumask_empty(policy->cpus)) {
+ gpstates->timer.expires = jiffies +
+ msecs_to_jiffies(1);
+ add_timer_on(&gpstates->timer,
+ cpumask_first(policy->cpus));
+ }
+ spin_unlock(&gpstates->gpstate_lock);
+ return;
+ }
/*
* If PMCR was last updated was using fast_swtich then
@@ -718,10 +737,8 @@ void gpstate_timer_handler(struct timer_list *t)
if (gpstate_idx != gpstates->last_lpstate_idx)
queue_gpstate_timer(gpstates);
+ set_pstate(&freq_data);
spin_unlock(&gpstates->gpstate_lock);
-
- /* Timer may get migrated to a different cpu on cpu hot unplug */
- smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
}
/*
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Viresh Kumar @ 2018-04-25 9:17 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, npiggin, linux-pm, ppaidipe, linuxppc-dev, linux-kernel,
svaidy, stable
In-Reply-To: <1524646968-526-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
On 25-04-18, 14:32, Shilpasri G Bhat wrote:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
>
> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c000003fe566ba50] [c00000000004876c]
> arch_send_call_function_ipi_mask+0x4c/0x130
> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>
> One way to avoid this is removing the smp-call. We can ensure that the timer
> always runs on one of the policy-cpus. If the timer gets migrated to a
> cpu outside the policy then re-queue it back on the policy->cpus. This way
> we can get rid of the smp-call which was being used to set the pstate
> on the policy->cpus.
>
> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
> Cc: <stable@vger.kernel.org> [4.8+]
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> Changes from V1:
> - Remove smp_call in the pstate handler.
>
> drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 71f8682..dc8ffb5 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
>
> if (!spin_trylock(&gpstates->gpstate_lock))
> return;
> + /*
> + * If the timer has migrated to the different cpu then bring
> + * it back to one of the policy->cpus
> + */
> + if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
> + /*
> + * Timer should be deleted if policy is inactive.
> + * If policy is active then re-queue on one of the
> + * policy->cpus.
> + */
This looks racy. Shouldn't you guarantee that the timer is already
removed in a synchronous way before de-activating the policy ?
> + if (!cpumask_empty(policy->cpus)) {
> + gpstates->timer.expires = jiffies +
> + msecs_to_jiffies(1);
> + add_timer_on(&gpstates->timer,
> + cpumask_first(policy->cpus));
> + }
> + spin_unlock(&gpstates->gpstate_lock);
> + return;
> + }
>
> /*
> * If PMCR was last updated was using fast_swtich then
> @@ -718,10 +737,8 @@ void gpstate_timer_handler(struct timer_list *t)
> if (gpstate_idx != gpstates->last_lpstate_idx)
> queue_gpstate_timer(gpstates);
>
> + set_pstate(&freq_data);
> spin_unlock(&gpstates->gpstate_lock);
> -
> - /* Timer may get migrated to a different cpu on cpu hot unplug */
> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
> }
>
> /*
> --
> 1.8.3.1
--
viresh
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/powernv: Fix OPAL RTC driver OPAL_BUSY loops
From: Alexandre Belloni @ 2018-04-25 9:41 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Nicholas Piggin, linux-rtc, linuxppc-dev
In-Reply-To: <87d0yo55ic.fsf@concordia.ellerman.id.au>
On 25/04/2018 13:28:27+1000, Michael Ellerman wrote:
> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
> > On 10/04/2018 23:01:36+1000, Nicholas Piggin wrote:
> >> On Tue, 10 Apr 2018 14:07:28 +0200
> >> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> >> > > Fixes ("powerpc/powernv: Add RTC and NVRAM support plus RTAS fallbacks"
> >> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> > > Cc: linux-rtc@vger.kernel.org
> >> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> > > ---
> >> > > arch/powerpc/platforms/powernv/opal-rtc.c | 8 +++--
> >> > > drivers/rtc/rtc-opal.c | 37 ++++++++++++++---------
> >> >
> >> > From what I understand, the changes in those files are fairly
> >> > independent, they should probably be separated to ease merging.
> >>
> >> I'm happy to do that. It's using the same firmware call, so I thought
> >> a single patch would be fine. But I guess the boot call can be
> >> dropped from this patch because it does not not solve the problem
> >> described in the changelog.
> >>
> >> Would you be happy for the driver change to be merged via the powerpc
> >> tree? The code being fixed here came from the same original patch as
> >> a similar issue being fixed in the OPAL NVRAM driver so it might be
> >> easier that way.
> >
> > Ok then, just add my
> >
> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> >
> > and let it go through the powerpc tree.
>
> Thanks.
>
> It's still mostly an rtc patch by lines changed, so I changed the
> subject to:
>
> rtc: opal: Fix OPAL RTC driver OPAL_BUSY loops
>
Great, thanks!
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-25 10:02 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, npiggin, linux-pm, ppaidipe, linuxppc-dev, linux-kernel,
svaidy, stable
In-Reply-To: <20180425091701.kxxslgalw3sdpiym@vireshk-i7>
Hi,
On 04/25/2018 02:47 PM, Viresh Kumar wrote:
> On 25-04-18, 14:32, Shilpasri G Bhat wrote:
>> gpstate_timer_handler() uses synchronous smp_call to set the pstate
>> on the requested core. This causes the below hard lockup:
>>
>> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
>> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
>> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
>> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
>> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
>> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
>> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
>> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
>> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
>> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
>> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
>> LR = arch_send_call_function_ipi_mask+0x120/0x130
>> [c000003fe566ba50] [c00000000004876c]
>> arch_send_call_function_ipi_mask+0x4c/0x130
>> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
>> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
>> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
>> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
>> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
>> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
>> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>>
>> One way to avoid this is removing the smp-call. We can ensure that the timer
>> always runs on one of the policy-cpus. If the timer gets migrated to a
>> cpu outside the policy then re-queue it back on the policy->cpus. This way
>> we can get rid of the smp-call which was being used to set the pstate
>> on the policy->cpus.
>>
>> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
>> Cc: <stable@vger.kernel.org> [4.8+]
>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> Changes from V1:
>> - Remove smp_call in the pstate handler.
>>
>> drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++++---
>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 71f8682..dc8ffb5 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
>>
>> if (!spin_trylock(&gpstates->gpstate_lock))
>> return;
>> + /*
>> + * If the timer has migrated to the different cpu then bring
>> + * it back to one of the policy->cpus
>> + */
>> + if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
>> + /*
>> + * Timer should be deleted if policy is inactive.
>> + * If policy is active then re-queue on one of the
>> + * policy->cpus.
>> + */
>
> This looks racy. Shouldn't you guarantee that the timer is already
> removed in a synchronous way before de-activating the policy ?
>
The timer is deleted in driver->stop_cpu(). So we ensure to remove the timer
before de-activating the policy.
>> + if (!cpumask_empty(policy->cpus)) {
So are you suggesting to remove ^^ the check for active policy here?
(I put that as a safety check.)
Thanks and Regards,
Shilpa
>> + gpstates->timer.expires = jiffies +
>> + msecs_to_jiffies(1);
>> + add_timer_on(&gpstates->timer,
>> + cpumask_first(policy->cpus));
>> + }
>> + spin_unlock(&gpstates->gpstate_lock);
>> + return;
>> + }
>>
>> /*
>> * If PMCR was last updated was using fast_swtich then
>> @@ -718,10 +737,8 @@ void gpstate_timer_handler(struct timer_list *t)
>> if (gpstate_idx != gpstates->last_lpstate_idx)
>> queue_gpstate_timer(gpstates);
>>
>> + set_pstate(&freq_data);
>> spin_unlock(&gpstates->gpstate_lock);
>> -
>> - /* Timer may get migrated to a different cpu on cpu hot unplug */
>> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
>> }
>>
>> /*
>> --
>> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH V2] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Viresh Kumar @ 2018-04-25 10:14 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, npiggin, linux-pm, ppaidipe, linuxppc-dev, linux-kernel,
svaidy, stable
In-Reply-To: <a827bf62-efd6-fb8b-86d7-e6b37bcf4a9a@linux.vnet.ibm.com>
On 25-04-18, 15:32, Shilpasri G Bhat wrote:
> Hi,
>
> On 04/25/2018 02:47 PM, Viresh Kumar wrote:
> > On 25-04-18, 14:32, Shilpasri G Bhat wrote:
> >> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> >> on the requested core. This causes the below hard lockup:
> >>
> >> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> >> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> >> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> >> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> >> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> >> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> >> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> >> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> >> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> >> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> >> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> >> LR = arch_send_call_function_ipi_mask+0x120/0x130
> >> [c000003fe566ba50] [c00000000004876c]
> >> arch_send_call_function_ipi_mask+0x4c/0x130
> >> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> >> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> >> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> >> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> >> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> >> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> >> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
> >>
> >> One way to avoid this is removing the smp-call. We can ensure that the timer
> >> always runs on one of the policy-cpus. If the timer gets migrated to a
> >> cpu outside the policy then re-queue it back on the policy->cpus. This way
> >> we can get rid of the smp-call which was being used to set the pstate
> >> on the policy->cpus.
> >>
> >> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
> >> Cc: <stable@vger.kernel.org> [4.8+]
> >> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> >> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> >> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> >> ---
> >> Changes from V1:
> >> - Remove smp_call in the pstate handler.
> >>
> >> drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++++---
> >> 1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> >> index 71f8682..dc8ffb5 100644
> >> --- a/drivers/cpufreq/powernv-cpufreq.c
> >> +++ b/drivers/cpufreq/powernv-cpufreq.c
> >> @@ -679,6 +679,25 @@ void gpstate_timer_handler(struct timer_list *t)
> >>
> >> if (!spin_trylock(&gpstates->gpstate_lock))
> >> return;
> >> + /*
> >> + * If the timer has migrated to the different cpu then bring
> >> + * it back to one of the policy->cpus
> >> + */
> >> + if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
> >> + /*
> >> + * Timer should be deleted if policy is inactive.
> >> + * If policy is active then re-queue on one of the
> >> + * policy->cpus.
> >> + */
> >
> > This looks racy. Shouldn't you guarantee that the timer is already
> > removed in a synchronous way before de-activating the policy ?
> >
>
> The timer is deleted in driver->stop_cpu(). So we ensure to remove the timer
> before de-activating the policy.
>
>
> >> + if (!cpumask_empty(policy->cpus)) {
>
> So are you suggesting to remove ^^ the check for active policy here?
> (I put that as a safety check.)
Either you are sure or you are not, and you don't need a safety check
if you are sure :)
--
viresh
^ permalink raw reply
* [PATCH V3] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-25 10:59 UTC (permalink / raw)
To: rjw, viresh.kumar
Cc: npiggin, benh, mpe, linux-pm, linuxppc-dev, linux-kernel,
ppaidipe, svaidy, Shilpasri G Bhat, stable
gpstate_timer_handler() uses synchronous smp_call to set the pstate
on the requested core. This causes the below hard lockup:
[c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
[c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
[c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
[c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
[c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
[c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
[c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
[c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
[c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
[c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
-- interrupt: 901 at doorbell_global_ipi+0x34/0x50
LR = arch_send_call_function_ipi_mask+0x120/0x130
[c000003fe566ba50] [c00000000004876c]
arch_send_call_function_ipi_mask+0x4c/0x130
[c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
[c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
[c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
[c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
[c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
[c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
[c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
One way to avoid this is removing the smp-call. We can ensure that the timer
always runs on one of the policy-cpus. If the timer gets migrated to a
cpu outside the policy then re-queue it back on the policy->cpus. This way
we can get rid of the smp-call which was being used to set the pstate
on the policy->cpus.
Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
Cc: <stable@vger.kernel.org> [4.8+]
Reported-by: Nicholas Piggin <npiggin@gmail.com>
Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
Changes from V2:
- Remove the check for active policy while requeing the migrated timer
Changes from V1:
- Remove smp_call in the pstate handler.
drivers/cpufreq/powernv-cpufreq.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 71f8682..e368e1f 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -679,6 +679,16 @@ void gpstate_timer_handler(struct timer_list *t)
if (!spin_trylock(&gpstates->gpstate_lock))
return;
+ /*
+ * If the timer has migrated to the different cpu then bring
+ * it back to one of the policy->cpus
+ */
+ if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
+ gpstates->timer.expires = jiffies + msecs_to_jiffies(1);
+ add_timer_on(&gpstates->timer, cpumask_first(policy->cpus));
+ spin_unlock(&gpstates->gpstate_lock);
+ return;
+ }
/*
* If PMCR was last updated was using fast_swtich then
@@ -718,10 +728,8 @@ void gpstate_timer_handler(struct timer_list *t)
if (gpstate_idx != gpstates->last_lpstate_idx)
queue_gpstate_timer(gpstates);
+ set_pstate(&freq_data);
spin_unlock(&gpstates->gpstate_lock);
-
- /* Timer may get migrated to a different cpu on cpu hot unplug */
- smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
}
/*
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 1/2] cxl: Set the PBCQ Tunnel BAR register when enabling capi mode
From: Philippe Bergheaud @ 2018-04-25 11:08 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat, clombard, benh, Philippe Bergheaud
Skiboot used to set the default Tunnel BAR register value when capi mode
was enabled. This approach was ok for the cxl driver, but prevented other
drivers from choosing different values.
Skiboot versions > 5.11 will not set the default value any longer. This
patch modifies the cxl driver to set/reset the Tunnel BAR register when
entering/exiting the cxl mode, with pnv_pci_set_tunnel_bar().
Signed-off-by: Philippe Bergheaud <felix@linux.ibm.com>
Reviewed-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
---
v2: Restrict tunnel bar setting to power9.
Do not fail cxl_configure_adapter() on tunnel bar setting error.
Log an info message instead, and continue configuring capi mode.
v3: No change.
---
drivers/misc/cxl/pci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 83f1d08058fc..355c789406f7 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1742,6 +1742,10 @@ static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
/* Required for devices using CAPP DMA mode, harmless for others */
pci_set_master(dev);
+ if (cxl_is_power9())
+ if (pnv_pci_set_tunnel_bar(dev, 0x00020000E0000000ull, 1))
+ dev_info(&dev->dev, "Tunneled operations unsupported\n");
+
if ((rc = pnv_phb_to_cxl_mode(dev, adapter->native->sl_ops->capi_mode)))
goto err;
@@ -1768,6 +1772,8 @@ static void cxl_deconfigure_adapter(struct cxl *adapter)
{
struct pci_dev *pdev = to_pci_dev(adapter->dev.parent);
+ if (cxl_is_power9())
+ pnv_pci_set_tunnel_bar(pdev, 0x00020000E0000000ull, 0);
cxl_native_release_psl_err_irq(adapter);
cxl_unmap_adapter_regs(adapter);
--
2.16.3
^ permalink raw reply related
* [PATCH v3 2/2] cxl: Report the tunneled operations status
From: Philippe Bergheaud @ 2018-04-25 11:08 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat, clombard, benh, Philippe Bergheaud
In-Reply-To: <20180425110834.16048-1-felix@linux.ibm.com>
Failure to synchronize the tunneled operations does not prevent
the initialization of the cxl card. This patch reports the tunneled
operations status via /sys.
Signed-off-by: Philippe Bergheaud <felix@linux.ibm.com>
---
v3: Added this patch to report the tunneled operations status.
---
drivers/misc/cxl/cxl.h | 1 +
drivers/misc/cxl/pci.c | 7 ++++++-
drivers/misc/cxl/sysfs.c | 10 ++++++++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index a4c9c8297a6d..918d4fb742d1 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -717,6 +717,7 @@ struct cxl {
bool perst_select_user;
bool perst_same_image;
bool psl_timebase_synced;
+ bool tunneled_ops_supported;
/*
* number of contexts mapped on to this card. Possible values are:
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 355c789406f7..008f50a0c465 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1742,9 +1742,14 @@ static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
/* Required for devices using CAPP DMA mode, harmless for others */
pci_set_master(dev);
- if (cxl_is_power9())
+ adapter->tunneled_ops_supported = false;
+
+ if (cxl_is_power9()) {
if (pnv_pci_set_tunnel_bar(dev, 0x00020000E0000000ull, 1))
dev_info(&dev->dev, "Tunneled operations unsupported\n");
+ else
+ adapter->tunneled_ops_supported = true;
+ }
if ((rc = pnv_phb_to_cxl_mode(dev, adapter->native->sl_ops->capi_mode)))
goto err;
diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index 95285b7f636f..4b5a4c5d3c01 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c
@@ -78,6 +78,15 @@ static ssize_t psl_timebase_synced_show(struct device *device,
return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->psl_timebase_synced);
}
+static ssize_t tunneled_ops_supported_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct cxl *adapter = to_cxl_adapter(device);
+
+ return scnprintf(buf, PAGE_SIZE, "%i\n", adapter->tunneled_ops_supported);
+}
+
static ssize_t reset_adapter_store(struct device *device,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -183,6 +192,7 @@ static struct device_attribute adapter_attrs[] = {
__ATTR_RO(base_image),
__ATTR_RO(image_loaded),
__ATTR_RO(psl_timebase_synced),
+ __ATTR_RO(tunneled_ops_supported),
__ATTR_RW(load_image_on_perst),
__ATTR_RW(perst_reloads_same_image),
__ATTR(reset, S_IWUSR, NULL, reset_adapter_store),
--
2.16.3
^ permalink raw reply related
* Re: [PATCH V3] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Nicholas Piggin @ 2018-04-25 11:26 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy, stable
In-Reply-To: <1524653971-4919-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
On Wed, 25 Apr 2018 16:29:31 +0530
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
>
> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> -- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c000003fe566ba50] [c00000000004876c]
> arch_send_call_function_ipi_mask+0x4c/0x130
> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>
> One way to avoid this is removing the smp-call. We can ensure that the timer
> always runs on one of the policy-cpus. If the timer gets migrated to a
> cpu outside the policy then re-queue it back on the policy->cpus. This way
> we can get rid of the smp-call which was being used to set the pstate
> on the policy->cpus.
>
> Fixes: 7bc54b652f13 (timers, cpufreq/powernv: Initialize the gpstate timer as pinned)
> Cc: <stable@vger.kernel.org> [4.8+]
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Thanks, this looks good to me. I don't know the code though, so
Acked-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Changes from V2:
> - Remove the check for active policy while requeing the migrated timer
> Changes from V1:
> - Remove smp_call in the pstate handler.
>
> drivers/cpufreq/powernv-cpufreq.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 71f8682..e368e1f 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -679,6 +679,16 @@ void gpstate_timer_handler(struct timer_list *t)
>
> if (!spin_trylock(&gpstates->gpstate_lock))
> return;
I still think it would be good to do something about the trylock failure.
It may be rare, but if it happens it could stop the timer and lead to
some rare unpredictable behaviour? Not for this patch, but while you're
looking at the code it would be good to consider it. Just queueing up
another timer seems like it should be enough.
> + /*
> + * If the timer has migrated to the different cpu then bring
> + * it back to one of the policy->cpus
> + */
> + if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) {
> + gpstates->timer.expires = jiffies + msecs_to_jiffies(1);
> + add_timer_on(&gpstates->timer, cpumask_first(policy->cpus));
> + spin_unlock(&gpstates->gpstate_lock);
> + return;
> + }
Really small nitpick, but you could use cpumask_any there.
Thanks,
Nick
>
> /*
> * If PMCR was last updated was using fast_swtich then
> @@ -718,10 +728,8 @@ void gpstate_timer_handler(struct timer_list *t)
> if (gpstate_idx != gpstates->last_lpstate_idx)
> queue_gpstate_timer(gpstates);
>
> + set_pstate(&freq_data);
> spin_unlock(&gpstates->gpstate_lock);
> -
> - /* Timer may get migrated to a different cpu on cpu hot unplug */
> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
> }
^ permalink raw reply
* [PATCH v2] powerpc: Fix smp_send_stop NMI IPI handling
From: Nicholas Piggin @ 2018-04-25 11:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Abdul Haleem, Michael Ellerman
The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
over the handler function call, which causes later smp_send_nmi_ipi()
callers to spin until the call is finished.
The smp_send_stop function never returns, so the busy count is never
decremeted, which can cause the system to hang in some cases. For
example panic() will call smp_send_stop early on, then later in the
reboot path, pnv_restart will call smp_send_stop again, which hangs.
Fix this by adding a special case to the smp_send_stop handler to
decrement the busy count, because it will never return.
Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Changes since v1:
- Reduce #ifdef spaghetti suggested by mpe
arch/powerpc/kernel/smp.c | 36 +++++++++++++++++++++++++++++-------
1 file changed, 29 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e16ec7b3b427..41d42c2f88d4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -566,10 +566,35 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
#endif
#ifdef CONFIG_NMI_IPI
-static void stop_this_cpu(struct pt_regs *regs)
-#else
+static void nmi_stop_this_cpu(struct pt_regs *regs)
+{
+ /*
+ * This is a special case because it never returns, so the NMI IPI
+ * handling would never mark it as done, which makes any later
+ * smp_send_nmi_ipi() call spin forever. Mark it done now.
+ *
+ * IRQs are already hard disabled by the smp_handle_nmi_ipi.
+ */
+ nmi_ipi_lock();
+ nmi_ipi_busy_count--;
+ nmi_ipi_unlock();
+
+ /* Remove this CPU */
+ set_cpu_online(smp_processor_id(), false);
+
+ spin_begin();
+ while (1)
+ spin_cpu_relax();
+}
+
+void smp_send_stop(void)
+{
+ smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, nmi_stop_this_cpu, 1000000);
+}
+
+#else /* CONFIG_NMI_IPI */
+
static void stop_this_cpu(void *dummy)
-#endif
{
/* Remove this CPU */
set_cpu_online(smp_processor_id(), false);
@@ -582,12 +607,9 @@ static void stop_this_cpu(void *dummy)
void smp_send_stop(void)
{
-#ifdef CONFIG_NMI_IPI
- smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
-#else
smp_call_function(stop_this_cpu, NULL, 0);
-#endif
}
+#endif /* CONFIG_NMI_IPI */
struct thread_info *current_set[NR_CPUS];
--
2.17.0
^ permalink raw reply related
* [PATCH] powerpc/xics: add missing of_node_put() in error path
From: YueHaibing @ 2018-04-25 11:27 UTC (permalink / raw)
To: benh, paulus, mpe, geoff, joe; +Cc: linuxppc-dev, linux-kernel, YueHaibing
The device node obtained with of_find_compatible_node() should be
released by calling of_node_put(). But it was not released when
of_get_property() failed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
arch/powerpc/sysdev/xics/xics-common.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index 77e864d..e54a9e6 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -448,7 +448,10 @@ static void __init xics_get_server_size(void)
return;
isize = of_get_property(np, "ibm,interrupt-server#-size", NULL);
if (!isize)
+ {
+ of_node_put(np);
return;
+ }
xics_interrupt_server_size = be32_to_cpu(*isize);
of_node_put(np);
}
--
2.7.0
^ permalink raw reply related
* [PATCH 00/11] KVM: PPC: reconstruct mmio emulation with analyse_instr()
From: wei.guo.simon @ 2018-04-25 11:54 UTC (permalink / raw)
To: kvm-ppc; +Cc: Paul Mackerras, kvm, linuxppc-dev, Simon Guo
From: Simon Guo <wei.guo.simon@gmail.com>
We already have analyse_instr() which analyzes instructions for the instruction
type, size, addtional flags, etc. What kvmppc_emulate_loadstore() did is somehow
duplicated and it will be good to utilize analyse_instr() to reconstruct the
code. The advantage is that the code logic will be shared and more clean to be
maintained.
This patch series reconstructs kvmppc_emulate_loadstore() for various load/store
instructions.
The testcase locates at:
https://github.com/justdoitqd/publicFiles/blob/master/test_mmio.c
- Tested at both PR/HV KVM.
- Also tested with little endian host & big endian guest.
Tested instruction list:
lbz lbzu lbzx ld ldbrx
ldu ldx lfd lfdu lfdx
lfiwax lfiwzx lfs lfsu lfsx
lha lhau lhax lhbrx lhz
lhzu lhzx lvx lwax lwbrx
lwz lwzu lwzx lxsdx lxsiwax
lxsiwzx lxsspx lxvd2x lxvdsx lxvw4x
stb stbu stbx std stdbrx
stdu stdx stfd stfdu stfdx
stfiwx stfs stfsx sth sthbrx
sthu sthx stvx stw stwbrx
stwu stwx stxsdx stxsiwx stxsspx
stxvd2x stxvw4x
Simon Guo (11):
KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[]
into it
KVM: PPC: mov nip/ctr/lr/xer registers to pt_regs in kvm_vcpu_arch
KVM: PPC: Fix a mmio_host_swabbed uninitialized usage issue when VMX
store
KVM: PPC: fix incorrect element_size for stxsiwx in analyse_instr
KVM: PPC: add GPR RA update skeleton for MMIO emulation
KVM: PPC: add KVMPPC_VSX_COPY_WORD_LOAD_DUMP type support for mmio
emulation
KVM: PPC: reconstruct non-SIMD LOAD/STORE instruction mmio emulation
with analyse_intr() input
KVM: PPC: add giveup_ext() hook for PPC KVM ops
KVM: PPC: reconstruct LOAD_FP/STORE_FP instruction mmio emulation with
analyse_intr() input
KVM: PPC: reconstruct LOAD_VMX/STORE_VMX instruction mmio emulation
with analyse_intr() input
KVM: PPC: reconstruct LOAD_VSX/STORE_VSX instruction mmio emulation
with analyse_intr() input
arch/powerpc/include/asm/kvm_book3s.h | 20 +-
arch/powerpc/include/asm/kvm_book3s_64.h | 20 +-
arch/powerpc/include/asm/kvm_booke.h | 20 +-
arch/powerpc/include/asm/kvm_host.h | 9 +-
arch/powerpc/include/asm/kvm_ppc.h | 2 +
arch/powerpc/include/asm/sstep.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 22 +-
arch/powerpc/kvm/book3s_32_mmu.c | 2 +-
arch/powerpc/kvm/book3s_64_vio_hv.c | 2 +-
arch/powerpc/kvm/book3s_hv.c | 11 +-
arch/powerpc/kvm/book3s_hv_builtin.c | 6 +-
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 15 +-
arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +-
arch/powerpc/kvm/book3s_hv_tm.c | 10 +-
arch/powerpc/kvm/book3s_hv_tm_builtin.c | 10 +-
arch/powerpc/kvm/book3s_pr.c | 73 +--
arch/powerpc/kvm/book3s_xive_template.c | 4 +-
arch/powerpc/kvm/booke.c | 41 +-
arch/powerpc/kvm/booke_emulate.c | 6 +-
arch/powerpc/kvm/e500_emulate.c | 6 +-
arch/powerpc/kvm/e500_mmu.c | 2 +-
arch/powerpc/kvm/emulate_loadstore.c | 734 +++++++++----------------------
arch/powerpc/kvm/powerpc.c | 53 ++-
arch/powerpc/lib/sstep.c | 2 +-
24 files changed, 407 insertions(+), 667 deletions(-)
--
1.8.3.1
^ 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