Joerg Roedel
2020-Apr-16 15:24 UTC
[PATCH 05/70] x86/insn: Make inat-tables.c suitable for pre-decompression code
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 at alien8.de> wrote: > > > + Masami. > > > > On Thu, Mar 19, 2020 at 10:13:02AM +0100, Joerg Roedel wrote: > > > From: Joerg Roedel <jroedel at 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.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.> > > + 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. 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.> 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().> > > + 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).> 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. 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
Apparently Analagous Threads
- [PATCH 05/70] x86/insn: Make inat-tables.c suitable for pre-decompression code
- [PATCH v3 23/75] x86/boot/compressed/64: Setup GHCB Based VC Exception handler
- [PATCH v3 23/75] x86/boot/compressed/64: Setup GHCB Based VC Exception handler
- [PATCH v3 23/75] x86/boot/compressed/64: Setup GHCB Based VC Exception handler
- [PATCH v3 23/75] x86/boot/compressed/64: Setup GHCB Based VC Exception handler