diff mbox series

[v7,09/45] x86/sev: Save the negotiated GHCB version

Message ID 20211110220731.2396491-10-brijesh.singh@amd.com (mailing list archive)
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand

Commit Message

Brijesh Singh Nov. 10, 2021, 10:06 p.m. UTC 
The SEV-ES guest calls the sev_es_negotiate_protocol() to negotiate the
GHCB protocol version before establishing the GHCB. Cache the negotiated
GHCB version so that it can be used later.

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

Comments

Tianyu Lan Dec. 7, 2021, 12:51 p.m. UTC | #1 
Hi Brijesh:
      We find this patch breaks AMD SEV support in the Hyper-V Isolation
VM. Hyper-V code also uses sev_es_ghcb_hv_call() to read or write msr
value. The sev_es_check_cpu_features() isn't called in the Hyper-V code
and so the ghcb_version is always 0. Could you add a new parameter 
ghcb_version for sev_es_ghcb_hv_call() and then caller may input 
ghcb_version?

Thanks.

On 11/11/2021 6:06 AM, Brijesh Singh wrote:
> The SEV-ES guest calls the sev_es_negotiate_protocol() to negotiate the
> GHCB protocol version before establishing the GHCB. Cache the negotiated
> GHCB version so that it can be used later.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>   arch/x86/include/asm/sev.h   |  2 +-
>   arch/x86/kernel/sev-shared.c | 17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index ec060c433589..9b9c190e8c3b 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -12,7 +12,7 @@
>   #include <asm/insn.h>
>   #include <asm/sev-common.h>
>   
> -#define GHCB_PROTO_OUR		0x0001UL
> +#define GHCB_PROTOCOL_MIN	1ULL
>   #define GHCB_PROTOCOL_MAX	1ULL
>   #define GHCB_DEFAULT_USAGE	0ULL
>   
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 2abf8a7d75e5..91105f5a02a8 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -14,6 +14,15 @@
>   #define has_cpuflag(f)	boot_cpu_has(f)
>   #endif
>   
> +/*
> + * Since feature negotiation related variables are set early in the boot
> + * process they must reside in the .data section so as not to be zeroed
> + * out when the .bss section is later cleared.
> + *
> + * GHCB protocol version negotiated with the hypervisor.
> + */
> +static u16 ghcb_version __ro_after_init;
> +
>   static bool __init sev_es_check_cpu_features(void)
>   {
>   	if (!has_cpuflag(X86_FEATURE_RDRAND)) {
> @@ -51,10 +60,12 @@ static bool sev_es_negotiate_protocol(void)
>   	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
>   		return false;
>   
> -	if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTO_OUR ||
> -	    GHCB_MSR_PROTO_MIN(val) > GHCB_PROTO_OUR)
> +	if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN ||
> +	    GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX)
>   		return false;
>   
> +	ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX);
> +
>   	return true;
>   }
>   
> @@ -127,7 +138,7 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
>   				   u64 exit_info_1, u64 exit_info_2)
>   {
>   	/* Fill in protocol and format specifiers */
> -	ghcb->protocol_version = GHCB_PROTOCOL_MAX;
> +	ghcb->protocol_version = ghcb_version;
>   	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
>   
>   	ghcb_set_sw_exit_code(ghcb, exit_code);
>
Brijesh Singh Dec. 7, 2021, 4:58 p.m. UTC | #2 
On 12/7/21 7:17 AM, Borislav Petkov wrote:
> On Tue, Dec 07, 2021 at 08:51:53PM +0800, Tianyu Lan wrote:
>> Hi Brijesh:
> 
> Do me a favor please and learn not to top-post on a public mailing list.
> Also, take the time to read Documentation/process/ before you send
> upstream patches and how to work with the community in general.
> 
>> We find this patch breaks AMD SEV support in the Hyper-V Isolation
>> VM. Hyper-V code also uses sev_es_ghcb_hv_call() to read or write msr
>> value. The sev_es_check_cpu_features() isn't called in the Hyper-V
>> code and so the ghcb_version is always 0.
> 
> If hyperv is going to expose hypervisor features, then it better report
> GHCB v2. If not, then I guess < 2 or 1 or so, depending on how this is
> defined.
> 
>> Could you add a new parameter ghcb_version for sev_es_ghcb_hv_call()
>> and then caller may input ghcb_version?
> 
> No, your hypervisor needs to adhere to the spec and report proper ghcb
> version. We won't be doing any accomodate-hyperv hacks.
> 


Agreed, the HyperV support need to negotiate the GHCB version before 
making those HC. In the current patch, the sev_es_negotiate_protocol() 
will save the ghcb_version in global variable and the saved value is 
used during the HC. Just make sure that initial HyperV support is 
adhering to the specification.

thanks
diff mbox series

Patch

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index ec060c433589..9b9c190e8c3b 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -12,7 +12,7 @@ 
 #include <asm/insn.h>
 #include <asm/sev-common.h>
 
-#define GHCB_PROTO_OUR		0x0001UL
+#define GHCB_PROTOCOL_MIN	1ULL
 #define GHCB_PROTOCOL_MAX	1ULL
 #define GHCB_DEFAULT_USAGE	0ULL
 
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 2abf8a7d75e5..91105f5a02a8 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -14,6 +14,15 @@ 
 #define has_cpuflag(f)	boot_cpu_has(f)
 #endif
 
+/*
+ * Since feature negotiation related variables are set early in the boot
+ * process they must reside in the .data section so as not to be zeroed
+ * out when the .bss section is later cleared.
+ *
+ * GHCB protocol version negotiated with the hypervisor.
+ */
+static u16 ghcb_version __ro_after_init;
+
 static bool __init sev_es_check_cpu_features(void)
 {
 	if (!has_cpuflag(X86_FEATURE_RDRAND)) {
@@ -51,10 +60,12 @@  static bool sev_es_negotiate_protocol(void)
 	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
 		return false;
 
-	if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTO_OUR ||
-	    GHCB_MSR_PROTO_MIN(val) > GHCB_PROTO_OUR)
+	if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN ||
+	    GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX)
 		return false;
 
+	ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX);
+
 	return true;
 }
 
@@ -127,7 +138,7 @@  enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
 				   u64 exit_info_1, u64 exit_info_2)
 {
 	/* Fill in protocol and format specifiers */
-	ghcb->protocol_version = GHCB_PROTOCOL_MAX;
+	ghcb->protocol_version = ghcb_version;
 	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
 
 	ghcb_set_sw_exit_code(ghcb, exit_code);