Re: [PATCH] uapi: fix asm/signal.h userspace compilation errors

2017-03-06 Thread Carlos O'Donell 
On Fri, Mar 3, 2017 at 8:23 PM, Carlos O'Donell <car...@systemhalted.org> wrote:
> On Thu, Mar 2, 2017 at 10:48 AM, Dmitry V. Levin <l...@altlinux.org> wrote:
>> On Thu, Mar 02, 2017 at 10:22:18AM -0500, Carlos O'Donell wrote:
>>> On Wed, Mar 1, 2017 at 11:20 AM, Arnd Bergmann <a...@arndb.de> wrote:
>>> > On Sun, Feb 26, 2017 at 2:01 AM, Dmitry V. Levin <l...@altlinux.org> 
>>> > wrote:
>>> >> Include  (guarded by #ifndef __KERNEL__) to fix asm/signal.h
>>> >> userspace compilation errors like this:
>>> >>
>>> >> /usr/include/asm/signal.h:126:2: error: unknown type name 'size_t'
>>> >>   size_t ss_size;
>>> >>
>>> >> As no uapi header provides a definition of size_t, inclusion
>>> >> of  seems to be the most conservative fix available.
>> [...]
>>> > I'm not sure if this is the best fix. We generally should not include one
>>> > standard header from another standard header. Would it be possible
>>> > to use __kernel_size_t instead of size_t?
>>>
>>> In glibc we handle this with special use of __need_size_t with GCC's
>>> provided stddef.h.
>>>
>>> For example glibc's signal.h does this:
>>>
>>> # define __need_size_t
>>> # include 
>>
>> Just to make it clear, do you suggest this approach for asm/signal.h as well?

The best practice from the glibc community looks like this:

(a) Create a bits/types/*.h header for the type you need.

e.g.
./time/bits/types/struct_timeval.h
./time/bits/types/struct_itimerspec.h
./time/bits/types/time_t.h
./time/bits/types/struct_timespec.h
./time/bits/types/struct_tm.h
./time/bits/types/clockid_t.h
./time/bits/types/clock_t.h
./time/bits/types/timer_t.h

(b) If neccessary the bits/types/*.h header is a wrapper:
~~~
#define __need_size_t
#include 
~~~
to get access to the compiler provided type.

This way all of the code you need simplifies to includes for the types you need.

e.g.

time/sys/time.h:
...
#include 
#include 
#include 
...

This is what we've been doing in glibc starting last September as we
cleaned up all the convoluted conditional logic to get the types we
needed in the headers that needed them.

Cheers,
Carlos.

Re: [PATCH] uapi: fix asm/signal.h userspace compilation errors

2017-03-03 Thread Carlos O'Donell 
On Thu, Mar 2, 2017 at 10:48 AM, Dmitry V. Levin <l...@altlinux.org> wrote:
> On Thu, Mar 02, 2017 at 10:22:18AM -0500, Carlos O'Donell wrote:
>> On Wed, Mar 1, 2017 at 11:20 AM, Arnd Bergmann <a...@arndb.de> wrote:
>> > On Sun, Feb 26, 2017 at 2:01 AM, Dmitry V. Levin <l...@altlinux.org> wrote:
>> >> Include  (guarded by #ifndef __KERNEL__) to fix asm/signal.h
>> >> userspace compilation errors like this:
>> >>
>> >> /usr/include/asm/signal.h:126:2: error: unknown type name 'size_t'
>> >>   size_t ss_size;
>> >>
>> >> As no uapi header provides a definition of size_t, inclusion
>> >> of  seems to be the most conservative fix available.
> [...]
>> > I'm not sure if this is the best fix. We generally should not include one
>> > standard header from another standard header. Would it be possible
>> > to use __kernel_size_t instead of size_t?
>>
>> In glibc we handle this with special use of __need_size_t with GCC's
>> provided stddef.h.
>>
>> For example glibc's signal.h does this:
>>
>> # define __need_size_t
>> # include 
>
> Just to make it clear, do you suggest this approach for asm/signal.h as well?

The kernel is duplicating userspace headers in the UAPI implementation
and running into exactly the same problems we have already solved in
userspace. We currently have no better solution than the "__need_*"
interface for avoiding the duplication of the type definitions and the
problems that come with that.

I am taking this up with senior glibc developers on libc-alpha to see
if we have a better suggestion. If you give me 72 hours I'll either
have a better suggestion or the acknowledgement that my suggestion is
the best practical solution we have.

Note that in a GNU userspace stddef.h here comes from gcc, which
completes the implementation of the C development environment that
provides the types you need. The use of "__need_size_t" is a collusion
between the components of the implementation and use by the Linux
kernel would mean expecting something specific to a GNU
implementation.

I might suggest you use include/uapi/linux/libc-compat.h in an attempt
to abstract away the GNU-specific magic for getting just size_t from
stddef.h. That way you have documented the places that other runtime
authors need to fill out for things to work.

Cheers,
Carlos.

Re: [PATCH] uapi: fix asm/signal.h userspace compilation errors

2017-03-02 Thread Carlos O'Donell 
On Wed, Mar 1, 2017 at 11:20 AM, Arnd Bergmann  wrote:
> On Sun, Feb 26, 2017 at 2:01 AM, Dmitry V. Levin  wrote:
>> Include  (guarded by #ifndef __KERNEL__) to fix asm/signal.h
>> userspace compilation errors like this:
>>
>> /usr/include/asm/signal.h:126:2: error: unknown type name 'size_t'
>>   size_t ss_size;
>>
>> As no uapi header provides a definition of size_t, inclusion
>> of  seems to be the most conservative fix available.
>>
>> On the kernel side size_t is typedef'ed to __kernel_size_t, so
>> an alternative fix would be to change the type of sigaltstack.ss_size
>> from size_t to __kernel_size_t for all architectures except those where
>> sizeof(size_t) < sizeof(__kernel_size_t), namely, x32 and mips n32.
>>
>> On x32 and mips n32, however, #include  seems to be the most
>> straightforward way to obtain the definition for sigaltstack.ss_size's
>> type.
>>
>> Signed-off-by: Dmitry V. Levin 
>
> I'm not sure if this is the best fix. We generally should not include one
> standard header from another standard header. Would it be possible
> to use __kernel_size_t instead of size_t?

In glibc we handle this with special use of __need_size_t with GCC's
provided stddef.h.

For example glibc's signal.h does this:

# define __need_size_t
# include 

And...

/* Any one of these symbols __need_* means that GNU libc
   wants us just to define one data type.  So don't define
   the symbols that indicate this file's entire job has been done.  */
#if (!defined(__need_wchar_t) && !defined(__need_size_t)\
 && !defined(__need_ptrdiff_t) && !defined(__need_NULL) \
 && !defined(__need_wint_t))

