[RFC,2/5] RISC-V: Reserve about 5K space in mcontext_t to support future ISA expansion.

  Following the changes of struct sigcontext in Linux to reserve about 5K space
to support future ISA expansion.
---
 sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
 1 file changed, 2 insertions(+)
  

* Vincent Chen:

> Following the changes of struct sigcontext in Linux to reserve about 5K space
> to support future ISA expansion.
> ---
>  sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> index cfafa44..80caf07 100644
> --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> @@ -82,6 +82,8 @@ typedef struct mcontext_t
>    {
>      __riscv_mc_gp_state __gregs;
>      union  __riscv_mc_fp_state __fpregs;
> +    /* 5K + 256 reserved for vector state and future expansion.  */
> +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
>    } mcontext_t;

This changes the size of struct ucontext_t, which is an ABI break
(getcontext callers are supposed to provide their own object).

This shouldn't be necessary if the additional vector registers are
caller-saved.

Thanks,
Florian
  

On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
> * Vincent Chen:
> 
> > Following the changes of struct sigcontext in Linux to reserve about 5K space
> > to support future ISA expansion.
> > ---
> >  sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > index cfafa44..80caf07 100644
> > --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > @@ -82,6 +82,8 @@ typedef struct mcontext_t
> >    {
> >      __riscv_mc_gp_state __gregs;
> >      union  __riscv_mc_fp_state __fpregs;
> > +    /* 5K + 256 reserved for vector state and future expansion.  */
> > +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
> >    } mcontext_t;
> 
> This changes the size of struct ucontext_t, which is an ABI break
> (getcontext callers are supposed to provide their own object).
> 
> This shouldn't be necessary if the additional vector registers are
> caller-saved.

Indeed, that was my first thought when I saw this too. Any late
additions to the register file must be call-clobbered or else they are
a new ABI. And mcontext_t does not need to represent any
call-clobbered state.

Rich
  

On Mon, Sep 13, 2021 at 9:52 PM Rich Felker <dalias@libc.org> wrote:
>
> On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
> > * Vincent Chen:
> >
> > > Following the changes of struct sigcontext in Linux to reserve about 5K space
> > > to support future ISA expansion.
> > > ---
> > >  sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > index cfafa44..80caf07 100644
> > > --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > @@ -82,6 +82,8 @@ typedef struct mcontext_t
> > >    {
> > >      __riscv_mc_gp_state __gregs;
> > >      union  __riscv_mc_fp_state __fpregs;
> > > +    /* 5K + 256 reserved for vector state and future expansion.  */
> > > +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
> > >    } mcontext_t;
> >
Hi Florian and Rich,
Sorry for the late reply and thank you for reminding me the
modification will cause ABI break.

> > This changes the size of struct ucontext_t, which is an ABI break
> > (getcontext callers are supposed to provide their own object).
> >

The riscv vector registers are all caller-saved registers except for
VCSR. Therefore, the struct mcontext_t needs to reserve a space for
it. In addition, RISCV ISA is growing, so I also hope the struct
mcontext_t has a space for future expansion. Based on the above ideas,
I reserved a 5K space here.

> > This shouldn't be necessary if the additional vector registers are
> > caller-saved.

Here I am a little confused about the usage of struct mcontext_t. As
far as I know, the struct mcontext_t is used to save the
machine-specific information in user context operation. Therefore, in
this case, the struct mcontext_t is allowed to reserve the space only
for saving caller-saved registers. However, in the signal handler, the
user seems to be allowed to use uc_mcontext whose data type is struct
mcontext_t to access the content of the signal context. In this case,
the struct mcontext_t may need to be the same as the struct sigcontext
defined at kernel. However, it will have a conflict with your
suggestion because the struct sigcontext cannot just reserve a space
for saving caller-saved registers. Could you help me point out my
misunderstanding? Thank you.

> Indeed, that was my first thought when I saw this too. Any late
> additions to the register file must be call-clobbered or else they are
> a new ABI. And mcontext_t does not need to represent any
> call-clobbered state.
>
> Rich
  

* Vincent Chen:

>> > This changes the size of struct ucontext_t, which is an ABI break
>> > (getcontext callers are supposed to provide their own object).
>> >
>
> The riscv vector registers are all caller-saved registers except for
> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> it. In addition, RISCV ISA is growing, so I also hope the struct
> mcontext_t has a space for future expansion. Based on the above ideas,
> I reserved a 5K space here.

You have reserved space in ucontext_t that you could use for this.

>> > This shouldn't be necessary if the additional vector registers are
>> > caller-saved.
>
> Here I am a little confused about the usage of struct mcontext_t. As
> far as I know, the struct mcontext_t is used to save the
> machine-specific information in user context operation. Therefore, in
> this case, the struct mcontext_t is allowed to reserve the space only
> for saving caller-saved registers. However, in the signal handler, the
> user seems to be allowed to use uc_mcontext whose data type is struct
> mcontext_t to access the content of the signal context. In this case,
> the struct mcontext_t may need to be the same as the struct sigcontext
> defined at kernel. However, it will have a conflict with your
> suggestion because the struct sigcontext cannot just reserve a space
> for saving caller-saved registers. Could you help me point out my
> misunderstanding? Thank you.

