Re: [PATCH v4] pseries: prevent free CPU ids to be reused on another node

[Nathan Lynch]

Laurent Dufour  writes:
>
> Changes since V3, addressing Nathan's comment:
>  - Rename the local variable named 'nid' into 'assigned_node'
> Changes since V2, addressing Nathan's comments:
>  - Remove the retry feature
>  - Reduce the number of local variables (removing 'i')
>  - Add comment about the cpu_add_remove_lock protecting the added CPU mask.
> Changes since V1 (no functional changes):
>  - update the test's output in the commit's description
>  - node_recorded_ids_map should be static
>
> Signed-off-by: Laurent Dufour 

Thanks Laurent.

Reviewed-by: Nathan Lynch 

Re: [PATCH v3] pseries: prevent free CPU ids to be reused on another node

[Nathan Lynch]

Laurent Dufour  writes:
> Changes since V2, addressing Nathan's comments:
>  - Remove the retry feature
>  - Reduce the number of local variables (removing 'i')

I was more interested in not having two variables for NUMA nodes in the
function named 'node' and 'nid', hoping at least one of them could have
a more descriptive name. See below.

>  static int pseries_add_processor(struct device_node *np)
>  {
> - unsigned int cpu;
> + unsigned int cpu, node;
>   cpumask_var_t candidate_mask, tmp;
> - int err = -ENOSPC, len, nthreads, i;
> + int err = -ENOSPC, len, nthreads, nid;
>   const __be32 *intserv;
>  
>   intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", );
> @@ -163,9 +169,17 @@ static int pseries_add_processor(struct device_node *np)
>   zalloc_cpumask_var(_mask, GFP_KERNEL);
>   zalloc_cpumask_var(, GFP_KERNEL);
>  
> + /*
> +  * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
> +  * node id the added CPU belongs to.
> +  */
> + nid = of_node_to_nid(np);
> + if (nid < 0 || !node_possible(nid))
> + nid = first_online_node;
> +
>   nthreads = len / sizeof(u32);
> - for (i = 0; i < nthreads; i++)
> - cpumask_set_cpu(i, tmp);
> + for (cpu = 0; cpu < nthreads; cpu++)
> + cpumask_set_cpu(cpu, tmp);
>  
>   cpu_maps_update_begin();
>  
> @@ -173,6 +187,19 @@ static int pseries_add_processor(struct device_node *np)
>  
>   /* Get a bitmap of unoccupied slots. */
>   cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> +
> + /*
> +  * Remove free ids previously assigned on the other nodes. We can walk
> +  * only online nodes because once a node became online it is not turned
> +  * offlined back.
> +  */
> + for_each_online_node(node) {
> + if (node == nid) /* Keep our node's recorded ids */
> + continue;
> + cpumask_andnot(candidate_mask, candidate_mask,
> +node_recorded_ids_map[node]);
> + }
> +

e.g. change 'nid' to 'assigned_node' or similar, and I think this
becomes easier to follow.

Otherwise the patch looks fine to me now.

Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node

[Nathan Lynch]

Laurent Dufour  writes:
> Le 02/04/2021 à 15:34, Nathan Lynch a écrit :
>> Laurent Dufour  writes:
>>> When a CPU is hot added, the CPU ids are taken from the available mask from
>>> the lower possible set. If that set of values was previously used for CPU
>>> attached to a different node, this seems to application like if these CPUs
>>> have migrated from a node to another one which is not expected in real
>>> life.
>> 
>> This seems like a problem that could affect other architectures or
>> platforms? I guess as long as arch code is responsible for placing new
>> CPUs in cpu_present_mask, that code will have the responsibility of
>> ensuring CPU IDs' NUMA assignments remain stable.
>
> Actually, x86 is already handling this issue in the arch code specific
> code, see 8f54969dc8d6 ("x86/acpi: Introduce persistent storage for
> cpuid <-> apicid mapping"). I didn't check for other architectures but
> as CPU id allocation is in the arch part, I believe this is up to each
> arch to deal with this issue.
>
> Making the CPU id allocation common to all arch is outside the scope
> of this patch.

Well... we'd better avoid a situation where architectures impose
different policies in this area. (I guess we're already in this
situation, which is the problem.) A more maintainable way to achieve
that would be to put the higher-level policy in arch-independent code,
as much as possible.

I don't insist, though.


>> I don't know, should we not fail the request instead of doing the
>> ABI-breaking thing the code in this change is trying to prevent? I
>> don't think a warning in the kernel log is going to help any
>> application that would be affected by this.
>
> That's a really good question. One should argue that the most
> important is to satisfy the CPU add operation, assuming that only few
> are interested in the CPU numbering, while others would prefer the CPU
> adding to fail (which may prevent adding CPUs on another nodes if the
> whole operation is aborted as soon as a CPU add is failing).
>
> I was conservative here, but if failing the operation is the best
> option, then this will make that code simpler, removing the backward
> jump.
>
> Who is deciding?

I favor failing the request. Apart from the implications for user space,
it's not clear to me that allowing the cpu-node relationship to change
once initialized is benign in terms of internal kernel assumptions
(e.g. sched domains, workqueues?). And as you say, it would make for
more straightforward code.

Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node

[Nathan Lynch]

Hi Laurent,

Laurent Dufour  writes:
> When a CPU is hot added, the CPU ids are taken from the available mask from
> the lower possible set. If that set of values was previously used for CPU
> attached to a different node, this seems to application like if these CPUs
> have migrated from a node to another one which is not expected in real
> life.

This seems like a problem that could affect other architectures or
platforms? I guess as long as arch code is responsible for placing new
CPUs in cpu_present_mask, that code will have the responsibility of
ensuring CPU IDs' NUMA assignments remain stable.

[...]

> The effect of this patch can be seen by removing and adding CPUs using the
> Qemu monitor. In the following case, the first CPU from the node 2 is
> removed, then the first one from the node 1 is removed too. Later, the
> first CPU of the node 2 is added back. Without that patch, the kernel will
> numbered these CPUs using the first CPU ids available which are the ones
> freed when removing the second CPU of the node 0. This leads to the CPU ids
> 16-23 to move from the node 1 to the node 2. With the patch applied, the
> CPU ids 32-39 are used since they are the lowest free ones which have not
> been used on another node.
>
> At boot time:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Vanilla kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>
> Patched kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47

Good demonstration of the problem. CPUs 16-23 "move" from node 1 to
node 2.


> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c 
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 12cbffd3c2e3..48c7943b25b0 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -39,6 +39,8 @@
>  /* This version can't take the spinlock, because it never returns */
>  static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>  
> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];

I guess this should have documentation that it must be
accessed/manipulated with cpu_add_remove_lock held?


> +
>  static void rtas_stop_self(void)
>  {
>   static struct rtas_args args;
> @@ -151,29 +153,61 @@ static void pseries_cpu_die(unsigned int cpu)
>   */
>  static int pseries_add_processor(struct device_node *np)
>  {
> - unsigned int cpu;
> + unsigned int cpu, node;
>   cpumask_var_t candidate_mask, tmp;
> - int err = -ENOSPC, len, nthreads, i;
> + int err = -ENOSPC, len, nthreads, i, nid;

>From eight local vars to ten, and the two new variables' names are
"node" and "nid". More distinctive names would help readers.


>   const __be32 *intserv;
> + bool force_reusing = false;
>  
>   intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", );
>   if (!intserv)
>   return 0;
>  
> - zalloc_cpumask_var(_mask, GFP_KERNEL);
> - zalloc_cpumask_var(, GFP_KERNEL);
> + alloc_cpumask_var(_mask, GFP_KERNEL);
> + alloc_cpumask_var(, GFP_KERNEL);
> +
> + /*
> +  * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
> +  * node id the added CPU belongs to.
> +  */
> + nid = of_node_to_nid(np);
> + if (nid < 0 || !node_possible(nid))
> + nid = first_online_node;
>  
>   nthreads = len / sizeof(u32);
> - for (i = 0; i < nthreads; i++)
> - cpumask_set_cpu(i, tmp);
>  
>   cpu_maps_update_begin();
>  
>   BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
>  
> +again:
> + cpumask_clear(candidate_mask);
> + cpumask_clear(tmp);
> + for (i = 0; i < nthreads; i++)
> + cpumask_set_cpu(i, tmp);
> +
>   /* Get a bitmap of unoccupied slots. */
>   cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> +
> + /*
> +  * Remove free ids previously assigned on the other nodes. We can walk
> +  * only online nodes because once a node became online it is not turned
> +  * offlined back.
> +  */
> + if (!force_reusing)
> + for_each_online_node(node) {
> + if (node == nid) /* Keep our node's recorded ids */
> + continue;
> + cpumask_andnot(candidate_mask, candidate_mask,
> +node_recorded_ids_map[node]);
> + }
> +
>   if (cpumask_empty(candidate_mask)) {
> +

Re: Build regressions/improvements in v5.10-rc1

[Nathan Lynch]

Geert Uytterhoeven  writes:
> On Mon, Oct 26, 2020 at 10:46 AM Geert Uytterhoeven
>  wrote:
>> Below is the list of build error/warning regressions/improvements in
>> v5.10-rc1[1] compared to v5.9[2].
>>
>> Summarized:
>>   - build errors: +3/-7
>>   - build warnings: +26/-28
>>
>> Happy fixing! ;-)
>>
>> Thanks to the linux-next team for providing the build service.
>>
>> [1] 
>> http://kisskb.ellerman.id.au/kisskb/branch/linus/head/3650b228f83adda7e5ee532e2b90429c03f7b9ec/
>>  (all 192 configs)
>> [2] 
>> http://kisskb.ellerman.id.au/kisskb/branch/linus/head/bbf5c979011a099af5dc76498918ed7df445635b/
>>  (all 192 configs)
>>
>>
>> *** ERRORS ***
>>
>> 3 error regressions:
>>   + /kisskb/src/arch/um/kernel/skas/clone.c: error: expected declaration 
>> specifiers or '...' before string constant:  => 24:16
>
> um-all{mod,yes}config
>
>>   + error: hotplug-memory.c: undefined reference to 
>> `of_drconf_to_nid_single':  => .text+0x5e0)
>
> powerpc-gcc5/pseries_le_defconfig+NO_NUMA

Probably introduced by:

commit 72cdd117c449896c707fc6cfe5b90978160697d0
Author: Scott Cheloha 
Date:   Wed Sep 16 09:51:22 2020 -0500

pseries/hotplug-memory: hot-add: skip redundant LMB lookup

Scott?

Re: [PATCH 1/1] powerpc/rtas: Implement reentrant rtas call

[Nathan Lynch]

Hi Leonardo,

Leonardo Bras  writes:
> Hello Nathan, thanks for the feedback!
>
> On Fri, 2020-04-10 at 14:28 -0500, Nathan Lynch wrote:
>> Leonardo Bras  writes:
>> > Implement rtas_call_reentrant() for reentrant rtas-calls:
>> > "ibm,int-on", "ibm,int-off",ibm,get-xive" and  "ibm,set-xive".
>> > 
>> > On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
>> > items 2 and 3 say:
>> > 
>> > 2 - For the PowerPC External Interrupt option: The * call must be
>> > reentrant to the number of processors on the platform.
>> > 3 - For the PowerPC External Interrupt option: The * argument call
>> > buffer for each simultaneous call must be physically unique.
>> > 
>> > So, these rtas-calls can be called in a lockless way, if using
>> > a different buffer for each call.
>> > 
>
>> From the language in the spec it's clear that these calls are intended
>> to be reentrant with respect to themselves, but it's less clear to me
>> that they are safe to call simultaneously with respect to each other or
>> arbitrary other RTAS methods.
>
> In my viewpoint, being reentrant to themselves, without being reentrant
> to others would be very difficult to do, considering the way the
> rtas_call is crafted to work.
>
> I mean, I have no experience in rtas code, it's my viewpoint. In my
> thoughts there is something like this:
>
> common_path -> selects function by token -> reentrant function
>   |-> non-reentrant function
>
> If there is one function that is reentrant, it means the common_path
> and function selection by token would need to be reentrant too.

I checked with partition firmware development and these calls can be
used concurrently with arbitrary other RTAS calls, which confirms your
interpretation. Thanks for bearing with me.

Re: [PATCH v3 2/2] powerpc/rtas: Implement reentrant rtas call

[Nathan Lynch]

Hi,

Leonardo Bras  writes:
> +/**
> + * rtas_call_reentrant() - Used for reentrant rtas calls
> + * @token:   Token for desired reentrant RTAS call
> + * @nargs:   Number of Input Parameters
> + * @nret:Number of Output Parameters
> + * @outputs: Array of outputs
> + * @...: Inputs for desired RTAS call
> + *
> + * According to LoPAR documentation, only "ibm,int-on", "ibm,int-off",
> + * "ibm,get-xive" and "ibm,set-xive" are currently reentrant.
> + * Reentrant calls need their own rtas_args buffer, so not using rtas.args, 
> but
> + * PACA one instead.
> + *
> + * Return:   -1 on error,
> + *   First output value of RTAS call if (nret > 0),
> + *   0 otherwise,
> + */
> +
> +int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...)
> +{
> + va_list list;
> + struct rtas_args *args;
> + int i;
> +
> + if (!rtas.entry || token == RTAS_UNKNOWN_SERVICE)
> + return -1;
> +
> + /* We use the per-cpu (PACA) rtas args buffer */
> + args = _paca->reentrant_args;
> +
> + va_start(list, outputs);
> + va_rtas_call_unlocked(args, token, nargs, nret, list);
> + va_end(list);
> +
> + if (nret > 1 && outputs)
> + for (i = 0; i < nret - 1; ++i)
> + outputs[i] = be32_to_cpu(args->rets[i + 1]);

Doesn't this need to be more careful about preemption and exceptions?
I.e. the args structure in the paca needs to be protected from
concurrent use somehow, like any per-cpu data structure.

local_irq_save/restore while accessing local_paca->reentrant_args here
would be sufficient I think?

Re: [PATCH 0/2] pseries/hotplug: Change the default behaviour of cede_offline

[Nathan Lynch]

Gautham R Shenoy  writes:
> The accounting problem in tools such as lparstat with
> "cede_offline=on" is affecting customers who are using these tools for
> capacity-planning. That problem needs a fix in the short-term, for
> which Patch 1 changes the default behaviour of cede_offline from "on"
> to "off". Since this patch would break the existing userspace tools
> that use the CPU-Offline infrastructure to fold CPUs for saving power,
> the sysfs interface allowing a runtime change of cede_offline_enabled
> was provided to enable these userspace tools to cope with minimal
> change.

So we would be putting users in the position of choosing between folding
and correct accounting. :-(

Actually how does changing the offline mechanism to stop-self break the
folding utility?

Re: [PATCH 0/2] pseries/hotplug: Change the default behaviour of cede_offline

[Nathan Lynch]

Gautham R Shenoy  writes:
> On Thu, Sep 12, 2019 at 10:39:45AM -0500, Nathan Lynch wrote:
>> "Gautham R. Shenoy"  writes:
>> > The patchset also defines a new sysfs attribute
>> > "/sys/device/system/cpu/cede_offline_enabled" on PSeries Linux guests
>> > to allow userspace programs to change the state into which the
>> > offlined CPU need to be put to at runtime.
>> 
>> A boolean sysfs interface will become awkward if we need to add another
>> mode in the future.
>> 
>> What do you think about naming the attribute something like
>> 'offline_mode', with the possible values 'extended-cede' and
>> 'rtas-stopped'?
>
> We can do that. However, IMHO in the longer term, on PSeries guests,
> we should have only one offline state - rtas-stopped.  The reason for
> this being, that on Linux, SMT switch is brought into effect through
> the CPU Hotplug interface. The only state in which the SMT switch will
> recognized by the hypervisors such as PHYP is rtas-stopped.

OK. Why "longer term" though, instead of doing it now?


> All other states (such as extended-cede) should in the long-term be
> exposed via the cpuidle interface.
>
> With this in mind, I made the sysfs interface boolean to mirror the
> current "cede_offline" commandline parameter. Eventually when we have
> only one offline-state, we can deprecate the commandline parameter as
> well as the sysfs interface.

I don't care for adding a sysfs interface that is intended from the
beginning to become vestigial...

This strikes me as unnecessarily incremental if you're changing the
default offline state. Any user space programs depending on the current
behavior will have to change anyway (and why is it OK to break them?)

Why isn't the plan:

  1. Add extended cede support to the pseries cpuidle driver
  2. Make stop-self the only cpu offline state for pseries (no sysfs
 interface necessary)

?

Re: [PATCH 0/2] pseries/hotplug: Change the default behaviour of cede_offline

[Nathan Lynch]

"Gautham R. Shenoy"  writes:
> The patchset also defines a new sysfs attribute
> "/sys/device/system/cpu/cede_offline_enabled" on PSeries Linux guests
> to allow userspace programs to change the state into which the
> offlined CPU need to be put to at runtime.

A boolean sysfs interface will become awkward if we need to add another
mode in the future.

What do you think about naming the attribute something like
'offline_mode', with the possible values 'extended-cede' and
'rtas-stopped'?

Re: [PATCH 2/2] powerpc/pseries: Delete an error message for a failed string duplication in dlpar_store()

[Nathan Lynch]

Markus Elfring  writes:

> From: Markus Elfring 
> Date: Tue, 27 Aug 2019 13:37:56 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> Signed-off-by: Markus Elfring 

Acked-by: Nathan Lynch 

Re: [PATCH 1/2] powerpc/pseries: Delete an unnecessary kfree() call in dlpar_store()

[Nathan Lynch]

Markus Elfring  writes:
> From: Markus Elfring 
> Date: Tue, 27 Aug 2019 13:34:02 +0200
>
> A null pointer would be passed to a call of the function “kfree”
> immediately after a call of the function “kstrdup” failed at one place.
> Remove this superfluous function call.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 

Acked-by: Nathan Lynch 

Re: [PATCH] powerpc/vdso32: Add support for CLOCK_{REALTIME/MONOTONIC}_COARSE

[Nathan Lynch]

Christophe Leroy  writes:

> Hi,
>
> Le 19/08/2019 à 18:37, Nathan Lynch a écrit :
>> Hi,
>> 
>> Christophe Leroy  writes:
>>> Benchmark from vdsotest:
>> 
>> I assume you also ran the verification/correctness parts of vdsotest...? :-)
>> 
>
> I did run vdsotest-all. I guess it runs the verifications too ?

It does, but at a quick glance it runs the validation for "only" 1
second per API. It may provide more confidence to allow the validation
to run across several second (tv_sec) transitions, e.g.

vdsotest -d 30 clock-gettime-monotonic-coarse verify

Regardless, I did not see any problem with your patch.

Re: [PATCH] powerpc/vdso32: Add support for CLOCK_{REALTIME/MONOTONIC}_COARSE

[Nathan Lynch]

Hi,

Christophe Leroy  writes:
> Benchmark from vdsotest:

I assume you also ran the verification/correctness parts of vdsotest...? :-)

Re: [PATCH v2 03/35] powerpc: Use kmemdup rather than duplicating its implementation

[Nathan Lynch]

Fuqian Huang  writes:
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memcpy, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memcpy.
>
> Signed-off-by: Fuqian Huang 
> ---
> Changes in v2:
>   - Fix a typo in commit message (memset -> memcpy)

Thanks, but this and the unchecked kmalloc result (and incorrect gfp
flags) have already been addressed in commit
348ea30f51fc63ce3c7fd7dba6043e8e3ee0ef34 ("powerpc/pseries: avoid
blocking in irq when queuing hotplug events"):

https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?h=next=348ea30f51fc63ce3c7fd7dba6043e8e3ee0ef34

Re: [PATCH] dlpar: Fix a missing-check bug in dlpar_parse_cc_property()

[Nathan Lynch]

Gen Zhang  writes:
> In dlpar_parse_cc_property(), 'prop->name' is allocated by kstrdup().
> kstrdup() may return NULL, so it should be checked and handle error.
> And prop should be freed if 'prop->name' is NULL.
>
> Signed-off-by: Gen Zhang 
> ---
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
> b/arch/powerpc/platforms/pseries/dlpar.c
> index 1795804..c852024 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -61,6 +61,10 @@ static struct property *dlpar_parse_cc_property(struct 
> cc_workarea *ccwa)
>  
>   name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
>   prop->name = kstrdup(name, GFP_KERNEL);
> + if (!prop->name) {
> + dlpar_free_cc_property(prop);
> + return NULL;
> + }

Acked-by: Nathan Lynch 

Re: [PATCH] ARM: VDSO: Drop implicit common-page-size linker flag

[Nathan Lynch]

Arnd Bergmann  writes:
> On Mon, Dec 10, 2018 at 11:32 PM  wrote:
>>
>> GNU linker's -z common-page-size's default value is based on the target
>> architecture. arch/arm/vdso/Makefile sets it to the architecture
>> default, which is implicit and redundant. Drop it.
>>
>> Link: 
>> https://lkml.kernel.org/r/20181206191231.192355-1-ndesaulni...@google.com
>> Signed-off-by: Nick Desaulniers 
>> ---
>
> The patch looks good to me,
>
> Acked-by: Arnd Bergmann 
>
> Adding Nathan Lynch to Cc though for further comments, he originally
> added the Makefile flags here and might still remember why he did it.
>
> Unless Nathan objects, please add the patch to Russell's patch
> tracker.

Thanks for cc'ing me -- no objection.

Acked-by: Nathan Lynch 

Re: [PATCH v2] ARM: vdso: Define vdso_{start,end} as array

[Nathan Lynch]

Arnd Bergmann <a...@arndb.de> writes:

> gcc-8 correctly points out that reading four bytes from a pointer to a
> 'char' variable is wrong
>
> arch/arm/kernel/vdso.c: In function 'vdso_init':
> arch/arm/kernel/vdso.c:200:6: error: '__builtin_memcmp_eq' reading 4 bytes 
> from a region of size 1 [-Werror=stringop-overflow=]
>
> However, in this case the variable just stands for the beginning of the
> vdso and is not actually a 'char', so the code is doing what it is meant
> to do.
>
> This uses the same approach as arm64 and x86, declaring the addresses
> as char arrays.
>
> See also: dbbb08f500d6 ("arm64, vdso: Define vdso_{start,end} as array")
>
> Suggested-by: Mark Rutland <mark.rutl...@arm.com>
> Suggested-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Acked-by: Nathan Lynch <nathan_ly...@mentor.com>

Thanks!

Re: [PATCH v2] ARM: vdso: Define vdso_{start,end} as array

[Nathan Lynch]

Arnd Bergmann  writes:

> gcc-8 correctly points out that reading four bytes from a pointer to a
> 'char' variable is wrong
>
> arch/arm/kernel/vdso.c: In function 'vdso_init':
> arch/arm/kernel/vdso.c:200:6: error: '__builtin_memcmp_eq' reading 4 bytes 
> from a region of size 1 [-Werror=stringop-overflow=]
>
> However, in this case the variable just stands for the beginning of the
> vdso and is not actually a 'char', so the code is doing what it is meant
> to do.
>
> This uses the same approach as arm64 and x86, declaring the addresses
> as char arrays.
>
> See also: dbbb08f500d6 ("arm64, vdso: Define vdso_{start,end} as array")
>
> Suggested-by: Mark Rutland 
> Suggested-by: Ard Biesheuvel 
> Signed-off-by: Arnd Bergmann 

Acked-by: Nathan Lynch 

Thanks!

Re: [PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.

[Nathan Lynch]

Will Deacon  writes:
> On Tue, May 30, 2017 at 05:34:19PM -0700, Andrew Pinski wrote:
>> Note I noticed a bug in the old implementation of __kernel_clock_getres;
>> it was checking only the lower 32bits of the pointer; this would work
>> for most cases but could fail in a few.
>> 
>> Changes from v1:
>> * Fixed bug in __kernel_clock_getres for checking the pointer argument.
>> * Fix comments to refer to functions in arm64.

FWIW: despite asking around, I've never been able to determine the
original rationale for putting clock_getres in a vdso.  It seems like it
originated in powerpc and crept into other implementations.  I think
clock_getres should be dropped from new vdso implementations unless its
inclusion can be justified.


> I tested this patch on a few platforms I have access to and didn't see the
> perf regressions I saw when I looked at this in the past with an older
> toolchain (it was mostly about the same, with a couple of improvements).
>
> So, in principle, I'm not opposed to moving this into C. However, we're
> currently close to a "vDSO-explosion" on arm64 with people wanting a compat
> variant and also an ILP32 variant. When Kevin posted his compat variant
> (also in C):
>
>   http://lkml.kernel.org/r/20161206160353.14581-1-kevin.brod...@arm.com
>
> Nathan (who apparently needs to set his mail host address ;) was concerned
> about duplication between arm and arm64:
>
>   
> http://lkml.kernel.org/r/87r35jmv3e.fsf@wedge.i-did-not-set--mail-host-address--so-tickle-me
>
> I'm firmly of the opinion that we should try to write an arch-agnostic vDSO
> implementation in core code (lib/vdso or something) where the arch header
> provides things like:
>
>   * The mechanism to read the counter
>   * The mechanism to issue a syscall
>   * A function to determine whether or not the current clocksource is
> suitable
>
> I think the datapage format could be defined in core code and it would be
> worth looking to see how much the virtual mapping code can be consolidated
> too.
>
> If we can get something that works for arm native, arm64 native, arm64
> compat and arm64 ilp32 then it's probably going to be useful for other
> architectures too, even if we need to add more customisation points in
> future.
>
> I've spoken to Kevin about this, but I'm not sure whether he's had a chance
> to look at knocking up a prototype. A first stab could just unconditionally
> fallback to the system call.

I was thinking something like CONFIG_GENERIC_VDSO that arches can opt
into over time makes sense.  But the generic code needs to be amenable
to being "sourced" by the arch code for multiple ABIs (compat, ILP32) in
a single build.

There are some additional (likely somewhat dated) considerations here,
from one of the arm vdso discusssions:

https://marc.info/?l=linux-arm-kernel=140972320130624=2

Re: [PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.

[Nathan Lynch]

Will Deacon  writes:
> On Tue, May 30, 2017 at 05:34:19PM -0700, Andrew Pinski wrote:
>> Note I noticed a bug in the old implementation of __kernel_clock_getres;
>> it was checking only the lower 32bits of the pointer; this would work
>> for most cases but could fail in a few.
>> 
>> Changes from v1:
>> * Fixed bug in __kernel_clock_getres for checking the pointer argument.
>> * Fix comments to refer to functions in arm64.

FWIW: despite asking around, I've never been able to determine the
original rationale for putting clock_getres in a vdso.  It seems like it
originated in powerpc and crept into other implementations.  I think
clock_getres should be dropped from new vdso implementations unless its
inclusion can be justified.


> I tested this patch on a few platforms I have access to and didn't see the
> perf regressions I saw when I looked at this in the past with an older
> toolchain (it was mostly about the same, with a couple of improvements).
>
> So, in principle, I'm not opposed to moving this into C. However, we're
> currently close to a "vDSO-explosion" on arm64 with people wanting a compat
> variant and also an ILP32 variant. When Kevin posted his compat variant
> (also in C):
>
>   http://lkml.kernel.org/r/20161206160353.14581-1-kevin.brod...@arm.com
>
> Nathan (who apparently needs to set his mail host address ;) was concerned
> about duplication between arm and arm64:
>
>   
> http://lkml.kernel.org/r/87r35jmv3e.fsf@wedge.i-did-not-set--mail-host-address--so-tickle-me
>
> I'm firmly of the opinion that we should try to write an arch-agnostic vDSO
> implementation in core code (lib/vdso or something) where the arch header
> provides things like:
>
>   * The mechanism to read the counter
>   * The mechanism to issue a syscall
>   * A function to determine whether or not the current clocksource is
> suitable
>
> I think the datapage format could be defined in core code and it would be
> worth looking to see how much the virtual mapping code can be consolidated
> too.
>
> If we can get something that works for arm native, arm64 native, arm64
> compat and arm64 ilp32 then it's probably going to be useful for other
> architectures too, even if we need to add more customisation points in
> future.
>
> I've spoken to Kevin about this, but I'm not sure whether he's had a chance
> to look at knocking up a prototype. A first stab could just unconditionally
> fallback to the system call.

I was thinking something like CONFIG_GENERIC_VDSO that arches can opt
into over time makes sense.  But the generic code needs to be amenable
to being "sourced" by the arch code for multiple ABIs (compat, ILP32) in
a single build.

There are some additional (likely somewhat dated) considerations here,
from one of the arm vdso discusssions:

https://marc.info/?l=linux-arm-kernel=140972320130624=2

Re: [PATCH v3] ARM: VDSO: put RO and RO after init objects into proper sections

[Nathan Lynch]

On 08/12/2016 01:06 PM, Kees Cook wrote:
> On Fri, Aug 12, 2016 at 1:12 AM, Jisheng Zhang <jszh...@marvell.com> wrote:
>> vdso_data_mapping is never modified, so mark it as const.
>>
>> vdso_total_pages, vdso_data_page, vdso_text_mapping and cntvct_ok are
>> initialized by vdso_init(), thereafter are read only.
>>
>> The fact that they are read only after init makes them candidates for
>> __ro_after_init declarations.
>>
>> Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
> 
> Looks great; thanks for keeping __ro_after_init in mind. :)
> 
> Reviewed-by: Kees Cook <keesc...@chromium.org>

Looks fine and nothing goes wrong with my vdso tests.  Please send it
through Russell.

Acked-by: Nathan Lynch <nathan_ly...@mentor.com>

Re: [PATCH v3] ARM: VDSO: put RO and RO after init objects into proper sections

[Nathan Lynch]

On 08/12/2016 01:06 PM, Kees Cook wrote:
> On Fri, Aug 12, 2016 at 1:12 AM, Jisheng Zhang  wrote:
>> vdso_data_mapping is never modified, so mark it as const.
>>
>> vdso_total_pages, vdso_data_page, vdso_text_mapping and cntvct_ok are
>> initialized by vdso_init(), thereafter are read only.
>>
>> The fact that they are read only after init makes them candidates for
>> __ro_after_init declarations.
>>
>> Signed-off-by: Jisheng Zhang 
> 
> Looks great; thanks for keeping __ro_after_init in mind. :)
> 
> Reviewed-by: Kees Cook 

Looks fine and nothing goes wrong with my vdso tests.  Please send it
through Russell.

Acked-by: Nathan Lynch 

Re: [PATCH] ARM: VDSO: put read only/mostly objects into proper sections

[Nathan Lynch]

On 08/10/2016 01:47 PM, Kees Cook wrote:
> On Wed, Aug 10, 2016 at 2:59 AM, Jisheng Zhang  wrote:
>> + Kees
>>
>> On Wed, 10 Aug 2016 17:09:47 +0800 wrote:
>>
>>> vdso_data_mapping is never modified, so mark it as const.
>>>
>>> vdso_data_page and vdso_text_mapping are initialized by vdso_init(),
>>> thereafter are mostly read during vdso special mapping handling.
>>>
>>> The fact that they are mostly read and not written to makes them
>>> candidates for __read_mostly declarations.
>>
>> Inspired by Kees's "arm: apply more __ro_after_init", is it better
>> to mark these vars as __ro_after_init?
> 
> Yeah, if they're not written outside of __init, please do. It would be
> a nice complement to commit 11bf9b865898 ("ARM/vdso: Mark the vDSO
> code read-only after init").

I agree.

Re: [PATCH] ARM: VDSO: put read only/mostly objects into proper sections

[Nathan Lynch]

On 08/10/2016 01:47 PM, Kees Cook wrote:
> On Wed, Aug 10, 2016 at 2:59 AM, Jisheng Zhang  wrote:
>> + Kees
>>
>> On Wed, 10 Aug 2016 17:09:47 +0800 wrote:
>>
>>> vdso_data_mapping is never modified, so mark it as const.
>>>
>>> vdso_data_page and vdso_text_mapping are initialized by vdso_init(),
>>> thereafter are mostly read during vdso special mapping handling.
>>>
>>> The fact that they are mostly read and not written to makes them
>>> candidates for __read_mostly declarations.
>>
>> Inspired by Kees's "arm: apply more __ro_after_init", is it better
>> to mark these vars as __ro_after_init?
> 
> Yeah, if they're not written outside of __init, please do. It would be
> a nice complement to commit 11bf9b865898 ("ARM/vdso: Mark the vDSO
> code read-only after init").

I agree.

Re: [PATCH v4] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/16/2015 01:38 AM, H. Nikolaus Schaller wrote:
> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c
> index aedec81..0cebd98 100644
> --- a/arch/arm/vdso/vdsomunge.c
> +++ b/arch/arm/vdso/vdsomunge.c
> @@ -45,7 +45,6 @@
>   * it does.
>   */
>  
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +58,16 @@
>  #include 
>  #include 
>  
> +#define swab16(x) \
> + x) & 0x00ff) << 8) | \
> +  (((x) & 0xff00) >> 8))
> +
> +#define swab32(x) \
> + x) & 0x00ff) << 24) | \
> +  (((x) & 0xff00) <<  8) | \
> +  (((x) & 0x00ff) >>  8) | \
> +  (((x) & 0xff00) << 24))
> +
>  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>  #define HOST_ORDER ELFDATA2LSB
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> @@ -104,17 +113,17 @@ static void cleanup(void)
>  
>  static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
>  {
> - return swap ? bswap_32(word) : word;
> + return swap ? swab32(word) : word;
>  }
>  
>  static Elf32_Half read_elf_half(Elf32_Half half, bool swap)
>  {
> - return swap ? bswap_16(half) : half;
> + return swap ? swab16(half) : half;
>  }
>  
>  static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
>  {
> - *dst = swap ? bswap_32(val) : val;
> + *dst = swap ? swab32(val) : val;
>  }

I think this looks good now and it checks out in my local testing.
Thanks for your persistence.

Assuming Russell has no further comments I'll put this in his patch
system later today.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/16/2015 01:38 AM, H. Nikolaus Schaller wrote:
> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c
> index aedec81..0cebd98 100644
> --- a/arch/arm/vdso/vdsomunge.c
> +++ b/arch/arm/vdso/vdsomunge.c
> @@ -45,7 +45,6 @@
>   * it does.
>   */
>  
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +58,16 @@
>  #include 
>  #include 
>  
> +#define swab16(x) \
> + x) & 0x00ff) << 8) | \
> +  (((x) & 0xff00) >> 8))
> +
> +#define swab32(x) \
> + x) & 0x00ff) << 24) | \
> +  (((x) & 0xff00) <<  8) | \
> +  (((x) & 0x00ff) >>  8) | \
> +  (((x) & 0xff00) << 24))
> +
>  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>  #define HOST_ORDER ELFDATA2LSB
>  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> @@ -104,17 +113,17 @@ static void cleanup(void)
>  
>  static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
>  {
> - return swap ? bswap_32(word) : word;
> + return swap ? swab32(word) : word;
>  }
>  
>  static Elf32_Half read_elf_half(Elf32_Half half, bool swap)
>  {
> - return swap ? bswap_16(half) : half;
> + return swap ? swab16(half) : half;
>  }
>  
>  static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
>  {
> - *dst = swap ? bswap_32(val) : val;
> + *dst = swap ? swab32(val) : val;
>  }

I think this looks good now and it checks out in my local testing.
Thanks for your persistence.

Assuming Russell has no further comments I'll put this in his patch
system later today.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/15/2015 12:16 PM, H. Nikolaus Schaller wrote:
> Does this mean it is ok now in V3 in all cases? I have switched my patch 
> editor
> tool from indirect git imap-send to git send-email (which appears to be more
> compatible).

v3 applied without issue, so yes, in that respect it is fine.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/15/2015 12:52 AM, H. Nikolaus Schaller wrote:
> Am 14.10.2015 um 16:16 schrieb Nathan Lynch :
>> and it would be easier for me to deal with a patch that isn't
>> whitespace-damaged.
> 
> You mean:
> 
> ERROR: space prohibited before that close parenthesis ')'
> #46: FILE: arch/arm/vdso/vdsomunge.c:64:
> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) ))
> 
> ERROR: space prohibited before that close parenthesis ')'
> #53: FILE: arch/arm/vdso/vdsomunge.c:71:
> + (((unsigned int)(x) & (unsigned int)0xff00UL) >> 24) ))
> 
> Well, I had copied that verbatim from arch/mips/boot/elf2ecoff.c and
> thought that it is better readable, but it is easy to fix.

That's not what I was referring to, but it is fine to fix that too.

What I meant was that the first column of the patch body is corrupted.
Your v2 has hunks like this (bad):

static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
{
-   return swap ? bswap_32(word) : word;
+   return swap ? swab32(word) : word;
}


Your v3 has this (good):

 static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
 {
-   *dst = swap ? bswap_32(val) : val;
+   *dst = swap ? swab32(val) : val;
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/15/2015 12:52 AM, H. Nikolaus Schaller wrote:
> Am 14.10.2015 um 16:16 schrieb Nathan Lynch <nathan_ly...@mentor.com>:
>> and it would be easier for me to deal with a patch that isn't
>> whitespace-damaged.
> 
> You mean:
> 
> ERROR: space prohibited before that close parenthesis ')'
> #46: FILE: arch/arm/vdso/vdsomunge.c:64:
> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) ))
> 
> ERROR: space prohibited before that close parenthesis ')'
> #53: FILE: arch/arm/vdso/vdsomunge.c:71:
> + (((unsigned int)(x) & (unsigned int)0xff00UL) >> 24) ))
> 
> Well, I had copied that verbatim from arch/mips/boot/elf2ecoff.c and
> thought that it is better readable, but it is easy to fix.

That's not what I was referring to, but it is fine to fix that too.

What I meant was that the first column of the patch body is corrupted.
Your v2 has hunks like this (bad):

static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
{
-   return swap ? bswap_32(word) : word;
+   return swap ? swab32(word) : word;
}


Your v3 has this (good):

 static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
 {
-   *dst = swap ? bswap_32(val) : val;
+   *dst = swap ? swab32(val) : val;
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/15/2015 12:16 PM, H. Nikolaus Schaller wrote:
> Does this mean it is ok now in V3 in all cases? I have switched my patch 
> editor
> tool from indirect git imap-send to git send-email (which appears to be more
> compatible).

v3 applied without issue, so yes, in that respect it is fine.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/14/2015 07:47 AM, H. Nikolaus Schaller wrote:
>> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c
>> index aedec81..27a9a0b 100644
>> --- a/arch/arm/vdso/vdsomunge.c
>> +++ b/arch/arm/vdso/vdsomunge.c
>> @@ -45,7 +45,18 @@
>> * it does.
>> */
>>
>> -#include 
>> +#define swab16(x) \
>> +((unsigned short)( \
>> +(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \
>> +(((unsigned short)(x) & (unsigned short)0xff00U) >> 8) ))
>> +
>> +#define swab32(x) \
>> +((unsigned int)( \
>> +(((unsigned int)(x) & (unsigned int)0x00ffUL) << 24) | \
>> +(((unsigned int)(x) & (unsigned int)0xff00UL) <<  8) | \
>> +(((unsigned int)(x) & (unsigned int)0x00ffUL) >>  8) | \
>> +(((unsigned int)(x) & (unsigned int)0xff00UL) >> 24) ))
>> +
>> #include 
>> #include 
>> #include 
>> @@ -104,17 +115,17 @@ static void cleanup(void)
>>
>> static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
>> {
>> -return swap ? bswap_32(word) : word;
>> +return swap ? swab32(word) : word;
>> }
>>
>> static Elf32_Half read_elf_half(Elf32_Half half, bool swap)
>> {
>> -return swap ? bswap_16(half) : half;
>> +return swap ? swab16(half) : half;
>> }
>>
>> static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
>> {
>> -*dst = swap ? bswap_32(val) : val;
>> +*dst = swap ? swab32(val) : val;
>> }
> ping.

Sorry for the delay.

This is okay but I'd prefer the swab macros to be below the #include
lines, and it would be easier for me to deal with a patch that isn't
whitespace-damaged.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] ARM: fix vdsomunge not to depend on glibc specific byteswap.h

[Nathan Lynch]

On 10/14/2015 07:47 AM, H. Nikolaus Schaller wrote:
>> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c
>> index aedec81..27a9a0b 100644
>> --- a/arch/arm/vdso/vdsomunge.c
>> +++ b/arch/arm/vdso/vdsomunge.c
>> @@ -45,7 +45,18 @@
>> * it does.
>> */
>>
>> -#include 
>> +#define swab16(x) \
>> +((unsigned short)( \
>> +(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \
>> +(((unsigned short)(x) & (unsigned short)0xff00U) >> 8) ))
>> +
>> +#define swab32(x) \
>> +((unsigned int)( \
>> +(((unsigned int)(x) & (unsigned int)0x00ffUL) << 24) | \
>> +(((unsigned int)(x) & (unsigned int)0xff00UL) <<  8) | \
>> +(((unsigned int)(x) & (unsigned int)0x00ffUL) >>  8) | \
>> +(((unsigned int)(x) & (unsigned int)0xff00UL) >> 24) ))
>> +
>> #include 
>> #include 
>> #include 
>> @@ -104,17 +115,17 @@ static void cleanup(void)
>>
>> static Elf32_Word read_elf_word(Elf32_Word word, bool swap)
>> {
>> -return swap ? bswap_32(word) : word;
>> +return swap ? swab32(word) : word;
>> }
>>
>> static Elf32_Half read_elf_half(Elf32_Half half, bool swap)
>> {
>> -return swap ? bswap_16(half) : half;
>> +return swap ? swab16(half) : half;
>> }
>>
>> static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap)
>> {
>> -*dst = swap ? bswap_32(val) : val;
>> +*dst = swap ? swab32(val) : val;
>> }
> ping.

Sorry for the delay.

This is okay but I'd prefer the swab macros to be below the #include
lines, and it would be easier for me to deal with a patch that isn't
whitespace-damaged.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: fix vdsomunge depends on glibc specific byteswap.h

[Nathan Lynch]

On 09/30/2015 12:47 PM, H. Nikolaus Schaller wrote:
> 
> Am 30.09.2015 um 19:37 schrieb Nathan Lynch :
>>  Why not just use a generic
>> implementation like is found in mips' elf2ecoff?
> 
> Do you have a reference?
> I can't find byte swapping in
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/elf2ecoff.c?id=refs/tags/v4.3-rc3

See the swab16 and swab32 macros (yes, "swab" not "swap").

Or the __constant_swab* macros in include/linux/uapi/swab.h.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: fix vdsomunge depends on glibc specific byteswap.h

[Nathan Lynch]

On 09/30/2015 11:17 AM, Ard Biesheuvel wrote:
> On 30 September 2015 at 18:13, H. Nikolaus Schaller  
> wrote:
>>
>> Am 30.09.2015 um 18:02 schrieb Ard Biesheuvel :
>>>
>>> Have you tried this?
>>>
>>> #define bswap_16 __builtin_bswap16
>>> #define bswap_32 __builtin_bswap32
>>> #define bswap_64 __builtin_bswap64
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
>>
>> OS X host uses llvm and I am not sure if these builtins are
>> always available.
>>
> 
> I am pretty sure recent clang supports these as well. Could you please try it?

Well, I think GCC did not provide __builtin_bswap16 consistently until
the 4.8 release:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52624

That seems too recent to me.

vdsomunge makes only three or four potentially byteswapped accesses to
the ELF header.  It's not worth a lot of effort to try to use the most
optimal implementation available.  Why not just use a generic
implementation like is found in mips' elf2ecoff?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: fix vdsomunge depends on glibc specific byteswap.h

[Nathan Lynch]

On 09/30/2015 12:47 PM, H. Nikolaus Schaller wrote:
> 
> Am 30.09.2015 um 19:37 schrieb Nathan Lynch <nathan_ly...@mentor.com>:
>>  Why not just use a generic
>> implementation like is found in mips' elf2ecoff?
> 
> Do you have a reference?
> I can't find byte swapping in
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/elf2ecoff.c?id=refs/tags/v4.3-rc3

See the swab16 and swab32 macros (yes, "swab" not "swap").

Or the __constant_swab* macros in include/linux/uapi/swab.h.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: fix vdsomunge depends on glibc specific byteswap.h

[Nathan Lynch]

On 09/30/2015 11:17 AM, Ard Biesheuvel wrote:
> On 30 September 2015 at 18:13, H. Nikolaus Schaller  
> wrote:
>>
>> Am 30.09.2015 um 18:02 schrieb Ard Biesheuvel :
>>>
>>> Have you tried this?
>>>
>>> #define bswap_16 __builtin_bswap16
>>> #define bswap_32 __builtin_bswap32
>>> #define bswap_64 __builtin_bswap64
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
>>
>> OS X host uses llvm and I am not sure if these builtins are
>> always available.
>>
> 
> I am pretty sure recent clang supports these as well. Could you please try it?

Well, I think GCC did not provide __builtin_bswap16 consistently until
the 4.8 release:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52624

That seems too recent to me.

vdsomunge makes only three or four potentially byteswapped accesses to
the ELF header.  It's not worth a lot of effort to try to use the most
optimal implementation available.  Why not just use a generic
implementation like is found in mips' elf2ecoff?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 17/23] arm64:ilp32: add vdso-ilp32 and use for signal return

[Nathan Lynch]

On 09/29/2015 05:14 PM, Yury Norov wrote:
> From: Philipp Tomsich 
> 
> Adjusted to move the move data page before code pages in sync with
> commit 601255ae3c98fd3a8bb4696425e4f868b4f1

This commit message needs more information about how the ilp32 VDSO uses
the existing arm64 code.  I had to really hunt through the Makefile to
figure out what's going on.

The commit message should also identify the APIs that are supported.
The subject line mentions signal return, but gettimeofday, clock_gettime
and clock_getres are being added here too, and it is not obvious.


> Signed-off-by: Philipp Tomsich 
> Signed-off-by: Christoph Muellner 
> Signed-off-by: Yury Norov 
> 
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/.gitignore
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/Makefile
>  copy arch/arm64/{include/asm/vdso.h => kernel/vdso-ilp32/vdso-ilp32.S} (56%)
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S

How are you invoking git-format-patch?  The copy detection in this case
is not conducive to review.

It looks like the existing arm64 vdso Makefile has been copied to
vdso-ilp32/ and adjusted for paths and naming.  While the gettimeofday
assembly implementation is reused, the build logic is duplicated.  x86
produces VDSOs for multiple ABIs with a single Makefile; is a similar
approach not appropriate for arm64?


> diff --git a/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S 
> b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
> new file mode 100644
> index 000..ac8029b
> --- /dev/null
> +++ b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
> @@ -0,0 +1,98 @@

[...]

> +#include 
> +#include 
> +#include 
> +
> +/*OUTPUT_FORMAT("elf32-littleaarch64", "elf32-bigaarch64", 
> "elf32-littleaarch64")
> +OUTPUT_ARCH(aarch64)
> +*/

If these lines aren't needed then omit them.

[...]


> +/*
> + * This controls what symbols we export from the DSO.
> + */
> +VERSION
> +{
> + LINUX_2.6.39 {
> + global:
> + __kernel_rt_sigreturn;
> + __kernel_gettimeofday;
> + __kernel_clock_gettime;
> + __kernel_clock_getres;
> + local: *;
> + };
> +}

Something that came up during review of arch/arm's VDSO code: consider
using version and names that match x86, i.e. LINUX_2.6, __vdso_gettimeofday.

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267940.html

Using LINUX_2.6.39 for this code is nonsensical.


> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index b239b9b..bed6cf1 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -40,6 +40,12 @@ extern char vdso_start, vdso_end;
>  static unsigned long vdso_pages;
>  static struct page **vdso_pagelist;
>  
> +#ifdef CONFIG_ARM64_ILP32
> +extern char vdso_ilp32_start, vdso_ilp32_end;
> +static unsigned long vdso_ilp32_pages;
> +static struct page **vdso_ilp32_pagelist;
> +#endif
> +
>  /*
>   * The vDSO data page.
>   */
> @@ -117,24 +123,29 @@ int aarch32_setup_vectors_page(struct linux_binprm 
> *bprm, int uses_interp)
>  }
>  #endif /* CONFIG_AARCH32_EL0 */
>  
> -static struct vm_special_mapping vdso_spec[2];
> -
> -static int __init vdso_init(void)
> +static inline int __init vdso_init_common(char *vdso_start, char *vdso_end,

No inline please.


> +   unsigned long *vdso_pagesp,
> +   struct page ***vdso_pagelistp,
> +   struct vm_special_mapping* vdso_spec)
>  {

[...]

>  int arch_setup_additional_pages(struct linux_binprm *bprm,
>   int uses_interp)
>  {
>   struct mm_struct *mm = current->mm;
>   unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
> - void *ret;
> + void* ret;

Gratuitous (and incorrect) style change.


> + unsigned long pages = vdso_pages;
> + struct vm_special_mapping* spec = vdso_spec;

Incorrect style:  *spec

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 17/23] arm64:ilp32: add vdso-ilp32 and use for signal return

[Nathan Lynch]

On 09/29/2015 05:14 PM, Yury Norov wrote:
> From: Philipp Tomsich 
> 
> Adjusted to move the move data page before code pages in sync with
> commit 601255ae3c98fd3a8bb4696425e4f868b4f1

This commit message needs more information about how the ilp32 VDSO uses
the existing arm64 code.  I had to really hunt through the Makefile to
figure out what's going on.

The commit message should also identify the APIs that are supported.
The subject line mentions signal return, but gettimeofday, clock_gettime
and clock_getres are being added here too, and it is not obvious.


> Signed-off-by: Philipp Tomsich 
> Signed-off-by: Christoph Muellner 
> Signed-off-by: Yury Norov 
> 
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/.gitignore
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/Makefile
>  copy arch/arm64/{include/asm/vdso.h => kernel/vdso-ilp32/vdso-ilp32.S} (56%)
>  create mode 100644 arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S

How are you invoking git-format-patch?  The copy detection in this case
is not conducive to review.

It looks like the existing arm64 vdso Makefile has been copied to
vdso-ilp32/ and adjusted for paths and naming.  While the gettimeofday
assembly implementation is reused, the build logic is duplicated.  x86
produces VDSOs for multiple ABIs with a single Makefile; is a similar
approach not appropriate for arm64?


> diff --git a/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S 
> b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
> new file mode 100644
> index 000..ac8029b
> --- /dev/null
> +++ b/arch/arm64/kernel/vdso-ilp32/vdso-ilp32.lds.S
> @@ -0,0 +1,98 @@

[...]

> +#include 
> +#include 
> +#include 
> +
> +/*OUTPUT_FORMAT("elf32-littleaarch64", "elf32-bigaarch64", 
> "elf32-littleaarch64")
> +OUTPUT_ARCH(aarch64)
> +*/

If these lines aren't needed then omit them.

[...]


> +/*
> + * This controls what symbols we export from the DSO.
> + */
> +VERSION
> +{
> + LINUX_2.6.39 {
> + global:
> + __kernel_rt_sigreturn;
> + __kernel_gettimeofday;
> + __kernel_clock_gettime;
> + __kernel_clock_getres;
> + local: *;
> + };
> +}

Something that came up during review of arch/arm's VDSO code: consider
using version and names that match x86, i.e. LINUX_2.6, __vdso_gettimeofday.

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267940.html

Using LINUX_2.6.39 for this code is nonsensical.


> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index b239b9b..bed6cf1 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -40,6 +40,12 @@ extern char vdso_start, vdso_end;
>  static unsigned long vdso_pages;
>  static struct page **vdso_pagelist;
>  
> +#ifdef CONFIG_ARM64_ILP32
> +extern char vdso_ilp32_start, vdso_ilp32_end;
> +static unsigned long vdso_ilp32_pages;
> +static struct page **vdso_ilp32_pagelist;
> +#endif
> +
>  /*
>   * The vDSO data page.
>   */
> @@ -117,24 +123,29 @@ int aarch32_setup_vectors_page(struct linux_binprm 
> *bprm, int uses_interp)
>  }
>  #endif /* CONFIG_AARCH32_EL0 */
>  
> -static struct vm_special_mapping vdso_spec[2];
> -
> -static int __init vdso_init(void)
> +static inline int __init vdso_init_common(char *vdso_start, char *vdso_end,

No inline please.


> +   unsigned long *vdso_pagesp,
> +   struct page ***vdso_pagelistp,
> +   struct vm_special_mapping* vdso_spec)
>  {

[...]

>  int arch_setup_additional_pages(struct linux_binprm *bprm,
>   int uses_interp)
>  {
>   struct mm_struct *mm = current->mm;
>   unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
> - void *ret;
> + void* ret;

Gratuitous (and incorrect) style change.


> + unsigned long pages = vdso_pages;
> + struct vm_special_mapping* spec = vdso_spec;

Incorrect style:  *spec

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 28c711f..72e97d7 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
> It's safe to say n here if you're not interested in multimedia
> offloading.
>  
> +config ST_REMOTEPROC
> + tristate "ST remoteproc support"
> + depends on ARCH_STI
> + select REMOTEPROC
> + help
> +   Say y here to support ST's adjunct processors via the remote
> +   processor framework.
> +   This can be either built-in or a loadable module.
> +

The code uses reset_control_* APIs, so this should depend on
RESET_CONTROLLER, no?

> +/*
> + * ST's Remote Processor Control Driver
> + *
> + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> + *
> + * Author: Ludovic Barre 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + */

OK, but:

> +MODULE_LICENSE("GPL v2");

These are not in agreement.  You want "GPL" for MODULE_LICENSE if you
intend v2 or later.


> +static int st_rproc_stop(struct rproc *rproc)
> +{
> + struct st_rproc *st_rproc = rproc->priv;
> + int ret, err = 0;
> +
> + if (st_rproc->config->sw_reset) {
> + ret = reset_control_assert(st_rproc->sw_reset);
> + if (ret)
> + dev_err(>dev, "Failed to assert S/W Reset\n");
> + }
> +
> + if (st_rproc->config->pwr_reset) {
> + err = reset_control_assert(st_rproc->pwr_reset);
> + if (err)
> + dev_err(>dev, "Failed to assert Power Reset\n");
> + }
> +
> + clk_disable(st_rproc->clk);
> +
> + return ret ?: err;
> +}

Sorry, but I think this is a stylistically inadequate response to my
earlier comments.  At least name the status variables sw_ret and pwr_ret
or something.  And it looks like ret could be used uninitialized.

Also, do you want to unconditionally call clk_disable even if you've
encountered errors?


> +static int st_rproc_start(struct rproc *rproc)
> +{
> + struct st_rproc *st_rproc = rproc->priv;
> + int err;
> +
> + regmap_update_bits(st_rproc->boot_base, st_rproc->boot_offset,
> +st_rproc->config->bootaddr_mask, rproc->bootaddr);
> +
> + err = clk_enable(st_rproc->clk);
> + if (err) {
> + dev_err(>dev, "Failed to enable clock\n");
> + return err;
> + }
> +
> + if (st_rproc->config->sw_reset) {
> + err = reset_control_deassert(st_rproc->sw_reset);
> + if (err) {
> + dev_err(>dev, "Failed to deassert S/W Reset\n");
> + return err;
> + }
> + }
> +
> + if (st_rproc->config->pwr_reset) {
> + err = reset_control_deassert(st_rproc->pwr_reset);
> + if (err) {
> + dev_err(>dev, "Failed to deassert Power 
> Reset\n");
> + return err;
> + }
> + }
> +
> + dev_info(>dev, "Started from 0x%x\n", rproc->bootaddr);
> +
> + return 0;
> +}

Does this want to unwind any of its operations if it encounters a failure?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
> +static ssize_t rproc_state_write(struct file *filp, const char __user 
> *userbuf,
> +  size_t count, loff_t *ppos)
> +{
> + struct rproc *rproc = filp->private_data;
> + char buf[2];
> + int ret;
> +
> + ret = copy_from_user(buf, userbuf, 1);
> + if (ret)
> + return -EFAULT;
> +
> + switch (buf[0]) {
> + case '0':
> + rproc_shutdown(rproc);
> + break;
> + case '1':
> + ret = rproc_boot(rproc);
> + if (ret)
> + dev_warn(>dev, "Boot failed: %d\n", ret);
> + break;
> + default:
> + dev_err(>dev, "Unrecognised option: %x\n", buf[1]);
> + return -EINVAL;

This prints uninitialized kernel stack contents instead of what was
copied from user space.

Is the dev_err statement really necessary anyway?


> + }
> +
> + return count;
> +}

If rproc_boot fails, that should be reflected in the syscall result.

This interface is essentially exposing the remoteproc->power refcount to
user space; is that okay?  Seems like it makes it easy to underflow
remoteproc->power through successive shutdown calls.

The other debugfs interface in remoteproc that has a write method
(recovery) accepts more expressive string commands as opposed to 0/1.
It would be more consistent for this interface to take commands such as
"boot" and "shutdown" IMO.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
 +static ssize_t rproc_state_write(struct file *filp, const char __user 
 *userbuf,
 +  size_t count, loff_t *ppos)
 +{
 + struct rproc *rproc = filp-private_data;
 + char buf[2];
 + int ret;
 +
 + ret = copy_from_user(buf, userbuf, 1);
 + if (ret)
 + return -EFAULT;
 +
 + switch (buf[0]) {
 + case '0':
 + rproc_shutdown(rproc);
 + break;
 + case '1':
 + ret = rproc_boot(rproc);
 + if (ret)
 + dev_warn(rproc-dev, Boot failed: %d\n, ret);
 + break;
 + default:
 + dev_err(rproc-dev, Unrecognised option: %x\n, buf[1]);
 + return -EINVAL;

This prints uninitialized kernel stack contents instead of what was
copied from user space.

Is the dev_err statement really necessary anyway?


 + }
 +
 + return count;
 +}

If rproc_boot fails, that should be reflected in the syscall result.

This interface is essentially exposing the remoteproc-power refcount to
user space; is that okay?  Seems like it makes it easy to underflow
remoteproc-power through successive shutdown calls.

The other debugfs interface in remoteproc that has a write method
(recovery) accepts more expressive string commands as opposed to 0/1.
It would be more consistent for this interface to take commands such as
boot and shutdown IMO.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] remoteproc: Supply controller driver for ST's Remote Processors

[Nathan Lynch]

On 08/28/2015 05:31 AM, Lee Jones wrote:
 diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
 index 28c711f..72e97d7 100644
 --- a/drivers/remoteproc/Kconfig
 +++ b/drivers/remoteproc/Kconfig
 @@ -77,4 +77,13 @@ config DA8XX_REMOTEPROC
 It's safe to say n here if you're not interested in multimedia
 offloading.
  
 +config ST_REMOTEPROC
 + tristate ST remoteproc support
 + depends on ARCH_STI
 + select REMOTEPROC
 + help
 +   Say y here to support ST's adjunct processors via the remote
 +   processor framework.
 +   This can be either built-in or a loadable module.
 +

The code uses reset_control_* APIs, so this should depend on
RESET_CONTROLLER, no?

 +/*
 + * ST's Remote Processor Control Driver
 + *
 + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
 + *
 + * Author: Ludovic Barre ludovic.ba...@st.com
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2, or (at your option)
 + * any later version.
 + */

OK, but:

 +MODULE_LICENSE(GPL v2);

These are not in agreement.  You want GPL for MODULE_LICENSE if you
intend v2 or later.


 +static int st_rproc_stop(struct rproc *rproc)
 +{
 + struct st_rproc *st_rproc = rproc-priv;
 + int ret, err = 0;
 +
 + if (st_rproc-config-sw_reset) {
 + ret = reset_control_assert(st_rproc-sw_reset);
 + if (ret)
 + dev_err(rproc-dev, Failed to assert S/W Reset\n);
 + }
 +
 + if (st_rproc-config-pwr_reset) {
 + err = reset_control_assert(st_rproc-pwr_reset);
 + if (err)
 + dev_err(rproc-dev, Failed to assert Power Reset\n);
 + }
 +
 + clk_disable(st_rproc-clk);
 +
 + return ret ?: err;
 +}

Sorry, but I think this is a stylistically inadequate response to my
earlier comments.  At least name the status variables sw_ret and pwr_ret
or something.  And it looks like ret could be used uninitialized.

Also, do you want to unconditionally call clk_disable even if you've
encountered errors?


 +static int st_rproc_start(struct rproc *rproc)
 +{
 + struct st_rproc *st_rproc = rproc-priv;
 + int err;
 +
 + regmap_update_bits(st_rproc-boot_base, st_rproc-boot_offset,
 +st_rproc-config-bootaddr_mask, rproc-bootaddr);
 +
 + err = clk_enable(st_rproc-clk);
 + if (err) {
 + dev_err(rproc-dev, Failed to enable clock\n);
 + return err;
 + }
 +
 + if (st_rproc-config-sw_reset) {
 + err = reset_control_deassert(st_rproc-sw_reset);
 + if (err) {
 + dev_err(rproc-dev, Failed to deassert S/W Reset\n);
 + return err;
 + }
 + }
 +
 + if (st_rproc-config-pwr_reset) {
 + err = reset_control_deassert(st_rproc-pwr_reset);
 + if (err) {
 + dev_err(rproc-dev, Failed to deassert Power 
 Reset\n);
 + return err;
 + }
 + }
 +
 + dev_info(rproc-dev, Started from 0x%x\n, rproc-bootaddr);
 +
 + return 0;
 +}

Does this want to unwind any of its operations if it encounters a failure?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: ti: reset irq affinity before freeing irq

[Nathan Lynch]

On 08/27/2015 01:23 PM, Murali Karicheri wrote:
> On 08/27/2015 12:43 PM, Nathan Lynch wrote:
>> On 08/27/2015 10:51 AM, Murali Karicheri wrote:
>>> When using accumulator queue for rx side for network driver, following
>>> warning is seen when doing a reboot command from Linux console. This
>>> is because, affinity value is not reset before calling free_irq(). This
>>> patch fixes this.
>>>
>>> Deconfiguring network interfaces...
>>> [   55.176589] [ cut here ]---
>>> [   55.181232] WARNING: CPU: 0 PID: 2081 at kernel/irq/manage.c:1370
>>> __free_irq+0x208/0x214
>>
>> The full content of the warning should be included in the commit
>> message; __free_irq has several potential sources of warning messages,
>> and line 1370 doesn't correspond to any of them in 4.2-rc8.
> 
> The log corresponds to 4.1.x kernel. Corresponding WARN_ONCE is
> 
> #ifdef CONFIG_SMP
> /* make sure affinity_hint is cleaned up */
> if (WARN_ON_ONCE(desc->affinity_hint))
> desc->affinity_hint = NULL;
> #endif
> 
> Which is line 1922. I can edit to make it 1922. Does that work?

Eh, I guess I wasn't thinking clearly about this clearly when I wrote
that.  (I somehow had the notion that WARN_ON... maybe printed more than
just the file and line number, but that is clearly mistaken.)  I think
your message is fine as is, sorry for the noise.

I don't think changing the line number will make it any easier on future
readers.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: ti: reset irq affinity before freeing irq

[Nathan Lynch]

On 08/27/2015 10:51 AM, Murali Karicheri wrote:
> When using accumulator queue for rx side for network driver, following
> warning is seen when doing a reboot command from Linux console. This
> is because, affinity value is not reset before calling free_irq(). This
> patch fixes this.
> 
> Deconfiguring network interfaces...
> [   55.176589] [ cut here ]---
> [   55.181232] WARNING: CPU: 0 PID: 2081 at kernel/irq/manage.c:1370
> __free_irq+0x208/0x214

The full content of the warning should be included in the commit
message; __free_irq has several potential sources of warning messages,
and line 1370 doesn't correspond to any of them in 4.2-rc8.


> Signed-off-by: Murali Karicheri 
> ---
>  - Applies to v4.2.0-rc8
> 
>  drivers/soc/ti/knav_qmss_acc.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/soc/ti/knav_qmss_acc.c b/drivers/soc/ti/knav_qmss_acc.c
> index ef6f69d..b98fe56 100644
> --- a/drivers/soc/ti/knav_qmss_acc.c
> +++ b/drivers/soc/ti/knav_qmss_acc.c
> @@ -261,6 +261,10 @@ static int knav_range_setup_acc_irq(struct 
> knav_range_info *range,
>   if (old && !new) {
>   dev_dbg(kdev->dev, "setup-acc-irq: freeing %s for channel %s\n",
>   acc->name, acc->name);
> + ret = irq_set_affinity_hint(irq, NULL);
> + if (ret)
> + dev_warn(range->kdev->dev,
> +  "Failed to set IRQ affinity\n");

Seems unnecessary to add a warning here.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: ti: reset irq affinity before freeing irq

[Nathan Lynch]

On 08/27/2015 10:51 AM, Murali Karicheri wrote:
 When using accumulator queue for rx side for network driver, following
 warning is seen when doing a reboot command from Linux console. This
 is because, affinity value is not reset before calling free_irq(). This
 patch fixes this.
 
 Deconfiguring network interfaces...
 [   55.176589] [ cut here ]---
 [   55.181232] WARNING: CPU: 0 PID: 2081 at kernel/irq/manage.c:1370
 __free_irq+0x208/0x214

The full content of the warning should be included in the commit
message; __free_irq has several potential sources of warning messages,
and line 1370 doesn't correspond to any of them in 4.2-rc8.


 Signed-off-by: Murali Karicheri m-kariche...@ti.com
 ---
  - Applies to v4.2.0-rc8
 
  drivers/soc/ti/knav_qmss_acc.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/drivers/soc/ti/knav_qmss_acc.c b/drivers/soc/ti/knav_qmss_acc.c
 index ef6f69d..b98fe56 100644
 --- a/drivers/soc/ti/knav_qmss_acc.c
 +++ b/drivers/soc/ti/knav_qmss_acc.c
 @@ -261,6 +261,10 @@ static int knav_range_setup_acc_irq(struct 
 knav_range_info *range,
   if (old  !new) {
   dev_dbg(kdev-dev, setup-acc-irq: freeing %s for channel %s\n,
   acc-name, acc-name);
 + ret = irq_set_affinity_hint(irq, NULL);
 + if (ret)
 + dev_warn(range-kdev-dev,
 +  Failed to set IRQ affinity\n);

Seems unnecessary to add a warning here.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: ti: reset irq affinity before freeing irq

[Nathan Lynch]

On 08/27/2015 01:23 PM, Murali Karicheri wrote:
 On 08/27/2015 12:43 PM, Nathan Lynch wrote:
 On 08/27/2015 10:51 AM, Murali Karicheri wrote:
 When using accumulator queue for rx side for network driver, following
 warning is seen when doing a reboot command from Linux console. This
 is because, affinity value is not reset before calling free_irq(). This
 patch fixes this.

 Deconfiguring network interfaces...
 [   55.176589] [ cut here ]---
 [   55.181232] WARNING: CPU: 0 PID: 2081 at kernel/irq/manage.c:1370
 __free_irq+0x208/0x214

 The full content of the warning should be included in the commit
 message; __free_irq has several potential sources of warning messages,
 and line 1370 doesn't correspond to any of them in 4.2-rc8.
 
 The log corresponds to 4.1.x kernel. Corresponding WARN_ONCE is
 
 #ifdef CONFIG_SMP
 /* make sure affinity_hint is cleaned up */
 if (WARN_ON_ONCE(desc-affinity_hint))
 desc-affinity_hint = NULL;
 #endif
 
 Which is line 1922. I can edit to make it 1922. Does that work?

Eh, I guess I wasn't thinking clearly about this clearly when I wrote
that.  (I somehow had the notion that WARN_ON... maybe printed more than
just the file and line number, but that is clearly mistaken.)  I think
your message is fine as is, sorry for the noise.

I don't think changing the line number will make it any easier on future
readers.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
> Signed-off-by: Lee Jones 
> ---
>  drivers/remoteproc/remoteproc_debugfs.c | 25 +
>  1 file changed, 25 insertions(+)

The commit message should describe why this is needed...


> diff --git a/drivers/remoteproc/remoteproc_debugfs.c 
> b/drivers/remoteproc/remoteproc_debugfs.c
> index 9d30809..9620962 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -88,8 +88,33 @@ static ssize_t rproc_state_read(struct file *filp, char 
> __user *userbuf,
>   return simple_read_from_buffer(userbuf, count, ppos, buf, i);
>  }
>  
> +static ssize_t rproc_state_write(struct file *filp, const char __user 
> *userbuf,
> +  size_t count, loff_t *ppos)
> +{
> + struct rproc *rproc = filp->private_data;
> + char buf[2];
> + int ret;
> +
> + ret = copy_from_user(buf, userbuf, 1);
> + if (ret)
> + return -EFAULT;
> +
> + switch (buf[0]) {
> + case '1':
> + ret = rproc_boot(rproc);
> + if (ret)
> + dev_warn(>dev, "Boot failed: %d\n", ret);
> + break;
> + default:
> + rproc_shutdown(rproc);
> + }
> +
> + return count;
> +}

... and I suggest that the user interface be reconsidered.  If '1' means
"boot" and literally anything else means "shut down" then you can't add
operations in the future without potentially breaking things.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] remoteproc: Supply controller driver for ST's Remote Processors

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/remoteproc/st_remoteproc.c
> @@ -0,0 +1,300 @@
> +/*
> + * ST's Remote Processor Control Driver
> + *
> + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
> + *
> + * Author: Ludovic Barre 

When submitting code you didn't write, I'd say it's better practice to
clearly indicate its provenance in the commit message.  E.g. something
like "Driver based on code authored by Ludovic Barre for ST".  And
obtain signoffs etc if possible.


> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.

Please review the wording here.  It's unclear whether this is intended
to be v2-only or v2 or later.


> +static int st_rproc_stop(struct rproc *rproc)
> +{
> + struct st_rproc *st_rproc = rproc->priv;
> + int err = 0;
> +
> + if (st_rproc->config->sw_reset) {
> + err = reset_control_assert(st_rproc->sw_reset);
> + if (err)
> + dev_warn(>dev, "Failed to assert S/W Reset\n");
> + }
> +
> + if (st_rproc->config->pwr_reset) {
> + err = reset_control_assert(st_rproc->pwr_reset);
> + if (err)
> + dev_warn(>dev, "Failed to assert Power Reset\n");
> + }
> +
> + clk_disable(st_rproc->clk);
> +
> + return 0;
> +}

Seems like st_rproc_stop should propagate errors back to its caller
instead of always returning 0.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
> diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt 
> b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> new file mode 100644
> index 000..6a933c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
> @@ -0,0 +1,38 @@
> +STMicroelectronics Remote Processor
> +---
> +
> +This binding provides support for adjunct processors found on ST SoCs.
> +
> +The remote processors can be controlled from the bootloader or the primary 
> OS.
> +If the bootloader starts a remote processor processor the primary OS must 
> detect
> +its state and act accordingly.
> +
> +The node name is significant, as it defines the name of the CPU and the name
> +of the firmware to load: "rproc--fw".

This business about the firmware name describes a behavior of Linux's
core remoteproc code and seems out of place in a binding document.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] remoteproc: dt: Provide bindings for ST's Remote Processor Controller driver

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
 diff --git a/Documentation/devicetree/bindings/remoteproc/st-rproc.txt 
 b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
 new file mode 100644
 index 000..6a933c1
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/remoteproc/st-rproc.txt
 @@ -0,0 +1,38 @@
 +STMicroelectronics Remote Processor
 +---
 +
 +This binding provides support for adjunct processors found on ST SoCs.
 +
 +The remote processors can be controlled from the bootloader or the primary 
 OS.
 +If the bootloader starts a remote processor processor the primary OS must 
 detect
 +its state and act accordingly.
 +
 +The node name is significant, as it defines the name of the CPU and the name
 +of the firmware to load: rproc-name-fw.

This business about the firmware name describes a behavior of Linux's
core remoteproc code and seems out of place in a binding document.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] remoteproc: debugfs: Add ability to boot remote processor using debugfs

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
 Signed-off-by: Lee Jones lee.jo...@linaro.org
 ---
  drivers/remoteproc/remoteproc_debugfs.c | 25 +
  1 file changed, 25 insertions(+)

The commit message should describe why this is needed...


 diff --git a/drivers/remoteproc/remoteproc_debugfs.c 
 b/drivers/remoteproc/remoteproc_debugfs.c
 index 9d30809..9620962 100644
 --- a/drivers/remoteproc/remoteproc_debugfs.c
 +++ b/drivers/remoteproc/remoteproc_debugfs.c
 @@ -88,8 +88,33 @@ static ssize_t rproc_state_read(struct file *filp, char 
 __user *userbuf,
   return simple_read_from_buffer(userbuf, count, ppos, buf, i);
  }
  
 +static ssize_t rproc_state_write(struct file *filp, const char __user 
 *userbuf,
 +  size_t count, loff_t *ppos)
 +{
 + struct rproc *rproc = filp-private_data;
 + char buf[2];
 + int ret;
 +
 + ret = copy_from_user(buf, userbuf, 1);
 + if (ret)
 + return -EFAULT;
 +
 + switch (buf[0]) {
 + case '1':
 + ret = rproc_boot(rproc);
 + if (ret)
 + dev_warn(rproc-dev, Boot failed: %d\n, ret);
 + break;
 + default:
 + rproc_shutdown(rproc);
 + }
 +
 + return count;
 +}

... and I suggest that the user interface be reconsidered.  If '1' means
boot and literally anything else means shut down then you can't add
operations in the future without potentially breaking things.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] remoteproc: Supply controller driver for ST's Remote Processors

[Nathan Lynch]

On 08/26/2015 08:08 AM, Lee Jones wrote:
 --- /dev/null
 +++ b/drivers/remoteproc/st_remoteproc.c
 @@ -0,0 +1,300 @@
 +/*
 + * ST's Remote Processor Control Driver
 + *
 + * Copyright (C) 2015 STMicroelectronics - All Rights Reserved
 + *
 + * Author: Ludovic Barre ludovic.ba...@st.com

When submitting code you didn't write, I'd say it's better practice to
clearly indicate its provenance in the commit message.  E.g. something
like Driver based on code authored by Ludovic Barre for ST.  And
obtain signoffs etc if possible.


 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License.

Please review the wording here.  It's unclear whether this is intended
to be v2-only or v2 or later.


 +static int st_rproc_stop(struct rproc *rproc)
 +{
 + struct st_rproc *st_rproc = rproc-priv;
 + int err = 0;
 +
 + if (st_rproc-config-sw_reset) {
 + err = reset_control_assert(st_rproc-sw_reset);
 + if (err)
 + dev_warn(rproc-dev, Failed to assert S/W Reset\n);
 + }
 +
 + if (st_rproc-config-pwr_reset) {
 + err = reset_control_assert(st_rproc-pwr_reset);
 + if (err)
 + dev_warn(rproc-dev, Failed to assert Power Reset\n);
 + }
 +
 + clk_disable(st_rproc-clk);
 +
 + return 0;
 +}

Seems like st_rproc_stop should propagate errors back to its caller
instead of always returning 0.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] ARM: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 ("timekeeping: Copy the shadow-timekeeper over the
real timekeeper last") it has become possible on ARM to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because ARM's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch 
---

Changes since v1:
- Add u32 cast to nsec calculation.

 arch/arm/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..54a5aeab988d 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -296,7 +296,6 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
struct timespec64 *wtm = >wall_to_monotonic;
 
if (!cntvct_ok) {
@@ -308,10 +307,10 @@ void update_vsyscall(struct timekeeper *tk)
 
vdso_write_begin(vdso_data);
 
-   xtime_coarse = __current_kernel_time();
vdso_data->tk_is_cntvct = tk_is_cntvct(tk);
-   vdso_data->xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data->xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data->xtime_coarse_sec = tk->xtime_sec;
+   vdso_data->xtime_coarse_nsec= (u32)(tk->tkr_mono.xtime_nsec 
>>
+   tk->tkr_mono.shift);
vdso_data->wtm_clock_sec= wtm->tv_sec;
vdso_data->wtm_clock_nsec   = wtm->tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] ARM: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 (timekeeping: Copy the shadow-timekeeper over the
real timekeeper last) it has become possible on ARM to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because ARM's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
---

Changes since v1:
- Add u32 cast to nsec calculation.

 arch/arm/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..54a5aeab988d 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -296,7 +296,6 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
struct timespec64 *wtm = tk-wall_to_monotonic;
 
if (!cntvct_ok) {
@@ -308,10 +307,10 @@ void update_vsyscall(struct timekeeper *tk)
 
vdso_write_begin(vdso_data);
 
-   xtime_coarse = __current_kernel_time();
vdso_data-tk_is_cntvct = tk_is_cntvct(tk);
-   vdso_data-xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data-xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data-xtime_coarse_sec = tk-xtime_sec;
+   vdso_data-xtime_coarse_nsec= (u32)(tk-tkr_mono.xtime_nsec 

+   tk-tkr_mono.shift);
vdso_data-wtm_clock_sec= wtm-tv_sec;
vdso_data-wtm_clock_nsec   = wtm-tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] ARM: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 ("timekeeping: Copy the shadow-timekeeper over the
real timekeeper last") it has become possible on ARM to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because ARM's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch 
---
 arch/arm/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..c8b243c1aef8 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -296,7 +296,6 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
struct timespec64 *wtm = >wall_to_monotonic;
 
if (!cntvct_ok) {
@@ -308,10 +307,10 @@ void update_vsyscall(struct timekeeper *tk)
 
vdso_write_begin(vdso_data);
 
-   xtime_coarse = __current_kernel_time();
vdso_data->tk_is_cntvct = tk_is_cntvct(tk);
-   vdso_data->xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data->xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data->xtime_coarse_sec = tk->xtime_sec;
+   vdso_data->xtime_coarse_nsec= tk->tkr_mono.xtime_nsec >>
+   tk->tkr_mono.shift;
vdso_data->wtm_clock_sec= wtm->tv_sec;
vdso_data->wtm_clock_nsec   = wtm->tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] fix vdso coarse clock monotonicity regressions

[Nathan Lynch]

Commit 906c55579a63 ("timekeeping: Copy the shadow-timekeeper over the
real timekeeper last") made it so the user can observe the coarse
clocks going backwards on arm and arm64, if they're really looking for
it.

Technically these are fixing regressions versus 4.1, but I won't be
bothered if they don't make 4.2 final at this late stage, since only
the (seldom-used?) coarse clocks are affected.

I'd like to collect review/acks for these now and make sure they at
least make it into 4.3-rc1 (and -stable after that).


Nathan Lynch (2):
  ARM: VDSO: fix coarse clock monotonicity regression
  arm64: VDSO: fix coarse clock monotonicity regression

 arch/arm/kernel/vdso.c   | 7 +++
 arch/arm64/kernel/vdso.c | 7 +++
 2 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] arm64: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 ("timekeeping: Copy the shadow-timekeeper over the
real timekeeper last") it has become possible on arm64 to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because arm64's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch 
---
 arch/arm64/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index ec37ab3f524f..97bc68f4c689 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -199,16 +199,15 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
u32 use_syscall = strcmp(tk->tkr_mono.clock->name, "arch_sys_counter");
 
++vdso_data->tb_seq_count;
smp_wmb();
 
-   xtime_coarse = __current_kernel_time();
vdso_data->use_syscall  = use_syscall;
-   vdso_data->xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data->xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data->xtime_coarse_sec = tk->xtime_sec;
+   vdso_data->xtime_coarse_nsec= tk->tkr_mono.xtime_nsec >>
+   tk->tkr_mono.shift;
vdso_data->wtm_clock_sec= tk->wall_to_monotonic.tv_sec;
vdso_data->wtm_clock_nsec   = tk->wall_to_monotonic.tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86/vdso: Emit a GNU hash

[Nathan Lynch]

On 08/06/2015 05:52 PM, Andy Lutomirski wrote:
> [adding lots of cc's]
> 
> On Thu, Aug 6, 2015 at 2:45 PM, Andy Lutomirski  wrote:
>> From: Andy Lutomirski 
>>
>> Some dynamic loaders may be slightly faster if a GNU hash is
>> available.  Strangely, this seems to have no effect at all on the
>> vdso size.

FWIW, I see arch/x86/entry/vdso/vdso64.so increase by 168 bytes here,
using GCC 4.9.2, Binutils 2.24 as packaged by Fedora 21.

I see a similar increase when I make the equivalent change for ARM.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] ARM: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 (timekeeping: Copy the shadow-timekeeper over the
real timekeeper last) it has become possible on ARM to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because ARM's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
---
 arch/arm/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..c8b243c1aef8 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -296,7 +296,6 @@ static bool tk_is_cntvct(const struct timekeeper *tk)
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
struct timespec64 *wtm = tk-wall_to_monotonic;
 
if (!cntvct_ok) {
@@ -308,10 +307,10 @@ void update_vsyscall(struct timekeeper *tk)
 
vdso_write_begin(vdso_data);
 
-   xtime_coarse = __current_kernel_time();
vdso_data-tk_is_cntvct = tk_is_cntvct(tk);
-   vdso_data-xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data-xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data-xtime_coarse_sec = tk-xtime_sec;
+   vdso_data-xtime_coarse_nsec= tk-tkr_mono.xtime_nsec 
+   tk-tkr_mono.shift;
vdso_data-wtm_clock_sec= wtm-tv_sec;
vdso_data-wtm_clock_nsec   = wtm-tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] fix vdso coarse clock monotonicity regressions

[Nathan Lynch]

Commit 906c55579a63 (timekeeping: Copy the shadow-timekeeper over the
real timekeeper last) made it so the user can observe the coarse
clocks going backwards on arm and arm64, if they're really looking for
it.

Technically these are fixing regressions versus 4.1, but I won't be
bothered if they don't make 4.2 final at this late stage, since only
the (seldom-used?) coarse clocks are affected.

I'd like to collect review/acks for these now and make sure they at
least make it into 4.3-rc1 (and -stable after that).


Nathan Lynch (2):
  ARM: VDSO: fix coarse clock monotonicity regression
  arm64: VDSO: fix coarse clock monotonicity regression

 arch/arm/kernel/vdso.c   | 7 +++
 arch/arm64/kernel/vdso.c | 7 +++
 2 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] arm64: VDSO: fix coarse clock monotonicity regression

[Nathan Lynch]

Since 906c55579a63 (timekeeping: Copy the shadow-timekeeper over the
real timekeeper last) it has become possible on arm64 to:

- Obtain a CLOCK_MONOTONIC_COARSE or CLOCK_REALTIME_COARSE timestamp
  via syscall.
- Subsequently obtain a timestamp for the same clock ID via VDSO which
  predates the first timestamp (by one jiffy).

This is because arm64's update_vsyscall is deriving the coarse time
using the __current_kernel_time interface, when it should really be
using the timekeeper object provided to it by the timekeeping core.
It happened to work before only because __current_kernel_time would
access the same timekeeper object which had been passed to
update_vsyscall.  This is no longer the case.

Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
---
 arch/arm64/kernel/vdso.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index ec37ab3f524f..97bc68f4c689 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -199,16 +199,15 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
  */
 void update_vsyscall(struct timekeeper *tk)
 {
-   struct timespec xtime_coarse;
u32 use_syscall = strcmp(tk-tkr_mono.clock-name, arch_sys_counter);
 
++vdso_data-tb_seq_count;
smp_wmb();
 
-   xtime_coarse = __current_kernel_time();
vdso_data-use_syscall  = use_syscall;
-   vdso_data-xtime_coarse_sec = xtime_coarse.tv_sec;
-   vdso_data-xtime_coarse_nsec= xtime_coarse.tv_nsec;
+   vdso_data-xtime_coarse_sec = tk-xtime_sec;
+   vdso_data-xtime_coarse_nsec= tk-tkr_mono.xtime_nsec 
+   tk-tkr_mono.shift;
vdso_data-wtm_clock_sec= tk-wall_to_monotonic.tv_sec;
vdso_data-wtm_clock_nsec   = tk-wall_to_monotonic.tv_nsec;
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] x86/vdso: Emit a GNU hash

[Nathan Lynch]

On 08/06/2015 05:52 PM, Andy Lutomirski wrote:
 [adding lots of cc's]
 
 On Thu, Aug 6, 2015 at 2:45 PM, Andy Lutomirski l...@kernel.org wrote:
 From: Andy Lutomirski l...@amacapital.net

 Some dynamic loaders may be slightly faster if a GNU hash is
 available.  Strangely, this seems to have no effect at all on the
 vdso size.

FWIW, I see arch/x86/entry/vdso/vdso64.so increase by 168 bytes here,
using GCC 4.9.2, Binutils 2.24 as packaged by Fedora 21.

I see a similar increase when I make the equivalent change for ARM.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 4/5] ARM: dts: mt8135: enable basic SMP bringup for mt8135

[Nathan Lynch]

On 07/14/2015 12:18 AM, Yingjoe Chen wrote:
> Add arch timer node to enable arch-timer support. MT8135 firmware
> doesn't correctly setup arch-timer frequency and CNTVOFF, add
> properties to workaround this.

[...]

>  
> + timer {
> + compatible = "arm,armv7-timer";
> + interrupt-parent = <>;
> + interrupts =  +   IRQ_TYPE_LEVEL_LOW)>,
> +   +   IRQ_TYPE_LEVEL_LOW)>,
> +   +   IRQ_TYPE_LEVEL_LOW)>,
> +   +   IRQ_TYPE_LEVEL_LOW)>;
> + clock-frequency = <1300>;
> + arm,cpu-registers-not-fw-configured;

It's disappointing to see this property in new DTS submissions.  It
prevents taking advantage of the VDSO for gettimeofday/clock_gettime.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 4/5] ARM: dts: mt8135: enable basic SMP bringup for mt8135

[Nathan Lynch]

On 07/14/2015 12:18 AM, Yingjoe Chen wrote:
 Add arch timer node to enable arch-timer support. MT8135 firmware
 doesn't correctly setup arch-timer frequency and CNTVOFF, add
 properties to workaround this.

[...]

  
 + timer {
 + compatible = arm,armv7-timer;
 + interrupt-parent = gic;
 + interrupts = GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
 +   IRQ_TYPE_LEVEL_LOW),
 +  GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) |
 +   IRQ_TYPE_LEVEL_LOW),
 +  GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) |
 +   IRQ_TYPE_LEVEL_LOW),
 +  GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) |
 +   IRQ_TYPE_LEVEL_LOW);
 + clock-frequency = 1300;
 + arm,cpu-registers-not-fw-configured;

It's disappointing to see this property in new DTS submissions.  It
prevents taking advantage of the VDSO for gettimeofday/clock_gettime.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] arm: vdso: .gitignore: ignore generated vdso.so.raw and vdsomunge

[Nathan Lynch]

On 04/28/2015 03:32 PM, Andrey Skvortsov wrote:
> After kernel is built 'git status' reports:
> 
>  # Untracked files:
>  #   (use "git add ..." to include in what will be committed)
>  #
>  #arch/arm/vdso/vdso.so.raw
>  #arch/arm/vdso/vdsomunge
> 
> These files are created during build process and removed with 'make
> clean'. This patch makes git to ignore them.

Thanks, but this should be fixed already by:

2b507a2d9c7f ARM: 8343/1: VDSO: add build artifacts to .gitignore

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

On 04/27/2015 05:55 AM, Will Deacon wrote:
> On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
>> The 32-bit ARM VDSO needs to know whether a generic timer is present
>> and whether it is suitable for use by user space.  The VDSO
>> initialization code currently duplicates some of the logic from the
>> driver to make this determination, but unfortunately it is incomplete;
>> it will incorrectly enable the VDSO if HYP mode is available or if no
>> interrupt is provided for the virtual timer (see arch_timer_init).  In
>> these cases the driver will switch to memory-backed access while the
>> VDSO will attempt to access the counter using cp15 reads.
>>
>> Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
>> init code whether the arch timer is present and usable.
>>
>> Signed-off-by: Nathan Lynch 
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 12 
>>  include/clocksource/arm_arch_timer.h |  6 ++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c 
>> b/drivers/clocksource/arm_arch_timer.c
>> index 0aa135ddbf80..b75215523d2f 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
>>  return 
>>  }
>>  
>> +/* The ARM VDSO init code needs to know:
>> + * - whether a cp15-based arch timer is present; and if so
>> + * - whether the physical or virtual counter is being used.
>> + */
>> +bool arch_timer_okay_for_vdso(void)
>> +{
>> +if (!(arch_timers_present & ARCH_CP15_TIMER))
>> +return false;
>> +
>> +return arch_timer_use_virtual;
>> +}
> 
> If we're adding this, then it wouldn't hurt to use the same check in
> arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
> `current clocksource' knowledge in there too, to remove the hardcoded
> "arch_sys_counter" check from the arch code?

While I think it makes sense to consolidate the current clocksource check,
I view that as distinct from this (which needs to run at boot, before
anything uses the vdso).

I'm actually now unsure about whether the implementation I have here
is correct.  Take arch_timer_init:

static void __init arch_timer_init(void)
{
/*
 * If HYP mode is available, we know that the physical timer
 * has been configured to be accessible from PL1. Use it, so
 * that a guest can use the virtual timer instead.
 *
 * If no interrupt provided for virtual timer, we'll have to
 * stick to the physical timer. It'd better be accessible...
 */
if (is_hyp_mode_available() || !arch_timer_ppi[VIRT_PPI]) {
arch_timer_use_virtual = false;

if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
!arch_timer_ppi[PHYS_NONSECURE_PPI]) {
pr_warn("arch_timer: No interrupt available, giving 
up\n");
return;
}
}

arch_timer_register();
arch_timer_common_init();
}

I assume this has been working fine for arm64 up to this point --
i.e. arch_timer_use_virtual is false, but the VDSO continues to use
the virtual counter and gets correct results?  If so, I don't see any
reason this wouldn't be true for ARM.  So I'm thinking
arch_timer_use_virtual isn't the right proxy for determining
whether the VDSO can work correctly.

The reason I initially turned to arch_timer_use_virtual is in
arch_timer_of_init:

/*
 * If we cannot rely on firmware initializing the timer registers then
 * we should use the physical timers instead.
 */
if (IS_ENABLED(CONFIG_ARM) &&
of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
arch_timer_use_virtual = false;

I definitely need to catch that case, and this is currently duplicated in
arch/arm/kernel/vdso.c.  Maybe this should toggle an additional boolean,
say "arch_timer_cntvct_ok", which captures the information that is truly
of interest with respect to the VDSO using the virtual counter.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

On 04/27/2015 05:55 AM, Will Deacon wrote:
 On Fri, Apr 24, 2015 at 10:43:20PM +0100, Nathan Lynch wrote:
 The 32-bit ARM VDSO needs to know whether a generic timer is present
 and whether it is suitable for use by user space.  The VDSO
 initialization code currently duplicates some of the logic from the
 driver to make this determination, but unfortunately it is incomplete;
 it will incorrectly enable the VDSO if HYP mode is available or if no
 interrupt is provided for the virtual timer (see arch_timer_init).  In
 these cases the driver will switch to memory-backed access while the
 VDSO will attempt to access the counter using cp15 reads.

 Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
 init code whether the arch timer is present and usable.

 Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
 ---
  drivers/clocksource/arm_arch_timer.c | 12 
  include/clocksource/arm_arch_timer.h |  6 ++
  2 files changed, 18 insertions(+)

 diff --git a/drivers/clocksource/arm_arch_timer.c 
 b/drivers/clocksource/arm_arch_timer.c
 index 0aa135ddbf80..b75215523d2f 100644
 --- a/drivers/clocksource/arm_arch_timer.c
 +++ b/drivers/clocksource/arm_arch_timer.c
 @@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
  return timecounter;
  }
  
 +/* The ARM VDSO init code needs to know:
 + * - whether a cp15-based arch timer is present; and if so
 + * - whether the physical or virtual counter is being used.
 + */
 +bool arch_timer_okay_for_vdso(void)
 +{
 +if (!(arch_timers_present  ARCH_CP15_TIMER))
 +return false;
 +
 +return arch_timer_use_virtual;
 +}
 
 If we're adding this, then it wouldn't hurt to use the same check in
 arch/arm64 when we update_vsyscall(...). Could we also encapsulate the
 `current clocksource' knowledge in there too, to remove the hardcoded
 arch_sys_counter check from the arch code?

While I think it makes sense to consolidate the current clocksource check,
I view that as distinct from this (which needs to run at boot, before
anything uses the vdso).

I'm actually now unsure about whether the implementation I have here
is correct.  Take arch_timer_init:

static void __init arch_timer_init(void)
{
/*
 * If HYP mode is available, we know that the physical timer
 * has been configured to be accessible from PL1. Use it, so
 * that a guest can use the virtual timer instead.
 *
 * If no interrupt provided for virtual timer, we'll have to
 * stick to the physical timer. It'd better be accessible...
 */
if (is_hyp_mode_available() || !arch_timer_ppi[VIRT_PPI]) {
arch_timer_use_virtual = false;

if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
!arch_timer_ppi[PHYS_NONSECURE_PPI]) {
pr_warn(arch_timer: No interrupt available, giving 
up\n);
return;
}
}

arch_timer_register();
arch_timer_common_init();
}

I assume this has been working fine for arm64 up to this point --
i.e. arch_timer_use_virtual is false, but the VDSO continues to use
the virtual counter and gets correct results?  If so, I don't see any
reason this wouldn't be true for ARM.  So I'm thinking
arch_timer_use_virtual isn't the right proxy for determining
whether the VDSO can work correctly.

The reason I initially turned to arch_timer_use_virtual is in
arch_timer_of_init:

/*
 * If we cannot rely on firmware initializing the timer registers then
 * we should use the physical timers instead.
 */
if (IS_ENABLED(CONFIG_ARM) 
of_property_read_bool(np, arm,cpu-registers-not-fw-configured))
arch_timer_use_virtual = false;

I definitely need to catch that case, and this is currently duplicated in
arch/arm/kernel/vdso.c.  Maybe this should toggle an additional boolean,
say arch_timer_cntvct_ok, which captures the information that is truly
of interest with respect to the VDSO using the virtual counter.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] arm: vdso: .gitignore: ignore generated vdso.so.raw and vdsomunge

[Nathan Lynch]

On 04/28/2015 03:32 PM, Andrey Skvortsov wrote:
 After kernel is built 'git status' reports:
 
  # Untracked files:
  #   (use git add file... to include in what will be committed)
  #
  #arch/arm/vdso/vdso.so.raw
  #arch/arm/vdso/vdsomunge
 
 These files are created during build process and removed with 'make
 clean'. This patch makes git to ignore them.

Thanks, but this should be fixed already by:

2b507a2d9c7f ARM: 8343/1: VDSO: add build artifacts to .gitignore

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

The 32-bit ARM VDSO needs to know whether a generic timer is present
and whether it is suitable for use by user space.  The VDSO
initialization code currently duplicates some of the logic from the
driver to make this determination, but unfortunately it is incomplete;
it will incorrectly enable the VDSO if HYP mode is available or if no
interrupt is provided for the virtual timer (see arch_timer_init).  In
these cases the driver will switch to memory-backed access while the
VDSO will attempt to access the counter using cp15 reads.

Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
init code whether the arch timer is present and usable.

Signed-off-by: Nathan Lynch 
---
 drivers/clocksource/arm_arch_timer.c | 12 
 include/clocksource/arm_arch_timer.h |  6 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 0aa135ddbf80..b75215523d2f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
return 
 }
 
+/* The ARM VDSO init code needs to know:
+ * - whether a cp15-based arch timer is present; and if so
+ * - whether the physical or virtual counter is being used.
+ */
+bool arch_timer_okay_for_vdso(void)
+{
+   if (!(arch_timers_present & ARCH_CP15_TIMER))
+   return false;
+
+   return arch_timer_use_virtual;
+}
+
 static void __init arch_counter_register(unsigned type)
 {
u64 start_count;
diff --git a/include/clocksource/arm_arch_timer.h 
b/include/clocksource/arm_arch_timer.h
index 9916d0e4eff5..bfc1e95280c4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -48,6 +48,7 @@ enum arch_timer_reg {
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
+extern bool arch_timer_okay_for_vdso(void);
 
 #else
 
@@ -66,6 +67,11 @@ static inline struct timecounter 
*arch_timer_get_timecounter(void)
return NULL;
 }
 
+static inline bool arch_timer_okay_for_vdso(void)
+{
+   return false;
+}
+
 #endif
 
 #endif
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] ARM: VDSO: use arch_timer_okay_for_vdso

[Nathan Lynch]

Use the facility now provided by the arm_arch_timer driver to
determine whether there's a usable virtual counter for the VDSO.

Signed-off-by: Nathan Lynch 
---
 arch/arm/kernel/vdso.c | 30 +-
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..f06fd6f3f65f 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -21,7 +21,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -69,33 +68,6 @@ struct elfinfo {
  */
 static bool cntvct_ok __read_mostly;
 
-static bool __init cntvct_functional(void)
-{
-   struct device_node *np;
-   bool ret = false;
-
-   if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-   goto out;
-
-   /* The arm_arch_timer core should export
-* arch_timer_use_virtual or similar so we don't have to do
-* this.
-*/
-   np = of_find_compatible_node(NULL, NULL, "arm,armv7-timer");
-   if (!np)
-   goto out_put;
-
-   if (of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
-   goto out_put;
-
-   ret = true;
-
-out_put:
-   of_node_put(np);
-out:
-   return ret;
-}
-
 static void * __init find_section(Elf32_Ehdr *ehdr, const char *name,
  unsigned long *size)
 {
@@ -208,7 +180,7 @@ static int __init vdso_init(void)
vdso_total_pages = 1; /* for the data/vvar page */
vdso_total_pages += text_pages;
 
-   cntvct_ok = cntvct_functional();
+   cntvct_ok = arch_timer_okay_for_vdso();
 
patch_vdso(_start);
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] clocksource: arm_arch_timer: add arch_timer_okay_for_vdso

[Nathan Lynch]

The 32-bit ARM VDSO needs to know whether a generic timer is present
and whether it is suitable for use by user space.  The VDSO
initialization code currently duplicates some of the logic from the
driver to make this determination, but unfortunately it is incomplete;
it will incorrectly enable the VDSO if HYP mode is available or if no
interrupt is provided for the virtual timer (see arch_timer_init).  In
these cases the driver will switch to memory-backed access while the
VDSO will attempt to access the counter using cp15 reads.

Add an arch_timer_okay_for_vdso API which can reliably inform the VDSO
init code whether the arch timer is present and usable.

Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
---
 drivers/clocksource/arm_arch_timer.c | 12 
 include/clocksource/arm_arch_timer.h |  6 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 0aa135ddbf80..b75215523d2f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -462,6 +462,18 @@ struct timecounter *arch_timer_get_timecounter(void)
return timecounter;
 }
 
+/* The ARM VDSO init code needs to know:
+ * - whether a cp15-based arch timer is present; and if so
+ * - whether the physical or virtual counter is being used.
+ */
+bool arch_timer_okay_for_vdso(void)
+{
+   if (!(arch_timers_present  ARCH_CP15_TIMER))
+   return false;
+
+   return arch_timer_use_virtual;
+}
+
 static void __init arch_counter_register(unsigned type)
 {
u64 start_count;
diff --git a/include/clocksource/arm_arch_timer.h 
b/include/clocksource/arm_arch_timer.h
index 9916d0e4eff5..bfc1e95280c4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -48,6 +48,7 @@ enum arch_timer_reg {
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
+extern bool arch_timer_okay_for_vdso(void);
 
 #else
 
@@ -66,6 +67,11 @@ static inline struct timecounter 
*arch_timer_get_timecounter(void)
return NULL;
 }
 
+static inline bool arch_timer_okay_for_vdso(void)
+{
+   return false;
+}
+
 #endif
 
 #endif
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] ARM: VDSO: use arch_timer_okay_for_vdso

[Nathan Lynch]

Use the facility now provided by the arm_arch_timer driver to
determine whether there's a usable virtual counter for the VDSO.

Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
---
 arch/arm/kernel/vdso.c | 30 +-
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/arch/arm/kernel/vdso.c b/arch/arm/kernel/vdso.c
index efe17dd9b921..f06fd6f3f65f 100644
--- a/arch/arm/kernel/vdso.c
+++ b/arch/arm/kernel/vdso.c
@@ -21,7 +21,6 @@
 #include linux/err.h
 #include linux/kernel.h
 #include linux/mm.h
-#include linux/of.h
 #include linux/printk.h
 #include linux/slab.h
 #include linux/timekeeper_internal.h
@@ -69,33 +68,6 @@ struct elfinfo {
  */
 static bool cntvct_ok __read_mostly;
 
-static bool __init cntvct_functional(void)
-{
-   struct device_node *np;
-   bool ret = false;
-
-   if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-   goto out;
-
-   /* The arm_arch_timer core should export
-* arch_timer_use_virtual or similar so we don't have to do
-* this.
-*/
-   np = of_find_compatible_node(NULL, NULL, arm,armv7-timer);
-   if (!np)
-   goto out_put;
-
-   if (of_property_read_bool(np, arm,cpu-registers-not-fw-configured))
-   goto out_put;
-
-   ret = true;
-
-out_put:
-   of_node_put(np);
-out:
-   return ret;
-}
-
 static void * __init find_section(Elf32_Ehdr *ehdr, const char *name,
  unsigned long *size)
 {
@@ -208,7 +180,7 @@ static int __init vdso_init(void)
vdso_total_pages = 1; /* for the data/vvar page */
vdso_total_pages += text_pages;
 
-   cntvct_ok = cntvct_functional();
+   cntvct_ok = arch_timer_okay_for_vdso();
 
patch_vdso(vdso_start);
 
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: build failure after merge of the tip tree

[Nathan Lynch]

On 03/30/2015 10:08 AM, Russell King - ARM Linux wrote:
> On Mon, Mar 30, 2015 at 09:57:48AM -0500, Nathan Lynch wrote:
>> On 03/30/2015 03:08 AM, Stephen Rothwell wrote:
>>> Hi Russell,
>>>
>>> On Mon, 30 Mar 2015 08:15:37 +0100 Russell King - ARM Linux 
>>>  wrote:
>>>>
>>>> I'll drop the VDSO stuff from the ARM tree; I can't see a way to keep
>>>> it in my tree and keep my tree buildable without dragging in the tip
>>>> tree.
>>>
>>> Does it affect your tree on its own?  If not, then it can be fixed when
>>> merged as I have done, or if you look at the tip tree, all you really
>>> need to merge is tip timers/core branch (which I am sure the tip guys
>>> can tell you if it is stable enough) which is about 28 commits ...
>>>
>>>> The ARM VDSO stuff will just have to wait for 4.2 instead.
>>>
>>> If that works for you.
>>
>> FWIW, Stephen's merge fix is correct and I have run my vdso tests
>> without problems on OMAP5 with next-20150330.
> 
> Hopefully, I can pull the tip stuff but if not, I'll try to remember
> to include Stephen's patch with my pull request, but I can't make any
> guarantees - Stephen's email will very quickly get buried in my mailbox,
> and I'll most likely forget about it too... I'm notoriously bad with
> email...

OK, let me know if I can help.

I'm happy to remind you about it when the merge window opens.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: build failure after merge of the tip tree

[Nathan Lynch]

On 03/30/2015 03:08 AM, Stephen Rothwell wrote:
> Hi Russell,
> 
> On Mon, 30 Mar 2015 08:15:37 +0100 Russell King - ARM Linux 
>  wrote:
>>
>> I'll drop the VDSO stuff from the ARM tree; I can't see a way to keep
>> it in my tree and keep my tree buildable without dragging in the tip
>> tree.
> 
> Does it affect your tree on its own?  If not, then it can be fixed when
> merged as I have done, or if you look at the tip tree, all you really
> need to merge is tip timers/core branch (which I am sure the tip guys
> can tell you if it is stable enough) which is about 28 commits ...
> 
>> The ARM VDSO stuff will just have to wait for 4.2 instead.
> 
> If that works for you.

FWIW, Stephen's merge fix is correct and I have run my vdso tests
without problems on OMAP5 with next-20150330.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: build failure after merge of the tip tree

[Nathan Lynch]

On 03/30/2015 03:08 AM, Stephen Rothwell wrote:
 Hi Russell,
 
 On Mon, 30 Mar 2015 08:15:37 +0100 Russell King - ARM Linux 
 li...@arm.linux.org.uk wrote:

 I'll drop the VDSO stuff from the ARM tree; I can't see a way to keep
 it in my tree and keep my tree buildable without dragging in the tip
 tree.
 
 Does it affect your tree on its own?  If not, then it can be fixed when
 merged as I have done, or if you look at the tip tree, all you really
 need to merge is tip timers/core branch (which I am sure the tip guys
 can tell you if it is stable enough) which is about 28 commits ...
 
 The ARM VDSO stuff will just have to wait for 4.2 instead.
 
 If that works for you.

FWIW, Stephen's merge fix is correct and I have run my vdso tests
without problems on OMAP5 with next-20150330.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: build failure after merge of the tip tree

[Nathan Lynch]

On 03/30/2015 10:08 AM, Russell King - ARM Linux wrote:
 On Mon, Mar 30, 2015 at 09:57:48AM -0500, Nathan Lynch wrote:
 On 03/30/2015 03:08 AM, Stephen Rothwell wrote:
 Hi Russell,

 On Mon, 30 Mar 2015 08:15:37 +0100 Russell King - ARM Linux 
 li...@arm.linux.org.uk wrote:

 I'll drop the VDSO stuff from the ARM tree; I can't see a way to keep
 it in my tree and keep my tree buildable without dragging in the tip
 tree.

 Does it affect your tree on its own?  If not, then it can be fixed when
 merged as I have done, or if you look at the tip tree, all you really
 need to merge is tip timers/core branch (which I am sure the tip guys
 can tell you if it is stable enough) which is about 28 commits ...

 The ARM VDSO stuff will just have to wait for 4.2 instead.

 If that works for you.

 FWIW, Stephen's merge fix is correct and I have run my vdso tests
 without problems on OMAP5 with next-20150330.
 
 Hopefully, I can pull the tip stuff but if not, I'll try to remember
 to include Stephen's patch with my pull request, but I can't make any
 guarantees - Stephen's email will very quickly get buried in my mailbox,
 and I'll most likely forget about it too... I'm notoriously bad with
 email...

OK, let me know if I can help.

I'm happy to remind you about it when the merge window opens.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Nathan Lynch]

On 01/19/2015 04:30 AM, Michal Simek wrote:
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 5e343bab9458..3955f42e9e9c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
> It's safe to say n here if you're not interested in multimedia
> offloading.
> 
> +config MB_REMOTEPROC
> + tristate "Support Microblaze remoteproc"
> + depends on ARCH_ZYNQ && !DEBUG_SG
> + select GPIO_XILINX
> + select REMOTEPROC
> + select RPMSG
> + default m
> + help
> +   Say y here to support Xilinx Microblaze remote processors
> +   on the Xilinx Zynq.
> +
>  endmenu

The dependency on !DEBUG_SG seems unusual, and worth a comment if it's
truly needed.  And the module should be disabled by default, no?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] remoteproc: microblaze: Add support for microblaze on Zynq

[Nathan Lynch]

On 01/19/2015 04:30 AM, Michal Simek wrote:
 diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
 index 5e343bab9458..3955f42e9e9c 100644
 --- a/drivers/remoteproc/Kconfig
 +++ b/drivers/remoteproc/Kconfig
 @@ -64,4 +64,15 @@ config DA8XX_REMOTEPROC
 It's safe to say n here if you're not interested in multimedia
 offloading.
 
 +config MB_REMOTEPROC
 + tristate Support Microblaze remoteproc
 + depends on ARCH_ZYNQ  !DEBUG_SG
 + select GPIO_XILINX
 + select REMOTEPROC
 + select RPMSG
 + default m
 + help
 +   Say y here to support Xilinx Microblaze remote processors
 +   on the Xilinx Zynq.
 +
  endmenu

The dependency on !DEBUG_SG seems unusual, and worth a comment if it's
truly needed.  And the module should be disabled by default, no?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tracing/syscalls: ignore numbers outside NR_syscalls' range

[Nathan Lynch]

On 10/30/2014 06:35 AM, Russell King - ARM Linux wrote:
> On Thu, Oct 30, 2014 at 07:30:28AM -0400, Steven Rostedt wrote:
>> On Thu, 30 Oct 2014 11:14:41 +
>> Russell King - ARM Linux  wrote:
>>
>>
>>> We have always had syscall number range of 0x90 or so.  The tracing
>>> design does not expect that.  Therefore, the tracing design did not take
>>> account of ARM when it was created.  Therefore, it's up to the tracing
>>> people to decide how to properly fit their ill-designed subsystem into
>>> one of the popular and well-established kernel architectures - or at
>>> least suggest a way to work around this issue.
>>>
>>
>>
>> Fine, lets define a MAX_SYSCALL_NR that is by default NR_syscalls, but
>> an architecture can override it.
>>
>> In trace_syscalls.c, where the checks are done, have this:
>>
>> #ifndef MAX_SYSCALL_NR
>> # define MAX_SYSCALL_NR NR_syscalls
>> #endif
>>
>> change all the checks to test against MAX_SYSCALL_NR instead of
>> NR_syscalls.
>>
>> Then in arch/arm/include/asm/syscall.h have:
>>
>> #define MAX_SYSCALL_NR 0xa0
>>
>> or whatever would be the highest syscall number for ARM.
> 
> Or do we just ignore the high "special" ARM syscalls and treat them (from
> the tracing point of view) as non-syscalls, avoiding the allocation of
> something around 1.2MB for the syscall bitmap.  I really don't know, I
> don't use any of this tracing stuff, so it isn't something I care about.
> 
> Maybe those who do use the facility should have an input here?

I checked strace and it knows about ARM's high syscalls.  I wouldn't
want to go from casually using strace to digging deeper with ftrace only
to get the impression that syscalls are disappearing.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] tracing/syscalls: ignore numbers outside NR_syscalls' range

[Nathan Lynch]

On 10/30/2014 06:35 AM, Russell King - ARM Linux wrote:
 On Thu, Oct 30, 2014 at 07:30:28AM -0400, Steven Rostedt wrote:
 On Thu, 30 Oct 2014 11:14:41 +
 Russell King - ARM Linux li...@arm.linux.org.uk wrote:


 We have always had syscall number range of 0x90 or so.  The tracing
 design does not expect that.  Therefore, the tracing design did not take
 account of ARM when it was created.  Therefore, it's up to the tracing
 people to decide how to properly fit their ill-designed subsystem into
 one of the popular and well-established kernel architectures - or at
 least suggest a way to work around this issue.



 Fine, lets define a MAX_SYSCALL_NR that is by default NR_syscalls, but
 an architecture can override it.

 In trace_syscalls.c, where the checks are done, have this:

 #ifndef MAX_SYSCALL_NR
 # define MAX_SYSCALL_NR NR_syscalls
 #endif

 change all the checks to test against MAX_SYSCALL_NR instead of
 NR_syscalls.

 Then in arch/arm/include/asm/syscall.h have:

 #define MAX_SYSCALL_NR 0xa0

 or whatever would be the highest syscall number for ARM.
 
 Or do we just ignore the high special ARM syscalls and treat them (from
 the tracing point of view) as non-syscalls, avoiding the allocation of
 something around 1.2MB for the syscall bitmap.  I really don't know, I
 don't use any of this tracing stuff, so it isn't something I care about.
 
 Maybe those who do use the facility should have an input here?

I checked strace and it knows about ARM's high syscalls.  I wouldn't
want to go from casually using strace to digging deeper with ftrace only
to get the impression that syscalls are disappearing.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: RCU bug with v3.17-rc3 ?

[Nathan Lynch]

On 10/10/2014 08:44 PM, Nathan Lynch wrote:
> On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>>
>> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
>> it seems that this has been known about for some time.)
> 
> Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
> are affected, as well as 4.9.0.

Correction -- 4.9.0 has this fixed, even though the GCC PR shows it as a
"known to fail" version.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: RCU bug with v3.17-rc3 ?

[Nathan Lynch]

On 10/10/2014 08:44 PM, Nathan Lynch wrote:
 On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:

 Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
 it seems that this has been known about for some time.)
 
 Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x  3
 are affected, as well as 4.9.0.

Correction -- 4.9.0 has this fixed, even though the GCC PR shows it as a
known to fail version.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: RCU bug with v3.17-rc3 ?

[Nathan Lynch]

On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
> 
> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
> it seems that this has been known about for some time.)

Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
are affected, as well as 4.9.0.

> We can blacklist these GCC versions quite easily.  We already have GCC
> 3.3 blacklisted, and it's trivial to add others.  I would want to include
> some proper details about the bug, just like the other existing entries
> we already have in asm-offsets.c, where we name the functions that the
> compiler is known to break where appropriate.

Before blacklisting anything, it's worth considering that simple version
checks would break existing pre-4.8.3 compilers that have been patched
for PR58854.  It looks like Yocto and Buildroot issued releases with
patched 4.8.2 compilers well before the (fixed) 4.8.3 release.  I think
the most we can reasonably do without breaking some correctly-behaving
toolchains is to emit a warning.

Hopefully nobody's still using gcc 4.8 from the Linaro 2013.11 toolchain
release -- since it's a 4.8.3 prerelease from before the fix was
committed you'll get GCC_VERSION == 40803 but still generate bad code.

> However, I'm rather annoyed that there are people here who have known
> for some time that GCC 4.8.1 and GCC 4.8.2 _can_ lead to filesystem
> corruption, and have sat on their backsides doing nothing about getting
> it blacklisted for something like a year.

Mea culpa, although I hadn't drawn the connection to FS corruption
reports until now.  I have known about the issue for some time, but
figured the prevalence of the fix in downstream projects largely
mitigated the issue.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: RCU bug with v3.17-rc3 ?

[Nathan Lynch]

On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
 
 Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
 it seems that this has been known about for some time.)

Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x  3
are affected, as well as 4.9.0.

 We can blacklist these GCC versions quite easily.  We already have GCC
 3.3 blacklisted, and it's trivial to add others.  I would want to include
 some proper details about the bug, just like the other existing entries
 we already have in asm-offsets.c, where we name the functions that the
 compiler is known to break where appropriate.

Before blacklisting anything, it's worth considering that simple version
checks would break existing pre-4.8.3 compilers that have been patched
for PR58854.  It looks like Yocto and Buildroot issued releases with
patched 4.8.2 compilers well before the (fixed) 4.8.3 release.  I think
the most we can reasonably do without breaking some correctly-behaving
toolchains is to emit a warning.

Hopefully nobody's still using gcc 4.8 from the Linaro 2013.11 toolchain
release -- since it's a 4.8.3 prerelease from before the fix was
committed you'll get GCC_VERSION == 40803 but still generate bad code.

 However, I'm rather annoyed that there are people here who have known
 for some time that GCC 4.8.1 and GCC 4.8.2 _can_ lead to filesystem
 corruption, and have sat on their backsides doing nothing about getting
 it blacklisted for something like a year.

Mea culpa, although I hadn't drawn the connection to FS corruption
reports until now.  I have known about the issue for some time, but
figured the prevalence of the fix in downstream projects largely
mitigated the issue.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/3] clocksource: arm_arch_timer: change clocksource name if CP15 unavailable

[Nathan Lynch]

Hi Daniel,

On 09/26/2014 02:04 AM, Daniel Lezcano wrote:
> On 09/18/2014 04:59 PM, Nathan Lynch wrote:
>> The arm and arm64 VDSOs need CP15 access to the architected counter.
>> If this is unavailable (which is allowed by ARM v7), indicate this by
>> changing the clocksource name to "arch_mem_counter" before registering
>> the clocksource.
>>
>> Suggested by Stephen Boyd.
>>
>> Signed-off-by: Nathan Lynch 
>> Reviewed-by: Stephen Boyd 
> 
> Hi Nathan,
> 
> is it possible to have the acked-by of the different maintainers for the
> other patches ?

As Will said, he acked the series.

> Are these patches targeted to be merged for 3.18 ?

3.18 is preferable, if it's not too late.  But nothing will break if it
needs to wait.

Thanks,
Nathan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/3] clocksource: arm_arch_timer: change clocksource name if CP15 unavailable

[Nathan Lynch]

Hi Daniel,

On 09/26/2014 02:04 AM, Daniel Lezcano wrote:
 On 09/18/2014 04:59 PM, Nathan Lynch wrote:
 The arm and arm64 VDSOs need CP15 access to the architected counter.
 If this is unavailable (which is allowed by ARM v7), indicate this by
 changing the clocksource name to arch_mem_counter before registering
 the clocksource.

 Suggested by Stephen Boyd.

 Signed-off-by: Nathan Lynch nathan_ly...@mentor.com
 Reviewed-by: Stephen Boyd sb...@codeaurora.org
 
 Hi Nathan,
 
 is it possible to have the acked-by of the different maintainers for the
 other patches ?

As Will said, he acked the series.

 Are these patches targeted to be merged for 3.18 ?

3.18 is preferable, if it's not too late.  But nothing will break if it
needs to wait.

Thanks,
Nathan

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/24/2014 09:50 AM, Russell King - ARM Linux wrote:
> On Wed, Sep 24, 2014 at 09:32:54AM -0500, Nathan Lynch wrote:
>> On 09/24/2014 09:12 AM, Christopher Covington wrote:
>>> Hi Nathan,
>>>
>>> On 09/22/2014 08:28 PM, Nathan Lynch wrote:
>>>> Hmm, this patch set is merely exposing the hardware counter when it is
>>>> present for the VDSO's use; I take it you have no objection to that?
>>>>
>>>> While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
>>>> facility that is required by the virtualization option in the
>>>> architecture, its utility is not limited to guest operating systems.
>>>
>>> Just to clarify, were the performance improvements you measured from a
>>> virtualized guest or native?
>>
>> Yeah I should have been explicit about this.  My tests and measurements
>> (and all test results I've received from others, I believe) have been on
>> native/host kernels, not guests.
> 
> Have there been any measurements on systems without the architected
> timers?

I do test on iMX6 regularly.  Afraid I don't have any pre-v7 hardware to
check though.

Here's a report from you from an earlier submission that shows little/no
impact:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267552.html

But admittedly vdsotest is just doing rudimentary microbenchmarking.

Running a lttng-ust workload that emits tracepoints as fast as possible
(lttng-ust calls clock_gettime and getcpu on every tracepoint), I see
about 1% degradation on iMX6.


>>> I count 18 dts* files that have "arm,armv7-timer", including platforms with
>>> Krait, Exynos, and Tegra processors.
>>
>> Yup.
> 
> That's not the full story.  Almost every ARM to date has not had an
> architected timer.  Architected timers are a recent addition - as
> pointed out, a Cortex A7/A12/A15 invention.  Most of the platforms I
> see are Cortex A9 which doesn't have any architected timers.
> 
> Yes, it may be fun to work on new hardware and make that perform
> much better than previous, but we should not loose sight that there
> is older hardware out there, and we shouldn't unnecessarily penalise
> it when adding new features.

Agreed, of course, and I'll include more detailed results from systems
without the architected timer in future submissions.


> What we /need/ to know is what the effect providing a VDSO in an
> environment without an architected timer (so using the VDSO fallback
> functions calling the syscalls) and having glibc use it is compared
> to the current situation where there is no VDSO for glibc to use.
> 
> If you can show that there's no difference, then I'm happy to go with
> always providing the VDSO.  If there's a detrimental effect (which I
> suspect there may be, since we now have to have glibc test to see if
> the VDSO is there, jump to the VDSO, the VDSO then tests whether we
> have an architected timer, and then we finally get to issue the
> syscall), then we must avoid providing the VDSO on systems which have
> no architected timer.

One point I would like to raise is that the VDSO provides (or could be
made to provide) acceleration for APIs that are unrelated to the
architected timer:

- clock_gettime with CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE.
This is currently included.

- getcpu, which I had planned on submitting later.

I don't know whether the coarse clock support is compelling; they don't
seem to be commonly used.  But there is a nice 4-5x speedup for those on
iMX6.

getcpu, on the other hand, is one of the two system calls lttng-ust uses
in every tracepoint emitted, and I would like to have it available in
the VDSO on all systems capable of supporting the implementation, which
may take the form of co-opting TPIDRURW or some other register.

So the question of whether to provide the VDSO may not hinge on whether
the architected timer is available.

None of which is to argue that unnecessarily degrading gettimeofday
performance on some systems for the benefit of others is acceptable.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/24/2014 09:12 AM, Christopher Covington wrote:
> Hi Nathan,
> 
> On 09/22/2014 08:28 PM, Nathan Lynch wrote:
>> On 09/22/2014 05:30 PM, Russell King - ARM Linux wrote:
>>> On Mon, Sep 22, 2014 at 04:39:19PM +0100, Will Deacon wrote:
>>>> On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
>>>>> This series contains the necessary changes to allow architected timer
>>>>> access from user-space on 32-bit ARM.  This allows the VDSO to support
>>>>> high resolution timestamps for clock_gettime and gettimeofday.  This
>>>>> also merges substantially similar code from arm and arm64 into the
>>>>> core arm_arch_timer driver.
>>>>>
>>>>> The functional changes are:
>>>>> - When available, CNTVCT is made readable by user space on arm, as it
>>>>>   is on arm64.
>>>>> - The clocksource name becomes "arch_mem_counter" if CP15 access to
>>>>>   the counter is not available.
>>>>>
>>>>> These changes have been carried as part of the ARM VDSO patch set over
>>>>> the last several months, but I am splitting them out here as I assume
>>>>> they should go through the clocksource maintainers.
>>>>
>>>> For the series:
>>>>
>>>>   Acked-by: Will Deacon 
>>>>
>>>> I'm not sure which tree the arch-timer stuff usually goes through, but
>>>> the arm/arm64 bits look fine so I'm happy for them to merged together.
>>>
>>> I raised a while back with Will whether there's much point to having
>>> this on ARM.  While it's useful for virtualisation, the majority of
>>> 32-bit ARM doesn't run virtualised.  So there's little point in having
>>> the VDSO on the majority of platforms - it will just add additional
>>> unnecessary cycles slowing down the system calls that the VDSO is
>>> designed to try to speed up.
>>
>> Hmm, this patch set is merely exposing the hardware counter when it is
>> present for the VDSO's use; I take it you have no objection to that?
>>
>> While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
>> facility that is required by the virtualization option in the
>> architecture, its utility is not limited to guest operating systems.
> 
> Just to clarify, were the performance improvements you measured from a
> virtualized guest or native?

Yeah I should have been explicit about this.  My tests and measurements
(and all test results I've received from others, I believe) have been on
native/host kernels, not guests.


>>> So, my view is that this VDSO will only be of very limited use for
>>> 32-bit ARM, and should not be exposed to userspace unless there is
>>> a reason for it to be exposed (iow, the hardware necessary to support
>>> it is present.)
>>
>> My thinking is that it should prove useful in a growing subset of v7
>> CPUs.  It is useful today on Cortex-A15 and -A7, and I believe -A12 and
>> -A17 implement the generic timer facility as well.
> 
> I count 18 dts* files that have "arm,armv7-timer", including platforms with
> Krait, Exynos, and Tegra processors.

Yup.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/24/2014 09:12 AM, Christopher Covington wrote:
 Hi Nathan,
 
 On 09/22/2014 08:28 PM, Nathan Lynch wrote:
 On 09/22/2014 05:30 PM, Russell King - ARM Linux wrote:
 On Mon, Sep 22, 2014 at 04:39:19PM +0100, Will Deacon wrote:
 On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
 This series contains the necessary changes to allow architected timer
 access from user-space on 32-bit ARM.  This allows the VDSO to support
 high resolution timestamps for clock_gettime and gettimeofday.  This
 also merges substantially similar code from arm and arm64 into the
 core arm_arch_timer driver.

 The functional changes are:
 - When available, CNTVCT is made readable by user space on arm, as it
   is on arm64.
 - The clocksource name becomes arch_mem_counter if CP15 access to
   the counter is not available.

 These changes have been carried as part of the ARM VDSO patch set over
 the last several months, but I am splitting them out here as I assume
 they should go through the clocksource maintainers.

 For the series:

   Acked-by: Will Deacon will.dea...@arm.com

 I'm not sure which tree the arch-timer stuff usually goes through, but
 the arm/arm64 bits look fine so I'm happy for them to merged together.

 I raised a while back with Will whether there's much point to having
 this on ARM.  While it's useful for virtualisation, the majority of
 32-bit ARM doesn't run virtualised.  So there's little point in having
 the VDSO on the majority of platforms - it will just add additional
 unnecessary cycles slowing down the system calls that the VDSO is
 designed to try to speed up.

 Hmm, this patch set is merely exposing the hardware counter when it is
 present for the VDSO's use; I take it you have no objection to that?

 While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
 facility that is required by the virtualization option in the
 architecture, its utility is not limited to guest operating systems.
 
 Just to clarify, were the performance improvements you measured from a
 virtualized guest or native?

Yeah I should have been explicit about this.  My tests and measurements
(and all test results I've received from others, I believe) have been on
native/host kernels, not guests.


 So, my view is that this VDSO will only be of very limited use for
 32-bit ARM, and should not be exposed to userspace unless there is
 a reason for it to be exposed (iow, the hardware necessary to support
 it is present.)

 My thinking is that it should prove useful in a growing subset of v7
 CPUs.  It is useful today on Cortex-A15 and -A7, and I believe -A12 and
 -A17 implement the generic timer facility as well.
 
 I count 18 dts* files that have arm,armv7-timer, including platforms with
 Krait, Exynos, and Tegra processors.

Yup.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/24/2014 09:50 AM, Russell King - ARM Linux wrote:
 On Wed, Sep 24, 2014 at 09:32:54AM -0500, Nathan Lynch wrote:
 On 09/24/2014 09:12 AM, Christopher Covington wrote:
 Hi Nathan,

 On 09/22/2014 08:28 PM, Nathan Lynch wrote:
 Hmm, this patch set is merely exposing the hardware counter when it is
 present for the VDSO's use; I take it you have no objection to that?

 While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
 facility that is required by the virtualization option in the
 architecture, its utility is not limited to guest operating systems.

 Just to clarify, were the performance improvements you measured from a
 virtualized guest or native?

 Yeah I should have been explicit about this.  My tests and measurements
 (and all test results I've received from others, I believe) have been on
 native/host kernels, not guests.
 
 Have there been any measurements on systems without the architected
 timers?

I do test on iMX6 regularly.  Afraid I don't have any pre-v7 hardware to
check though.

Here's a report from you from an earlier submission that shows little/no
impact:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/267552.html

But admittedly vdsotest is just doing rudimentary microbenchmarking.

Running a lttng-ust workload that emits tracepoints as fast as possible
(lttng-ust calls clock_gettime and getcpu on every tracepoint), I see
about 1% degradation on iMX6.


 I count 18 dts* files that have arm,armv7-timer, including platforms with
 Krait, Exynos, and Tegra processors.

 Yup.
 
 That's not the full story.  Almost every ARM to date has not had an
 architected timer.  Architected timers are a recent addition - as
 pointed out, a Cortex A7/A12/A15 invention.  Most of the platforms I
 see are Cortex A9 which doesn't have any architected timers.
 
 Yes, it may be fun to work on new hardware and make that perform
 much better than previous, but we should not loose sight that there
 is older hardware out there, and we shouldn't unnecessarily penalise
 it when adding new features.

Agreed, of course, and I'll include more detailed results from systems
without the architected timer in future submissions.


 What we /need/ to know is what the effect providing a VDSO in an
 environment without an architected timer (so using the VDSO fallback
 functions calling the syscalls) and having glibc use it is compared
 to the current situation where there is no VDSO for glibc to use.
 
 If you can show that there's no difference, then I'm happy to go with
 always providing the VDSO.  If there's a detrimental effect (which I
 suspect there may be, since we now have to have glibc test to see if
 the VDSO is there, jump to the VDSO, the VDSO then tests whether we
 have an architected timer, and then we finally get to issue the
 syscall), then we must avoid providing the VDSO on systems which have
 no architected timer.

One point I would like to raise is that the VDSO provides (or could be
made to provide) acceleration for APIs that are unrelated to the
architected timer:

- clock_gettime with CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE.
This is currently included.

- getcpu, which I had planned on submitting later.

I don't know whether the coarse clock support is compelling; they don't
seem to be commonly used.  But there is a nice 4-5x speedup for those on
iMX6.

getcpu, on the other hand, is one of the two system calls lttng-ust uses
in every tracepoint emitted, and I would like to have it available in
the VDSO on all systems capable of supporting the implementation, which
may take the form of co-opting TPIDRURW or some other register.

So the question of whether to provide the VDSO may not hinge on whether
the architected timer is available.

None of which is to argue that unnecessarily degrading gettimeofday
performance on some systems for the benefit of others is acceptable.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/22/2014 05:30 PM, Russell King - ARM Linux wrote:
> On Mon, Sep 22, 2014 at 04:39:19PM +0100, Will Deacon wrote:
>> On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
>>> This series contains the necessary changes to allow architected timer
>>> access from user-space on 32-bit ARM.  This allows the VDSO to support
>>> high resolution timestamps for clock_gettime and gettimeofday.  This
>>> also merges substantially similar code from arm and arm64 into the
>>> core arm_arch_timer driver.
>>>
>>> The functional changes are:
>>> - When available, CNTVCT is made readable by user space on arm, as it
>>>   is on arm64.
>>> - The clocksource name becomes "arch_mem_counter" if CP15 access to
>>>   the counter is not available.
>>>
>>> These changes have been carried as part of the ARM VDSO patch set over
>>> the last several months, but I am splitting them out here as I assume
>>> they should go through the clocksource maintainers.
>>
>> For the series:
>>
>>   Acked-by: Will Deacon 
>>
>> I'm not sure which tree the arch-timer stuff usually goes through, but
>> the arm/arm64 bits look fine so I'm happy for them to merged together.
> 
> I raised a while back with Will whether there's much point to having
> this on ARM.  While it's useful for virtualisation, the majority of
> 32-bit ARM doesn't run virtualised.  So there's little point in having
> the VDSO on the majority of platforms - it will just add additional
> unnecessary cycles slowing down the system calls that the VDSO is
> designed to try to speed up.

Hmm, this patch set is merely exposing the hardware counter when it is
present for the VDSO's use; I take it you have no objection to that?

While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
facility that is required by the virtualization option in the
architecture, its utility is not limited to guest operating systems.


> So, my view is that this VDSO will only be of very limited use for
> 32-bit ARM, and should not be exposed to userspace unless there is
> a reason for it to be exposed (iow, the hardware necessary to support
> it is present.)

My thinking is that it should prove useful in a growing subset of v7
CPUs.  It is useful today on Cortex-A15 and -A7, and I believe -A12 and
-A17 implement the generic timer facility as well.

Now if you're saying that we shouldn't slow down gettimeofday on systems
which lack a hardware counter that can be safely exposed to userspace, I
can work with that.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/22/2014 10:39 AM, Will Deacon wrote:
> On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
>> This series contains the necessary changes to allow architected timer
>> access from user-space on 32-bit ARM.  This allows the VDSO to support
>> high resolution timestamps for clock_gettime and gettimeofday.  This
>> also merges substantially similar code from arm and arm64 into the
>> core arm_arch_timer driver.
>>
>> The functional changes are:
>> - When available, CNTVCT is made readable by user space on arm, as it
>>   is on arm64.
>> - The clocksource name becomes "arch_mem_counter" if CP15 access to
>>   the counter is not available.
>>
>> These changes have been carried as part of the ARM VDSO patch set over
>> the last several months, but I am splitting them out here as I assume
>> they should go through the clocksource maintainers.
> 
> For the series:
> 
>   Acked-by: Will Deacon 
> 
> I'm not sure which tree the arch-timer stuff usually goes through, but
> the arm/arm64 bits look fine so I'm happy for them to merged together.

Thanks Will.

MAINTAINERS does not have a specific entry for
drivers/clocksource/arm_arch_timer.c, so I am working under the
assumption that the series would go through Daniel or Thomas.

Daniel and/or Thomas, can you take this series please?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/22/2014 10:39 AM, Will Deacon wrote:
 On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
 This series contains the necessary changes to allow architected timer
 access from user-space on 32-bit ARM.  This allows the VDSO to support
 high resolution timestamps for clock_gettime and gettimeofday.  This
 also merges substantially similar code from arm and arm64 into the
 core arm_arch_timer driver.

 The functional changes are:
 - When available, CNTVCT is made readable by user space on arm, as it
   is on arm64.
 - The clocksource name becomes arch_mem_counter if CP15 access to
   the counter is not available.

 These changes have been carried as part of the ARM VDSO patch set over
 the last several months, but I am splitting them out here as I assume
 they should go through the clocksource maintainers.
 
 For the series:
 
   Acked-by: Will Deacon will.dea...@arm.com
 
 I'm not sure which tree the arch-timer stuff usually goes through, but
 the arm/arm64 bits look fine so I'm happy for them to merged together.

Thanks Will.

MAINTAINERS does not have a specific entry for
drivers/clocksource/arm_arch_timer.c, so I am working under the
assumption that the series would go through Daniel or Thomas.

Daniel and/or Thomas, can you take this series please?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

On 09/22/2014 05:30 PM, Russell King - ARM Linux wrote:
 On Mon, Sep 22, 2014 at 04:39:19PM +0100, Will Deacon wrote:
 On Thu, Sep 18, 2014 at 03:59:32PM +0100, Nathan Lynch wrote:
 This series contains the necessary changes to allow architected timer
 access from user-space on 32-bit ARM.  This allows the VDSO to support
 high resolution timestamps for clock_gettime and gettimeofday.  This
 also merges substantially similar code from arm and arm64 into the
 core arm_arch_timer driver.

 The functional changes are:
 - When available, CNTVCT is made readable by user space on arm, as it
   is on arm64.
 - The clocksource name becomes arch_mem_counter if CP15 access to
   the counter is not available.

 These changes have been carried as part of the ARM VDSO patch set over
 the last several months, but I am splitting them out here as I assume
 they should go through the clocksource maintainers.

 For the series:

   Acked-by: Will Deacon will.dea...@arm.com

 I'm not sure which tree the arch-timer stuff usually goes through, but
 the arm/arm64 bits look fine so I'm happy for them to merged together.
 
 I raised a while back with Will whether there's much point to having
 this on ARM.  While it's useful for virtualisation, the majority of
 32-bit ARM doesn't run virtualised.  So there's little point in having
 the VDSO on the majority of platforms - it will just add additional
 unnecessary cycles slowing down the system calls that the VDSO is
 designed to try to speed up.

Hmm, this patch set is merely exposing the hardware counter when it is
present for the VDSO's use; I take it you have no objection to that?

While the 32-bit ARM VDSO I've posted (in a different thread) exploits a
facility that is required by the virtualization option in the
architecture, its utility is not limited to guest operating systems.


 So, my view is that this VDSO will only be of very limited use for
 32-bit ARM, and should not be exposed to userspace unless there is
 a reason for it to be exposed (iow, the hardware necessary to support
 it is present.)

My thinking is that it should prove useful in a growing subset of v7
CPUs.  It is useful today on Cortex-A15 and -A7, and I believe -A12 and
-A17 implement the generic timer facility as well.

Now if you're saying that we shouldn't slow down gettimeofday on systems
which lack a hardware counter that can be safely exposed to userspace, I
can work with that.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/3] clocksource: arm_arch_timer: consolidate arch_timer_evtstrm_enable

[Nathan Lynch]

The arch_timer_evtstrm_enable hooks in arm and arm64 are substantially
similar, the only difference being a CONFIG_COMPAT-conditional section
which is relevant only for arm64.  Copy the arm64 version to the
driver, removing the arch-specific hooks.

Signed-off-by: Nathan Lynch 
---
 arch/arm/include/asm/arch_timer.h| 11 ---
 arch/arm64/include/asm/arch_timer.h  | 14 --
 drivers/clocksource/arm_arch_timer.c | 15 +++
 3 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h 
b/arch/arm/include/asm/arch_timer.h
index b7ae44464231..92793ba69c40 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -99,17 +99,6 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
 }
 
-static inline void arch_timer_evtstrm_enable(int divider)
-{
-   u32 cntkctl = arch_timer_get_cntkctl();
-   cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
-   /* Set the divider and enable virtual event stream */
-   cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
-   | ARCH_TIMER_VIRT_EVT_EN;
-   arch_timer_set_cntkctl(cntkctl);
-   elf_hwcap |= HWCAP_EVTSTRM;
-}
-
 #endif
 
 #endif
diff --git a/arch/arm64/include/asm/arch_timer.h 
b/arch/arm64/include/asm/arch_timer.h
index 49e94c677e7a..f19097134b02 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -104,20 +104,6 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
asm volatile("msr   cntkctl_el1, %0" : : "r" (cntkctl));
 }
 
-static inline void arch_timer_evtstrm_enable(int divider)
-{
-   u32 cntkctl = arch_timer_get_cntkctl();
-   cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
-   /* Set the divider and enable virtual event stream */
-   cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
-   | ARCH_TIMER_VIRT_EVT_EN;
-   arch_timer_set_cntkctl(cntkctl);
-   elf_hwcap |= HWCAP_EVTSTRM;
-#ifdef CONFIG_COMPAT
-   compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
-#endif
-}
-
 static inline u64 arch_counter_get_cntvct(void)
 {
u64 cval;
diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 31781fdd7c19..ce5f99e957b9 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -299,6 +299,21 @@ static void __arch_timer_setup(unsigned type,
clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fff);
 }
 
+static void arch_timer_evtstrm_enable(int divider)
+{
+   u32 cntkctl = arch_timer_get_cntkctl();
+
+   cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
+   /* Set the divider and enable virtual event stream */
+   cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
+   | ARCH_TIMER_VIRT_EVT_EN;
+   arch_timer_set_cntkctl(cntkctl);
+   elf_hwcap |= HWCAP_EVTSTRM;
+#ifdef CONFIG_COMPAT
+   compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
+#endif
+}
+
 static void arch_timer_configure_evtstream(void)
 {
int evt_stream_div, pos;
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/3] clocksource: arm_arch_timer: change clocksource name if CP15 unavailable

[Nathan Lynch]

The arm and arm64 VDSOs need CP15 access to the architected counter.
If this is unavailable (which is allowed by ARM v7), indicate this by
changing the clocksource name to "arch_mem_counter" before registering
the clocksource.

Suggested by Stephen Boyd.

Signed-off-by: Nathan Lynch 
Reviewed-by: Stephen Boyd 
---
 drivers/clocksource/arm_arch_timer.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index 5163ec13429d..c99afdf12e98 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -429,11 +429,19 @@ static void __init arch_counter_register(unsigned type)
u64 start_count;
 
/* Register the CP15 based counter if we have one */
-   if (type & ARCH_CP15_TIMER)
+   if (type & ARCH_CP15_TIMER) {
arch_timer_read_counter = arch_counter_get_cntvct;
-   else
+   } else {
arch_timer_read_counter = arch_counter_get_cntvct_mem;
 
+   /* If the clocksource name is "arch_sys_counter" the
+* VDSO will attempt to read the CP15-based counter.
+* Ensure this does not happen when CP15-based
+* counter is not available.
+*/
+   clocksource_counter.name = "arch_mem_counter";
+   }
+
start_count = arch_timer_read_counter();
clocksource_register_hz(_counter, arch_timer_rate);
cyclecounter.mult = clocksource_counter.mult;
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/3] clocksource: arm_arch_timer: enable counter access for 32-bit ARM

[Nathan Lynch]

The only difference between arm and arm64's implementations of
arch_counter_set_user_access is that 32-bit ARM does not enable user
access to the virtual counter.  We want to enable this access for the
32-bit ARM VDSO, so copy the arm64 version to the driver itself, and
remove the arch-specific implementations.

Signed-off-by: Nathan Lynch 
---
 arch/arm/include/asm/arch_timer.h| 14 --
 arch/arm64/include/asm/arch_timer.h  | 17 -
 drivers/clocksource/arm_arch_timer.c | 17 +
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h 
b/arch/arm/include/asm/arch_timer.h
index 0704e0cf5571..b7ae44464231 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -99,20 +99,6 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
 }
 
-static inline void arch_counter_set_user_access(void)
-{
-   u32 cntkctl = arch_timer_get_cntkctl();
-
-   /* Disable user access to both physical/virtual counters/timers */
-   /* Also disable virtual event stream */
-   cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
-   | ARCH_TIMER_USR_VT_ACCESS_EN
-   | ARCH_TIMER_VIRT_EVT_EN
-   | ARCH_TIMER_USR_VCT_ACCESS_EN
-   | ARCH_TIMER_USR_PCT_ACCESS_EN);
-   arch_timer_set_cntkctl(cntkctl);
-}
-
 static inline void arch_timer_evtstrm_enable(int divider)
 {
u32 cntkctl = arch_timer_get_cntkctl();
diff --git a/arch/arm64/include/asm/arch_timer.h 
b/arch/arm64/include/asm/arch_timer.h
index 9400596a0f39..49e94c677e7a 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -104,23 +104,6 @@ static inline void arch_timer_set_cntkctl(u32 cntkctl)
asm volatile("msr   cntkctl_el1, %0" : : "r" (cntkctl));
 }
 
-static inline void arch_counter_set_user_access(void)
-{
-   u32 cntkctl = arch_timer_get_cntkctl();
-
-   /* Disable user access to the timers and the physical counter */
-   /* Also disable virtual event stream */
-   cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
-   | ARCH_TIMER_USR_VT_ACCESS_EN
-   | ARCH_TIMER_VIRT_EVT_EN
-   | ARCH_TIMER_USR_PCT_ACCESS_EN);
-
-   /* Enable user access to the virtual counter */
-   cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
-
-   arch_timer_set_cntkctl(cntkctl);
-}
-
 static inline void arch_timer_evtstrm_enable(int divider)
 {
u32 cntkctl = arch_timer_get_cntkctl();
diff --git a/drivers/clocksource/arm_arch_timer.c 
b/drivers/clocksource/arm_arch_timer.c
index c99afdf12e98..31781fdd7c19 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -312,6 +312,23 @@ static void arch_timer_configure_evtstream(void)
arch_timer_evtstrm_enable(min(pos, 15));
 }
 
+static void arch_counter_set_user_access(void)
+{
+   u32 cntkctl = arch_timer_get_cntkctl();
+
+   /* Disable user access to the timers and the physical counter */
+   /* Also disable virtual event stream */
+   cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
+   | ARCH_TIMER_USR_VT_ACCESS_EN
+   | ARCH_TIMER_VIRT_EVT_EN
+   | ARCH_TIMER_USR_PCT_ACCESS_EN);
+
+   /* Enable user access to the virtual counter */
+   cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
+
+   arch_timer_set_cntkctl(cntkctl);
+}
+
 static int arch_timer_setup(struct clock_event_device *clk)
 {
__arch_timer_setup(ARCH_CP15_TIMER, clk);
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

This series contains the necessary changes to allow architected timer
access from user-space on 32-bit ARM.  This allows the VDSO to support
high resolution timestamps for clock_gettime and gettimeofday.  This
also merges substantially similar code from arm and arm64 into the
core arm_arch_timer driver.

The functional changes are:
- When available, CNTVCT is made readable by user space on arm, as it
  is on arm64.
- The clocksource name becomes "arch_mem_counter" if CP15 access to
  the counter is not available.

These changes have been carried as part of the ARM VDSO patch set over
the last several months, but I am splitting them out here as I assume
they should go through the clocksource maintainers.

Changes since v1:
- Fix minor checkpatch complaints.
- Rework patches 2-4.  v1 copied arm64's arch_timer_evtstrm_enable
  and arch_counter_set_user_access to the driver with slightly
  different names in one patch, then removed the unused functions in
  subsequent patches for each of arm and arm64.  This seemed kind of
  awkward to me, so I've reorganized those changes into two patches
  that should be easier to review.  Patch #1 is unchanged.

Nathan Lynch (3):
  clocksource: arm_arch_timer: change clocksource name if CP15
unavailable
  clocksource: arm_arch_timer: enable counter access for 32-bit ARM
  clocksource: arm_arch_timer: consolidate arch_timer_evtstrm_enable

 arch/arm/include/asm/arch_timer.h| 25 
 arch/arm64/include/asm/arch_timer.h  | 31 -
 drivers/clocksource/arm_arch_timer.c | 44 ++--
 3 files changed, 42 insertions(+), 58 deletions(-)

-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/3] arm_arch_timer: VDSO preparation, code consolidation

[Nathan Lynch]

This series contains the necessary changes to allow architected timer
access from user-space on 32-bit ARM.  This allows the VDSO to support
high resolution timestamps for clock_gettime and gettimeofday.  This
also merges substantially similar code from arm and arm64 into the
core arm_arch_timer driver.

The functional changes are:
- When available, CNTVCT is made readable by user space on arm, as it
  is on arm64.
- The clocksource name becomes arch_mem_counter if CP15 access to
  the counter is not available.

These changes have been carried as part of the ARM VDSO patch set over
the last several months, but I am splitting them out here as I assume
they should go through the clocksource maintainers.

Changes since v1:
- Fix minor checkpatch complaints.
- Rework patches 2-4.  v1 copied arm64's arch_timer_evtstrm_enable
  and arch_counter_set_user_access to the driver with slightly
  different names in one patch, then removed the unused functions in
  subsequent patches for each of arm and arm64.  This seemed kind of
  awkward to me, so I've reorganized those changes into two patches
  that should be easier to review.  Patch #1 is unchanged.

Nathan Lynch (3):
  clocksource: arm_arch_timer: change clocksource name if CP15
unavailable
  clocksource: arm_arch_timer: enable counter access for 32-bit ARM
  clocksource: arm_arch_timer: consolidate arch_timer_evtstrm_enable

 arch/arm/include/asm/arch_timer.h| 25 
 arch/arm64/include/asm/arch_timer.h  | 31 -
 drivers/clocksource/arm_arch_timer.c | 44 ++--
 3 files changed, 42 insertions(+), 58 deletions(-)

-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/