1. Patchew
  2. linux
  3. View series

[PATCH] sev-snp: parse MP tables for VMware hypervisor

Ajay Kaher posted 1 patch 12 months ago
There is a newer version of this series
arch/x86/kernel/cpu/vmware.c | 7 +++++++
1 file changed, 7 insertions(+)
[PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Ajay Kaher 12 months ago 
For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
In this case, mpparse_find_mptable() has to be called to parse MP
tables which contains boot information.

Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
Signed-off-by: Ye Li <ye.li@broadcom.com>
Tested-by: Ye Li <ye.li@broadcom.com>
---
 arch/x86/kernel/cpu/vmware.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 00189cd..3e2594d 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -26,6 +26,7 @@
 #include <linux/export.h>
 #include <linux/clocksource.h>
 #include <linux/cpu.h>
+#include <linux/efi.h>
 #include <linux/reboot.h>
 #include <linux/static_call.h>
 #include <asm/div64.h>
@@ -35,6 +36,8 @@
 #include <asm/apic.h>
 #include <asm/vmware.h>
 #include <asm/svm.h>
+#include <asm/mem_encrypt.h>
+#include <asm/efi.h>
 
 #undef pr_fmt
 #define pr_fmt(fmt)	"vmware: " fmt
@@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
 		pr_warn("Failed to get TSC freq from the hypervisor\n");
 	}
 
+	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
+	    !efi_enabled(EFI_BOOT))
+		x86_init.mpparse.find_mptable = mpparse_find_mptable;
+
 	vmware_paravirt_ops_setup();
 
 #ifdef CONFIG_X86_IO_APIC
-- 
2.39.4
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Borislav Petkov 9 months, 1 week ago 
On Thu, Dec 19, 2024 at 11:44:00AM +0000, Ajay Kaher wrote:
> For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> In this case, mpparse_find_mptable() has to be called to parse MP
> tables which contains boot information.
> 
> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> Signed-off-by: Ye Li <ye.li@broadcom.com>
> Tested-by: Ye Li <ye.li@broadcom.com>

That SOB chain is wrong. Ye's SOB means, he's sending the patch but that
doesn't look like it.

> ---
>  arch/x86/kernel/cpu/vmware.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index 00189cd..3e2594d 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -26,6 +26,7 @@
>  #include <linux/export.h>
>  #include <linux/clocksource.h>
>  #include <linux/cpu.h>
> +#include <linux/efi.h>
>  #include <linux/reboot.h>
>  #include <linux/static_call.h>
>  #include <asm/div64.h>
> @@ -35,6 +36,8 @@
>  #include <asm/apic.h>
>  #include <asm/vmware.h>
>  #include <asm/svm.h>
> +#include <asm/mem_encrypt.h>
> +#include <asm/efi.h>
>  
>  #undef pr_fmt
>  #define pr_fmt(fmt)	"vmware: " fmt
> @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
>  		pr_warn("Failed to get TSC freq from the hypervisor\n");
>  	}
>  
> +	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&

cpu_feature_enabled(X86_FEATURE_SEV_SNP)

> +	    !efi_enabled(EFI_BOOT))
> +		x86_init.mpparse.find_mptable = mpparse_find_mptable;
> +
>  	vmware_paravirt_ops_setup();
>  
>  #ifdef CONFIG_X86_IO_APIC
> -- 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Tom Lendacky 9 months, 1 week ago 
On 3/10/25 05:26, Borislav Petkov wrote:
> On Thu, Dec 19, 2024 at 11:44:00AM +0000, Ajay Kaher wrote:
>> For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
>> In this case, mpparse_find_mptable() has to be called to parse MP
>> tables which contains boot information.
>>
>> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
>> Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
>> Signed-off-by: Ye Li <ye.li@broadcom.com>
>> Tested-by: Ye Li <ye.li@broadcom.com>
> 
> That SOB chain is wrong. Ye's SOB means, he's sending the patch but that
> doesn't look like it.
> 
>> ---
>>  arch/x86/kernel/cpu/vmware.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
>> index 00189cd..3e2594d 100644
>> --- a/arch/x86/kernel/cpu/vmware.c
>> +++ b/arch/x86/kernel/cpu/vmware.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/export.h>
>>  #include <linux/clocksource.h>
>>  #include <linux/cpu.h>
>> +#include <linux/efi.h>
>>  #include <linux/reboot.h>
>>  #include <linux/static_call.h>
>>  #include <asm/div64.h>
>> @@ -35,6 +36,8 @@
>>  #include <asm/apic.h>
>>  #include <asm/vmware.h>
>>  #include <asm/svm.h>
>> +#include <asm/mem_encrypt.h>
>> +#include <asm/efi.h>
>>  
>>  #undef pr_fmt
>>  #define pr_fmt(fmt)	"vmware: " fmt
>> @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
>>  		pr_warn("Failed to get TSC freq from the hypervisor\n");
>>  	}
>>  
>> +	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> 
> cpu_feature_enabled(X86_FEATURE_SEV_SNP)