struct sigcontext is allocated by the kernel, so you can have pointers
in reserved fields to out-of-line start, or after struct sigcontext.

I don't know how the kernel implements this, but there is considerable
flexibility and extensibility.  The main issues comes from small stacks
which are incompatible with large register files.

Thanks,
Florian
  

I know this patch set mostly deals with signal handling but don’t forget LD_AUDIT. It has a similar issue for the plt_enter and exit functions.

-ben

> On Sep 16, 2021, at 1:03 AM, Vincent Chen <vincent.chen@sifive.com> wrote:
> 
> On Mon, Sep 13, 2021 at 9:52 PM Rich Felker <dalias@libc.org> wrote:
>> 
>>> On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
>>> * Vincent Chen:
>>> 
>>>> Following the changes of struct sigcontext in Linux to reserve about 5K space
>>>> to support future ISA expansion.
>>>> ---
>>>> sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>> 
>>>> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
>>>> index cfafa44..80caf07 100644
>>>> --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
>>>> +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
>>>> @@ -82,6 +82,8 @@ typedef struct mcontext_t
>>>>   {
>>>>     __riscv_mc_gp_state __gregs;
>>>>     union  __riscv_mc_fp_state __fpregs;
>>>> +    /* 5K + 256 reserved for vector state and future expansion.  */
>>>> +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
>>>>   } mcontext_t;
>>> 
> Hi Florian and Rich,
> Sorry for the late reply and thank you for reminding me the
> modification will cause ABI break.
> 
>>> This changes the size of struct ucontext_t, which is an ABI break
>>> (getcontext callers are supposed to provide their own object).
>>> 
> 
> The riscv vector registers are all caller-saved registers except for
> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> it. In addition, RISCV ISA is growing, so I also hope the struct
> mcontext_t has a space for future expansion. Based on the above ideas,
> I reserved a 5K space here.
> 
>>> This shouldn't be necessary if the additional vector registers are
>>> caller-saved.
> 
> Here I am a little confused about the usage of struct mcontext_t. As
> far as I know, the struct mcontext_t is used to save the
> machine-specific information in user context operation. Therefore, in
> this case, the struct mcontext_t is allowed to reserve the space only
> for saving caller-saved registers. However, in the signal handler, the
> user seems to be allowed to use uc_mcontext whose data type is struct
> mcontext_t to access the content of the signal context. In this case,
> the struct mcontext_t may need to be the same as the struct sigcontext
> defined at kernel. However, it will have a conflict with your
> suggestion because the struct sigcontext cannot just reserve a space
> for saving caller-saved registers. Could you help me point out my
> misunderstanding? Thank you.
> 
>> Indeed, that was my first thought when I saw this too. Any late
>> additions to the register file must be call-clobbered or else they are
>> a new ABI. And mcontext_t does not need to represent any
>> call-clobbered state.
>> 
>> Rich
>
  

On Thu, Sep 16, 2021 at 04:02:50PM +0800, Vincent Chen wrote:
> On Mon, Sep 13, 2021 at 9:52 PM Rich Felker <dalias@libc.org> wrote:
> >
> > On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
> > > * Vincent Chen:
> > >
> > > > Following the changes of struct sigcontext in Linux to reserve about 5K space
> > > > to support future ISA expansion.
> > > > ---
> > > >  sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > index cfafa44..80caf07 100644
> > > > --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > @@ -82,6 +82,8 @@ typedef struct mcontext_t
> > > >    {
> > > >      __riscv_mc_gp_state __gregs;
> > > >      union  __riscv_mc_fp_state __fpregs;
> > > > +    /* 5K + 256 reserved for vector state and future expansion.  */
> > > > +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
> > > >    } mcontext_t;
> > >
> Hi Florian and Rich,
> Sorry for the late reply and thank you for reminding me the
> modification will cause ABI break.
> 
> > > This changes the size of struct ucontext_t, which is an ABI break
> > > (getcontext callers are supposed to provide their own object).
> > >
> 
> The riscv vector registers are all caller-saved registers except for
> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> it. In addition, RISCV ISA is growing, so I also hope the struct
> mcontext_t has a space for future expansion. Based on the above ideas,
> I reserved a 5K space here.

VCSR is not call-saved (aka 'callee-saved' in alternate notation)
either. It's thread-local state that may be changed or left alone by
calls, and that sj/lj/ucontext functions can't touch, just like fenv.
Saving and restoring it here would be wrong.

Rich
  

