[PATCH] linux-user/riscv: fix up struct target_ucontext definition

[PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

As struct target_ucontext will be transfered to signal handler, it
must keep pace with struct ucontext_t defined in Linux kernel.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 linux-user/riscv/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
index 83ecc6f799..67a95dbc7b 100644
--- a/linux-user/riscv/signal.c
+++ b/linux-user/riscv/signal.c
@@ -40,8 +40,9 @@ struct target_ucontext {
     unsigned long uc_flags;
     struct target_ucontext *uc_link;
     target_stack_t uc_stack;
-    struct target_sigcontext uc_mcontext;
     target_sigset_t uc_sigmask;
+    uint8_t   __unused[1024 / 8 - sizeof(target_sigset_t)];
+    struct target_sigcontext uc_mcontext QEMU_ALIGNED(16);
 };
 
 struct target_rt_sigframe {
-- 
2.23.0

[PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

As struct target_ucontext will be transfered to signal handler, it
must keep pace with struct ucontext_t defined in Linux kernel.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 linux-user/riscv/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
index 83ecc6f799..67a95dbc7b 100644
--- a/linux-user/riscv/signal.c
+++ b/linux-user/riscv/signal.c
@@ -40,8 +40,9 @@ struct target_ucontext {
     unsigned long uc_flags;
     struct target_ucontext *uc_link;
     target_stack_t uc_stack;
-    struct target_sigcontext uc_mcontext;
     target_sigset_t uc_sigmask;
+    uint8_t   __unused[1024 / 8 - sizeof(target_sigset_t)];
+    struct target_sigcontext uc_mcontext QEMU_ALIGNED(16);
 };
 
 struct target_rt_sigframe {
-- 
2.23.0

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

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

Ping.

When I port RISU, I find this bug. I can't get the correct registers 
from the
struct ucontext_t parameter in the signal handler.

If you want to reproduce it, just   register a signal handler for SIGILL,
and  output an illegal instruction, such as

#include <stdio.h>
#include <stdlib.h>
#include <inttypes.h>
#include <string.h>
#include <signal.h>
#include <ucontext.h>

void sigill(int sig, siginfo_t *si, void *uc)
{
     printf("Illegal pc: %016" PRIx64 "\n",
            ((ucontext_t *)uc)->uc_mcontext.__gregs[0]);
}

static void set_sigill_handler(void (*fn) (int, siginfo_t *, void *))
{
     struct sigaction sa;
     memset(&sa, 0, sizeof(struct sigaction));

     sa.sa_sigaction = fn;
     sa.sa_flags = SA_SIGINFO;
     sigemptyset(&sa.sa_mask);
     if (sigaction(SIGILL, &sa, 0) != 0) {
         perror("sigaction");
         exit(1);
     }
}

int main()
{
     set_sigill_handler(sigill);
     asm(".dword 0x0000006b");
     return 0;
}
~

Zhiwei

On 2020/4/12 10:08, LIU Zhiwei wrote:
> As struct target_ucontext will be transfered to signal handler, it
> must keep pace with struct ucontext_t defined in Linux kernel.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>   linux-user/riscv/signal.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
> index 83ecc6f799..67a95dbc7b 100644
> --- a/linux-user/riscv/signal.c
> +++ b/linux-user/riscv/signal.c
> @@ -40,8 +40,9 @@ struct target_ucontext {
>       unsigned long uc_flags;
>       struct target_ucontext *uc_link;
>       target_stack_t uc_stack;
> -    struct target_sigcontext uc_mcontext;
>       target_sigset_t uc_sigmask;
> +    uint8_t   __unused[1024 / 8 - sizeof(target_sigset_t)];
> +    struct target_sigcontext uc_mcontext QEMU_ALIGNED(16);
>   };
>   
>   struct target_rt_sigframe {


[-- Attachment #2: Type: text/html, Size: 2579 bytes --]

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

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

Ping.

When I port RISU, I find this bug. I can't get the correct registers 
from the
struct ucontext_t parameter in the signal handler.

If you want to reproduce it, just   register a signal handler for SIGILL,
and  output an illegal instruction, such as

#include <stdio.h>
#include <stdlib.h>
#include <inttypes.h>
#include <string.h>
#include <signal.h>
#include <ucontext.h>

void sigill(int sig, siginfo_t *si, void *uc)
{
     printf("Illegal pc: %016" PRIx64 "\n",
            ((ucontext_t *)uc)->uc_mcontext.__gregs[0]);
}

static void set_sigill_handler(void (*fn) (int, siginfo_t *, void *))
{
     struct sigaction sa;
     memset(&sa, 0, sizeof(struct sigaction));

     sa.sa_sigaction = fn;
     sa.sa_flags = SA_SIGINFO;
     sigemptyset(&sa.sa_mask);
     if (sigaction(SIGILL, &sa, 0) != 0) {
         perror("sigaction");
         exit(1);
     }
}

int main()
{
     set_sigill_handler(sigill);
     asm(".dword 0x0000006b");
     return 0;
}
~

Zhiwei

On 2020/4/12 10:08, LIU Zhiwei wrote:
> As struct target_ucontext will be transfered to signal handler, it
> must keep pace with struct ucontext_t defined in Linux kernel.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> ---
>   linux-user/riscv/signal.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
> index 83ecc6f799..67a95dbc7b 100644
> --- a/linux-user/riscv/signal.c
> +++ b/linux-user/riscv/signal.c
> @@ -40,8 +40,9 @@ struct target_ucontext {
>       unsigned long uc_flags;
>       struct target_ucontext *uc_link;
>       target_stack_t uc_stack;
> -    struct target_sigcontext uc_mcontext;
>       target_sigset_t uc_sigmask;
> +    uint8_t   __unused[1024 / 8 - sizeof(target_sigset_t)];
> +    struct target_sigcontext uc_mcontext QEMU_ALIGNED(16);
>   };
>   
>   struct target_rt_sigframe {


[-- Attachment #2: Type: text/html, Size: 2579 bytes --]

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/21/20 7:34 PM, LIU Zhiwei wrote:
> Ping.
> 
> When I port RISU, I find this bug. I can't get the correct registers from the
> struct ucontext_t parameter in the signal handler.

The RISC-V Linux ABI will need to be extended to handle RVV state.

There is room in your sigcontext structure:

> struct __riscv_q_ext_state {
>         __u64 f[64] __attribute__((aligned(16)));
>         __u32 fcsr;
>         /*
>          * Reserved for expansion of sigcontext structure.  Currently zeroed
>          * upon signal, and must be zero upon sigreturn.
>          */
>         __u32 reserved[3];
> };

in uc->uc_mcontext.sc_fpregs.q.

That reserved field is going to have to be used in some way.

My suggestion is to use some sort of extendable record list, akin to AArch64:

struct _aarch64_ctx {
        __u32 magic;
        __u32 size;
};

One of the 3 zeros could be the total size of the extensions, so that it's easy
to validate the size or memcpy the lot without parsing each individual record.
 The other two zeros could be the first header of the next record.  Which in
this case also allows the payload of that first record to be aligned mod 16,
which could come in handy.

Talk to the risc-v kernel engineers and come up with a plan that includes room
for the next architecture extension as well.  They may have already done so,
but I'm not monitoring the correct mailing list to know.


r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/21/20 7:34 PM, LIU Zhiwei wrote:
> Ping.
> 
> When I port RISU, I find this bug. I can't get the correct registers from the
> struct ucontext_t parameter in the signal handler.

The RISC-V Linux ABI will need to be extended to handle RVV state.

There is room in your sigcontext structure:

> struct __riscv_q_ext_state {
>         __u64 f[64] __attribute__((aligned(16)));
>         __u32 fcsr;
>         /*
>          * Reserved for expansion of sigcontext structure.  Currently zeroed
>          * upon signal, and must be zero upon sigreturn.
>          */
>         __u32 reserved[3];
> };

in uc->uc_mcontext.sc_fpregs.q.

That reserved field is going to have to be used in some way.

My suggestion is to use some sort of extendable record list, akin to AArch64:

struct _aarch64_ctx {
        __u32 magic;
        __u32 size;
};

One of the 3 zeros could be the total size of the extensions, so that it's easy
to validate the size or memcpy the lot without parsing each individual record.
 The other two zeros could be the first header of the next record.  Which in
this case also allows the payload of that first record to be aligned mod 16,
which could come in handy.

Talk to the risc-v kernel engineers and come up with a plan that includes room
for the next architecture extension as well.  They may have already done so,
but I'm not monitoring the correct mailing list to know.


r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Alistair Francis]

On Tue, Apr 21, 2020 at 9:10 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/21/20 7:34 PM, LIU Zhiwei wrote:
> > Ping.
> >
> > When I port RISU, I find this bug. I can't get the correct registers from the
> > struct ucontext_t parameter in the signal handler.
>
> The RISC-V Linux ABI will need to be extended to handle RVV state.
>
> There is room in your sigcontext structure:
>
> > struct __riscv_q_ext_state {
> >         __u64 f[64] __attribute__((aligned(16)));
> >         __u32 fcsr;
> >         /*
> >          * Reserved for expansion of sigcontext structure.  Currently zeroed
> >          * upon signal, and must be zero upon sigreturn.
> >          */
> >         __u32 reserved[3];
> > };
>
> in uc->uc_mcontext.sc_fpregs.q.
>
> That reserved field is going to have to be used in some way.

Just to clarify, this patch is still correct right?

It looks good to me.

Alistair

>
> My suggestion is to use some sort of extendable record list, akin to AArch64:
>
> struct _aarch64_ctx {
>         __u32 magic;
>         __u32 size;
> };
>
> One of the 3 zeros could be the total size of the extensions, so that it's easy
> to validate the size or memcpy the lot without parsing each individual record.
>  The other two zeros could be the first header of the next record.  Which in
> this case also allows the payload of that first record to be aligned mod 16,
> which could come in handy.
>
> Talk to the risc-v kernel engineers and come up with a plan that includes room
> for the next architecture extension as well.  They may have already done so,
> but I'm not monitoring the correct mailing list to know.
>
>
> r~
>

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Alistair Francis]

On Tue, Apr 21, 2020 at 9:10 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/21/20 7:34 PM, LIU Zhiwei wrote:
> > Ping.
> >
> > When I port RISU, I find this bug. I can't get the correct registers from the
> > struct ucontext_t parameter in the signal handler.
>
> The RISC-V Linux ABI will need to be extended to handle RVV state.
>
> There is room in your sigcontext structure:
>
> > struct __riscv_q_ext_state {
> >         __u64 f[64] __attribute__((aligned(16)));
> >         __u32 fcsr;
> >         /*
> >          * Reserved for expansion of sigcontext structure.  Currently zeroed
> >          * upon signal, and must be zero upon sigreturn.
> >          */
> >         __u32 reserved[3];
> > };
>
> in uc->uc_mcontext.sc_fpregs.q.
>
> That reserved field is going to have to be used in some way.

Just to clarify, this patch is still correct right?

It looks good to me.

Alistair

>
> My suggestion is to use some sort of extendable record list, akin to AArch64:
>
> struct _aarch64_ctx {
>         __u32 magic;
>         __u32 size;
> };
>
> One of the 3 zeros could be the total size of the extensions, so that it's easy
> to validate the size or memcpy the lot without parsing each individual record.
>  The other two zeros could be the first header of the next record.  Which in
> this case also allows the payload of that first record to be aligned mod 16,
> which could come in handy.
>
> Talk to the risc-v kernel engineers and come up with a plan that includes room
> for the next architecture extension as well.  They may have already done so,
> but I'm not monitoring the correct mailing list to know.
>
>
> r~
>

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/22/20 11:05 AM, Alistair Francis wrote:
> Just to clarify, this patch is still correct right?

Yes.


r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/22/20 11:05 AM, Alistair Francis wrote:
> Just to clarify, this patch is still correct right?

Yes.


r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Alistair Francis]

On Wed, Apr 22, 2020 at 12:20 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/22/20 11:05 AM, Alistair Francis wrote:
> > Just to clarify, this patch is still correct right?
>
> Yes.

That's what I thought. Thanks :)

Applied to the RISC-V tree for 5.1.

Alistair

>
>
> r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Alistair Francis]

On Wed, Apr 22, 2020 at 12:20 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 4/22/20 11:05 AM, Alistair Francis wrote:
> > Just to clarify, this patch is still correct right?
>
> Yes.

That's what I thought. Thanks :)

Applied to the RISC-V tree for 5.1.

Alistair

>
>
> r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

On 2020/4/22 12:10, Richard Henderson wrote:
> On 4/21/20 7:34 PM, LIU Zhiwei wrote:
>> Ping.
>>
>> When I port RISU, I find this bug. I can't get the correct registers from the
>> struct ucontext_t parameter in the signal handler.
> The RISC-V Linux ABI will need to be extended to handle RVV state.
>
> There is room in your sigcontext structure:
>
>> struct __riscv_q_ext_state {
>>          __u64 f[64] __attribute__((aligned(16)));
>>          __u32 fcsr;
>>          /*
>>           * Reserved for expansion of sigcontext structure.  Currently zeroed
>>           * upon signal, and must be zero upon sigreturn.
>>           */
>>          __u32 reserved[3];
>> };
> in uc->uc_mcontext.sc_fpregs.q.
>
> That reserved field is going to have to be used in some way.
>
> My suggestion is to use some sort of extendable record list, akin to AArch64:
>
> struct _aarch64_ctx {
>          __u32 magic;
>          __u32 size;
> };
>
> One of the 3 zeros could be the total size of the extensions, so that it's easy
> to validate the size or memcpy the lot without parsing each individual record.
>   The other two zeros could be the first header of the next record.  Which in
> this case also allows the payload of that first record to be aligned mod 16,
> which could come in handy.
>
> Talk to the risc-v kernel engineers and come up with a plan that includes room
> for the next architecture extension as well.  They may have already done so,
> but I'm not monitoring the correct mailing list to know.
Hi Richard,

As far as I know, Guo Ren and Greentime are supporting RVV on Linux, 
based on the v0.7.1 QEMU implementation.
The main problem is that VLEN is not a  fixed number.

Thanks for your advice. I will communicate with them.

When the Linux kernel released with RVV, I will push a new sigcontext 
structure here.

Zhiwei
>
> r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[LIU Zhiwei]

On 2020/4/22 12:10, Richard Henderson wrote:
> On 4/21/20 7:34 PM, LIU Zhiwei wrote:
>> Ping.
>>
>> When I port RISU, I find this bug. I can't get the correct registers from the
>> struct ucontext_t parameter in the signal handler.
> The RISC-V Linux ABI will need to be extended to handle RVV state.
>
> There is room in your sigcontext structure:
>
>> struct __riscv_q_ext_state {
>>          __u64 f[64] __attribute__((aligned(16)));
>>          __u32 fcsr;
>>          /*
>>           * Reserved for expansion of sigcontext structure.  Currently zeroed
>>           * upon signal, and must be zero upon sigreturn.
>>           */
>>          __u32 reserved[3];
>> };
> in uc->uc_mcontext.sc_fpregs.q.
>
> That reserved field is going to have to be used in some way.
>
> My suggestion is to use some sort of extendable record list, akin to AArch64:
>
> struct _aarch64_ctx {
>          __u32 magic;
>          __u32 size;
> };
>
> One of the 3 zeros could be the total size of the extensions, so that it's easy
> to validate the size or memcpy the lot without parsing each individual record.
>   The other two zeros could be the first header of the next record.  Which in
> this case also allows the payload of that first record to be aligned mod 16,
> which could come in handy.
>
> Talk to the risc-v kernel engineers and come up with a plan that includes room
> for the next architecture extension as well.  They may have already done so,
> but I'm not monitoring the correct mailing list to know.
Hi Richard,

As far as I know, Guo Ren and Greentime are supporting RVV on Linux, 
based on the v0.7.1 QEMU implementation.
The main problem is that VLEN is not a  fixed number.

Thanks for your advice. I will communicate with them.

When the Linux kernel released with RVV, I will push a new sigcontext 
structure here.

Zhiwei
>
> r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/22/20 6:55 PM, LIU Zhiwei wrote:
> As far as I know, Guo Ren and Greentime are supporting RVV on Linux, based on
> the v0.7.1 QEMU implementation.
> The main problem is that VLEN is not a  fixed number.

Neither is the SVE vector length fixed.

That's one of the reasons I pointed you at the AArch64 sigcontext for ideas.

> When the Linux kernel released with RVV, I will push a new sigcontext structure
> here.

Yep.


r~

Re: [PATCH] linux-user/riscv: fix up struct target_ucontext definition

[Richard Henderson]

On 4/22/20 6:55 PM, LIU Zhiwei wrote:
> As far as I know, Guo Ren and Greentime are supporting RVV on Linux, based on
> the v0.7.1 QEMU implementation.
> The main problem is that VLEN is not a  fixed number.

Neither is the SVE vector length fixed.

That's one of the reasons I pointed you at the AArch64 sigcontext for ideas.

> When the Linux kernel released with RVV, I will push a new sigcontext structure
> here.

Yep.


r~