Re: [PATCH v9 05/43] x86/compressed/64: Detect/setup SEV/SME features earlier in boot

[Borislav Petkov]

On Tue, Feb 01, 2022 at 02:35:07PM -0600, Michael Roth wrote:
> Unfortunately rdmsr()/wrmsr()/__rdmsr()/__wrmsr() etc. definitions are all
> already getting pulled in via:
> 
>   misc.h:
>     #include linux/elf.h
>       #include linux/thread_info.h
>         #include linux/cpufeature.h
>           #include linux/processor.h
>             #include linux/msr.h
> 
> Those definitions aren't usable in boot/compressed because of __ex_table
> and possibly some other dependency hellishness.

And they should not be. Mixing kernel proper and decompressor code needs
to stop and untangling that is a multi-year effort, unfortunately. ;-\

> Would read_msr()/write_msr() be reasonable alternative names for these new
> helpers, or something else that better distinguishes them from the
> kernel proper definitions?

Nah, just call them rdmsr/wrmsr(). There is already {read,write}_msr()
tracepoint symbols in kernel proper and there's no point in keeping them
apart using different names - that ship has long sailed.

> It doesn't look like anything in boot/ pulls in boot/compressed/
> headers. It seems to be the other way around, with boot/compressed
> pulling in headers and whole C files from boot/.
> 
> So perhaps these new definitions should be added to a small boot/msr.h
> header and pulled in from there?

That sounds good too.

> Should we introduce something like this as well for cpucheck.c? Or
> re-write cpucheck.c to make use of the u64 versions? Or just set the
> cpucheck.c rework aside for now? (but still introduce the above helpers
> as boot/msr.h in preparation)?

How about you model it after

static int msr_read(u32 msr, struct msr *m)

from arch/x86/lib/msr.c which takes struct msr from which you can return
either u32s or a u64?

The stuff you share between the decompressor and kernel proper you put
in a arch/x86/include/asm/shared/ folder, for an example, see what we do
there in the TDX patchset:

https://lore.kernel.org/r/20220124150215.36893-11-kirill.shutemov@xxxxxxxxxxxxxxx

I.e., you move struct msr in such a shared header and then you include
it everywhere needed.

The arch/x86/boot/ msr helpers are then plain and simple, without
tracepoints and exception fixups and you define them in ...boot/msr.c or
so.

If the patch gets too big, make sure to split it in a couple so that it
is clear what happens at each step.

How does that sound?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette