[PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

Here are the patches against 2.4.0-test1-ac12 for the fixes to the PIII
SSE/FXSR support.

glibc 2.1.3 and GDB 5.0 compile out of the box, but the glibc patch
contains the appropriate header file updates and the GDB patch contains
a few fixes to the masking of floating point environment data.

I've included a signal handling test that, when compiled with the 2.4.0
kernel and updated header files, will run on 2.2/2.3 and 2.4 kernels and
correctly determine if the FXSR data is available.

GDB 5.0 will use the new ptrace request, but will not have access to the
SSE registers from a core dump yet.

-- Gareth
patch-2.4.0test1-ac12-gth1.gz
patch-gdb-5.0-gth1.gz
patch-glibc-2.1.3-gth1.gz
sigfpe_test.c.gz


[-- Attachment #2: patch-2.4.0test1-ac12-gth1.gz --]
[-- Type: application/x-gzip, Size: 4133 bytes --]

[-- Attachment #3: patch-gdb-5.0-gth1.gz --]
[-- Type: application/x-gzip, Size: 1276 bytes --]

[-- Attachment #4: patch-glibc-2.1.3-gth1.gz --]
[-- Type: application/x-gzip, Size: 1464 bytes --]

[-- Attachment #5: sigfpe_test.c.gz --]
[-- Type: application/x-gzip, Size: 775 bytes --]

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Mark Kettenis]

   Date: Sat, 10 Jun 2000 12:51:46 -0600
   From: Gareth Hughes <gareth@precisioninsight.com>

   Here are the patches against 2.4.0-test1-ac12 for the fixes to the PIII
   SSE/FXSR support.

Sorry, but I don't think this is right:

 #define save_fpu(tsk) do { \
-	asm volatile("fxsave %0 ; fwait" \
-		     : "=m" (tsk->thread.i387.hard)); \
+	asm volatile("fnstenv %0 ; fxsave %1 ; fwait" \
+		     : "=m" (tsk->thread.i387.hard), \
+		       "=m" (tsk->thread.i387.hard.fxsr_space[0])); \

If I'm not mistaken this means that you've added an additional
overhead on *every* task switch between FPU-using tasks.  Instead you
should only convert to the old fsave format when that's actually
needed:

 1. When setting up a signal context for invoking a signal handler.

 2. For the PTRACE_GETFPREGS request.

I also don't understand why you expose the strange user_xfpregs_struct
format via the PTRACE_{GET,SET}XFPREGS requests.  Why don't you just
provide the 512-byte FXSAVE area?

All in all, I think that the origional code in 2.4.0test1 is much
closer to what the endresult should look like.  Just add the
PTRACE_{GET,SET}XFPREGS requests that return, write the FXSAVE data
(should be *very* simple), add the necessary bits for setting up the
signal frame (more or less what's in your patch, but this is the place
where the FXSAVE -> FSAVE conversion should be done), and add some
code to the core dumping code to add an extra note with the FXSAVE
area.

Mark

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

Mark Kettenis wrote:
> 
> Sorry, but I don't think this is right:
> 
>  #define save_fpu(tsk) do { \
> -       asm volatile("fxsave %0 ; fwait" \
> -                    : "=m" (tsk->thread.i387.hard)); \
> +       asm volatile("fnstenv %0 ; fxsave %1 ; fwait" \
> +                    : "=m" (tsk->thread.i387.hard), \
> +                      "=m" (tsk->thread.i387.hard.fxsr_space[0])); \
> 
> If I'm not mistaken this means that you've added an additional
> overhead on *every* task switch between FPU-using tasks.  Instead you
> should only convert to the old fsave format when that's actually
> needed:
> 
>  1. When setting up a signal context for invoking a signal handler.
> 
>  2. For the PTRACE_GETFPREGS request.

What happens when a signal occurs in a time slice where the application
hasn't used the FPU?  Having the regular FPU environment around means we
don't have to do the conversion between formats, and don't have to
convert the regular format back into the FXSAVE format when the signal
handler returns.  This ensures current signal handlers that change the
FPU state with the old sigcontext will be guaranteed to work correctly.

Linus has said that he doesn't want to convert between the formats
manually, and this is a nice way to not have to do that.

> I also don't understand why you expose the strange user_xfpregs_struct
> format via the PTRACE_{GET,SET}XFPREGS requests.  Why don't you just
> provide the 512-byte FXSAVE area?

Again, so we don't have to convert between formats.  This structure is
taken from a patch to get SSE working with GDB 5.0, but I guess the raw
FXSAVE data will do just fine.  I didn't really like this either.  I
will change this back.

> All in all, I think that the origional code in 2.4.0test1 is much
> closer to what the endresult should look like.  Just add the
> PTRACE_{GET,SET}XFPREGS requests that return, write the FXSAVE data
> (should be *very* simple), add the necessary bits for setting up the
> signal frame (more or less what's in your patch, but this is the place
> where the FXSAVE -> FSAVE conversion should be done), and add some
> code to the core dumping code to add an extra note with the FXSAVE
> area.

Okay, I tried too hard to please everyone :-)

I'll play around with the conversion into/out of the signal handler and
GETFPREGS request.  Should only take a short while, so don't do anything
with the stuff I sent out yesterday.

-- Gareth

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Alan Cox]

> What happens when a signal occurs in a time slice where the application
> hasn't used the FPU?  Having the regular FPU environment around means we

You touch the FPU, it loads the FPU state you save both. If the FPU state
is already loaded it costs nothing. The FNSAVE stuff is horribly slow so
it is a real concern

> I'll play around with the conversion into/out of the signal handler and
> GETFPREGS request.  Should only take a short while, so don't do anything
> with the stuff I sent out yesterday.

Right now its correct, this is about being correct clean and fast so we are
going the right way

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

Alan Cox wrote:
> 
> > What happens when a signal occurs in a time slice where the application
> > hasn't used the FPU?  Having the regular FPU environment around means we
> 
> You touch the FPU, it loads the FPU state you save both. If the FPU state
> is already loaded it costs nothing. The FNSAVE stuff is horribly slow so
> it is a real concern.

I'm not sure I understand what you're advocating here.  If you're okay
with converting the FXSAVE data back and forth, then so am I.

> > I'll play around with the conversion into/out of the signal handler and
> > GETFPREGS request.  Should only take a short while, so don't do anything
> > with the stuff I sent out yesterday.
> 
> Right now its correct, this is about being correct clean and fast so we are
> going the right way

I certainly agree with making it clean and fast, as well as correct. 
Would you prefer the conversion and just using FXSAVE/FXRSTOR?

-- Gareth

Re: [PATCH] Updates to FXSR/SSE support in 2.4.0-test1

[Alan Cox]

> I'm not sure I understand what you're advocating here.  If you're okay
> with converting the FXSAVE data back and forth, then so am I.

Providing we dont do it except when needed. What are the issues in detail, 
I feel Im missing something here

[PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

Here's a copy of my latest work, this time kernel and glibc headers
only.  I'm still working on updating GDB, as incorporating Mark's
suggestions leads to some problems with the stock 5.0 code (mainly in
the interpretation of the FPU tag word).

What are we doing now?

We use FXSAVE/FXRSTOR only to save and restore the FPU context as
before.  Nice and fast.  As always, the FPU state is only saved when the
FPU is used and restored with an #NM exception.

We (correctly) convert the FXSR-format data into the expanded signal
context, and include the original FXSR-format data like before.  This
includes doing a full conversion between tag word formats.

We (correctly) convert the FXSR-format data on the
PTRACE_[GET|SET]FPREGS requests.

We just use the 512-byte FXSR data on PTRACE_[GET|SET]XFPREGS requests. 
This requires updates to GDB, which I'm working on now.  Nothing major,
the code is the same as in the kernel.

The tag word conversion originally seen in 2.4.0-test1 was almost
correct, but this code does a more thorough job - important for GDB 5.0
correctness.

As always, please let me know if you have any grievances.  Doing the
conversion turned out to be a lot less ugly than I had imagined, and as
we only need to do it for signals, ptracing and dumping core, it doesn't
affect things too greatly.  I will add in the core dumping support when
the work in this patch gets applied (in whatever form).

The signal handler test from my previous email will still work, and I
encourage you to verify this.

-- Gareth
patch-2.4.0test1-ac12-gth2.gz
patch-glibc-2.1.3-gth2.gz


[-- Attachment #2: patch-2.4.0test1-ac12-gth2.gz --]
[-- Type: application/x-gzip, Size: 4084 bytes --]

[-- Attachment #3: patch-glibc-2.1.3-gth2.gz --]
[-- Type: application/x-gzip, Size: 1453 bytes --]

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Mark Kettenis]

   Date: Sun, 11 Jun 2000 18:03:41 -0600
   From: Gareth Hughes <gareth@precisioninsight.com>

   Here's a copy of my latest work, this time kernel and glibc headers
   only.  I'm still working on updating GDB, as incorporating Mark's
   suggestions leads to some problems with the stock 5.0 code (mainly in
   the interpretation of the FPU tag word).

Looks much better :-).  I didn't realize there were tag word problems
until yesterday :-(.  If this means that stock GDB 5.0 won't work
anymore, so be it.  A sane kernel interface is more important.

What exactly happens if you try to compile stock GDB 5.0 with the new
kernel?  Does it compile at all?  Is it seriously crippled?  Or does
it basically work right?

If there are major problems, it might be wise to choose a different
name for the PTRACE_{GET,SET}XFPREGS requests to avoid confusion.
That way, nobody will end up with a non-functional GDB (of course the
blame lies entirely with the GDB team for this mess).  My suggestion
would be to call those requests PTRACE_{GET,SET}FPXREGS.  It turns out
that Unixware uses a similar name for its FXSR support (they don't
have ptrace(), but they have a PCSFPXREG ioctl() and a __fpxregset_t
type[1]).  Moreover this makes the FSAVE/FPREGS vs. FXSAVE/FPXREGS
analogy a bit more explicit.

Mark

[1] The patch implementing the FXSR support for Unixware is PTF7406B,
    which you can get at the following URLs:

    ftp://ftp.sco.com/SLS/ptf7406b.txt (description)
    ftp://ftp.sco.com/SLS/ptf7406b.Z   (the actual patch)

    The patch is a `pkg Datastream (SVR4)' (according to file(1)), but
    if you ignore the binary bits you can read the documentation and
    header files.

    Anyhow, people might want to take a look at this for additional
    inspiration.

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

Mark Kettenis wrote:
> 
>    Date: Sun, 11 Jun 2000 18:03:41 -0600
>    From: Gareth Hughes <gareth@precisioninsight.com>
> 
>    Here's a copy of my latest work, this time kernel and glibc headers
>    only.  I'm still working on updating GDB, as incorporating Mark's
>    suggestions leads to some problems with the stock 5.0 code (mainly in
>    the interpretation of the FPU tag word).
> 
> Looks much better :-).  I didn't realize there were tag word problems
> until yesterday :-(.  If this means that stock GDB 5.0 won't work
> anymore, so be it.  A sane kernel interface is more important.

I agree.  I'm happy with this as it's correct, fast *and* backward
compatible.  Thanks again everyone for pulling me up on the third point
:-)

> What exactly happens if you try to compile stock GDB 5.0 with the new
> kernel?  Does it compile at all?  Is it seriously crippled?  Or does
> it basically work right?

It works (kind of), it's just extracting the FPU register data
incorrectly and then misinterpreting the tag word and thus
incorrectlying interpreting the incorrect data :-).  It's a pretty
trivial patch, the extraction fix is contained in my last patch and I've
just got to copy the tag word conversion code across.  Should have a
patch in a couple of hours - busy with work at the moment.

> If there are major problems, it might be wise to choose a different
> name for the PTRACE_{GET,SET}XFPREGS requests to avoid confusion.
> That way, nobody will end up with a non-functional GDB (of course the
> blame lies entirely with the GDB team for this mess).  My suggestion
> would be to call those requests PTRACE_{GET,SET}FPXREGS.  It turns out
> that Unixware uses a similar name for its FXSR support (they don't
> have ptrace(), but they have a PCSFPXREG ioctl() and a __fpxregset_t
> type[1]).  Moreover this makes the FSAVE/FPREGS vs. FXSAVE/FPXREGS
> analogy a bit more explicit.

I like this idea, makes a whole lot more sense.  This is kinda what I
was arguing for way back in our previous emails.  It also matches up
nicely with the signal handling types I've declared.  It would be good
to get a working GDB 5.0 with a patch rather than a broken one when you
haven't patched it.  The standard PTRACE_{GET|SET}FPREGS works fine as
it stands, and in many cases this will be good enough.  When is the next
release of GDB planned for?

I'd like some clarification on core dumping - will it be okay to add a
new note NT_PRFPXREG, a new elf type elf_fpxregset_t and so on?  If so,
I'll add this to the kernel and my GDB 5.0 patch and fire it off today.

-- Gareth

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Mark Kettenis]

   Date: Mon, 12 Jun 2000 14:56:27 -0600
   From: Gareth Hughes <gareth@precisioninsight.com>

   > What exactly happens if you try to compile stock GDB 5.0 with the new
   > kernel?  Does it compile at all?  Is it seriously crippled?  Or does
   > it basically work right?

   It works (kind of), it's just extracting the FPU register data
   incorrectly and then misinterpreting the tag word and thus
   incorrectlying interpreting the incorrect data :-).  It's a pretty
   trivial patch, the extraction fix is contained in my last patch and I've
   just got to copy the tag word conversion code across.  Should have a
   patch in a couple of hours - busy with work at the moment.

Don't put too much effort into it.  I'm going to add some routines to
GDB that do the conversion from an FXSAVE area to GDB's internal
format that can be reused for other i386 targets besides Linux.

   > If there are major problems, it might be wise to choose a different
   > name for the PTRACE_{GET,SET}XFPREGS requests to avoid confusion.
   > That way, nobody will end up with a non-functional GDB (of course the
   > blame lies entirely with the GDB team for this mess).  My suggestion
   > would be to call those requests PTRACE_{GET,SET}FPXREGS.  It turns out
   > that Unixware uses a similar name for its FXSR support (they don't
   > have ptrace(), but they have a PCSFPXREG ioctl() and a __fpxregset_t
   > type[1]).  Moreover this makes the FSAVE/FPREGS vs. FXSAVE/FPXREGS
   > analogy a bit more explicit.

   I like this idea, makes a whole lot more sense.  This is kinda what I
   was arguing for way back in our previous emails.  It also matches up
   nicely with the signal handling types I've declared.  It would be good
   to get a working GDB 5.0 with a patch rather than a broken one when you
   haven't patched it.  The standard PTRACE_{GET|SET}FPREGS works fine as
   it stands, and in many cases this will be good enough.  When is the next
   release of GDB planned for?

I believe a new release cycle will be started in July, which should
lead to GDB 5.1 somewhere in November.

   I'd like some clarification on core dumping - will it be okay to add a
   new note NT_PRFPXREG, a new elf type elf_fpxregset_t and so on?  If so,
   I'll add this to the kernel and my GDB 5.0 patch and fire it off today.

Yep, that would be fine.  You'll have to choose a name for the note
too.  The other notes have the name "CORE", which is also used by
other SVR4-derived systems.  In that case you'd have to make sure that
NT_PRFPXREG doesn't clash with types that are already in use.  It's
probably better to choose a different name.  There are no rules but
the System V ABI suggests using the name of the vendor.  The old SSE
support uses "LINUX", which isn't such a bad idea (there is no reason
to choose a different name).  You can pick any number for NT_PRFPXREG,
but choosing one that's not already used in libbfd makes life a bit
easier.

Right now we have (see include/elf/common.h in the GDB/binutils
distribution):

#define NT_PRSTATUS	1		/* Contains copy of prstatus struct */
#define NT_FPREGSET	2		/* Contains copy of fpregset struct */
#define NT_PRPSINFO	3		/* Contains copy of prpsinfo struct */
#define NT_TASKSTRUCT	4		/* Contains copy of task struct */
#define NT_PRXFPREG     0x46e62b7f	/* Contains a user_xfpregs_struct; */
					/*   note name must be "LINUX".  */

/* Note segments for core files on dir-style procfs systems. */

#define NT_PSTATUS	10		/* Has a struct pstatus */
#define NT_FPREGS	12		/* Has a struct fpregset */
#define NT_PSINFO	13		/* Has a struct psinfo */
#define NT_LWPSTATUS	16		/* Has a struct lwpstatus_t */
#define NT_LWPSINFO	17		/* Has a struct lwpsinfo_t */
#define NT_WIN32PSTATUS	18		/* Has a struct win32_pstatus */


Where obviously NT_PRXFPREG will dissapear.


Solaris has (see /usr/include/sys/elf.h):

#define NT_PRSTATUS     1       /* prstatus_t   <sys/old_procfs.h>      */
#define NT_PRFPREG      2       /* prfpregset_t <sys/old_procfs.h>      */
#define NT_PRPSINFO     3       /* prpsinfo_t   <sys/old_procfs.h>      */
#define NT_PRXREG       4       /* prxregset_t  <sys/procfs.h>          */
#define NT_PLATFORM     5       /* string from sysinfo(SI_PLATFORM)     */
#define NT_AUXV         6       /* auxv_t array <sys/auxv.h>            */
#define NT_GWINDOWS     7       /* gwindows_t   SPARC only              */
#define NT_ASRS         8       /* asrset_t     SPARC V9 only           */
#define NT_PSTATUS      10      /* pstatus_t    <sys/procfs.h>          */
#define NT_PSINFO       13      /* psinfo_t     <sys/procfs.h>          */
#define NT_PRCRED       14      /* prcred_t     <sys/procfs.h>          */
#define NT_UTSNAME      15      /* struct utsname <sys/utsname.h>       */
#define NT_LWPSTATUS    16      /* lwpstatus_t  <sys/procfs.h>          */
#define NT_LWPSINFO     17      /* lwpsinfo_t   <sys/procfs.h>          */


Pick a number, and I'll make sure the necessary support will be added
to libbfd and GDB.

Mark

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Jim Blandy]

> If there are major problems, it might be wise to choose a different
> name for the PTRACE_{GET,SET}XFPREGS requests to avoid confusion.
> That way, nobody will end up with a non-functional GDB (of course the
> blame lies entirely with the GDB team for this mess).

Well, the blame lies specifically with me, I assume.  I did the
original SSE debugging support.

How should I have done this to avoid the mess?

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Gareth Hughes]

Mark Kettenis wrote:
> 
> Don't put too much effort into it.  I'm going to add some routines to
> GDB that do the conversion from an FXSAVE area to GDB's internal
> format that can be reused for other i386 targets besides Linux.

Okay, I've been offline working on the next release of XFree86 but I've
got the necessary kernel changes in place.  Core dumps now include the
standard FPU format and the FXSAVE FPU format.  We use
PTRACE_{GET|SET}FPXREGS as discussed, with the corresponding
user_fpxregs_struct in user.h.

> Yep, that would be fine.  You'll have to choose a name for the note
> too.  The other notes have the name "CORE", which is also used by
> other SVR4-derived systems.  In that case you'd have to make sure that
> NT_PRFPXREG doesn't clash with types that are already in use.  It's
> probably better to choose a different name.  There are no rules but
> the System V ABI suggests using the name of the vendor.  The old SSE
> support uses "LINUX", which isn't such a bad idea (there is no reason
> to choose a different name).  You can pick any number for NT_PRFPXREG,
> but choosing one that's not already used in libbfd makes life a bit
> easier.

I'd really like to use "CORE" rather than "LINUX", and have chosen
NT_PRFPXREG=20 (or NT_FPXREGSET as appropriate) as this seems to be okay
from what I can tell.  What do Unixware use?  It would be nice to end up
with the note as "CORE", but I can accept "LINUX" as an interim
solution.  Having a standard, cross-platform elf note for the fxsave
data would be ideal.

> Pick a number, and I'll make sure the necessary support will be added
> to libbfd and GDB.

NT_PRFPXREG|NT_FPXREGSET = 20 would make me happy.

I'm merging my work with Linus' latest stuff and will get it out ASAP.

-- Gareth

Re: [PATCH] More updates to FXSR/SSE support in 2.4.0-test1

[Jim Blandy]

> > Yep, that would be fine.  You'll have to choose a name for the note
> > too.  The other notes have the name "CORE", which is also used by
> > other SVR4-derived systems.  In that case you'd have to make sure that
> > NT_PRFPXREG doesn't clash with types that are already in use.  It's
> > probably better to choose a different name.  There are no rules but
> > the System V ABI suggests using the name of the vendor.  The old SSE
> > support uses "LINUX", which isn't such a bad idea (there is no reason
> > to choose a different name).  You can pick any number for NT_PRFPXREG,
> > but choosing one that's not already used in libbfd makes life a bit
> > easier.
> 
> I'd really like to use "CORE" rather than "LINUX", and have chosen
> NT_PRFPXREG=20 (or NT_FPXREGSET as appropriate) as this seems to be okay
> from what I can tell.  What do Unixware use?  It would be nice to end up
> with the note as "CORE", but I can accept "LINUX" as an interim
> solution.  Having a standard, cross-platform elf note for the fxsave
> data would be ideal.

When I last talked with folks from SCO, they said they had no
debugging support for SSE registers.  I didn't feel I had the right to
speak for all Unixes, so I chose LINUX.

Keep in mind that it's easy for a debugger to recognize multiple note
types, but (in general) impossible to unravel conflicting definitions
for the same note type.  While it certainly feels cleaner to have a
"standard, cross-platform ELF note", I think it's actually asking for
trouble.  I recommend sticking with "LINUX".