Re: [PATCH RFC 1/2] m68k: Add tracirqs

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Jean-Michel,

Am 23.10.2024 um 18:53 schrieb Jean-Michel Hautbois:
> Hi Michael,
>
> On 23/10/2024 05:53, Michael Schmitz wrote:
>> Jean-Michel,
>>
>> thanks for your patches!
>>
>> Am 21.10.2024 um 22:44 schrieb Jean-Michel Hautbois:
>>> The TRACE_IRQFLAGS_SUPPORT requires the architecture to call
>>> trace_hardirqs_off() when interrupts are disabled and
>>> trace_hardirqs_on() when they are enabled.
>>> Add those calls to do_IRQ function.
>>
>> You will also have to add these calls to the three sites in arch/m68k/
>> coldfire/head.S where interrupts are enabled or disabled.
>
> Thanks for this ! I prepared a v2 with those calls added. I am wondering

Good -

> if my second patch is ok, I think it is not, could you please review it

I'll have to read up on how arch_stack_walk is supposed to work - have 
never seen that code before.

> ? There may already be something related to the stack in the assembly
> part useful to implement arch_stack_walk() in a different way ?

I doubt it

>>
>> Here:
>>> ENTRY(system_call)
>>>         SAVE_ALL_SYS
>>>         move    #0x2000,%sr             /* enable intrs again */
>>>         GET_CURRENT(%d2)
>>
>> and here:
>>> ret_from_exception:
>>>         move    #0x2700,%sr             /* disable intrs */
>>>         btst    #5,%sp@(PT_OFF_SR)      /* check if returning to
>>> kernel */
>>>         jeq     Luser_return            /* if so, skip resched,
>>> signals */
>>
>> and here:
>>> Lwork_to_do:
>>>         movel   %a0@(TINFO_FLAGS),%d1   /* get thread_info->flags */
>>>         move    #0x2000,%sr             /* enable intrs again */
>>>         btst    #TIF_NEED_RESCHED,%d1
>>>         jne     reschedule
>>
>> There's one similar site in arch/m68k/kernel/head.S
>> (ret_from_exception) where interrupts are enabled that would need a
>> similar change, if you want to enable this for all m68k.
>
> I won't be able to test it though ;-).
> I see there are a few interrupts disabling in
> arch/m68k/include/asm/entry.h too ?

Right - that's different in the coldfire stack save/restore macros. I 
missed that before.

Looks like wherever SAVE_ALL_SYS, SAVE_ALL_INT and RESTORE_USER are 
used, calls to trace_irqs_on/off() are required as well.

RESTORE_USER only restores the IPL in the CONFIG_COLDFIRE_SW_A7, which 
makes this a little tricky ... you might have to add these calls to the 
macros to get the correct behaviour.

>
>>
>> Registers %d0-%d5 and %a0-%a2 are saved on the stack at this point and
>> can be clobbered if need be.
>
> I don't know if they need to be clobbered...

I meant to say that if you need registers to prepare function arguments 
for trace_irqs_on/off() on the stack, these can be used. But that may 
not be necessary in this case.

Cheers,

	Michael


>
> Thanks,
> JM
>
>>
>> Cheers,
>>
>>      Michael
>>
>>
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
>>> ---
>>>  arch/m68k/Kconfig      | 1 +
>>>  arch/m68k/kernel/irq.c | 2 ++
>>>  2 files changed, 3 insertions(+)
>>>
>>> diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
>>> index
>>> cc26df907bfe3c8143a931d259eceabb16af7411..ab3375475721fa63418c40d4ba6ac76679ebc77d
>>> 100644
>>> --- a/arch/m68k/Kconfig
>>> +++ b/arch/m68k/Kconfig
>>> @@ -39,6 +39,7 @@ config M68K
>>>      select OLD_SIGSUSPEND3
>>>      select UACCESS_MEMCPY if !MMU
>>>      select ZONE_DMA
>>> +    select TRACE_IRQFLAGS_SUPPORT
>>>
>>>  config CPU_BIG_ENDIAN
>>>      def_bool y
>>> diff --git a/arch/m68k/kernel/irq.c b/arch/m68k/kernel/irq.c
>>> index
>>> 9ab4f550342e5de11c528f55781432675ffd66bf..74cf60ebbc4bca51f3caa4046dbd2bdb02355711
>>> 100644
>>> --- a/arch/m68k/kernel/irq.c
>>> +++ b/arch/m68k/kernel/irq.c
>>> @@ -21,9 +21,11 @@ asmlinkage void do_IRQ(int irq, struct pt_regs *regs)
>>>  {
>>>      struct pt_regs *oldregs = set_irq_regs(regs);
>>>
>>> +    trace_hardirqs_off();
>>>      irq_enter();
>>>      generic_handle_irq(irq);
>>>      irq_exit();
>>> +    trace_hardirqs_on();
>>>
>>>      set_irq_regs(oldregs);
>>>  }
>>>
>