diff mbox series

[v8,22/40] x86/sev: move MSR-based VMGEXITs for CPUID to helper

Message ID 20211210154332.11526-23-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 Dec. 10, 2021, 3:43 p.m. UTC 
From: Michael Roth <michael.roth@amd.com>

This code will also be used later for SEV-SNP-validated CPUID code in
some cases, so move it to a common helper.

Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 26 deletions(-)

Comments

Sean Christopherson Dec. 30, 2021, 6:52 p.m. UTC | #1 
On Fri, Dec 10, 2021, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> This code will also be used later for SEV-SNP-validated CPUID code in
> some cases, so move it to a common helper.
> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 3aaef1a18ffe..d89481b31022 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -194,6 +194,58 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
>  	return verify_exception_info(ghcb, ctxt);
>  }
>  
> +static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,

Having @subfunc, a.k.a. index, in is weird/confusing/fragile because it's not consumed,
nor is it checked.  Peeking ahead, it looks like all future users pass '0'.  Taking the
index but dropping it on the floor is asking for future breakage.  Either drop it or
assert that it's zero.

> +			u32 *ecx, u32 *edx)
> +{
> +	u64 val;
> +
> +	if (eax) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*eax = (val >> 32);
> +	}
> +
> +	if (ebx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*ebx = (val >> 32);
> +	}
> +
> +	if (ecx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*ecx = (val >> 32);
> +	}
> +
> +	if (edx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*edx = (val >> 32);
> +	}

That's a lot of pasta!  If you add

  static int __sev_cpuid_hv(u32 func, int reg_idx, u32 *reg)
  {
	u64 val;

	if (!reg)
		return 0;

	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, reg_idx));
	VMGEXIT();
	val = sev_es_rd_ghcb_msr();
	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
		return -EIO;

	*reg = (val >> 32);
	return 0;
  }

then this helper can become something like:

  static int sev_cpuid_hv(u32 func, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
  {
	int ret;

	ret = __sev_cpuid_hv(func, GHCB_CPUID_REQ_EAX, eax);
	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_EBX, ebx);
	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_ECX, ecx);
	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_EDX, edx);

	return ret;
  }

> +
> +	return 0;
> +}
> +
Borislav Petkov Jan. 4, 2022, 8:57 p.m. UTC | #2 
On Thu, Dec 30, 2021 at 06:52:52PM +0000, Sean Christopherson wrote:
> Having @subfunc, a.k.a. index, in is weird/confusing/fragile because it's not consumed,
> nor is it checked.  Peeking ahead, it looks like all future users pass '0'.  Taking the
> index but dropping it on the floor is asking for future breakage.  Either drop it or
> assert that it's zero.

Yah, just drop it please. 

It can always be added later if needed.