The idea being that the type you want is really defined by stddef.h,
but you just one the one type.

Changing the fundamental type causes the issues you see in patch v2
where sizeof(size_t) < sizeof(__kernel_size_t).

It will only lead to problem substituting the wrong type.

Cheers,
Carlos.

Re: [PATCH] Add hwcap2 bits for POWER9

2016-01-12 Thread Carlos O'Donell 
On 01/12/2016 11:39 AM, Steven Munroe wrote:
>> That's the rule. There are no other discussions to be had.
>>
> Well is was posted to to powerpc next:
> https://git.kernel.org/powerpc/c/e708c24cd01ce80b1609d8bacc
> 
> We have agreement between the kernel and GLIBC (and the ABI). 
> 
> The issue is just coordination across communities and individuals that
> may not being paying attention to other communities dead lines.
> 
> Have you ever tried to push a string, up hill. That is open source
> development in nutshell. ;)

I know exactly what this is like.

> So it is in flight and glibc is soft/slush freeze. I would hate to
> revert this one day just to add it back to the next. Especially if those
> days straddle the hard freeze ...
> 
> So can we let this ride a day or too?

Sure. I'm not an unreasonable person.

My goal as a glibc steward is to remind IBM that our best practice is that
we *wait* until it goes into mainline before committing to glibc master.

