* 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 1/3] hotplug/mobility: Apply assoc updates for Post Migration Topo
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: <c2207993-e618-9192-b79e-c5519afdff1b@linux.vnet.ibm.com>
See comments below:
On 04/24/2018 11:56 AM, Nathan Fontenot wrote:
> On 02/26/2018 02:52 PM, Michael Bringmann wrote:
>> hotplug/mobility: Recognize more changes to the associativity of
>> memory blocks described by the 'ibm,dynamic-memory' and 'cpu'
>> properties when processing the topology of LPARS in Post Migration
>> events. Previous efforts only recognized whether a memory block's
>> assignment had changed in the property. Changes here include:
>>
>> * Checking the aa_index values of the old/new properties and 'readd'
>> any block for which the setting has changed.
>> * Checking for changes in cpu associativity and making 'readd' calls
>> when differences are observed.
>
> As part of the post-migration updates do you need to hold a lock
> so that we don't attempt to process any of the cpu/memory changes
> while the device tree is being updated?
>
> You may be able to grab the device hotplug lock for this.
The CPU Re-add process reuses the dlpar_cpu_remove / dlpar_cpu_add
code for POWERPC. These functions end up invoking device_online() /
device_offline() which in turn end up invoking the 'cpus_write_lock/unlock'
around every kernel change to the CPU structures. It was modeled
on the Memory Re-add process as we discussed a while back, which
also uses device_online and a corresponding write lock for each
LMB processed.
Do you see a need for a coarser granularity of locking around
all or a group of the cpu/memory changes? The data structures
that the kernel outside of powerpc uses for CPUs and LMBs seem
to be quite well isolated from the device-tree properties.
>
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> Changes in RFC:
>> -- Simplify code to update CPU nodes during mobility checks.
>> Remove functions to generate extra HP_ELOG messages in favor
>> of direct function calls to dlpar_cpu_readd_by_index.
>> -- Move check for "cpu" node type from pseries_update_cpu to
>> pseries_smp_notifier in 'hotplug-cpu.c'
>> -- Remove functions 'pseries_memory_readd_by_index' and
>> 'pseries_cpu_readd_by_index' as no longer needed outside of
>> 'mobility.c'.
>> ---
>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 69 +++++++++++++++++++++++
>> arch/powerpc/platforms/pseries/hotplug-memory.c | 6 ++
>> 2 files changed, 75 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index a7d14aa7..91ef22a 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -636,6 +636,27 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
>> return rc;
>> }
>>
>> +static int dlpar_cpu_readd_by_index(u32 drc_index)
>> +{
>> + int rc = 0;
>> +
>> + pr_info("Attempting to update CPU, drc index %x\n", drc_index);
>
> Should make this say we are re-adding the CPU, it's a bit more specific as
> to what is really happening.
Okay. I will update the notice from dlpar_memory_readd_by_index, as well.
>
>> +
>> + if (dlpar_cpu_remove_by_index(drc_index))
>> + rc = -EINVAL;
>> + else if (dlpar_cpu_add(drc_index))
>> + rc = -EINVAL;
>> +
>> + if (rc)
>> + pr_info("Failed to update cpu at drc_index %lx\n",
>> + (unsigned long int)drc_index);
>> + else
>> + pr_info("CPU at drc_index %lx was updated\n",
>> + (unsigned long int)drc_index);
>> +
>> + return rc;
>> +}
>> +
>> static int find_dlpar_cpus_to_remove(u32 *cpu_drcs, int cpus_to_remove)
>> {
>> struct device_node *dn;
>> @@ -826,6 +847,9 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
>> else
>> rc = -EINVAL;
>> break;
>> + case PSERIES_HP_ELOG_ACTION_READD:
>> + rc = dlpar_cpu_readd_by_index(drc_index);
>> + break;
>> default:
>> pr_err("Invalid action (%d) specified\n", hp_elog->action);
>> rc = -EINVAL;
>> @@ -876,12 +900,53 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
>>
>> #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
>>
>> +static int pseries_update_cpu(struct of_reconfig_data *pr)
>> +{
>> + u32 old_entries, new_entries;
>> + __be32 *p, *old_assoc, *new_assoc;
>> + int rc = 0;
>> +
>> + /* So far, we only handle the 'ibm,associativity' property,
>> + * here.
>> + * The first int of the property is the number of domains
>> + * described. This is followed by an array of level values.
>> + */
>> + p = (__be32 *) pr->old_prop->value;
>> + if (!p)
>> + return -EINVAL;
>> + old_entries = be32_to_cpu(*p++);
>> + old_assoc = p;
>> +
>> + p = (__be32 *)pr->prop->value;
>> + if (!p)
>> + return -EINVAL;
>> + new_entries = be32_to_cpu(*p++);
>> + new_assoc = p;
>> +
>> + if (old_entries == new_entries) {
>> + int sz = old_entries * sizeof(int);
>> +
>> + if (!memcmp(old_assoc, new_assoc, sz))
>> + rc = dlpar_cpu_readd_by_index(
>> + be32_to_cpu(pr->dn->phandle));
>> +
>> + } else {
>> + rc = dlpar_cpu_readd_by_index(
>> + be32_to_cpu(pr->dn->phandle));
>> + }
>> +
>> + return rc;
>> +}
>
> Do we need to do the full compare of the new vs. the old affinity property?
>
> I would think we would only get an updated property if the property changes.
> We don't care what changes in the property at this point, only that it changed.
> You could just call dlpar_cpu_readd_by_index() directly.
Okay.
>
> -Nathan
Thanks.
Michael
>
>> +
>> static int pseries_smp_notifier(struct notifier_block *nb,
>> unsigned long action, void *data)
>> {
>> struct of_reconfig_data *rd = data;
>> int err = 0;
>>
>> + if (strcmp(rd->dn->type, "cpu"))
>> + return notifier_from_errno(err);
>> +
>> switch (action) {
>> case OF_RECONFIG_ATTACH_NODE:
>> err = pseries_add_processor(rd->dn);
>> @@ -889,6 +954,10 @@ static int pseries_smp_notifier(struct notifier_block *nb,
>> case OF_RECONFIG_DETACH_NODE:
>> pseries_remove_processor(rd->dn);
>> break;
>> + case OF_RECONFIG_UPDATE_PROPERTY:
>> + if (!strcmp(rd->prop->name, "ibm,associativity"))
>> + err = pseries_update_cpu(rd);
>> + break;
>> }
>> return notifier_from_errno(err);
>> }
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index c1578f5..2341eae 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -1040,6 +1040,12 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
>> memblock_size);
>> rc = (rc < 0) ? -EINVAL : 0;
>> break;
>> + } else if ((be32_to_cpu(old_drmem[i].aa_index) !=
>> + be32_to_cpu(new_drmem[i].aa_index)) &&
>> + (be32_to_cpu(new_drmem[i].flags) &
>> + DRCONF_MEM_ASSIGNED)) {
>> + rc = dlpar_memory_readd_by_index(
>> + be32_to_cpu(new_drmem[i].drc_index))> }
>> }
>> return rc;
>>
>
--
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: [PATCH v2 3/7] powerpc: use task_pid_nr() for TID allocation
From: Sukadev Bhattiprolu @ 2018-04-24 21:12 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Alastair D'Silva, linuxppc-dev, linux-kernel, linux-doc,
mikey, vaibhav, aneesh.kumar, malat, felix, pombredanne, sukadev,
npiggin, gregkh, arnd, fbarrat, corbet, Alastair D'Silva,
Christophe Lombard
In-Reply-To: <b088d063-058a-2557-812c-6d5659f9d67b@au1.ibm.com>
Andrew Donnellan [andrew.donnellan@au1.ibm.com] wrote:
> [+ Sukadev, Christophe]
>
> On 18/04/18 11:08, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > The current implementation of TID allocation, using a global IDR, may
> > result in an errant process starving the system of available TIDs.
> > Instead, use task_pid_nr(), as mentioned by the original author. The
> > scenario described which prevented it's use is not applicable, as
> > set_thread_tidr can only be called after the task struct has been
> > populated.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>
> So it's too late in the evening for me to completely get my head around
> what's going on here enough to give my Reviewed-by:, but my current thinking
> is:
>
> - In the first version of the patch to add TIDR support
> (https://patchwork.ozlabs.org/patch/799494/), it was originally proposed to
> call assign_thread_id() (as it was then called) from copy_thread()
>
> - The comment block documents the reason why we can't use task_pid_nr() but
> assumes that we're trying to assign a TIDR from within copy_thread()
>
> - The final patch that was accepted
> (https://patchwork.ozlabs.org/patch/835552/,
> ec233ede4c8654894610ea54f4dae7adc954ac62) instead sets the TIDR to 0 from
> copy_thread(), so the original reasoning regarding not using task_pid_nr()
> within copy_thread() is no longer applicable.
>
> Sukadev: does this sound right?
Yes. Like with PIDR, was trying to assign TIDR initially to all threads.
But since only a subset of threads need/use TIDR, we can assign the
value later (when set_thread_tidr() is called). So we should be able to
use task_pid_nr() then.
Sukadev
^ permalink raw reply
* Re: [PATCH v4 03/19] powerpc: Mark variable `l` as unused, remove `path`
From: christophe leroy @ 2018-04-24 19:20 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: LKML, linuxppc-dev, Paul Mackerras, Benjamin Herrenschmidt,
Michael Ellerman
In-Reply-To: <CA+7wUswhZHNabB6kAcX8Cu8zMai15RzGn9euwdKau8kY48i-CA@mail.gmail.com>
Le 06/04/2018 à 20:32, Mathieu Malaterre a écrit :
> On Fri, Apr 6, 2018 at 5:33 PM, LEROY Christophe
> <christophe.leroy@c-s.fr> wrote:
>> Mathieu Malaterre <malat@debian.org> a écrit :
>>
>>> Add gcc attribute unused for `l` variable, replace `path` variable
>>> directly
>>> with prom_scratch. Fix warnings treated as errors with W=1:
>>>
>>> arch/powerpc/kernel/prom_init.c:607:6: error: variable ‘l’ set but not
>>> used [-Werror=unused-but-set-variable]
>>> arch/powerpc/kernel/prom_init.c:1388:8: error: variable ‘path’ set but
>>> not used [-Werror=unused-but-set-variable]
>>>
>>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>>> ---
>>> v4: redo v3 since path variable can be avoided
>>> v3: really move path within ifdef DEBUG_PROM
>>> v2: move path within ifdef DEBUG_PROM
>>>
>>> arch/powerpc/kernel/prom_init.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/prom_init.c
>>> b/arch/powerpc/kernel/prom_init.c
>>> index f8a9a50ff9b5..4b223a9470be 100644
>>> --- a/arch/powerpc/kernel/prom_init.c
>>> +++ b/arch/powerpc/kernel/prom_init.c
>>> @@ -604,7 +604,7 @@ static void __init early_cmdline_parse(void)
>>> const char *opt;
>>>
>>> char *p;
>>> - int l = 0;
>>> + int l __maybe_unused = 0;
>>
>>
>> Instead of hiding the problem with __maybe_unused, I think we could replace
>> the
>> #ifdef CONFIG_CMDLINE
>> by a
>> if (IS_ENABLED(CONFIG_CMDLINE_BOOL))
>>
>> This is recommanded by Linux codying style
>
> Neat. I was not aware of this trick. Does not work in this case though:
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 7925f64fefde..19634739b279 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -604,17 +604,16 @@ static void __init early_cmdline_parse(void)
> const char *opt;
>
> char *p;
> - int l __maybe_unused = 0;
> + int l = 0;
>
> prom_cmd_line[0] = 0;
> p = prom_cmd_line;
> if ((long)prom.chosen > 0)
> l = prom_getprop(prom.chosen, "bootargs", p,
> COMMAND_LINE_SIZE-1);
> -#ifdef CONFIG_CMDLINE
> - if (l <= 0 || p[0] == '\0') /* dbl check */
> - strlcpy(prom_cmd_line,
> - CONFIG_CMDLINE, sizeof(prom_cmd_line));
> -#endif /* CONFIG_CMDLINE */
> + if (IS_ENABLED(CONFIG_CMDLINE_BOOL))
> + if (l <= 0 || p[0] == '\0') /* dbl check */
> + strlcpy(prom_cmd_line,
> + CONFIG_CMDLINE, sizeof(prom_cmd_line));
> prom_printf("command line: %s\n", prom_cmd_line);
>
> #ifdef CONFIG_PPC64
>
>
> leads to:
>
> CC arch/powerpc/kernel/prom_init.o
> ../arch/powerpc/kernel/prom_init.c: In function ‘early_cmdline_parse’:
> ../arch/powerpc/kernel/prom_init.c:616:5: error: ‘CONFIG_CMDLINE’
> undeclared (first use in this function)
> CONFIG_CMDLINE, sizeof(prom_cmd_line));
> ^~~~~~~~~~~~~~
> ../arch/powerpc/kernel/prom_init.c:616:5: note: each undeclared
> identifier is reported only once for each function it appears in
> ../scripts/Makefile.build:312: recipe for target
> 'arch/powerpc/kernel/prom_init.o' failed
> make[6]: *** [arch/powerpc/kernel/prom_init.o] Error 1
And what about something like :
+ if (l <= 0 || p[0] == '\0') { /* dbl check */
#ifdef CONFIG_CMDLINE
- if (l <= 0 || p[0] == '\0') /* dbl check */
strlcpy(prom_cmd_line,
CONFIG_CMDLINE, sizeof(prom_cmd_line));
#endif /* CONFIG_CMDLINE */
+}
prom_printf("command line: %s\n", prom_cmd_line);
Christophe
>
>
>> Christophe
>>
>>
>>>
>>> prom_cmd_line[0] = 0;
>>> p = prom_cmd_line;
>>> @@ -1386,7 +1386,7 @@ static void __init reserve_mem(u64 base, u64 size)
>>> static void __init prom_init_mem(void)
>>> {
>>> phandle node;
>>> - char *path, type[64];
>>> + char type[64];
>>> unsigned int plen;
>>> cell_t *p, *endp;
>>> __be32 val;
>>> @@ -1407,7 +1407,6 @@ static void __init prom_init_mem(void)
>>> prom_debug("root_size_cells: %x\n", rsc);
>>>
>>> prom_debug("scanning memory:\n");
>>> - path = prom_scratch;
>>>
>>> for (node = 0; prom_next_node(&node); ) {
>>> type[0] = 0;
>>> @@ -1432,9 +1431,9 @@ static void __init prom_init_mem(void)
>>> endp = p + (plen / sizeof(cell_t));
>>>
>>> #ifdef DEBUG_PROM
>>> - memset(path, 0, PROM_SCRATCH_SIZE);
>>> - call_prom("package-to-path", 3, 1, node, path,
>>> PROM_SCRATCH_SIZE-1);
>>> - prom_debug(" node %s :\n", path);
>>> + memset(prom_scratch, 0, PROM_SCRATCH_SIZE);
>>> + call_prom("package-to-path", 3, 1, node, prom_scratch,
>>> PROM_SCRATCH_SIZE - 1);
>>> + prom_debug(" node %s :\n", prom_scratch);
>>> #endif /* DEBUG_PROM */
>>>
>>> while ((endp - p) >= (rac + rsc)) {
>>> --
>>> 2.11.0
>>
>>
>>
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
^ permalink raw reply
* Re: [PATCH v2] xmon: Use __printf markup to silence compiler
From: Mathieu Malaterre @ 2018-04-24 18:55 UTC (permalink / raw)
To: Michael Ellerman
Cc: Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev, LKML
In-Reply-To: <20180325090648.1029-1-malat@debian.org>
No comment so far... Did I miss anything ?
On Sun, Mar 25, 2018 at 11:06 AM, Mathieu Malaterre <malat@debian.org> wrot=
e:
> Update the other prototype declarations in asm/xmon.h.
>
> Silence warnings (triggered at W=3D1) by adding relevant __printf attribu=
te.
> Move #define at bottom of the file to prevent conflict with gcc attribute=
.
>
> Solve the original warning:
>
> arch/powerpc/xmon/nonstdio.c:178:2: error: function might be possible c=
andidate for =E2=80=98gnu_printf=E2=80=99 format attribute [-Werror=3Dsugge=
st-attribute=3Dformat]
>
> In turn this uncovered the following (partial list) warnings (treated as
> errors with W=3D1):
>
> arch/powerpc/xmon/xmon.c:2866:17: error: format =E2=80=98%x=E2=80=99 ex=
pects argument of type =E2=80=98unsigned int=E2=80=99, but argument 2 has t=
ype =E2=80=98unsigned char *=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:1607:31: error: format =E2=80=98%lx=E2=80=99 e=
xpects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 4=
has type =E2=80=98struct pt_regs *=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:1611:9: error: too many arguments for format [=
-Werror=3Dformat-extra-args]
> arch/powerpc/xmon/xmon.c:1623:26: error: format =E2=80=98%lx=E2=80=99 e=
xpects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 2=
has type =E2=80=98struct task_struct *=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:630:36: error: format =E2=80=98%lx=E2=80=99 ex=
pects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 2 =
has type =E2=80=98int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:1392:15: error: format =E2=80=98%x=E2=80=99 ex=
pects argument of type =E2=80=98unsigned int=E2=80=99, but argument 2 has t=
ype =E2=80=98long int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:2570:16: error: format =E2=80=98%lx=E2=80=99 e=
xpects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 3=
has type =E2=80=98u64 {aka long long unsigned int}=E2=80=99 [-Werror=3Dfor=
mat=3D]
> arch/powerpc/xmon/xmon.c:1629:25: error: format =E2=80=98%ld=E2=80=99 e=
xpects argument of type =E2=80=98long int=E2=80=99, but argument 2 has type=
=E2=80=98pid_t {aka int}=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:1168:18: error: format =E2=80=98%x=E2=80=99 ex=
pects argument of type =E2=80=98unsigned int=E2=80=99, but argument 2 has t=
ype =E2=80=98long unsigned int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:3016:24: error: format =E2=80=98%lx=E2=80=99 e=
xpects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 2=
has type =E2=80=98pgd_t * {aka struct <anonymous> *}=E2=80=99 [-Werror=3Df=
ormat=3D]
> arch/powerpc/xmon/xmon.c:2339:9: error: format =E2=80=98%lx=E2=80=99 ex=
pects argument of type =E2=80=98long unsigned int=E2=80=99, but argument 5 =
has type =E2=80=98u64 {aka long long unsigned int}=E2=80=99 [-Werror=3Dform=
at=3D]
> arch/powerpc/xmon/xmon.c:2339:9: error: format =E2=80=98%llx=E2=80=99 e=
xpects argument of type =E2=80=98long long unsigned int=E2=80=99, but argum=
ent 5 has type =E2=80=98long unsigned int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:3827:10: error: format =E2=80=98%p=E2=80=99 ex=
pects argument of type =E2=80=98void *=E2=80=99, but argument 4 has type =
=E2=80=98long long unsigned int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:3896:50: error: format =E2=80=98%d=E2=80=99 ex=
pects argument of type =E2=80=98int=E2=80=99, but argument 2 has type =E2=
=80=98long unsigned int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/spu-dis.c:137:18: error: format =E2=80=98%d=E2=80=99 =
expects argument of type =E2=80=98int=E2=80=99, but argument 2 has type =E2=
=80=98long unsigned int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:3827:10: error: format =E2=80=98%d=E2=80=99 ex=
pects argument of type =E2=80=98int=E2=80=99, but argument 4 has type =E2=
=80=98u64 {aka long long unsigned int}=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:1665:17: error: format =E2=80=98%ld=E2=80=99 e=
xpects argument of type =E2=80=98long int=E2=80=99, but argument 2 has type=
=E2=80=98int=E2=80=99 [-Werror=3Dformat=3D]
> arch/powerpc/xmon/xmon.c:2339:9: error: '#' flag used with =E2=80=98%p=
=E2=80=99 gnu_printf format [-Werror=3Dformat=3D]
>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> ---
> v2: resubmit patch series a single patch
>
> arch/powerpc/include/asm/xmon.h | 2 +-
> arch/powerpc/xmon/nonstdio.h | 8 +--
> arch/powerpc/xmon/spu-dis.c | 18 +++---
> arch/powerpc/xmon/xmon.c | 133 ++++++++++++++++++++--------------=
------
> 4 files changed, 82 insertions(+), 79 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xmon.h b/arch/powerpc/include/asm/x=
mon.h
> index eb42a0c6e1d9..30ff69bd8f43 100644
> --- a/arch/powerpc/include/asm/xmon.h
> +++ b/arch/powerpc/include/asm/xmon.h
> @@ -29,7 +29,7 @@ static inline void xmon_register_spus(struct list_head =
*list) { };
> extern int cpus_are_in_xmon(void);
> #endif
>
> -extern void xmon_printf(const char *format, ...);
> +extern __printf(1, 2) void xmon_printf(const char *format, ...);
>
> #endif /* __KERNEL __ */
> #endif /* __ASM_POWERPC_XMON_H */
> diff --git a/arch/powerpc/xmon/nonstdio.h b/arch/powerpc/xmon/nonstdio.h
> index 2202ec61972c..e8deac6c84e2 100644
> --- a/arch/powerpc/xmon/nonstdio.h
> +++ b/arch/powerpc/xmon/nonstdio.h
> @@ -1,13 +1,13 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #define EOF (-1)
>
> -#define printf xmon_printf
> -#define putchar xmon_putchar
> -
> extern void xmon_set_pagination_lpp(unsigned long lpp);
> extern void xmon_start_pagination(void);
> extern void xmon_end_pagination(void);
> extern int xmon_putchar(int c);
> extern void xmon_puts(const char *);
> extern char *xmon_gets(char *, int);
> -extern void xmon_printf(const char *, ...);
> +extern __printf(1, 2) void xmon_printf(const char *fmt, ...);
> +
> +#define printf xmon_printf
> +#define putchar xmon_putchar
> diff --git a/arch/powerpc/xmon/spu-dis.c b/arch/powerpc/xmon/spu-dis.c
> index e5f89837c82e..4cbc7da88524 100644
> --- a/arch/powerpc/xmon/spu-dis.c
> +++ b/arch/powerpc/xmon/spu-dis.c
> @@ -102,7 +102,7 @@ print_insn_spu (unsigned long insn, unsigned long mem=
addr)
>
> if (index =3D=3D 0)
> {
> - printf(".long 0x%x", insn);
> + printf(".long 0x%lx", insn);
> }
> else
> {
> @@ -134,27 +134,27 @@ print_insn_spu (unsigned long insn, unsigned long m=
emaddr)
> switch (arg)
> {
> case A_T:
> - printf("$%d",
> + printf("$%lu",
> DECODE_INSN_RT (insn));
> break;
> case A_A:
> - printf("$%d",
> + printf("$%lu",
> DECODE_INSN_RA (insn));
> break;
> case A_B:
> - printf("$%d",
> + printf("$%lu",
> DECODE_INSN_RB (insn));
> break;
> case A_C:
> - printf("$%d",
> + printf("$%lu",
> DECODE_INSN_RC (insn));
> break;
> case A_S:
> - printf("$sp%d",
> + printf("$sp%lu",
> DECODE_INSN_RA (insn));
> break;
> case A_H:
> - printf("$ch%d",
> + printf("$ch%lu",
> DECODE_INSN_RA (insn));
> break;
> case A_P:
> @@ -162,11 +162,11 @@ print_insn_spu (unsigned long insn, unsigned long m=
emaddr)
> printf("(");
> break;
> case A_U7A:
> - printf("%d",
> + printf("%lu",
> 173 - DECODE_INSN_U8 (insn));
> break;
> case A_U7B:
> - printf("%d",
> + printf("%lu",
> 155 - DECODE_INSN_U8 (insn));
> break;
> case A_S3:
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index ee113a6e008c..edd7ea85272f 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -627,7 +627,7 @@ static int xmon_core(struct pt_regs *regs, int fromip=
i)
> excprint(regs);
> bp =3D at_breakpoint(regs->nip);
> if (bp) {
> - printf("Stopped at breakpoint %lx (", BP_NUM(bp))=
;
> + printf("Stopped at breakpoint %tx (", BP_NUM(bp))=
;
> xmon_print_symbol(regs->nip, " ", ")\n");
> }
> if (unrecoverable_excp(regs))
> @@ -1165,7 +1165,7 @@ static int cpu_cmd(void)
> }
> /* try to switch to cpu specified */
> if (!cpumask_test_cpu(cpu, &cpus_in_xmon)) {
> - printf("cpu 0x%x isn't in xmon\n", cpu);
> + printf("cpu 0x%lx isn't in xmon\n", cpu);
> return 0;
> }
> xmon_taken =3D 0;
> @@ -1179,7 +1179,7 @@ static int cpu_cmd(void)
> /* take control back */
> mb();
> xmon_owner =3D smp_processor_id();
> - printf("cpu 0x%x didn't take control\n", cpu);
> + printf("cpu 0x%lx didn't take control\n", cpu);
> return 0;
> }
> barrier();
> @@ -1362,7 +1362,7 @@ bpt_cmds(void)
> }
> }
>
> - printf("Cleared breakpoint %lx (", BP_NUM(bp));
> + printf("Cleared breakpoint %tx (", BP_NUM(bp));
> xmon_print_symbol(bp->address, " ", ")\n");
> bp->enabled =3D 0;
> break;
> @@ -1389,7 +1389,7 @@ bpt_cmds(void)
> for (bp =3D bpts; bp < &bpts[NBPTS]; ++bp) {
> if (!bp->enabled)
> continue;
> - printf("%2x %s ", BP_NUM(bp),
> + printf("%tx %s ", BP_NUM(bp),
> (bp->enabled & BP_CIABR) ? "inst": "t=
rap");
> xmon_print_symbol(bp->address, " ", "\n"=
);
> }
> @@ -1604,11 +1604,11 @@ static void excprint(struct pt_regs *fp)
> #endif /* CONFIG_SMP */
>
> trap =3D TRAP(fp);
> - printf("Vector: %lx %s at [%lx]\n", fp->trap, getvecname(trap), f=
p);
> + printf("Vector: %lx %s at [%p]\n", fp->trap, getvecname(trap), fp=
);
> printf(" pc: ");
> xmon_print_symbol(fp->nip, ": ", "\n");
>
> - printf(" lr: ", fp->link);
> + printf(" lr: ");
> xmon_print_symbol(fp->link, ": ", "\n");
>
> printf(" sp: %lx\n", fp->gpr[1]);
> @@ -1620,13 +1620,13 @@ static void excprint(struct pt_regs *fp)
> printf(" dsisr: %lx\n", fp->dsisr);
> }
>
> - printf(" current =3D 0x%lx\n", current);
> + printf(" current =3D 0x%p\n", current);
> #ifdef CONFIG_PPC64
> - printf(" paca =3D 0x%lx\t softe: %d\t irq_happened: 0x%02x\n"=
,
> + printf(" paca =3D 0x%p\t softe: %d\t irq_happened: 0x%02x\n",
> local_paca, local_paca->irq_soft_mask, local_paca->irq_hap=
pened);
> #endif
> if (current) {
> - printf(" pid =3D %ld, comm =3D %s\n",
> + printf(" pid =3D %d, comm =3D %s\n",
> current->pid, current->comm);
> }
>
> @@ -1662,16 +1662,16 @@ static void prregs(struct pt_regs *fp)
> #ifdef CONFIG_PPC64
> if (FULL_REGS(fp)) {
> for (n =3D 0; n < 16; ++n)
> - printf("R%.2ld =3D "REG" R%.2ld =3D "REG"\n",
> + printf("R%.2d =3D "REG" R%.2d =3D "REG"\n",
> n, fp->gpr[n], n+16, fp->gpr[n+16]);
> } else {
> for (n =3D 0; n < 7; ++n)
> - printf("R%.2ld =3D "REG" R%.2ld =3D "REG"\n",
> + printf("R%.2d =3D "REG" R%.2d =3D "REG"\n",
> n, fp->gpr[n], n+7, fp->gpr[n+7]);
> }
> #else
> for (n =3D 0; n < 32; ++n) {
> - printf("R%.2d =3D %.8x%s", n, fp->gpr[n],
> + printf("R%.2d =3D %.8lx%s", n, fp->gpr[n],
> (n & 3) =3D=3D 3? "\n": " ");
> if (n =3D=3D 12 && !FULL_REGS(fp)) {
> printf("\n");
> @@ -1775,9 +1775,9 @@ static void dump_206_sprs(void)
>
> /* Actually some of these pre-date 2.06, but whatevs */
>
> - printf("srr0 =3D %.16lx srr1 =3D %.16lx dsisr =3D %.8x\n",
> + printf("srr0 =3D %.16lx srr1 =3D %.16lx dsisr =3D %.8lx\n",
> mfspr(SPRN_SRR0), mfspr(SPRN_SRR1), mfspr(SPRN_DSISR));
> - printf("dscr =3D %.16lx ppr =3D %.16lx pir =3D %.8x\n",
> + printf("dscr =3D %.16lx ppr =3D %.16lx pir =3D %.8lx\n",
> mfspr(SPRN_DSCR), mfspr(SPRN_PPR), mfspr(SPRN_PIR));
> printf("amr =3D %.16lx uamor =3D %.16lx\n",
> mfspr(SPRN_AMR), mfspr(SPRN_UAMOR));
> @@ -1785,11 +1785,11 @@ static void dump_206_sprs(void)
> if (!(mfmsr() & MSR_HV))
> return;
>
> - printf("sdr1 =3D %.16lx hdar =3D %.16lx hdsisr =3D %.8x\n",
> + printf("sdr1 =3D %.16lx hdar =3D %.16lx hdsisr =3D %.8lx\n",
> mfspr(SPRN_SDR1), mfspr(SPRN_HDAR), mfspr(SPRN_HDSISR));
> printf("hsrr0 =3D %.16lx hsrr1 =3D %.16lx hdec =3D %.16lx\n",
> mfspr(SPRN_HSRR0), mfspr(SPRN_HSRR1), mfspr(SPRN_HDEC));
> - printf("lpcr =3D %.16lx pcr =3D %.16lx lpidr =3D %.8x\n",
> + printf("lpcr =3D %.16lx pcr =3D %.16lx lpidr =3D %.8lx\n",
> mfspr(SPRN_LPCR), mfspr(SPRN_PCR), mfspr(SPRN_LPID));
> printf("hsprg0 =3D %.16lx hsprg1 =3D %.16lx amor =3D %.16lx\n",
> mfspr(SPRN_HSPRG0), mfspr(SPRN_HSPRG1), mfspr(SPRN_AMOR))=
;
> @@ -1806,10 +1806,10 @@ static void dump_207_sprs(void)
> if (!cpu_has_feature(CPU_FTR_ARCH_207S))
> return;
>
> - printf("dpdes =3D %.16lx tir =3D %.16lx cir =3D %.8x\n",
> + printf("dpdes =3D %.16lx tir =3D %.16lx cir =3D %.8lx\n",
> mfspr(SPRN_DPDES), mfspr(SPRN_TIR), mfspr(SPRN_CIR));
>
> - printf("fscr =3D %.16lx tar =3D %.16lx pspb =3D %.8x\n",
> + printf("fscr =3D %.16lx tar =3D %.16lx pspb =3D %.8lx\n",
> mfspr(SPRN_FSCR), mfspr(SPRN_TAR), mfspr(SPRN_PSPB));
>
> msr =3D mfmsr();
> @@ -1822,12 +1822,12 @@ static void dump_207_sprs(void)
>
> printf("mmcr0 =3D %.16lx mmcr1 =3D %.16lx mmcr2 =3D %.16lx\n",
> mfspr(SPRN_MMCR0), mfspr(SPRN_MMCR1), mfspr(SPRN_MMCR2));
> - printf("pmc1 =3D %.8x pmc2 =3D %.8x pmc3 =3D %.8x pmc4 =3D =
%.8x\n",
> + printf("pmc1 =3D %.8lx pmc2 =3D %.8lx pmc3 =3D %.8lx pmc4 =
=3D %.8lx\n",
> mfspr(SPRN_PMC1), mfspr(SPRN_PMC2),
> mfspr(SPRN_PMC3), mfspr(SPRN_PMC4));
> - printf("mmcra =3D %.16lx siar =3D %.16lx pmc5 =3D %.8x\n",
> + printf("mmcra =3D %.16lx siar =3D %.16lx pmc5 =3D %.8lx\n",
> mfspr(SPRN_MMCRA), mfspr(SPRN_SIAR), mfspr(SPRN_PMC5));
> - printf("sdar =3D %.16lx sier =3D %.16lx pmc6 =3D %.8x\n",
> + printf("sdar =3D %.16lx sier =3D %.16lx pmc6 =3D %.8lx\n",
> mfspr(SPRN_SDAR), mfspr(SPRN_SIER), mfspr(SPRN_PMC6));
> printf("ebbhr =3D %.16lx ebbrr =3D %.16lx bescr =3D %.16lx\n",
> mfspr(SPRN_EBBHR), mfspr(SPRN_EBBRR), mfspr(SPRN_BESCR));
> @@ -2337,22 +2337,25 @@ static void dump_one_paca(int cpu)
>
> #define DUMP(paca, name, format) \
> printf(" %-*s =3D %#-*"format"\t(0x%lx)\n", 20, #name, 18, paca->=
name, \
> - offsetof(struct paca_struct, name));
> + offsetof(struct paca_struct, name))
> +#define DUMPPTR(paca, name, format) \
> + printf(" %-*s =3D %-*"format"\t(0x%lx)\n", 20, #name, 18, paca->n=
ame, \
> + offsetof(struct paca_struct, name))
>
> DUMP(p, lock_token, "x");
> DUMP(p, paca_index, "x");
> - DUMP(p, kernel_toc, "lx");
> - DUMP(p, kernelbase, "lx");
> - DUMP(p, kernel_msr, "lx");
> - DUMP(p, emergency_sp, "px");
> + DUMP(p, kernel_toc, "llx");
> + DUMP(p, kernelbase, "llx");
> + DUMP(p, kernel_msr, "llx");
> + DUMPPTR(p, emergency_sp, "p");
> #ifdef CONFIG_PPC_BOOK3S_64
> - DUMP(p, nmi_emergency_sp, "px");
> - DUMP(p, mc_emergency_sp, "px");
> + DUMPPTR(p, nmi_emergency_sp, "p");
> + DUMPPTR(p, mc_emergency_sp, "p");
> DUMP(p, in_nmi, "x");
> DUMP(p, in_mce, "x");
> DUMP(p, hmi_event_available, "x");
> #endif
> - DUMP(p, data_offset, "lx");
> + DUMP(p, data_offset, "llx");
> DUMP(p, hw_cpu_id, "x");
> DUMP(p, cpu_start, "x");
> DUMP(p, kexec_state, "x");
> @@ -2367,16 +2370,16 @@ static void dump_one_paca(int cpu)
> vsid =3D be64_to_cpu(p->slb_shadow_ptr->save_area[i].vsid=
);
>
> if (esid || vsid) {
> - printf(" slb_shadow[%d]: =3D 0x%016lx 0x%01=
6lx\n",
> + printf(" slb_shadow[%d]: =3D 0x%016llx 0x%0=
16llx\n",
> i, esid, vsid);
> }
> }
> DUMP(p, vmalloc_sllp, "x");
> DUMP(p, slb_cache_ptr, "x");
> for (i =3D 0; i < SLB_CACHE_ENTRIES; i++)
> - printf(" slb_cache[%d]: =3D 0x%016lx\n", i, p->slb=
_cache[i]);
> + printf(" slb_cache[%d]: =3D 0x%016x\n", i, p->slb_=
cache[i]);
>
> - DUMP(p, rfi_flush_fallback_area, "px");
> + DUMPPTR(p, rfi_flush_fallback_area, "p");
> #endif
> DUMP(p, dscr_default, "llx");
> #ifdef CONFIG_PPC_BOOK3E
> @@ -2387,11 +2390,11 @@ static void dump_one_paca(int cpu)
> DUMP(p, crit_kstack, "px");
> DUMP(p, dbg_kstack, "px");
> #endif
> - DUMP(p, __current, "px");
> - DUMP(p, kstack, "lx");
> - printf(" kstack_base =3D 0x%016lx\n", p->kstack & ~(THRE=
AD_SIZE - 1));
> - DUMP(p, stab_rr, "lx");
> - DUMP(p, saved_r1, "lx");
> + DUMPPTR(p, __current, "p");
> + DUMP(p, kstack, "llx");
> + printf(" kstack_base =3D 0x%016llx\n", p->kstack & ~(THR=
EAD_SIZE - 1));
> + DUMP(p, stab_rr, "llx");
> + DUMP(p, saved_r1, "llx");
> DUMP(p, trap_save, "x");
> DUMP(p, irq_soft_mask, "x");
> DUMP(p, irq_happened, "x");
> @@ -2405,20 +2408,20 @@ static void dump_one_paca(int cpu)
> #endif
>
> #ifdef CONFIG_PPC_POWERNV
> - DUMP(p, core_idle_state_ptr, "px");
> + DUMPPTR(p, core_idle_state_ptr, "p");
> DUMP(p, thread_idle_state, "x");
> DUMP(p, thread_mask, "x");
> DUMP(p, subcore_sibling_mask, "x");
> #endif
>
> - DUMP(p, accounting.utime, "llx");
> - DUMP(p, accounting.stime, "llx");
> - DUMP(p, accounting.utime_scaled, "llx");
> - DUMP(p, accounting.starttime, "llx");
> - DUMP(p, accounting.starttime_user, "llx");
> - DUMP(p, accounting.startspurr, "llx");
> - DUMP(p, accounting.utime_sspurr, "llx");
> - DUMP(p, accounting.steal_time, "llx");
> + DUMP(p, accounting.utime, "lx");
> + DUMP(p, accounting.stime, "lx");
> + DUMP(p, accounting.utime_scaled, "lx");
> + DUMP(p, accounting.starttime, "lx");
> + DUMP(p, accounting.starttime_user, "lx");
> + DUMP(p, accounting.startspurr, "lx");
> + DUMP(p, accounting.utime_sspurr, "lx");
> + DUMP(p, accounting.steal_time, "lx");
> #undef DUMP
>
> catch_memory_errors =3D 0;
> @@ -2564,7 +2567,7 @@ static void dump_by_size(unsigned long addr, long c=
ount, int size)
> default: val =3D 0;
> }
>
> - printf("%0*lx", size * 2, val);
> + printf("%0*llx", size * 2, val);
> }
> printf("\n");
> }
> @@ -2728,7 +2731,7 @@ generic_inst_dump(unsigned long adr, long count, in=
t praddr,
> dotted =3D 0;
> last_inst =3D inst;
> if (praddr)
> - printf(REG" %.8x", adr, inst);
> + printf(REG" %.8lx", adr, inst);
> printf("\t");
> dump_func(inst, adr);
> printf("\n");
> @@ -2860,7 +2863,7 @@ memdiffs(unsigned char *p1, unsigned char *p2, unsi=
gned nb, unsigned maxpr)
> for( n =3D nb; n > 0; --n )
> if( *p1++ !=3D *p2++ )
> if( ++prt <=3D maxpr )
> - printf("%.16x %.2x # %.16x %.2x\n", p1 - =
1,
> + printf("%p %.2x # %p %.2x\n", p1 - 1,
> p1[-1], p2 - 1, p2[-1]);
> if( prt > maxpr )
> printf("Total of %d differences\n", prt);
> @@ -2920,13 +2923,13 @@ memzcan(void)
> if (ok && !ook) {
> printf("%.8x .. ", a);
> } else if (!ok && ook)
> - printf("%.8x\n", a - mskip);
> + printf("%.8lx\n", a - mskip);
> ook =3D ok;
> if (a + mskip < a)
> break;
> }
> if (ook)
> - printf("%.8x\n", a - mskip);
> + printf("%.8lx\n", a - mskip);
> }
>
> static void show_task(struct task_struct *tsk)
> @@ -3010,13 +3013,13 @@ static void show_pte(unsigned long addr)
> return;
> }
>
> - printf("pgd @ 0x%016lx\n", pgdir);
> + printf("pgd @ 0x%p\n", pgdir);
>
> if (pgd_huge(*pgdp)) {
> format_pte(pgdp, pgd_val(*pgdp));
> return;
> }
> - printf("pgdp @ 0x%016lx =3D 0x%016lx\n", pgdp, pgd_val(*pgdp));
> + printf("pgdp @ 0x%p =3D 0x%016lx\n", pgdp, pgd_val(*pgdp));
>
> pudp =3D pud_offset(pgdp, addr);
>
> @@ -3030,7 +3033,7 @@ static void show_pte(unsigned long addr)
> return;
> }
>
> - printf("pudp @ 0x%016lx =3D 0x%016lx\n", pudp, pud_val(*pudp));
> + printf("pudp @ 0x%p =3D 0x%016lx\n", pudp, pud_val(*pudp));
>
> pmdp =3D pmd_offset(pudp, addr);
>
> @@ -3043,7 +3046,7 @@ static void show_pte(unsigned long addr)
> format_pte(pmdp, pmd_val(*pmdp));
> return;
> }
> - printf("pmdp @ 0x%016lx =3D 0x%016lx\n", pmdp, pmd_val(*pmdp));
> + printf("pmdp @ 0x%p =3D 0x%016lx\n", pmdp, pmd_val(*pmdp));
>
> ptep =3D pte_offset_map(pmdp, addr);
> if (pte_none(*ptep)) {
> @@ -3847,19 +3850,19 @@ static void dump_spu_fields(struct spu *spu)
> DUMP_FIELD(spu, "0x%lx", ls_size);
> DUMP_FIELD(spu, "0x%x", node);
> DUMP_FIELD(spu, "0x%lx", flags);
> - DUMP_FIELD(spu, "%d", class_0_pending);
> - DUMP_FIELD(spu, "0x%lx", class_0_dar);
> - DUMP_FIELD(spu, "0x%lx", class_1_dar);
> - DUMP_FIELD(spu, "0x%lx", class_1_dsisr);
> - DUMP_FIELD(spu, "0x%lx", irqs[0]);
> - DUMP_FIELD(spu, "0x%lx", irqs[1]);
> - DUMP_FIELD(spu, "0x%lx", irqs[2]);
> + DUMP_FIELD(spu, "%llu", class_0_pending);
> + DUMP_FIELD(spu, "0x%llx", class_0_dar);
> + DUMP_FIELD(spu, "0x%llx", class_1_dar);
> + DUMP_FIELD(spu, "0x%llx", class_1_dsisr);
> + DUMP_FIELD(spu, "0x%x", irqs[0]);
> + DUMP_FIELD(spu, "0x%x", irqs[1]);
> + DUMP_FIELD(spu, "0x%x", irqs[2]);
> DUMP_FIELD(spu, "0x%x", slb_replace);
> DUMP_FIELD(spu, "%d", pid);
> DUMP_FIELD(spu, "0x%p", mm);
> DUMP_FIELD(spu, "0x%p", ctx);
> DUMP_FIELD(spu, "0x%p", rq);
> - DUMP_FIELD(spu, "0x%p", timestamp);
> + DUMP_FIELD(spu, "0x%llx", timestamp);
> DUMP_FIELD(spu, "0x%lx", problem_phys);
> DUMP_FIELD(spu, "0x%p", problem);
> DUMP_VALUE("0x%x", problem->spu_runcntl_RW,
> @@ -3890,7 +3893,7 @@ static void dump_spu_ls(unsigned long num, int subc=
md)
> __delay(200);
> } else {
> catch_memory_errors =3D 0;
> - printf("*** Error: accessing spu info for spu %d\n", num)=
;
> + printf("*** Error: accessing spu info for spu %ld\n", num=
);
> return;
> }
> catch_memory_errors =3D 0;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Mathieu Malaterre @ 2018-04-24 18:53 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, LKML,
linuxppc-dev
In-Reply-To: <20180424160425.DB9946C5A7@po15720vm.idsi0.si.c-s.fr>
On Tue, Apr 24, 2018 at 6:04 PM, Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
> Use fault_in_pages_readable() to prefault user context
> instead of open coding
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v2: using sizeof(*ctx) as size of ctx instead of 1
>
> arch/powerpc/kernel/signal_32.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 492f03451877..4a9e4d6d555b 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -25,6 +25,7 @@
> #include <linux/errno.h>
> #include <linux/elf.h>
> #include <linux/ptrace.h>
> +#include <linux/pagemap.h>
> #include <linux/ratelimit.h>
> #ifdef CONFIG_PPC64
> #include <linux/syscalls.h>
> @@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
> struct ucontext __user *new_ctx,
> int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
> {
> - unsigned char tmp __maybe_unused;
> int ctx_has_vsx_region = 0;
>
> #ifdef CONFIG_PPC64
> @@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
> }
> if (new_ctx == NULL)
> return 0;
> - if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
> - || __get_user(tmp, (u8 __user *) new_ctx)
> - || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
> + if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
> + fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
> return -EFAULT;
>
> /*
> @@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
> {
> struct sig_dbg_op op;
> int i;
> - unsigned char tmp __maybe_unused;
> unsigned long new_msr = regs->msr;
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> unsigned long new_dbcr0 = current->thread.debug.dbcr0;
> @@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
> current->thread.debug.dbcr0 = new_dbcr0;
> #endif
>
> - if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
> - || __get_user(tmp, (u8 __user *) ctx)
> - || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
> + if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
> + fault_in_pages_readable((u8 __user *)ctx, sizeof(*ctx)))
> return -EFAULT;
>
> /*
> --
> 2.13.3
>
Looks good:
Reviewed-by: Mathieu Malaterre <malat@debian.org>
Thanks for fixing my previous attempt !
^ permalink raw reply
* Re: [PATCH] powerpc: Allow selection of CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
From: Mathieu Malaterre @ 2018-04-24 18:48 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe LEROY, Benjamin Herrenschmidt, Paul Mackerras,
linuxppc-dev, LKML
In-Reply-To: <87fu3spmy9.fsf@concordia.ellerman.id.au>
On Wed, Apr 18, 2018 at 5:13 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Mathieu Malaterre <malat@debian.org> writes:
>> On Wed, Apr 18, 2018 at 8:34 AM, Christophe LEROY
> ...
>>
>>> Can you also provide a copy of the messages you can see (prom_init ...) when
>>> boot is ok ?
>>
>> Hum. I've always been interested in seeing it also myself. Is there a
>> way to setup env to see those message (netconsole, delayed boot
>> messages ...) ? I never found a clear documentation on how to do that
>> on (closed) Apple hardware.
>
> If you see nothing after prom_init it usually indicates the kernel died
> very early in boot before it could find the console.
>
> The only option then is to enable one of the hard-coded EARLY_DEBUG
> options.
Right I see them now.
> I don't know which one works on a G4, maybe CONFIG_PPC_EARLY_DEBUG_BOOTX ?
Exactly !
> I assume it doesn't have a serial port.
- 56K Modem line (yes really!)
- 2 USB
- 1 Firewire
:(
> cheers
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/powernv: Fix OPAL RTC driver OPAL_BUSY loops
From: Alexandre Belloni @ 2018-04-24 18:39 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, Benjamin Herrenschmidt, linux-rtc
In-Reply-To: <20180410230136.7d0b357e@roar.ozlabs.ibm.com>
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.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [RFC v2 3/3] postmigration/memory: Associativity & ibm,dynamic-memory-v2
From: Nathan Fontenot @ 2018-04-24 17:17 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev; +Cc: John Allen, Tyrel Datwyler, Thomas Falcon
In-Reply-To: <28b32a01-3214-90be-9d24-9c717673fe5b@linux.vnet.ibm.com>
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.
> 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.
-Nathan
> }
>
> #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;
>
^ permalink raw reply
* Re: [RFC v2 2/3] postmigration/memory: Review assoc lookup array changes
From: Nathan Fontenot @ 2018-04-24 17:01 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev; +Cc: John Allen, Tyrel Datwyler, Thomas Falcon
In-Reply-To: <02fbce66-e91a-e553-21da-b9deedc2c528@linux.vnet.ibm.com>
On 02/26/2018 02:53 PM, Michael Bringmann wrote:
> postmigration/memory: In an LPAR migration scenario, the property
> "ibm,associativity-lookup-arrays" may change. In the event that a
> row of the array differs, locate all assigned memory blocks with that
> 'aa_index' and 're-add' them to the system memory block data structures.
> In the process of the 're-add', the appropriate entry of the property
> 'ibm,dynamic-memory' would be updated as well as any other applicable
> system data structures.
>
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> Changes in RFC v2:
> -- Simplify code to update memory nodes during mobility checks.
> Remove functions to generate extra HP_ELOG messages in favor
> of direct function calls to dlpar_memory_readd_by_index.
> ---
> arch/powerpc/platforms/pseries/hotplug-memory.c | 120 +++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 2341eae..b63181d 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -1051,6 +1051,123 @@ 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 int pseries_update_ala_memory_aai(int aa_index,
> + struct property *dmprop)
> +{
> + struct of_drconf_cell *drmem;
> + u32 entries;
> + __be32 *p;
> + int i;
> + int rc = 0;
> +
> + p = (__be32 *) dmprop->value;
> + if (!p)
> + 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.
> + */
> + entries = be32_to_cpu(*p++);
> + drmem = (struct of_drconf_cell *)p;
> +
> + 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));
> + }
> + }
> +
> + return rc;
> +}
> +
> +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;
> + __be32 *p;
> + int i, lim;
> +
> + if (rtas_hp_event)
> + return 0;
> +
> + dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> + if (!dn)
> + return -ENODEV;
> +
> + dmprop = of_find_property(dn, "ibm,dynamic-memory", NULL);
> + if (!dmprop) {
> + of_node_put(dn);
> + return -ENODEV;
> + }
> +
> + /*
> + * The layout of the ibm,associativity-lookup-arrays
> + * property is a number N indicating the number of
> + * associativity arrays, followed by a number M
> + * indicating the size of each associativity array,
> + * followed by a list of N associativity arrays.
> + */
> +
> + p = (__be32 *) pr->old_prop->value;
> + if (!p) {
> + of_node_put(dn);
> + return -EINVAL;
> + }
> + old_ala.n_arrays = of_read_number(p++, 1);
> + old_ala.array_sz = of_read_number(p++, 1);
> + old_ala.arrays = p;
> +
> + p = (__be32 *) pr->prop->value;
> + if (!p) {
> + of_node_put(dn);
> + return -EINVAL;
> + }
> + new_ala.n_arrays = of_read_number(p++, 1);
> + new_ala.array_sz = of_read_number(p++, 1);
> + new_ala.arrays = p;
> +
> + lim = (new_ala.n_arrays > old_ala.n_arrays) ? old_ala.n_arrays :
> + new_ala.n_arrays;
> +
> + if (old_ala.array_sz == new_ala.array_sz) {
> +
> + for (i = 0; i < lim; i++) {
> + int index = (i * new_ala.array_sz);
> +
> + if (!memcmp(&old_ala.arrays[index],
> + &new_ala.arrays[index],
> + new_ala.array_sz))
> + continue;
> +
> + pseries_update_ala_memory_aai(i, dmprop);
> + }
> +
> + for (i = lim; i < new_ala.n_arrays; i++)
> + pseries_update_ala_memory_aai(i, dmprop);
> +
> + } else {
> + /* Update all entries representing these rows;
> + * as all rows have different sizes, none can
> + * have equivalent values.
> + */
> + for (i = 0; i < lim; i++)
> + pseries_update_ala_memory_aai(i, dmprop);
> + }
> +
> + of_node_put(dn);
> + return 0;
> +}
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.
-Nathan
> +
> 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);
>
^ permalink raw reply
* Re: [RFC v2 1/3] hotplug/mobility: Apply assoc updates for Post Migration Topo
From: Nathan Fontenot @ 2018-04-24 16:56 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev; +Cc: John Allen, Tyrel Datwyler, Thomas Falcon
In-Reply-To: <54713533-2d91-60f0-5901-02b41a2b948d@linux.vnet.ibm.com>
On 02/26/2018 02:52 PM, Michael Bringmann wrote:
> hotplug/mobility: Recognize more changes to the associativity of
> memory blocks described by the 'ibm,dynamic-memory' and 'cpu'
> properties when processing the topology of LPARS in Post Migration
> events. Previous efforts only recognized whether a memory block's
> assignment had changed in the property. Changes here include:
>
> * Checking the aa_index values of the old/new properties and 'readd'
> any block for which the setting has changed.
> * Checking for changes in cpu associativity and making 'readd' calls
> when differences are observed.
As part of the post-migration updates do you need to hold a lock
so that we don't attempt to process any of the cpu/memory changes
while the device tree is being updated?
You may be able to grab the device hotplug lock for this.
>
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> Changes in RFC:
> -- Simplify code to update CPU nodes during mobility checks.
> Remove functions to generate extra HP_ELOG messages in favor
> of direct function calls to dlpar_cpu_readd_by_index.
> -- Move check for "cpu" node type from pseries_update_cpu to
> pseries_smp_notifier in 'hotplug-cpu.c'
> -- Remove functions 'pseries_memory_readd_by_index' and
> 'pseries_cpu_readd_by_index' as no longer needed outside of
> 'mobility.c'.
> ---
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 69 +++++++++++++++++++++++
> arch/powerpc/platforms/pseries/hotplug-memory.c | 6 ++
> 2 files changed, 75 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index a7d14aa7..91ef22a 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -636,6 +636,27 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
> return rc;
> }
>
> +static int dlpar_cpu_readd_by_index(u32 drc_index)
> +{
> + int rc = 0;
> +
> + pr_info("Attempting to update CPU, drc index %x\n", drc_index);
Should make this say we are re-adding the CPU, it's a bit more specific as
to what is really happening.
> +
> + if (dlpar_cpu_remove_by_index(drc_index))
> + rc = -EINVAL;
> + else if (dlpar_cpu_add(drc_index))
> + rc = -EINVAL;
> +
> + if (rc)
> + pr_info("Failed to update cpu at drc_index %lx\n",
> + (unsigned long int)drc_index);
> + else
> + pr_info("CPU at drc_index %lx was updated\n",
> + (unsigned long int)drc_index);
> +
> + return rc;
> +}
> +
> static int find_dlpar_cpus_to_remove(u32 *cpu_drcs, int cpus_to_remove)
> {
> struct device_node *dn;
> @@ -826,6 +847,9 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
> else
> rc = -EINVAL;
> break;
> + case PSERIES_HP_ELOG_ACTION_READD:
> + rc = dlpar_cpu_readd_by_index(drc_index);
> + break;
> default:
> pr_err("Invalid action (%d) specified\n", hp_elog->action);
> rc = -EINVAL;
> @@ -876,12 +900,53 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
>
> #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
>
> +static int pseries_update_cpu(struct of_reconfig_data *pr)
> +{
> + u32 old_entries, new_entries;
> + __be32 *p, *old_assoc, *new_assoc;
> + int rc = 0;
> +
> + /* So far, we only handle the 'ibm,associativity' property,
> + * here.
> + * The first int of the property is the number of domains
> + * described. This is followed by an array of level values.
> + */
> + p = (__be32 *) pr->old_prop->value;
> + if (!p)
> + return -EINVAL;
> + old_entries = be32_to_cpu(*p++);
> + old_assoc = p;
> +
> + p = (__be32 *)pr->prop->value;
> + if (!p)
> + return -EINVAL;
> + new_entries = be32_to_cpu(*p++);
> + new_assoc = p;
> +
> + if (old_entries == new_entries) {
> + int sz = old_entries * sizeof(int);
> +
> + if (!memcmp(old_assoc, new_assoc, sz))
> + rc = dlpar_cpu_readd_by_index(
> + be32_to_cpu(pr->dn->phandle));
> +
> + } else {
> + rc = dlpar_cpu_readd_by_index(
> + be32_to_cpu(pr->dn->phandle));
> + }
> +
> + return rc;
> +}
Do we need to do the full compare of the new vs. the old affinity property?
I would think we would only get an updated property if the property changes.
We don't care what changes in the property at this point, only that it changed.
You could just call dlpar_cpu_readd_by_index() directly.
-Nathan
> +
> static int pseries_smp_notifier(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> struct of_reconfig_data *rd = data;
> int err = 0;
>
> + if (strcmp(rd->dn->type, "cpu"))
> + return notifier_from_errno(err);
> +
> switch (action) {
> case OF_RECONFIG_ATTACH_NODE:
> err = pseries_add_processor(rd->dn);
> @@ -889,6 +954,10 @@ static int pseries_smp_notifier(struct notifier_block *nb,
> case OF_RECONFIG_DETACH_NODE:
> pseries_remove_processor(rd->dn);
> break;
> + case OF_RECONFIG_UPDATE_PROPERTY:
> + if (!strcmp(rd->prop->name, "ibm,associativity"))
> + err = pseries_update_cpu(rd);
> + break;
> }
> return notifier_from_errno(err);
> }
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c1578f5..2341eae 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -1040,6 +1040,12 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
> memblock_size);
> rc = (rc < 0) ? -EINVAL : 0;
> break;
> + } else if ((be32_to_cpu(old_drmem[i].aa_index) !=
> + be32_to_cpu(new_drmem[i].aa_index)) &&
> + (be32_to_cpu(new_drmem[i].flags) &
> + DRCONF_MEM_ASSIGNED)) {
> + rc = dlpar_memory_readd_by_index(
> + be32_to_cpu(new_drmem[i].drc_index))> }
> }
> return rc;
>
^ permalink raw reply
* [PATCH 3/3] powerpc/nohash: use IS_ENABLED() to simplify __set_pte_at()
From: Christophe Leroy @ 2018-04-24 16:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <02633d43f29e1ba01865cd334216dc8efb8b4b11.1524587425.git.christophe.leroy@c-s.fr>
By using IS_ENABLED() we can simplify __set_pte_at() by removing
redundant *ptep = pte
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/nohash/pgtable.h | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index f2fe3cbe90af..077472640b35 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -148,40 +148,33 @@ extern void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte, int percpu)
{
-#if defined(CONFIG_PPC32) && defined(CONFIG_PTE_64BIT)
/* Second case is 32-bit with 64-bit PTE. In this case, we
* can just store as long as we do the two halves in the right order
* with a barrier in between.
* In the percpu case, we also fallback to the simple update
*/
- if (percpu) {
- *ptep = pte;
+ if (IS_ENABLED(CONFIG_PPC32) && IS_ENABLED(CONFIG_PTE_64BIT) && !percpu) {
+ __asm__ __volatile__("\
+ stw%U0%X0 %2,%0\n\
+ eieio\n\
+ stw%U0%X0 %L2,%1"
+ : "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
+ : "r" (pte) : "memory");
return;
}
- __asm__ __volatile__("\
- stw%U0%X0 %2,%0\n\
- eieio\n\
- stw%U0%X0 %L2,%1"
- : "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
- : "r" (pte) : "memory");
-
-#else
/* Anything else just stores the PTE normally. That covers all 64-bit
* cases, and 32-bit non-hash with 32-bit PTEs.
*/
*ptep = pte;
-#ifdef CONFIG_PPC_BOOK3E_64
/*
* With hardware tablewalk, a sync is needed to ensure that
* subsequent accesses see the PTE we just wrote. Unlike userspace
* mappings, we can't tolerate spurious faults, so make sure
* the new PTE will be seen the first time.
*/
- if (is_kernel_addr(addr))
+ if (IS_ENABLED(CONFIG_PPC_BOOK3E_64) && is_kernel_addr(addr))
mb();
-#endif
-#endif
}
--
2.13.3
^ permalink raw reply related
* [PATCH 2/3] powerpc/nohash: remove _PAGE_BUSY
From: Christophe Leroy @ 2018-04-24 16:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
In-Reply-To: <02633d43f29e1ba01865cd334216dc8efb8b4b11.1524587425.git.christophe.leroy@c-s.fr>
_PAGE_BUSY is always 0, remove it
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/nohash/64/pgtable.h | 10 +++-------
arch/powerpc/include/asm/nohash/pte-book3e.h | 5 -----
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 251d74c9013e..e8de7cb4d3fb 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -186,14 +186,12 @@ static inline unsigned long pte_update(struct mm_struct *mm,
__asm__ __volatile__(
"1: ldarx %0,0,%3 # pte_update\n\
- andi. %1,%0,%6\n\
- bne- 1b \n\
andc %1,%0,%4 \n\
- or %1,%1,%7\n\
+ or %1,%1,%6\n\
stdcx. %1,0,%3 \n\
bne- 1b"
: "=&r" (old), "=&r" (tmp), "=m" (*ptep)
- : "r" (ptep), "r" (clr), "m" (*ptep), "i" (_PAGE_BUSY), "r" (set)
+ : "r" (ptep), "r" (clr), "m" (*ptep), "r" (set)
: "cc" );
#else
unsigned long old = pte_val(*ptep);
@@ -290,13 +288,11 @@ static inline void __ptep_set_access_flags(struct mm_struct *mm,
__asm__ __volatile__(
"1: ldarx %0,0,%4\n\
- andi. %1,%0,%6\n\
- bne- 1b \n\
or %0,%3,%0\n\
stdcx. %0,0,%4\n\
bne- 1b"
:"=&r" (old), "=&r" (tmp), "=m" (*ptep)
- :"r" (bits), "r" (ptep), "m" (*ptep), "i" (_PAGE_BUSY)
+ :"r" (bits), "r" (ptep), "m" (*ptep)
:"cc");
#else
unsigned long old = pte_val(*ptep);
diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h b/arch/powerpc/include/asm/nohash/pte-book3e.h
index 9ff51b4c0cac..12730b81cd98 100644
--- a/arch/powerpc/include/asm/nohash/pte-book3e.h
+++ b/arch/powerpc/include/asm/nohash/pte-book3e.h
@@ -57,13 +57,8 @@
#define _PAGE_USER (_PAGE_BAP_UR | _PAGE_BAP_SR) /* Can be read */
#define _PAGE_PRIVILEGED (_PAGE_BAP_SR)
-#define _PAGE_BUSY 0
-
#define _PAGE_SPECIAL _PAGE_SW0
-/* Flags to be preserved on PTE modifications */
-#define _PAGE_HPTEFLAGS _PAGE_BUSY
-
/* Base page size */
#ifdef CONFIG_PPC_64K_PAGES
#define _PAGE_PSIZE _PAGE_PSIZE_64K
--
2.13.3
^ permalink raw reply related
* [PATCH 1/3] powerpc/nohash: remove hash related code from nohash headers.
From: Christophe Leroy @ 2018-04-24 16:31 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
When nohash and book3s header were split, some hash related stuff
remained in the nohash header. This patch removes them.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/nohash/32/pgtable.h | 29 +++------------------
arch/powerpc/include/asm/nohash/64/pgtable.h | 16 ++----------
arch/powerpc/include/asm/nohash/pgtable.h | 38 +++-------------------------
arch/powerpc/include/asm/nohash/pte-book3e.h | 1 -
4 files changed, 10 insertions(+), 74 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index a717b5c39b9c..b413abcd5a09 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -129,7 +129,7 @@ extern int icache_44x_need_flush;
#ifndef __ASSEMBLY__
#define pte_clear(mm, addr, ptep) \
- do { pte_update(ptep, ~_PAGE_HASHPTE, 0); } while (0)
+ do { pte_update(ptep, ~0, 0); } while (0)
#define pmd_none(pmd) (!pmd_val(pmd))
#define pmd_bad(pmd) (pmd_val(pmd) & _PMD_BAD)
@@ -142,21 +142,6 @@ static inline void pmd_clear(pmd_t *pmdp)
/*
- * When flushing the tlb entry for a page, we also need to flush the hash
- * table entry. flush_hash_pages is assembler (for speed) in hashtable.S.
- */
-extern int flush_hash_pages(unsigned context, unsigned long va,
- unsigned long pmdval, int count);
-
-/* Add an HPTE to the hash table */
-extern void add_hash_page(unsigned context, unsigned long va,
- unsigned long pmdval);
-
-/* Flush an entry from the TLB/hash table */
-extern void flush_hash_entry(struct mm_struct *mm, pte_t *ptep,
- unsigned long address);
-
-/*
* PTE updates. This function is called whenever an existing
* valid PTE is updated. This does -not- include set_pte_at()
* which nowadays only sets a new PTE.
@@ -242,12 +227,6 @@ static inline int __ptep_test_and_clear_young(unsigned int context, unsigned lon
{
unsigned long old;
old = pte_update(ptep, _PAGE_ACCESSED, 0);
-#if _PAGE_HASHPTE != 0
- if (old & _PAGE_HASHPTE) {
- unsigned long ptephys = __pa(ptep) & PAGE_MASK;
- flush_hash_pages(context, addr, ptephys, 1);
- }
-#endif
return (old & _PAGE_ACCESSED) != 0;
}
#define ptep_test_and_clear_young(__vma, __addr, __ptep) \
@@ -257,7 +236,7 @@ static inline int __ptep_test_and_clear_young(unsigned int context, unsigned lon
static inline pte_t ptep_get_and_clear(struct mm_struct *mm, unsigned long addr,
pte_t *ptep)
{
- return __pte(pte_update(ptep, ~_PAGE_HASHPTE, 0));
+ return __pte(pte_update(ptep, ~0, 0));
}
#define __HAVE_ARCH_PTEP_SET_WRPROTECT
@@ -285,7 +264,7 @@ static inline void __ptep_set_access_flags(struct mm_struct *mm,
}
#define __HAVE_ARCH_PTE_SAME
-#define pte_same(A,B) (((pte_val(A) ^ pte_val(B)) & ~_PAGE_HASHPTE) == 0)
+#define pte_same(A,B) ((pte_val(A) ^ pte_val(B)) == 0)
/*
* Note that on Book E processors, the pmd contains the kernel virtual
@@ -326,7 +305,7 @@ static inline void __ptep_set_access_flags(struct mm_struct *mm,
/*
* Encode and decode a swap entry.
* Note that the bits we use in a PTE for representing a swap entry
- * must not include the _PAGE_PRESENT bit or the _PAGE_HASHPTE bit (if used).
+ * must not include the _PAGE_PRESENT bit.
* -- paulus
*/
#define __swp_type(entry) ((entry).val & 0x1f)
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 5c5f75d005ad..251d74c9013e 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -173,8 +173,6 @@ static inline void pgd_set(pgd_t *pgdp, unsigned long val)
/* to find an entry in a kernel page-table-directory */
/* This now only contains the vmalloc pages */
#define pgd_offset_k(address) pgd_offset(&init_mm, address)
-extern void hpte_need_flush(struct mm_struct *mm, unsigned long addr,
- pte_t *ptep, unsigned long pte, int huge);
/* Atomic PTE updates */
static inline unsigned long pte_update(struct mm_struct *mm,
@@ -205,11 +203,6 @@ static inline unsigned long pte_update(struct mm_struct *mm,
if (!huge)
assert_pte_locked(mm, addr);
-#ifdef CONFIG_PPC_BOOK3S_64
- if (old & _PAGE_HASHPTE)
- hpte_need_flush(mm, addr, ptep, old, huge);
-#endif
-
return old;
}
@@ -218,7 +211,7 @@ static inline int __ptep_test_and_clear_young(struct mm_struct *mm,
{
unsigned long old;
- if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
+ if (pte_young(*ptep))
return 0;
old = pte_update(mm, addr, ptep, _PAGE_ACCESSED, 0, 0);
return (old & _PAGE_ACCESSED) != 0;
@@ -312,7 +305,7 @@ static inline void __ptep_set_access_flags(struct mm_struct *mm,
}
#define __HAVE_ARCH_PTE_SAME
-#define pte_same(A,B) (((pte_val(A) ^ pte_val(B)) & ~_PAGE_HPTEFLAGS) == 0)
+#define pte_same(A,B) ((pte_val(A) ^ pte_val(B)) == 0)
#define pte_ERROR(e) \
pr_err("%s:%d: bad pte %08lx.\n", __FILE__, __LINE__, pte_val(e))
@@ -324,11 +317,6 @@ static inline void __ptep_set_access_flags(struct mm_struct *mm,
/* Encode and de-code a swap entry */
#define MAX_SWAPFILES_CHECK() do { \
BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS); \
- /* \
- * Don't have overlapping bits with _PAGE_HPTEFLAGS \
- * We filter HPTEFLAGS on set_pte. \
- */ \
- BUILD_BUG_ON(_PAGE_HPTEFLAGS & (0x1f << _PAGE_BIT_SWAP_TYPE)); \
} while (0)
/*
* on pte we don't need handle RADIX_TREE_EXCEPTIONAL_SHIFT;
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index c56de1e8026f..f2fe3cbe90af 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -148,37 +148,16 @@ extern void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte, int percpu)
{
-#if defined(CONFIG_PPC_STD_MMU_32) && defined(CONFIG_SMP) && !defined(CONFIG_PTE_64BIT)
- /* First case is 32-bit Hash MMU in SMP mode with 32-bit PTEs. We use the
- * helper pte_update() which does an atomic update. We need to do that
- * because a concurrent invalidation can clear _PAGE_HASHPTE. If it's a
- * per-CPU PTE such as a kmap_atomic, we do a simple update preserving
- * the hash bits instead (ie, same as the non-SMP case)
- */
- if (percpu)
- *ptep = __pte((pte_val(*ptep) & _PAGE_HASHPTE)
- | (pte_val(pte) & ~_PAGE_HASHPTE));
- else
- pte_update(ptep, ~_PAGE_HASHPTE, pte_val(pte));
-
-#elif defined(CONFIG_PPC32) && defined(CONFIG_PTE_64BIT)
+#if defined(CONFIG_PPC32) && defined(CONFIG_PTE_64BIT)
/* Second case is 32-bit with 64-bit PTE. In this case, we
* can just store as long as we do the two halves in the right order
- * with a barrier in between. This is possible because we take care,
- * in the hash code, to pre-invalidate if the PTE was already hashed,
- * which synchronizes us with any concurrent invalidation.
- * In the percpu case, we also fallback to the simple update preserving
- * the hash bits
+ * with a barrier in between.
+ * In the percpu case, we also fallback to the simple update
*/
if (percpu) {
- *ptep = __pte((pte_val(*ptep) & _PAGE_HASHPTE)
- | (pte_val(pte) & ~_PAGE_HASHPTE));
+ *ptep = pte;
return;
}
-#if _PAGE_HASHPTE != 0
- if (pte_val(*ptep) & _PAGE_HASHPTE)
- flush_hash_entry(mm, ptep, addr);
-#endif
__asm__ __volatile__("\
stw%U0%X0 %2,%0\n\
eieio\n\
@@ -186,15 +165,6 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
: "=m" (*ptep), "=m" (*((unsigned char *)ptep+4))
: "r" (pte) : "memory");
-#elif defined(CONFIG_PPC_STD_MMU_32)
- /* Third case is 32-bit hash table in UP mode, we need to preserve
- * the _PAGE_HASHPTE bit since we may not have invalidated the previous
- * translation in the hash yet (done in a subsequent flush_tlb_xxx())
- * and see we need to keep track that this PTE needs invalidating
- */
- *ptep = __pte((pte_val(*ptep) & _PAGE_HASHPTE)
- | (pte_val(pte) & ~_PAGE_HASHPTE));
-
#else
/* Anything else just stores the PTE normally. That covers all 64-bit
* cases, and 32-bit non-hash with 32-bit PTEs.
diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h b/arch/powerpc/include/asm/nohash/pte-book3e.h
index ccee8eb509bb..9ff51b4c0cac 100644
--- a/arch/powerpc/include/asm/nohash/pte-book3e.h
+++ b/arch/powerpc/include/asm/nohash/pte-book3e.h
@@ -57,7 +57,6 @@
#define _PAGE_USER (_PAGE_BAP_UR | _PAGE_BAP_SR) /* Can be read */
#define _PAGE_PRIVILEGED (_PAGE_BAP_SR)
-#define _PAGE_HASHPTE 0
#define _PAGE_BUSY 0
#define _PAGE_SPECIAL _PAGE_SW0
--
2.13.3
^ permalink raw reply related
* Re: [PATCH] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Christophe LEROY @ 2018-04-24 16:05 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, LKML,
linuxppc-dev
In-Reply-To: <CA+7wUsxXwRVHDrE_bzog2+m+kdyDsC4MnjB4_BMc2owDUe7d8A@mail.gmail.com>
Le 24/04/2018 à 16:50, Mathieu Malaterre a écrit :
> On Tue, Apr 24, 2018 at 4:45 PM, Mathieu Malaterre <malat@debian.org> wrote:
>> On Tue, Apr 24, 2018 at 4:17 PM, Christophe Leroy
>> <christophe.leroy@c-s.fr> wrote:
>>> Use fault_in_pages_readable() to prefault user context
>>> instead of open coding
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>> arch/powerpc/kernel/signal_32.c | 13 +++++--------
>>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>>> index 492f03451877..cfacb2726152 100644
>>> --- a/arch/powerpc/kernel/signal_32.c
>>> +++ b/arch/powerpc/kernel/signal_32.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/errno.h>
>>> #include <linux/elf.h>
>>> #include <linux/ptrace.h>
>>> +#include <linux/pagemap.h>
>>> #include <linux/ratelimit.h>
>>> #ifdef CONFIG_PPC64
>>> #include <linux/syscalls.h>
>>> @@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
>>> struct ucontext __user *new_ctx,
>>> int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
>>> {
>>> - unsigned char tmp __maybe_unused;
>>> int ctx_has_vsx_region = 0;
>>>
>>> #ifdef CONFIG_PPC64
>>> @@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
>>> }
>>> if (new_ctx == NULL)
>>> return 0;
>>> - if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
>>> - || __get_user(tmp, (u8 __user *) new_ctx)
>>> - || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
>>> + if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
>>> + fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
>>> return -EFAULT;
>>>
>>> /*
>>> @@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
>>> {
>>> struct sig_dbg_op op;
>>> int i;
>>> - unsigned char tmp __maybe_unused;
>>> unsigned long new_msr = regs->msr;
>>> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>>> unsigned long new_dbcr0 = current->thread.debug.dbcr0;
>>> @@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
>>> current->thread.debug.dbcr0 = new_dbcr0;
>>> #endif
>>>
>>> - if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
>>> - || __get_user(tmp, (u8 __user *) ctx)
>>> - || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
>>> + if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
>>> + fault_in_pages_readable((u8 __user *)ctx, 1))
>>
>> I believe you meant:
>>
>> fault_in_pages_readable((u8 __user *)new_ctx, ctx_size)
>>
>> Since (u8 __user *) (ctx + 1) - 1 really is (u8 __user *) new_ctx + ctx_size - 1
>
> Without the copy/paste errors:
>
> [...]
> fault_in_pages_readable((u8 __user *)ctx, sizeof(*ctx))
>
> Since (u8 __user *) (ctx + 1) - 1 really is (u8 __user *) ctx + sizeof(*ctx) - 1
> [...]
Oops you're right thanks.
v2 submitted
Christophe
>>> return -EFAULT;
>>>
>>> /*
>>> --
>>> 2.13.3
>>>
^ permalink raw reply
* [PATCH v2] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Christophe Leroy @ 2018-04-24 16:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev, Mathieu Malaterre
Use fault_in_pages_readable() to prefault user context
instead of open coding
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: using sizeof(*ctx) as size of ctx instead of 1
arch/powerpc/kernel/signal_32.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 492f03451877..4a9e4d6d555b 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -25,6 +25,7 @@
#include <linux/errno.h>
#include <linux/elf.h>
#include <linux/ptrace.h>
+#include <linux/pagemap.h>
#include <linux/ratelimit.h>
#ifdef CONFIG_PPC64
#include <linux/syscalls.h>
@@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
struct ucontext __user *new_ctx,
int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
{
- unsigned char tmp __maybe_unused;
int ctx_has_vsx_region = 0;
#ifdef CONFIG_PPC64
@@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
}
if (new_ctx == NULL)
return 0;
- if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
- || __get_user(tmp, (u8 __user *) new_ctx)
- || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
+ if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
+ fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
return -EFAULT;
/*
@@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
{
struct sig_dbg_op op;
int i;
- unsigned char tmp __maybe_unused;
unsigned long new_msr = regs->msr;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long new_dbcr0 = current->thread.debug.dbcr0;
@@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
current->thread.debug.dbcr0 = new_dbcr0;
#endif
- if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
- || __get_user(tmp, (u8 __user *) ctx)
- || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
+ if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
+ fault_in_pages_readable((u8 __user *)ctx, sizeof(*ctx)))
return -EFAULT;
/*
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Randy Dunlap @ 2018-04-24 15:21 UTC (permalink / raw)
To: Greg KH
Cc: Bryant G. Ly, benh, mpe, arnd, corbet, seroyer, mrochs, adreznec,
fbarrat, davem, linus.walleij, akpm, mikey, pombredanne, tlfalcon,
msuchanek, linux-doc, linuxppc-dev
In-Reply-To: <20180424142918.GA2459@kroah.com>
On 04/24/2018 07:29 AM, Greg KH wrote:
> On Mon, Apr 23, 2018 at 02:17:28PM -0700, Randy Dunlap wrote:
>> On 04/23/18 12:53, Greg KH wrote:
>>> On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote:
>>>> On 04/23/18 07:46, Bryant G. Ly wrote:
>>>>> This driver is a logical device which provides an
>>>>> interface between the hypervisor and a management
>>>>> partition.
>>>>>
>>>>> This driver is to be used for the POWER Virtual
>>>>> Management Channel Virtual Adapter on the PowerVM
>>>>> platform. It provides both request/response and
>>>>> async message support through the /dev/ibmvmc node.
>>>>>
>>>>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>>>>> Reviewed-by: Steven Royer <seroyer@linux.vnet.ibm.com>
>>>>> Reviewed-by: Adam Reznechek <adreznec@linux.vnet.ibm.com>
>>>>> Tested-by: Taylor Jakobson <tjakobs@us.ibm.com>
>>>>> Tested-by: Brad Warrum <bwarrum@us.ibm.com>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>>>> ---
>>>>> Documentation/ioctl/ioctl-number.txt | 1 +
>>>>> Documentation/misc-devices/ibmvmc.txt | 161 +++
>>>>> MAINTAINERS | 6 +
>>>>> arch/powerpc/include/asm/hvcall.h | 1 +
>>>>> drivers/misc/Kconfig | 14 +
>>>>> drivers/misc/Makefile | 1 +
>>>>> drivers/misc/ibmvmc.c | 2415 +++++++++++++++++++++++++++++++++
>>>>> drivers/misc/ibmvmc.h | 209 +++
>>>>> 8 files changed, 2808 insertions(+)
>>>>> create mode 100644 Documentation/misc-devices/ibmvmc.txt
>>>>> create mode 100644 drivers/misc/ibmvmc.c
>>>>> create mode 100644 drivers/misc/ibmvmc.h
>>>>
>>>>> diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-devices/ibmvmc.txt
>>>>> new file mode 100644
>>>>> index 0000000..bae1064
>>>>> --- /dev/null
>>>>> +++ b/Documentation/misc-devices/ibmvmc.txt
>>>
>>> Aren't we doing new documentation in .rst format instead of .txt?
>>
>> I am not aware that .rst format is a *requirement* for new documentation.
>>
>> ??
>
> Why wouldn't you create new documentation in that format? Just saves
> the step of having to change it again in the future.
Not everything needs to be in that format.
If a document contains graphics or lots of tables, then sure, use RST.
Do you do kernel Documentation builds yourself? Do you know how slow they are?
--
~Randy
^ permalink raw reply
* Re: [PATCH] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Mathieu Malaterre @ 2018-04-24 14:50 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, LKML,
linuxppc-dev
In-Reply-To: <CA+7wUsz6NNG=sVUYL1n+ntAJUWNvjtpjCHzJmPuQ4Zb7TuFrZA@mail.gmail.com>
On Tue, Apr 24, 2018 at 4:45 PM, Mathieu Malaterre <malat@debian.org> wrote:
> On Tue, Apr 24, 2018 at 4:17 PM, Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>> Use fault_in_pages_readable() to prefault user context
>> instead of open coding
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/signal_32.c | 13 +++++--------
>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index 492f03451877..cfacb2726152 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -25,6 +25,7 @@
>> #include <linux/errno.h>
>> #include <linux/elf.h>
>> #include <linux/ptrace.h>
>> +#include <linux/pagemap.h>
>> #include <linux/ratelimit.h>
>> #ifdef CONFIG_PPC64
>> #include <linux/syscalls.h>
>> @@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
>> struct ucontext __user *new_ctx,
>> int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
>> {
>> - unsigned char tmp __maybe_unused;
>> int ctx_has_vsx_region = 0;
>>
>> #ifdef CONFIG_PPC64
>> @@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
>> }
>> if (new_ctx == NULL)
>> return 0;
>> - if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
>> - || __get_user(tmp, (u8 __user *) new_ctx)
>> - || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
>> + if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
>> + fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
>> return -EFAULT;
>>
>> /*
>> @@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
>> {
>> struct sig_dbg_op op;
>> int i;
>> - unsigned char tmp __maybe_unused;
>> unsigned long new_msr = regs->msr;
>> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>> unsigned long new_dbcr0 = current->thread.debug.dbcr0;
>> @@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
>> current->thread.debug.dbcr0 = new_dbcr0;
>> #endif
>>
>> - if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
>> - || __get_user(tmp, (u8 __user *) ctx)
>> - || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
>> + if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
>> + fault_in_pages_readable((u8 __user *)ctx, 1))
>
> I believe you meant:
>
> fault_in_pages_readable((u8 __user *)new_ctx, ctx_size)
>
> Since (u8 __user *) (ctx + 1) - 1 really is (u8 __user *) new_ctx + ctx_size - 1
Without the copy/paste errors:
[...]
fault_in_pages_readable((u8 __user *)ctx, sizeof(*ctx))
Since (u8 __user *) (ctx + 1) - 1 really is (u8 __user *) ctx + sizeof(*ctx) - 1
[...]
>> return -EFAULT;
>>
>> /*
>> --
>> 2.13.3
>>
^ permalink raw reply
* Re: [PATCH] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Mathieu Malaterre @ 2018-04-24 14:45 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, LKML,
linuxppc-dev
In-Reply-To: <20180424141759.F02AF6C59D@po15720vm.idsi0.si.c-s.fr>
On Tue, Apr 24, 2018 at 4:17 PM, Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
> Use fault_in_pages_readable() to prefault user context
> instead of open coding
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/signal_32.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 492f03451877..cfacb2726152 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -25,6 +25,7 @@
> #include <linux/errno.h>
> #include <linux/elf.h>
> #include <linux/ptrace.h>
> +#include <linux/pagemap.h>
> #include <linux/ratelimit.h>
> #ifdef CONFIG_PPC64
> #include <linux/syscalls.h>
> @@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
> struct ucontext __user *new_ctx,
> int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
> {
> - unsigned char tmp __maybe_unused;
> int ctx_has_vsx_region = 0;
>
> #ifdef CONFIG_PPC64
> @@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
> }
> if (new_ctx == NULL)
> return 0;
> - if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
> - || __get_user(tmp, (u8 __user *) new_ctx)
> - || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
> + if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
> + fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
> return -EFAULT;
>
> /*
> @@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
> {
> struct sig_dbg_op op;
> int i;
> - unsigned char tmp __maybe_unused;
> unsigned long new_msr = regs->msr;
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> unsigned long new_dbcr0 = current->thread.debug.dbcr0;
> @@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
> current->thread.debug.dbcr0 = new_dbcr0;
> #endif
>
> - if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
> - || __get_user(tmp, (u8 __user *) ctx)
> - || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
> + if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
> + fault_in_pages_readable((u8 __user *)ctx, 1))
I believe you meant:
fault_in_pages_readable((u8 __user *)new_ctx, ctx_size)
Since (u8 __user *) (ctx + 1) - 1 really is (u8 __user *) new_ctx + ctx_size - 1
> return -EFAULT;
>
> /*
> --
> 2.13.3
>
^ permalink raw reply
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Greg KH @ 2018-04-24 14:29 UTC (permalink / raw)
To: Randy Dunlap
Cc: Bryant G. Ly, benh, mpe, arnd, corbet, seroyer, mrochs, adreznec,
fbarrat, davem, linus.walleij, akpm, mikey, pombredanne, tlfalcon,
msuchanek, linux-doc, linuxppc-dev
In-Reply-To: <4142fd3b-05eb-d934-bcd3-f8f564e24fa7@infradead.org>
On Mon, Apr 23, 2018 at 02:17:28PM -0700, Randy Dunlap wrote:
> On 04/23/18 12:53, Greg KH wrote:
> > On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote:
> >> On 04/23/18 07:46, Bryant G. Ly wrote:
> >>> This driver is a logical device which provides an
> >>> interface between the hypervisor and a management
> >>> partition.
> >>>
> >>> This driver is to be used for the POWER Virtual
> >>> Management Channel Virtual Adapter on the PowerVM
> >>> platform. It provides both request/response and
> >>> async message support through the /dev/ibmvmc node.
> >>>
> >>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> >>> Reviewed-by: Steven Royer <seroyer@linux.vnet.ibm.com>
> >>> Reviewed-by: Adam Reznechek <adreznec@linux.vnet.ibm.com>
> >>> Tested-by: Taylor Jakobson <tjakobs@us.ibm.com>
> >>> Tested-by: Brad Warrum <bwarrum@us.ibm.com>
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Cc: Arnd Bergmann <arnd@arndb.de>
> >>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>> Cc: Michael Ellerman <mpe@ellerman.id.au>
> >>> ---
> >>> Documentation/ioctl/ioctl-number.txt | 1 +
> >>> Documentation/misc-devices/ibmvmc.txt | 161 +++
> >>> MAINTAINERS | 6 +
> >>> arch/powerpc/include/asm/hvcall.h | 1 +
> >>> drivers/misc/Kconfig | 14 +
> >>> drivers/misc/Makefile | 1 +
> >>> drivers/misc/ibmvmc.c | 2415 +++++++++++++++++++++++++++++++++
> >>> drivers/misc/ibmvmc.h | 209 +++
> >>> 8 files changed, 2808 insertions(+)
> >>> create mode 100644 Documentation/misc-devices/ibmvmc.txt
> >>> create mode 100644 drivers/misc/ibmvmc.c
> >>> create mode 100644 drivers/misc/ibmvmc.h
> >>
> >>> diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-devices/ibmvmc.txt
> >>> new file mode 100644
> >>> index 0000000..bae1064
> >>> --- /dev/null
> >>> +++ b/Documentation/misc-devices/ibmvmc.txt
> >
> > Aren't we doing new documentation in .rst format instead of .txt?
>
> I am not aware that .rst format is a *requirement* for new documentation.
>
> ??
Why wouldn't you create new documentation in that format? Just saves
the step of having to change it again in the future.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] powerpc/signal32: Use fault_in_pages_readable() to prefault user context
From: Christophe Leroy @ 2018-04-24 14:17 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev, Mathieu Malaterre
Use fault_in_pages_readable() to prefault user context
instead of open coding
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/signal_32.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 492f03451877..cfacb2726152 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -25,6 +25,7 @@
#include <linux/errno.h>
#include <linux/elf.h>
#include <linux/ptrace.h>
+#include <linux/pagemap.h>
#include <linux/ratelimit.h>
#ifdef CONFIG_PPC64
#include <linux/syscalls.h>
@@ -1045,7 +1046,6 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
struct ucontext __user *new_ctx,
int ctx_size, int r6, int r7, int r8, struct pt_regs *regs)
{
- unsigned char tmp __maybe_unused;
int ctx_has_vsx_region = 0;
#ifdef CONFIG_PPC64
@@ -1109,9 +1109,8 @@ long sys_swapcontext(struct ucontext __user *old_ctx,
}
if (new_ctx == NULL)
return 0;
- if (!access_ok(VERIFY_READ, new_ctx, ctx_size)
- || __get_user(tmp, (u8 __user *) new_ctx)
- || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
+ if (!access_ok(VERIFY_READ, new_ctx, ctx_size) ||
+ fault_in_pages_readable((u8 __user *)new_ctx, ctx_size))
return -EFAULT;
/*
@@ -1231,7 +1230,6 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
{
struct sig_dbg_op op;
int i;
- unsigned char tmp __maybe_unused;
unsigned long new_msr = regs->msr;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long new_dbcr0 = current->thread.debug.dbcr0;
@@ -1287,9 +1285,8 @@ int sys_debug_setcontext(struct ucontext __user *ctx,
current->thread.debug.dbcr0 = new_dbcr0;
#endif
- if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx))
- || __get_user(tmp, (u8 __user *) ctx)
- || __get_user(tmp, (u8 __user *) (ctx + 1) - 1))
+ if (!access_ok(VERIFY_READ, ctx, sizeof(*ctx)) ||
+ fault_in_pages_readable((u8 __user *)ctx, 1))
return -EFAULT;
/*
--
2.13.3
^ permalink raw reply related
* Re: [PATCH v3 01/19] powerpc/powermac: Mark variable x as unused
From: Christophe LEROY @ 2018-04-24 14:12 UTC (permalink / raw)
To: Mathieu Malaterre, Michael Ellerman
Cc: Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev,
linux-kernel
In-Reply-To: <20180404200746.27379-1-malat@debian.org>
Le 04/04/2018 à 22:07, Mathieu Malaterre a écrit :
> Since the value of x is never intended to be read, declare it with gcc
> attribute as unused. Fix warning treated as error with W=1:
>
> arch/powerpc/platforms/powermac/bootx_init.c:471:21: error: variable ‘x’ set but not used [-Werror=unused-but-set-variable]
>
> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> ---
> v3: style: add missing empty line after declaration
> v2: move x variable within its local scope
>
> arch/powerpc/platforms/powermac/bootx_init.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/powermac/bootx_init.c b/arch/powerpc/platforms/powermac/bootx_init.c
> index c3c9bbb3573a..ba0964c17620 100644
> --- a/arch/powerpc/platforms/powermac/bootx_init.c
> +++ b/arch/powerpc/platforms/powermac/bootx_init.c
> @@ -468,7 +468,7 @@ void __init bootx_init(unsigned long r3, unsigned long r4)
> boot_infos_t *bi = (boot_infos_t *) r4;
> unsigned long hdr;
> unsigned long space;
> - unsigned long ptr, x;
> + unsigned long ptr;
> char *model;
> unsigned long offset = reloc_offset();
>
> @@ -562,6 +562,8 @@ void __init bootx_init(unsigned long r3, unsigned long r4)
> * MMU switched OFF, so this should not be useful anymore.
> */
> if (bi->version < 4) {
> + unsigned long x __maybe_unused;
> +
That's detail, but shouldn't it be marked __always_unused instead ?
Christophe
> bootx_printf("Touching pages...\n");
>
> /*
>
^ permalink raw reply
* Re: [PATCH RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: Cédric Le Goater @ 2018-04-24 7:44 UTC (permalink / raw)
To: Sam Bobroff; +Cc: David Gibson, linuxppc-dev, kvm, kvm-ppc, paulus
In-Reply-To: <20180424031914.GA25846@tungsten.ozlabs.ibm.com>
On 04/24/2018 05:19 AM, Sam Bobroff wrote:
> On Mon, Apr 23, 2018 at 11:06:35AM +0200, Cédric Le Goater wrote:
>> On 04/16/2018 06:09 AM, David Gibson wrote:
>>> On Thu, Apr 12, 2018 at 05:02:06PM +1000, Sam Bobroff wrote:
>>>> It is not currently possible to create the full number of possible
>>>> VCPUs (KVM_MAX_VCPUS) on Power9 with KVM-HV when the guest uses less
>>>> threads per core than it's core stride (or "VSMT mode"). This is
>>>> because the VCORE ID and XIVE offsets to grow beyond KVM_MAX_VCPUS
>>>> even though the VCPU ID is less than KVM_MAX_VCPU_ID.
>>>>
>>>> To address this, "pack" the VCORE ID and XIVE offsets by using
>>>> knowledge of the way the VCPU IDs will be used when there are less
>>>> guest threads per core than the core stride. The primary thread of
>>>> each core will always be used first. Then, if the guest uses more than
>>>> one thread per core, these secondary threads will sequentially follow
>>>> the primary in each core.
>>>>
>>>> So, the only way an ID above KVM_MAX_VCPUS can be seen, is if the
>>>> VCPUs are being spaced apart, so at least half of each core is empty
>>>> and IDs between KVM_MAX_VCPUS and (KVM_MAX_VCPUS * 2) can be mapped
>>>> into the second half of each core (4..7, in an 8-thread core).
>>>>
>>>> Similarly, if IDs above KVM_MAX_VCPUS * 2 are seen, at least 3/4 of
>>>> each core is being left empty, and we can map down into the second and
>>>> third quarters of each core (2, 3 and 5, 6 in an 8-thread core).
>>>>
>>>> Lastly, if IDs above KVM_MAX_VCPUS * 4 are seen, only the primary
>>>> threads are being used and 7/8 of the core is empty, allowing use of
>>>> the 1, 3, 5 and 7 thread slots.
>>>>
>>>> (Strides less than 8 are handled similarly.)
>>>>
>>>> This allows the VCORE ID or offset to be calculated quickly from the
>>>> VCPU ID or XIVE server numbers, without access to the VCPU structure.
>>>>
>>>> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
>>>> ---
>>>> Hello everyone,
>>>>
>>>> I've tested this on P8 and P9, in lots of combinations of host and guest
>>>> threading modes and it has been fine but it does feel like a "tricky"
>>>> approach, so I still feel somewhat wary about it.
>>
>> Have you done any migration ?
>
> No, but I will :-)
>
>>>> I've posted it as an RFC because I have not tested it with guest native-XIVE,
>>>> and I suspect that it will take some work to support it.
>>
>> The KVM XIVE device will be different for XIVE exploitation mode, same structures
>> though. I will send a patchset shortly.
>
> Great. This is probably where conflicts between the host and guest
> numbers will show up. (See dwg's question below.)
The 'server' part looks better than the XICS-over-XIVE glue in fact,
may be because it has not yet been tortured.
Here is my take on the server topic :
All the OPAL calls should take a 'vp_id' of some sort, the one from the
struct kvmppc_xive_vcpu, or the result of a routine translating a guest
side CPU number to a VP id in the range defined for the guest. Moreover,
it would be better to make sure the guest side CPU number is valid in
KVM and do a kvmppc_xive_vcpu lookup each time we use one before calling
OPAL, like that we would also get the associated struct kvmppc_xive_vcpu
and its 'vp_id'.
The 'server_num' of kvmppc_xive_vcpu should probably still be a guest
side CPU number, but we need to check its usage. The only problem is
when it is compared to 'act_server' of 'kvmppc_xive_irq_state'.
if 'act_server' was a VP id that would make our life easier. we could
get rid of xive->vp_base + NUMBER usage in :
xive_native_configure_irq( ..., xive->vp_base + server, ...)
Would it be complex to have a routine converting back a VP id to a
guest side cpu number ? we would need it in get_xive() and get_source()
If we start shuffling the XIVE code in the direction above, I rather
do it to make sure the XIVE native exploitation mode patchset stays in
sync.
>>>> arch/powerpc/include/asm/kvm_book3s.h | 19 +++++++++++++++++++
>>>> arch/powerpc/kvm/book3s_hv.c | 14 ++++++++++----
>>>> arch/powerpc/kvm/book3s_xive.c | 9 +++++++--
>>>> 3 files changed, 36 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>>>> index 376ae803b69c..1295056d564a 100644
>>>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>>>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>>>> @@ -368,4 +368,23 @@ extern int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu);
>>>> #define SPLIT_HACK_MASK 0xff000000
>>>> #define SPLIT_HACK_OFFS 0xfb000000
>>>>
>>>> +/* Pack a VCPU ID from the [0..KVM_MAX_VCPU_ID) space down to the
>>>> + * [0..KVM_MAX_VCPUS) space, while using knowledge of the guest's core stride
>>>> + * (but not it's actual threading mode, which is not available) to avoid
>>>> + * collisions.
>>>> + */
>>>> +static inline u32 kvmppc_pack_vcpu_id(struct kvm *kvm, u32 id)
>>>> +{
>>>> + const int block_offsets[MAX_SMT_THREADS] = {0, 4, 2, 6, 1, 5, 3, 7};
>>>
>>> I'd suggest 1,3,5,7 at the end rather than 1,5,3,7 - accomplishes
>>> roughly the same thing, but I think makes the pattern more obvious.
>
> OK.
>
>>>> + int stride = kvm->arch.emul_smt_mode > 1 ?
>>>> + kvm->arch.emul_smt_mode : kvm->arch.smt_mode;
>>>
>>> AFAICT from BUG_ON()s etc. at the callsites, kvm->arch.smt_mode must
>>> always be 1 when this is called, so the conditional here doesn't seem
>>> useful.
>
> Ah yes, right. (That was an older version when I was thinking of using
> it for P8 as well but that didn't seem to be a good idea.)
>
>>>> + int block = (id / KVM_MAX_VCPUS) * (MAX_SMT_THREADS / stride);
>>>> + u32 packed_id;
>>>> +
>>>> + BUG_ON(block >= MAX_SMT_THREADS);
>>>> + packed_id = (id % KVM_MAX_VCPUS) + block_offsets[block];
>>>> + BUG_ON(packed_id >= KVM_MAX_VCPUS);
>>>> + return packed_id;
>>>> +}
>>>
>>> It took me a while to wrap my head around the packing function, but I
>>> think I got there in the end. It's pretty clever.
>
> Thanks, I'll try to add a better description as well :-)
>
>>> One thing bothers me, though. This certainly packs things under
>>> KVM_MAX_VCPUS, but not necessarily under the actual number of vcpus.
>>> e.g. KVM_MAC_VCPUS==16, 8 vcpus total, stride 8, 2 vthreads/vcore (as
>>> qemu sees it), gives both unpacked IDs (0, 1, 8, 9, 16, 17, 24, 25)
>>> and packed ids of (0, 1, 8, 9, 4, 5, 12, 13) - leaving 2, 3, 6, 7
>>> etc. unused.
>
> That's right. The property it provides is that all the numbers are under
> KVM_MAX_VCPUS (which, see below, is the size of the fixed areas) not
> that they are sequential.
>
>>> So again, the question is what exactly are these remapped IDs useful
>>> for. If we're indexing into a bare array of structures of size
>>> KVM_MAX_VCPUS then we're *already* wasting a bunch of space by having
>>> more entries than vcpus. If we're indexing into something sparser,
>>> then why is the remapping worthwhile?
>
> Well, here's my thinking:
>
> At the moment, kvm->vcores[] and xive->vp_base are both sized by NR_CPUS
> (via KVM_MAX_VCPUS and KVM_MAX_VCORES which are both NR_CPUS). This is
> enough space for the maximum number of VCPUs, and some space is wasted
> when the guest uses less than this (but KVM doesn't know how many will
> be created, so we can't do better easily). The problem is that the
> indicies overflow before all of those VCPUs can be created, not that
> more space is needed.
>
> We could fix the overflow by expanding these areas to KVM_MAX_VCPU_ID
> but that will use 8x the space we use now, and we know that no more than
> KVM_MAX_VCPUS will be used so all this new space is basically wasted.
>
> So remapping seems better if it will work. (Ben H. was strongly against
> wasting more XIVE space if possible.)
remapping is 'nearly' done. kvmppc_xive_vcpu holds both values already.
it's a question of good usage. the KVM XIVE layer should use internally
VP ids and do a translation at the frontier: hcalls and host kernel
routines (get/set_xive)
Thanks,
C.
> In short, remapping provides a way to allow the guest to create it's full set
> of VCPUs without wasting any more space than we do currently, without
> having to do something more complicated like tracking used IDs or adding
> additional KVM CAPs.
>
>>>> +
>>>> #endif /* __ASM_KVM_BOOK3S_H__ */
>>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>>> index 9cb9448163c4..49165cc90051 100644
>>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>>> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
>>>> return threads_per_subcore;
>>>> }
>>>>
>>>> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
>>>> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
>>>> {
>>>> struct kvmppc_vcore *vcore;
>>>>
>>>> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
>>>> init_swait_queue_head(&vcore->wq);
>>>> vcore->preempt_tb = TB_NIL;
>>>> vcore->lpcr = kvm->arch.lpcr;
>>>> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
>>>> + vcore->first_vcpuid = id;
>>>> vcore->kvm = kvm;
>>>> INIT_LIST_HEAD(&vcore->preempt_list);
>>>>
>>>> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
>>>> mutex_lock(&kvm->lock);
>>>> vcore = NULL;
>>>> err = -EINVAL;
>>>> - core = id / kvm->arch.smt_mode;
>>>> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
>>>> + BUG_ON(kvm->arch.smt_mode != 1);
>>>> + core = kvmppc_pack_vcpu_id(kvm, id);
>>>> + } else {
>>>> + core = id / kvm->arch.smt_mode;
>>>> + }
>>>> if (core < KVM_MAX_VCORES) {
>>>> vcore = kvm->arch.vcores[core];
>>>> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
>>>> if (!vcore) {
>>>> err = -ENOMEM;
>>>> - vcore = kvmppc_vcore_create(kvm, core);
>>>> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
>>>> kvm->arch.vcores[core] = vcore;
>>>> kvm->arch.online_vcores++;
>>>> }
>>>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>>>> index f9818d7d3381..681dfe12a5f3 100644
>>>> --- a/arch/powerpc/kvm/book3s_xive.c
>>>> +++ b/arch/powerpc/kvm/book3s_xive.c
>>>> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
>>>> return -EBUSY;
>>>> }
>>>>
>>>> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
>>>> +{
>>>> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
>>>> +}
>>>> +
>>>
>>> I'm finding the XIVE indexing really baffling. There are a bunch of
>>> other places where the code uses (xive->vp_base + NUMBER) directly.
>
> Ugh, yes. It looks like I botched part of my final cleanup and all the
> cases you saw in kvm/book3s_xive.c should have been replaced with a call to
> xive_vp(). I'll fix it and sorry for the confusion.
>
>> This links the QEMU vCPU server NUMBER to a XIVE virtual processor number
>> in OPAL. So we need to check that all used NUMBERs are, first, consistent
>> and then, in the correct range.
>
> Right. My approach was to allow XIVE to keep using server numbers that
> are equal to VCPU IDs, and just pack down the ID before indexing into
> the vp_base area.
>
>>> If those are host side references, I guess they don't need updates for
>>> this.
>
> These are all guest side references.
>
>>> But if that's the case, then how does indexing into the same array
>>> with both host and guest server numbers make sense?
>
> Right, it doesn't make sense to mix host and guest server numbers when
> we're remapping only the guest ones, but in this case (without native
> guest XIVE support) it's just guest ones.
>
>> yes. VPs are allocated with KVM_MAX_VCPUS :
>>
>> xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);
>>
>> but
>>
>> #define KVM_MAX_VCPU_ID (threads_per_subcore * KVM_MAX_VCORES)
>>
>> WE would need to change the allocation of the VPs I guess.
>
> Yes, this is one of the structures that overflow if we don't pack the IDs.
>
>>>> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
>>>> struct kvmppc_xive_src_block *sb,
>>>> struct kvmppc_xive_irq_state *state)
>>>> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>>>> pr_devel("Duplicate !\n");
>>>> return -EEXIST;
>>>> }
>>>> - if (cpu >= KVM_MAX_VCPUS) {
>>>> + if (cpu >= KVM_MAX_VCPU_ID) {>>
>>>> pr_devel("Out of bounds !\n");
>>>> return -EINVAL;
>>>> }
>>>> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>>>> xc->xive = xive;
>>>> xc->vcpu = vcpu;
>>>> xc->server_num = cpu;
>>>> - xc->vp_id = xive->vp_base + cpu;
>>>> + xc->vp_id = xive_vp(xive, cpu);
>>>> xc->mfrr = 0xff;
>>>> xc->valid = true;
>>>>
>>>
>>
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-24 10:47 UTC (permalink / raw)
To: Nicholas Piggin
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy
In-Reply-To: <20180424173147.7bcd86c5@roar.ozlabs.ibm.com>
On 04/24/2018 01:01 PM, Nicholas Piggin wrote:
> On Tue, 24 Apr 2018 12:47:32 +0530
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
>
>> Hi,
>>
>> On 04/24/2018 11:30 AM, Nicholas Piggin wrote:
>>> On Tue, 24 Apr 2018 10:11:46 +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 (unreliable)
>>>> [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
>>>>
>>>> Fix this by using the asynchronus smp_call in the timer interrupt handler.
>>>> We don't have to wait in this handler until the pstates are changed on
>>>> the core. This change will not have any impact on the global pstate
>>>> ramp-down algorithm.
>>>>
>>>> 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>
>>>> ---
>>>> drivers/cpufreq/powernv-cpufreq.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>>>> index 0591874..7e0c752 100644
>>>> --- a/drivers/cpufreq/powernv-cpufreq.c
>>>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>>>> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
>>>> 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);
>>>> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
>>>> }
>>>>
>>>> /*
>>>
>>> This can still deadlock because !wait case still ends up having to wait
>>> if another !wait smp_call_function caller had previously used the
>>> call single data for this cpu.
>>>
>>> If you go this way you would have to use smp_call_function_async, which
>>> is more work.
>>>
>>> As a rule it would be better to avoid smp_call_function entirely if
>>> possible. Can you ensure the timer is running on the right CPU? Use
>>> add_timer_on and try again if the timer is on the wrong CPU, perhaps?
>>>
>>
>> Yeah that is doable we can check for the cpu and re-queue it. We will only
>> ramp-down slower in that case which is no harm.
>
> Great, I'd be much happier avoiding that IPI. I guess it should happen
> quite rarely that we have to queue on a different CPU. I would say just
> do add_timer unless we have migrated to the wrong CPU, then do add_timer_on
> in that case (it's a bit slower).
(The gpstates->timer is initialized with TIMER_PINNED and is a timer per cpufreq
policy / or per core)
We are currently using mod_timer() and this gets triggered in the code-path of
the cpufreq's governor timer which is per-policy (i.e per core in our case).
This ensures the timer is always fired on one of the policy->cpus as the
deferred kworker is also scheduled on one of the policy->cpus.
We were good until this patch 7bc54b652f13119f64e87dd96bb792efbfc5a786
where after we could leave a migrated timer and subsequent re-queues from the
timer context on the wrong cpu. For this I agree we need a add_timer_on() to
correct it.
>
>> (If the targeted core turns out to be offline then we will not queue the timer
>> again as we would have already set the pstate to min in the cpu-down path.)
>
> Something I noticed is that if we can not get the lock (trylock fails),
> then the timer does not get queued again. Should it?
>
Since the gpstates->timer is per-core I am assuming that it should not fail in
the trylock. (which sounds like an unlikely case to me where we have two expiry
on the same timer)
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Nicholas Piggin @ 2018-04-24 7:31 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy
In-Reply-To: <c88d51af-9eb8-6bb3-4201-8909cda0241b@linux.vnet.ibm.com>
On Tue, 24 Apr 2018 12:47:32 +0530
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
> Hi,
>
> On 04/24/2018 11:30 AM, Nicholas Piggin wrote:
> > On Tue, 24 Apr 2018 10:11:46 +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 (unreliable)
> >> [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
> >>
> >> Fix this by using the asynchronus smp_call in the timer interrupt handler.
> >> We don't have to wait in this handler until the pstates are changed on
> >> the core. This change will not have any impact on the global pstate
> >> ramp-down algorithm.
> >>
> >> 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>
> >> ---
> >> drivers/cpufreq/powernv-cpufreq.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> >> index 0591874..7e0c752 100644
> >> --- a/drivers/cpufreq/powernv-cpufreq.c
> >> +++ b/drivers/cpufreq/powernv-cpufreq.c
> >> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
> >> 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);
> >> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
> >> }
> >>
> >> /*
> >
> > This can still deadlock because !wait case still ends up having to wait
> > if another !wait smp_call_function caller had previously used the
> > call single data for this cpu.
> >
> > If you go this way you would have to use smp_call_function_async, which
> > is more work.
> >
> > As a rule it would be better to avoid smp_call_function entirely if
> > possible. Can you ensure the timer is running on the right CPU? Use
> > add_timer_on and try again if the timer is on the wrong CPU, perhaps?
> >
>
> Yeah that is doable we can check for the cpu and re-queue it. We will only
> ramp-down slower in that case which is no harm.
Great, I'd be much happier avoiding that IPI. I guess it should happen
quite rarely that we have to queue on a different CPU. I would say just
do add_timer unless we have migrated to the wrong CPU, then do add_timer_on
in that case (it's a bit slower).
> (If the targeted core turns out to be offline then we will not queue the timer
> again as we would have already set the pstate to min in the cpu-down path.)
Something I noticed is that if we can not get the lock (trylock fails),
then the timer does not get queued again. Should it?
Thanks,
Nick
^ 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