Thx.
Michael Roth Jan. 4, 2022, 11:36 p.m. UTC | #3 
On Thu, Dec 30, 2021 at 06:52:52PM +0000, Sean Christopherson wrote:
> On Fri, Dec 10, 2021, Brijesh Singh wrote:
> > From: Michael Roth <michael.roth@amd.com>
> > 
> > This code will also be used later for SEV-SNP-validated CPUID code in
> > some cases, so move it to a common helper.
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> >  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> > index 3aaef1a18ffe..d89481b31022 100644
> > --- a/arch/x86/kernel/sev-shared.c
> > +++ b/arch/x86/kernel/sev-shared.c
> > @@ -194,6 +194,58 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
> >  	return verify_exception_info(ghcb, ctxt);
> >  }
> >  
> > +static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> 
> Having @subfunc, a.k.a. index, in is weird/confusing/fragile because it's not consumed,
> nor is it checked.  Peeking ahead, it looks like all future users pass '0'.  Taking the
> index but dropping it on the floor is asking for future breakage.  Either drop it or
> assert that it's zero.
> 
> > +			u32 *ecx, u32 *edx)
> > +{
> > +	u64 val;
> > +
> > +	if (eax) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*eax = (val >> 32);
> > +	}
> > +
> > +	if (ebx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*ebx = (val >> 32);
> > +	}
> > +
> > +	if (ecx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*ecx = (val >> 32);
> > +	}
> > +
> > +	if (edx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*edx = (val >> 32);
> > +	}
> 
> That's a lot of pasta!  If you add
> 
>   static int __sev_cpuid_hv(u32 func, int reg_idx, u32 *reg)
>   {
> 	u64 val;
> 
> 	if (!reg)
> 		return 0;
> 
> 	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, reg_idx));
> 	VMGEXIT();
> 	val = sev_es_rd_ghcb_msr();
> 	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> 		return -EIO;
> 
> 	*reg = (val >> 32);
> 	return 0;
>   }
> 
> then this helper can become something like:
> 
>   static int sev_cpuid_hv(u32 func, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
>   {
> 	int ret;
> 
> 	ret = __sev_cpuid_hv(func, GHCB_CPUID_REQ_EAX, eax);
> 	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_EBX, ebx);
> 	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_ECX, ecx);
> 	ret = ret ? : __sev_cpuid_hv(func, GHCB_CPUID_REQ_EDX, edx);
> 
> 	return ret;

Looks good, will make these changes.

-Mike
Venu Busireddy Jan. 6, 2022, 6:38 p.m. UTC | #4 
On 2021-12-10 09:43:14 -0600, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> This code will also be used later for SEV-SNP-validated CPUID code in
> some cases, so move it to a common helper.
> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 3aaef1a18ffe..d89481b31022 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -194,6 +194,58 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
>  	return verify_exception_info(ghcb, ctxt);
>  }
>  
> +static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> +			u32 *ecx, u32 *edx)
> +{
> +	u64 val;
> +
> +	if (eax) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*eax = (val >> 32);
> +	}
> +
> +	if (ebx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*ebx = (val >> 32);
> +	}
> +
> +	if (ecx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*ecx = (val >> 32);
> +	}
> +
> +	if (edx) {
> +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
> +		VMGEXIT();
> +		val = sev_es_rd_ghcb_msr();
> +
> +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +			return -EIO;
> +
> +		*edx = (val >> 32);
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Boot VC Handler - This is the first VC handler during boot, there is no GHCB
>   * page yet, so it only supports the MSR based communication with the
> @@ -202,39 +254,19 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
>  void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>  {
>  	unsigned int fn = lower_bits(regs->ax, 32);
> -	unsigned long val;
> +	u32 eax, ebx, ecx, edx;
>  
>  	/* Only CPUID is supported via MSR protocol */
>  	if (exit_code != SVM_EXIT_CPUID)
>  		goto fail;
>  
> -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
> -	VMGEXIT();
> -	val = sev_es_rd_ghcb_msr();
> -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> +	if (sev_cpuid_hv(fn, 0, &eax, &ebx, &ecx, &edx))
>  		goto fail;
> -	regs->ax = val >> 32;
>  
> -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EBX));
> -	VMGEXIT();
> -	val = sev_es_rd_ghcb_msr();
> -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> -		goto fail;
> -	regs->bx = val >> 32;
> -
> -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_ECX));
> -	VMGEXIT();
> -	val = sev_es_rd_ghcb_msr();
> -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> -		goto fail;
> -	regs->cx = val >> 32;
> -
> -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EDX));
> -	VMGEXIT();
> -	val = sev_es_rd_ghcb_msr();
> -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> -		goto fail;
> -	regs->dx = val >> 32;
> +	regs->ax = eax;
> +	regs->bx = ebx;
> +	regs->cx = ecx;
> +	regs->dx = edx;

