lkml.org 
[lkml]   [2007]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 05/10] Text Edit Lock - Alternative code for i386 and x86_64
* Andi Kleen (andi@firstfloor.org) wrote:
> On Fri, Sep 07, 2007 at 10:04:42AM -0400, Mathieu Desnoyers wrote:
> > * Andi Kleen (andi@firstfloor.org) wrote:
> > > On Thu, Sep 06, 2007 at 04:01:29PM -0400, Mathieu Desnoyers wrote:
> > > > + sync_core();
> > > > + /* Not strictly needed, but can speed CPU recovery up. */
> > >
> > > That turned out to break on some VIA CPUs. Should be removed.
> > >
> >
> > Hrm, when does it break ? At boot time ? Is it the cpuid that breaks or
>
> Yes.
>
> > the clflush ? How do you work around the problem when sync_core or
>
> The CLFLUSH
>
> > clflush is called from elsewhere; does it cause a problem if I call it
> > when I update immediate values ?
>
> Unknown currently what are the exact circumstances.
>
> For the other cases it is ignored right now, but when we get
> more information it might be needed to clear the CLFLUSH
> feature bit on those CPUs.
>

Ok, it's better to simply remove the CLFLUSH since it's not needed
anyway. Do you recommend to only remove the CLFLUSH from the _early code
(executed before alternatives are applied) or to remove the CLFLUSH from
the normal execution time code also ?

> > Is it me or __inline_memcpy is simply a copy of i386's __memcpy ?
> > Is there any reason for this name change ?
>
> x86-64 __memcpy does something different.
>
> It might make more sense
>
> At some point I hope to change the i386 setup to be more like x86-64
> anyways -- the x86-64 version is imho much better.
>

It looks like gorund work that should be done in i386 before we start
using __inline_memcpy in alternative.c which is shared between i386 and
x86_64. I haven't seen the alternative that would touch memcpy at all
anyway, am I missing something ? Also, being "faster" is not really an
issue there, since it is not done often. The only thing that matters is
if memcpy could be touched by alternative.c.

> > A- ugly
> > B- breaking vim syntax highlighting. (actually, all the rest of the
> > file becomes weird after that. The problem is similar to declaration
> > of #defile name ({ some code }). It does not really matter as long as
> > it is in a header, but at the middle of a C file it gets rather
> > annoying). (it never though I would use vim as a coding style
> > reference) ;)
>
> Then define a macro
>
> #define BREAKPOINTS(x) \
> ((unsigned char [x]){ [0 ... x] = BREAKPOINT_INSTRUCTIONS })
>
> and use that
>

How about adding :

#define INIT_ARRAY(type, val, len) ((type [len]){ [0 ... len] = (val) })

to kernel.h ? It would be more generic.

> > And what is rather different between the 2 functions is when we want to
> > fill multiple bytes with the same pattern (I fill the unused part of my
> > immediate values bypass with 0x90 nops, but I agree that I could use
> > add_nops if it was exported).
> >
> > Declaration of a variable length array on text_set's stack would break
> > older compilers, so I don't think it is a neat solution neither. kmalloc
>
> All supported gccs support variable length arrays.
>

ok

> > The idea is to mimic the local_irq_save/restore semantic, where the
> > flags argument is passed without &. This is why I use a macro instead of
> > an inline function
>
> Sounds like a bogus idea to me.
>

So you would prefer
unsigned long cr0;
kernel_wp_save(&cr0)
..
kernel_wp_restore(cr0);

to

unsigned long cr0;
kernel_wp_save(cr0)
..
kernel_wp_restore(cr0);

? It seems odd to me, since everyone would expect flags save/restore to
behave like local_irq_save/restore. Or I may misunderstand your point.


> > The good effect of disabling interrupts is that it would make sure no
> > interrupt handler will run with WP flag cleared on the CPU. However, it
>
> Yes that was my point. Not a very strong one admittedly.
>

I agree that most kernel_wp_save/restore users should disable interrupts
before calling it (it would be a "good practice"), but I don't expect to
encapsulate irq disabling in these macros, since it is not mandatory.

Mathieu

> -Andi

--
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-09-11 22:03    [W:0.097 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site