There really isn't any reason to check this in to glibc master right now.
It could wait.

Adhemerval as a release manager is also not an unreasonable person.
I have already discussed with Tulio that he should have just waited to
commit these changes, but gotten an exception from Adhemerval to checkin
the fairly low-risk patches late in the freeze. That's exactly the purpose
of a release managers job, to grant you exceptions as we approach release,
particularly when schedules don't quite line up.

Cheers,
Carlos.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Add hwcap2 bits for POWER9

2016-01-11 Thread Carlos O'Donell 
On 01/11/2016 02:55 PM, Tulio Magno Quites Machado Filho wrote:
> "Carlos O'Donell" <car...@redhat.com> writes:
> 
>> On 01/11/2016 10:16 AM, Tulio Magno Quites Machado Filho wrote:
>>> Adhemerval Zanella <adhemerval.zane...@linaro.org> writes:
>>>
>>>> On 08-01-2016 13:36, Peter Bergner wrote:
>>>>> On Fri, 2016-01-08 at 11:25 -0200, Tulio Magno Quites Machado Filho wrote:
>>>>>> Peter, this solves the issue you reported previously [1].
>>>>>>
>>>>>> [1] https://sourceware.org/ml/libc-alpha/2015-12/msg00522.html
>>>>>
>>>>> Agreed, thanks.  I'll also add the POWER9 support to the GCC side
>>>>> of the patch now that the glibc code is upstream.
>>>>
>>>> I do not see these bits being added in kernel side yet and GLIBC usual
>>>> only sync these kind of bits *after* they are included in kernel side.
>>>> So I would advise to either get these pieces (kernel support and hwcap
>>>> advertise) in kernel before 2.23 release, otherwise revert the patches.
>>>
>>> Ack.
>>> It has just been sent to the correspondent Linux mailing list:
>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-January/137763.html
>>
>> Please revert the changes from glibc until you checkin support to linux
>> kernel mainline.
>>
>> Leaving these bits in increases the risk that someone uses to deploy a glibc
>> that then may have the wrong value.
> 
> Could you clarify this statement, please?
> I fail to see how they could have the wrong value.

Until it is checked into the mainline kernel it is not canonical.

That's the rule. There are no other discussions to be had.

The single rule avoids discussions like "it can never be wrong because that's
what our ABI says it is."
 
> However, I do agree with the concerns raised by Peter and Adhemerval: glibc
> should be in sync with the kernel by the time of the release in order to
> guarantee both bits are reserved for the exact same goal and we should have
> both AT_HWCAP and AT_PLATFORM supporting the new processor.
> With that said, I was planning to revert both commits d2de9ef7 and b1f19b8e
> if we don't get the kernel patch accepted into the powerpc tree in time for
> the release 2.23.

Exactly. That's perfect. We can backport them to 2.23.1 if you get in later.

Cheers,
Carlos.
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Add hwcap2 bits for POWER9

2016-01-11 Thread Carlos O'Donell 
On 01/11/2016 10:16 AM, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella  writes:
> 
>> On 08-01-2016 13:36, Peter Bergner wrote:
>>> On Fri, 2016-01-08 at 11:25 -0200, Tulio Magno Quites Machado Filho wrote:
 Peter, this solves the issue you reported previously [1].

 [1] https://sourceware.org/ml/libc-alpha/2015-12/msg00522.html
>>>
>>> Agreed, thanks.  I'll also add the POWER9 support to the GCC side
>>> of the patch now that the glibc code is upstream.
>>
>> I do not see these bits being added in kernel side yet and GLIBC usual
>> only sync these kind of bits *after* they are included in kernel side.
>> So I would advise to either get these pieces (kernel support and hwcap
>> advertise) in kernel before 2.23 release, otherwise revert the patches.
> 
> Ack.
> It has just been sent to the correspondent Linux mailing list:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-January/137763.html

