lkml.org 
[lkml]   [2007]   [Sep]   [7]   [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 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
    the clflush ? How do you work around the problem when sync_core or
    clflush is called from elsewhere; does it cause a problem if I call it
    when I update immediate values ?

    > > + if (cpu_has_clflush)
    > > + for (faddr = addr; faddr < addr + len;
    > > + faddr += boot_cpu_data.x86_clflush_size)
    > > + asm("clflush (%0) " :: "r" (faddr) : "memory");
    > > +}
    > > +
    > > +void * text_poke_early(void *addr, const void *opcode,
    > > + size_t len)
    > > +{
    > > + memcpy(addr, opcode, len);
    >
    > It would be best to copy __inline_memcpy from x86-64 to i386
    > and use that here. That avoids the dependency on a patched
    > memcpy and is slightly safer.
    >

    Is it me or __inline_memcpy is simply a copy of i386's __memcpy ?
    Is there any reason for this name change ?

    > > +
    > > + if (len > sizeof(long)) {
    > > + printk(KERN_ERR "text_poke of len %zu too big (max %lu)\n",
    > > + len, sizeof(long));
    > > + BUG_ON(1);
    >
    > In general BUG_ON only should be enough because these values can
    > be recovered from the registers.
    >

    Ok.

    > > + }
    > > + unaligned = (((long)addr + len - 1) & ~(sizeof(long) - 1))
    > > + - ((long)addr & ~(sizeof(long) - 1));
    > > + if (unlikely(unaligned)) {
    > > + printk(KERN_ERR "text_poke of at addr %p of len %zu is "
    > > + "unaligned (%d)\n",
    > > + addr, len, unaligned);
    > > + BUG_ON(1);
    > > + }
    >
    > The common code should be in a common function. In fact they're so
    > similar that the caller could just pass a buffer for the text_set
    > case, couldn't it?
    >

    I found out that doing a text_set is relatively common. What I want to
    remove is things such as:

    text_poke(addr, ((unsigned char []){BREAKPOINT_INSTRUCTION}, 1);

    which is :
    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) ;)

    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
    seems overkill to me.

    I'll try to come up with a single static function, called from both
    text_set and text_poke, that will merge the code and execute either
    memset or memcpy depending on a supplementary argument.

    >
    > > +#define kernel_wp_save(cr0) \
    >
    > Is there a real reason this has to be an macro? It could
    > be just a normal function. In fact a shared on in alternative.c.
    > That would also avoid adding more include dependencies.
    >

    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.

    > > + do { \
    > > + typecheck(unsigned long, cr0); \
    >
    > typecheck is probably overkill
    >

    ok, I'll remove it.

    > > + preempt_disable(); \
    >
    > Should disable interrupts too just to be safer?
    >

    Well, the only thing that we really don't want here is to be scheduled
    to a different CPU, so preempt disable should be enough.

    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
    would add a flags parameter to kernel_wp_save/restore which would be
    rather ugly :( This is why I prefer to go with preempt_disable, but I am
    open to other considerations.

    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-07 16:07    [W:0.028 / U:62.444 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site