On Thu, Sep 16, 2021 at 4:14 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Vincent Chen:
>
> >> > This changes the size of struct ucontext_t, which is an ABI break
> >> > (getcontext callers are supposed to provide their own object).
> >> >
> >
> > The riscv vector registers are all caller-saved registers except for
> > VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> > it. In addition, RISCV ISA is growing, so I also hope the struct
> > mcontext_t has a space for future expansion. Based on the above ideas,
> > I reserved a 5K space here.
>
> You have reserved space in ucontext_t that you could use for this.
>
Sorry, I cannot really understand what you mean. The following is the
contents of ucontext_t
typedef struct ucontext_t
  {
    unsigned long int  __uc_flags;
    struct ucontext_t *uc_link;
    stack_t            uc_stack;
    sigset_t           uc_sigmask;
    /* There's some padding here to allow sigset_t to be expanded in the
       future.  Though this is unlikely, other architectures put uc_sigmask
       at the end of this structure and explicitly state it can be
       expanded, so we didn't want to box ourselves in here.  */
    char               __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
    /* We can't put uc_sigmask at the end of this structure because we need
       to be able to expand sigcontext in the future.  For example, the
       vector ISA extension will almost certainly add ISA state.  We want
       to ensure all user-visible ISA state can be saved and restored via a
       ucontext, so we're putting this at the end in order to allow for
       infinite extensibility.  Since we know this will be extended and we
       assume sigset_t won't be extended an extreme amount, we're
       prioritizing this.  */
    mcontext_t uc_mcontext;
  } ucontext_t;

Currently, we only reserve a space, __glibc_reserved[], for the future
expansion of sigset_t.
Do you mean I could use __glibc_reserved[] to for future expansion of
ISA as well?

> >> > This shouldn't be necessary if the additional vector registers are
> >> > caller-saved.
> >
> > Here I am a little confused about the usage of struct mcontext_t. As
> > far as I know, the struct mcontext_t is used to save the
> > machine-specific information in user context operation. Therefore, in
> > this case, the struct mcontext_t is allowed to reserve the space only
> > for saving caller-saved registers. However, in the signal handler, the
> > user seems to be allowed to use uc_mcontext whose data type is struct
> > mcontext_t to access the content of the signal context. In this case,
> > the struct mcontext_t may need to be the same as the struct sigcontext
> > defined at kernel. However, it will have a conflict with your
> > suggestion because the struct sigcontext cannot just reserve a space
> > for saving caller-saved registers. Could you help me point out my
> > misunderstanding? Thank you.
>
> struct sigcontext is allocated by the kernel, so you can have pointers
> in reserved fields to out-of-line start, or after struct sigcontext.
>
> I don't know how the kernel implements this, but there is considerable
> flexibility and extensibility.  The main issues comes from small stacks
> which are incompatible with large register files.
>

I have the same concern as you for reserving a huge space in
mcontext_t. If the content of mcontext_t is allowed to be different
from the content of sigcontext_t, and it has been confirmed that VCSR
should not be saved or restored by the *context function, then there
seems to be no need to reserve a space in mcontext to support V
extension. I will review it again. Thank you !!


> Thanks,
> Florian
>
  

On Fri, Sep 17, 2021 at 7:56 AM Ben Woodard <woodard@redhat.com> wrote:
>
> I know this patch set mostly deals with signal handling but don’t forget LD_AUDIT. It has a similar issue for the plt_enter and exit functions.
>
> -ben

Hi Ben,
I am not familiar with the mechanism of LD_AUDIT, so I actually do not
know if this modification may have any effect on LD_AUDIT. If
possible, could you briefly introduce the issues for me? Thank you
very much.

Regards,
Vincent
>
> > On Sep 16, 2021, at 1:03 AM, Vincent Chen <vincent.chen@sifive.com> wrote:
> >
> > On Mon, Sep 13, 2021 at 9:52 PM Rich Felker <dalias@libc.org> wrote:
> >>
> >>> On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
> >>> * Vincent Chen:
> >>>
> >>>> Following the changes of struct sigcontext in Linux to reserve about 5K space
> >>>> to support future ISA expansion.
> >>>> ---
> >>>> sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
> >>>> 1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> >>>> index cfafa44..80caf07 100644
> >>>> --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> >>>> +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> >>>> @@ -82,6 +82,8 @@ typedef struct mcontext_t
> >>>>   {
> >>>>     __riscv_mc_gp_state __gregs;
> >>>>     union  __riscv_mc_fp_state __fpregs;
> >>>> +    /* 5K + 256 reserved for vector state and future expansion.  */
> >>>> +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
> >>>>   } mcontext_t;
> >>>
> > Hi Florian and Rich,
> > Sorry for the late reply and thank you for reminding me the
> > modification will cause ABI break.
> >
> >>> This changes the size of struct ucontext_t, which is an ABI break
> >>> (getcontext callers are supposed to provide their own object).
> >>>
> >
> > The riscv vector registers are all caller-saved registers except for
> > VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> > it. In addition, RISCV ISA is growing, so I also hope the struct
> > mcontext_t has a space for future expansion. Based on the above ideas,
> > I reserved a 5K space here.
> >
> >>> This shouldn't be necessary if the additional vector registers are
> >>> caller-saved.
> >
> > Here I am a little confused about the usage of struct mcontext_t. As
> > far as I know, the struct mcontext_t is used to save the
> > machine-specific information in user context operation. Therefore, in
> > this case, the struct mcontext_t is allowed to reserve the space only
> > for saving caller-saved registers. However, in the signal handler, the
> > user seems to be allowed to use uc_mcontext whose data type is struct
> > mcontext_t to access the content of the signal context. In this case,
> > the struct mcontext_t may need to be the same as the struct sigcontext
> > defined at kernel. However, it will have a conflict with your
> > suggestion because the struct sigcontext cannot just reserve a space
> > for saving caller-saved registers. Could you help me point out my
> > misunderstanding? Thank you.
> >
> >> Indeed, that was my first thought when I saw this too. Any late
> >> additions to the register file must be call-clobbered or else they are
> >> a new ABI. And mcontext_t does not need to represent any
> >> call-clobbered state.
> >>
> >> Rich
> >
>
  

