Re: [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in Isolation VM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux