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

[Pinski, Andrew]

> -Original Message-
> From: Will Deacon [mailto:will.dea...@arm.com]
> Sent: Wednesday, May 31, 2017 5:45 AM
> To: Pinski, Andrew <andrew.pin...@cavium.com>
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> Norov, Yuri <yuri.no...@cavium.com>; catalin.mari...@arm.com;
> nathan_ly...@mentor.com; kevin.brod...@arm.com; dave.mar...@arm.com;
> john.stu...@linaro.org; a...@arndb.de
> Subject: Re: [PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.
> 
> Hi Andrew,
> 
> Thanks for posting this, but please try to cc the maintainers in future --
> I almost missed it!

Oh sorry; I didn't know when I am supposed to CC the maintainers or not.  
Different project, different rules.

> 
> On Tue, May 30, 2017 at 05:34:19PM -0700, Andrew Pinski wrote:
> > This allows the compiler to optimize the divide by 1000.
> > And remove the other divide.
> >
> > On ThunderX, gettimeofday improves by 32%.  On ThunderX 2,
> > gettimeofday improves by 18%.
> >
> > 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.
> 
> 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.

To share code between the three vdso is a good goal and shouldn't be a hard to 
expand my patch to handle the arm compat vdso.  To expand it to the arm native 
code shouldn't be too hard.  The main thing is add a few #ifdef/#define in a 
header.
I will try to do that but I don't know when I will be able to finish it.

Thanks,
Andrew


> 
> 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.
> 
> Will

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

[Pinski, Andrew]

> -Original Message-
> From: Will Deacon [mailto:will.dea...@arm.com]
> Sent: Wednesday, May 31, 2017 5:45 AM
> To: Pinski, Andrew 
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> Norov, Yuri ; catalin.mari...@arm.com;
> nathan_ly...@mentor.com; kevin.brod...@arm.com; dave.mar...@arm.com;
> john.stu...@linaro.org; a...@arndb.de
> Subject: Re: [PATCHv2 1/2] arm64:vdso: Rewrite gettimeofday into C.
> 
> Hi Andrew,
> 
> Thanks for posting this, but please try to cc the maintainers in future --
> I almost missed it!

Oh sorry; I didn't know when I am supposed to CC the maintainers or not.  
Different project, different rules.

> 
> On Tue, May 30, 2017 at 05:34:19PM -0700, Andrew Pinski wrote:
> > This allows the compiler to optimize the divide by 1000.
> > And remove the other divide.
> >
> > On ThunderX, gettimeofday improves by 32%.  On ThunderX 2,
> > gettimeofday improves by 18%.
> >
> > 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.
> 
> 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.

To share code between the three vdso is a good goal and shouldn't be a hard to 
expand my patch to handle the arm compat vdso.  To expand it to the arm native 
code shouldn't be too hard.  The main thing is add a few #ifdef/#define in a 
header.
I will try to do that but I don't know when I will be able to finish it.

Thanks,
Andrew


> 
> 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.
> 
> Will

RE: [PATCH 2/2] arm64:vdso: Remove ISB from gettimeofday.

[Pinski, Andrew]

Sorry sending again as plain text (I did not notice that before).

On 5/6/2017 9:29 AM, Jon Masters wrote:
On 04/23/2017 07:47 PM, Andrew Pinski wrote:
> ISB is normally required before mrs CNTVCT if we want the
> mrs to completed after the loads. In this case it is not.
> As we are taking the difference and if that difference
> was going to be negative, we just use the last counter value
> instead.
> 
> Signed-off-by: Andrew Pinski 

Humor me. Walk me through this?

Here is what the ARM ARM says (D6.1.3):
Reads of CNTVCT_EL0 can occur speculatively and out of order relative to other 
instructions executed on the same PE.
For example, if a read from memory is used to obtain a signal from another 
agent that indicates that CNTVCT_EL0 must be read, an ISB must be used to 
ensure that the read of CNTVCT_EL0 occurs after the signal has been read from 
memory
--- CUT ---

So we could have the read of the virtual timer happen before the reading of the 
cycles last field of the vdso data.  In the current code, there is an ISB which 
forces that case not to happen.  Without the ISB it could happen but since we 
know the time (virtual timer) cannot go backwards, we just take the last cycle 
field as the current time; this could happen when the last cycle field was 
updated from another core.  I hope this walk through a little better than my 
original description.

Basically the current virtual timer is either newly read value or the read from 
last cycle counter.

Thanks,
Andrew Pinski


> ---
>  arch/arm64/kernel/vdso/gettimeofday.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso/gettimeofday.c 
> b/arch/arm64/kernel/vdso/gettimeofday.c
> index a0ab8b1..cf3235a 100644
> --- a/arch/arm64/kernel/vdso/gettimeofday.c
> +++ b/arch/arm64/kernel/vdso/gettimeofday.c
> @@ -117,10 +117,20 @@ static notrace u64 get_clock_shifted_nsec(u64 
> cycle_last, u64 mult)
>    u64 res;
>  
>    /* Read the virtual counter. */
> - isb();
> + /*
> +  * This normally requires an ISB but since we know the
> +  * read of the last cycle will always be after the
> +  * read of the values are valid word.
> +  */
>    asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>  
> - res = res - cycle_last;
> + /*
> +  * If the current cycle is greater than the last,
> +  *  then get the difference.
> +  */
> + if (res > cycle_last)
> + res = res - cycle_last;
> +
>    /* We can only guarantee 56 bits of precision. */
>    res &= ~(0xff00ul<<48);
>    return res * mult;
> 


-- 
Computer Architect

RE: [PATCH 2/2] arm64:vdso: Remove ISB from gettimeofday.

[Pinski, Andrew]

Sorry sending again as plain text (I did not notice that before).

On 5/6/2017 9:29 AM, Jon Masters wrote:
On 04/23/2017 07:47 PM, Andrew Pinski wrote:
> ISB is normally required before mrs CNTVCT if we want the
> mrs to completed after the loads. In this case it is not.
> As we are taking the difference and if that difference
> was going to be negative, we just use the last counter value
> instead.
> 
> Signed-off-by: Andrew Pinski 

Humor me. Walk me through this?

Here is what the ARM ARM says (D6.1.3):
Reads of CNTVCT_EL0 can occur speculatively and out of order relative to other 
instructions executed on the same PE.
For example, if a read from memory is used to obtain a signal from another 
agent that indicates that CNTVCT_EL0 must be read, an ISB must be used to 
ensure that the read of CNTVCT_EL0 occurs after the signal has been read from 
memory
--- CUT ---

So we could have the read of the virtual timer happen before the reading of the 
cycles last field of the vdso data.  In the current code, there is an ISB which 
forces that case not to happen.  Without the ISB it could happen but since we 
know the time (virtual timer) cannot go backwards, we just take the last cycle 
field as the current time; this could happen when the last cycle field was 
updated from another core.  I hope this walk through a little better than my 
original description.

Basically the current virtual timer is either newly read value or the read from 
last cycle counter.

Thanks,
Andrew Pinski


> ---
>  arch/arm64/kernel/vdso/gettimeofday.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso/gettimeofday.c 
> b/arch/arm64/kernel/vdso/gettimeofday.c
> index a0ab8b1..cf3235a 100644
> --- a/arch/arm64/kernel/vdso/gettimeofday.c
> +++ b/arch/arm64/kernel/vdso/gettimeofday.c
> @@ -117,10 +117,20 @@ static notrace u64 get_clock_shifted_nsec(u64 
> cycle_last, u64 mult)
>    u64 res;
>  
>    /* Read the virtual counter. */
> - isb();
> + /*
> +  * This normally requires an ISB but since we know the
> +  * read of the last cycle will always be after the
> +  * read of the values are valid word.
> +  */
>    asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>  
> - res = res - cycle_last;
> + /*
> +  * If the current cycle is greater than the last,
> +  *  then get the difference.
> +  */
> + if (res > cycle_last)
> + res = res - cycle_last;
> +
>    /* We can only guarantee 56 bits of precision. */
>    res &= ~(0xff00ul<<48);
>    return res * mult;
> 


-- 
Computer Architect

Re: [PATCH v5 00/23] ILP32 for ARM64

[Pinski, Andrew]

> On Oct 5, 2015, at 8:59 AM, Catalin Marinas  wrote:
> 
>> On Sat, Oct 03, 2015 at 02:18:57AM +, Kapoor, Prasun wrote:
>>> On 10/2/15, 2:37 AM, "Catalin Marinas"  wrote:
>>> So, at the time, following x32 discussions, we thought of using the
>>> native ABI as much as possible. However, two important things happened
>>> since:
>>> 
>>> 1. libc community didn't like breaking the POSIX compliance
>>> 2. No-one seems desperate for ILP32 on AArch64
>>> 
>>> (1) is a fair point and I would rather be careful as we don't know the
>>> extent of the code affected. In the meantime, we've also had ongoing
>>> work for addressing the 2038 issue on 32-bit architectures.
>>> 
>>> The second point is equally important. The benchmarks I've seen didn't
>>> show a significant improvement and the messages I got on various
>>> channels pretty much labeled ILP32 as a transitional stage to full LP64.
>>> In this case, we need to balance the benefits of a close to native ABI
>>> (future proof, slightly higher performance) vs. the cost of maintaining
>>> such ABI in the kernel on the long term, especially if it's not widely
>>> used/tested.
>> 
>> For us ILP32  is not about putting this into our product flier at all, it
>> is about supporting real applications. We have an existing product line of
>> MIPS based SoCs where a large number of N32 (an exact equivalent of ILP32)
>> applications are currently in production. Our customers are looking to
>> bring those applications (mostly in Networking and Telecom space) over to
>> ARMv8. 
>> 
>> We think its an extremely risky strategy to say either future processors
>> should incur the additional cost (power and complexity) of implementing
>> Aarch32 instruction set or have no way of  supporting 32 bit applications
>> at all.
> 
> Well, given that Cavium posted only 3 versions of this series since
> September 2013, it doesn't seem critical at all.

We are going to post another version as soon as we finish the changes that you 
requested this time around. I am working on the glibc and yury (with my help) 
will doing the kernel side. 


> 
>> Apart from there being an installed base of 32 bit networking and telecom
>> applications, we have also seen non-trivial performance gains with ILP32
>> (for example, our SPECINT score goes up by 7% with ILP32 compared to
>> LP64).
> 
> It would be good to re-run the benchmarks with the latest gcc since
> LP64/AArch64 support has evolved in the meantime.

We are also rerunning the numbers with the latest released gcc (5.2) and will 
report results when we submit the next version of the patch set.  

Thanks,
Andrew

> 
> -- 
> Catalin
--
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 00/23] ILP32 for ARM64

[Pinski, Andrew]

> On Oct 5, 2015, at 8:59 AM, Catalin Marinas  wrote:
> 
>> On Sat, Oct 03, 2015 at 02:18:57AM +, Kapoor, Prasun wrote:
>>> On 10/2/15, 2:37 AM, "Catalin Marinas"  wrote:
>>> So, at the time, following x32 discussions, we thought of using the
>>> native ABI as much as possible. However, two important things happened
>>> since:
>>> 
>>> 1. libc community didn't like breaking the POSIX compliance
>>> 2. No-one seems desperate for ILP32 on AArch64
>>> 
>>> (1) is a fair point and I would rather be careful as we don't know the
>>> extent of the code affected. In the meantime, we've also had ongoing
>>> work for addressing the 2038 issue on 32-bit architectures.
>>> 
>>> The second point is equally important. The benchmarks I've seen didn't
>>> show a significant improvement and the messages I got on various
>>> channels pretty much labeled ILP32 as a transitional stage to full LP64.
>>> In this case, we need to balance the benefits of a close to native ABI
>>> (future proof, slightly higher performance) vs. the cost of maintaining
>>> such ABI in the kernel on the long term, especially if it's not widely
>>> used/tested.
>> 
>> For us ILP32  is not about putting this into our product flier at all, it
>> is about supporting real applications. We have an existing product line of
>> MIPS based SoCs where a large number of N32 (an exact equivalent of ILP32)
>> applications are currently in production. Our customers are looking to
>> bring those applications (mostly in Networking and Telecom space) over to
>> ARMv8. 
>> 
>> We think its an extremely risky strategy to say either future processors
>> should incur the additional cost (power and complexity) of implementing
>> Aarch32 instruction set or have no way of  supporting 32 bit applications
>> at all.
> 
> Well, given that Cavium posted only 3 versions of this series since
> September 2013, it doesn't seem critical at all.

We are going to post another version as soon as we finish the changes that you 
requested this time around. I am working on the glibc and yury (with my help) 
will doing the kernel side. 


> 
>> Apart from there being an installed base of 32 bit networking and telecom
>> applications, we have also seen non-trivial performance gains with ILP32
>> (for example, our SPECINT score goes up by 7% with ILP32 compared to
>> LP64).
> 
> It would be good to re-run the benchmarks with the latest gcc since
> LP64/AArch64 support has evolved in the meantime.

We are also rerunning the numbers with the latest released gcc (5.2) and will 
report results when we submit the next version of the patch set.  

Thanks,
Andrew

> 
> -- 
> Catalin
--
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 00/23] ILP32 for ARM64

[Pinski, Andrew]

> On Oct 1, 2015, at 2:29 PM, Arnd Bergmann  wrote:
> 
>> On Thursday 01 October 2015 22:15:20 Yury Norov wrote:
>> 
>> Regarding time_t, it, of course, doesn't takes much time to make it
>> 32-bit, but I think 64 bit is better because of Y2038. X32 and mips
>> n32 has time_t 64-bit (and ppc, not sure), and that's OK for them.
> 
> I'm pretty sure that n32 has 32-bit time_t, and we know that it still
> causes real-world problems on x32: socket timestamps, v4l, alsa and
> other subsystems all have bugs in this area that are hard to fix.
> 
>> That's OK for BSD as well. The objection may come from users of ABI,
>> complaining portability problems, but I found no such complains in
>> public discussions.
>> 
>> Nevertheless, as I told, I do not see any problem to rework time_t.
>> But some arguments supporting this decision are appreciated.
>> 
>> The downside of 32 bit time_t is that we still face Y2038 problem,
>> but that's the other story fixing it.
> 
> The main reason for 32-bit time_t is compatibility with existing
> ioctls (also getsockopts and some others), and having a sane way
> for fixing them. We cannot change compat_time_t to be 64-bit
> without breaking arm32 compat mode, and we can't use the native
> 64-bit ioctl implementation on ARM64/ILP32 because that breaks
> all interfaces that pass 'long' or a pointer.
> 
> This means drivers that currently pass a time_t (or timeval, timespec
> etc) need to not only have a compat_ioctl handler to convert it,
> they also need to check whether which of the two compat modes they
> are talking to. This is a mess to add (I know, because I'm working
> on this for y2038 compliance for normal 32-bit mode), and making
> the two behave differently makes it even harder to get right for
> all cases.
> 
>> __kernel_long_t is the same. Now it's 64 bits length. Compatibility
>> may suffer, but, again, there're no complains, and in long run it
>> looks better.
> 
> __kernel_long_t isn't actually used that much, and rarely used in
> places where it matters. The idea was to be able to reuse the
> native syscalls rather than the compat syscall calls, but that
> comes with the downside of defining the ABI in a way that is
> incompatible with all other 32-bit user space.
> 
> Having a 64-bit __kernel_off_t is similar to the 64-bit time_t:
> a good idea in principle, but it breaks device drivers that
> expect user space to pass 32-bit arguments. For any interface
> that really needs 64-bit data, we have to fix it for all
> 32-bit architectures, and we're better off avoiding special
> cases.

Ok, we will rewrite these patches using 32bit time_t and 32bit off_t and redo 
the toolchain support for them.  Note this is going back to the abi I had 
originally done when I submitted my original version when it was asked to 
change time_t to be 64bit. 

Thanks,
Andrew


> 
>Arnd
--
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 00/23] ILP32 for ARM64

[Pinski, Andrew]