On Sat, Sep 18, 2021 at 1:03 AM Rich Felker <dalias@libc.org> wrote:
>
> On Thu, Sep 16, 2021 at 04:02:50PM +0800, Vincent Chen wrote:
> > On Mon, Sep 13, 2021 at 9:52 PM Rich Felker <dalias@libc.org> wrote:
> > >
> > > On Mon, Sep 13, 2021 at 03:44:09PM +0200, Florian Weimer via Libc-alpha wrote:
> > > > * Vincent Chen:
> > > >
> > > > > Following the changes of struct sigcontext in Linux to reserve about 5K space
> > > > > to support future ISA expansion.
> > > > > ---
> > > > >  sysdeps/unix/sysv/linux/riscv/sys/ucontext.h | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > > index cfafa44..80caf07 100644
> > > > > --- a/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > > +++ b/sysdeps/unix/sysv/linux/riscv/sys/ucontext.h
> > > > > @@ -82,6 +82,8 @@ typedef struct mcontext_t
> > > > >    {
> > > > >      __riscv_mc_gp_state __gregs;
> > > > >      union  __riscv_mc_fp_state __fpregs;
> > > > > +    /* 5K + 256 reserved for vector state and future expansion.  */
> > > > > +    unsigned char __reserved[5376] __attribute__ ((__aligned__ (16)));
> > > > >    } mcontext_t;
> > > >
> > Hi Florian and Rich,
> > Sorry for the late reply and thank you for reminding me the
> > modification will cause ABI break.
> >
> > > > This changes the size of struct ucontext_t, which is an ABI break
> > > > (getcontext callers are supposed to provide their own object).
> > > >
> >
> > The riscv vector registers are all caller-saved registers except for
> > VCSR. Therefore, the struct mcontext_t needs to reserve a space for
> > it. In addition, RISCV ISA is growing, so I also hope the struct
> > mcontext_t has a space for future expansion. Based on the above ideas,
> > I reserved a 5K space here.
>
> VCSR is not call-saved (aka 'callee-saved' in alternate notation)
> either. It's thread-local state that may be changed or left alone by
> calls, and that sj/lj/ucontext functions can't touch, just like fenv.
> Saving and restoring it here would be wrong.
>

You are right. Joseph pointed it out in my 3rd patch. I will remove it
from my next version patch. Thank you.
  

Vincent Chen <vincent.chen@sifive.com> writes:
> I am not familiar with the mechanism of LD_AUDIT, so I actually do not
> know if this modification may have any effect on LD_AUDIT. If
> possible, could you briefly introduce the issues for me? Thank you
> very much.

In general, when function foo() calls DSO function bar(), and bar() is
in an object that needs to be loaded from disk, the loader needs to save
foo()'s context, do a bunch of work, restore the context, and call
bar().

The LD_AUDIT feature adds a lot more "do a bunch of work" both on the
foo->bar call, and on the bar->foo return, typically calling some third
party functions to process the audit messages.

However, if the "do a bunch of work" changes registers that aren't saved
in the context, and aren't agreed on as "call clobbered" and thus
changeable, problems happen.  If foo() expects a register to be
preserved across the call to bar(), and the loader and audit functions
don't know that and clobber it, foo() breaks.

If everything is built with the same ISA, this normally isn't a problem,
but when you mix ISAs, or use optional/experimental ISAs that glibc (or
the auding code) doesn't know about, it may be.
  

* DJ Delorie via Libc-alpha:

> Vincent Chen <vincent.chen@sifive.com> writes:
>> I am not familiar with the mechanism of LD_AUDIT, so I actually do not
>> know if this modification may have any effect on LD_AUDIT. If
>> possible, could you briefly introduce the issues for me? Thank you
>> very much.
>
> In general, when function foo() calls DSO function bar(), and bar() is
> in an object that needs to be loaded from disk, the loader needs to save
> foo()'s context, do a bunch of work, restore the context, and call
> bar().
>
> The LD_AUDIT feature adds a lot more "do a bunch of work" both on the
> foo->bar call, and on the bar->foo return, typically calling some third
> party functions to process the audit messages.
>
> However, if the "do a bunch of work" changes registers that aren't saved
> in the context, and aren't agreed on as "call clobbered" and thus
> changeable, problems happen.  If foo() expects a register to be
> preserved across the call to bar(), and the loader and audit functions
> don't know that and clobber it, foo() breaks.

One point of clarification:

The issue is with register usage for passing argument and return values.
It's more or less unrelated to whether registers are callee-saved or
caller-saved.  So you need special LD_AUDIT support as soon it's
possible to pass vector arguments and return values in registers (as
opposed to memory).

Thanks,
Florian
  

On Tue, Sep 21, 2021 at 1:10 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * DJ Delorie via Libc-alpha:
>
> > Vincent Chen <vincent.chen@sifive.com> writes:
> >> I am not familiar with the mechanism of LD_AUDIT, so I actually do not
> >> know if this modification may have any effect on LD_AUDIT. If
> >> possible, could you briefly introduce the issues for me? Thank you
> >> very much.
> >
> > In general, when function foo() calls DSO function bar(), and bar() is
> > in an object that needs to be loaded from disk, the loader needs to save
> > foo()'s context, do a bunch of work, restore the context, and call
> > bar().
> >
> > The LD_AUDIT feature adds a lot more "do a bunch of work" both on the
> > foo->bar call, and on the bar->foo return, typically calling some third
> > party functions to process the audit messages.
> >
> > However, if the "do a bunch of work" changes registers that aren't saved
> > in the context, and aren't agreed on as "call clobbered" and thus
> > changeable, problems happen.  If foo() expects a register to be
> > preserved across the call to bar(), and the loader and audit functions
> > don't know that and clobber it, foo() breaks.
>
> One point of clarification:
>
> The issue is with register usage for passing argument and return values.
> It's more or less unrelated to whether registers are callee-saved or
> caller-saved.  So you need special LD_AUDIT support as soon it's
> possible to pass vector arguments and return values in registers (as
> opposed to memory).
>
> Thanks,
> Florian
>
Thank DJ Delorie and Florian very much for the detailed explanation
and clarification. It is really helpful for me to understand this
problem I have not noticed. Currently, I have some findings. If my
understanding is wrong, please correct me. Thank you.

The ABI for using vector registers to pass arguments and return value
is under discussion and close to ratification. As far as I know, the
riscv Glibc resolver will have a similar issue to this LD_AUDIT
problem after this new ABI is used. Hsiangkai Wang has sent a patch,
https://sourceware.org/pipermail/libc-alpha/2021-August/129931.html,
to deal with this issue in GLIBC resolver. This patch adds a new tag,
STO_RISCV_VARIANT_CC, to indicate whether the function uses this new
ABI or not. During the relocation process, if STO_RISCV_VARIANT_CC is
set in the st_other field of the symbol being processed, the delayed
binding mechanism will be disabled. It can avoid saving vector
registers before entering the resolver function.

The LD_AUDIT problem is similar to this because we need to prevent the
auditing function from clobbering the vector registers that store the
argument to pass to the audited symbol. Therefore, I think this patch
can resolve the LD_AUDIT issue as well.
  

On 30/09/2021 22:43, Vincent Chen wrote:
> On Tue, Sep 21, 2021 at 1:10 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * DJ Delorie via Libc-alpha:
>>
>>> Vincent Chen <vincent.chen@sifive.com> writes:
>>>> I am not familiar with the mechanism of LD_AUDIT, so I actually do not
>>>> know if this modification may have any effect on LD_AUDIT. If
>>>> possible, could you briefly introduce the issues for me? Thank you
>>>> very much.
>>>
>>> In general, when function foo() calls DSO function bar(), and bar() is
>>> in an object that needs to be loaded from disk, the loader needs to save
>>> foo()'s context, do a bunch of work, restore the context, and call
>>> bar().
>>>
>>> The LD_AUDIT feature adds a lot more "do a bunch of work" both on the
>>> foo->bar call, and on the bar->foo return, typically calling some third
>>> party functions to process the audit messages.
>>>
>>> However, if the "do a bunch of work" changes registers that aren't saved
>>> in the context, and aren't agreed on as "call clobbered" and thus
>>> changeable, problems happen.  If foo() expects a register to be
>>> preserved across the call to bar(), and the loader and audit functions
>>> don't know that and clobber it, foo() breaks.
>>
>> One point of clarification:
>>
>> The issue is with register usage for passing argument and return values.
>> It's more or less unrelated to whether registers are callee-saved or
>> caller-saved.  So you need special LD_AUDIT support as soon it's
>> possible to pass vector arguments and return values in registers (as
>> opposed to memory).
>>
>> Thanks,
>> Florian
>>
> Thank DJ Delorie and Florian very much for the detailed explanation
> and clarification. It is really helpful for me to understand this
> problem I have not noticed. Currently, I have some findings. If my
> understanding is wrong, please correct me. Thank you.
> 
> The ABI for using vector registers to pass arguments and return value
> is under discussion and close to ratification. As far as I know, the
> riscv Glibc resolver will have a similar issue to this LD_AUDIT
> problem after this new ABI is used. Hsiangkai Wang has sent a patch,
> https://sourceware.org/pipermail/libc-alpha/2021-August/129931.html,
> to deal with this issue in GLIBC resolver. This patch adds a new tag,
> STO_RISCV_VARIANT_CC, to indicate whether the function uses this new
> ABI or not. During the relocation process, if STO_RISCV_VARIANT_CC is
> set in the st_other field of the symbol being processed, the delayed
> binding mechanism will be disabled. It can avoid saving vector
> registers before entering the resolver function.
> 
> The LD_AUDIT problem is similar to this because we need to prevent the
> auditing function from clobbering the vector registers that store the
> argument to pass to the audited symbol. Therefore, I think this patch
> can resolve the LD_AUDIT issue as well.
> 