I think this should really be cc_platform_has(CC_ATTR_GUEST_SEV_SNP).

Thanks,
Tom

> 
>> +	    !efi_enabled(EFI_BOOT))
>> +		x86_init.mpparse.find_mptable = mpparse_find_mptable;
>> +
>>  	vmware_paravirt_ops_setup();
>>  
>>  #ifdef CONFIG_X86_IO_APIC
>> -- 
>
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Ajay Kaher 9 months, 1 week ago 
On Mon, Mar 10, 2025 at 6:42 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 3/10/25 05:26, Borislav Petkov wrote:
> > On Thu, Dec 19, 2024 at 11:44:00AM +0000, Ajay Kaher wrote:
> >> For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> >> In this case, mpparse_find_mptable() has to be called to parse MP
> >> tables which contains boot information.
> >>
> >> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> >> Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> >> Signed-off-by: Ye Li <ye.li@broadcom.com>
> >> Tested-by: Ye Li <ye.li@broadcom.com>
> >
> > That SOB chain is wrong. Ye's SOB means, he's sending the patch but that
> > doesn't look like it.

I will add CDB for Ye in v2.

> >> ---
> >>  arch/x86/kernel/cpu/vmware.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> >> index 00189cd..3e2594d 100644
> >> --- a/arch/x86/kernel/cpu/vmware.c
> >> +++ b/arch/x86/kernel/cpu/vmware.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/export.h>
> >>  #include <linux/clocksource.h>
> >>  #include <linux/cpu.h>
> >> +#include <linux/efi.h>
> >>  #include <linux/reboot.h>
> >>  #include <linux/static_call.h>
> >>  #include <asm/div64.h>
> >> @@ -35,6 +36,8 @@
> >>  #include <asm/apic.h>
> >>  #include <asm/vmware.h>
> >>  #include <asm/svm.h>
> >> +#include <asm/mem_encrypt.h>
> >> +#include <asm/efi.h>
> >>
> >>  #undef pr_fmt
> >>  #define pr_fmt(fmt) "vmware: " fmt
> >> @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
> >>              pr_warn("Failed to get TSC freq from the hypervisor\n");
> >>      }
> >>
> >> +    if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> >
> > cpu_feature_enabled(X86_FEATURE_SEV_SNP)
>
> I think this should really be cc_platform_has(CC_ATTR_GUEST_SEV_SNP).

cc_platform_has(CC_ATTR_GUEST_SEV_SNP) works, I will make appropriate
changes in v2.

Thanks Borislav, Tom.

- Ajay
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Borislav Petkov 9 months, 1 week ago 
On Mon, Mar 10, 2025 at 08:12:46AM -0500, Tom Lendacky wrote:
> >> +	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> > 
> > cpu_feature_enabled(X86_FEATURE_SEV_SNP)
> 
> I think this should really be cc_platform_has(CC_ATTR_GUEST_SEV_SNP).

Ah, we wanna check the guest, sure.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Kevin Loughlin 11 months, 3 weeks ago 
On Thu, Dec 19, 2024 at 6:44 AM Ajay Kaher <ajay.kaher@broadcom.com> wrote:
>
> For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> In this case, mpparse_find_mptable() has to be called to parse MP
> tables which contains boot information.
>
> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> Signed-off-by: Ye Li <ye.li@broadcom.com>
> Tested-by: Ye Li <ye.li@broadcom.com>
> ---
>  arch/x86/kernel/cpu/vmware.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index 00189cd..3e2594d 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -26,6 +26,7 @@
>  #include <linux/export.h>
>  #include <linux/clocksource.h>
>  #include <linux/cpu.h>
> +#include <linux/efi.h>
>  #include <linux/reboot.h>
>  #include <linux/static_call.h>
>  #include <asm/div64.h>
> @@ -35,6 +36,8 @@
>  #include <asm/apic.h>
>  #include <asm/vmware.h>
>  #include <asm/svm.h>
> +#include <asm/mem_encrypt.h>
> +#include <asm/efi.h>
>
>  #undef pr_fmt
>  #define pr_fmt(fmt)    "vmware: " fmt
> @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
>                 pr_warn("Failed to get TSC freq from the hypervisor\n");
>         }
>
> +       if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> +           !efi_enabled(EFI_BOOT))
> +               x86_init.mpparse.find_mptable = mpparse_find_mptable;

As far I know, Linux itself currently doesn't PVALIDATE the address
ranges scanned in mpparse_find_mptable(), and Linux accesses these
addresses as encrypted during early boot. Given this, am I correct
that the guest firmware that you're using is doing the PVALIDATE of
these ranges before starting Linux (else there would be a crash upon
scan)? And then presumably the firmware is also making sure that this
memory is encrypted so that Linux isn't reading unencrypted data as
encrypted (i.e., garbage)?

If so, does that mean all the ROM region scans removed in [0] are
permissible for SEV-SNP guests booting on whichever guest firmware
this is? But you only want/need the mptable info here?

[0] 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
SEV-SNP guests")

More broadly, ideally the guest firmware would communicate to Linux
that these ranges are safe to access (perhaps via the e820 table),
rather than Linux making the assumption that the ranges are safe for
non-EFI SEV-SNP guest boots in this scenario. However, since you're
only changing vmware_platform_setup() for such boots, I don't think we
need to hold up this patch on that generality.
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Ajay Kaher 11 months, 2 weeks ago 
On Thu, Dec 26, 2024 at 9:26 PM Kevin Loughlin <kevinloughlin@google.com> wrote:
>
> On Thu, Dec 19, 2024 at 6:44 AM Ajay Kaher <ajay.kaher@broadcom.com> wrote:
> >
> > For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> > In this case, mpparse_find_mptable() has to be called to parse MP
> > tables which contains boot information.
> >
> > Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> > Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> > Signed-off-by: Ye Li <ye.li@broadcom.com>
> > Tested-by: Ye Li <ye.li@broadcom.com>
> > ---
> >  arch/x86/kernel/cpu/vmware.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> > index 00189cd..3e2594d 100644
> > --- a/arch/x86/kernel/cpu/vmware.c
> > +++ b/arch/x86/kernel/cpu/vmware.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/export.h>
> >  #include <linux/clocksource.h>
> >  #include <linux/cpu.h>
> > +#include <linux/efi.h>
> >  #include <linux/reboot.h>
> >  #include <linux/static_call.h>
> >  #include <asm/div64.h>
> > @@ -35,6 +36,8 @@
> >  #include <asm/apic.h>
> >  #include <asm/vmware.h>
> >  #include <asm/svm.h>
> > +#include <asm/mem_encrypt.h>
> > +#include <asm/efi.h>
> >
> >  #undef pr_fmt
> >  #define pr_fmt(fmt)    "vmware: " fmt
> > @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
> >                 pr_warn("Failed to get TSC freq from the hypervisor\n");
> >         }
> >
> > +       if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> > +           !efi_enabled(EFI_BOOT))
> > +               x86_init.mpparse.find_mptable = mpparse_find_mptable;
>
> As far I know, Linux itself currently doesn't PVALIDATE the address
> ranges scanned in mpparse_find_mptable(), and Linux accesses these
> addresses as encrypted during early boot. Given this, am I correct
> that the guest firmware that you're using is doing the PVALIDATE of
> these ranges before starting Linux (else there would be a crash upon
> scan)? And then presumably the firmware is also making sure that this
> memory is encrypted so that Linux isn't reading unencrypted data as
> encrypted (i.e., garbage)?

Yes, Linux (as a guest) receives valid data.

> If so, does that mean all the ROM region scans removed in [0] are
> permissible for SEV-SNP guests booting on whichever guest firmware
> this is? But you only want/need the mptable info here?

