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:

quoted

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.

quoted

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.

quoted

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?