On Thu, 16 Apr 2020 17:24:06 +0200
Joerg Roedel <joro@xxxxxxxxxx> wrote:
> Hi Masami,
>
> On Fri, Mar 27, 2020 at 12:02:32PM +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Mar 2020 16:39:45 +0100
> > Borislav Petkov <bp@xxxxxxxxx> wrote:
> >
> > > + Masami.
> > >
> > > On Thu, Mar 19, 2020 at 10:13:02AM +0100, Joerg Roedel wrote:
> > > > From: Joerg Roedel <jroedel@xxxxxxx>
> > > >
> > > > 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.
>
> The runtime-initialization function is only used in the
> pre-decompression boot code (arch/x86/boot/compressed/) which is not
> part of the running kernel image. At that stage of booting there is no
> support for kprobe or tracing or any other neat features that might
> break things here.
Ah, I got it. So you intended to port the instruction decoder to
pre-decompression boot code, correct?
> > > > + 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)".
>
> The inat_*_tables are all declared const, so this way it is not possible
> to change them at runtime.
Indeed.
> For the running kernel image this is fine, as
> there are ELF relocations which fix things up, but at the
> pre-decompression boot stage there are no ELF relocations which can fix
> the tables, so the pointers in there need to be initialized at runtime.
OK.
>
> > BTW, where is the __BOOT_COMPRESSED defined?
>
> It is defined in arch/x86/boot/compressed/sev-es.c by patch
>
> x86/boot/compressed/64: Setup GHCB Based VC Exception handler
>
> which also includes parts of the instruction decoder into the
> pre-decompression boot code and adds the only call-site for
> inat_init_tables().
Thanks, I understand it.
>
> > > > + 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.
>
> The call-site is added with the patch that includes the
> instruction decoder into the pre-decompression code. If possible I'd
> like to keep those things separate, as both patches are already pretty
> big by themselfes (and they do different things, in different parts of
> the code).
OK, if you will send v2, please CC both to me.
>
> > If possible, please include call-site with definition (especially
> > new init function) so that I can check the init call timing too.
>
> The function is called at the first #VC exception after a GHCB has been
> set up. Call-path is: boot_vc_handler -> sev_es_setup_ghcb ->
> inat_init_tables.
sound good to me.
Thank you,
>
> See
>
> https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/tree/arch/x86/boot/compressed/sev-es.c?h=sev-es-client-v5.6-rc6
>
> for the full code there.
>
> Thanks,
>
> Joerg
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