It solves the LD_AUDIT issue by not enabling it, so it won't work with
STO_RISCV_VARIANT_CC. It is essentially the same issue we have for AArch64
SVE. 

It should be a fair approach to just not support it, although it seems
that HPC tools does use it extensively for some specific usercases.  For
SVE case I tried to fix by saving/restoring the required SVE defined
registers by ABI argument passing [1], although the ABI does not really
defines it (Szabolcs thinks we should save *all* registers).

In any case, LD_AUDIT is corner case and I think we can workaround within
glibc (since we can use a different runtime trampoline to save/restore
the required state).

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c8315ccd30fcecc1b93a9bc3f073010190a86e05;hp=171fdd4bd4f337001db053721477add60d205ed8
  

Hi Florian,

P.S. Since I'm revisiting a year old thread with some new CC recipients, 
here's the link to original patch/thread [1]

On 9/17/21 20:04, Vincent Chen wrote:
> On Thu, Sep 16, 2021 at 4:14 PM Florian Weimer <fweimer@redhat.com> wrote:
>>>>> This changes the size of struct ucontext_t, which is an ABI break
>>>>> (getcontext callers are supposed to provide their own object).
>>>>>
>>> The riscv vector registers are all caller-saved registers except for
>>> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
>>> it. In addition, RISCV ISA is growing, so I also hope the struct
>>> mcontext_t has a space for future expansion. Based on the above ideas,
>>> I reserved a 5K space here.
>> You have reserved space in ucontext_t that you could use for this.
>>
> Sorry, I cannot really understand what you mean. The following is the
> contents of ucontext_t
> typedef struct ucontext_t
>    {
>      unsigned long int  __uc_flags;
>      struct ucontext_t *uc_link;
>      stack_t            uc_stack;
>      sigset_t           uc_sigmask;
>      /* There's some padding here to allow sigset_t to be expanded in the
>         future.  Though this is unlikely, other architectures put uc_sigmask
>         at the end of this structure and explicitly state it can be
>         expanded, so we didn't want to box ourselves in here.  */
>      char               __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
>      /* We can't put uc_sigmask at the end of this structure because we need
>         to be able to expand sigcontext in the future.  For example, the
>         vector ISA extension will almost certainly add ISA state.  We want
>         to ensure all user-visible ISA state can be saved and restored via a
>         ucontext, so we're putting this at the end in order to allow for
>         infinite extensibility.  Since we know this will be extended and we
>         assume sigset_t won't be extended an extreme amount, we're
>         prioritizing this.  */
>      mcontext_t uc_mcontext;
>    } ucontext_t;
>
> Currently, we only reserve a space, __glibc_reserved[], for the future
> expansion of sigset_t.
> Do you mean I could use __glibc_reserved[] to for future expansion of
> ISA as well?

Given unlikely sigset expansion, we could in theory use some of those 
reserved fields to store pointers (offsets) to actual V state, but not 
for actual V state which is way too large for non-embedded machines with 
typical 128 or even wider V regs.


>
>>>>> This shouldn't be necessary if the additional vector registers are
>>>>> caller-saved.
>>> Here I am a little confused about the usage of struct mcontext_t. As
>>> far as I know, the struct mcontext_t is used to save the
>>> machine-specific information in user context operation. Therefore, in
>>> this case, the struct mcontext_t is allowed to reserve the space only
>>> for saving caller-saved registers. However, in the signal handler, the
>>> user seems to be allowed to use uc_mcontext whose data type is struct
>>> mcontext_t to access the content of the signal context. In this case,
>>> the struct mcontext_t may need to be the same as the struct sigcontext
>>> defined at kernel. However, it will have a conflict with your
>>> suggestion because the struct sigcontext cannot just reserve a space
>>> for saving caller-saved registers. Could you help me point out my
>>> misunderstanding? Thank you.