Please revert the changes from glibc until you checkin support to linux
kernel mainline.

Leaving these bits in increases the risk that someone uses to deploy a glibc
that then may have the wrong value.

Cheers,
Carlos.
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

2013-11-21 Thread Carlos O'Donell 
On 11/21/2013 06:33 AM, Michael Ellerman wrote:
 On Wed, Nov 20, 2013 at 04:18:54PM +1100, Michael Neuling wrote:
 The VSX MSR bit in the user context indicates if the context contains VSX
 state.  Currently we set this when the process has touched VSX at any stage.

 Unfortunately, if the user has not provided enough space to save the VSX 
 state,
 we can't save it but we currently still set the MSR VSX bit.

 This patch changes this to clear the MSR VSX bit when the user doesn't 
 provide
 enough space.  This indicates that there is no valid VSX state in the user
 context.

 This is needed to support get/set/make/swapcontext for applications that use
 VSX but only provide a small context.  For example, getcontext in glibc
 provides a smaller context since the VSX registers don't need to be saved 
 over
 the glibc function call.  But since the program calling getcontext may have
 used VSX, the kernel currently says the VSX state is valid when it's not.  If
 the returned context is then used in setcontext (ie. a small context without
 VSX but with MSR VSX set), the kernel will refuse the context.  This 
 situation
 has been reported by the glibc community.
 
 Broken since forever?

Yes, broken since forever. At least it was known in glibc 2.18 that this was
broken, but without an active distribution using it the defect wasn't analyzed.

 Tested-by: Haren Myneni ha...@linux.vnet.ibm.com
 Signed-off-by: Michael Neuling mi...@neuling.org
 Cc: sta...@vger.kernel.org
 ---
  arch/powerpc/kernel/signal_32.c | 10 +-
 
 What about the 64-bit code? I don't know the code but it appears at a glance 
 to
 have the same bug.
 
It doesn't happen with 64-bit code because there the context contains
a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
state. Therefore 64-bit ppc always has space to store the VMX registers
in a userspace context switch. It is only the 32-bit ppc ABI that lacks
the space.
 
 diff --git a/arch/powerpc/kernel/signal_32.c 
 b/arch/powerpc/kernel/signal_32.c
 index 749778e..1844298 100644
 --- a/arch/powerpc/kernel/signal_32.c
 +++ b/arch/powerpc/kernel/signal_32.c
 @@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct 
 mcontext __user *frame,
  if (copy_vsx_to_user(frame-mc_vsregs, current))
  return 1;
  msr |= MSR_VSX;
 -}
 +} else if (!ctx_has_vsx_region)
 +/*
 + * With a small context structure we can't hold the VSX
 + * registers, hence clear the MSR value to indicate the state
 + * was not saved.
 + */
 +msr = ~MSR_VSX;
 
 I think it'd be clearer if this was just the else case. The full context is:
 
 if (current-thread.used_vsr  ctx_has_vsx_region) {
 __giveup_vsx(current);
 if (copy_vsx_to_user(frame-mc_vsregs, current))
 return 1;
 msr |= MSR_VSX;
 +   } else if (!ctx_has_vsx_region)
 +   /*
 +* With a small context structure we can't hold the VSX
 +* registers, hence clear the MSR value to indicate the state
 +* was not saved.
 +*/
 +   msr = ~MSR_VSX;
 
 Which means if current-thread.user_vsr and ctx_has_vsx_region are both false
 we potentially leave MSR_VSX set in msr. I think it should be the case that
 MSR_VSX is only ever set if current-thread.used_vsr is true, so it should be
 OK in pratice, but it seems unnecessarily fragile.

If current-thread.user_vsr and ctx_has_vsx_region are both false then
!ctx_has_vsx_region is true and we clear MSR_VSX.

Perhaps you mean if current-thread.user_vsr is false, but ctx_has_vsx_region
is true? 

Previously the else clause reset MSR_VSX if:
1. current-thread.used_vsr == 0  ctx_has_vsx_region == 0
2. current-thread.used_vsr == 1  ctx_has_vsx_region == 0,