Yes, VMware firmware validates MPTABLE info when 'non-EFI guest boots
on VMware Hypervisor'. The other ROM regions removed in [0] were not
encrypted and validated. This is a specific use case for the VMware platform.
Linux today assumes SNP VMs will be booted with UEFI.

> [0] 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
> SEV-SNP guests")
>
> More broadly, ideally the guest firmware would communicate to Linux
> that these ranges are safe to access (perhaps via the e820 table),
> rather than Linux making the assumption that the ranges are safe for
> non-EFI SEV-SNP guest boots in this scenario. However, since you're
> only changing vmware_platform_setup() for such boots, I don't think we
> need to hold up this patch on that generality.

This is a VMware specific scenario. VMware Hypervisor makes the MPTABLE
memory available in e820 when 'non-EFI guest boots on VMware Hypervisor'.
So it makes sense to do the changes only for vmware_platform_setup().

- Ajay
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Kevin Loughlin 11 months ago 
On Thu, Jan 2, 2025 at 8:31 PM Ajay Kaher <ajay.kaher@broadcom.com> wrote:
>
> On Thu, Dec 26, 2024 at 9:26 PM Kevin Loughlin <kevinloughlin@google.com> wrote:
> >
> > On Thu, Dec 19, 2024 at 6:44 AM Ajay Kaher <ajay.kaher@broadcom.com> wrote:
> > >
> > > For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> > > In this case, mpparse_find_mptable() has to be called to parse MP
> > > tables which contains boot information.
> > >
> > > Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> > > Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> > > Signed-off-by: Ye Li <ye.li@broadcom.com>
> > > Tested-by: Ye Li <ye.li@broadcom.com>
> > > ---
> > >  arch/x86/kernel/cpu/vmware.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> > > index 00189cd..3e2594d 100644
> > > --- a/arch/x86/kernel/cpu/vmware.c
> > > +++ b/arch/x86/kernel/cpu/vmware.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/export.h>
> > >  #include <linux/clocksource.h>
> > >  #include <linux/cpu.h>
> > > +#include <linux/efi.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/static_call.h>
> > >  #include <asm/div64.h>
> > > @@ -35,6 +36,8 @@
> > >  #include <asm/apic.h>
> > >  #include <asm/vmware.h>
> > >  #include <asm/svm.h>
> > > +#include <asm/mem_encrypt.h>
> > > +#include <asm/efi.h>
> > >
> > >  #undef pr_fmt
> > >  #define pr_fmt(fmt)    "vmware: " fmt
> > > @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
> > >                 pr_warn("Failed to get TSC freq from the hypervisor\n");
> > >         }
> > >
> > > +       if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> > > +           !efi_enabled(EFI_BOOT))
> > > +               x86_init.mpparse.find_mptable = mpparse_find_mptable;
> >
> > As far I know, Linux itself currently doesn't PVALIDATE the address
> > ranges scanned in mpparse_find_mptable(), and Linux accesses these
> > addresses as encrypted during early boot. Given this, am I correct
> > that the guest firmware that you're using is doing the PVALIDATE of
> > these ranges before starting Linux (else there would be a crash upon
> > scan)? And then presumably the firmware is also making sure that this
> > memory is encrypted so that Linux isn't reading unencrypted data as
> > encrypted (i.e., garbage)?
>
> Yes, Linux (as a guest) receives valid data.
>
> > If so, does that mean all the ROM region scans removed in [0] are
> > permissible for SEV-SNP guests booting on whichever guest firmware
> > this is? But you only want/need the mptable info here?
>
> Yes, VMware firmware validates MPTABLE info when 'non-EFI guest boots
> on VMware Hypervisor'. The other ROM regions removed in [0] were not
> encrypted and validated. This is a specific use case for the VMware platform.
> Linux today assumes SNP VMs will be booted with UEFI.
>
> > [0] 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
> > SEV-SNP guests")
> >
> > More broadly, ideally the guest firmware would communicate to Linux
> > that these ranges are safe to access (perhaps via the e820 table),
> > rather than Linux making the assumption that the ranges are safe for
> > non-EFI SEV-SNP guest boots in this scenario. However, since you're
> > only changing vmware_platform_setup() for such boots, I don't think we
> > need to hold up this patch on that generality.
>
> This is a VMware specific scenario. VMware Hypervisor makes the MPTABLE
> memory available in e820 when 'non-EFI guest boots on VMware Hypervisor'.
> So it makes sense to do the changes only for vmware_platform_setup().