What is the intent behind declaring e?x as local variables, instead
of passing the addresses of regs->?x to sev_cpuid_hv()? Is it to
prevent touching any of the regs->?x unless there is no error from
sev_cpuid_hv()? If so, wouldn't it be better to hide this logic from
the callers by declaring the local variables in sev_cpuid_hv() itself,
and moving the four "*e?x = (val >> 32);" statements there to the end
of the function (just before last the return)? With that change, the
callers can safely pass the addresses of regs->?x to do_vc_no_ghcb(),
knowing that the values will only be touched if there is no error?

Venu
Michael Roth Jan. 6, 2022, 8:21 p.m. UTC | #5 
On Thu, Jan 06, 2022 at 12:38:37PM -0600, Venu Busireddy wrote:
> On 2021-12-10 09:43:14 -0600, Brijesh Singh wrote:
> > From: Michael Roth <michael.roth@amd.com>
> > 
> > This code will also be used later for SEV-SNP-validated CPUID code in
> > some cases, so move it to a common helper.
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> >  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> > index 3aaef1a18ffe..d89481b31022 100644
> > --- a/arch/x86/kernel/sev-shared.c
> > +++ b/arch/x86/kernel/sev-shared.c
> > @@ -194,6 +194,58 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
> >  	return verify_exception_info(ghcb, ctxt);
> >  }
> >  
> > +static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> > +			u32 *ecx, u32 *edx)
> > +{
> > +	u64 val;
> > +
> > +	if (eax) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*eax = (val >> 32);
> > +	}
> > +
> > +	if (ebx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*ebx = (val >> 32);
> > +	}
> > +
> > +	if (ecx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*ecx = (val >> 32);
> > +	}
> > +
> > +	if (edx) {
> > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
> > +		VMGEXIT();
> > +		val = sev_es_rd_ghcb_msr();
> > +
> > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +			return -EIO;
> > +
> > +		*edx = (val >> 32);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Boot VC Handler - This is the first VC handler during boot, there is no GHCB
> >   * page yet, so it only supports the MSR based communication with the
> > @@ -202,39 +254,19 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
> >  void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> >  {
> >  	unsigned int fn = lower_bits(regs->ax, 32);
> > -	unsigned long val;
> > +	u32 eax, ebx, ecx, edx;
> >  
> >  	/* Only CPUID is supported via MSR protocol */
> >  	if (exit_code != SVM_EXIT_CPUID)
> >  		goto fail;
> >  
> > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
> > -	VMGEXIT();
> > -	val = sev_es_rd_ghcb_msr();
> > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > +	if (sev_cpuid_hv(fn, 0, &eax, &ebx, &ecx, &edx))
> >  		goto fail;
> > -	regs->ax = val >> 32;
> >  
> > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EBX));
> > -	VMGEXIT();
> > -	val = sev_es_rd_ghcb_msr();
> > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > -		goto fail;
> > -	regs->bx = val >> 32;
> > -
> > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_ECX));
> > -	VMGEXIT();
> > -	val = sev_es_rd_ghcb_msr();
> > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > -		goto fail;
> > -	regs->cx = val >> 32;
> > -
> > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EDX));
> > -	VMGEXIT();
> > -	val = sev_es_rd_ghcb_msr();
> > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > -		goto fail;
> > -	regs->dx = val >> 32;
> > +	regs->ax = eax;
> > +	regs->bx = ebx;
> > +	regs->cx = ecx;
> > +	regs->dx = edx;
> 
> What is the intent behind declaring e?x as local variables, instead
> of passing the addresses of regs->?x to sev_cpuid_hv()? Is it to
> prevent touching any of the regs->?x unless there is no error from
> sev_cpuid_hv()? If so, wouldn't it be better to hide this logic from
> the callers by declaring the local variables in sev_cpuid_hv() itself,
> and moving the four "*e?x = (val >> 32);" statements there to the end
> of the function (just before last the return)? With that change, the
> callers can safely pass the addresses of regs->?x to do_vc_no_ghcb(),
> knowing that the values will only be touched if there is no error?