Now it resets MSR_VSX additionally for:
3. current-thread.used_vsr == 0  ctx_has_vsx_region == 1,

3. is a valid state. The task has not touched the VSX state and the context
is large enough to be saved into. This may be a future state for ppc32 if 
we adjust the ABI to have a large enough context buffer. However at present 
it's not a plausible state. It's also a don't care state since there is 
nothing saved in the context, and if nothing was saved in the context then
MSR_VSX is not set.
 
 The logic should be if we write VSX we set MSR_VSX, else we clear MSR_VSX, 
 ie:
 
 if (current-thread.used_vsr  ctx_has_vsx_region) {
 __giveup_vsx(current);
 if (copy_vsx_to_user(frame-mc_vsregs, current))
 return 1;
 msr |= MSR_VSX;
 } else
 msr = ~MSR_VSX;

If anything I dislike this because it might mask a bug in earlier code that
might erroneously set MSR_VSX even though current-thread.user_vsr is not
true. If anything the extra state 3. covered here is a buggy state.

I agree that your suggestion is more robust though since the

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

2013-11-21 Thread Carlos O'Donell 
On 11/21/2013 05:21 PM, Michael Neuling wrote:
 What about the 64-bit code? I don't know the code but it appears at a 
 glance to
 have the same bug.
  
 It doesn't happen with 64-bit code because there the context contains
 a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
 state. Therefore 64-bit ppc always has space to store the VMX registers
 in a userspace context switch. It is only the 32-bit ppc ABI that lacks
 the space.
 
 VMX?  I don't understand this at all.  We extended the ucontext to
 handle the extra VSX state, so older code may still be using the small
 ucontext and we already have a bunch of checks in the 64bit case for
 this.
 
 I agree with Michael, we should add this to the 64 bit case.  If we
 can't put VSX state in, then clear MSR VSX.

Sorry, typo, VSX not VMX.

I had not gone through the historical implementation of the 64-bit
code, I assumed it started with a sufficiently sized context structure,
but on closer review it didn't.

The addition of the *context functions in glibc for 64-bit power
happened in 2003 by glibc commit 609b4783, with the mcontext_t
being expanded to include 

I see that the 64-bit userspace context was extended in 2008 by your 
kernel commit ce48b210.

Thus you're right the check is needed in the 64-bit case.

However, at present the issue doesn't seem to trigger in the
64-bit userspace. Which is odd now that I review the code and 
see that the 64-bit userspace context is smaller than the kernel
context (lacks the `+32' to the vmx_reserve space). It could just
be that the compiler finds no chance to use VSX and therefore
the existing test cases don't trigger the bug. I don't plan to
investigate this further given that we're going to fix the 64-bit case
also.
 
 So how about we make it that simple and put it independent of the other
 if statement?
 
 diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
 index 749778e..f4a7fd4 100644
 --- a/arch/powerpc/kernel/signal_32.c
 +++ b/arch/powerpc/kernel/signal_32.c
 @@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct 
 mcon
 return 1;
 msr |= MSR_VSX;
 }
 +   /*
 +* With a small context structure we can't hold the VSX registers,
 +* hence clear the MSR value to indicate the state was not saved.
 +*/
 +   if (!ctx_has_vsx_region)
 +   msr = ~MSR_VSX;
 +
 +
  #endif /* CONFIG_VSX */
  #ifdef CONFIG_SPE
 /* save spe registers */
 

Looks good to me, along with a similar fix for signal_64.c.

Cheers,
Carlos.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

2013-11-21 Thread Carlos O'Donell 
On 11/21/2013 07:53 PM, Carlos O'Donell wrote:
 The addition of the *context functions in glibc for 64-bit power
 happened in 2003 by glibc commit 609b4783, with the mcontext_t
 being expanded to include 

... support for VMX state via `long vmx_reserve[NVRREG+NVRREG+1];'.

Cheers,
Carlos.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev