Re: [RFC PATCH glibc 11/12] hurd, htl: Add some x86_64-specific code

[Samuel Thibault <samuel.thibault@gnu.org>]

Sergey Bugaev, le dim. 12 févr. 2023 19:25:11 +0300, a ecrit:
> On Sun, Feb 12, 2023 at 7:11 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Sergey Bugaev, le dim. 12 févr. 2023 14:10:42 +0300, a ecrit:
> > > We should not need a getter routine, because one can simply inspect the target
> > > thread's state (unless, again, I misunderstand things horribly).
> >
> > For 16bit fs/gs values we could read them from userland yes. But for
> > fs/gs base, the FSGSBASE instruction is not available on all 64bit
> > processors. And ATM in THREAD_TCB we want to be able to get the base of
> > another thread.
> 
> What I've meant is:
> 
> __thread_get_state (whatever_thread, &state);
> uintptr_t its_fs_base = state->fs_base;
> 
> You can't really do the same to *write* [fg]s_base, because doing
> thread_set_state on your own thread is bound to end badly.

? Well, sure, just like setting fs/gs through thread state was not done
for i386.

I don't see where you're aiming. Getting fs/gs from __thread_get_state
won't actually give you the base, you'll just read something like 0.

> > > diff --git a/sysdeps/mach/hurd/x86_64/static-start.S b/sysdeps/mach/hurd/x86_64/static-start.S
> > > new file mode 100644
> > > index 00000000..982d3d52
> > > --- /dev/null
> > > +++ b/sysdeps/mach/hurd/x86_64/static-start.S
> > > @@ -0,0 +1,27 @@
> > > +/* Type of the TCB.  */
> > > +typedef struct
> > > +{
> > > +  void *tcb;                 /* Points to this structure.  */
> > > +  dtv_t *dtv;                        /* Vector of pointers to TLS data.  */
> > > +  thread_t self;             /* This thread's control port.  */
> > > +  int __glibc_padding1;
> > > +  int multiple_threads;
> > > +  int gscope_flag;
> > > +  uintptr_t sysinfo;
> > > +  uintptr_t stack_guard;
> > > +  uintptr_t pointer_guard;
> > > +  long __glibc_padding2[2];
> > > +  int private_futex;
> >
> > ? Isn't that rather feature_1 ?
> 
> sysdeps/mach/hurd/i386/tls.h has 'int private_futex;', which is where
> I stole this from. A quick grep confirms that it's never used,

Yes, this was just to align on the nptl tls.h. But apparently that got
renamed and hurd's tls wasn't updated.

> so we might rename both to feature_1, or maybe another instance of
> __glibc_padding.

Better stay coherent with the nptl version.

> > > +/* GCC generates %fs:0x28 to access the stack guard.  */
> > > +_Static_assert (offsetof (tcbhead_t, stack_guard) == 0x28,
> > > +                "stack guard offset");
> > > +/* libgcc uses %fs:0x70 to access the split stack pointer.  */
> > > +_Static_assert (offsetof (tcbhead_t, __private_ss) == 0x70,
> > > +                "split stack pointer offset");
> >
> > Indeed. Could you perhaps also add them to the i386 tls.h?
> 
> > > +/* Install new dtv for current thread.  */
> > > +# define INSTALL_NEW_DTV(dtvp) THREAD_SETMEM (THREAD_SELF, dtv, dtvp)
> > > +/* Return the address of the dtv for the current thread.  */
> > > +# define THREAD_DTV() THREAD_GETMEM (THREAD_SELF, dtv)
> >
> > While at it, try to make the i386 version use that too?
> 
> Yeah, I have not ported the improvements back to the 32-bit version;
> maybe I should.

Better always keep things as coherent as possible. Otherwise another you
will later wonder why in the hell we have differences between the two
versions.

> Another cool one is doing fs/gs-relative access using
> GCC's __seg_fs/__seg_gs when supported.

Yes, that's nice indeed!

Samuel