> On Oct 1, 2015, at 2:29 PM, Arnd Bergmann  wrote:
> 
>> On Thursday 01 October 2015 22:15:20 Yury Norov wrote:
>> 
>> Regarding time_t, it, of course, doesn't takes much time to make it
>> 32-bit, but I think 64 bit is better because of Y2038. X32 and mips
>> n32 has time_t 64-bit (and ppc, not sure), and that's OK for them.
> 
> I'm pretty sure that n32 has 32-bit time_t, and we know that it still
> causes real-world problems on x32: socket timestamps, v4l, alsa and
> other subsystems all have bugs in this area that are hard to fix.
> 
>> That's OK for BSD as well. The objection may come from users of ABI,
>> complaining portability problems, but I found no such complains in
>> public discussions.
>> 
>> Nevertheless, as I told, I do not see any problem to rework time_t.
>> But some arguments supporting this decision are appreciated.
>> 
>> The downside of 32 bit time_t is that we still face Y2038 problem,
>> but that's the other story fixing it.
> 
> The main reason for 32-bit time_t is compatibility with existing
> ioctls (also getsockopts and some others), and having a sane way
> for fixing them. We cannot change compat_time_t to be 64-bit
> without breaking arm32 compat mode, and we can't use the native
> 64-bit ioctl implementation on ARM64/ILP32 because that breaks
> all interfaces that pass 'long' or a pointer.
> 
> This means drivers that currently pass a time_t (or timeval, timespec
> etc) need to not only have a compat_ioctl handler to convert it,
> they also need to check whether which of the two compat modes they
> are talking to. This is a mess to add (I know, because I'm working
> on this for y2038 compliance for normal 32-bit mode), and making
> the two behave differently makes it even harder to get right for
> all cases.
> 
>> __kernel_long_t is the same. Now it's 64 bits length. Compatibility
>> may suffer, but, again, there're no complains, and in long run it
>> looks better.
> 
> __kernel_long_t isn't actually used that much, and rarely used in
> places where it matters. The idea was to be able to reuse the
> native syscalls rather than the compat syscall calls, but that
> comes with the downside of defining the ABI in a way that is
> incompatible with all other 32-bit user space.
> 
> Having a 64-bit __kernel_off_t is similar to the 64-bit time_t:
> a good idea in principle, but it breaks device drivers that
> expect user space to pass 32-bit arguments. For any interface
> that really needs 64-bit data, we have to fix it for all
> 32-bit architectures, and we're better off avoiding special
> cases.

Ok, we will rewrite these patches using 32bit time_t and 32bit off_t and redo 
the toolchain support for them.  Note this is going back to the abi I had 
originally done when I submitted my original version when it was asked to 
change time_t to be 64bit. 

Thanks,
Andrew


> 
>Arnd
--
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: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 3, 2015, at 1:12 AM, Catalin Marinas  wrote:
> 
>> On Wed, Sep 02, 2015 at 10:52:05PM +0800, Andrew Pinski wrote:
>> On Wed, Sep 2, 2015 at 9:57 PM, Siarhei Siamashka 
>>  wrote:
> [...]
> On Wed, 2 Sep 2015 01:58:56 +0800 pins...@gmail.com wrote:
>> Yes but I guess you talk about caching the value in userspace but doing
>> it via the aux vector is the same as your suggestion. Just one
>> difference is you don't get the aux vector entry if there is a CPU
>> that is online which is different. No difference from your suggestion
>> of caching it. Without considering hot pug for a second (that is a
>> huge different issue all together), if userland wants to know if all
>> up CPUs have the same midr, they would either read /sys entries (lots
>> of syscalls) or bind to each CPU and do the trap. That means at least
>> three or two syscalls/traps for each CPU. My way is none and gets a
>> value of midr if they are all the Same for free.
> 
> Whether we like it or not, big.LITTLE systems are present in the ARM
> ecosystem. You are looking to add a specific AT_ to solve a particular
> problem on fully symmetric systems, ignoring the rest. I want this fixed
> for all systems rather than trying to invent something else for
> big.LITTLE which won't help user space at all.
> 
> If you want to avoid parsing all /sys entries, I would rather have a
> HWCAP_ASYMMETRIC bit for big.LITTLE systems and let user space decide
> whether to read all entries or not.
> 
>>> I wonder if we still can try to make "sched_getcpu()" use vDSO
>>> instead of the syscall to make it faster? Now that there exists a
>>> vDSO implementation for gettimeofday(), everything should be easy if
>>> we can find an unused userspace readable coprocessor register.
>>> In the past, Catalin Marinas mentioned that "We have a user read-only
>>> thread register unused on arm64":
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/220664.html
> 
> We have a patch under development internally, it will appear on the list
> at some point.
> 
>>> And if I understand it correctly, also one of the "scratch registers"
>>> should exist in 32-bit ARM, which "isn't present in ARMv8/AArch64":
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/221056.html
>>> What kind of registers are these exactly?
>>> 
>>> In principle, the aux vector can be extended to also contain a pointer
>>> to an array of MIDR values for all the CPU cores if reducing the setup
>>> overhead is critical.
>> 
>> That is not a bad idea.  Put this array in the data section of the
>> VDSO too.  It should be small enough though on systems with 96 or more
>> cores (dual socket ThunderX has 96 cores total), it is slightly
>> getting big.
>> The struct would be something like:
>> struct
>> {
>>  int32 numcores;
>>  int32 midr[];
>> };
> 
> First of all, I'm against hard-coding (VDSO) data as ABI. So far we used
> VDSO to override some weak glibc functions but the VDSO-specific data is
> parsed by the VDSO function implementation and not directly by glibc (or
> user space). I prefer helper functions that read the VDSO-internal data
> structures.

You don't like the idea of a fixed structure ABI that resides inside vdso data? 
Having a fixed struct ABI should be ok.  The location inside the data part was 
going to be passed via an aux vector entry.  Userland does even need to know it 
is really located in the vdso at all. It just happens to reside in there. The 
data structure would be well defined for the aux vector. 

> 
> Secondly, you seem to be only interested in MIDR_EL1 but we also have
> REVIDR_EL1 and AIDR_EL1 which may be relevant. Once we realise that more
> information is needed, it's not always clear where the boundaries are
> so I would rather have this exposed via /sys and/or MRS emulation (there
> are patches for both).
> 
> Anyway, you need to involve the toolchain people in such discussions,
> they may have different needs (like ifunc).

I am a toolchain person first and needed this in the first place for memset and 
memcpy on thunderx. I had asked our kernel engineers here first before even 
going forward with my original approach. I touch the kernel only because I need 
to really. I even had asked on #linaro-kernel and #linaro-toolchain around 
two-three months ago about this approach and I only had time to post it this 
week. 

Thanks,
Andrew


> 
> -- 
> Catalin
--
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: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 3, 2015, at 1:12 AM, Catalin Marinas  wrote:
> 
>> On Wed, Sep 02, 2015 at 10:52:05PM +0800, Andrew Pinski wrote:
>> On Wed, Sep 2, 2015 at 9:57 PM, Siarhei Siamashka 
>>  wrote:
> [...]
> On Wed, 2 Sep 2015 01:58:56 +0800 pins...@gmail.com wrote:
>> Yes but I guess you talk about caching the value in userspace but doing
>> it via the aux vector is the same as your suggestion. Just one
>> difference is you don't get the aux vector entry if there is a CPU
>> that is online which is different. No difference from your suggestion
>> of caching it. Without considering hot pug for a second (that is a
>> huge different issue all together), if userland wants to know if all
>> up CPUs have the same midr, they would either read /sys entries (lots
>> of syscalls) or bind to each CPU and do the trap. That means at least
>> three or two syscalls/traps for each CPU. My way is none and gets a
>> value of midr if they are all the Same for free.
> 
> Whether we like it or not, big.LITTLE systems are present in the ARM
> ecosystem. You are looking to add a specific AT_ to solve a particular
> problem on fully symmetric systems, ignoring the rest. I want this fixed
> for all systems rather than trying to invent something else for
> big.LITTLE which won't help user space at all.
> 
> If you want to avoid parsing all /sys entries, I would rather have a
> HWCAP_ASYMMETRIC bit for big.LITTLE systems and let user space decide
> whether to read all entries or not.
> 
>>> I wonder if we still can try to make "sched_getcpu()" use vDSO
>>> instead of the syscall to make it faster? Now that there exists a
>>> vDSO implementation for gettimeofday(), everything should be easy if
>>> we can find an unused userspace readable coprocessor register.
>>> In the past, Catalin Marinas mentioned that "We have a user read-only
>>> thread register unused on arm64":
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/220664.html
> 
> We have a patch under development internally, it will appear on the list
> at some point.
> 
>>> And if I understand it correctly, also one of the "scratch registers"
>>> should exist in 32-bit ARM, which "isn't present in ARMv8/AArch64":
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/221056.html
>>> What kind of registers are these exactly?
>>> 
>>> In principle, the aux vector can be extended to also contain a pointer
>>> to an array of MIDR values for all the CPU cores if reducing the setup
>>> overhead is critical.
>> 
>> That is not a bad idea.  Put this array in the data section of the
>> VDSO too.  It should be small enough though on systems with 96 or more
>> cores (dual socket ThunderX has 96 cores total), it is slightly
>> getting big.
>> The struct would be something like:
>> struct
>> {
>>  int32 numcores;
>>  int32 midr[];
>> };
> 
> First of all, I'm against hard-coding (VDSO) data as ABI. So far we used
> VDSO to override some weak glibc functions but the VDSO-specific data is
> parsed by the VDSO function implementation and not directly by glibc (or
> user space). I prefer helper functions that read the VDSO-internal data
> structures.

You don't like the idea of a fixed structure ABI that resides inside vdso data? 
Having a fixed struct ABI should be ok.  The location inside the data part was 
going to be passed via an aux vector entry.  Userland does even need to know it 
is really located in the vdso at all. It just happens to reside in there. The 
data structure would be well defined for the aux vector. 

> 
> Secondly, you seem to be only interested in MIDR_EL1 but we also have
> REVIDR_EL1 and AIDR_EL1 which may be relevant. Once we realise that more
> information is needed, it's not always clear where the boundaries are
> so I would rather have this exposed via /sys and/or MRS emulation (there
> are patches for both).
> 
> Anyway, you need to involve the toolchain people in such discussions,
> they may have different needs (like ifunc).

I am a toolchain person first and needed this in the first place for memset and 
memcpy on thunderx. I had asked our kernel engineers here first before even 
going forward with my original approach. I touch the kernel only because I need 
to really. I even had asked on #linaro-kernel and #linaro-toolchain around 
two-three months ago about this approach and I only had time to post it this 
week. 

Thanks,
Andrew


> 
> -- 
> Catalin
--
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: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 3:13 AM, Siarhei Siamashka  
> wrote:
> 
> On Wed, 2 Sep 2015 01:58:56 +0800
> pins...@gmail.com wrote:
> 
>>> On Sep 2, 2015, at 1:30 AM, Mark Rutland  wrote:
>>> 
>>> [...]
>>> 
>>> On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
>>> It is useful to pass down MIDR register down to userland if all of
>>> the online cores are all the same type.  This adds AT_ARM64_MIDR
>>> aux vector type and passes down the midr system register.
>>> 
>>> This is alternative to MIDR_EL1 part of
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
>>> It allows for faster access to midr_el1 than going through a trap and
>>> does not exist if the set of cores are not the same.
>> 
>> I'm not sure I follow the rationale. If speed is important the
>> application can cache the value the first time it reads it with a trap.
> 
> It is also about compatibility also. Exposing the register is not 
> backwards compatible but using the aux vector is.
 
 That would also break big.little too. So either break it with hot plug or 
 break it in userland, your choice.
