2020-05-25 03:49:55

by YuanJunQing

[permalink] [raw]
Subject: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

Register "a1" is unsaved in this function,
when CONFIG_TRACE_IRQFLAGS is enabled,
the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
and this may change register "a1".
The variment of register "a1" may send SIGFPE signal
to task when call do_fpe(),and this may kill the task.

Signed-off-by: YuanJunQing <[email protected]>
---
arch/mips/kernel/genex.S | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 8236fb291e3f..956a76429773 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
/* gas fails to assemble cfc1 for some archs (octeon).*/ \
.set mips1
SET_HARDFLOAT
- cfc1 a1, fcr31
+ cfc1 s0, fcr31
.set pop
CLI
TRACE_IRQS_OFF
+ move a1,s0
.endm

.macro __build_clear_msa_fpe
- _cfcmsa a1, MSA_CSR
+ _cfcmsa s0, MSA_CSR
CLI
TRACE_IRQS_OFF
+ move a1,s0
.endm

.macro __build_clear_ade
--
2.17.1


2020-05-25 08:37:18

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

Hello!

On 25.05.2020 6:31, YuanJunQing wrote:

> Register "a1" is unsaved in this function,
> when CONFIG_TRACE_IRQFLAGS is enabled,
> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
> and this may change register "a1".
> The variment of register "a1" may send SIGFPE signal

Variment?

> to task when call do_fpe(),and this may kill the task.

Need space after comma.

> Signed-off-by: YuanJunQing <[email protected]>
> ---
> arch/mips/kernel/genex.S | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index 8236fb291e3f..956a76429773 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
> /* gas fails to assemble cfc1 for some archs (octeon).*/ \
> .set mips1
> SET_HARDFLOAT
> - cfc1 a1, fcr31
> + cfc1 s0, fcr31
> .set pop
> CLI
> TRACE_IRQS_OFF
> + move a1,s0

Need space after comma.

> .endm
>
> .macro __build_clear_msa_fpe
> - _cfcmsa a1, MSA_CSR
> + _cfcmsa s0, MSA_CSR
> CLI
> TRACE_IRQS_OFF
> + move a1,s0

Ditto.

[...]

MBR, Sergei

2020-05-25 08:46:16

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

On Mon, May 25, 2020 at 11:31:23AM +0800, YuanJunQing wrote:
> Register "a1" is unsaved in this function,
> when CONFIG_TRACE_IRQFLAGS is enabled,
> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
> and this may change register "a1".
> The variment of register "a1" may send SIGFPE signal
> to task when call do_fpe(),and this may kill the task.
>
> Signed-off-by: YuanJunQing <[email protected]>
> ---
> arch/mips/kernel/genex.S | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index 8236fb291e3f..956a76429773 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
> /* gas fails to assemble cfc1 for some archs (octeon).*/ \
> .set mips1
> SET_HARDFLOAT
> - cfc1 a1, fcr31
> + cfc1 s0, fcr31
> .set pop
> CLI
> TRACE_IRQS_OFF
> + move a1,s0
> .endm

do we realy need to read fcr31 that early ? Wouldn't it work to
just move the cfc1 below TRACE_IRQS_OFF ?

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2020-05-26 07:59:48

by YuanJunQing

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()


在 2020/5/25 下午4:42, Thomas Bogendoerfer 写道:
> On Mon, May 25, 2020 at 11:31:23AM +0800, YuanJunQing wrote:
>> Register "a1" is unsaved in this function,
>> when CONFIG_TRACE_IRQFLAGS is enabled,
>> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
>> and this may change register "a1".
>> The variment of register "a1" may send SIGFPE signal
>> to task when call do_fpe(),and this may kill the task.
>>
>> Signed-off-by: YuanJunQing <[email protected]>
>> ---
>> arch/mips/kernel/genex.S | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
>> index 8236fb291e3f..956a76429773 100644
>> --- a/arch/mips/kernel/genex.S
>> +++ b/arch/mips/kernel/genex.S
>> @@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
>> /* gas fails to assemble cfc1 for some archs (octeon).*/ \
>> .set mips1
>> SET_HARDFLOAT
>> - cfc1 a1, fcr31
>> + cfc1 s0, fcr31
>> .set pop
>> CLI
>> TRACE_IRQS_OFF
>> + move a1,s0
>> .endm
> do we realy need to read fcr31 that early ? Wouldn't it work to
> just move the cfc1 below TRACE_IRQS_OFF ?
>
> Thomas.


yes, it can work when we just move the cfc1 below TRACE_IRQS_OFF,
 and the code is written as follows.

CLI
TRACE_IRQS_OFF
.set mips1
SET_HARDFLOAT
cfc1 a1, fcr31
.set pop
  .endm


