Re: [PATCH Part1 v5 28/38] x86/compressed/64: enable SEV-SNP-validated CPUID in #VC handler

[Michael Roth <michael.roth@amd.com>]

On Tue, Aug 31, 2021 at 12:04:33PM +0200, Borislav Petkov wrote:
> On Fri, Aug 27, 2021 at 11:46:01AM -0500, Michael Roth wrote:
> > Will make sure to great these together, but there seems to be a convention
> > of including misc.h first, since it does some fixups for subsequent
> > includes. So maybe that should be moved to the top? There's a comment in
> > boot/compressed/sev.c:
> > 
> > /*
> >  * misc.h needs to be first because it knows how to include the other kernel
> >  * headers in the pre-decompression code in a way that does not break
> >  * compilation.
> >  */
> > 
> > And while it's not an issue here, asm/sev.h now needs to have
> > __BOOT_COMPRESSED #define'd in advance. So maybe that #define should be
> > moved into misc.h so it doesn't have to happen before each include?
> 
> Actually, I'd like to avoid all such nasty games, if possible, with the
> compressed kernel includes because this is where it leads us: sprinkling
> defines left and right and all kinds of magic include order which is
> fragile and error prone.
> 
> So please try to be very conservative here with all the including games.
> 
> So I'd like to understand first *why* asm/sev.h needs to have
> __BOOT_COMPRESSED defined and can that be avoided? Maybe in a separate
> mail because this one already deals with a bunch of things.

I think I just convinced myself at some point that that's where all
these sev-shared.c declarations are supposed to go, but you're right, I
could just as easily move all the __BOOT_COMPRESSED-only definitions
into boot/compressed/misc.h and avoid the mess.

That'll make it nicer if I can get some of the __BOOT_COMPRESSED-guarded
definitions in sev-shared.c moved out boot/compressed/sev.c and
kernel/sev.c as well, with the help of some common setter/getter helpers
to still keep most of the core logic/data structures contained in
sev-shared.c.

> 
> > cpuid.h is for cpuid_function_is_indexed(), which was introduced in this
> > series with patch "KVM: x86: move lookup of indexed CPUID leafs to helper".
> 
> Ok, if we keep cpuid.h only strictly with cpuid-specific helpers, I
> guess that's fine.
> 
> > efi.h is for EFI_CC_BLOB_GUID, which gets referenced by sev-shared.c
> > when it gets included here. However, misc.h seems to already include it,
> > so it can be safely dropped from this patch.
> 
> Yeah, and this is what I mean: efi.h includes a bunch of linux/
> namespace headers and then we have to go deal with compressed
> pulling all kinds of definitions from kernel proper, with hacks like
> __BOOT_COMPRESSED, for example.
> 
> That EFI_CC_BLOB_GUID is only needed in the compressed kernel, right?
> That is, if you move all the CC blob parsing to the compressed kernel
> and supply the thusly parsed info to kernel proper. In that case, you
> can simply define in there, in efi.c or so.

It was used previously in kernel proper to get at the secrets page later,
but now it's obtained via the cached entry in boot_params.cc_blob_address.
Unfortunately it uses EFI_GUID() macro, so maybe efi.c or misc.h where
it makes more sense to add a copy of the macro?