lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 05/70] x86/insn: Make inat-tables.c suitable for pre-decompression code
    Hi,

    On Wed, 25 Mar 2020 16:39:45 +0100
    Borislav Petkov <bp@alien8.de> wrote:

    > + Masami.
    >
    > On Thu, Mar 19, 2020 at 10:13:02AM +0100, Joerg Roedel wrote:
    > > From: Joerg Roedel <jroedel@suse.de>
    > >
    > > The inat-tables.c file has some arrays in it that contain pointers to
    > > other arrays. These pointers need to be relocated when the kernel
    > > image is moved to a different location.
    > >
    > > The pre-decompression boot-code has no support for applying ELF
    > > relocations, so initialize these arrays at runtime in the
    > > pre-decompression code to make sure all pointers are correctly
    > > initialized.

    I need to check the whole series, but as far as I can understand from
    this patch, this seems not allowing to store the address value in
    static pointers. It may break more things, for example _kprobe_blacklist
    records the NOKPROBE_SYMBOL() symbol addresses at the build time.

    I have some comments here.

    > > Signed-off-by: Joerg Roedel <jroedel@suse.de>
    > > ---
    > > arch/x86/tools/gen-insn-attr-x86.awk | 50 +++++++++++++++++++++-
    > > tools/arch/x86/tools/gen-insn-attr-x86.awk | 50 +++++++++++++++++++++-
    > > 2 files changed, 98 insertions(+), 2 deletions(-)
    > >
    > > diff --git a/arch/x86/tools/gen-insn-attr-x86.awk b/arch/x86/tools/gen-insn-attr-x86.awk
    > > index a42015b305f4..af38469afd14 100644
    > > --- a/arch/x86/tools/gen-insn-attr-x86.awk
    > > +++ b/arch/x86/tools/gen-insn-attr-x86.awk
    > > @@ -362,6 +362,9 @@ function convert_operands(count,opnd, i,j,imm,mod)
    > > END {
    > > if (awkchecked != "")
    > > exit 1
    > > +
    > > + print "#ifndef __BOOT_COMPRESSED\n"
    > > +
    > > # print escape opcode map's array
    > > print "/* Escape opcode map array */"
    > > print "const insn_attr_t * const inat_escape_tables[INAT_ESC_MAX + 1]" \
    > > @@ -388,6 +391,51 @@ END {
    > > for (j = 0; j < max_lprefix; j++)
    > > if (atable[i,j])
    > > print " ["i"]["j"] = "atable[i,j]","
    > > - print "};"
    > > + print "};\n"
    > > +
    > > + print "#else /* !__BOOT_COMPRESSED */\n"

    I think the definitions of inat_*_tables can be shared in both case.
    If __BOOT_COMPRESSED is set, we can define inat_init_tables() as a
    initialize function, and if not, it will be just a dummy "do {} while (0)".

    BTW, where is the __BOOT_COMPRESSED defined?

    > > +
    > > + print "/* Escape opcode map array */"
    > > + print "static const insn_attr_t *inat_escape_tables[INAT_ESC_MAX + 1]" \
    > > + "[INAT_LSTPFX_MAX + 1];"
    > > + print ""
    > > +
    > > + print "/* Group opcode map array */"
    > > + print "static const insn_attr_t *inat_group_tables[INAT_GRP_MAX + 1]"\
    > > + "[INAT_LSTPFX_MAX + 1];"
    > > + print ""
    > > +
    > > + print "/* AVX opcode map array */"
    > > + print "static const insn_attr_t *inat_avx_tables[X86_VEX_M_MAX + 1]"\
    > > + "[INAT_LSTPFX_MAX + 1];"
    > > + print ""
    > > +
    > > + print "static void inat_init_tables(void)"

    This functions should be "inline".
    And I can not see the call-site of inat_init_tables() in this patch.

    If possible, please include call-site with definition (especially
    new init function) so that I can check the init call timing too.

    > > + print "{"
    > > +
    > > + # print escape opcode map's array
    > > + print "\t/* Print Escape opcode map array */"
    > > + for (i = 0; i < geid; i++)
    > > + for (j = 0; j < max_lprefix; j++)
    > > + if (etable[i,j])
    > > + print "\tinat_escape_tables["i"]["j"] = "etable[i,j]";"
    > > + print ""
    > > +
    > > + # print group opcode map's array
    > > + print "\t/* Print Group opcode map array */"
    > > + for (i = 0; i < ggid; i++)
    > > + for (j = 0; j < max_lprefix; j++)
    > > + if (gtable[i,j])
    > > + print "\tinat_group_tables["i"]["j"] = "gtable[i,j]";"
    > > + print ""
    > > + # print AVX opcode map's array
    > > + print "\t/* Print AVX opcode map array */"
    > > + for (i = 0; i < gaid; i++)
    > > + for (j = 0; j < max_lprefix; j++)
    > > + if (atable[i,j])
    > > + print "\tinat_avx_tables["i"]["j"] = "atable[i,j]";"
    > > +
    > > + print "}"
    > > + print "#endif"
    > > }

    The code itself looks good to me.

    Thank you,

    --
    Masami Hiramatsu <mhiramat@kernel.org>

    \
     
     \ /
      Last update: 2020-03-27 04:03    [W:4.204 / U:0.536 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site