lkml.org 
[lkml]   [2008]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86: clean up after: move entry_64.S register saving out of the macros
    On Thu, Nov 20, 2008 at 04:39:54PM +0100, Ingo Molnar wrote:
    >
    > * Alexander van Heukelum <heukelum@mailshack.com> wrote:
    >
    > > On Thu, Nov 20, 2008 at 04:04:12PM +0100, Ingo Molnar wrote:
    > > >
    > > > * Alexander van Heukelum <heukelum@mailshack.com> wrote:
    > > >
    > > > > This add-on patch to x86: move entry_64.S register saving out of the
    > > > > macros visually cleans up the appearance of the code by introducing
    > > > > some basic helper macro's. It also adds some cfi annotations which
    > > > > were missing.
    > > > >
    > > > > Signed-off-by: Alexander van Heukelum <heukelum@fastmail.fm>
    > > > > ---
    > > > > arch/x86/kernel/entry_64.S | 220 ++++++++++++++++++++++----------------------
    > > > > 1 files changed, 112 insertions(+), 108 deletions(-)
    > > > >
    > > > > Hello Ingo,
    > > > >
    > > > > This patch improves the CFI-situation in entry_64.S, but restricted
    > > > > mostly to the areas touched by "x86: move entry_64.S register saving
    > > > > out of the macros". I'm sure there will be some small errors
    > > > > somewhere, but it compiles and runs fine.
    > > >
    > > > very nice cleanup! This is exactly what should be done. Applied to
    > > > tip/x86/irq.
    > > >
    > > > Note, i did a small rename:
    > > >
    > > > cfi_pushq => pushq_cfi
    > > > cfi_popq => popq_cfi
    > > > cfi_store => movq_cfi

    Does not work... But if you are attached to the underscores, I
    think we can force it to work by using CPP to convert it to
    something the assembler does parse right:

    #define pushq_cfi pushq.cfi

    etc?

    Or is that just too ugly?

    Alexander

    > > > as the goal is to have the actual source code read mostly as regular
    > > > assembly code. The fact that the macro is equivalent to a
    > > > default-annotated pushq/popq/movq instruction is much more important
    > > > than the fact that it also does CFI annotations.
    > > >
    > > > Also, while cfi_store is correct as well, the usual x86 assembly term
    > > > (and instruction used here) is movq.
    > >
    > > Now I have a little problem with my next patch... I wanted to
    > > introduce cfi_load. Guess what assembly instruction that maps to ;).
    >
    > heh ;-)
    >
    > the restore direction could be named movq_cfi_restore, and have the same
    > order of arguments as the regular movq that it replaces. I.e.:
    >
    > movq 8(%rsp),%r11
    > CFI_RESTORE r11
    >
    > would map to:
    >
    > movq_cfi_restore 8, r11
    >
    > or so.
    >
    > cfi_store has really a bad name: it's confusing whether it's the CFI
    > info we are storing/registering (which we are), or a 'store' instruction
    > (which this is too).
    >
    > If then we should name it movq_cfi_store or movq_cfi_register - but
    > that's too long.
    >
    > movq_cfi for the frame construction direction and movq_cfi_restore for
    > the frame deconstruction phase sounds like a good naming compromise, hm?
    >
    > Ingo

    --
    Alexander van Heukelum


    \
     
     \ /
      Last update: 2008-11-20 17:03    [W:9.055 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site