Message-ID: <PH0PR21MB302527315F7BE39E0709C5E6D7A59@PH0PR21MB3025.namprd21.prod.outlook.com>
Date:   Tue, 7 Jun 2022 20:30:40 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Tianyu Lan <ltykernel@...il.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>
CC:     Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        vkuznets <vkuznets@...hat.com>,
        "parri.andrea@...il.com" <parri.andrea@...il.com>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>
Subject: RE: [PATCH V2] x86/Hyper-V: Add SEV negotiate protocol support in
 Isolation VM

From: Tianyu Lan <ltykernel@...il.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@...rosoft.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

Powered by blists - more mailing lists