1. Patchew
  2. linux
  3. [PATCH v4 0/3] efi: Don't initalize SEV-SNP from the EFI stub
  4. View patch

[PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub

Ard Biesheuvel posted 3 patches 3 weeks, 2 days ago
[PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Ard Biesheuvel 3 weeks, 2 days ago 
From: Ard Biesheuvel <ardb@kernel.org>

One of the last things the EFI stub does before handing over to the core
kernel when booting as a SEV guest is enabling SEV, even though this is
mostly redundant: one of the first things the core kernel does is
calling sme_enable(), after setting up the early GDT and IDT but before
even setting up the kernel page tables. sme_enable() performs the same
SEV-SNP initialization that the decompressor performs in sev_enable().

So let's just drop this call to sev_enable(), and rely on the core
kernel to initiaize SEV correctly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/sev.h              | 2 --
 drivers/firmware/efi/libstub/x86-stub.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index d7be1ff3f7e0..b017e1dab705 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
 		__sev_es_nmi_complete();
 }
 extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
-extern void sev_enable(struct boot_params *bp);
 
 /*
  * RMPADJUST modifies the RMP permissions of a page of a lesser-
@@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
 static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
 static inline void sev_es_nmi_complete(void) { }
 static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
-static inline void sev_enable(struct boot_params *bp) { }
 static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
 static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
 static inline void setup_ghcb(void) { }
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index c4ef645762ec..354bc3901193 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 		goto fail;
 	}
 
-	/*
-	 * Call the SEV init code while still running with the firmware's
-	 * GDT/IDT, so #VC exceptions will be handled by EFI.
-	 */
-	sev_enable(boot_params);
-
 	efi_5level_switch();
 
 	enter_kernel(kernel_entry, boot_params);
-- 
2.51.0.384.g4c02a37b29-goog
Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Tom Lendacky 3 weeks ago 
On 9/9/25 03:06, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> One of the last things the EFI stub does before handing over to the core
> kernel when booting as a SEV guest is enabling SEV, even though this is
> mostly redundant: one of the first things the core kernel does is
> calling sme_enable(), after setting up the early GDT and IDT but before
> even setting up the kernel page tables. sme_enable() performs the same
> SEV-SNP initialization that the decompressor performs in sev_enable().
> 
> So let's just drop this call to sev_enable(), and rely on the core
> kernel to initiaize SEV correctly.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/include/asm/sev.h              | 2 --
>  drivers/firmware/efi/libstub/x86-stub.c | 6 ------
>  2 files changed, 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index d7be1ff3f7e0..b017e1dab705 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
>  		__sev_es_nmi_complete();
>  }
>  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
> -extern void sev_enable(struct boot_params *bp);
>  
>  /*
>   * RMPADJUST modifies the RMP permissions of a page of a lesser-
> @@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
>  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>  static inline void sev_es_nmi_complete(void) { }
>  static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
> -static inline void sev_enable(struct boot_params *bp) { }
>  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>  static inline void setup_ghcb(void) { }
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index c4ef645762ec..354bc3901193 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>  		goto fail;
>  	}
>  
> -	/*
> -	 * Call the SEV init code while still running with the firmware's
> -	 * GDT/IDT, so #VC exceptions will be handled by EFI.
> -	 */
> -	sev_enable(boot_params);

I think we lose the check for GHCB_HV_FT_SNP_MULTI_VMPL by doing this. It
might need move into svsm_setup_ca() now.

Thanks,
Tom