2020-05-26 13:52:57

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

On Tue, May 26, 2020 at 03:07:16PM +0800, yuanjunqing wrote:
>
> 在 2020/5/25 下午4:42, Thomas Bogendoerfer 写道:
> > On Mon, May 25, 2020 at 11:31:23AM +0800, YuanJunQing wrote:
> >> Register "a1" is unsaved in this function,
> >> when CONFIG_TRACE_IRQFLAGS is enabled,
> >> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
> >> and this may change register "a1".
> >> The variment of register "a1" may send SIGFPE signal
> >> to task when call do_fpe(),and this may kill the task.
> >>
> >> Signed-off-by: YuanJunQing <[email protected]>
> >> ---
> >> arch/mips/kernel/genex.S | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> >> index 8236fb291e3f..956a76429773 100644
> >> --- a/arch/mips/kernel/genex.S
> >> +++ b/arch/mips/kernel/genex.S
> >> @@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
> >> /* gas fails to assemble cfc1 for some archs (octeon).*/ \
> >> .set mips1
> >> SET_HARDFLOAT
> >> - cfc1 a1, fcr31
> >> + cfc1 s0, fcr31
> >> .set pop
> >> CLI
> >> TRACE_IRQS_OFF
> >> + move a1,s0
> >> .endm
> > do we realy need to read fcr31 that early ? Wouldn't it work to
> > just move the cfc1 below TRACE_IRQS_OFF ?
> >
>
> yes, it can work when we just move the cfc1 below TRACE_IRQS_OFF,
>  and the code is written as follows.
>
> CLI
> TRACE_IRQS_OFF
> .set mips1
> SET_HARDFLOAT
> cfc1 a1, fcr31
> .set pop
>   .endm

good, could we do the same with _cfcmsa a1, MSA_CSR in the msa case ?

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2020-05-26 13:52:57

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

On Tue, May 26, 2020 at 02:03:14PM +0800, Lichao Liu wrote:
> From: YuanJunQing <[email protected]>
>
> Register "a1" is unsaved in this function,
> when CONFIG_TRACE_IRQFLAGS is enabled,
> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
> and this may change register "a1".
> The variment of register "a1" may send SIGFPE signal
> to task when call do_fpe(),and this may kill the task.
>
> Signed-off-by: YuanJunQing <[email protected]>

if you send patches from other people, please add your
Signed-off-by.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2020-05-27 10:16:51

by YuanJunQing

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Fix IRQ tracing when call handle_fpe()

yes, I will re-send email for this patch.

在 2020/5/26 下午9:04, Thomas Bogendoerfer 写道:
> On Tue, May 26, 2020 at 03:07:16PM +0800, yuanjunqing wrote:
>> 在 2020/5/25 下午4:42, Thomas Bogendoerfer 写道:
>>> On Mon, May 25, 2020 at 11:31:23AM +0800, YuanJunQing wrote:
>>>> Register "a1" is unsaved in this function,
>>>> when CONFIG_TRACE_IRQFLAGS is enabled,
>>>> the TRACE_IRQS_OFF macro will call trace_hardirqs_off(),
>>>> and this may change register "a1".
>>>> The variment of register "a1" may send SIGFPE signal
>>>> to task when call do_fpe(),and this may kill the task.
>>>>
>>>> Signed-off-by: YuanJunQing <[email protected]>
>>>> ---
>>>> arch/mips/kernel/genex.S | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
>>>> index 8236fb291e3f..956a76429773 100644
>>>> --- a/arch/mips/kernel/genex.S
>>>> +++ b/arch/mips/kernel/genex.S
>>>> @@ -480,16 +480,18 @@ NESTED(nmi_handler, PT_SIZE, sp)
>>>> /* gas fails to assemble cfc1 for some archs (octeon).*/ \
>>>> .set mips1
>>>> SET_HARDFLOAT
>>>> - cfc1 a1, fcr31
>>>> + cfc1 s0, fcr31
>>>> .set pop
>>>> CLI
>>>> TRACE_IRQS_OFF
>>>> + move a1,s0
>>>> .endm
>>> do we realy need to read fcr31 that early ? Wouldn't it work to
>>> just move the cfc1 below TRACE_IRQS_OFF ?
>>>
>> yes, it can work when we just move the cfc1 below TRACE_IRQS_OFF,
>>  and the code is written as follows.
>>
>> CLI
>> TRACE_IRQS_OFF
>> .set mips1
>> SET_HARDFLOAT
>> cfc1 a1, fcr31
>> .set pop
>>   .endm
> good, could we do the same with _cfcmsa a1, MSA_CSR in the msa case ?
>
> Thomas.
>