>>> 
>>> The value wouldn't be representative of the system as a whole; that is
>>> true. However, we never guaranteed that it was, while the aux vector
>>> code implied that we did.
>> 
>> Yes but I guess you talk about caching the value in userspace but doing
>> it via the aux vector is the same as your suggestion. Just one
>> difference is you don't get the aux vector entry if there is a CPU
>> that is online which is different. No difference from your suggestion
>> of caching it. Without considering hot pug for a second (that is a
>> huge different issue all together), if userland wants to know if all
>> up CPUs have the same midr, they would either read /sys entries (lots
>> of syscalls) or bind to each CPU and do the trap. That means at least
>> three or two syscalls/traps for each CPU. My way is none and gets a
>> value of midr if they are all the Same for free. 
> 
> Andrew, how do you propose to get the value of MIDR? Open the
> "/proc/self/auxv", read it, do a linear search in the buffer to find
> the required entry and then read the value? Or use the glibc specific
> getauxval() function (https://lwn.net/Articles/519085) ?

This is inside glibc I am talking about so getauxval. 

> 
> Regarding the caching implementation, one can open and parse the
> "/proc/cpuinfo" file (with older kernels) or read the new sysfs
> entries to get the MIDR value for each core. Then create a lookup
> table. As an additional bonus, this lookup table can contain not
> just the MIDR values, but any arbitrary data in any format (for
> example, a function pointer to the memcpy function or anything else).

You don't want to do that early on in ld.so each time a program starts up. Too 
much overhead. 

> 
> After the lookup table is available, one can use the getcpu() syscall
> for getting the CPU number and do the table lookup. And for getting
> reasonable performance, implement the vdso variant of the getcpu()
> syscall.
> 
> All of this internal ugliness would be best abstracted inside
> of the GCC __builtin_cpu_init(), __builtin_cpu_is() and
> __builtin_cpu_supports() builtins:
>http://gcc.gnu.org/gcc-4.8/changes.html

Yes but this is about glibc support and not other userland support. Having 
glibc depend on that is even worse. 

Thanks,
Andrew


> 
> One big.LITTLE systems, the __builtin_cpu_is() could be implemented
> via a single getcpu() syscall and the table lookup, like explained
> above. The __builtin_cpu_init() could prepare the lookup table.
> And on normal systems with identical cores, the use of the syscall
> is not required.
> 
> It might be interesting to also optionally allow something like this:
>__builtin_cpu_is("cortex-a7 || cortex-a15")
> Which would mean that we are interested in checking for the
> Cortex-A7+Cortex-A15 pair in a big.LITTLE system, but are not
> interested in knowing whether we are running on A7 or A15 in this
> particular moment (and avoid the syscall overhead).
> 
> We had an old discussion on a similar CPU type identification topic
> in the past:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/220542.html
> I have been told that it had been forwarded to the Linaro toolchain
> people, but did not track if this resulted in anything useful or not.
> 
> I think that it would be best to prefer something that is easily usable
> for all applications and libraries, and not just something for a private
> use by glibc. To sum everything up:
> 
> One the kernel side it means:
>  1. Maybe implement vdso for getcpu(), this will make things faster
> on big.LITTLE systems.
>  2. Maybe implement sysfs entries for per-core MIDR values from
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359127.html
> This will make things faster and allow to avoid potentially

Re: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 1:19 AM, Mark Rutland  wrote:
> 
>> On Tue, Sep 01, 2015 at 05:51:44PM +0100, pins...@gmail.com wrote:
>> 
>> 
>> 
>> 
>>> On Sep 2, 2015, at 12:33 AM, Mark Rutland  wrote:
>>> 
>>> Hi,
>>> 
 On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
 It is useful to pass down MIDR register down to userland if all of
 the online cores are all the same type.  This adds AT_ARM64_MIDR
 aux vector type and passes down the midr system register.
 
 This is alternative to MIDR_EL1 part of
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
 It allows for faster access to midr_el1 than going through a trap and
 does not exist if the set of cores are not the same.
>>> 
>>> I'm not sure I follow the rationale. If speed is important the
>>> application can cache the value the first time it reads it with a trap.
>> 
>> It is also about compatibility also. Exposing the register is not
>> backwards compatible but using the aux vector is. 
> 
> So long as we have HWCAP_CPUID describing the availability of register
> access [2], then userspace can test for that before attempting to access
> the MIDR.

So two checks always. Bad choice. 

> 
> Other than that, I don't see a backwards or forwards compatibility
> issue.
> 
 +u32 get_arm64_midr(void)
 +{
 +int i;
 +u32 midr = 0;
 +
 +for_each_online_cpu(i) {
 +struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
 +u32 oldmidr = midr;
 +
 +midr = cpuinfo->reg_midr;
 +/*
 + * If there are cpus which have a different
 + * midr just return 0.
 + */
 +if (oldmidr && oldmidr != midr)
 +return 0;
 +}
 +
 +return midr;
 +}
>>> 
>>> If I have a big.LITTLE system where all the big CPUs are currently
>>> offline, this will leave the MIDR the little CPUs in the auxvec.
>>> However, at any point after this has run, I could hotplug the big CPUs
>>> on and the little CPUs off, leaving this reporting a MIDR that
>>> represents none of the online CPUs.
>>> 
>>> Given big.LITTLE and the potential for physical/dynamic hotplug (where
>>> we won't know all the MIDRs in advance), I don't think that we can
>>> generally expose a common MIDR in this fashion, and I don't think that
>>> we should give the impression that we can.
>> 
>> This is standard issue with hot plug and big.little. Really big.little
>> is a design flaw but I am not going into that here. 
> 
> Regardless of our personal feelings on big.LITTLE, it's something we
> have to deal with.

You did not respond to the caching in userspace issue. You raised that as a 
speed optimization but then considered my patch as a non starter. 

> 
> Hopefully it's a non-issue anyway; a MIDR provided by this interface can
> really only be used to derive optimisation criteria rather than
> non-architected properties required for correctness.

Like hardware workarounds? Yes that is going to be used for that and is already 
right now by reading /proc/cpuinfo . Also it would useful to use what ever 
interface for gcc's -mcpu=native option. 


> 
>>> I think that the only things we can do are expose the MIDR for CPU the
>>> code is currently executing on (as Suzuki's patches do), and/or expose
>>> all the MIDRs for currently online CPUs (as Steve's [1] patch does).
>>> Anything else leaves us trying to provide semantics that we cannot
>>> guarantee.
>> 
>> Except they are not backwards compatible which means nobody in their
>> right mind would use the register to get the midr that way.
> 
> I assume you missed the discussion of HWCAP_CPUID, which prevents the
> compatibility issue I believe you're considering here.

And you suggested to cache midr while not considering big.little. You can't 
have it both ways. I read those and I still think they were wrong in rejecting 
this.  Also there are two markets arm is in and things like this is causing one 
of those markets to suffer. Big.little is not going into servers. 

> 
>> I am sorry but having a newer version of glibc working on a year old
>> kernel is not going to fly. 
> 
> I'm not sure I follow this, unless you meant _not_ working.

Because of the double checks, it will be slower and the trap. And gives a bad 
interface to userland really.  Aux vector is a much cleaner interface to 
userland than a trapped instruction. 

Thanks,
Andrew

> 
> Thanks,
> Mark.
> 
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359127.html
> [2] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/363559.html
--
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: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 12:51 AM, "pins...@gmail.com"  wrote:
> 
> 
> 
> 
> 
>> On Sep 2, 2015, at 12:33 AM, Mark Rutland  wrote:
>> 
>> Hi,
>> 
>>> On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
>>> It is useful to pass down MIDR register down to userland if all of
>>> the online cores are all the same type.  This adds AT_ARM64_MIDR
>>> aux vector type and passes down the midr system register.
>>> 
>>> This is alternative to MIDR_EL1 part of
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
>>> It allows for faster access to midr_el1 than going through a trap and
>>> does not exist if the set of cores are not the same.
>> 
>> I'm not sure I follow the rationale. If speed is important the
>> application can cache the value the first time it reads it with a trap.
> 
> It is also about compatibility also. Exposing the register is not backwards 
> compatible but using the aux vector is. 

That would also break big.little too. So either break it with hot plug or break 
it in userland, your choice. 


> 
>> 
>> This also means that the behaviour is different across homogeneous and
>> heterogeneous systems.

That should be ok because it is still backwards compatible with what was done 
before.  My goal here is just to allow quick easy access to midr in the case of 
a homogeneous system which I care about, thunderx and to allow glibc to select 
a memcpy/memset that is better for thunderx. 

Thanks,
Andrew

>> 
>>> Changes from v1:
>>> Forgot to include the auxvec.h part.
>>> 
>>> Signed-off-by: Andrew Pinski 
>>> ---
>>> arch/arm64/include/asm/cpu.h |1 +
>>> arch/arm64/include/asm/elf.h |6 ++
>>> arch/arm64/include/uapi/asm/auxvec.h |3 +++
>>> arch/arm64/kernel/cpuinfo.c  |   22 ++
>>> 4 files changed, 32 insertions(+)
>>> 
>>> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
>>> index 8e797b2..fab0aa1 100644
>>> --- a/arch/arm64/include/asm/cpu.h
>>> +++ b/arch/arm64/include/asm/cpu.h
>>> @@ -62,5 +62,6 @@ DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);
>>> 
>>> void cpuinfo_store_cpu(void);
>>> void __init cpuinfo_store_boot_cpu(void);
>>> +u32 get_arm64_midr(void);
>>> 
>>> #endif /* __ASM_CPU_H */
>>> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>>> index faad6df..d3549de 100644
>>> --- a/arch/arm64/include/asm/elf.h
>>> +++ b/arch/arm64/include/asm/elf.h
>>> @@ -17,6 +17,7 @@
>>> #define __ASM_ELF_H
>>> 
>>> #include 
>>> +#include 
>>> 
>>> /*
>>> * ELF register definitions..
>>> @@ -138,8 +139,13 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>>> 
>>> #define ARCH_DLINFO\
>>> do {\
>>> +u32 midr;\
>>> +\
>>>   NEW_AUX_ENT(AT_SYSINFO_EHDR,\
>>>   (elf_addr_t)current->mm->context.vdso);\
>>> +midr = get_arm64_midr();\
>>> +if (midr != 0)\
>>> +NEW_AUX_ENT(AT_ARM64_MIDR, (elf_addr_t)midr);\
>>> } while (0)
>>> 
>>> #define ARCH_HAS_SETUP_ADDITIONAL_PAGES
>>> diff --git a/arch/arm64/include/uapi/asm/auxvec.h 
>>> b/arch/arm64/include/uapi/asm/auxvec.h
>>> index 22d6d88..dc55c56 100644
>>> --- a/arch/arm64/include/uapi/asm/auxvec.h
>>> +++ b/arch/arm64/include/uapi/asm/auxvec.h
>>> @@ -19,4 +19,7 @@
>>> /* vDSO location */
>>> #define AT_SYSINFO_EHDR33
>>> 
>>> +/* Machine IDenfier Register (MDIR). */
>>> +#define AT_ARM64_MIDR 38
>>> +
>>> #endif
>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
>>> index 75d5a86..b14c87d 100644
>>> --- a/arch/arm64/kernel/cpuinfo.c
>>> +++ b/arch/arm64/kernel/cpuinfo.c
>>> @@ -254,3 +254,25 @@ void __init cpuinfo_store_boot_cpu(void)
>>> 
>>>   boot_cpu_data = *info;
>>> }
>>> +
>>> +u32 get_arm64_midr(void)
>>> +{
>>> +int i;
>>> +u32 midr = 0;
>>> +
>>> +for_each_online_cpu(i) {
>>> +struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
>>> +u32 oldmidr = midr;
>>> +
>>> +midr = cpuinfo->reg_midr;
>>> +/*
>>> + * If there are cpus which have a different
>>> + * midr just return 0.
>>> + */
>>> +if (oldmidr && oldmidr != midr)
>>> +return 0;
>>> +}
>>> +
>>> +return midr;
>>> +}
>> 
>> If I have a big.LITTLE system where all the big CPUs are currently
>> offline, this will leave the MIDR the little CPUs in the auxvec.
>> However, at any point after this has run, I could hotplug the big CPUs
>> on and the little CPUs off, leaving this reporting a MIDR that
>> represents none of the online CPUs.
>> 
>> Given big.LITTLE and the potential for physical/dynamic hotplug (where
>> we won't know all the MIDRs in advance), I don't think that we can
>> generally expose a common MIDR in this fashion, and I don't think that
>> we should give the impression that we can.

Re: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 12:51 AM, "pins...@gmail.com"  wrote:
> 
> 
> 
> 
> 
>> On Sep 2, 2015, at 12:33 AM, Mark Rutland  wrote:
>> 
>> Hi,
>> 
>>> On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
>>> It is useful to pass down MIDR register down to userland if all of
>>> the online cores are all the same type.  This adds AT_ARM64_MIDR
>>> aux vector type and passes down the midr system register.
>>> 
>>> This is alternative to MIDR_EL1 part of
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
>>> It allows for faster access to midr_el1 than going through a trap and
>>> does not exist if the set of cores are not the same.
>> 
>> I'm not sure I follow the rationale. If speed is important the
>> application can cache the value the first time it reads it with a trap.
> 
> It is also about compatibility also. Exposing the register is not backwards 
> compatible but using the aux vector is. 

That would also break big.little too. So either break it with hot plug or break 
it in userland, your choice. 


> 
>> 
>> This also means that the behaviour is different across homogeneous and
>> heterogeneous systems.

That should be ok because it is still backwards compatible with what was done 
before.  My goal here is just to allow quick easy access to midr in the case of 
a homogeneous system which I care about, thunderx and to allow glibc to select 
a memcpy/memset that is better for thunderx. 

Thanks,
Andrew

>> 
>>> Changes from v1:
>>> Forgot to include the auxvec.h part.
>>> 
>>> Signed-off-by: Andrew Pinski 
>>> ---
>>> arch/arm64/include/asm/cpu.h |1 +
>>> arch/arm64/include/asm/elf.h |6 ++
>>> arch/arm64/include/uapi/asm/auxvec.h |3 +++
>>> arch/arm64/kernel/cpuinfo.c  |   22 ++
>>> 4 files changed, 32 insertions(+)
>>> 
>>> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
>>> index 8e797b2..fab0aa1 100644
>>> --- a/arch/arm64/include/asm/cpu.h
>>> +++ b/arch/arm64/include/asm/cpu.h
>>> @@ -62,5 +62,6 @@ DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);
>>> 
>>> void cpuinfo_store_cpu(void);
>>> void __init cpuinfo_store_boot_cpu(void);
>>> +u32 get_arm64_midr(void);
>>> 
>>> #endif /* __ASM_CPU_H */
>>> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
>>> index faad6df..d3549de 100644
>>> --- a/arch/arm64/include/asm/elf.h
>>> +++ b/arch/arm64/include/asm/elf.h
>>> @@ -17,6 +17,7 @@
>>> #define __ASM_ELF_H
>>> 
>>> #include 
>>> +#include 
>>> 
>>> /*
>>> * ELF register definitions..
>>> @@ -138,8 +139,13 @@ typedef struct user_fpsimd_state elf_fpregset_t;
>>> 
>>> #define ARCH_DLINFO\
>>> do {\
>>> +u32 midr;\
>>> +\
>>>   NEW_AUX_ENT(AT_SYSINFO_EHDR,\
>>>   (elf_addr_t)current->mm->context.vdso);\
>>> +midr = get_arm64_midr();\
>>> +if (midr != 0)\
>>> +NEW_AUX_ENT(AT_ARM64_MIDR, (elf_addr_t)midr);\
>>> } while (0)
>>> 
>>> #define ARCH_HAS_SETUP_ADDITIONAL_PAGES
>>> diff --git a/arch/arm64/include/uapi/asm/auxvec.h 
>>> b/arch/arm64/include/uapi/asm/auxvec.h
>>> index 22d6d88..dc55c56 100644
>>> --- a/arch/arm64/include/uapi/asm/auxvec.h
>>> +++ b/arch/arm64/include/uapi/asm/auxvec.h
>>> @@ -19,4 +19,7 @@
>>> /* vDSO location */
>>> #define AT_SYSINFO_EHDR33
>>> 
>>> +/* Machine IDenfier Register (MDIR). */
>>> +#define AT_ARM64_MIDR 38
>>> +
>>> #endif
>>> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
>>> index 75d5a86..b14c87d 100644
>>> --- a/arch/arm64/kernel/cpuinfo.c
>>> +++ b/arch/arm64/kernel/cpuinfo.c
>>> @@ -254,3 +254,25 @@ void __init cpuinfo_store_boot_cpu(void)
>>> 
>>>   boot_cpu_data = *info;
>>> }
>>> +
>>> +u32 get_arm64_midr(void)
>>> +{
>>> +int i;
>>> +u32 midr = 0;
>>> +
>>> +for_each_online_cpu(i) {
>>> +struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
>>> +u32 oldmidr = midr;
>>> +
>>> +midr = cpuinfo->reg_midr;
>>> +/*
>>> + * If there are cpus which have a different
>>> + * midr just return 0.
>>> + */
>>> +if (oldmidr && oldmidr != midr)
>>> +return 0;
>>> +}
>>> +
>>> +return midr;
>>> +}
>> 
>> If I have a big.LITTLE system where all the big CPUs are currently
>> offline, this will leave the MIDR the little CPUs in the auxvec.
>> However, at any point after this has run, I could hotplug the big CPUs
>> on and the little CPUs off, leaving this reporting a MIDR that
>> represents none of the online CPUs.
>> 
>> Given big.LITTLE and the potential for physical/dynamic hotplug (where
>> we won't know all the MIDRs in advance), I don't think that we can
>> generally expose a common MIDR in this fashion, and I 

Re: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 1:19 AM, Mark Rutland  wrote:
> 
>> On Tue, Sep 01, 2015 at 05:51:44PM +0100, pins...@gmail.com wrote:
>> 
>> 
>> 
>> 
>>> On Sep 2, 2015, at 12:33 AM, Mark Rutland  wrote:
>>> 
>>> Hi,
>>> 
 On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
 It is useful to pass down MIDR register down to userland if all of
 the online cores are all the same type.  This adds AT_ARM64_MIDR
 aux vector type and passes down the midr system register.
 
 This is alternative to MIDR_EL1 part of
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
 It allows for faster access to midr_el1 than going through a trap and
 does not exist if the set of cores are not the same.
>>> 
>>> I'm not sure I follow the rationale. If speed is important the
>>> application can cache the value the first time it reads it with a trap.
>> 
>> It is also about compatibility also. Exposing the register is not
>> backwards compatible but using the aux vector is. 
> 
> So long as we have HWCAP_CPUID describing the availability of register
> access [2], then userspace can test for that before attempting to access
> the MIDR.

So two checks always. Bad choice. 

> 
> Other than that, I don't see a backwards or forwards compatibility
> issue.
> 
 +u32 get_arm64_midr(void)
 +{
 +int i;
 +u32 midr = 0;
 +
 +for_each_online_cpu(i) {
 +struct cpuinfo_arm64 *cpuinfo = _cpu(cpu_data, i);
 +u32 oldmidr = midr;
 +
 +midr = cpuinfo->reg_midr;
 +/*
 + * If there are cpus which have a different
 + * midr just return 0.
 + */
 +if (oldmidr && oldmidr != midr)
 +return 0;
 +}
 +
 +return midr;
 +}
>>> 
>>> If I have a big.LITTLE system where all the big CPUs are currently
>>> offline, this will leave the MIDR the little CPUs in the auxvec.
>>> However, at any point after this has run, I could hotplug the big CPUs
>>> on and the little CPUs off, leaving this reporting a MIDR that
>>> represents none of the online CPUs.
>>> 
>>> Given big.LITTLE and the potential for physical/dynamic hotplug (where
>>> we won't know all the MIDRs in advance), I don't think that we can
>>> generally expose a common MIDR in this fashion, and I don't think that
>>> we should give the impression that we can.
>> 
>> This is standard issue with hot plug and big.little. Really big.little
>> is a design flaw but I am not going into that here. 
> 
> Regardless of our personal feelings on big.LITTLE, it's something we
> have to deal with.

You did not respond to the caching in userspace issue. You raised that as a 
speed optimization but then considered my patch as a non starter. 

> 
> Hopefully it's a non-issue anyway; a MIDR provided by this interface can
> really only be used to derive optimisation criteria rather than
> non-architected properties required for correctness.

Like hardware workarounds? Yes that is going to be used for that and is already 
right now by reading /proc/cpuinfo . Also it would useful to use what ever 
interface for gcc's -mcpu=native option. 


> 
>>> I think that the only things we can do are expose the MIDR for CPU the
>>> code is currently executing on (as Suzuki's patches do), and/or expose
>>> all the MIDRs for currently online CPUs (as Steve's [1] patch does).
>>> Anything else leaves us trying to provide semantics that we cannot
>>> guarantee.
>> 
>> Except they are not backwards compatible which means nobody in their
>> right mind would use the register to get the midr that way.
> 
> I assume you missed the discussion of HWCAP_CPUID, which prevents the
> compatibility issue I believe you're considering here.

And you suggested to cache midr while not considering big.little. You can't 
have it both ways. I read those and I still think they were wrong in rejecting 
this.  Also there are two markets arm is in and things like this is causing one 
of those markets to suffer. Big.little is not going into servers. 

> 
>> I am sorry but having a newer version of glibc working on a year old
>> kernel is not going to fly. 
> 
> I'm not sure I follow this, unless you meant _not_ working.

Because of the double checks, it will be slower and the trap. And gives a bad 
interface to userland really.  Aux vector is a much cleaner interface to 
userland than a trapped instruction. 

Thanks,
Andrew

> 
> Thanks,
> Mark.
> 
> [1] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359127.html
> [2] 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/363559.html
--
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: [PATCHv2] ARM64: Add AT_ARM64_MIDR to the aux vector

[Pinski, Andrew]

> On Sep 2, 2015, at 3:13 AM, Siarhei Siamashka  
> wrote:
> 
> On Wed, 2 Sep 2015 01:58:56 +0800
> pins...@gmail.com wrote:
> 
>>> On Sep 2, 2015, at 1:30 AM, Mark Rutland  wrote:
>>> 
>>> [...]
>>> 
>>> On Sat, Aug 29, 2015 at 07:46:22PM +0100, Andrew Pinski wrote:
>>> It is useful to pass down MIDR register down to userland if all of
>>> the online cores are all the same type.  This adds AT_ARM64_MIDR
>>> aux vector type and passes down the midr system register.
>>> 
>>> This is alternative to MIDR_EL1 part of
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/358995.html.
>>> It allows for faster access to midr_el1 than going through a trap and
>>> does not exist if the set of cores are not the same.
>> 
>> I'm not sure I follow the rationale. If speed is important the
>> application can cache the value the first time it reads it with a trap.
> 
> It is also about compatibility also. Exposing the register is not 
> backwards compatible but using the aux vector is.
 
 That would also break big.little too. So either break it with hot plug or 
 break it in userland, your choice.
>>> 
>>> The value wouldn't be representative of the system as a whole; that is
>>> true. However, we never guaranteed that it was, while the aux vector
>>> code implied that we did.
>> 
>> Yes but I guess you talk about caching the value in userspace but doing
>> it via the aux vector is the same as your suggestion. Just one
>> difference is you don't get the aux vector entry if there is a CPU
>> that is online which is different. No difference from your suggestion
>> of caching it. Without considering hot pug for a second (that is a
>> huge different issue all together), if userland wants to know if all
>> up CPUs have the same midr, they would either read /sys entries (lots
>> of syscalls) or bind to each CPU and do the trap. That means at least
>> three or two syscalls/traps for each CPU. My way is none and gets a
>> value of midr if they are all the Same for free. 
> 
> Andrew, how do you propose to get the value of MIDR? Open the
> "/proc/self/auxv", read it, do a linear search in the buffer to find
> the required entry and then read the value? Or use the glibc specific
> getauxval() function (https://lwn.net/Articles/519085) ?

This is inside glibc I am talking about so getauxval. 

> 
> Regarding the caching implementation, one can open and parse the
> "/proc/cpuinfo" file (with older kernels) or read the new sysfs
> entries to get the MIDR value for each core. Then create a lookup
> table. As an additional bonus, this lookup table can contain not
> just the MIDR values, but any arbitrary data in any format (for
> example, a function pointer to the memcpy function or anything else).

You don't want to do that early on in ld.so each time a program starts up. Too 
much overhead. 

> 
> After the lookup table is available, one can use the getcpu() syscall
> for getting the CPU number and do the table lookup. And for getting
> reasonable performance, implement the vdso variant of the getcpu()
> syscall.
> 
> All of this internal ugliness would be best abstracted inside
> of the GCC __builtin_cpu_init(), __builtin_cpu_is() and
> __builtin_cpu_supports() builtins:
>http://gcc.gnu.org/gcc-4.8/changes.html

Yes but this is about glibc support and not other userland support. Having 
glibc depend on that is even worse. 

Thanks,
Andrew


> 
> One big.LITTLE systems, the __builtin_cpu_is() could be implemented
> via a single getcpu() syscall and the table lookup, like explained
> above. The __builtin_cpu_init() could prepare the lookup table.
> And on normal systems with identical cores, the use of the syscall
> is not required.
> 
> It might be interesting to also optionally allow something like this:
>__builtin_cpu_is("cortex-a7 || cortex-a15")
> Which would mean that we are interested in checking for the
> Cortex-A7+Cortex-A15 pair in a big.LITTLE system, but are not
> interested in knowing whether we are running on A7 or A15 in this
> particular moment (and avoid the syscall overhead).
> 
> We had an old discussion on a similar CPU type identification topic
> in the past:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/220542.html
> I have been told that it had been forwarded to the Linaro toolchain
> people, but did not track if this resulted in anything useful or not.
> 
> I think that it would be best to prefer something that is easily usable
> for all applications and libraries, and not just something for a private
> use by glibc. To sum everything up:
> 
> One the kernel side it means:
>  1. Maybe implement vdso for getcpu(), this will make things faster
> on big.LITTLE systems.
>  2. Maybe implement sysfs entries for per-core MIDR values from
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359127.html
> This 

Re: [PATCH v4 22/24] arm64:ilp32: use compat for stack_t

[Pinski, Andrew]

> On Apr 13, 2015, at 1:18 PM, Philipp Tomsich 
>  wrote:
> 
> We use a 'natively sized' stack_t in glibc (i.e. having a 32bit pointer for
> ss_sp), which requires the invocation of the compat layer for the following
> functionality:
> * sigaltstack
> * saving and restoring uc_stack during signal setup and returns

Can you explain why you want to use a natively sized stack_t?  My patches for 
glibc included changing stack_t too.  I would rather keep the same size stack_t 
between lp64 and ilp32 due easier gdb support. 

Thanks,
Andrew


> 
> As the userspace stack_t is natively sized, we avoid code duplication in the
> syscall table and can use the compat-functions to zero-extend the pointers
> involved.
> 
> Signed-off-by: Philipp Tomsich 
> Signed-off-by: Christoph Muellner 
> ---
> arch/arm64/kernel/signal.c| 19 +++
> arch/arm64/kernel/sys_ilp32.c | 44 +--
> 2 files changed, 20 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 99e36be..b3f6e52 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -34,6 +34,7 @@
> #include 
> #include 
> #include 
> +#include 
> 
> /*
>  * Do a signal return; undo the signal stack. These are aligned to 128-bit.
> @@ -148,9 +149,22 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
>if (restore_sigframe(regs, frame))
>goto badframe;
> 
> +
> +#if defined(CONFIG_ARM64_ILP32)
> +if (is_ilp32_compat_task()) {
> +/* For ILP32, we have a different stack_t (the ss_sp
> +   field will be only 32bit sized), which fits into
> +   the memory area reserved for the (larger) LP64
> +   stack_t and which we place into uc_stack: this
> +   implies padding after the ILP32 stack_t. */
> +if 
> (compat_restore_altstack((compat_stack_t*)>uc.uc_stack))
> +goto badframe;
> +} else
> +#endif
>if (restore_altstack(>uc.uc_stack))
>goto badframe;
> 
> +
>return regs->regs[0];
> 
> badframe:
> @@ -264,6 +278,11 @@ static int setup_rt_frame(int usig, struct ksignal 
> *ksig, sigset_t *set,
>__put_user_error(0, >uc.uc_flags, err);
>__put_user_error(NULL, >uc.uc_link, err);
> 
> +#if defined(CONFIG_ARM64_ILP32)
> +if (is_ilp32_compat_task())
> +err |= __compat_save_altstack((compat_stack_t*)>uc.uc_stack, 
> regs->sp);
> +else
> +#endif
>err |= __save_altstack(>uc.uc_stack, regs->sp);
>err |= setup_sigframe(frame, regs, set);
>if (err == 0) {
> diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c
> index 3471f27..31f82ca 100644
> --- a/arch/arm64/kernel/sys_ilp32.c
> +++ b/arch/arm64/kernel/sys_ilp32.c
> @@ -77,6 +77,7 @@
> 
> /* Pointer in struct */
> #define sys_mount   compat_sys_mount
> +#define sys_sigaltstack compat_sys_sigaltstack
> 
> /* NUMA */
> /* unsigned long bitmaps */
> @@ -122,49 +123,6 @@ asmlinkage long ilp32_sys_mq_notify(mqd_t mqdes, const 
> struct sigevent __user *u
>but need special handling due to padding for SIGEV_THREAD.  */
> #define sys_mq_notifyilp32_sys_mq_notify
> 
> -
> -/* sigaltstack needs some special handling as the
> -   padding for stack_t might not be non-zero. */
> -long ilp32_sys_sigaltstack(const stack_t __user *uss_ptr,
> -   stack_t __user *uoss_ptr)
> -{
> -stack_t uss, uoss;
> -int ret;
> -mm_segment_t seg;
> -
> -if (uss_ptr) {
> -if (!access_ok(VERIFY_READ, uss_ptr, sizeof(*uss_ptr)))
> -return -EFAULT;
> -if (__get_user(uss.ss_sp, _ptr->ss_sp) |
> -__get_user(uss.ss_flags, _ptr->ss_flags) |
> -__get_user(uss.ss_size, _ptr->ss_size))
> -return -EFAULT;
> -/* Zero extend the sp address and the size. */
> -uss.ss_sp = (void *)(uintptr_t)(unsigned int)(uintptr_t)uss.ss_sp;
> -uss.ss_size = (size_t)(unsigned int)uss.ss_size;
> -}
> -seg = get_fs();
> -set_fs(KERNEL_DS);
> -/* Note we need to use uoss as we have changed the segment to the
> -   kernel one so passing an user one around is wrong. */
> -ret = sys_sigaltstack((stack_t __force __user *) (uss_ptr ?  : NULL),
> -  (stack_t __force __user *) );
> -set_fs(seg);
> -if (ret >= 0 && uoss_ptr)  {
> -if (!access_ok(VERIFY_WRITE, uoss_ptr, sizeof(stack_t)) ||
> -__put_user(uoss.ss_sp, _ptr->ss_sp) ||
> -__put_user(uoss.ss_flags, _ptr->ss_flags) ||
> -__put_user(uoss.ss_size, _ptr->ss_size))
> -ret = -EFAULT;
> -}
> -return ret;
> -}
> -
> -/* sigaltstack needs some special handling as the padding
> -   for stack_t might not be non-zero. */
> -#define sys_sigaltstackilp32_sys_sigaltstack
> -
> -
> #include 
> 
> #undef __SYSCALL
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

Re: [PATCH v4 22/24] arm64:ilp32: use compat for stack_t

[Pinski, Andrew]

 On Apr 13, 2015, at 1:18 PM, Philipp Tomsich 
 philipp.toms...@theobroma-systems.com wrote:
 
 We use a 'natively sized' stack_t in glibc (i.e. having a 32bit pointer for
 ss_sp), which requires the invocation of the compat layer for the following
 functionality:
 * sigaltstack
 * saving and restoring uc_stack during signal setup and returns

Can you explain why you want to use a natively sized stack_t?  My patches for 
glibc included changing stack_t too.  I would rather keep the same size stack_t 
between lp64 and ilp32 due easier gdb support. 

Thanks,
Andrew


 
 As the userspace stack_t is natively sized, we avoid code duplication in the
 syscall table and can use the compat-functions to zero-extend the pointers
 involved.
 
 Signed-off-by: Philipp Tomsich philipp.toms...@theobroma-systems.com
 Signed-off-by: Christoph Muellner christoph.muell...@theobroma-systems.com
 ---
 arch/arm64/kernel/signal.c| 19 +++
 arch/arm64/kernel/sys_ilp32.c | 44 +--
 2 files changed, 20 insertions(+), 43 deletions(-)
 
 diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
 index 99e36be..b3f6e52 100644
 --- a/arch/arm64/kernel/signal.c
 +++ b/arch/arm64/kernel/signal.c
 @@ -34,6 +34,7 @@
 #include asm/fpsimd.h
 #include asm/signal32.h
 #include asm/vdso.h
 +#include asm/syscalls.h
 
 /*
  * Do a signal return; undo the signal stack. These are aligned to 128-bit.
 @@ -148,9 +149,22 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
if (restore_sigframe(regs, frame))
goto badframe;
 
 +
 +#if defined(CONFIG_ARM64_ILP32)
 +if (is_ilp32_compat_task()) {
 +/* For ILP32, we have a different stack_t (the ss_sp
 +   field will be only 32bit sized), which fits into
 +   the memory area reserved for the (larger) LP64
 +   stack_t and which we place into uc_stack: this
 +   implies padding after the ILP32 stack_t. */
 +if 
 (compat_restore_altstack((compat_stack_t*)frame-uc.uc_stack))
 +goto badframe;
 +} else
 +#endif
if (restore_altstack(frame-uc.uc_stack))
goto badframe;
 
 +
return regs-regs[0];
 
 badframe:
 @@ -264,6 +278,11 @@ static int setup_rt_frame(int usig, struct ksignal 
 *ksig, sigset_t *set,
__put_user_error(0, frame-uc.uc_flags, err);
__put_user_error(NULL, frame-uc.uc_link, err);
 
 +#if defined(CONFIG_ARM64_ILP32)
 +if (is_ilp32_compat_task())
 +err |= __compat_save_altstack((compat_stack_t*)frame-uc.uc_stack, 
 regs-sp);
 +else
 +#endif
err |= __save_altstack(frame-uc.uc_stack, regs-sp);
err |= setup_sigframe(frame, regs, set);
if (err == 0) {
 diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c
 index 3471f27..31f82ca 100644
 --- a/arch/arm64/kernel/sys_ilp32.c
 +++ b/arch/arm64/kernel/sys_ilp32.c
 @@ -77,6 +77,7 @@
 
 /* Pointer in struct */
 #define sys_mount   compat_sys_mount
 +#define sys_sigaltstack compat_sys_sigaltstack
 
 /* NUMA */
 /* unsigned long bitmaps */
 @@ -122,49 +123,6 @@ asmlinkage long ilp32_sys_mq_notify(mqd_t mqdes, const 
 struct sigevent __user *u
but need special handling due to padding for SIGEV_THREAD.  */
 #define sys_mq_notifyilp32_sys_mq_notify
 
 -
 -/* sigaltstack needs some special handling as the
 -   padding for stack_t might not be non-zero. */
 -long ilp32_sys_sigaltstack(const stack_t __user *uss_ptr,
 -   stack_t __user *uoss_ptr)
 -{
 -stack_t uss, uoss;
 -int ret;
 -mm_segment_t seg;
 -
 -if (uss_ptr) {
 -if (!access_ok(VERIFY_READ, uss_ptr, sizeof(*uss_ptr)))
 -return -EFAULT;
 -if (__get_user(uss.ss_sp, uss_ptr-ss_sp) |
 -__get_user(uss.ss_flags, uss_ptr-ss_flags) |
 -__get_user(uss.ss_size, uss_ptr-ss_size))
 -return -EFAULT;
 -/* Zero extend the sp address and the size. */
 -uss.ss_sp = (void *)(uintptr_t)(unsigned int)(uintptr_t)uss.ss_sp;
 -uss.ss_size = (size_t)(unsigned int)uss.ss_size;
 -}
 -seg = get_fs();
 -set_fs(KERNEL_DS);
 -/* Note we need to use uoss as we have changed the segment to the
 -   kernel one so passing an user one around is wrong. */
 -ret = sys_sigaltstack((stack_t __force __user *) (uss_ptr ? uss : NULL),
 -  (stack_t __force __user *) uoss);
 -set_fs(seg);
 -if (ret = 0  uoss_ptr)  {
 -if (!access_ok(VERIFY_WRITE, uoss_ptr, sizeof(stack_t)) ||
 -__put_user(uoss.ss_sp, uoss_ptr-ss_sp) ||
 -__put_user(uoss.ss_flags, uoss_ptr-ss_flags) ||
 -__put_user(uoss.ss_size, uoss_ptr-ss_size))
 -ret = -EFAULT;
 -}
 -return ret;
 -}
 -
 -/* sigaltstack needs some special handling as the padding
 -   for stack_t might not be non-zero. */
 -#define sys_sigaltstackilp32_sys_sigaltstack
 -
 -
 #include asm/syscalls.h
 
 #undef __SYSCALL
 -- 

Re: [PATCH v4 00/24] ILP32 for ARM64

[Pinski, Andrew]

> On Apr 16, 2015, at 4:19 AM, Dr. Philipp Tomsich 
>  wrote:
> 
> Just for the record (and to avoid anyone wasting their time on what’s 
> available
> today): we are migrating this over to option (a) now, even though we would
> prefer to see option (b) implemented.
> 
> If we get a consensus on (b) in the next couple of days, we’ll redo things for
> option (b). If not, we will have an implementation for option (a) available 
> that
> we can hopefully all agree on merging.

I don't think either a or b are good in the long run. There are only a few 
places where long should be 32bit rather than 64bit. The non-time_t field of 
timespec is the only one I can think of. The rest are valid and good idea to 
stay as 64bit. Including the limits. I think this whole discussion should have 
happened over a year ago. And I thought c was decided back then. I had even 
implemented a originally and then asked to move over to c. So I am a bit upset 
now we are making this kind of huge changes to the abi a year after the 
original posting of the patch. 

Also why does it takes over a year to accept patches into the linux kernel when 
it takes much less time to make huge changes into gcc (pointer plus is an 
example which took only a few months to accept and it was an infrastructure 
change and this is not even an infrastructure change). 

Thanks,
Andrew


> 
> Best,
> Phil.
> 
>> On 16 Apr 2015, at 13:03, Catalin Marinas  wrote:
>> 
>> On Thu, Apr 16, 2015 at 12:25:36AM +0200, Alexander Graf wrote:
>>> We've just started to bootstrap openSUSE for ILP32 with the non-final
>>> abi. However, keep in mind that at least for us bootstrapping is a
>>> manual process. So changing syscall numbers means we'll need to go
>>> through the manual process again.
>>> 
>>> So if you need any help on getting you an environment that allows you to
>>> build LTP with whatever syscall abi people come up with, feel free to
>>> poke Andreas or me.
>> 
>> Thanks for the offer. It's great to see a full distro being built (even
>> though no commitment).
>> 
>> But I'm a bit worried that people started using these patches and we
>> haven't agreed on the ABI yet (well, for a while we thought that the x32
>> approach was fine until I've seen objections from others).
>> 
>> Maybe a quick poll on the options, ignoring syscall number details (we
>> go for the generic ones) or syscall tables sharing:
>> 
>> a) AArch32-like ILP32 ABI, 32-bit time_t, 32-bit __kernel_long_t
>>  (POSIX-compliant)
>> b) Future-proof ILP32 ABI, 64-bit time_t, 32-bit __kernel_long_t (as
>>  seen by the user) (POSIX-compliant)
>> c) LP64-like ILP32 ABI, 64-bit time_t, 64-bit __kernel_long_t
>>  (non-POSIX-compliant)
>> 
>> Option (a) is the easiest from the kernel perspective and has the
>> highest chance of not breaking legacy AArch32 applications.
>> 
>> Option (b) is more future looking (beyond 2038) but it introduces a
>> third ABI in the kernel (incompatible with both compat and native).
>> There is also a risk that legacy applications assume a 32-bit time_t.
>> 
>> Option (c) is pretty much what we currently have in these patches. While
>> many syscalls are native LP64, it doesn't fully solve pointers in
>> structures shared between user and kernel (ioctl being one of the
>> affected areas)
>> 
>> I can't tell how bad the impact of non-POSIX compliance is. If this is
>> essential, between (a) and (b) I'm more in favour of (a) as the easiest
>> for both kernel and user. If we were to start a new ABI from scratch
>> without legacy applications, I would have definitely gone for (b) but
>> I'm worried about legacy applications still not fully working with this
>> option while adding more maintenance burden in the kernel.
>> 
>> -- 
>> Catalin
> 
--
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 00/24] ILP32 for ARM64

[Pinski, Andrew]

 On Apr 16, 2015, at 4:19 AM, Dr. Philipp Tomsich 
 philipp.toms...@theobroma-systems.com wrote:
 
 Just for the record (and to avoid anyone wasting their time on what’s 
 available
 today): we are migrating this over to option (a) now, even though we would
 prefer to see option (b) implemented.
 
 If we get a consensus on (b) in the next couple of days, we’ll redo things for
 option (b). If not, we will have an implementation for option (a) available 
 that
 we can hopefully all agree on merging.

I don't think either a or b are good in the long run. There are only a few 
places where long should be 32bit rather than 64bit. The non-time_t field of 
timespec is the only one I can think of. The rest are valid and good idea to 
stay as 64bit. Including the limits. I think this whole discussion should have 
happened over a year ago. And I thought c was decided back then. I had even 
implemented a originally and then asked to move over to c. So I am a bit upset 
now we are making this kind of huge changes to the abi a year after the 
original posting of the patch. 

Also why does it takes over a year to accept patches into the linux kernel when 
it takes much less time to make huge changes into gcc (pointer plus is an 
example which took only a few months to accept and it was an infrastructure 
change and this is not even an infrastructure change). 

Thanks,
Andrew


 
 Best,
 Phil.
 
 On 16 Apr 2015, at 13:03, Catalin Marinas catalin.mari...@arm.com wrote:
 
 On Thu, Apr 16, 2015 at 12:25:36AM +0200, Alexander Graf wrote:
 We've just started to bootstrap openSUSE for ILP32 with the non-final
 abi. However, keep in mind that at least for us bootstrapping is a
 manual process. So changing syscall numbers means we'll need to go
 through the manual process again.
 
 So if you need any help on getting you an environment that allows you to
 build LTP with whatever syscall abi people come up with, feel free to
 poke Andreas or me.
 
 Thanks for the offer. It's great to see a full distro being built (even
 though no commitment).
 
 But I'm a bit worried that people started using these patches and we
 haven't agreed on the ABI yet (well, for a while we thought that the x32
 approach was fine until I've seen objections from others).
 
 Maybe a quick poll on the options, ignoring syscall number details (we
 go for the generic ones) or syscall tables sharing:
 
 a) AArch32-like ILP32 ABI, 32-bit time_t, 32-bit __kernel_long_t
  (POSIX-compliant)
 b) Future-proof ILP32 ABI, 64-bit time_t, 32-bit __kernel_long_t (as
  seen by the user) (POSIX-compliant)
 c) LP64-like ILP32 ABI, 64-bit time_t, 64-bit __kernel_long_t
  (non-POSIX-compliant)
 
 Option (a) is the easiest from the kernel perspective and has the
 highest chance of not breaking legacy AArch32 applications.
 
 Option (b) is more future looking (beyond 2038) but it introduces a
 third ABI in the kernel (incompatible with both compat and native).
 There is also a risk that legacy applications assume a 32-bit time_t.
 
 Option (c) is pretty much what we currently have in these patches. While
 many syscalls are native LP64, it doesn't fully solve pointers in
 structures shared between user and kernel (ioctl being one of the
 affected areas)
 
 I can't tell how bad the impact of non-POSIX compliance is. If this is
 essential, between (a) and (b) I'm more in favour of (a) as the easiest
 for both kernel and user. If we were to start a new ABI from scratch
 without legacy applications, I would have definitely gone for (b) but
 I'm worried about legacy applications still not fully working with this
 option while adding more maintenance burden in the kernel.
 
 -- 
 Catalin
 
--
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 00/24] ILP32 for ARM64

