svn commit: r279937 - in head/sys/powerpc: include powerpc

[Nathan Whitehorn]

On 03/13/15 01:47, Konstantin Belousov wrote:
> On Thu, Mar 12, 2015 at 02:46:53PM -0700, Nathan Whitehorn wrote:
>> On 03/12/15 14:35, Konstantin Belousov wrote:
>>> On Thu, Mar 12, 2015 at 02:29:43PM -0700, Nathan Whitehorn wrote:
>>>> On 03/12/15 14:22, Konstantin Belousov wrote:
>>>>> On Thu, Mar 12, 2015 at 09:15:39PM +0000, Nathan Whitehorn wrote:
>>>>>> Author: nwhitehorn
>>>>>> Date: Thu Mar 12 21:15:38 2015
>>>>>> New Revision: 279937
>>>>>> URL: https://svnweb.freebsd.org/changeset/base/279937
>>>>>>
>>>>>> Log:
>>>>>>      Provide VSX context in ucontext(3) API.
>>>>>>
>>>>>> Modified:
>>>>>>      head/sys/powerpc/include/ucontext.h
>>>>>>      head/sys/powerpc/powerpc/exec_machdep.c
>>>>>>
>>>>>> Modified: head/sys/powerpc/include/ucontext.h
>>>>>> ==============================================================================
>>>>>> --- head/sys/powerpc/include/ucontext.h	Thu Mar 12 20:14:48 2015	(r279936)
>>>>>> +++ head/sys/powerpc/include/ucontext.h	Thu Mar 12 21:15:38 2015	(r279937)
>>>>>> @@ -46,6 +46,7 @@ typedef struct __mcontext {
>>>>>>     	uint32_t	mc_av[2];
>>>>>>     	register_t	mc_frame[42];
>>>>>>     	uint64_t	mc_fpreg[33];
>>>>>> +	uint64_t	mc_vsxfpreg[32];	/* low-order half of VSR0-31 */
>>>>>>     } mcontext_t __aligned(16);
>>>>>>     
>>>>>>     #if defined(_KERNEL) && defined(__powerpc64__)
>>>>>> @@ -60,6 +61,7 @@ typedef struct __mcontext32 {
>>>>>>     	uint32_t	mc_av[2];
>>>>>>     	uint32_t	mc_frame[42];
>>>>>>     	uint64_t	mc_fpreg[33];
>>>>>> +	uint64_t	mc_vsxfpreg[32];	/* low-order half of VSR0-31 */
>>>>>>     } mcontext32_t __aligned(16);
>>>>>>     #endif
>>>>> It looks as if you broken the ABI compatibility by the change.  Am I wrong ?
>>>>>
>>>> That is correct. It's a tier-2 platform and -CURRENT, so I'm not sure
>>>> it's worth the compatibility shims. I'm happy to add them if you think
>>>> otherwise.
>>> You are main maintainer of PowerPC port, IMO, so it is your decision.
>>>
>>> Note that 'this is current' argument is not applicable, since the change
>>> also breaks stable/* binaries.
>>>
>>> I do understand the argument of PowerPC being tier 2 architecture, but this
>>> makes me sad.  Anyway, it is yours.  For x86, I have to introduce
>>> getcontextx(3) mechanism.
>>>
>> This is a good point. I'll try to fix it. Is my understanding of how
>> this works correct?
>>
>> 1. Provide a sysarch() for the extended FPU state.
>> 2. Implement getcontextx() in the C library to fill extra properties if
>> required.
>> 3. Store state for signal trampoline in variable-sized stack area
> 4. Implement __getcontextx_size() and __fillcontextx2() for use
> in the deferred signal delivery while libthr is in critical section,
> see lib/libthr/thread/thr_sig.c:check_deferred_signal().

OK.

>> Implementation of (2) seems to rely on having spare members in ucontext,
>> which PowerPC unfortunately does not have. Is there a way around that?
> Indeed, this is very unfortunate.
>
> My concern is that typical application allocating ucontext_t on stack or
> by mallocing it, would get silent memory corruption after the extension
> of mcontext_t.  It seems indeed that ABI breakage cannot be completely
> avoided, but it could be significantly reduced IMO.  Is it true that
> mc_avec is only valid when _MC_AV_VALID bit is set ?

That is correct.

> If yes, we can introduce another mcontext flag, say _MC_XSTATE_VALID,
> which is mutually exclusive with the _MC_AC_VALID. The new flag
> indicates that there is external data, and the data is pointed to by
> some word placed in the previous mc_avec file, say mc_avec[0]. The
> altivec registers file content is moved into that external data area as
> well. Providing the external area size in mc_avec[1] allows to extend
> that block in the future-compatible manner.
>
> This way, applications which use ucontext_t and which are not aware about
> VSX, get the expected behaviour, possibly without seeing altivec.  I.e.
> in typical case, we do not get random memory corruption.

OK, I guess that's reasonable. I'll think about this some and try to get 
some code written in the next few days.

> Meantime, I have a question. I looked at the powerpc/include/ucontex.h
> and tried to match it with the PowerISA specs 2.06 and 2.07. From what I
> understand, mc_fpreg corresponds to the floating-point registers file,
> mc_avec and mc_av to the 'Vector Facility Registers'.  But I fail to see
> what would match the
> +	uint64_t	mc_vsxfpreg[32];	/* low-order half of VSR0-31 */
> file. The 7.2.1 Vector-Scalar Registers says 'Sixty-four 128-bit VSRs
> are provided'.
>

The 64 128-bit registers are a superset of the existing registers. 
Registers 33-64 are just the Altivec registers, with no changes. 
Registers 1-32 are the normal floating point registers, but widened to 
128 bits from 64. What I had tried to do was to keep the layout of the 
bottom part of the structure unchanged for compatibility by storing only 
the extra half of registers 1-32 in a separate area. In particular, I 
wanted to keep the FP registers readable in a consecutive way.
-Nathan