linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM
@ 2022-06-02 11:05 Tianyu Lan
  2022-06-07 20:30 ` Michael Kelley (LINUX)
  0 siblings, 1 reply; 3+ messages in thread
From: Tianyu Lan @ 2022-06-02 11:05 UTC (permalink / raw)
  To: kys, haiyangz, sthemmin, wei.liu, decui, tglx, mingo, bp,
	dave.hansen, x86, hpa, michael.h.kelley
  Cc: Tianyu Lan, linux-hyperv, linux-kernel, vkuznets, parri.andrea,
	thomas.lendacky

From: Tianyu Lan <Tianyu.Lan@microsoft.com>

Hyper-V Isolation VM current code uses sev_es_ghcb_hv_call()
to read/write MSR via GHCB page and depends on the sev code.
This may cause regression when sev code changes interface
design.

The latest SEV-ES code requires to negotiate GHCB version before
reading/writing MSR via GHCB page and sev_es_ghcb_hv_call() doesn't
work for Hyper-V Isolation VM. Add Hyper-V ghcb related implementation
to decouple SEV and Hyper-V code. Negotiate GHCB version in the
hyperv_init_ghcb() and use the version to communicate with Hyper-V
in the ghcb hv call function.

Fixes: 2ea29c5abbc2 ("x86/sev: Save the negotiated GHCB version")
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
 arch/x86/hyperv/hv_init.c       |  6 +++
 arch/x86/hyperv/ivm.c           | 88 ++++++++++++++++++++++++++++++---
 arch/x86/include/asm/mshyperv.h |  4 ++
 3 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 8b392b6b7b93..40b6874accdb 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -29,6 +29,7 @@
 #include <clocksource/hyperv_timer.h>
 #include <linux/highmem.h>
 #include <linux/swiotlb.h>
+#include <asm/sev.h>
 
 int hyperv_init_cpuhp;
 u64 hv_current_partition_id = ~0ull;
@@ -70,6 +71,11 @@ static int hyperv_init_ghcb(void)
 	ghcb_base = (void **)this_cpu_ptr(hv_ghcb_pg);
 	*ghcb_base = ghcb_va;
 
+	/* Negotiate GHCB Version. */
+	if (!hv_ghcb_negotiate_protocol())
+		hv_ghcb_terminate(SEV_TERM_SET_GEN,
+				  GHCB_SEV_ES_PROT_UNSUPPORTED);
+
 	return 0;
 }
 
diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 2b994117581e..4b67c4d7c4f5 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -53,6 +53,8 @@ union hv_ghcb {
 	} hypercall;
 } __packed __aligned(HV_HYP_PAGE_SIZE);
 
+static u16 ghcb_version __ro_after_init;
+
 u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size)
 {
 	union hv_ghcb *hv_ghcb;
@@ -96,12 +98,89 @@ u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size)
 	return status;
 }
 
+static inline u64 rd_ghcb_msr(void)
+{
+	return __rdmsr(MSR_AMD64_SEV_ES_GHCB);
+}
+
+static inline void wr_ghcb_msr(u64 val)
+{
+	u32 low, high;
+
+	low  = (u32)(val);
+	high = (u32)(val >> 32);
+
+	native_wrmsr(MSR_AMD64_SEV_ES_GHCB, low, high);
+}
+
+static enum es_result ghcb_hv_call(struct ghcb *ghcb, u64 exit_code,
+				   u64 exit_info_1, u64 exit_info_2)
+{
+	/* Fill in protocol and format specifiers */
+	ghcb->protocol_version = ghcb_version;
+	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
+
+	ghcb_set_sw_exit_code(ghcb, exit_code);
+	ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
+	ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
+
+	VMGEXIT();
+
+	if (ghcb->save.sw_exit_info_1 & GENMASK_ULL(31, 0))
+		return ES_VMM_ERROR;
+	else
+		return ES_OK;
+}
+
+void hv_ghcb_terminate(unsigned int set, unsigned int reason)
+{
+	u64 val = GHCB_MSR_TERM_REQ;
+
+	/* Tell the hypervisor what went wrong. */
+	val |= GHCB_SEV_TERM_REASON(set, reason);
+
+	/* Request Guest Termination from Hypvervisor */
+	wr_ghcb_msr(val);
+	VMGEXIT();
+
+	while (true)
+		asm volatile("hlt\n" : : : "memory");
+}
+
+bool hv_ghcb_negotiate_protocol(void)
+{
+	u64 ghcb_gpa;
+	u64 val;
+
+	/* Save ghcb page gpa. */
+	ghcb_gpa = rd_ghcb_msr();
+
+	/* Do the GHCB protocol version negotiation */
+	wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
+	VMGEXIT();
+	val = rd_ghcb_msr();
+
+	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
+		return false;
+
+	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);
+
+	/* Write ghcb page back after negotiating protocol. */
+	wr_ghcb_msr(ghcb_gpa);
+	VMGEXIT();
+
+	return true;
+}
+
 void hv_ghcb_msr_write(u64 msr, u64 value)
 {
 	union hv_ghcb *hv_ghcb;
 	void **ghcb_base;
 	unsigned long flags;
-	struct es_em_ctxt ctxt;
 
 	if (!hv_ghcb_pg)
 		return;
@@ -120,8 +199,7 @@ void hv_ghcb_msr_write(u64 msr, u64 value)
 	ghcb_set_rax(&hv_ghcb->ghcb, lower_32_bits(value));
 	ghcb_set_rdx(&hv_ghcb->ghcb, upper_32_bits(value));
 
-	if (sev_es_ghcb_hv_call(&hv_ghcb->ghcb, false, &ctxt,
-				SVM_EXIT_MSR, 1, 0))
+	if (ghcb_hv_call(&hv_ghcb->ghcb, SVM_EXIT_MSR, 1, 0))
 		pr_warn("Fail to write msr via ghcb %llx.\n", msr);
 
 	local_irq_restore(flags);
@@ -133,7 +211,6 @@ void hv_ghcb_msr_read(u64 msr, u64 *value)
 	union hv_ghcb *hv_ghcb;
 	void **ghcb_base;
 	unsigned long flags;
-	struct es_em_ctxt ctxt;
 
 	/* Check size of union hv_ghcb here. */
 	BUILD_BUG_ON(sizeof(union hv_ghcb) != HV_HYP_PAGE_SIZE);
@@ -152,8 +229,7 @@ void hv_ghcb_msr_read(u64 msr, u64 *value)
 	}
 
 	ghcb_set_rcx(&hv_ghcb->ghcb, msr);
-	if (sev_es_ghcb_hv_call(&hv_ghcb->ghcb, false, &ctxt,
-				SVM_EXIT_MSR, 0, 0))
+	if (ghcb_hv_call(&hv_ghcb->ghcb, SVM_EXIT_MSR, 0, 0))
 		pr_warn("Fail to read msr via ghcb %llx.\n", msr);
 	else
 		*value = (u64)lower_32_bits(hv_ghcb->ghcb.save.rax)
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index a82f603d4312..61f0c206bff0 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -179,9 +179,13 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible);
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 void hv_ghcb_msr_write(u64 msr, u64 value);
 void hv_ghcb_msr_read(u64 msr, u64 *value);
+bool hv_ghcb_negotiate_protocol(void);
+void hv_ghcb_terminate(unsigned int set, unsigned int reason);
 #else
 static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
 static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
+static inline bool hv_ghcb_negotiate_protocol(void) { return false; }
+static inline void hv_ghcb_terminate(unsigned int set, unsigned int reason) {}
 #endif
 
 extern bool hv_isolation_type_snp(void);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* RE: [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM
  2022-06-02 11:05 [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM Tianyu Lan
@ 2022-06-07 20:30 ` Michael Kelley (LINUX)
  2022-06-09  7:55   ` Tianyu Lan
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Kelley (LINUX) @ 2022-06-07 20:30 UTC (permalink / raw)
  To: Tianyu Lan, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	wei.liu@kernel.org, Dexuan Cui, tglx@linutronix.de,
	mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
	x86@kernel.org, hpa@zytor.com
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, vkuznets, parri.andrea@gmail.com,
	thomas.lendacky@amd.com

From: Tianyu Lan <ltykernel@gmail.com> Sent: Thursday, June 2, 2022 4:05 AM
> 
> Hyper-V Isolation VM current code uses sev_es_ghcb_hv_call()
> to read/write MSR via GHCB page and depends on the sev code.
> This may cause regression when sev code changes interface
> design.
> 
> The latest SEV-ES code requires to negotiate GHCB version before
> reading/writing MSR via GHCB page and sev_es_ghcb_hv_call() doesn't
> work for Hyper-V Isolation VM. Add Hyper-V ghcb related implementation
> to decouple SEV and Hyper-V code. Negotiate GHCB version in the
> hyperv_init_ghcb() and use the version to communicate with Hyper-V
> in the ghcb hv call function.
> 
> Fixes: 2ea29c5abbc2 ("x86/sev: Save the negotiated GHCB version")
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c       |  6 +++
>  arch/x86/hyperv/ivm.c           | 88 ++++++++++++++++++++++++++++++---
>  arch/x86/include/asm/mshyperv.h |  4 ++
>  3 files changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 8b392b6b7b93..40b6874accdb 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -29,6 +29,7 @@
>  #include <clocksource/hyperv_timer.h>
>  #include <linux/highmem.h>
>  #include <linux/swiotlb.h>
> +#include <asm/sev.h>
> 
>  int hyperv_init_cpuhp;
>  u64 hv_current_partition_id = ~0ull;
> @@ -70,6 +71,11 @@ static int hyperv_init_ghcb(void)
>  	ghcb_base = (void **)this_cpu_ptr(hv_ghcb_pg);
>  	*ghcb_base = ghcb_va;
> 
> +	/* Negotiate GHCB Version. */
> +	if (!hv_ghcb_negotiate_protocol())
> +		hv_ghcb_terminate(SEV_TERM_SET_GEN,
> +				  GHCB_SEV_ES_PROT_UNSUPPORTED);
> +

Negotiating the protocol here is unexpected for me.  The
function hyperv_init_ghcb() is called for each CPU as it
initializes, so the static variable ghcb_version will be set
multiple times.  I can see that setup_ghbc(), which this is
patterned after, is also called for each CPU during the early
CPU initialization, which is also a bit weird.  I see two
problems:

1) hv_ghcb_negotiate_protocol() could get called in parallel
on two different CPUs at the same time, and the Hyper-V
version modifies global state (the MSR_AMD64_SEV_ES_GHCB
MSR).  I'm not sure if the sev_es version has the same
problem.

2) The Hyper-V version would get called when taking a CPU
from on offline state to an online state.  I'm not sure if taking
CPUs online and offline is allowed in an SNP isolated VM, but
if it is, then ghcb_version could be modified well after Linux
initialization, violating the __ro_after_init qualifier on the
variable.

Net, it seems like we should find a way to negotiate the
GHCB version only once at boot time.

>  	return 0;
>  }
> 
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 2b994117581e..4b67c4d7c4f5 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -53,6 +53,8 @@ union hv_ghcb {
>  	} hypercall;
>  } __packed __aligned(HV_HYP_PAGE_SIZE);
> 
> +static u16 ghcb_version __ro_after_init;
> +

This is same name as the equivalent sev_es variable.  Could this one
be changed to hv_ghcb_version to avoid any confusion?

>  u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size)
>  {
>  	union hv_ghcb *hv_ghcb;
> @@ -96,12 +98,89 @@ u64 hv_ghcb_hypercall(u64 control, void *input, void *output,
> u32 input_size)
>  	return status;
>  }
> 
> +static inline u64 rd_ghcb_msr(void)
> +{
> +	return __rdmsr(MSR_AMD64_SEV_ES_GHCB);
> +}
> +
> +static inline void wr_ghcb_msr(u64 val)
> +{
> +	u32 low, high;
> +
> +	low  = (u32)(val);
> +	high = (u32)(val >> 32);
> +
> +	native_wrmsr(MSR_AMD64_SEV_ES_GHCB, low, high);

This whole function could be coded as just

	native_wrmsrl(MSR_AMD64_SEV_ES_GHCB, val);

since the "l" version handles breaking the 64-bit argument
into two 32-bit arguments.

> +}
> +
> +static enum es_result ghcb_hv_call(struct ghcb *ghcb, u64 exit_code,
> +				   u64 exit_info_1, u64 exit_info_2)

Seems like the function name here should be hv_ghcb_hv_call.

> +{
> +	/* Fill in protocol and format specifiers */
> +	ghcb->protocol_version = ghcb_version;
> +	ghcb->ghcb_usage       = GHCB_DEFAULT_USAGE;
> +
> +	ghcb_set_sw_exit_code(ghcb, exit_code);
> +	ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
> +	ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
> +
> +	VMGEXIT();
> +
> +	if (ghcb->save.sw_exit_info_1 & GENMASK_ULL(31, 0))
> +		return ES_VMM_ERROR;
> +	else
> +		return ES_OK;
> +}
> +
> +void hv_ghcb_terminate(unsigned int set, unsigned int reason)
> +{
> +	u64 val = GHCB_MSR_TERM_REQ;
> +
> +	/* Tell the hypervisor what went wrong. */
> +	val |= GHCB_SEV_TERM_REASON(set, reason);
> +
> +	/* Request Guest Termination from Hypvervisor */
> +	wr_ghcb_msr(val);
> +	VMGEXIT();
> +
> +	while (true)
> +		asm volatile("hlt\n" : : : "memory");
> +}
> +
> +bool hv_ghcb_negotiate_protocol(void)
> +{
> +	u64 ghcb_gpa;
> +	u64 val;
> +
> +	/* Save ghcb page gpa. */
> +	ghcb_gpa = rd_ghcb_msr();
> +
> +	/* Do the GHCB protocol version negotiation */
> +	wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
> +	VMGEXIT();
> +	val = rd_ghcb_msr();
> +
> +	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
> +		return false;
> +
> +	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);
> +
> +	/* Write ghcb page back after negotiating protocol. */
> +	wr_ghcb_msr(ghcb_gpa);
> +	VMGEXIT();
> +
> +	return true;
> +}
> +
>  void hv_ghcb_msr_write(u64 msr, u64 value)
>  {
>  	union hv_ghcb *hv_ghcb;
>  	void **ghcb_base;
>  	unsigned long flags;
> -	struct es_em_ctxt ctxt;
> 
>  	if (!hv_ghcb_pg)
>  		return;
> @@ -120,8 +199,7 @@ void hv_ghcb_msr_write(u64 msr, u64 value)
>  	ghcb_set_rax(&hv_ghcb->ghcb, lower_32_bits(value));
>  	ghcb_set_rdx(&hv_ghcb->ghcb, upper_32_bits(value));
> 
> -	if (sev_es_ghcb_hv_call(&hv_ghcb->ghcb, false, &ctxt,
> -				SVM_EXIT_MSR, 1, 0))
> +	if (ghcb_hv_call(&hv_ghcb->ghcb, SVM_EXIT_MSR, 1, 0))
>  		pr_warn("Fail to write msr via ghcb %llx.\n", msr);
> 
>  	local_irq_restore(flags);
> @@ -133,7 +211,6 @@ void hv_ghcb_msr_read(u64 msr, u64 *value)
>  	union hv_ghcb *hv_ghcb;
>  	void **ghcb_base;
>  	unsigned long flags;
> -	struct es_em_ctxt ctxt;
> 
>  	/* Check size of union hv_ghcb here. */
>  	BUILD_BUG_ON(sizeof(union hv_ghcb) != HV_HYP_PAGE_SIZE);
> @@ -152,8 +229,7 @@ void hv_ghcb_msr_read(u64 msr, u64 *value)
>  	}
> 
>  	ghcb_set_rcx(&hv_ghcb->ghcb, msr);
> -	if (sev_es_ghcb_hv_call(&hv_ghcb->ghcb, false, &ctxt,
> -				SVM_EXIT_MSR, 0, 0))
> +	if (ghcb_hv_call(&hv_ghcb->ghcb, SVM_EXIT_MSR, 0, 0))
>  		pr_warn("Fail to read msr via ghcb %llx.\n", msr);
>  	else
>  		*value = (u64)lower_32_bits(hv_ghcb->ghcb.save.rax)

Since these changes remove the two cases where sev_es_ghcb_hv_call()
is invoked with the 2nd argument as "false", it seems like there should be
a follow-on patch to remove that argument and Hyper-V specific hack
from sev_es_ghcb_hv_call().

Michael


> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index a82f603d4312..61f0c206bff0 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -179,9 +179,13 @@ int hv_set_mem_host_visibility(unsigned long addr, int
> numpages, bool visible);
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  void hv_ghcb_msr_write(u64 msr, u64 value);
>  void hv_ghcb_msr_read(u64 msr, u64 *value);
> +bool hv_ghcb_negotiate_protocol(void);
> +void hv_ghcb_terminate(unsigned int set, unsigned int reason);
>  #else
>  static inline void hv_ghcb_msr_write(u64 msr, u64 value) {}
>  static inline void hv_ghcb_msr_read(u64 msr, u64 *value) {}
> +static inline bool hv_ghcb_negotiate_protocol(void) { return false; }
> +static inline void hv_ghcb_terminate(unsigned int set, unsigned int reason) {}
>  #endif
> 
>  extern bool hv_isolation_type_snp(void);
> --
> 2.25.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM
  2022-06-07 20:30 ` Michael Kelley (LINUX)