Works for me. Just wanted to make sure we don't regress Linux to
making assumptions about the safety of accessing memory regions in
CoCo guests without having entries in the e820 table for them.

Reviewed-by: Kevin Loughlin <kevinloughlin@google.com>
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Ajay Kaher 9 months, 2 weeks ago 
On Sat, Jan 18, 2025 at 4:26 AM Kevin Loughlin <kevinloughlin@google.com> wrote:
>
> > > > For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> > > > In this case, mpparse_find_mptable() has to be called to parse MP
> > > > tables which contains boot information.
> > > >
> > > > Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> > > > Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> > > > Signed-off-by: Ye Li <ye.li@broadcom.com>
> > > > Tested-by: Ye Li <ye.li@broadcom.com>
> > > > ---
> > > >  arch/x86/kernel/cpu/vmware.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> > > > index 00189cd..3e2594d 100644
> > > > --- a/arch/x86/kernel/cpu/vmware.c
> > > > +++ b/arch/x86/kernel/cpu/vmware.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include <linux/export.h>
> > > >  #include <linux/clocksource.h>
> > > >  #include <linux/cpu.h>
> > > > +#include <linux/efi.h>
> > > >  #include <linux/reboot.h>
> > > >  #include <linux/static_call.h>
> > > >  #include <asm/div64.h>
> > > > @@ -35,6 +36,8 @@
> > > >  #include <asm/apic.h>
> > > >  #include <asm/vmware.h>
> > > >  #include <asm/svm.h>
> > > > +#include <asm/mem_encrypt.h>
> > > > +#include <asm/efi.h>
> > > >
> > > >  #undef pr_fmt
> > > >  #define pr_fmt(fmt)    "vmware: " fmt
> > > > @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
> > > >                 pr_warn("Failed to get TSC freq from the hypervisor\n");
> > > >         }
> > > >
> > > > +       if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> > > > +           !efi_enabled(EFI_BOOT))
> > > > +               x86_init.mpparse.find_mptable = mpparse_find_mptable;
> > >
> > > As far I know, Linux itself currently doesn't PVALIDATE the address
> > > ranges scanned in mpparse_find_mptable(), and Linux accesses these
> > > addresses as encrypted during early boot. Given this, am I correct
> > > that the guest firmware that you're using is doing the PVALIDATE of
> > > these ranges before starting Linux (else there would be a crash upon
> > > scan)? And then presumably the firmware is also making sure that this
> > > memory is encrypted so that Linux isn't reading unencrypted data as
> > > encrypted (i.e., garbage)?
> >
> > Yes, Linux (as a guest) receives valid data.
> >
> > > If so, does that mean all the ROM region scans removed in [0] are
> > > permissible for SEV-SNP guests booting on whichever guest firmware
> > > this is? But you only want/need the mptable info here?
> >
> > Yes, VMware firmware validates MPTABLE info when 'non-EFI guest boots
> > on VMware Hypervisor'. The other ROM regions removed in [0] were not
> > encrypted and validated. This is a specific use case for the VMware platform.
> > Linux today assumes SNP VMs will be booted with UEFI.
> >
> > > [0] 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
> > > SEV-SNP guests")
> > >
> > > More broadly, ideally the guest firmware would communicate to Linux
> > > that these ranges are safe to access (perhaps via the e820 table),
> > > rather than Linux making the assumption that the ranges are safe for
> > > non-EFI SEV-SNP guest boots in this scenario. However, since you're
> > > only changing vmware_platform_setup() for such boots, I don't think we
> > > need to hold up this patch on that generality.
> >
> > This is a VMware specific scenario. VMware Hypervisor makes the MPTABLE
> > memory available in e820 when 'non-EFI guest boots on VMware Hypervisor'.
> > So it makes sense to do the changes only for vmware_platform_setup().
>
> Works for me. Just wanted to make sure we don't regress Linux to
> making assumptions about the safety of accessing memory regions in
> CoCo guests without having entries in the e820 table for them.
>
> Reviewed-by: Kevin Loughlin <kevinloughlin@google.com>

