[Xenomai-core] Re: [PATCH] kgdb/x86 over I-pipe

[Jan Kiszka <jan.kiszka@domain.hid>]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>
>>> Jan Kiszka wrote:
>>>
>>>> Philippe Gerum wrote:
>>>>
>>>>
>>>>> Hi Jan,
>>>>>
>>>>> Based on your previous work, here is a set of patches coupling KGDB
>>>>> and
>>>>> the I-pipe. Basically, I've attempted to shrink the extra patches
>>>>> needed
>>>>> against the original KGDB + I-pipe ones to the bare minimum. This has
>>>>> been obtained by having the I-pipe provide ipipe_current_safe(), and
>>>>> drastically reduce the amount of fiddling with smp_processor_id().
>>>>>
>>>>> The key difference with the former implementation is that a domain
>>>>> (e.g.
>>>>> Xenomai) is now expected to tell the I-pipe when it's switching to a
>>>>> non-Linux stack, and the I-pipe makes good use of this information to
>>>>> return the proper "current" value when asked to through
>>>>> ipipe_safe_current() from the KGDB code. The issue of swapping
>>>>
>>>>
>>>> This looks nice.
>>>>
>>>>
>>>>
>>>>> smp_processor_id() with ipipe_processor_id() has been addressed the
>>>>> hard
>>>>> way: smp_processor_id() is simply defined as ipipe_processor_id() when
>>>>> CONFIG_IPIPE and CONFIG_KGDB are both enabled in include/linux/smp.h.
>>>>> This approach was actually used during the old Adeos times when
>>>>> pipeline
>>>>> domain had their own separate stack. I take for granted that the CPU
>>>>> penalty taken in doing this is perfectly acceptable, since well, we
>>>>> are
>>>>> debugging after all.
>>>>
>>>>
>>>> There is only one drawback: we will not be able to debug
>>>> smp_processor_id-related bugs in ipipe/Xenomai anymore...
>>>>
>>>
>>> Good point. Here is an updated patch.
>>>
>>
>>
>> Nope that's not the point I meant. I was referring to bugs like the
>> missing smp_processor_id patch in asm-i386/mmu_context.h. Your way makes
>> such problems disappear once you switch on the debugger. Remember that
>> we spotted the mmu_context issue via kgdb.
>>
> 
> Got it now. Indeed, making such kind of bugs disappear would be a very
> undesirable side-effect of enabling KGDB.
> 
>> The attached kgdb-ipipe patch (which also updates to kgdb-CVS head)
>> addresses maintenance and smp-safeness by redefining the involved
>> services only per source file, not kernel-wide. What's do you think
>> about this approach?
>>
> 
> Mixed feelings. On one hand, it locally fixes the issue without masking
> any bugs and that's good. On the other hand, I've been bitten using this
> kind of tricks in earlier Adeos releases for patching printk() the same
> way, substituting it with spin_lock_irqsave_hw. At some point,
> spin_lock_irqsave became local_irq_save + spin_lock in the vanilla code,
> making the substitution of the former pointless, and opening a
> preemption hole in the code. This said, no spinlock macros are involved
> in the substitution you are suggesting, so there is no such risk, and in
> any case, the risk is confined to one file and not spread all over the
> place like with the raw_smp_processor_id() substitution. IOW, let's go
> your way.

I agree that it's kind of fragile, but we need to check on kgdb updates
anyway. I hope for kgdb becoming mainline one day. Then the code should
stabilise (if it hasn't already), and we can track it via ipipe directly
- without tricks. So far, this should help to keep efforts lower for us.

> 
> While we are at it,
> #define current ipipe_safe_current() /* ? */

Nope, there is the need for some special changes.

> 
>> I'm also posting two fixes for ipipe itself to 1) make kgdb work over
>> SMP and 2) fix a compiler warning.
>>
>> This version works fine for me, at least in qemu. I tried to check SMP
>> as well, but qemu obviously lacks support for NMIs raised via IPI, and
>> this is used by kgdb to sync all CPUs on debugging events (vanilla kgdb
>> suffers too).
>>
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]