@ 2022-06-09  7:55   ` Tianyu Lan
  0 siblings, 0 replies; 3+ messages in thread
From: Tianyu Lan @ 2022-06-09  7:55 UTC (permalink / raw)
  To: Michael Kelley (LINUX), KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, wei.liu@kernel.org, Dexuan Cui,
	tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com
  Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org, vkuznets, parri.andrea@gmail.com,
	thomas.lendacky@amd.com

Hi Michael:
	Thanks for your review.

On 6/8/2022 4:30 AM, Michael Kelley (LINUX) wrote:
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index 8b392b6b7b93..40b6874accdb 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -29,6 +29,7 @@
>>   #include <clocksource/hyperv_timer.h>
>>   #include <linux/highmem.h>
>>   #include <linux/swiotlb.h>
>> +#include <asm/sev.h>
>>
>>   int hyperv_init_cpuhp;
>>   u64 hv_current_partition_id = ~0ull;
>> @@ -70,6 +71,11 @@ static int hyperv_init_ghcb(void)
>>   	ghcb_base = (void **)this_cpu_ptr(hv_ghcb_pg);
>>   	*ghcb_base = ghcb_va;
>>
>> +	/* Negotiate GHCB Version. */
>> +	if (!hv_ghcb_negotiate_protocol())
>> +		hv_ghcb_terminate(SEV_TERM_SET_GEN,
>> +				  GHCB_SEV_ES_PROT_UNSUPPORTED);
>> +
> Negotiating the protocol here is unexpected for me.  The
> function hyperv_init_ghcb() is called for each CPU as it
> initializes, so the static variable ghcb_version will be set
> multiple times.  I can see that setup_ghbc(), which this is
> patterned after, is also called for each CPU during the early
> CPU initialization, which is also a bit weird.  I see two
> problems:
> 
> 1) hv_ghcb_negotiate_protocol() could get called in parallel
> on two different CPUs at the same time, and the Hyper-V
> version modifies global state (the MSR_AMD64_SEV_ES_GHCB
> MSR).  I'm not sure if the sev_es version has the same
> problem.
> 
> 2) The Hyper-V version would get called when taking a CPU
> from on offline state to an online state.  I'm not sure if taking
> CPUs online and offline is allowed in an SNP isolated VM, but
> if it is, then ghcb_version could be modified well after Linux
> initialization, violating the __ro_after_init qualifier on the
> variable.
> 
> Net, it seems like we should find a way to negotiate the
> GHCB version only once at boot time.

Yes, this makes sense and will update.
> 
>> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
>> index 2b994117581e..4b67c4d7c4f5 100644
>> --- a/arch/x86/hyperv/ivm.c
>> +++ b/arch/x86/hyperv/ivm.c
>> @@ -53,6 +53,8 @@ union hv_ghcb {
>>   	} hypercall;
>>   } __packed __aligned(HV_HYP_PAGE_SIZE);
>>
>> +static u16 ghcb_version __ro_after_init;
>> +
> This is same name as the equivalent sev_es variable.  Could this one
> be changed to hv_ghcb_version to avoid any confusion?
> 
>> +static inline void wr_ghcb_msr(u64 val)
>> +{
>> +	u32 low, high;
>> +
>> +	low  = (u32)(val);
>> +	high = (u32)(val >> 32);
>> +
>> +	native_wrmsr(MSR_AMD64_SEV_ES_GHCB, low, high);
> This whole function could be coded as just
> 
> 	native_wrmsrl(MSR_AMD64_SEV_ES_GHCB, val);
> 
> since the "l" version handles breaking the 64-bit argument
> into two 32-bit arguments.

This follows SEV ES code and will update.

> 
>> +}
>> +
>> +static enum es_result ghcb_hv_call(struct ghcb *ghcb, u64 exit_code,
>> +				   u64 exit_info_1, u64 exit_info_2)
> Seems like the function name here should be hv_ghcb_hv_call.
> 
>> @@ -152,8 +229,7 @@ void hv_ghcb_msr_read(u64 msr, u64 *value)
>>   	}
>>
>>   	ghcb_set_rcx(&hv_ghcb->ghcb, msr);
>> -	if (sev_es_ghcb_hv_call(&hv_ghcb->ghcb, false, &ctxt,
>> -				SVM_EXIT_MSR, 0, 0))
>> +	if (ghcb_hv_call(&hv_ghcb->ghcb, SVM_EXIT_MSR, 0, 0))
>>   		pr_warn("Fail to read msr via ghcb %llx.\n", msr);
>>   	else
>>   		*value = (u64)lower_32_bits(hv_ghcb->ghcb.save.rax)
> Since these changes remove the two cases where sev_es_ghcb_hv_call()
> is invoked with the 2nd argument as "false", it seems like there should be
> a follow-on patch to remove that argument and Hyper-V specific hack
> from sev_es_ghcb_hv_call().

OK. Will update.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-06-09  7:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-02 11:05 [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM Tianyu Lan
2022-06-07 20:30 ` Michael Kelley (LINUX)
2022-06-09  7:55   ` Tianyu Lan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).