> -
>  	efi_5level_switch();
>  
>  	enter_kernel(kernel_entry, boot_params);
Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Ard Biesheuvel 2 weeks, 6 days ago 
On Thu, 11 Sept 2025 at 23:53, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 9/9/25 03:06, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > One of the last things the EFI stub does before handing over to the core
> > kernel when booting as a SEV guest is enabling SEV, even though this is
> > mostly redundant: one of the first things the core kernel does is
> > calling sme_enable(), after setting up the early GDT and IDT but before
> > even setting up the kernel page tables. sme_enable() performs the same
> > SEV-SNP initialization that the decompressor performs in sev_enable().
> >
> > So let's just drop this call to sev_enable(), and rely on the core
> > kernel to initiaize SEV correctly.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/include/asm/sev.h              | 2 --
> >  drivers/firmware/efi/libstub/x86-stub.c | 6 ------
> >  2 files changed, 8 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > index d7be1ff3f7e0..b017e1dab705 100644
> > --- a/arch/x86/include/asm/sev.h
> > +++ b/arch/x86/include/asm/sev.h
> > @@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
> >               __sev_es_nmi_complete();
> >  }
> >  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
> > -extern void sev_enable(struct boot_params *bp);
> >
> >  /*
> >   * RMPADJUST modifies the RMP permissions of a page of a lesser-
> > @@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
> >  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> >  static inline void sev_es_nmi_complete(void) { }
> >  static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
> > -static inline void sev_enable(struct boot_params *bp) { }
> >  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> >  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> >  static inline void setup_ghcb(void) { }
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index c4ef645762ec..354bc3901193 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
> >               goto fail;
> >       }
> >
> > -     /*
> > -      * Call the SEV init code while still running with the firmware's
> > -      * GDT/IDT, so #VC exceptions will be handled by EFI.
> > -      */
> > -     sev_enable(boot_params);
>
> I think we lose the check for GHCB_HV_FT_SNP_MULTI_VMPL by doing this. It
> might need move into svsm_setup_ca() now.
>

Currently, this check only occurs inside sev_enable(), and so it
happens too late to have an impact, given that the core kernel will
set up all of this state from scratch right away.

So if this check is needed in the EFI stub to begin with, it should be
moved into early_is_sevsnp_guest() so that the check occurs before
attempting to accept memory.
Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Ard Biesheuvel 2 weeks, 6 days ago 
On Fri, 12 Sept 2025 at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 11 Sept 2025 at 23:53, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >
> > On 9/9/25 03:06, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > One of the last things the EFI stub does before handing over to the core
> > > kernel when booting as a SEV guest is enabling SEV, even though this is
> > > mostly redundant: one of the first things the core kernel does is
> > > calling sme_enable(), after setting up the early GDT and IDT but before
> > > even setting up the kernel page tables. sme_enable() performs the same
> > > SEV-SNP initialization that the decompressor performs in sev_enable().
> > >
> > > So let's just drop this call to sev_enable(), and rely on the core
> > > kernel to initiaize SEV correctly.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/x86/include/asm/sev.h              | 2 --
> > >  drivers/firmware/efi/libstub/x86-stub.c | 6 ------
> > >  2 files changed, 8 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > > index d7be1ff3f7e0..b017e1dab705 100644
> > > --- a/arch/x86/include/asm/sev.h
> > > +++ b/arch/x86/include/asm/sev.h
> > > @@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
> > >               __sev_es_nmi_complete();
> > >  }
> > >  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
> > > -extern void sev_enable(struct boot_params *bp);
> > >
> > >  /*
> > >   * RMPADJUST modifies the RMP permissions of a page of a lesser-
> > > @@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
> > >  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> > >  static inline void sev_es_nmi_complete(void) { }
> > >  static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
> > > -static inline void sev_enable(struct boot_params *bp) { }
> > >  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> > >  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> > >  static inline void setup_ghcb(void) { }
> > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > > index c4ef645762ec..354bc3901193 100644
> > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > @@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
> > >               goto fail;
> > >       }
> > >
> > > -     /*
> > > -      * Call the SEV init code while still running with the firmware's
> > > -      * GDT/IDT, so #VC exceptions will be handled by EFI.
> > > -      */
> > > -     sev_enable(boot_params);
> >
> > I think we lose the check for GHCB_HV_FT_SNP_MULTI_VMPL by doing this. It
> > might need move into svsm_setup_ca() now.
> >
>
> Currently, this check only occurs inside sev_enable(), and so it
> happens too late to have an impact, given that the core kernel will
> set up all of this state from scratch right away.
>

Hmm, I only just spotted that this check only happens in the legacy
decompressor.

I think it makes sense for this check to live in svsm_setup_ca(), but
what is your take on the need to perform this check when accepting
memory from the stub using the CA address obtained from the firmware?
(i.e., way before sev_enable() is called)
Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Tom Lendacky 2 weeks, 6 days ago 
On 9/12/25 03:26, Ard Biesheuvel wrote:
> On Fri, 12 Sept 2025 at 09:29, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Thu, 11 Sept 2025 at 23:53, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>
>>> On 9/9/25 03:06, Ard Biesheuvel wrote:
>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>
>>>> One of the last things the EFI stub does before handing over to the core
>>>> kernel when booting as a SEV guest is enabling SEV, even though this is
>>>> mostly redundant: one of the first things the core kernel does is
>>>> calling sme_enable(), after setting up the early GDT and IDT but before
>>>> even setting up the kernel page tables. sme_enable() performs the same
>>>> SEV-SNP initialization that the decompressor performs in sev_enable().
>>>>
>>>> So let's just drop this call to sev_enable(), and rely on the core
>>>> kernel to initiaize SEV correctly.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>>  arch/x86/include/asm/sev.h              | 2 --
>>>>  drivers/firmware/efi/libstub/x86-stub.c | 6 ------
>>>>  2 files changed, 8 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>>>> index d7be1ff3f7e0..b017e1dab705 100644
>>>> --- a/arch/x86/include/asm/sev.h
>>>> +++ b/arch/x86/include/asm/sev.h
>>>> @@ -462,7 +462,6 @@ static __always_inline void sev_es_nmi_complete(void)
>>>>               __sev_es_nmi_complete();
>>>>  }
>>>>  extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
>>>> -extern void sev_enable(struct boot_params *bp);
>>>>
>>>>  /*
>>>>   * RMPADJUST modifies the RMP permissions of a page of a lesser-
>>>> @@ -588,7 +587,6 @@ static inline void sev_es_ist_exit(void) { }
>>>>  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>>>>  static inline void sev_es_nmi_complete(void) { }
>>>>  static inline int sev_es_efi_map_ghcbs_cas(pgd_t *pgd) { return 0; }
>>>> -static inline void sev_enable(struct boot_params *bp) { }
>>>>  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>>>>  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>>>>  static inline void setup_ghcb(void) { }
>>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>>> index c4ef645762ec..354bc3901193 100644
>>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>>> @@ -938,12 +938,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>>>               goto fail;
>>>>       }
>>>>
>>>> -     /*
>>>> -      * Call the SEV init code while still running with the firmware's
>>>> -      * GDT/IDT, so #VC exceptions will be handled by EFI.
>>>> -      */
>>>> -     sev_enable(boot_params);
>>>
>>> I think we lose the check for GHCB_HV_FT_SNP_MULTI_VMPL by doing this. It
>>> might need move into svsm_setup_ca() now.
>>>
>>
>> Currently, this check only occurs inside sev_enable(), and so it
>> happens too late to have an impact, given that the core kernel will
>> set up all of this state from scratch right away.
>>
> 
> Hmm, I only just spotted that this check only happens in the legacy
> decompressor.
> 
> I think it makes sense for this check to live in svsm_setup_ca(), but
> what is your take on the need to perform this check when accepting
> memory from the stub using the CA address obtained from the firmware?
> (i.e., way before sev_enable() is called)

Yes, it seems like it should be checked before memory acceptance if we're
using an SVSM, so early_is_sevsnp_guest() looks appropriate. But since
this may not be called, the check also has to be performed by the core
kernel in svsm_setup_ca(), too.

Just wondering if it is truly necessary to check in the stub just for this
case. Theoretically, the SVSM should have validated that the HV has the
necessary support before ever invoking the firmware. Just putting it in
svsm_setup_ca() seems enough to me.

@Boris, what do you think?

Thanks,
Tom
Re: [PATCH v4 3/3] x86/efistub: Don't bother enabling SEV in the EFI stub
Posted by Borislav Petkov 2 weeks, 3 days ago 
On Fri, Sep 12, 2025 at 08:32:30AM -0500, Tom Lendacky wrote:
> @Boris, what do you think?

Right, as we just talked, this should be ok, but it needs a more fine-grained
review to check whether the code that goes away is present in kernel proper.
It should be but...

And then we probably should delay this until the next cycle so that it gets
a full cycle of testing instead of rushing it in now and then patching it out
again...

That's me being overly cautious ofc.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.