diff mbox series

[Part1,RFC,v4,05/36] x86/sev: Define the Linux specific guest termination reasons

Message ID 20210707181506.30489-6-brijesh.singh@amd.com
State Accepted
Commit 6c0f74d678c94060932683738b3e227995b363d3
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand

Commit Message

Brijesh Singh July 7, 2021, 6:14 p.m. UTC
GHCB specification defines the reason code for reason set 0. The reason
codes defined in the set 0 do not cover all possible causes for a guest
to request termination.

The reason set 1 to 255 is reserved for the vendor-specific codes.
Reseve the reason set 1 for the Linux guest. Define an error codes for
reason set 1.

While at it, change the sev_es_terminate() to accept the reason set
parameter.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/boot/compressed/sev.c    |  6 +++---
 arch/x86/include/asm/sev-common.h |  8 ++++++++
 arch/x86/kernel/sev-shared.c      | 11 ++++-------
 arch/x86/kernel/sev.c             |  4 ++--
 4 files changed, 17 insertions(+), 12 deletions(-)

Comments

Borislav Petkov Aug. 10, 2021, 11:33 a.m. UTC | #1
On Wed, Jul 07, 2021 at 01:14:35PM -0500, Brijesh Singh wrote:
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 23929a3010df..e75e29c05f59 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -63,9 +63,17 @@
>  	(((((u64)reason_set) &  GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \
>  	((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS))
>  
> +/* Error code from reason set 0 */

... Error codes...

> +#define SEV_TERM_SET_GEN		0
>  #define GHCB_SEV_ES_GEN_REQ		0
>  #define GHCB_SEV_ES_PROT_UNSUPPORTED	1
>  
>  #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>  
> +/* Linux specific reason codes (used with reason set 1) */

... Linux-specific ...

> +#define SEV_TERM_SET_LINUX		1

GHCB doc says:

"This document defines and owns reason code set 0x0"

Should it also say, reason code set 1 is allocated for Linux guest use?
I don't see why not...

Tom?

> +#define GHCB_TERM_REGISTER		0	/* GHCB GPA registration failure */
> +#define GHCB_TERM_PSC			1	/* Page State Change failure */
> +#define GHCB_TERM_PVALIDATE		2	/* Pvalidate failure */
> +
>  #endif
Brijesh Singh Aug. 10, 2021, 2:59 p.m. UTC | #2
On 8/10/21 6:33 AM, Borislav Petkov wrote:
> On Wed, Jul 07, 2021 at 01:14:35PM -0500, Brijesh Singh wrote:
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 23929a3010df..e75e29c05f59 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -63,9 +63,17 @@
>>   	(((((u64)reason_set) &  GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \
>>   	((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS))
>>   
>> +/* Error code from reason set 0 */
> 
> ... Error codes...
> 
Noted.

>> +#define SEV_TERM_SET_GEN		0
>>   #define GHCB_SEV_ES_GEN_REQ		0
>>   #define GHCB_SEV_ES_PROT_UNSUPPORTED	1
>>   
>>   #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>>   
>> +/* Linux specific reason codes (used with reason set 1) */
> 
> ... Linux-specific ...

Noted.

> 
>> +#define SEV_TERM_SET_LINUX		1
> 
> GHCB doc says:
> 
> "This document defines and owns reason code set 0x0"
> 
> Should it also say, reason code set 1 is allocated for Linux guest use?
> I don't see why not...
>  > Tom?
> 

If Tom is okay with it then maybe in next version of the GHCB doc can 
add this text.
Tom Lendacky Aug. 10, 2021, 7:30 p.m. UTC | #3
On 8/10/21 9:59 AM, Brijesh Singh wrote:
> On 8/10/21 6:33 AM, Borislav Petkov wrote:
>> On Wed, Jul 07, 2021 at 01:14:35PM -0500, Brijesh Singh wrote:

>>> +#define SEV_TERM_SET_LINUX        1
>>
>> GHCB doc says:
>>
>> "This document defines and owns reason code set 0x0"
>>
>> Should it also say, reason code set 1 is allocated for Linux guest use?
>> I don't see why not...
>>  > Tom?
>>
> 
> If Tom is okay with it then maybe in next version of the GHCB doc can add
> this text.

IIRC, during the review of the first GHCB version there was discussion
about assigning reason sets outside of 0 within the spec and the overall
feeling was to not do that as part of the spec.

We can re-open that discussion for the next version of the GHCB document.

Thanks,
Tom
Borislav Petkov Aug. 10, 2021, 9:52 p.m. UTC | #4
On Tue, Aug 10, 2021 at 02:30:44PM -0500, Tom Lendacky wrote:
> IIRC, during the review of the first GHCB version there was discussion
> about assigning reason sets outside of 0 within the spec and the overall
> feeling was to not do that as part of the spec.
> 
> We can re-open that discussion for the next version of the GHCB document.

My worry is that if nothing documents which sets are allocated to which
vendor, it'll become a mess.

Imagine a Linux SNP guest and a windoze one, both running on a KVM
hypervisor (is that even possible?) and both using the same termination
reason set with conflicting reason numbers.

Unneeded confusion.

Unless the spec says, "reason set 1 is allocated to Linux, set 2 to
windoze, etc"

Then all know which is which.

And so on...

Thx.
diff mbox series

Patch

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 28bcf04c022e..7760959fe96d 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -122,7 +122,7 @@  static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,
 static bool early_setup_sev_es(void)
 {
 	if (!sev_es_negotiate_protocol())
-		sev_es_terminate(GHCB_SEV_ES_PROT_UNSUPPORTED);
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_PROT_UNSUPPORTED);
 
 	if (set_page_decrypted((unsigned long)&boot_ghcb_page))
 		return false;
@@ -175,7 +175,7 @@  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
 	enum es_result result;
 
 	if (!boot_ghcb && !early_setup_sev_es())
-		sev_es_terminate(GHCB_SEV_ES_GEN_REQ);
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 
 	vc_ghcb_invalidate(boot_ghcb);
 	result = vc_init_em_ctxt(&ctxt, regs, exit_code);
@@ -202,5 +202,5 @@  void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
 	if (result == ES_OK)
 		vc_finish_insn(&ctxt);
 	else if (result != ES_RETRY)
-		sev_es_terminate(GHCB_SEV_ES_GEN_REQ);
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 }
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 23929a3010df..e75e29c05f59 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -63,9 +63,17 @@ 
 	(((((u64)reason_set) &  GHCB_MSR_TERM_REASON_SET_MASK) << GHCB_MSR_TERM_REASON_SET_POS) | \
 	((((u64)reason_val) & GHCB_MSR_TERM_REASON_MASK) << GHCB_MSR_TERM_REASON_POS))
 
+/* Error code from reason set 0 */
+#define SEV_TERM_SET_GEN		0
 #define GHCB_SEV_ES_GEN_REQ		0
 #define GHCB_SEV_ES_PROT_UNSUPPORTED	1
 
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
+/* Linux specific reason codes (used with reason set 1) */
+#define SEV_TERM_SET_LINUX		1
+#define GHCB_TERM_REGISTER		0	/* GHCB GPA registration failure */
+#define GHCB_TERM_PSC			1	/* Page State Change failure */
+#define GHCB_TERM_PVALIDATE		2	/* Pvalidate failure */
+
 #endif
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 34821da5f05e..c54be2698df0 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -36,15 +36,12 @@  static bool __init sev_es_check_cpu_features(void)
 	return true;
 }
 
-static void __noreturn sev_es_terminate(unsigned int reason)
+static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
 {
 	u64 val = GHCB_MSR_TERM_REQ;
 
-	/*
-	 * Tell the hypervisor what went wrong - only reason-set 0 is
-	 * currently supported.
-	 */
-	val |= GHCB_SEV_TERM_REASON(0, reason);
+	/* Tell the hypervisor what went wrong. */
+	val |= GHCB_SEV_TERM_REASON(set, reason);
 
 	/* Request Guest Termination from Hypvervisor */
 	sev_es_wr_ghcb_msr(val);
@@ -242,7 +239,7 @@  void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
 
 fail:
 	/* Terminate the guest */
-	sev_es_terminate(GHCB_SEV_ES_GEN_REQ);
+	sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 }
 
 static enum es_result vc_insn_string_read(struct es_em_ctxt *ctxt,
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 540b81ac54c9..0245fe986615 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1432,7 +1432,7 @@  DEFINE_IDTENTRY_VC_KERNEL(exc_vmm_communication)
 		show_regs(regs);
 
 		/* Ask hypervisor to sev_es_terminate */
-		sev_es_terminate(GHCB_SEV_ES_GEN_REQ);
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 
 		/* If that fails and we get here - just panic */
 		panic("Returned from Terminate-Request to Hypervisor\n");
@@ -1480,7 +1480,7 @@  bool __init handle_vc_boot_ghcb(struct pt_regs *regs)
 
 	/* Do initial setup or terminate the guest */
 	if (unlikely(boot_ghcb == NULL && !sev_es_setup_ghcb()))
-		sev_es_terminate(GHCB_SEV_ES_GEN_REQ);
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
 
 	vc_ghcb_invalidate(boot_ghcb);