I think the confusion comes from apparent equivalence of kernel struct 
sigcontext and glibc mcontext_t as they appear in respective struct 
ucontext definitions.
I've enumerated the actual RV structs below to keep them handy in one 
place for discussion.

>> struct sigcontext is allocated by the kernel, so you can have pointers
>> in reserved fields to out-of-line start, or after struct sigcontext.

In this scheme, would the actual V regfile contents (at the out-of-line 
location w.r.t kernel sigcontext) be anonymous for glibc i.e. do we not 
need to expose them to glibc userspace ABI ?


>> I don't know how the kernel implements this, but there is considerable
>> flexibility and extensibility.  The main issues comes from small stacks
>> which are incompatible with large register files.

Simplistically, Linux kernel needs to preserve the V regfile across task 
switch. The necessary evil that follows is preserving V across 
signal-handling (sigaction/sigreturn).

In RV kernel we have following:

struct rt_sigframe {
   struct siginfo info;
   struct ucontext uc;
};

struct ucontext {
    unsigned long uc_flags;
    struct ucontext *uc_link;
    stack_t uc_stack;
    sigset_t uc_sigmask;
    __u8 __unused[1024 / 8 - sizeof(sigset_t)];     // this is for 
sigset_t expansion
    struct sigcontext uc_mcontext;
};

struct sigcontext {
    struct user_regs_struct sc_regs;
    union __riscv_fp_state sc_fpregs;
+  __u8 sc_extn[4096+128] __attribute__((__aligned__(16)));   // handle 
128B V regs
};

The sc_extn[] would have V state (regfile + control state) in kernel 
defined format.

As I understand it, you are suggesting to prevent ABI break, we should 
not add anything to kernel struct sigcontext i.e. do something like this

struct rt_sigframe {
   struct siginfo info;
   struct ucontext uc;
+__u8 sc_extn[4096+128] __attribute__((__aligned__(16)));
}

So kernel sig handling can continue to save/restore the V regfile on 
user stack, w/o making it part of actual struct sigcontext.
So they are not explicitly visible to userspace at all - is that 
feasible ? I know that SA_SIGINFO handlers can access the scalar/fp 
regs, they won't do it V.
Is there a POSIX req for SA_SIGINFO handlers being able to access all 
machine regs saved by signal handling.

An alternate approach is what Vincent did originally, to add sc_exn to 
struct sigcontext. Here to prevent ABI breakage, we can choose to not 
reflect this in the glibc sigcontext. But the question remains, is that OK ?

The other topic is changing glibc mcontext_t to add V-regs. It would 
seem one has to as mcontext is "visually equivalent" to struct 
sigcontext in the respective ucontext structs. But in unserspace 
*context routine semantics only require callee-regs to be saved, which V 
regs are not per psABI [2]. So looks like this can be avoided which is 
what Vincent did in v2 series [3]


[1] https://sourceware.org/pipermail/libc-alpha/2021-September/130899.html
[2] 
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc
[3] https://sourceware.org/pipermail/libc-alpha/2022-January/135416.html
  

+CC Greentime and Andy

On 12/8/22 19:39, Vineet Gupta wrote:
> Hi Florian,
>
> P.S. Since I'm revisiting a year old thread with some new CC 
> recipients, here's the link to original patch/thread [1]
>
> On 9/17/21 20:04, Vincent Chen wrote:
>> On Thu, Sep 16, 2021 at 4:14 PM Florian Weimer <fweimer@redhat.com> 
>> wrote:
>>>>>> This changes the size of struct ucontext_t, which is an ABI break
>>>>>> (getcontext callers are supposed to provide their own object).
>>>>>>
>>>> The riscv vector registers are all caller-saved registers except for
>>>> VCSR. Therefore, the struct mcontext_t needs to reserve a space for
>>>> it. In addition, RISCV ISA is growing, so I also hope the struct
>>>> mcontext_t has a space for future expansion. Based on the above ideas,
>>>> I reserved a 5K space here.
>>> You have reserved space in ucontext_t that you could use for this.
>>>
>> Sorry, I cannot really understand what you mean. The following is the
>> contents of ucontext_t
>> typedef struct ucontext_t
>>    {
>>      unsigned long int  __uc_flags;
>>      struct ucontext_t *uc_link;
>>      stack_t            uc_stack;
>>      sigset_t           uc_sigmask;
>>      /* There's some padding here to allow sigset_t to be expanded in 
>> the
>>         future.  Though this is unlikely, other architectures put 
>> uc_sigmask
>>         at the end of this structure and explicitly state it can be
>>         expanded, so we didn't want to box ourselves in here. */
>>      char               __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
>>      /* We can't put uc_sigmask at the end of this structure because 
>> we need
>>         to be able to expand sigcontext in the future.  For example, the
>>         vector ISA extension will almost certainly add ISA state.  We 
>> want
>>         to ensure all user-visible ISA state can be saved and 
>> restored via a
>>         ucontext, so we're putting this at the end in order to allow for
>>         infinite extensibility.  Since we know this will be extended 
>> and we
>>         assume sigset_t won't be extended an extreme amount, we're
>>         prioritizing this.  */
>>      mcontext_t uc_mcontext;
>>    } ucontext_t;
>>
>> Currently, we only reserve a space, __glibc_reserved[], for the future
>> expansion of sigset_t.
>> Do you mean I could use __glibc_reserved[] to for future expansion of
>> ISA as well?
>
> Given unlikely sigset expansion, we could in theory use some of those 
> reserved fields to store pointers (offsets) to actual V state, but not 
> for actual V state which is way too large for non-embedded machines 
> with typical 128 or even wider V regs.
>
>
>>
>>>>>> This shouldn't be necessary if the additional vector registers are
>>>>>> caller-saved.
>>>> Here I am a little confused about the usage of struct mcontext_t. As
>>>> far as I know, the struct mcontext_t is used to save the
>>>> machine-specific information in user context operation. Therefore, in
>>>> this case, the struct mcontext_t is allowed to reserve the space only
>>>> for saving caller-saved registers. However, in the signal handler, the
>>>> user seems to be allowed to use uc_mcontext whose data type is struct
>>>> mcontext_t to access the content of the signal context. In this case,
>>>> the struct mcontext_t may need to be the same as the struct sigcontext
>>>> defined at kernel. However, it will have a conflict with your
>>>> suggestion because the struct sigcontext cannot just reserve a space
>>>> for saving caller-saved registers. Could you help me point out my
>>>> misunderstanding? Thank you.
>
> I think the confusion comes from apparent equivalence of kernel struct 
> sigcontext and glibc mcontext_t as they appear in respective struct 
> ucontext definitions.
> I've enumerated the actual RV structs below to keep them handy in one 
> place for discussion.
>
>>> struct sigcontext is allocated by the kernel, so you can have pointers
>>> in reserved fields to out-of-line start, or after struct sigcontext.
>
> In this scheme, would the actual V regfile contents (at the 
> out-of-line location w.r.t kernel sigcontext) be anonymous for glibc 
> i.e. do we not need to expose them to glibc userspace ABI ?
>
>
>>> I don't know how the kernel implements this, but there is considerable
>>> flexibility and extensibility.  The main issues comes from small stacks
>>> which are incompatible with large register files.
>
> Simplistically, Linux kernel needs to preserve the V regfile across 
> task switch. The necessary evil that follows is preserving V across 
> signal-handling (sigaction/sigreturn).
>
> In RV kernel we have following:
>
> struct rt_sigframe {
>   struct siginfo info;
>   struct ucontext uc;
> };
>
> struct ucontext {
>    unsigned long uc_flags;
>    struct ucontext *uc_link;
>    stack_t uc_stack;
>    sigset_t uc_sigmask;
>    __u8 __unused[1024 / 8 - sizeof(sigset_t)];     // this is for 
> sigset_t expansion
>    struct sigcontext uc_mcontext;
> };
>
> struct sigcontext {
>    struct user_regs_struct sc_regs;
>    union __riscv_fp_state sc_fpregs;
> +  __u8 sc_extn[4096+128] __attribute__((__aligned__(16)));   // 
> handle 128B V regs
> };
>
> The sc_extn[] would have V state (regfile + control state) in kernel 
> defined format.
>
> As I understand it, you are suggesting to prevent ABI break, we should 
> not add anything to kernel struct sigcontext i.e. do something like this
>
> struct rt_sigframe {
>   struct siginfo info;
>   struct ucontext uc;
> +__u8 sc_extn[4096+128] __attribute__((__aligned__(16)));
> }
>
> So kernel sig handling can continue to save/restore the V regfile on 
> user stack, w/o making it part of actual struct sigcontext.
> So they are not explicitly visible to userspace at all - is that 
> feasible ? I know that SA_SIGINFO handlers can access the scalar/fp 
> regs, they won't do it V.
> Is there a POSIX req for SA_SIGINFO handlers being able to access all 
> machine regs saved by signal handling.
>
> An alternate approach is what Vincent did originally, to add sc_exn to 
> struct sigcontext. Here to prevent ABI breakage, we can choose to not 
> reflect this in the glibc sigcontext. But the question remains, is 
> that OK ?
>
> The other topic is changing glibc mcontext_t to add V-regs. It would 
> seem one has to as mcontext is "visually equivalent" to struct 
> sigcontext in the respective ucontext structs. But in unserspace 
> *context routine semantics only require callee-regs to be saved, which 
> V regs are not per psABI [2]. So looks like this can be avoided which 
> is what Vincent did in v2 series [3]
>
>
> [1] 
> https://sourceware.org/pipermail/libc-alpha/2021-September/130899.html
> [2] 
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc
> [3] https://sourceware.org/pipermail/libc-alpha/2022-January/135416.html