Adding 'Borislav Petkov <bp@suse.de>' to this mail.

I am looking for an update on this patch, please let me know
if there are any issues preventing it from being merged, or if
you need any additional information.

Link to patch:
https://lore.kernel.org/lkml/20241219114400.858980-1-ajay.kaher@broadcom.com/

-Ajay
Re: [PATCH] sev-snp: parse MP tables for VMware hypervisor
Posted by Ajay Kaher 11 months ago 
On Fri, Jan 3, 2025 at 10:01 AM Ajay Kaher <ajay.kaher@broadcom.com> wrote:
> > > For VMware hypervisor, SEV-SNP enabled VM's could boot without UEFI.
> > > In this case, mpparse_find_mptable() has to be called to parse MP
> > > tables which contains boot information.
> > >
> > > Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> > > Signed-off-by: Ajay Kaher <ajay.kaher@broadcom.com>
> > > Signed-off-by: Ye Li <ye.li@broadcom.com>
> > > Tested-by: Ye Li <ye.li@broadcom.com>
> > > ---
> > >  arch/x86/kernel/cpu/vmware.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> > > index 00189cd..3e2594d 100644
> > > --- a/arch/x86/kernel/cpu/vmware.c
> > > +++ b/arch/x86/kernel/cpu/vmware.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/export.h>
> > >  #include <linux/clocksource.h>
> > >  #include <linux/cpu.h>
> > > +#include <linux/efi.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/static_call.h>
> > >  #include <asm/div64.h>
> > > @@ -35,6 +36,8 @@
> > >  #include <asm/apic.h>
> > >  #include <asm/vmware.h>
> > >  #include <asm/svm.h>
> > > +#include <asm/mem_encrypt.h>
> > > +#include <asm/efi.h>
> > >
> > >  #undef pr_fmt
> > >  #define pr_fmt(fmt)    "vmware: " fmt
> > > @@ -429,6 +432,10 @@ static void __init vmware_platform_setup(void)
> > >                 pr_warn("Failed to get TSC freq from the hypervisor\n");
> > >         }
> > >
> > > +       if (sev_status & MSR_AMD64_SEV_SNP_ENABLED &&
> > > +           !efi_enabled(EFI_BOOT))
> > > +               x86_init.mpparse.find_mptable = mpparse_find_mptable;
> >
> > As far I know, Linux itself currently doesn't PVALIDATE the address
> > ranges scanned in mpparse_find_mptable(), and Linux accesses these
> > addresses as encrypted during early boot. Given this, am I correct
> > that the guest firmware that you're using is doing the PVALIDATE of
> > these ranges before starting Linux (else there would be a crash upon
> > scan)? And then presumably the firmware is also making sure that this
> > memory is encrypted so that Linux isn't reading unencrypted data as
> > encrypted (i.e., garbage)?
>
> Yes, Linux (as a guest) receives valid data.
>
> > If so, does that mean all the ROM region scans removed in [0] are
> > permissible for SEV-SNP guests booting on whichever guest firmware
> > this is? But you only want/need the mptable info here?
>
> Yes, VMware firmware validates MPTABLE info when 'non-EFI guest boots
> on VMware Hypervisor'. The other ROM regions removed in [0] were not
> encrypted and validated. This is a specific use case for the VMware platform.
> Linux today assumes SNP VMs will be booted with UEFI.
>
> > [0] 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
> > SEV-SNP guests")
> >
> > More broadly, ideally the guest firmware would communicate to Linux
> > that these ranges are safe to access (perhaps via the e820 table),
> > rather than Linux making the assumption that the ranges are safe for
> > non-EFI SEV-SNP guest boots in this scenario. However, since you're
> > only changing vmware_platform_setup() for such boots, I don't think we
> > need to hold up this patch on that generality.
>
> This is a VMware specific scenario. VMware Hypervisor makes the MPTABLE
> memory available in e820 when 'non-EFI guest boots on VMware Hypervisor'.
> So it makes sense to do the changes only for vmware_platform_setup().

Kevin, thanks for your time and sharing your thoughts. Please let me know
if you have any updates or need further information from my side.

Looking forward to hearing from you.

- Ajay

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.