For me it was more about readability. E?X are well-defined as 32-bit
values, whereas regs->?x are longs. It seemed more readable to me to
have sev_cpuid_hv()/snp_cpuid() expect/return the actual native types,
and leave it up to the caller to cast/shift if necessary.

It also seems more robust for future re-use, since, for instance, if we
ever introduced another callsite that happened to already use u32 locally,
it seems like it would be a mess trying to setup up temp long* args or do
casts to pass them into these functions and then shift/cast them back just
so we could save a few lines at this particular callsite.

> 
> Venu
Venu Busireddy Jan. 6, 2022, 8:36 p.m. UTC | #6 
On 2022-01-06 14:21:35 -0600, Michael Roth wrote:
> On Thu, Jan 06, 2022 at 12:38:37PM -0600, Venu Busireddy wrote:
> > On 2021-12-10 09:43:14 -0600, Brijesh Singh wrote:
> > > From: Michael Roth <michael.roth@amd.com>
> > > 
> > > This code will also be used later for SEV-SNP-validated CPUID code in
> > > some cases, so move it to a common helper.
> > > 
> > > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

Reviewed-by: Venu Busireddy <venu.busireddy@oracle.com>

> > > ---
> > >  arch/x86/kernel/sev-shared.c | 84 +++++++++++++++++++++++++-----------
> > >  1 file changed, 58 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> > > index 3aaef1a18ffe..d89481b31022 100644
> > > --- a/arch/x86/kernel/sev-shared.c
> > > +++ b/arch/x86/kernel/sev-shared.c
> > > @@ -194,6 +194,58 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
> > >  	return verify_exception_info(ghcb, ctxt);
> > >  }
> > >  
> > > +static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
> > > +			u32 *ecx, u32 *edx)
> > > +{
> > > +	u64 val;
> > > +
> > > +	if (eax) {
> > > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
> > > +		VMGEXIT();
> > > +		val = sev_es_rd_ghcb_msr();
> > > +
> > > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > +			return -EIO;
> > > +
> > > +		*eax = (val >> 32);
> > > +	}
> > > +
> > > +	if (ebx) {
> > > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
> > > +		VMGEXIT();
> > > +		val = sev_es_rd_ghcb_msr();
> > > +
> > > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > +			return -EIO;
> > > +
> > > +		*ebx = (val >> 32);
> > > +	}
> > > +
> > > +	if (ecx) {
> > > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
> > > +		VMGEXIT();
> > > +		val = sev_es_rd_ghcb_msr();
> > > +
> > > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > +			return -EIO;
> > > +
> > > +		*ecx = (val >> 32);
> > > +	}
> > > +
> > > +	if (edx) {
> > > +		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
> > > +		VMGEXIT();
> > > +		val = sev_es_rd_ghcb_msr();
> > > +
> > > +		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > +			return -EIO;
> > > +
> > > +		*edx = (val >> 32);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * Boot VC Handler - This is the first VC handler during boot, there is no GHCB
> > >   * page yet, so it only supports the MSR based communication with the
> > > @@ -202,39 +254,19 @@ enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
> > >  void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> > >  {
> > >  	unsigned int fn = lower_bits(regs->ax, 32);
> > > -	unsigned long val;
> > > +	u32 eax, ebx, ecx, edx;
> > >  
> > >  	/* Only CPUID is supported via MSR protocol */
> > >  	if (exit_code != SVM_EXIT_CPUID)
> > >  		goto fail;
> > >  
> > > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
> > > -	VMGEXIT();
> > > -	val = sev_es_rd_ghcb_msr();
> > > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > +	if (sev_cpuid_hv(fn, 0, &eax, &ebx, &ecx, &edx))
> > >  		goto fail;
> > > -	regs->ax = val >> 32;
> > >  
> > > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EBX));
> > > -	VMGEXIT();
> > > -	val = sev_es_rd_ghcb_msr();
> > > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > -		goto fail;
> > > -	regs->bx = val >> 32;
> > > -
> > > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_ECX));
> > > -	VMGEXIT();
> > > -	val = sev_es_rd_ghcb_msr();
> > > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > -		goto fail;
> > > -	regs->cx = val >> 32;
> > > -
> > > -	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EDX));
> > > -	VMGEXIT();
> > > -	val = sev_es_rd_ghcb_msr();
> > > -	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
> > > -		goto fail;
> > > -	regs->dx = val >> 32;
> > > +	regs->ax = eax;
> > > +	regs->bx = ebx;
> > > +	regs->cx = ecx;
> > > +	regs->dx = edx;
> > 
> > What is the intent behind declaring e?x as local variables, instead
> > of passing the addresses of regs->?x to sev_cpuid_hv()? Is it to
> > prevent touching any of the regs->?x unless there is no error from
> > sev_cpuid_hv()? If so, wouldn't it be better to hide this logic from
> > the callers by declaring the local variables in sev_cpuid_hv() itself,
> > and moving the four "*e?x = (val >> 32);" statements there to the end
> > of the function (just before last the return)? With that change, the
> > callers can safely pass the addresses of regs->?x to do_vc_no_ghcb(),
> > knowing that the values will only be touched if there is no error?
> 
> For me it was more about readability. E?X are well-defined as 32-bit
> values, whereas regs->?x are longs. It seemed more readable to me to
> have sev_cpuid_hv()/snp_cpuid() expect/return the actual native types,
> and leave it up to the caller to cast/shift if necessary.
> 
> It also seems more robust for future re-use, since, for instance, if we
> ever introduced another callsite that happened to already use u32 locally,
> it seems like it would be a mess trying to setup up temp long* args or do
> casts to pass them into these functions and then shift/cast them back just
> so we could save a few lines at this particular callsite.

