During early boot phases, check for the presence of an SVSM when running
as an SEV-SNP guest.
An SVSM is present if not running at VMPL0 and the 64-bit value at offset
0x148 into the secrets page is non-zero. If an SVSM is present, save the
SVSM Calling Area address (CAA), located at offset 0x150 into the secrets
page, and set the VMPL level of the guest, which should be non-zero, to
indicate the presence of an SVSM.
Signed-off-by: Tom Lendacky <[email protected]>
---
.../arch/x86/amd-memory-encryption.rst | 22 ++++++
arch/x86/boot/compressed/sev.c | 8 +++
arch/x86/include/asm/sev-common.h | 4 ++
arch/x86/include/asm/sev.h | 25 ++++++-
arch/x86/kernel/sev-shared.c | 70 +++++++++++++++++++
arch/x86/kernel/sev.c | 7 ++
6 files changed, 135 insertions(+), 1 deletion(-)
diff --git a/Documentation/arch/x86/amd-memory-encryption.rst b/Documentation/arch/x86/amd-memory-encryption.rst
index 414bc7402ae7..32737718d4a2 100644
--- a/Documentation/arch/x86/amd-memory-encryption.rst
+++ b/Documentation/arch/x86/amd-memory-encryption.rst
@@ -130,4 +130,26 @@ SNP feature support.
More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
+Secure VM Service Module (SVSM)
+===============================
+
+SNP provides a feature called Virtual Machine Privilege Levels (VMPL). The most
+privileged VMPL is 0 with numerically higher numbers having lesser privileges.
+More details in AMD64 APM[1] Vol 2: 15.35.7 Virtual Machine Privilege Levels.
+
+The VMPL feature provides the ability to run software services at a more
+privileged level than the guest OS is running at. This provides a secure
+environment for services within the guest's SNP environment, while protecting
+the service from hypervisor interference. An example of a secure service
+would be a virtual TPM (vTPM). Additionally, certain operations require the
+guest to be running at VMPL0 in order for them to be performed. For example,
+the PVALIDATE instruction is required to be executed at VMPL0.
+
+When a guest is not running at VMPL0, it needs to communicate with the software
+running at VMPL0 to perform privileged operations or to interact with secure
+services. This software running at VMPL0 is known as a Secure VM Service Module
+(SVSM). Discovery of an SVSM and the API used to communicate with it is
+documented in Secure VM Service Module for SEV-SNP Guests[2].
+
[1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
+[2] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 0457a9d7e515..cb771b380a6b 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -12,6 +12,7 @@
*/
#include "misc.h"
+#include <linux/mm.h>
#include <asm/bootparam.h>
#include <asm/pgtable_types.h>
#include <asm/sev.h>
@@ -462,6 +463,13 @@ static bool early_snp_init(struct boot_params *bp)
*/
setup_cpuid_table(cc_info);
+ /*
+ * Record the SVSM Calling Area (CA) address if the guest is not
+ * running at VMPL0. The CA will be used to communicate with the
+ * SVSM to perform the SVSM services.
+ */
+ setup_svsm_ca(cc_info);
+
/*
* Pass run-time kernel a pointer to CC info via boot_params so EFI
* config table doesn't need to be searched again during early startup
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index b463fcbd4b90..1225744a069b 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -159,6 +159,10 @@ struct snp_psc_desc {
#define GHCB_TERM_NOT_VMPL0 3 /* SNP guest is not running at VMPL-0 */
#define GHCB_TERM_CPUID 4 /* CPUID-validation failure */
#define GHCB_TERM_CPUID_HV 5 /* CPUID failure during hypervisor fallback */
+#define GHCB_TERM_SECRETS_PAGE 6 /* Secrets page failure */
+#define GHCB_TERM_NO_SVSM 7 /* SVSM is not advertised in the secrets page */
+#define GHCB_TERM_SVSM_VMPL0 8 /* SVSM is present but has set VMPL to 0 */
+#define GHCB_TERM_SVSM_CAA 9 /* SVSM is present but CAA is not page aligned */
#define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 48bc397db649..56f7d843f7a4 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -152,9 +152,32 @@ struct snp_secrets_page {
u8 vmpck2[VMPCK_KEY_LEN];
u8 vmpck3[VMPCK_KEY_LEN];
struct secrets_os_area os_area;
- u8 rsvd3[3840];
+
+ u8 vmsa_tweak_bitmap[64];
+
+ /* SVSM fields */
+ u64 svsm_base;
+ u64 svsm_size;
+ u64 svsm_caa;
+ u32 svsm_max_version;
+ u8 svsm_guest_vmpl;
+ u8 rsvd3[3];
+
+ /* Remainder of page */
+ u8 rsvd4[3744];
} __packed;
+/*
+ * The SVSM Calling Area (CA) related structures.
+ */
+struct svsm_ca {
+ u8 call_pending;
+ u8 mem_available;
+ u8 rsvd1[6];
+
+ u8 svsm_buffer[PAGE_SIZE - 8];
+};
+
#ifdef CONFIG_AMD_MEM_ENCRYPT
extern void __sev_es_ist_enter(struct pt_regs *regs);
extern void __sev_es_ist_exit(void);
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index b4f8fa0f722c..46ea4e5e118a 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -23,6 +23,21 @@
#define sev_printk_rtl(fmt, ...)
#endif
+/*
+ * SVSM related information:
+ * When running under an SVSM, the VMPL that Linux is executing at must be
+ * non-zero. The VMPL is therefore used to indicate the presence of an SVSM.
+ *
+ * During boot, the page tables are set up as identity mapped and later
+ * changed to use kernel virtual addresses. Maintain separate virtual and
+ * physical addresses for the CAA to allow SVSM functions to be used during
+ * early boot, both with identity mapped virtual addresses and proper kernel
+ * virtual addresses.
+ */
+static u8 vmpl __ro_after_init;
+static struct svsm_ca *boot_svsm_caa __ro_after_init;
+static u64 boot_svsm_caa_pa __ro_after_init;
+
/* I/O parameters for CPUID-related helpers */
struct cpuid_leaf {
u32 fn;
@@ -1269,3 +1284,58 @@ static enum es_result vc_check_opcode_bytes(struct es_em_ctxt *ctxt,
return ES_UNSUPPORTED;
}
+
+/*
+ * Maintain the GPA of the SVSM Calling Area (CA) in order to utilize the SVSM
+ * services needed when not running in VMPL0.
+ */
+static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
+{
+ struct snp_secrets_page *secrets_page;
+ u64 caa;
+
+ BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
+
+ /*
+ * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
+ * higher) privilege level. Here, clear the VMPL1 permission mask of the
+ * GHCB page. If the guest is not running at VMPL0, this will fail.
+ *
+ * If the guest is running at VMPL0, it will succeed. Even if that operation
+ * modifies permission bits, it is still ok to do so currently because Linux
+ * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
+ * permission mask changes are a don't-care.
+ *
+ * Use __pa() since this routine is running identity mapped when called,
+ * both by the decompressor code and the early kernel code.
+ */
+ if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
+ return;
+
+ /*
+ * Not running at VMPL0, ensure everything has been properly supplied
+ * for running under an SVSM.
+ */
+ if (!cc_info || !cc_info->secrets_phys || cc_info->secrets_len != PAGE_SIZE)
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECRETS_PAGE);
+
+ secrets_page = (struct snp_secrets_page *)cc_info->secrets_phys;
+ if (!secrets_page->svsm_size)
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NO_SVSM);
+
+ if (!secrets_page->svsm_guest_vmpl)
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_VMPL0);
+
+ vmpl = secrets_page->svsm_guest_vmpl;
+
+ caa = secrets_page->svsm_caa;
+ if (!PAGE_ALIGNED(caa))
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_CAA);
+
+ /*
+ * The CA is identity mapped when this routine is called, both by the
+ * decompressor code and the early kernel code.
+ */
+ boot_svsm_caa = (struct svsm_ca *)caa;
+ boot_svsm_caa_pa = caa;
+}
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 6949fbccec40..a500df807e79 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -2108,6 +2108,13 @@ bool __head snp_init(struct boot_params *bp)
setup_cpuid_table(cc_info);
+ /*
+ * Record the SVSM Calling Area address (CAA) if the guest is not
+ * running at VMPL0. The CA will be used to communicate with the
+ * SVSM to perform the SVSM services.
+ */
+ setup_svsm_ca(cc_info);
+
/*
* The CC blob will be used later to access the secrets page. Cache
* it here like the boot kernel does.
--
2.43.2
On Wed, Apr 24, 2024 at 10:58:00AM -0500, Tom Lendacky wrote:
> During early boot phases, check for the presence of an SVSM when running
> as an SEV-SNP guest.
>
> An SVSM is present if not running at VMPL0 and the 64-bit value at offset
> 0x148 into the secrets page is non-zero. If an SVSM is present, save the
> SVSM Calling Area address (CAA), located at offset 0x150 into the secrets
> page, and set the VMPL level of the guest, which should be non-zero, to
> indicate the presence of an SVSM.
>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> .../arch/x86/amd-memory-encryption.rst | 22 ++++++
> arch/x86/boot/compressed/sev.c | 8 +++
> arch/x86/include/asm/sev-common.h | 4 ++
> arch/x86/include/asm/sev.h | 25 ++++++-
> arch/x86/kernel/sev-shared.c | 70 +++++++++++++++++++
> arch/x86/kernel/sev.c | 7 ++
> 6 files changed, 135 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/arch/x86/amd-memory-encryption.rst b/Documentation/arch/x86/amd-memory-encryption.rst
> index 414bc7402ae7..32737718d4a2 100644
> --- a/Documentation/arch/x86/amd-memory-encryption.rst
> +++ b/Documentation/arch/x86/amd-memory-encryption.rst
> @@ -130,4 +130,26 @@ SNP feature support.
>
> More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
>
> +Secure VM Service Module (SVSM)
> +===============================
> +
> +SNP provides a feature called Virtual Machine Privilege Levels (VMPL). The most
> +privileged VMPL is 0 with numerically higher numbers having lesser privileges.
> +More details in AMD64 APM[1] Vol 2: 15.35.7 Virtual Machine Privilege Levels.
> +
> +The VMPL feature provides the ability to run software services at a more
> +privileged level than the guest OS is running at. This provides a secure
Too many "provides".
> +environment for services within the guest's SNP environment, while protecting
> +the service from hypervisor interference. An example of a secure service
> +would be a virtual TPM (vTPM). Additionally, certain operations require the
> +guest to be running at VMPL0 in order for them to be performed. For example,
> +the PVALIDATE instruction is required to be executed at VMPL0.
> +
> +When a guest is not running at VMPL0, it needs to communicate with the software
> +running at VMPL0 to perform privileged operations or to interact with secure
> +services. This software running at VMPL0 is known as a Secure VM Service Module
> +(SVSM). Discovery of an SVSM and the API used to communicate with it is
> +documented in Secure VM Service Module for SEV-SNP Guests[2].
This paragraph needs to go second, not third.
Somehow that text is missing "restraint" and is all over the place.
Lemme try to restructure it:
"SNP provides a feature called Virtual Machine Privilege Levels (VMPL) which
defines four privilege levels at which guest software can run. The most
privileged level is 0 and numerically higher numbers have lesser privileges.
More details in the AMD64 APM[1] Vol 2, section "15.35.7 Virtual Machine
Privilege Levels", docID: 24593.
When using that feature, different services can run at different protection
levels, apart from the guest OS but still within the secure SNP environment.
They can provide services to the guest, like a vTPM, for example.
When a guest is not running at VMPL0, it needs to communicate with the software
running at VMPL0 to perform privileged operations or to interact with secure
services. An example fur such a privileged operation is PVALIDATE which is
*required* to be executed at VMPL0.
In this scenario, the software running at VMPL0 is usually called a Secure VM
Service Module (SVSM). Discovery of an SVSM and the API used to communicate
with it is documented in "Secure VM Service Module for SEV-SNP Guests", docID:
58019."
How's that?
> +
> [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
> +[2] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
Yeah, about those links - they get stale pretty quickly. I think it suffices to
explain what the document is and what it is called so that one can find it by
searching the web. See what I did above.
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 0457a9d7e515..cb771b380a6b 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -12,6 +12,7 @@
> */
> #include "misc.h"
>
> +#include <linux/mm.h>
Please do not include a kernel-proper header into the decompresssor.
Those things are solved by exposing the shared *minimal* functionality
into
arch/x86/include/asm/shared/
There are examples there.
By the looks of it:
In file included from arch/x86/boot/compressed/sev.c:130:
arch/x86/boot/compressed/../../kernel/sev-shared.c: In function ‘setup_svsm_ca’:
arch/x86/boot/compressed/../../kernel/sev-shared.c:1332:14: warning: implicit declaration of function ‘PAGE_ALIGNED’; did you mean ‘IS_ALIGNED’? [-Wimplicit-function-declaration]
1332 | if (!PAGE_ALIGNED(caa))
| ^~~~~~~~~~~~
| IS_ALIGNED
it'll need PAGE_ALIGNED and IS_ALIGNED into an arch/x86/include/asm/shared/mm.h
header.
> #include <asm/bootparam.h>
> #include <asm/pgtable_types.h>
> #include <asm/sev.h>
..
> +static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
> +{
> + struct snp_secrets_page *secrets_page;
> + u64 caa;
> +
> + BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
> +
> + /*
> + * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> + * higher) privilege level. Here, clear the VMPL1 permission mask of the
> + * GHCB page. If the guest is not running at VMPL0, this will fail.
> + *
> + * If the guest is running at VMPL0, it will succeed. Even if that operation
> + * modifies permission bits, it is still ok to do so currently because Linux
> + * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> + * permission mask changes are a don't-care.
> + *
> + * Use __pa() since this routine is running identity mapped when called,
> + * both by the decompressor code and the early kernel code.
> + */
Let's not replicate that comment. Diff ontop:
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index cb771b380a6b..cde1890c8843 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -576,18 +576,7 @@ void sev_enable(struct boot_params *bp)
if (!(get_hv_features() & GHCB_HV_FT_SNP))
sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
- /*
- * Enforce running at VMPL0.
- *
- * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
- * higher) privilege level. Here, clear the VMPL1 permission mask of the
- * GHCB page. If the guest is not running at VMPL0, this will fail.
- *
- * If the guest is running at VMPL0, it will succeed. Even if that operation
- * modifies permission bits, it is still ok to do so currently because Linux
- * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
- * permission mask changes are a don't-care.
- */
+ /* Enforce running at VMPL0 - see comment above rmpadjust(). */
if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1))
sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
}
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 350db22e66be..b168403c07be 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -204,6 +204,17 @@ static __always_inline void sev_es_nmi_complete(void)
extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
extern void sev_enable(struct boot_params *bp);
+/*
+ * RMPADJUST modifies RMP permissions of a lesser-privileged
+ * (numerically higher) privilege level. If @attrs==0, it will attempt
+ * to clear the VMPL1 permission mask of @vaddr. If the guest is not
+ * running at VMPL0, this will fail.
+ *
+ * If the guest is running at VMPL0, it will succeed. Even if that operation
+ * modifies permission bits, it is still ok to do so currently because Linux
+ * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
+ * permission mask changes are a don't-care.
+ */
static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
{
int rc;
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 46ea4e5e118a..9ca54bcf0e99 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -1297,17 +1297,9 @@ static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
/*
- * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
- * higher) privilege level. Here, clear the VMPL1 permission mask of the
- * GHCB page. If the guest is not running at VMPL0, this will fail.
- *
- * If the guest is running at VMPL0, it will succeed. Even if that operation
- * modifies permission bits, it is still ok to do so currently because Linux
- * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
- * permission mask changes are a don't-care.
- *
- * Use __pa() since this routine is running identity mapped when called,
- * both by the decompressor code and the early kernel code.
+ * See comment above rmpadjust() for details. Use __pa() since
+ * this routine is running identity mapped when called both by
+ * the decompressor code and the early kernel code.
*/
if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
return;
> + if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
> + return;
> +
> + /*
> + * Not running at VMPL0, ensure everything has been properly supplied
> + * for running under an SVSM.
> + */
> + if (!cc_info || !cc_info->secrets_phys || cc_info->secrets_len != PAGE_SIZE)
> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECRETS_PAGE);
> +
> + secrets_page = (struct snp_secrets_page *)cc_info->secrets_phys;
> + if (!secrets_page->svsm_size)
> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NO_SVSM);
> +
> + if (!secrets_page->svsm_guest_vmpl)
> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_VMPL0);
0x15C 1 byte SVSM_GUEST_VMPL Indicates the VMPL at which the guest is executing.
Do I understand it correctly that this contains the VMPL of the guest and the
SVSM is running below it?
IOW, SVSM should be at VMPL0 and the guest should be a at a level determined by
that value and it cannot be 0.
Just making sure I'm reading it right.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 5/2/24 04:35, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 10:58:00AM -0500, Tom Lendacky wrote:
>> During early boot phases, check for the presence of an SVSM when running
>> as an SEV-SNP guest.
>>
>> An SVSM is present if not running at VMPL0 and the 64-bit value at offset
>> 0x148 into the secrets page is non-zero. If an SVSM is present, save the
>> SVSM Calling Area address (CAA), located at offset 0x150 into the secrets
>> page, and set the VMPL level of the guest, which should be non-zero, to
>> indicate the presence of an SVSM.
>>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> .../arch/x86/amd-memory-encryption.rst | 22 ++++++
>> arch/x86/boot/compressed/sev.c | 8 +++
>> arch/x86/include/asm/sev-common.h | 4 ++
>> arch/x86/include/asm/sev.h | 25 ++++++-
>> arch/x86/kernel/sev-shared.c | 70 +++++++++++++++++++
>> arch/x86/kernel/sev.c | 7 ++
>> 6 files changed, 135 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/arch/x86/amd-memory-encryption.rst b/Documentation/arch/x86/amd-memory-encryption.rst
>> index 414bc7402ae7..32737718d4a2 100644
>> --- a/Documentation/arch/x86/amd-memory-encryption.rst
>> +++ b/Documentation/arch/x86/amd-memory-encryption.rst
>> @@ -130,4 +130,26 @@ SNP feature support.
>>
>> More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
>>
>> +Secure VM Service Module (SVSM)
>> +===============================
>> +
>> +SNP provides a feature called Virtual Machine Privilege Levels (VMPL). The most
>> +privileged VMPL is 0 with numerically higher numbers having lesser privileges.
>> +More details in AMD64 APM[1] Vol 2: 15.35.7 Virtual Machine Privilege Levels.
>> +
>> +The VMPL feature provides the ability to run software services at a more
>> +privileged level than the guest OS is running at. This provides a secure
>
> Too many "provides".
>
>> +environment for services within the guest's SNP environment, while protecting
>> +the service from hypervisor interference. An example of a secure service
>> +would be a virtual TPM (vTPM). Additionally, certain operations require the
>> +guest to be running at VMPL0 in order for them to be performed. For example,
>> +the PVALIDATE instruction is required to be executed at VMPL0.
>> +
>> +When a guest is not running at VMPL0, it needs to communicate with the software
>> +running at VMPL0 to perform privileged operations or to interact with secure
>> +services. This software running at VMPL0 is known as a Secure VM Service Module
>> +(SVSM). Discovery of an SVSM and the API used to communicate with it is
>> +documented in Secure VM Service Module for SEV-SNP Guests[2].
>
> This paragraph needs to go second, not third.
>
> Somehow that text is missing "restraint" and is all over the place.
> Lemme try to restructure it:
>
> "SNP provides a feature called Virtual Machine Privilege Levels (VMPL) which
> defines four privilege levels at which guest software can run. The most
> privileged level is 0 and numerically higher numbers have lesser privileges.
> More details in the AMD64 APM[1] Vol 2, section "15.35.7 Virtual Machine
> Privilege Levels", docID: 24593.
>
> When using that feature, different services can run at different protection
> levels, apart from the guest OS but still within the secure SNP environment.
> They can provide services to the guest, like a vTPM, for example.
>
> When a guest is not running at VMPL0, it needs to communicate with the software
> running at VMPL0 to perform privileged operations or to interact with secure
> services. An example fur such a privileged operation is PVALIDATE which is
> *required* to be executed at VMPL0.
>
> In this scenario, the software running at VMPL0 is usually called a Secure VM
> Service Module (SVSM). Discovery of an SVSM and the API used to communicate
> with it is documented in "Secure VM Service Module for SEV-SNP Guests", docID:
> 58019."
>
> How's that?
Works for me.
>
>> +
>> [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
>> +[2] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
>
> Yeah, about those links - they get stale pretty quickly. I think it suffices to
> explain what the document is and what it is called so that one can find it by
> searching the web. See what I did above.
>
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index 0457a9d7e515..cb771b380a6b 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c
>> @@ -12,6 +12,7 @@
>> */
>> #include "misc.h"
>>
>> +#include <linux/mm.h>
>
> Please do not include a kernel-proper header into the decompresssor.
> Those things are solved by exposing the shared *minimal* functionality
> into
Right, should've known that.
>
> arch/x86/include/asm/shared/
>
> There are examples there.
>
> By the looks of it:
>
> In file included from arch/x86/boot/compressed/sev.c:130:
> arch/x86/boot/compressed/../../kernel/sev-shared.c: In function ‘setup_svsm_ca’:
> arch/x86/boot/compressed/../../kernel/sev-shared.c:1332:14: warning: implicit declaration of function ‘PAGE_ALIGNED’; did you mean ‘IS_ALIGNED’? [-Wimplicit-function-declaration]
> 1332 | if (!PAGE_ALIGNED(caa))
> | ^~~~~~~~~~~~
> | IS_ALIGNED
>
> it'll need PAGE_ALIGNED and IS_ALIGNED into an arch/x86/include/asm/shared/mm.h
> header.
PAGE_ALIGNED and IS_ALIGNED are from two separate header files (mm.h and
align.h) which seems like a lot of extra changes for just one check.
Any objection to either adding this define to sev-shared.c on the "else"
patch of the "#ifndef __BOOT_COMPRESSED" check:
#define PAGE_ALIGNED(x) IS_ALIGNED((x), PAGE_SIZE)
or just changing the above check to:
if (!IS_ALIGNED(caa, PAGE_SIZE))
>
>> #include <asm/bootparam.h>
>> #include <asm/pgtable_types.h>
>> #include <asm/sev.h>
>
> ..
>
>> +static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
>> +{
>> + struct snp_secrets_page *secrets_page;
>> + u64 caa;
>> +
>> + BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
>> +
>> + /*
>> + * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
>> + * higher) privilege level. Here, clear the VMPL1 permission mask of the
>> + * GHCB page. If the guest is not running at VMPL0, this will fail.
>> + *
>> + * If the guest is running at VMPL0, it will succeed. Even if that operation
>> + * modifies permission bits, it is still ok to do so currently because Linux
>> + * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
>> + * permission mask changes are a don't-care.
>> + *
>> + * Use __pa() since this routine is running identity mapped when called,
>> + * both by the decompressor code and the early kernel code.
>> + */
>
> Let's not replicate that comment. Diff ontop:
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index cb771b380a6b..cde1890c8843 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -576,18 +576,7 @@ void sev_enable(struct boot_params *bp)
> if (!(get_hv_features() & GHCB_HV_FT_SNP))
> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>
> - /*
> - * Enforce running at VMPL0.
> - *
> - * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> - * higher) privilege level. Here, clear the VMPL1 permission mask of the
> - * GHCB page. If the guest is not running at VMPL0, this will fail.
> - *
> - * If the guest is running at VMPL0, it will succeed. Even if that operation
> - * modifies permission bits, it is still ok to do so currently because Linux
> - * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> - * permission mask changes are a don't-care.
> - */
> + /* Enforce running at VMPL0 - see comment above rmpadjust(). */
Not sure I agree. I'd prefer to keep the comment here because it is
specific to this rmpadjust() call. See below.
> if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1))
> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
> }
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 350db22e66be..b168403c07be 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -204,6 +204,17 @@ static __always_inline void sev_es_nmi_complete(void)
> extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> extern void sev_enable(struct boot_params *bp);
>
> +/*
> + * RMPADJUST modifies RMP permissions of a lesser-privileged
> + * (numerically higher) privilege level. If @attrs==0, it will attempt
> + * to clear the VMPL1 permission mask of @vaddr. If the guest is not
> + * running at VMPL0, this will fail.
> + *
> + * If the guest is running at VMPL0, it will succeed. Even if that operation
> + * modifies permission bits, it is still ok to do so currently because Linux
> + * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> + * permission mask changes are a don't-care.
If you want to put a comment here, then it needs to be more generic. The
attrs value would be 1 if VMPL0 was attempting to clear VMPL1
permissions. Also, you could be running at VMPL2 and successfully clear
or set VMPL3 permissions. So this comment doesn't really flow with a
generic RMPADJUST function.
/*
* RMPAJDUST modifies the RMP permissions of a lesser-privileged
* (numerically higher) VMPL. The @attrs option contains the VMPL
* level to be modified for @vaddr. The operation will succeed only
* if the guest is running at a higher-privileged (numerically lower)
* VMPL.
*/
> + */
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
> {
> int rc;
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 46ea4e5e118a..9ca54bcf0e99 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -1297,17 +1297,9 @@ static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
> BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
>
> /*
> - * RMPADJUST modifies RMP permissions of a lesser-privileged (numerically
> - * higher) privilege level. Here, clear the VMPL1 permission mask of the
> - * GHCB page. If the guest is not running at VMPL0, this will fail.
> - *
> - * If the guest is running at VMPL0, it will succeed. Even if that operation
> - * modifies permission bits, it is still ok to do so currently because Linux
> - * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
> - * permission mask changes are a don't-care.
> - *
> - * Use __pa() since this routine is running identity mapped when called,
> - * both by the decompressor code and the early kernel code.
> + * See comment above rmpadjust() for details. Use __pa() since
> + * this routine is running identity mapped when called both by
> + * the decompressor code and the early kernel code.
> */
> if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
> return;
>
>> + if (!rmpadjust((unsigned long)__pa(&boot_ghcb_page), RMP_PG_SIZE_4K, 1))
>> + return;
>> +
>> + /*
>> + * Not running at VMPL0, ensure everything has been properly supplied
>> + * for running under an SVSM.
>> + */
>> + if (!cc_info || !cc_info->secrets_phys || cc_info->secrets_len != PAGE_SIZE)
>> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SECRETS_PAGE);
>> +
>> + secrets_page = (struct snp_secrets_page *)cc_info->secrets_phys;
>> + if (!secrets_page->svsm_size)
>> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NO_SVSM);
>> +
>> + if (!secrets_page->svsm_guest_vmpl)
>> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_VMPL0);
>
> 0x15C 1 byte SVSM_GUEST_VMPL Indicates the VMPL at which the guest is executing.
>
> Do I understand it correctly that this contains the VMPL of the guest and the
> SVSM is running below it?
Right, the SVSM is supposed to place the VMPL level that it starts the
guest at in this location.
>
> IOW, SVSM should be at VMPL0 and the guest should be a at a level determined by
> that value and it cannot be 0.
Right. Not sure about the "cannot", more like "must not." The
specification states that the guest should run at a VMPL other than 0.
If an SVSM starts the guest at VMPL0, then the SVSM would not be
protected from guest.
Thanks,
Tom
>
> Just making sure I'm reading it right.
>
> Thx.
>
On Thu, May 02, 2024 at 10:29:02AM -0500, Tom Lendacky wrote:
> PAGE_ALIGNED and IS_ALIGNED are from two separate header files (mm.h and
> align.h) which seems like a lot of extra changes for just one check.
No, pls put them in a single shared/mm.h header. And no, those are not
a lot of extra changes - those are changes which are moving the code in
the right direction and we do them sooner rather than later, otherwise
they'd pile up and we'll never be able to find time to do them - sev.c
movement attempt case-in-point.
> Not sure I agree. I'd prefer to keep the comment here because it is
> specific to this rmpadjust() call. See below.
Just don't replicate some versions of the same comment all over the
place. Do one big comment which explains which RMPADJUST has to do with
VMPL levels - perhaps over the insn - and then refer to it from the
other places after adding the specific explanations for them.
> Right. Not sure about the "cannot", more like "must not." The specification
> states that the guest should run at a VMPL other than 0. If an SVSM starts
> the guest at VMPL0, then the SVSM would not be protected from guest.
Yeah, well, you do terminate the guest if it is running at VMPL 0 *in*
the presence of a SVSM so it is a "must not". Ok.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 5/17/24 10:58, Borislav Petkov wrote:
> On Thu, May 02, 2024 at 10:29:02AM -0500, Tom Lendacky wrote:
>> PAGE_ALIGNED and IS_ALIGNED are from two separate header files (mm.h and
>> align.h) which seems like a lot of extra changes for just one check.
>
> No, pls put them in a single shared/mm.h header. And no, those are not
> a lot of extra changes - those are changes which are moving the code in
> the right direction and we do them sooner rather than later, otherwise
> they'd pile up and we'll never be able to find time to do them - sev.c
> movement attempt case-in-point.
So this will be a new shared directory in the top level include
directory (as PAGE_ALIGNED is defined in include/linux/mm.h), not just
in the arch/x86/include directory like the others (io.h, msr.h and
tdx.h). Is that what you want?
Thanks,
Tom
>
>> Not sure I agree. I'd prefer to keep the comment here because it is
>> specific to this rmpadjust() call. See below.
>
> Just don't replicate some versions of the same comment all over the
> place. Do one big comment which explains which RMPADJUST has to do with
> VMPL levels - perhaps over the insn - and then refer to it from the
> other places after adding the specific explanations for them.
>
>> Right. Not sure about the "cannot", more like "must not." The specification
>> states that the guest should run at a VMPL other than 0. If an SVSM starts
>> the guest at VMPL0, then the SVSM would not be protected from guest.
>
> Yeah, well, you do terminate the guest if it is running at VMPL 0 *in*
> the presence of a SVSM so it is a "must not". Ok.
>
On Mon, May 20, 2024 at 08:57:43AM -0500, Tom Lendacky wrote:
> So this will be a new shared directory in the top level include directory
> (as PAGE_ALIGNED is defined in include/linux/mm.h), not just in the
> arch/x86/include directory like the others (io.h, msr.h and tdx.h). Is that
> what you want?
You can actually do this - it is a lot easier and still clean:
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index cb771b380a6b..5ee53a7a060e 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -12,7 +12,6 @@
*/
#include "misc.h"
-#include <linux/mm.h>
#include <asm/bootparam.h>
#include <asm/pgtable_types.h>
#include <asm/sev.h>
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 46ea4e5e118a..bd4bbb30ef0c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -1329,7 +1329,12 @@ static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
vmpl = secrets_page->svsm_guest_vmpl;
caa = secrets_page->svsm_caa;
- if (!PAGE_ALIGNED(caa))
+
+ /*
+ * Open-code PAGE_ALIGNED() to avoid pulling in the world and
+ * more by including linux/mm.h.
+ */
+ if (caa & (PAGE_SIZE - 1))
sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_CAA);
/*
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 5/22/24 10:27, Borislav Petkov wrote:
> On Mon, May 20, 2024 at 08:57:43AM -0500, Tom Lendacky wrote:
>> So this will be a new shared directory in the top level include directory
>> (as PAGE_ALIGNED is defined in include/linux/mm.h), not just in the
>> arch/x86/include directory like the others (io.h, msr.h and tdx.h). Is that
>> what you want?
>
> You can actually do this - it is a lot easier and still clean:
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index cb771b380a6b..5ee53a7a060e 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -12,7 +12,6 @@
> */
> #include "misc.h"
>
> -#include <linux/mm.h>
> #include <asm/bootparam.h>
> #include <asm/pgtable_types.h>
> #include <asm/sev.h>
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 46ea4e5e118a..bd4bbb30ef0c 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -1329,7 +1329,12 @@ static void __head setup_svsm_ca(const struct cc_blob_sev_info *cc_info)
> vmpl = secrets_page->svsm_guest_vmpl;
>
> caa = secrets_page->svsm_caa;
> - if (!PAGE_ALIGNED(caa))
> +
> + /*
> + * Open-code PAGE_ALIGNED() to avoid pulling in the world and
> + * more by including linux/mm.h.
> + */
> + if (caa & (PAGE_SIZE - 1))
Or what I originally proposed:
if (!IS_ALIGNED(caa, PAGE_SIZE))
Which also works without including mm.h.
Thanks,
Tom
> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SVSM_CAA);
>
> /*
>
On Wed, May 22, 2024 at 11:15:28AM -0500, Tom Lendacky wrote:
> Or what I originally proposed:
>
> if (!IS_ALIGNED(caa, PAGE_SIZE))
>
> Which also works without including mm.h.
It works because of the same nasty reason:
$ make arch/x86/boot/compressed/sev.i
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CPP arch/x86/boot/compressed/sev.i
$ grep align.h arch/x86/boot/compressed/sev.i
# 1 "./include/linux/align.h" 1
The include hell pulls in that linux/ namespace header into the
decompressor. Which it should not to.
So no, please use the open-coded thing. I probably should start
untangling the decompressor slowly and do small sets because otherwise
this'll never get split properly.
;-\
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette