diff mbox series

[V3,5/9] x86/hyperv: Use vmmcall to implement Hyper-V hypercall in sev-snp enlightened guest

Message ID 20230718032304.136888-6-ltykernel@gmail.com
State New
Headers
Series x86/hyperv: Add AMD sev-snp enlightened guest support on hyperv |

Commit Message

Tianyu Lan July 18, 2023, 3:22 a.m. UTC 
  From: Tianyu Lan <tiala@microsoft.com>

In sev-snp enlightened guest, Hyper-V hypercall needs
to use vmmcall to trigger vmexit and notify hypervisor
to handle hypercall request.

Signed-off-by: Tianyu Lan <tiala@microsoft.com>
---
 arch/x86/include/asm/mshyperv.h | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

Comments

Michael Kelley (LINUX) July 26, 2023, 3:44 a.m. UTC | #1 
From: Tianyu Lan <ltykernel@gmail.com> Sent: Monday, July 17, 2023 8:23 PM
> 
> In sev-snp enlightened guest, Hyper-V hypercall needs
> to use vmmcall to trigger vmexit and notify hypervisor
> to handle hypercall request.
> 
> Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> ---
>  arch/x86/include/asm/mshyperv.h | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 2fa38e9f6207..025eda129d99 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -64,12 +64,12 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
>  	if (!hv_hypercall_pg)
>  		return U64_MAX;
> 
> -	__asm__ __volatile__("mov %4, %%r8\n"
> -			     CALL_NOSPEC
> +	__asm__ __volatile__("mov %[output], %%r8\n"
> +			     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)

Since this code is for SEV-SNP, what's the thinking behind using
X86_FEATURE_SEV_ES in the ALTERNATIVE statements?   Don't you need
to use X86_FEATURE_SEV_SNP (which is being added in another patch set that
Boris Petkov pointed out).

Also, does this patch depend on Peter Zijlstra's patch to support nested
ALTERNATIVE statements?  If so, that needs to be called out, probably in
the cover letter.  Peter's patch doesn't yet appear in linux-next.

Michael

>  			     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
> -			       "+c" (control), "+d" (input_address)
> -			     :  "r" (output_address),
> -				THUNK_TARGET(hv_hypercall_pg)
> +			     "+c" (control), "+d" (input_address)
> +			     : [output] "r" (output_address),
> +			       THUNK_TARGET(hv_hypercall_pg)
>  			     : "cc", "memory", "r8", "r9", "r10", "r11");
>  #else
>  	u32 input_address_hi = upper_32_bits(input_address);
> @@ -105,7 +105,8 @@ static inline u64 _hv_do_fast_hypercall8(u64 control, u64 input1)
> 
>  #ifdef CONFIG_X86_64
>  	{
> -		__asm__ __volatile__(CALL_NOSPEC
> +		__asm__ __volatile__("mov %[thunk_target], %%r8\n"
> +				     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
>  				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
>  				       "+c" (control), "+d" (input1)
>  				     : THUNK_TARGET(hv_hypercall_pg)
> @@ -150,13 +151,13 @@ static inline u64 _hv_do_fast_hypercall16(u64 control, u64 input1, u64 input2)
> 
>  #ifdef CONFIG_X86_64
>  	{
> -		__asm__ __volatile__("mov %4, %%r8\n"
> -				     CALL_NOSPEC
> -				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
> -				       "+c" (control), "+d" (input1)
> -				     : "r" (input2),
> -				       THUNK_TARGET(hv_hypercall_pg)
> -				     : "cc", "r8", "r9", "r10", "r11");
> +		__asm__ __volatile__("mov %[output], %%r8\n"
> +		     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
> +		     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
> +		       "+c" (control), "+d" (input1)
> +		     : [output] "r" (input2),
> +		       THUNK_TARGET(hv_hypercall_pg)
> +		     : "cc", "r8", "r9", "r10", "r11");
>  	}
>  #else
>  	{
> --
> 2.25.1
Tianyu Lan July 26, 2023, 1:47 p.m. UTC | #2 
On 7/26/2023 11:44 AM, Michael Kelley (LINUX) wrote:
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 2fa38e9f6207..025eda129d99 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -64,12 +64,12 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
>>   	if (!hv_hypercall_pg)
>>   		return U64_MAX;
>>
>> -	__asm__ __volatile__("mov %4, %%r8\n"
>> -			     CALL_NOSPEC
>> +	__asm__ __volatile__("mov %[output], %%r8\n"
>> +			     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
> Since this code is for SEV-SNP, what's the thinking behind using
> X86_FEATURE_SEV_ES in the ALTERNATIVE statements?   Don't you need
> to use X86_FEATURE_SEV_SNP (which is being added in another patch set that
> Boris Petkov pointed out).

Hi Michael:
	Thanks for your review. The patch mentioned by Boris has not been 
merged and so still use X86_FEATURE_SEV_ES here. We may replace the 
feature flag with X86_FEATURE_SEV_SNP after it's upstreamed.

> 
> Also, does this patch depend on Peter Zijlstra's patch to support nested
> ALTERNATIVE statements?  If so, that needs to be called out, probably in
> the cover letter.  Peter's patch doesn't yet appear in linux-next.
> 

It may work without Peterz's patch. Please see 
https://lkml.org/lkml/2023/6/27/520.
Peterz's patch optimizes ALTERNATIVE_n implementation with nested 
expression.
Michael Kelley (LINUX) July 26, 2023, 2:29 p.m. UTC | #3 
From: Tianyu Lan <ltykernel@gmail.com> Sent: Wednesday, July 26, 2023 6:47 AM
> 
> On 7/26/2023 11:44 AM, Michael Kelley (LINUX) wrote:
> >> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> >> index 2fa38e9f6207..025eda129d99 100644
> >> --- a/arch/x86/include/asm/mshyperv.h
> >> +++ b/arch/x86/include/asm/mshyperv.h
> >> @@ -64,12 +64,12 @@ static inline u64 hv_do_hypercall(u64 control, void *input,
> void *output)
> >>   	if (!hv_hypercall_pg)
> >>   		return U64_MAX;
> >>
> >> -	__asm__ __volatile__("mov %4, %%r8\n"
> >> -			     CALL_NOSPEC
> >> +	__asm__ __volatile__("mov %[output], %%r8\n"
> >> +			     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
> > Since this code is for SEV-SNP, what's the thinking behind using
> > X86_FEATURE_SEV_ES in the ALTERNATIVE statements?   Don't you need
> > to use X86_FEATURE_SEV_SNP (which is being added in another patch set that
> > Boris Petkov pointed out).
> 
> Hi Michael:
> 	Thanks for your review. The patch mentioned by Boris has not been
> merged and so still use X86_FEATURE_SEV_ES here. We may replace the
> feature flag with X86_FEATURE_SEV_SNP after it's upstreamed.
> 

Just so I'm clear, is it true that in an SEV-SNP VM, the CPUID flags for
SEV-ES *and* SEV-SNP are set?  That would seem to be necessary for
your approach to work.

I wonder if it would be better to take the patch from Brijesh Singh
that adds X86_FEATURE_SEV_SNP and add it to your patch set (with
Brijesh's agreement, of course).  That patch is small and straightforward.

> >
> > Also, does this patch depend on Peter Zijlstra's patch to support nested
> > ALTERNATIVE statements?  If so, that needs to be called out, probably in
> > the cover letter.  Peter's patch doesn't yet appear in linux-next.
> >
> 
> It may work without Peterz's patch. Please see
> https://lkml.org/lkml/2023/6/27/520
> Peterz's patch optimizes ALTERNATIVE_n implementation with nested
> expression.

OK, good.

Michael
Tianyu Lan July 28, 2023, 10:45 a.m. UTC | #4 
On 7/26/2023 10:29 PM, Michael Kelley (LINUX) wrote:
>> Hi Michael:
>> 	Thanks for your review. The patch mentioned by Boris has not been
>> merged and so still use X86_FEATURE_SEV_ES here. We may replace the
>> feature flag with X86_FEATURE_SEV_SNP after it's upstreamed.
>>
> Just so I'm clear, is it true that in an SEV-SNP VM, the CPUID flags for
> SEV-ES*and*  SEV-SNP are set?  That would seem to be necessary for
> your approach to work.

Yes, SEV and SEV-ES flags are set in the SEV-SNP guest and they are 
necessary.

> 
> I wonder if it would be better to take the patch from Brijesh Singh
> that adds X86_FEATURE_SEV_SNP and add it to your patch set (with
> Brijesh's agreement, of course).  That patch is small and straightforward.
>

I will sync with Brijesh. Thanks for suggestion.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 2fa38e9f6207..025eda129d99 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -64,12 +64,12 @@  static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
 	if (!hv_hypercall_pg)
 		return U64_MAX;
 
-	__asm__ __volatile__("mov %4, %%r8\n"
-			     CALL_NOSPEC
+	__asm__ __volatile__("mov %[output], %%r8\n"
+			     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
 			     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-			       "+c" (control), "+d" (input_address)
-			     :  "r" (output_address),
-				THUNK_TARGET(hv_hypercall_pg)
+			     "+c" (control), "+d" (input_address)
+			     : [output] "r" (output_address),
+			       THUNK_TARGET(hv_hypercall_pg)
 			     : "cc", "memory", "r8", "r9", "r10", "r11");
 #else
 	u32 input_address_hi = upper_32_bits(input_address);
@@ -105,7 +105,8 @@  static inline u64 _hv_do_fast_hypercall8(u64 control, u64 input1)
 
 #ifdef CONFIG_X86_64
 	{
-		__asm__ __volatile__(CALL_NOSPEC
+		__asm__ __volatile__("mov %[thunk_target], %%r8\n"
+				     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
 				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
 				       "+c" (control), "+d" (input1)
 				     : THUNK_TARGET(hv_hypercall_pg)
@@ -150,13 +151,13 @@  static inline u64 _hv_do_fast_hypercall16(u64 control, u64 input1, u64 input2)
 
 #ifdef CONFIG_X86_64
 	{
-		__asm__ __volatile__("mov %4, %%r8\n"
-				     CALL_NOSPEC
-				     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
-				       "+c" (control), "+d" (input1)
-				     : "r" (input2),
-				       THUNK_TARGET(hv_hypercall_pg)
-				     : "cc", "r8", "r9", "r10", "r11");
+		__asm__ __volatile__("mov %[output], %%r8\n"
+		     ALTERNATIVE("vmmcall", CALL_NOSPEC, X86_FEATURE_SEV_ES)
+		     : "=a" (hv_status), ASM_CALL_CONSTRAINT,
+		       "+c" (control), "+d" (input1)
+		     : [output] "r" (input2),
+		       THUNK_TARGET(hv_hypercall_pg)
+		     : "cc", "r8", "r9", "r10", "r11");
 	}
 #else
 	{