Got it.

Venu
diff mbox series

Patch

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 3aaef1a18ffe..d89481b31022 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -194,6 +194,58 @@  enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
 	return verify_exception_info(ghcb, ctxt);
 }
 
+static int sev_cpuid_hv(u32 func, u32 subfunc, u32 *eax, u32 *ebx,
+			u32 *ecx, u32 *edx)
+{
+	u64 val;
+
+	if (eax) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EAX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*eax = (val >> 32);
+	}
+
+	if (ebx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EBX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*ebx = (val >> 32);
+	}
+
+	if (ecx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_ECX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*ecx = (val >> 32);
+	}
+
+	if (edx) {
+		sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(func, GHCB_CPUID_REQ_EDX));
+		VMGEXIT();
+		val = sev_es_rd_ghcb_msr();
+
+		if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+			return -EIO;
+
+		*edx = (val >> 32);
+	}
+
+	return 0;
+}
+
 /*
  * Boot VC Handler - This is the first VC handler during boot, there is no GHCB
  * page yet, so it only supports the MSR based communication with the
@@ -202,39 +254,19 @@  enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr,
 void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
 {
 	unsigned int fn = lower_bits(regs->ax, 32);
-	unsigned long val;
+	u32 eax, ebx, ecx, edx;
 
 	/* Only CPUID is supported via MSR protocol */
 	if (exit_code != SVM_EXIT_CPUID)
 		goto fail;
 
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
+	if (sev_cpuid_hv(fn, 0, &eax, &ebx, &ecx, &edx))
 		goto fail;
-	regs->ax = val >> 32;
 
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EBX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->bx = val >> 32;
-
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_ECX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->cx = val >> 32;
-
-	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EDX));
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-	if (GHCB_RESP_CODE(val) != GHCB_MSR_CPUID_RESP)
-		goto fail;
-	regs->dx = val >> 32;
+	regs->ax = eax;
+	regs->bx = ebx;
+	regs->cx = ecx;
+	regs->dx = edx;
 
 	/*
 	 * This is a VC handler and the #VC is only raised when SEV-ES is