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

From: Michael Roth <michael.roth@amd.com>

With upcoming SEV-SNP support, SEV-related features need to be
initialized earlier in boot, at the same point the initial #VC handler
is set up, so that the SEV-SNP CPUID table can be utilized during the
initial feature checks. Also, SEV-SNP feature detection will rely on
EFI helper functions to scan the EFI config table for the Confidential
Computing blob, and so would need to be implemented at least partially
in C.

Currently set_sev_encryption_mask() is used to initialize the
sev_status and sme_me_mask globals that advertise what SEV/SME features
are available in a guest. Rename it to sev_enable() to better reflect
that (SME is only enabled in the case of SEV guests in the
boot/compressed kernel), and move it to just after the stage1 #VC
handler is set up so that it can be used to initialize SEV-SNP as well
in future patches.

While at it, re-implement it as C code so that all SEV feature
detection can be better consolidated with upcoming SEV-SNP feature
detection, which will also be in C.

The 32-bit entry path remains unchanged, as it never relied on the
set_sev_encryption_mask() initialization to begin with, possibly due to
the normal rva() helper for accessing globals only being usable by code
in .head.text. Either way, 32-bit entry for SEV-SNP would likely only
be supported for non-EFI boot paths, and so wouldn't rely on existing
EFI helper functions, and so could be handled by a separate/simpler
32-bit initializer in the future if needed.

Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/boot/compressed/head_64.S     | 32 +++++++++++--------
 arch/x86/boot/compressed/mem_encrypt.S | 36 ---------------------
 arch/x86/boot/compressed/misc.h        |  4 +--
 arch/x86/boot/compressed/sev.c         | 44 ++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 51 deletions(-)

On Fri, Jan 28, 2022 at 11:17:26AM -0600, Brijesh Singh wrote:
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index fd9441f40457..49064a9f96e2 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -191,9 +191,8 @@ SYM_FUNC_START(startup_32)
>  	/*
>  	 * Mark SEV as active in sev_status so that startup32_check_sev_cbit()
>  	 * will do a check. The sev_status memory will be fully initialized

That "sev_status memory" formulation is just weird. Pls fix it while
you're touching that comment.

> +static inline u64 rd_sev_status_msr(void)
> +{
> +	unsigned long low, high;
> +
> +	asm volatile("rdmsr" : "=a" (low), "=d" (high) :
> +			"c" (MSR_AMD64_SEV));
> +
> +	return ((high << 32) | low);
> +}

Don't you see sev_es_rd_ghcb_msr() in that same file above? Do a common
rdmsr() helper and call it where needed, pls, instead of duplicating
code.

misc.h looks like a good place.

Extra bonus points will be given if you unify callers in
arch/x86/boot/cpucheck.c too but you don't have to - I can do that
ontop.

Thx.

On Tue, Feb 01, 2022 at 07:08:21PM +0100, Borislav Petkov wrote:
> On Fri, Jan 28, 2022 at 11:17:26AM -0600, Brijesh Singh wrote:
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index fd9441f40457..49064a9f96e2 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -191,9 +191,8 @@ SYM_FUNC_START(startup_32)
> >  	/*
> >  	 * Mark SEV as active in sev_status so that startup32_check_sev_cbit()
> >  	 * will do a check. The sev_status memory will be fully initialized
> 
> That "sev_status memory" formulation is just weird. Pls fix it while
> you're touching that comment.

Will do.

> 
> > +static inline u64 rd_sev_status_msr(void)
> > +{
> > +	unsigned long low, high;
> > +
> > +	asm volatile("rdmsr" : "=a" (low), "=d" (high) :
> > +			"c" (MSR_AMD64_SEV));
> > +
> > +	return ((high << 32) | low);
> > +}
> 
> Don't you see sev_es_rd_ghcb_msr() in that same file above? Do a common
> rdmsr() helper and call it where needed, pls, instead of duplicating
> code.

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.

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?

> 
> misc.h looks like a good place.

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?

> 
> Extra bonus points will be given if you unify callers in
> arch/x86/boot/cpucheck.c too but you don't have to - I can do that
> ontop.

I have these new helpers defined with similar signatures to
__rdmsr/__wrmsr:

  /* rdmsr/wrmsr helpers */
  static inline u64 read_msr(unsigned int msr)
  {
         u64 low, high;
  
         asm volatile("rdmsr" : "=a" (low), "=d" (high) : "c" (msr));
  
         return ((high << 32) | low);
  }
  
  static inline void write_msr(unsigned int msr, u32 low, u32 high)
  {
         asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
  }

but cpucheck.c code flow really lends itself to having a read_msr()
variant that loads into 2 separate high/low u32 values, like what
native_rdmsr does:

  #define native_rdmsr(msr, val1, val2)                   \
  do {                                                    \
          u64 __val = __rdmsr((msr));                     \
          (void)((val1) = (u32)__val);                    \
          (void)((val2) = (u32)(__val >> 32));            \
  } while (0)

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)?

Thanks,

Mike

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7Cec7f8621a6934039cfff08d9e5addaca%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637793357136301050%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=FMoP5ZskuxwanWTe5DxMnIYNPBSi%2FhRrOExp9hIHaCo%3D&amp;reserved=0

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@linux.intel.com

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.

On Tue, Feb 01, 2022 at 10:28:39PM +0100, Borislav Petkov wrote:
> 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.

Since the kernel proper rdmsr()/wrmsr() definitions are getting pulled in via
misc.h, I have to use a different name to avoid compiler errors. For now I've
gone with rd_msr()/wr_msr(), but no problem changing those if needed.

> > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20220124150215.36893-11-kirill.shutemov%40linux.intel.com&data=04%7C01%7Cmichael.roth%40amd.com%7C1662b963f1c54f3663df08d9e5c9d6bd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637793477399827883%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=rtdo9ci3XjOHn2HphvNE7ciFR6tKh1pVclFuNhXkNGs%3D&reserved=0
> 
> 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?

Thanks for the suggestions, this works out nicely, but as far as defining
them in boot/msr.c, it looks like the Makefile in boot/compressed doesn't
currently have any instances of linking to objects in boot/, and the status
quo seems to be #include'ing the whole C file in cases where boot/ code is
needed in boot/compressed.

Since the rd_msr/wr_msr are oneliners, it seemed like it might be a
little cleaner to just define them in boot/msr.h as static inline and
include them directly as part of the header.

Here's what it looks like on top of this tree, and roughly how I plan to
split the patches for v10:

- define the rd_msr/wr_msr helpers
  https://github.com/mdroth/linux/commit/982c6c5741478c8f634db8ac0ba36575b5eff946

- use the helpers in boot/compressed/sev.c and boot/cpucheck.c
  https://github.com/mdroth/linux/commit/a16e11f727c01fc478d3b741e1bdd2fd44975d7c

For v10 though I'll likely just drop rd_sev_status_msr() completely and use
rd_msr() directly.

Let me know if I should make any changes and I'll make sure to get those in for
the next spin.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C1662b963f1c54f3663df08d9e5c9d6bd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637793477399827883%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=njDai06MS2mcyMLZ5YvLMcgXSXwfoO01U2c0D%2BE3HG4%3D&reserved=0

On Tue, Feb 01, 2022 at 06:52:12PM -0600, Michael Roth wrote:
> Since the kernel proper rdmsr()/wrmsr() definitions are getting pulled in via
> misc.h, I have to use a different name to avoid compiler errors. For now I've
> gone with rd_msr()/wr_msr(), but no problem changing those if needed.

Does that fix it too?

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 16ed360b6692..346c46d072c8 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -21,7 +21,6 @@
 
 #include <linux/linkage.h>
 #include <linux/screen_info.h>
-#include <linux/elf.h>
 #include <linux/io.h>
 #include <asm/page.h>
 #include <asm/boot.h>
---

This is exactly what I mean with a multi-year effort of untangling what
has been mindlessly mixed in over the years...

> Since the rd_msr/wr_msr are oneliners, it seemed like it might be a
> little cleaner to just define them in boot/msr.h as static inline and
> include them directly as part of the header.

Ok, that's fine too.

> Here's what it looks like on top of this tree, and roughly how I plan to
> split the patches for v10:
> 
> - define the rd_msr/wr_msr helpers
>   https://github.com/mdroth/linux/commit/982c6c5741478c8f634db8ac0ba36575b5eff946
> 
> - use the helpers in boot/compressed/sev.c and boot/cpucheck.c
>   https://github.com/mdroth/linux/commit/a16e11f727c01fc478d3b741e1bdd2fd44975d7c
> 
> For v10 though I'll likely just drop rd_sev_status_msr() completely and use
> rd_msr() directly.
> 
> Let me know if I should make any changes and I'll make sure to get those in for
> the next spin.

Yap, looks good.

Thanks for doing that!

On Wed, Feb 02, 2022 at 07:09:24AM +0100, Borislav Petkov wrote:
> On Tue, Feb 01, 2022 at 06:52:12PM -0600, Michael Roth wrote:
> > Since the kernel proper rdmsr()/wrmsr() definitions are getting pulled in via
> > misc.h, I have to use a different name to avoid compiler errors. For now I've
> > gone with rd_msr()/wr_msr(), but no problem changing those if needed.
> 
> Does that fix it too?
> 
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 16ed360b6692..346c46d072c8 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -21,7 +21,6 @@
>  
>  #include <linux/linkage.h>
>  #include <linux/screen_info.h>
> -#include <linux/elf.h>
>  #include <linux/io.h>
>  #include <asm/page.h>
>  #include <asm/boot.h>
> ---
> 
> This is exactly what I mean with a multi-year effort of untangling what
> has been mindlessly mixed in over the years...

Indeed... it looks like linux/{elf,io,efi,acpi}.h all end up pulling in
kernel proper's rdmsr()/wrmsr() definitions, and pulling them out ends up
breaking a bunch of other stuff, so I think we might be stuck using a
different name like rd_msr()/wr_msr() in the meantime.

On Wed, Feb 02, 2022 at 11:28:01AM -0600, Michael Roth wrote:
> Indeed... it looks like linux/{elf,io,efi,acpi}.h all end up pulling in
> kernel proper's rdmsr()/wrmsr() definitions, and pulling them out ends up
> breaking a bunch of other stuff,

It's a nightmare - just gave it a try. No wonder they call it include
hell.

> so I think we might be stuck using a different name like
> rd_msr()/wr_msr() in the meantime.

Ok, but then pls call them boot_rdmsr() and boot_wrmsr() so that there's
a clear distinction from all the other msr helpers. And put a comment
above them in arch/x86/boot/msr.h explaining why they're called this
way.

One fine day I'll have this mess untangled and clean...

Thx.