[Pinski, Andrew]

> On Apr 14, 2015, at 3:08 AM, Arnd Bergmann  wrote:
> 
>> On Tuesday 14 April 2015 11:33:13 Dr.  Philipp Tomsich wrote:
>> Arnd,
>> 
>> After getting a good night’s sleep, the “reuse the existing system call 
>> table” comment
>> makes a little more sense as I construe it as having just one merged system 
>> call table
>> for both LP64 and ILP32 and handling the differences through a different 
>> system call
>> numbering in unistd.h towards LP64 and ILP32 processes.
>> 
>> If this is the intended implementation, I am not fully sold on the benefit: 
>> having a private
>> copy of unistd.h for ARM64 seems to be a less readable and less 
>> maintenance-friendly
>> solution to having separate tables.
>> 
>> We’re open to input on this and—if merging the system call tables is the 
>> consensus—
>> would like to get the change underway as soon as possible.
> 
> There are multiple ways of doing this:
> 
> a) separate syscall table for arm64: as you say, this is the current approach,
>   and I'd like to avoid that too
> b) add syscalls for ilp32 as additional numbers in the normal lp64 version of
>   asm-generic/unistd.h, and share the binary tables between ilp32 and lp64
>   on aarch64
> c) change asm-generic/unistd.h to generate three possible tables: instead of
>   just native (lp64 or ilp32 depending on the arch), compat (support for
>   existing ilp32 binaries on some architectures, there would also be a
>   "modern" ilp32 variant that is a mix of the two, as your table today
> d) don't use the asm-generic/unistd.h table for aarch64-ilp32 at all, but 
> instead
>   reuse the table from arch/arm64/include/asm/unistd32.h
> 
> I think you are referring to approach b) or c) above, but my preferred one
> would actually be d).

D is the worst of all 4 options in my mind. The reason is when a new syscall is 
added, then you have to update that file too. Also d is worse than the rest as 
you no longer default to 64bit off_t which is not a good thing.  B is just as 
bad and goes against using the generic syscall numbers. 

I was trying to model ilp32 so there was less maintain hassle if a new syscall 
was added. 

Also about time_t, my original patch had used 32bit but was asked to change it 
to the 64bit one. So now I am upset this being asked again to change it back. 
The review process for the linux kernel is much harder than the review process 
of gcc or even glibc now. 

Thanks,
Andrew


> 
 On 14 Apr 2015, at 00:58, Dr. Philipp Tomsich 
  wrote:
 
 2. The ABI follows what x86 has their "x32" ABI. This never saw a lot of
 adoption and in retrospect the decision to have separate system calls seems
 to not have helped them. My feeling now is that if we add support for the
 ARM64 ILP32 ELF ABI, we should better stick to the existing system call ABI
 as close as possible and reuse the existing system call table. I realize
 that this is a bit controversial, but please let's talk about this now.
>>> 
>>> I see benefits and drawback to merging the system tables.  Our philosophy is
>>> already somewhat different from x32 and from the original patch-series, as 
>>> you
>>> can see from the changes dealing with stack_t in the ‘sys_rt_sigreturn' and 
>>> ‘setup_rt_frame’ functions.  While these could have been duplicated and 
>>> specialized for each ABI (as on x32), the attempt was made to keep these 
>>> changes localized.
>>> 
>>> However, this approach can not always work: if you consider cases like 
>>> ‘sys_msgsnd’ and ‘compat_sys_msgsnd’, there’s little to no benefit in having
>>> just a ‘aarch64_sys_msgsnd’, which then calls either the LP64 or the compat
>>> version of the underlying system call.  Having a second system call table 
>>> helps to reduce the overheads in this case and keeps things readable.
>>> 
>>> This comes down to the fact, that a few calls will always be different due 
>>> to
>>> historical baggage in data structures shared between userspace and kernel:
>>> 'struct msgbuf’ immediatly comes to mind.
>>> 
>>> I would liken the situation with ARM64 more of MIPS64 with its 64bit ABI 
>>> and 
>>> its n32 ABI than to x32… but even there it’s two separate system call tables
>>> (although sequentially concatenated).
>>> 
>>> In other words: I fail to see the benefit from keeping the existing table.
>>> I you elaborate on how such a solution should look, I might be better able
>>> to follow. 
> 
> I mainly want to avoid accidentally creating new ABIs for syscalls and ioctls:
> we have many drivers that today use ioctls with data structures derived from
> '__kernel_ulong_t' in some form, often by including a timespec or time_t in
> their own data structures. These are almost all broken today, because the
> data structures are a mix of the aarch32 and aarch64 variants, while the
> ioctl() system call in ilp32 always uses the aarch32 format by default.
> 
> An example here would be 
> 
> struct cyclades_idle_stats {
>__kernel_time_t in_use; /* Time device 

Re: [PATCH v4 00/24] ILP32 for ARM64

[Pinski, Andrew]

> On Apr 14, 2015, at 4:15 AM, Arnd Bergmann  wrote:
> 
> On Tuesday 14 April 2015 10:45:43 Pinski, Andrew wrote:
>>> On Apr 14, 2015, at 3:08 AM, Arnd Bergmann  wrote:
>>> 
>>>> On Tuesday 14 April 2015 11:33:13 Dr.  Philipp Tomsich wrote:
>>>> Arnd,
>>>> 
>>>> After getting a good night’s sleep, the “reuse the existing system call 
>>>> table” comment
>>>> makes a little more sense as I construe it as having just one merged 
>>>> system call table
>>>> for both LP64 and ILP32 and handling the differences through a different 
>>>> system call
>>>> numbering in unistd.h towards LP64 and ILP32 processes.
>>>> 
>>>> If this is the intended implementation, I am not fully sold on the 
>>>> benefit: having a private
>>>> copy of unistd.h for ARM64 seems to be a less readable and less 
>>>> maintenance-friendly
>>>> solution to having separate tables.
>>>> 
>>>> We’re open to input on this and—if merging the system call tables is the 
>>>> consensus—
>>>> would like to get the change underway as soon as possible.
>>> 
>>> There are multiple ways of doing this:
>>> 
>>> a) separate syscall table for arm64: as you say, this is the current 
>>> approach,
>>>  and I'd like to avoid that too
>>> b) add syscalls for ilp32 as additional numbers in the normal lp64 version 
>>> of
>>>  asm-generic/unistd.h, and share the binary tables between ilp32 and lp64
>>>  on aarch64
>>> c) change asm-generic/unistd.h to generate three possible tables: instead of
>>>  just native (lp64 or ilp32 depending on the arch), compat (support for
>>>  existing ilp32 binaries on some architectures, there would also be a
>>>  "modern" ilp32 variant that is a mix of the two, as your table today
>>> d) don't use the asm-generic/unistd.h table for aarch64-ilp32 at all, but 
>>> instead
>>>  reuse the table from arch/arm64/include/asm/unistd32.h
>>> 
>>> I think you are referring to approach b) or c) above, but my preferred one
>>> would actually be d).
>> 
>> D is the worst of all 4 options in my mind. The reason is when a new syscall 
>> is
>> added, then you have to update that file too.
> 
> I don't know what the miscommunication is here, but the advantage of d is
> specifically that it is /less/ work to maintain: With the current approach,
> each new syscall that gets added needs to be checked to see if the normal
> aarch64 version works or if it needs another wrapper, while with d) we
> get the update for free, because we follow exactly what aarch32 is doing.

More than that d won't work due to ucontext being different between aarch32 and 
aarch64. I still say the current way is the best approach and is better option 
than the rest and it was what was agreed upon when I wrote the patch. I don't 
see why you are agreeing a different way. The split 64bit long was decided not 
to be split too, there was a previous discussion about that too. 

Also this abi is about to be used in a product so any changes need to happen 
fast and need to thought out why making changes to it make senses. Changing to 
use the aarch32 syscall #'s make less sense since this is not a legacy 
syscalls. 

> 
>> Also d is worse than the rest as
>> you no longer default to 64bit off_t which is not a good thing.
> 
> That decision is up to the libc implementation, just as it is for the existing
> aarch32 libc. The kernel just offers both versions and the libc can pick
> one, or use the _LARGEFILE64_SOURCE hack that glibc has to also implement
> both. It would probably be reasonable to use 64-bit off_t only for a libc
> and ignore the old calls.
> 
>> B is just as bad and goes against using the generic syscall numbers.
> 
> How so? The newly introduce syscalls then would be the generic ones.
> 
>> I was trying to model ilp32 so there was less maintain hassle if a new 
>> syscall was added. 
>> 
>> Also about time_t, my original patch had used 32bit but was asked to change
>> it to the 64bit one. So now I am upset this being asked again to change it 
>> back.
>> The review process for the linux kernel is much harder than the review 
>> process
>> of gcc or even glibc now.
> 
> For now, I'm just opening that discussion again, but the reason this
> comes up again now is that a lot has happened in the meantime on this
> front, and we have already decided to merge new architecture ports with
> 32-bit time_t since.
> 
>Arnd
--
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 00/24] ILP32 for ARM64

[Pinski, Andrew]

 On Apr 14, 2015, at 3:08 AM, Arnd Bergmann a...@arndb.de wrote:
 
 On Tuesday 14 April 2015 11:33:13 Dr.  Philipp Tomsich wrote:
 Arnd,
 
 After getting a good night’s sleep, the “reuse the existing system call 
 table” comment
 makes a little more sense as I construe it as having just one merged system 
 call table
 for both LP64 and ILP32 and handling the differences through a different 
 system call
 numbering in unistd.h towards LP64 and ILP32 processes.
 
 If this is the intended implementation, I am not fully sold on the benefit: 
 having a private
 copy of unistd.h for ARM64 seems to be a less readable and less 
 maintenance-friendly
 solution to having separate tables.
 
 We’re open to input on this and—if merging the system call tables is the 
 consensus—
 would like to get the change underway as soon as possible.
 
 There are multiple ways of doing this:
 
 a) separate syscall table for arm64: as you say, this is the current approach,
   and I'd like to avoid that too
 b) add syscalls for ilp32 as additional numbers in the normal lp64 version of
   asm-generic/unistd.h, and share the binary tables between ilp32 and lp64
   on aarch64
 c) change asm-generic/unistd.h to generate three possible tables: instead of
   just native (lp64 or ilp32 depending on the arch), compat (support for
   existing ilp32 binaries on some architectures, there would also be a
   modern ilp32 variant that is a mix of the two, as your table today
 d) don't use the asm-generic/unistd.h table for aarch64-ilp32 at all, but 
 instead
   reuse the table from arch/arm64/include/asm/unistd32.h
 
 I think you are referring to approach b) or c) above, but my preferred one
 would actually be d).

D is the worst of all 4 options in my mind. The reason is when a new syscall is 
added, then you have to update that file too. Also d is worse than the rest as 
you no longer default to 64bit off_t which is not a good thing.  B is just as 
bad and goes against using the generic syscall numbers. 

I was trying to model ilp32 so there was less maintain hassle if a new syscall 
was added. 

Also about time_t, my original patch had used 32bit but was asked to change it 
to the 64bit one. So now I am upset this being asked again to change it back. 
The review process for the linux kernel is much harder than the review process 
of gcc or even glibc now. 

Thanks,
Andrew


 
 On 14 Apr 2015, at 00:58, Dr. Philipp Tomsich 
 philipp.toms...@theobroma-systems.com wrote:
 
 2. The ABI follows what x86 has their x32 ABI. This never saw a lot of
 adoption and in retrospect the decision to have separate system calls seems
 to not have helped them. My feeling now is that if we add support for the
 ARM64 ILP32 ELF ABI, we should better stick to the existing system call ABI
 as close as possible and reuse the existing system call table. I realize
 that this is a bit controversial, but please let's talk about this now.
 
 I see benefits and drawback to merging the system tables.  Our philosophy is
 already somewhat different from x32 and from the original patch-series, as 
 you
 can see from the changes dealing with stack_t in the ‘sys_rt_sigreturn' and 
 ‘setup_rt_frame’ functions.  While these could have been duplicated and 
 specialized for each ABI (as on x32), the attempt was made to keep these 
 changes localized.
 
 However, this approach can not always work: if you consider cases like 
 ‘sys_msgsnd’ and ‘compat_sys_msgsnd’, there’s little to no benefit in having
 just a ‘aarch64_sys_msgsnd’, which then calls either the LP64 or the compat
 version of the underlying system call.  Having a second system call table 
 helps to reduce the overheads in this case and keeps things readable.
 
 This comes down to the fact, that a few calls will always be different due 
 to
 historical baggage in data structures shared between userspace and kernel:
 'struct msgbuf’ immediatly comes to mind.
 
 I would liken the situation with ARM64 more of MIPS64 with its 64bit ABI 
 and 
 its n32 ABI than to x32… but even there it’s two separate system call tables
 (although sequentially concatenated).
 
 In other words: I fail to see the benefit from keeping the existing table.
 I you elaborate on how such a solution should look, I might be better able
 to follow. 
 
 I mainly want to avoid accidentally creating new ABIs for syscalls and ioctls:
 we have many drivers that today use ioctls with data structures derived from
 '__kernel_ulong_t' in some form, often by including a timespec or time_t in
 their own data structures. These are almost all broken today, because the
 data structures are a mix of the aarch32 and aarch64 variants, while the
 ioctl() system call in ilp32 always uses the aarch32 format by default.
 
 An example here would be 
 
 struct cyclades_idle_stats {
__kernel_time_t in_use; /* Time device has been in use (secs) */
__kernel_time_t recv_idle;  /* Time since last char received (secs) */
__kernel_time_t xmit_idle;  /* Time since 

Re: [PATCH v4 00/24] ILP32 for ARM64

[Pinski, Andrew]

 On Apr 14, 2015, at 4:15 AM, Arnd Bergmann a...@arndb.de wrote:
 
 On Tuesday 14 April 2015 10:45:43 Pinski, Andrew wrote:
 On Apr 14, 2015, at 3:08 AM, Arnd Bergmann a...@arndb.de wrote:
 
 On Tuesday 14 April 2015 11:33:13 Dr.  Philipp Tomsich wrote:
 Arnd,
 
 After getting a good night’s sleep, the “reuse the existing system call 
 table” comment
 makes a little more sense as I construe it as having just one merged 
 system call table
 for both LP64 and ILP32 and handling the differences through a different 
 system call
 numbering in unistd.h towards LP64 and ILP32 processes.
 
 If this is the intended implementation, I am not fully sold on the 
 benefit: having a private
 copy of unistd.h for ARM64 seems to be a less readable and less 
 maintenance-friendly
 solution to having separate tables.
 
 We’re open to input on this and—if merging the system call tables is the 
 consensus—
 would like to get the change underway as soon as possible.
 
 There are multiple ways of doing this:
 
 a) separate syscall table for arm64: as you say, this is the current 
 approach,
  and I'd like to avoid that too
 b) add syscalls for ilp32 as additional numbers in the normal lp64 version 
 of
  asm-generic/unistd.h, and share the binary tables between ilp32 and lp64
  on aarch64
 c) change asm-generic/unistd.h to generate three possible tables: instead of
  just native (lp64 or ilp32 depending on the arch), compat (support for
  existing ilp32 binaries on some architectures, there would also be a
  modern ilp32 variant that is a mix of the two, as your table today
 d) don't use the asm-generic/unistd.h table for aarch64-ilp32 at all, but 
 instead
  reuse the table from arch/arm64/include/asm/unistd32.h
 
 I think you are referring to approach b) or c) above, but my preferred one
 would actually be d).
 
 D is the worst of all 4 options in my mind. The reason is when a new syscall 
 is
 added, then you have to update that file too.
 
 I don't know what the miscommunication is here, but the advantage of d is
 specifically that it is /less/ work to maintain: With the current approach,
 each new syscall that gets added needs to be checked to see if the normal
 aarch64 version works or if it needs another wrapper, while with d) we
 get the update for free, because we follow exactly what aarch32 is doing.

More than that d won't work due to ucontext being different between aarch32 and 
aarch64. I still say the current way is the best approach and is better option 
than the rest and it was what was agreed upon when I wrote the patch. I don't 
see why you are agreeing a different way. The split 64bit long was decided not 
to be split too, there was a previous discussion about that too. 

Also this abi is about to be used in a product so any changes need to happen 
fast and need to thought out why making changes to it make senses. Changing to 
use the aarch32 syscall #'s make less sense since this is not a legacy 
syscalls. 

 
 Also d is worse than the rest as
 you no longer default to 64bit off_t which is not a good thing.
 
 That decision is up to the libc implementation, just as it is for the existing
 aarch32 libc. The kernel just offers both versions and the libc can pick
 one, or use the _LARGEFILE64_SOURCE hack that glibc has to also implement
 both. It would probably be reasonable to use 64-bit off_t only for a libc
 and ignore the old calls.
 
 B is just as bad and goes against using the generic syscall numbers.
 
 How so? The newly introduce syscalls then would be the generic ones.
 
 I was trying to model ilp32 so there was less maintain hassle if a new 
 syscall was added. 
 
 Also about time_t, my original patch had used 32bit but was asked to change
 it to the 64bit one. So now I am upset this being asked again to change it 
 back.
 The review process for the linux kernel is much harder than the review 
 process
 of gcc or even glibc now.
 
 For now, I'm just opening that discussion again, but the reason this
 comes up again now is that a lot has happened in the meantime on this
 front, and we have already decided to merge new architecture ports with
 32-bit time_t since.
 
Arnd
--
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 22/24] ARM64:ILP32: Use a seperate syscall table as a few syscalls need to be using the compat syscalls.

[Pinski, Andrew]

> On Jul 1, 2014, at 8:07 AM, "Catalin Marinas"  wrote:
> 
>> On Sat, May 24, 2014 at 12:02:17AM -0700, Andrew Pinski wrote:
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 1e1ebfc..8241ffe 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -620,9 +620,14 @@ ENDPROC(ret_from_fork)
>>  */
>>.align6
>> el0_svc:
>> -adrpstbl, sys_call_table// load syscall table pointer
>>uxtwscno, w8// syscall number in w8
>>movsc_nr, #__NR_syscalls
>> +#ifdef CONFIG_ARM64_ILP32
>> +get_thread_info tsk
>> +ldrx16, [tsk, #TI_FLAGS]
>> +tbnzx16, #TIF_32BIT_AARCH64, el0_ilp32_svc // We are using ILP32
>> +#endif
>> +adrpstbl, sys_call_table// load syscall table pointer
> 
> This adds a slight penalty on the AArch64 SVC entry path. I can't tell
> whether that's visible or not but I think the x86 guys decided to set an
> extra bit to the syscall number to distinguish it from native calls.
> 
>> diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c
>> new file mode 100644
>> index 000..1da1d11
>> --- /dev/null
>> +++ b/arch/arm64/kernel/sys_ilp32.c
> [...]
>> +/*
>> + * Wrappers to pass the pt_regs argument.
>> + */
>> +#define sys_rt_sigreturn sys_rt_sigreturn_wrapper
>> +
>> +
>> +/* Using Compat syscalls where necessary */
>> +#define sys_ioctlcompat_sys_ioctl
>> +/* iovec */
>> +#define sys_readvcompat_sys_readv
>> +#define sys_writevcompat_sys_writev
>> +#define sys_preadvcompat_sys_preadv64
>> +#define sys_pwritevcompat_sys_pwritev64
>> +#define sys_vmsplicecompat_sys_vmsplice
> 
> Do these actually work? compat_iovec has two members of 32-bit each
> while the ILP32 iovec has a void * (32-bit) and a __kernel_size_t which
> is 64-bit.

size_t should be unsigned long in ilp32 so a 32bit unsigned integer type.  That 
part of the abi was already defined in the arm abi documents.  Now are saying 
we should pass size_t different between user and kernel space?

> 
>> +/* robust_list_head */
>> +#define sys_set_robust_listcompat_sys_set_robust_list
>> +#define sys_get_robust_listcompat_sys_get_robust_list
> 
> Same here, we have a size_t * argument. The compat function would write
> back 32-bit but size_t is 64-bit for ILP32.

See above. Size_t is 32bits. 

> 
>> +/* kexec_segment */
>> +#define sys_kexec_loadcompat_sys_kexec_load
> 
> More size_t members in the kexec_segment structure (but we don't yet
> have kexec on arm64).

See above. Size_t is 32bits. 

> 
>> +/* struct msghdr */
>> +#define sys_recvfromcompat_sys_recvfrom
> 
> Why compat here? struct sockaddr seems to be the same as the native one.
> 
>> +#define sys_recvmmsgcompat_sys_recvmmsg
>> +#define sys_sendmmsgcompat_sys_sendmmsg
>> +#define sys_sendmsgcompat_sys_sendmsg
>> +#define sys_recvmsgcompat_sys_recvmsg
> 
> These get messier as well with a different size_t affecting struct
> msghdr.

See above about size_t. 


> 
>> +#define sys_setsockoptcompat_sys_setsockopt
>> +#define sys_getsockoptcompat_sys_getsockopt
> 
> Looking at the sock_getsockopt() function, we have a union v copied
> to/from user. However, such a union contains a struct timeval which for
> ILP32 would be different than the compat one.

I will look into this one but it might already be taken care of due to the 
compact uses 64bit time spec define. I will add a comment saying that if it is 
true. 


> 
>> +/* iovec */
>> +#define sys_process_vm_readvcompat_sys_process_vm_readv
>> +#define sys_process_vm_writevcompat_sys_process_vm_writev
> 
> See above for iovec.

See above for my size_t question. 

> 
>> +/* Pointer in struct */
>> +#define sys_mount   compat_sys_mount
> 
> Which structure is this?

NFS structure, I can expand out the comment if needed. 


> 
>> +/* Scheduler */
>> +/* unsigned long bitmaps */
>> +#define sys_sched_setaffinity   compat_sys_sched_setaffinity
>> +#define sys_sched_getaffinity   compat_sys_sched_getaffinity
> 
> Does the long bitmask matter here? I can see the length is passed in
> bytes.

Yes for big endian. If we were only supporting little endian, bit fields would 
not matter. 

> 
>> +/* iov usage */
>> +#define sys_keyctl  compat_sys_keyctl
> 
> Same problem as iovec above.
> 
>> +/* aio */
>> +/* Pointer to Pointer  */
>> +#define sys_io_setupcompat_sys_io_setup
> 
> sys_io_setup takes a pointer to aio_context_t which is defined as
> __kernel_ulong_t (same as LP64).

Let me look at why I did this one, I think the code which used aio was not in 
glibc which is why I used the compat version. 

Thanks,
Andrew

> 
> -- 
> Catalin
--
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 

Re: [PATCH 22/24] ARM64:ILP32: Use a seperate syscall table as a few syscalls need to be using the compat syscalls.

[Pinski, Andrew]

 On Jul 1, 2014, at 8:07 AM, Catalin Marinas catalin.mari...@arm.com wrote:
 
 On Sat, May 24, 2014 at 12:02:17AM -0700, Andrew Pinski wrote:
 diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
 index 1e1ebfc..8241ffe 100644
 --- a/arch/arm64/kernel/entry.S
 +++ b/arch/arm64/kernel/entry.S
 @@ -620,9 +620,14 @@ ENDPROC(ret_from_fork)
  */
.align6
 el0_svc:
 -adrpstbl, sys_call_table// load syscall table pointer
uxtwscno, w8// syscall number in w8
movsc_nr, #__NR_syscalls
 +#ifdef CONFIG_ARM64_ILP32
 +get_thread_info tsk
 +ldrx16, [tsk, #TI_FLAGS]
 +tbnzx16, #TIF_32BIT_AARCH64, el0_ilp32_svc // We are using ILP32
 +#endif
 +adrpstbl, sys_call_table// load syscall table pointer
 
 This adds a slight penalty on the AArch64 SVC entry path. I can't tell
 whether that's visible or not but I think the x86 guys decided to set an
 extra bit to the syscall number to distinguish it from native calls.
 
 diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c
 new file mode 100644
 index 000..1da1d11
 --- /dev/null
 +++ b/arch/arm64/kernel/sys_ilp32.c
 [...]
 +/*
 + * Wrappers to pass the pt_regs argument.
 + */
 +#define sys_rt_sigreturn sys_rt_sigreturn_wrapper
 +
 +
 +/* Using Compat syscalls where necessary */
 +#define sys_ioctlcompat_sys_ioctl
 +/* iovec */
 +#define sys_readvcompat_sys_readv
 +#define sys_writevcompat_sys_writev
 +#define sys_preadvcompat_sys_preadv64
 +#define sys_pwritevcompat_sys_pwritev64
 +#define sys_vmsplicecompat_sys_vmsplice
 
 Do these actually work? compat_iovec has two members of 32-bit each
 while the ILP32 iovec has a void * (32-bit) and a __kernel_size_t which
 is 64-bit.

size_t should be unsigned long in ilp32 so a 32bit unsigned integer type.  That 
part of the abi was already defined in the arm abi documents.  Now are saying 
we should pass size_t different between user and kernel space?

 
 +/* robust_list_head */
 +#define sys_set_robust_listcompat_sys_set_robust_list
 +#define sys_get_robust_listcompat_sys_get_robust_list
 
 Same here, we have a size_t * argument. The compat function would write
 back 32-bit but size_t is 64-bit for ILP32.

See above. Size_t is 32bits. 

 
 +/* kexec_segment */
 +#define sys_kexec_loadcompat_sys_kexec_load
 
 More size_t members in the kexec_segment structure (but we don't yet
 have kexec on arm64).

See above. Size_t is 32bits. 

 
 +/* struct msghdr */
 +#define sys_recvfromcompat_sys_recvfrom
 
 Why compat here? struct sockaddr seems to be the same as the native one.
 
 +#define sys_recvmmsgcompat_sys_recvmmsg
 +#define sys_sendmmsgcompat_sys_sendmmsg
 +#define sys_sendmsgcompat_sys_sendmsg
 +#define sys_recvmsgcompat_sys_recvmsg
 
 These get messier as well with a different size_t affecting struct
 msghdr.

See above about size_t. 


 
 +#define sys_setsockoptcompat_sys_setsockopt
 +#define sys_getsockoptcompat_sys_getsockopt
 
 Looking at the sock_getsockopt() function, we have a union v copied
 to/from user. However, such a union contains a struct timeval which for
 ILP32 would be different than the compat one.

I will look into this one but it might already be taken care of due to the 
compact uses 64bit time spec define. I will add a comment saying that if it is 
true. 


 
 +/* iovec */
 +#define sys_process_vm_readvcompat_sys_process_vm_readv
 +#define sys_process_vm_writevcompat_sys_process_vm_writev
 
 See above for iovec.

See above for my size_t question. 

 
 +/* Pointer in struct */
 +#define sys_mount   compat_sys_mount
 
 Which structure is this?

NFS structure, I can expand out the comment if needed. 


 
 +/* Scheduler */
 +/* unsigned long bitmaps */
 +#define sys_sched_setaffinity   compat_sys_sched_setaffinity
 +#define sys_sched_getaffinity   compat_sys_sched_getaffinity
 
 Does the long bitmask matter here? I can see the length is passed in
 bytes.

Yes for big endian. If we were only supporting little endian, bit fields would 
not matter. 

 
 +/* iov usage */
 +#define sys_keyctl  compat_sys_keyctl
 
 Same problem as iovec above.
 
 +/* aio */
 +/* Pointer to Pointer  */
 +#define sys_io_setupcompat_sys_io_setup
 
 sys_io_setup takes a pointer to aio_context_t which is defined as
 __kernel_ulong_t (same as LP64).

Let me look at why I did this one, I think the code which used aio was not in 
glibc which is why I used the compat version. 

Thanks,
Andrew

 
 -- 
 Catalin
--
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: [PATCHv2 00/24] ILP32 Support in ARM64

[Pinski, Andrew]

> On Jun 17, 2014, at 3:48 AM, "Catalin Marinas"  
> wrote:
> 
> On Mon, Jun 16, 2014 at 05:19:32PM +, Pinski, Andrew wrote:
>>>> On Jun 16, 2014, at 10:08 AM, "Catalin Marinas"  
>>>> wrote:
>>>> On Sat, May 24, 2014 at 12:01:55AM -0700, Andrew Pinski wrote:
>>>> New version of the patches with documentation, signal changes are
>>>> simplified, using less compat syscalls and splitting up the patches so
>>>> it is easier to review.  I have tested LTP on both LP64 and ILP32.
>>>> There is a few LTP failures but they are due to LTP being incorrect
>>>> (sigaction structure in glibc is not the one which is used by the
>>>> kernel)
>>> 
>>> Do you have more details about what's wrong here and where the fix
>>> should go? LTP? glibc? Kernel?
>> 
>> LTP assumes some sigaction structure is the same between userland and kernel.
>> Glibc has the correct idea of what the kernel structure will be when
>> passing to the kernel already. The fix should be done in LTP. There is
>> already code in LTP for x86 for a similar issue which we should be
>> able to advantage of. 
> 
> OK. I guess you are planning to submit the LTP patch at some point (once
> kernel and glibc changes are agreed).
> 
> Any plans for big-endian ILP32?

The support is there already and ltp results are no difference from 
little-endian. 

> 
>>> I'll give you more specific comments on the code in the next couple of
>>> days. But for cosmetics, please wrap the lines around 72 (or whatever)
>>> characters both in emails, commit logs and Documentation/* files (and
>>> you can drop the "Thanks" part in every commit log ;)).
> 
> I forgot to mention dropping the full stop at the end of every subject.
> 
>> Will do this with the rest of the review. 
> 
> More coding style issues: please have a look at
> Documentation/CodingStyle. While I'm not usually bothered with minor
> aspects, I would like at least some consistency for multi-line comment
> style.
> 
> Also please get the patches through checkpatch.pl (it doesn't need to be
> 100% pass but it gives some clues).

I did run them through checkpatch already. The only warnings left were over 80 
column warnings. 


> 
> There are a few #defines you added without corresponding brackets (hpa
> commented on one already).

Checkpatch did not warn about these but I will fix them.

Thanks,
Andrew


> 
> -- 
> Catalin
--
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: [PATCHv2 00/24] ILP32 Support in ARM64

[Pinski, Andrew]

 On Jun 17, 2014, at 3:48 AM, Catalin Marinas catalin.mari...@arm.com 
 wrote:
 
 On Mon, Jun 16, 2014 at 05:19:32PM +, Pinski, Andrew wrote:
 On Jun 16, 2014, at 10:08 AM, Catalin Marinas catalin.mari...@arm.com 
 wrote:
 On Sat, May 24, 2014 at 12:01:55AM -0700, Andrew Pinski wrote:
 New version of the patches with documentation, signal changes are
 simplified, using less compat syscalls and splitting up the patches so
 it is easier to review.  I have tested LTP on both LP64 and ILP32.
 There is a few LTP failures but they are due to LTP being incorrect
 (sigaction structure in glibc is not the one which is used by the
 kernel)
 
 Do you have more details about what's wrong here and where the fix
 should go? LTP? glibc? Kernel?
 
 LTP assumes some sigaction structure is the same between userland and kernel.
 Glibc has the correct idea of what the kernel structure will be when
 passing to the kernel already. The fix should be done in LTP. There is
 already code in LTP for x86 for a similar issue which we should be
 able to advantage of. 
 
 OK. I guess you are planning to submit the LTP patch at some point (once
 kernel and glibc changes are agreed).
 
 Any plans for big-endian ILP32?

The support is there already and ltp results are no difference from 
little-endian. 

 
 I'll give you more specific comments on the code in the next couple of
 days. But for cosmetics, please wrap the lines around 72 (or whatever)
 characters both in emails, commit logs and Documentation/* files (and
 you can drop the Thanks part in every commit log ;)).
 
 I forgot to mention dropping the full stop at the end of every subject.
 
 Will do this with the rest of the review. 
 
 More coding style issues: please have a look at
 Documentation/CodingStyle. While I'm not usually bothered with minor
 aspects, I would like at least some consistency for multi-line comment
 style.
 
 Also please get the patches through checkpatch.pl (it doesn't need to be
 100% pass but it gives some clues).

I did run them through checkpatch already. The only warnings left were over 80 
column warnings. 


 
 There are a few #defines you added without corresponding brackets (hpa
 commented on one already).

Checkpatch did not warn about these but I will fix them.

Thanks,
Andrew


 
 -- 
 Catalin
--
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: [PATCHv2 00/24] ILP32 Support in ARM64

[Pinski, Andrew]

> On Jun 16, 2014, at 10:08 AM, "Catalin Marinas"  
> wrote:
> 
> Andrew,
> 
>> On Sat, May 24, 2014 at 12:01:55AM -0700, Andrew Pinski wrote:
>> New version of the patches with documentation, signal changes are
>> simplified, using less compat syscalls and splitting up the patches so
>> it is easier to review.  I have tested LTP on both LP64 and ILP32.
>> There is a few LTP failures but they are due to LTP being incorrect
>> (sigaction structure in glibc is not the one which is used by the
>> kernel)
> 
> Do you have more details about what's wrong here and where the fix
> should go? LTP? glibc? Kernel?

LTP assumes some sigaction structure is the same between userland and kernel.
Glibc has the correct idea of what the kernel structure will be when passing to 
the kernel already. The fix should be done in LTP. There is already code in LTP 
for x86 for a similar issue which we should be able to advantage of. 

> 
> I'll give you more specific comments on the code in the next couple of
> days. But for cosmetics, please wrap the lines around 72 (or whatever)
> characters both in emails, commit logs and Documentation/* files (and
> you can drop the "Thanks" part in every commit log ;)).

Will do this with the rest of the review. 

> 
>> Andrew Pinski (24):
>>  ARM64: Force LP64 to compile the kernel.
>>  ARM64: Rename COMPAT to AARCH32_EL0 in Kconfig.
>>  ARM64: Change some CONFIG_COMPAT over to use CONFIG_AARCH32_EL0
>>instead.
>>  ARM64:ILP32: Set kernel_long to long long so we can reuse most of the
>>same syscalls as LP64.
>>  ARM64:UAPI: Set the correct __BITS_PER_LONG for ILP32.
>>  Allow for some signal structures to be the same between a 32bit ABI
>>and the 64bit ABI.
>>  ARM64:ILP32: Use the same size and layout of the signal structures
>>for ILP32 as for LP64.
>>  Allow a 32bit ABI to use the naming of the 64bit ABI syscalls to
>>avoid confusion of not splitting the registers.
>>  ARM64:ILP32: Use the same syscall names as LP64.
>>  ARM64: Introduce is_a32_task and is_a32_thread. Use them in the
>>correct locations.
>>  ARM64: Add ARM64_ILP32 to Kconfig.
> 
> Does this patch need to be in the middle of the series? It should come
> after the ILP32 support is complete.


Ok, I will move it. I had added in the middle to test the newly added code. 

Thanks,
Andrew

> 
> Thanks.
> 
> -- 
> Catalin
--
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: [PATCHv2 00/24] ILP32 Support in ARM64

[Pinski, Andrew]

 On Jun 16, 2014, at 10:08 AM, Catalin Marinas catalin.mari...@arm.com 
 wrote:
 
 Andrew,
 
 On Sat, May 24, 2014 at 12:01:55AM -0700, Andrew Pinski wrote:
 New version of the patches with documentation, signal changes are
 simplified, using less compat syscalls and splitting up the patches so
 it is easier to review.  I have tested LTP on both LP64 and ILP32.
 There is a few LTP failures but they are due to LTP being incorrect
 (sigaction structure in glibc is not the one which is used by the
 kernel)
 
 Do you have more details about what's wrong here and where the fix
 should go? LTP? glibc? Kernel?

LTP assumes some sigaction structure is the same between userland and kernel.
Glibc has the correct idea of what the kernel structure will be when passing to 
the kernel already. The fix should be done in LTP. There is already code in LTP 
for x86 for a similar issue which we should be able to advantage of. 

 
 I'll give you more specific comments on the code in the next couple of
 days. But for cosmetics, please wrap the lines around 72 (or whatever)
 characters both in emails, commit logs and Documentation/* files (and
 you can drop the Thanks part in every commit log ;)).

Will do this with the rest of the review. 

 
 Andrew Pinski (24):
  ARM64: Force LP64 to compile the kernel.
  ARM64: Rename COMPAT to AARCH32_EL0 in Kconfig.
  ARM64: Change some CONFIG_COMPAT over to use CONFIG_AARCH32_EL0
instead.
  ARM64:ILP32: Set kernel_long to long long so we can reuse most of the
same syscalls as LP64.
  ARM64:UAPI: Set the correct __BITS_PER_LONG for ILP32.
  Allow for some signal structures to be the same between a 32bit ABI
and the 64bit ABI.
  ARM64:ILP32: Use the same size and layout of the signal structures
for ILP32 as for LP64.
  Allow a 32bit ABI to use the naming of the 64bit ABI syscalls to
avoid confusion of not splitting the registers.
  ARM64:ILP32: Use the same syscall names as LP64.
  ARM64: Introduce is_a32_task and is_a32_thread. Use them in the
correct locations.
  ARM64: Add ARM64_ILP32 to Kconfig.
 
 Does this patch need to be in the middle of the series? It should come
 after the ILP32 support is complete.


Ok, I will move it. I had added in the middle to test the newly added code. 

Thanks,
Andrew

 
 Thanks.
 
 -- 
 Catalin
--
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 00/28] nios2 Linux kernel port

[Pinski, Andrew]

> On Apr 24, 2014, at 11:06 PM, "Chung-Lin Tang"  
> wrote:
> 
>> On 2014/4/25 02:42 AM, Pinski, Andrew wrote:
>> 
>> 
>>>> On Apr 24, 2014, at 11:37 AM, "Chung-Lin Tang"  
>>>> wrote:
>>>> 
>>>>> On 2014/4/24 11:28 PM, Catalin Marinas wrote:
>>>>>> On Thu, Apr 24, 2014 at 09:55:25AM +0100, Chung-Lin Tang wrote:
>>>>>>> On 2014/4/24 02:26 PM, Chung-Lin Tang wrote:
>>>>>>> On 2014/4/24 上午 02:15, Pinski, Andrew wrote:
>>>>>>> 
>>>>>>>>> On Apr 23, 2014, at 10:59 AM, "Chung-Lin Tang" 
>>>>>>>>>  wrote:
>>>>>>>>> 
>>>>>>>>>>> On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
>>>>>>>>>>> On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann  
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>> On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
>>>>>>>>>>>>>>>>>>> Hi Arnd and Peter Anvin,
>>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>>>> Other than 64-bit time_t, clock_t and suseconds_t, can you 
>>>>>>>>>>>>>>>>>>> confirm
>>>>>>>>>>>>>>>>>>> that we don't need to have 64 bit off_t? See detail in link 
>>>>>>>>>>>>>>>>>>> below.
>>>>>>>>>>>>>>>>>>> I can submit the patches for 64-bit time changes
>>>>>>>>>>>>>>>>>>> (include/asm-generic/posix_types.h and other archs) if 
>>>>>>>>>>>>>>>>>>> everyone is
>>>>>>>>>>>>>>>>>>> agreed on this.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Yes.
>>>>>>>>>>> Okay, will doing that.
>>>>>>>>> 
>>>>>>>>> I believe that arm64 ILP32 will also be affected. What is the status 
>>>>>>>>> of
>>>>>>>>> this configuration? Has the glibc/kernel ABI been finalized?
>>>>>>> Not yet.  I am still working out the signal handling part. But we
>>>>>>> already agreed on 64bit time_t, clock_t, and suseconds_t.  And we
>>>>>>> agreed to a 64bit offset_t too. 
>>>>>>> 
>>>>>>> On a related note suseconds in the timespec in posix is defined to
>>>>>>> be long. So it would nice if the kernel ignores the upper 32bits so
>>>>>>> we (glibc developers) can fix this for new targets including x32
>>>>>>> and arm64/ilp32.
>>>>>> 
>>>>>> Hmm, but that means for purely 32-bit architectures like nios2, which
>>>>>> unlike x86_64 or arm64, never has a 64-bit mode, suseconds_t as a 64-bit
>>>>>> type in the kernel is simply wasted.
>>>>> 
>>>>> The more I think of this, the more I feel that suseconds_t should jsut
>>>>> be 'long', not strictly 64-bitified. An ILP32 sub-mode in a 64-bit
>>>>> kernel should be using compat_* code paths, something like a
>>>>> COMPAT_USE_32BIT_SUSECONDS case.
>>>> 
>>>> ILP32 mode should use LP64 syscalls as much as possible and that's the
>>>> aim with arm64 as well (of course, we still have a few that wouldn't be
>>>> possible and we route them via compat).
>>>> 
>>>> But here if time_t is 64-bit while susecconds_t is 32-bit, the compat
>>>> code wouldn't help.
>>> 
>>> Why not? You can define the arm64 'struct compat_timeval' with
>>> suseconds_t as s32, and add the 32<-->64 case in the
>>> compat_get/put_timeval path, triggered when the process is ILP32 (test
>>> wrapped in the above hypothetical COMPAT_USE_32BIT_SUSECONDS macro).
>>> Similar to how x32 does COMPAT_USE_64BIT_TIME.
>> 
>> We would three timeval then. One for aarch32, one for lp64 and one for 
>> ilp32. We really don't want three. Two is already one too many in my mind 
>> after developing the ilp32 syscall abi. 
>> 
>> Thanks,
>> Andrew
> 
> Okay I now see you'

Re: [PATCH 00/28] nios2 Linux kernel port

[Pinski, Andrew]

 On Apr 24, 2014, at 11:06 PM, Chung-Lin Tang clt...@codesourcery.com 
 wrote:
 
 On 2014/4/25 02:42 AM, Pinski, Andrew wrote:
 
 
 On Apr 24, 2014, at 11:37 AM, Chung-Lin Tang clt...@codesourcery.com 
 wrote:
 
 On 2014/4/24 11:28 PM, Catalin Marinas wrote:
 On Thu, Apr 24, 2014 at 09:55:25AM +0100, Chung-Lin Tang wrote:
 On 2014/4/24 02:26 PM, Chung-Lin Tang wrote:
 On 2014/4/24 上午 02:15, Pinski, Andrew wrote:
 
 On Apr 23, 2014, at 10:59 AM, Chung-Lin Tang 
 clt...@codesourcery.com wrote:
 
 On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
 On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann a...@arndb.de 
 wrote:
 On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
 Hi Arnd and Peter Anvin,
 
 Other than 64-bit time_t, clock_t and suseconds_t, can you 
 confirm
 that we don't need to have 64 bit off_t? See detail in link 
 below.
 I can submit the patches for 64-bit time changes
 (include/asm-generic/posix_types.h and other archs) if 
 everyone is
 agreed on this.
 
 Yes.
 Okay, will doing that.
 
 I believe that arm64 ILP32 will also be affected. What is the status 
 of
 this configuration? Has the glibc/kernel ABI been finalized?
 Not yet.  I am still working out the signal handling part. But we
 already agreed on 64bit time_t, clock_t, and suseconds_t.  And we
 agreed to a 64bit offset_t too. 
 
 On a related note suseconds in the timespec in posix is defined to
 be long. So it would nice if the kernel ignores the upper 32bits so
 we (glibc developers) can fix this for new targets including x32
 and arm64/ilp32.
 
 Hmm, but that means for purely 32-bit architectures like nios2, which
 unlike x86_64 or arm64, never has a 64-bit mode, suseconds_t as a 64-bit
 type in the kernel is simply wasted.
 
 The more I think of this, the more I feel that suseconds_t should jsut
 be 'long', not strictly 64-bitified. An ILP32 sub-mode in a 64-bit
 kernel should be using compat_* code paths, something like a
 COMPAT_USE_32BIT_SUSECONDS case.
 
 ILP32 mode should use LP64 syscalls as much as possible and that's the
 aim with arm64 as well (of course, we still have a few that wouldn't be
 possible and we route them via compat).
 
 But here if time_t is 64-bit while susecconds_t is 32-bit, the compat
 code wouldn't help.
 
 Why not? You can define the arm64 'struct compat_timeval' with
 suseconds_t as s32, and add the 32--64 case in the
 compat_get/put_timeval path, triggered when the process is ILP32 (test
 wrapped in the above hypothetical COMPAT_USE_32BIT_SUSECONDS macro).
 Similar to how x32 does COMPAT_USE_64BIT_TIME.
 
 We would three timeval then. One for aarch32, one for lp64 and one for 
 ilp32. We really don't want three. Two is already one too many in my mind 
 after developing the ilp32 syscall abi. 
 
 Thanks,
 Andrew
 
 Okay I now see you're already doing that for 32-bit ARM.
 
 Still, you would probably just need to have an arm64-ILP32 specific case
 to be careful about the last padding word upon kernel entry/exit.
 (accommodating the difference in sizeof(long))  Penalizing all
 architectures does not seem like the best solution.

Considering the alignment of long long would be 64bits, the struct does not 
change sizes if suseconds_t is 32bits or 64bits. 

 
 Having suseconds_t as a strictly 64-bit C type in the kernel, while
 defined as = long in user-space may cause other problems.
 
 I'll try to explain a probable situation for Nios II. I'm not sure about
 other soft-cores, but nios2 is sort of uncommon in that the maximum
 alignment is 4-bytes (32-bits), even for doubles/long-longs.

Yes does that include even if users of aligned?  If so that seems broken.  Also 
yes nios ii is unstandard when it comes to alignment here. 

 
 So if time_t is 64-bits (which makes sense), then struct timeval, which
 is time_t+suseconds_t in userspace is 12-bytes/aligned-4 (unlike many
 archs where a 64-bit time_t will expand the size to 16-bytes, due to
 align-8)

Unlike most other targets where the struct would 16bits no matter what. 

Thanks,
Andrew


 
 If the kernel suseconds_t is forced to be 64-bits, then nios2 will have
 a 16-byte kernel timeval vs. 12-byte userspace timeval situation. Just
 this will require us to do something using compat_*, or weird hacks in
 glibc, which is unfair. Nios II has no other-mode. We are just
 strictly ILP32, everywhere.
 
 Of course, we can probably still at the end just use a Nios II specific
 posix_types.h header to override things, but I'm just stating this as a
 matter of which are the most reasonable default settings in the generic
 headers. Making pure ILP32 archs diverge from POSIX standards by default
 does not seem to be right.
 
 Thanks,
 Chung-Lin
 
--
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 00/28] nios2 Linux kernel port

[Pinski, Andrew]

> On Apr 24, 2014, at 11:37 AM, "Chung-Lin Tang"  
> wrote:
> 
>> On 2014/4/24 11:28 PM, Catalin Marinas wrote:
>>> On Thu, Apr 24, 2014 at 09:55:25AM +0100, Chung-Lin Tang wrote:
>>>> On 2014/4/24 02:26 PM, Chung-Lin Tang wrote:
>>>>> On 2014/4/24 上午 02:15, Pinski, Andrew wrote:
>>>>> 
>>>>>>> On Apr 23, 2014, at 10:59 AM, "Chung-Lin Tang" 
>>>>>>>  wrote:
>>>>>>> 
>>>>>>>>> On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
>>>>>>>>> On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann  wrote:
>>>>>>>>>>>>> On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
>>>>>>>>>>>>>>>>> Hi Arnd and Peter Anvin,
>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>>> Other than 64-bit time_t, clock_t and suseconds_t, can you 
>>>>>>>>>>>>>>>>> confirm
>>>>>>>>>>>>>>>>> that we don't need to have 64 bit off_t? See detail in link 
>>>>>>>>>>>>>>>>> below.
>>>>>>>>>>>>>>>>> I can submit the patches for 64-bit time changes
>>>>>>>>>>>>>>>>> (include/asm-generic/posix_types.h and other archs) if 
>>>>>>>>>>>>>>>>> everyone is
>>>>>>>>>>>>>>>>> agreed on this.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Yes.
>>>>>>>>> Okay, will doing that.
>>>>>>> 
>>>>>>> I believe that arm64 ILP32 will also be affected. What is the status of
>>>>>>> this configuration? Has the glibc/kernel ABI been finalized?
>>>>> Not yet.  I am still working out the signal handling part. But we
>>>>> already agreed on 64bit time_t, clock_t, and suseconds_t.  And we
>>>>> agreed to a 64bit offset_t too. 
>>>>> 
>>>>> On a related note suseconds in the timespec in posix is defined to
>>>>> be long. So it would nice if the kernel ignores the upper 32bits so
>>>>> we (glibc developers) can fix this for new targets including x32
>>>>> and arm64/ilp32.
>>>> 
>>>> Hmm, but that means for purely 32-bit architectures like nios2, which
>>>> unlike x86_64 or arm64, never has a 64-bit mode, suseconds_t as a 64-bit
>>>> type in the kernel is simply wasted.
>>> 
>>> The more I think of this, the more I feel that suseconds_t should jsut
>>> be 'long', not strictly 64-bitified. An ILP32 sub-mode in a 64-bit
>>> kernel should be using compat_* code paths, something like a
>>> COMPAT_USE_32BIT_SUSECONDS case.
>> 
>> ILP32 mode should use LP64 syscalls as much as possible and that's the
>> aim with arm64 as well (of course, we still have a few that wouldn't be
>> possible and we route them via compat).
>> 
>> But here if time_t is 64-bit while susecconds_t is 32-bit, the compat
>> code wouldn't help.
> 
> Why not? You can define the arm64 'struct compat_timeval' with
> suseconds_t as s32, and add the 32<-->64 case in the
> compat_get/put_timeval path, triggered when the process is ILP32 (test
> wrapped in the above hypothetical COMPAT_USE_32BIT_SUSECONDS macro).
> Similar to how x32 does COMPAT_USE_64BIT_TIME.

We would three timeval then. One for aarch32, one for lp64 and one for ilp32. 
We really don't want three. Two is already one too many in my mind after 
developing the ilp32 syscall abi. 

Thanks,
Andrew


> 
> Chung-Lin
> 
> 
> 
> 
> 
--
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 00/28] nios2 Linux kernel port

[Pinski, Andrew]

 On Apr 24, 2014, at 11:37 AM, Chung-Lin Tang clt...@codesourcery.com 
 wrote:
 
 On 2014/4/24 11:28 PM, Catalin Marinas wrote:
 On Thu, Apr 24, 2014 at 09:55:25AM +0100, Chung-Lin Tang wrote:
 On 2014/4/24 02:26 PM, Chung-Lin Tang wrote:
 On 2014/4/24 上午 02:15, Pinski, Andrew wrote:
 
 On Apr 23, 2014, at 10:59 AM, Chung-Lin Tang 
 clt...@codesourcery.com wrote:
 
 On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
 On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann a...@arndb.de wrote:
 On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
 Hi Arnd and Peter Anvin,
 
 Other than 64-bit time_t, clock_t and suseconds_t, can you 
 confirm
 that we don't need to have 64 bit off_t? See detail in link 
 below.
 I can submit the patches for 64-bit time changes
 (include/asm-generic/posix_types.h and other archs) if 
 everyone is
 agreed on this.
 
 Yes.
 Okay, will doing that.
 
 I believe that arm64 ILP32 will also be affected. What is the status of
 this configuration? Has the glibc/kernel ABI been finalized?
 Not yet.  I am still working out the signal handling part. But we
 already agreed on 64bit time_t, clock_t, and suseconds_t.  And we
 agreed to a 64bit offset_t too. 
 
 On a related note suseconds in the timespec in posix is defined to
 be long. So it would nice if the kernel ignores the upper 32bits so
 we (glibc developers) can fix this for new targets including x32
 and arm64/ilp32.
 
 Hmm, but that means for purely 32-bit architectures like nios2, which
 unlike x86_64 or arm64, never has a 64-bit mode, suseconds_t as a 64-bit
 type in the kernel is simply wasted.
 
 The more I think of this, the more I feel that suseconds_t should jsut
 be 'long', not strictly 64-bitified. An ILP32 sub-mode in a 64-bit
 kernel should be using compat_* code paths, something like a
 COMPAT_USE_32BIT_SUSECONDS case.
 
 ILP32 mode should use LP64 syscalls as much as possible and that's the
 aim with arm64 as well (of course, we still have a few that wouldn't be
 possible and we route them via compat).
 
 But here if time_t is 64-bit while susecconds_t is 32-bit, the compat
 code wouldn't help.
 
 Why not? You can define the arm64 'struct compat_timeval' with
 suseconds_t as s32, and add the 32--64 case in the
 compat_get/put_timeval path, triggered when the process is ILP32 (test
 wrapped in the above hypothetical COMPAT_USE_32BIT_SUSECONDS macro).
 Similar to how x32 does COMPAT_USE_64BIT_TIME.

We would three timeval then. One for aarch32, one for lp64 and one for ilp32. 
We really don't want three. Two is already one too many in my mind after 
developing the ilp32 syscall abi. 

Thanks,
Andrew


 
 Chung-Lin
 
 
 
 
 
--
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 00/28] nios2 Linux kernel port

[Pinski, Andrew]

> On Apr 23, 2014, at 10:59 AM, "Chung-Lin Tang"  
> wrote:
> 
>> On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
>> On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann  wrote:
 On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
>> Hi Arnd and Peter Anvin,
>> 
>> Other than 64-bit time_t, clock_t and suseconds_t, can you confirm
>> that we don't need to have 64 bit off_t? See detail in link below.
>> I can submit the patches for 64-bit time changes
>> (include/asm-generic/posix_types.h and other archs) if everyone is
>> agreed on this.
 
 Yes.
>> Okay, will doing that.
> 
> I believe that arm64 ILP32 will also be affected. What is the status of
> this configuration? Has the glibc/kernel ABI been finalized?

Not yet.  I am still working out the signal handling part. But we already 
agreed on 64bit time_t, clock_t, and suseconds_t.  And we agreed to a 64bit 
offset_t too. 

On a related note suseconds in the timespec in posix is defined to be long. So 
it would nice if the kernel ignores the upper 32bits so we (glibc developers) 
can fix this for new targets including x32 and arm64/ilp32. 

Thanks,
Andrew

> 
> Thanks,
> Chung-Lin
> 
--
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 00/28] nios2 Linux kernel port

[Pinski, Andrew]

 On Apr 23, 2014, at 10:59 AM, Chung-Lin Tang clt...@codesourcery.com 
 wrote:
 
 On 2014/4/22 07:20 PM, Ley Foon Tan wrote:
 On Tue, Apr 22, 2014 at 6:56 PM, Arnd Bergmann a...@arndb.de wrote:
 On Tuesday 22 April 2014 18:37:11 Ley Foon Tan wrote:
 Hi Arnd and Peter Anvin,
 
 Other than 64-bit time_t, clock_t and suseconds_t, can you confirm
 that we don't need to have 64 bit off_t? See detail in link below.
 I can submit the patches for 64-bit time changes
 (include/asm-generic/posix_types.h and other archs) if everyone is
 agreed on this.
 
 Yes.
 Okay, will doing that.
 
 I believe that arm64 ILP32 will also be affected. What is the status of
 this configuration? Has the glibc/kernel ABI been finalized?

Not yet.  I am still working out the signal handling part. But we already 
agreed on 64bit time_t, clock_t, and suseconds_t.  And we agreed to a 64bit 
offset_t too. 

On a related note suseconds in the timespec in posix is defined to be long. So 
it would nice if the kernel ignores the upper 32bits so we (glibc developers) 
can fix this for new targets including x32 and arm64/ilp32. 

Thanks,
Andrew

 
 Thanks,
 Chung-Lin
 
--
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: Issue with BUG() in asm-gemeric/bug.h if CONFIG_BUG=n

[Pinski, Andrew]

> On Sep 30, 2013, at 9:20 AM, "David Daney"  wrote:

> 
>> On 09/30/2013 07:56 AM, Ralf Baechle wrote:
>> Lately I received several patches for build issues that only strike if
>> CONFIG_BUG is disabled.  Here's a test case extracted from one of them:
>> 
>> /*
>>  * Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case
>>  */
>> #define BUG()do {} while(0)
>> 
>> int foo(int arg)
>> {
>>int res;
>> 
>>if (arg == 1)
>>res = 23;
>>else if (arg == 2)
>>res = 42;
>>else
>>BUG();
>> 
>>return res;
>> }
>> 
>> [ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
>> bug.c: In function ‘foo’:
>> bug.c:17:2: warning: ‘res’ may be used uninitialized in this function 
>> [-Wmaybe-uninitialized]
>>   return res;
>>   ^
>> 
>> It's fairly obvious to see what's happening here - GCC doesn't know that
>> the else case can not be reached, thus razorsharply concludes that res
>> may be used uninitialized.
>> 
>> There several locations where MIPS - possibly other architectures as well -
>> is affected by this.
>> 
>> I think the definition of BUG should be changed to something like
>> 
>> #define BUG()unreachable()
>> 16304
>> unreachable() will depending on the compiler being used, expand either
>> into a call to __builtin_unreachable() or where that function is
>> unavailable, into do {} while (1).
> 
> The *only* reason we have CONFIG_BUG=n is to reduce code size.
> 
> Sticking in that empty loop, negates the entire point.
> 
> IMHO: We should do one of:
> o Make CONFIG_BUG=y mandatory
> o Ignore the warnings.
> o Fix the warning sites so they quit Warning.
> 
> So I don't think the patch is really an improvement over the status quo.

What about using __builtin_unreachable when we can but turn off warnings and 
use do{}while(0) when __builtin_unreachable does not exist?  This seems the 
both worlds.  Newer compilers produce better code with unreachable anyways.

Thanks,
Andrew


> 
> David Daney
>> 
>> __builtin_unreachable() was introduce for GCC 4.5.0.
>> 
>> This means there'd be minor bloat for antique compilers - but probably
>> even better code generation for compilers supporting __builtin_unreachable().
>> 
>>   Ralf
>> 
>> Signed-off-by: Ralf Baechle 
>> 
>>  include/asm-generic/bug.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 7d10f96..6f78771 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const 
>> int line);
>> 
>>  #else /* !CONFIG_BUG */
>>  #ifndef HAVE_ARCH_BUG
>> -#define BUG() do {} while(0)
>> +#define BUG() unreachable()
>>  #endif
>> 
>>  #ifndef HAVE_ARCH_BUG_ON
>> 
>> - End forwarded message -
>> 
>>   Ralf
> 
> 
--
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: Issue with BUG() in asm-gemeric/bug.h if CONFIG_BUG=n

[Pinski, Andrew]

 On Sep 30, 2013, at 9:20 AM, David Daney ddaney.c...@gmail.com wrote:

 
 On 09/30/2013 07:56 AM, Ralf Baechle wrote:
 Lately I received several patches for build issues that only strike if
 CONFIG_BUG is disabled.  Here's a test case extracted from one of them:
 
 /*
  * Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case
  */
 #define BUG()do {} while(0)
 
 int foo(int arg)
 {
int res;
 
if (arg == 1)
res = 23;
else if (arg == 2)
res = 42;
else
BUG();
 
return res;
 }
 
 [ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
 bug.c: In function ‘foo’:
 bug.c:17:2: warning: ‘res’ may be used uninitialized in this function 
 [-Wmaybe-uninitialized]
   return res;
   ^
 
 It's fairly obvious to see what's happening here - GCC doesn't know that
 the else case can not be reached, thus razorsharply concludes that res
 may be used uninitialized.
 
 There several locations where MIPS - possibly other architectures as well -
 is affected by this.
 
 I think the definition of BUG should be changed to something like
 
 #define BUG()unreachable()
 16304
 unreachable() will depending on the compiler being used, expand either
 into a call to __builtin_unreachable() or where that function is
 unavailable, into do {} while (1).
 
 The *only* reason we have CONFIG_BUG=n is to reduce code size.
 
 Sticking in that empty loop, negates the entire point.
 
 IMHO: We should do one of:
 o Make CONFIG_BUG=y mandatory
 o Ignore the warnings.
 o Fix the warning sites so they quit Warning.
 
 So I don't think the patch is really an improvement over the status quo.

What about using __builtin_unreachable when we can but turn off warnings and 
use do{}while(0) when __builtin_unreachable does not exist?  This seems the 
both worlds.  Newer compilers produce better code with unreachable anyways.

Thanks,
Andrew


 
 David Daney
 
 __builtin_unreachable() was introduce for GCC 4.5.0.
 
 This means there'd be minor bloat for antique compilers - but probably
 even better code generation for compilers supporting __builtin_unreachable().
 
   Ralf
 
 Signed-off-by: Ralf Baechle r...@linux-mips.org
 
  include/asm-generic/bug.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
 index 7d10f96..6f78771 100644
 --- a/include/asm-generic/bug.h
 +++ b/include/asm-generic/bug.h
 @@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const 
 int line);
 
  #else /* !CONFIG_BUG */
  #ifndef HAVE_ARCH_BUG
 -#define BUG() do {} while(0)
 +#define BUG() unreachable()
  #endif
 
  #ifndef HAVE_ARCH_BUG_ON
 
 - End forwarded message -
 
   Ralf
 
 
--
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/