[PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky]

When processing a hypercall for a guest with protected state, currently
SEV-ES guests, the guest CS segment register can't be checked to
determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
expected that communication between the guest and the hypervisor is
performed to shared memory using the GHCB. In order to use the GHCB, the
guest must have been in long mode, otherwise writes by the guest to the
GHCB would be encrypted and not be able to be comprehended by the
hypervisor.

Create a new helper function, is_64_bit_hypercall(), that assumes the
guest is in 64-bit mode when the guest has protected state, and returns
true, otherwise invoking is_64_bit_mode() to determine the mode. Update
the hypercall related routines to use is_64_bit_hypercall() instead of
is_64_bit_mode().

Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
this helper function for a guest running with protected state.

Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
Reported-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---

Changes since v1:
- Create a new helper routine, is_64_bit_hypercall(), and use it in place
  of is_64_bit_mode() in hypercall related areas.
- Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
  for a guest with protected state.
---
 arch/x86/kvm/hyperv.c |  4 ++--
 arch/x86/kvm/x86.c    |  2 +-
 arch/x86/kvm/x86.h    | 12 ++++++++++++
 arch/x86/kvm/xen.c    |  2 +-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f98370a39936..1cdf2b213f41 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
 {
 	bool longmode;
 
-	longmode = is_64_bit_mode(vcpu);
+	longmode = is_64_bit_hypercall(vcpu);
 	if (longmode)
 		kvm_rax_write(vcpu, result);
 	else {
@@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
 	}
 
 #ifdef CONFIG_X86_64
-	if (is_64_bit_mode(vcpu)) {
+	if (is_64_bit_hypercall(vcpu)) {
 		param = kvm_rcx_read(vcpu);
 		ingpa = kvm_rdx_read(vcpu);
 		outgpa = kvm_r8_read(vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b6bca616929..dc72f0a1609a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
 	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
-	op_64_bit = is_64_bit_mode(vcpu);
+	op_64_bit = is_64_bit_hypercall(vcpu);
 	if (!op_64_bit) {
 		nr &= 0xFFFFFFFF;
 		a0 &= 0xFFFFFFFF;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 521f74e5bbf2..3102caf689d2 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
 {
 	int cs_db, cs_l;
 
+	WARN_ON_ONCE(vcpu->arch.guest_state_protected);
+
 	if (!is_long_mode(vcpu))
 		return false;
 	static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
 	return cs_l;
 }
 
+static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * If running with protected guest state, the CS register is not
+	 * accessible. The hypercall register values will have had to been
+	 * provided in 64-bit mode, so assume the guest is in 64-bit.
+	 */
+	return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
+}
+
 static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
 {
 #ifdef CONFIG_X86_64
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index ae17250e1efe..c58f6369e668 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
 	    kvm_hv_hypercall_enabled(vcpu))
 		return kvm_hv_hypercall(vcpu);
 
-	longmode = is_64_bit_mode(vcpu);
+	longmode = is_64_bit_hypercall(vcpu);
 	if (!longmode) {
 		params[0] = (u32)kvm_rbx_read(vcpu);
 		params[1] = (u32)kvm_rcx_read(vcpu);
-- 
2.31.0

Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Vitaly Kuznetsov]

Tom Lendacky <thomas.lendacky@amd.com> writes:

> When processing a hypercall for a guest with protected state, currently
> SEV-ES guests, the guest CS segment register can't be checked to
> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
> expected that communication between the guest and the hypervisor is
> performed to shared memory using the GHCB. In order to use the GHCB, the
> guest must have been in long mode, otherwise writes by the guest to the
> GHCB would be encrypted and not be able to be comprehended by the
> hypervisor.
>
> Create a new helper function, is_64_bit_hypercall(), that assumes the
> guest is in 64-bit mode when the guest has protected state, and returns
> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
> the hypercall related routines to use is_64_bit_hypercall() instead of
> is_64_bit_mode().
>
> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
> this helper function for a guest running with protected state.
>
> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> Reported-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>
> Changes since v1:
> - Create a new helper routine, is_64_bit_hypercall(), and use it in place
>   of is_64_bit_mode() in hypercall related areas.
> - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
>   for a guest with protected state.
> ---
>  arch/x86/kvm/hyperv.c |  4 ++--
>  arch/x86/kvm/x86.c    |  2 +-
>  arch/x86/kvm/x86.h    | 12 ++++++++++++
>  arch/x86/kvm/xen.c    |  2 +-
>  4 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index f98370a39936..1cdf2b213f41 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>  {
>  	bool longmode;
>  
> -	longmode = is_64_bit_mode(vcpu);
> +	longmode = is_64_bit_hypercall(vcpu);
>  	if (longmode)
>  		kvm_rax_write(vcpu, result);
>  	else {
> @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>  	}
>  
>  #ifdef CONFIG_X86_64
> -	if (is_64_bit_mode(vcpu)) {
> +	if (is_64_bit_hypercall(vcpu)) {
>  		param = kvm_rcx_read(vcpu);
>  		ingpa = kvm_rdx_read(vcpu);
>  		outgpa = kvm_r8_read(vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..dc72f0a1609a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  
>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>  
> -	op_64_bit = is_64_bit_mode(vcpu);
> +	op_64_bit = is_64_bit_hypercall(vcpu);
>  	if (!op_64_bit) {
>  		nr &= 0xFFFFFFFF;
>  		a0 &= 0xFFFFFFFF;
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 521f74e5bbf2..3102caf689d2 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>  {
>  	int cs_db, cs_l;
>  
> +	WARN_ON_ONCE(vcpu->arch.guest_state_protected);
> +
>  	if (!is_long_mode(vcpu))
>  		return false;
>  	static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
>  	return cs_l;
>  }
>  
> +static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * If running with protected guest state, the CS register is not
> +	 * accessible. The hypercall register values will have had to been
> +	 * provided in 64-bit mode, so assume the guest is in 64-bit.
> +	 */
> +	return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
> +}
> +
>  static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
>  {
>  #ifdef CONFIG_X86_64
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index ae17250e1efe..c58f6369e668 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>  	    kvm_hv_hypercall_enabled(vcpu))
>  		return kvm_hv_hypercall(vcpu);
>  
> -	longmode = is_64_bit_mode(vcpu);
> +	longmode = is_64_bit_hypercall(vcpu);
>  	if (!longmode) {
>  		params[0] = (u32)kvm_rbx_read(vcpu);
>  		params[1] = (u32)kvm_rcx_read(vcpu);

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Thanks!

-- 
Vitaly

Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky]

On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
> Tom Lendacky <thomas.lendacky@amd.com> writes:
> 
>> When processing a hypercall for a guest with protected state, currently
>> SEV-ES guests, the guest CS segment register can't be checked to
>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
>> expected that communication between the guest and the hypervisor is
>> performed to shared memory using the GHCB. In order to use the GHCB, the
>> guest must have been in long mode, otherwise writes by the guest to the
>> GHCB would be encrypted and not be able to be comprehended by the
>> hypervisor.
>>
>> Create a new helper function, is_64_bit_hypercall(), that assumes the
>> guest is in 64-bit mode when the guest has protected state, and returns
>> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
>> the hypercall related routines to use is_64_bit_hypercall() instead of
>> is_64_bit_mode().
>>
>> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
>> this helper function for a guest running with protected state.
>>
>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
>> Reported-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>
>> Changes since v1:
>> - Create a new helper routine, is_64_bit_hypercall(), and use it in place
>>    of is_64_bit_mode() in hypercall related areas.
>> - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
>>    for a guest with protected state.
>> ---
>>   arch/x86/kvm/hyperv.c |  4 ++--
>>   arch/x86/kvm/x86.c    |  2 +-
>>   arch/x86/kvm/x86.h    | 12 ++++++++++++
>>   arch/x86/kvm/xen.c    |  2 +-
>>   4 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index f98370a39936..1cdf2b213f41 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>>   {
>>   	bool longmode;
>>   
>> -	longmode = is_64_bit_mode(vcpu);
>> +	longmode = is_64_bit_hypercall(vcpu);
>>   	if (longmode)
>>   		kvm_rax_write(vcpu, result);
>>   	else {
>> @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>>   	}
>>   
>>   #ifdef CONFIG_X86_64
>> -	if (is_64_bit_mode(vcpu)) {
>> +	if (is_64_bit_hypercall(vcpu)) {
>>   		param = kvm_rcx_read(vcpu);
>>   		ingpa = kvm_rdx_read(vcpu);
>>   		outgpa = kvm_r8_read(vcpu);
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 9b6bca616929..dc72f0a1609a 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>   
>>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>   
>> -	op_64_bit = is_64_bit_mode(vcpu);
>> +	op_64_bit = is_64_bit_hypercall(vcpu);
>>   	if (!op_64_bit) {
>>   		nr &= 0xFFFFFFFF;
>>   		a0 &= 0xFFFFFFFF;
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 521f74e5bbf2..3102caf689d2 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>>   {
>>   	int cs_db, cs_l;
>>   
>> +	WARN_ON_ONCE(vcpu->arch.guest_state_protected);
>> +
>>   	if (!is_long_mode(vcpu))
>>   		return false;
>>   	static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
>>   	return cs_l;
>>   }
>>   
>> +static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
>> +{
>> +	/*
>> +	 * If running with protected guest state, the CS register is not
>> +	 * accessible. The hypercall register values will have had to been
>> +	 * provided in 64-bit mode, so assume the guest is in 64-bit.
>> +	 */
>> +	return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
>> +}
>> +
>>   static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
>>   {
>>   #ifdef CONFIG_X86_64
>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> index ae17250e1efe..c58f6369e668 100644
>> --- a/arch/x86/kvm/xen.c
>> +++ b/arch/x86/kvm/xen.c
>> @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>   	    kvm_hv_hypercall_enabled(vcpu))
>>   		return kvm_hv_hypercall(vcpu);
>>   
>> -	longmode = is_64_bit_mode(vcpu);
>> +	longmode = is_64_bit_hypercall(vcpu);
>>   	if (!longmode) {
>>   		params[0] = (u32)kvm_rbx_read(vcpu);
>>   		params[1] = (u32)kvm_rcx_read(vcpu);
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Thanks!

Paolo,

This got lost in my stack of work... any comments?

Thanks,
Tom

> 

Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky]

On 10/1/21 12:06 PM, Tom Lendacky wrote:
> On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
>> Tom Lendacky <thomas.lendacky@amd.com> writes:
>>
>>> When processing a hypercall for a guest with protected state, currently
>>> SEV-ES guests, the guest CS segment register can't be checked to
>>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
>>> expected that communication between the guest and the hypervisor is
>>> performed to shared memory using the GHCB. In order to use the GHCB, the
>>> guest must have been in long mode, otherwise writes by the guest to the
>>> GHCB would be encrypted and not be able to be comprehended by the
>>> hypervisor.
>>>
>>> Create a new helper function, is_64_bit_hypercall(), that assumes the
>>> guest is in 64-bit mode when the guest has protected state, and returns
>>> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
>>> the hypercall related routines to use is_64_bit_hypercall() instead of
>>> is_64_bit_mode().
>>>
>>> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
>>> this helper function for a guest running with protected state.
>>>
>>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support 
>>> intercepts under SEV-ES")
>>> Reported-by: Sean Christopherson <seanjc@google.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>
>>> Changes since v1:
>>> - Create a new helper routine, is_64_bit_hypercall(), and use it in place
>>>    of is_64_bit_mode() in hypercall related areas.
>>> - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
>>>    for a guest with protected state.
>>> ---
>>>   arch/x86/kvm/hyperv.c |  4 ++--
>>>   arch/x86/kvm/x86.c    |  2 +-
>>>   arch/x86/kvm/x86.h    | 12 ++++++++++++
>>>   arch/x86/kvm/xen.c    |  2 +-
>>>   4 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>>> index f98370a39936..1cdf2b213f41 100644
>>> --- a/arch/x86/kvm/hyperv.c
>>> +++ b/arch/x86/kvm/hyperv.c
>>> @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct 
>>> kvm_vcpu *vcpu, u64 result)
>>>   {
>>>       bool longmode;
>>> -    longmode = is_64_bit_mode(vcpu);
>>> +    longmode = is_64_bit_hypercall(vcpu);
>>>       if (longmode)
>>>           kvm_rax_write(vcpu, result);
>>>       else {
>>> @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>>>       }
>>>   #ifdef CONFIG_X86_64
>>> -    if (is_64_bit_mode(vcpu)) {
>>> +    if (is_64_bit_hypercall(vcpu)) {
>>>           param = kvm_rcx_read(vcpu);
>>>           ingpa = kvm_rdx_read(vcpu);
>>>           outgpa = kvm_r8_read(vcpu);
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 9b6bca616929..dc72f0a1609a 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>       trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>> -    op_64_bit = is_64_bit_mode(vcpu);
>>> +    op_64_bit = is_64_bit_hypercall(vcpu);
>>>       if (!op_64_bit) {
>>>           nr &= 0xFFFFFFFF;
>>>           a0 &= 0xFFFFFFFF;
>>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>>> index 521f74e5bbf2..3102caf689d2 100644
>>> --- a/arch/x86/kvm/x86.h
>>> +++ b/arch/x86/kvm/x86.h
>>> @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu 
>>> *vcpu)
>>>   {
>>>       int cs_db, cs_l;
>>> +    WARN_ON_ONCE(vcpu->arch.guest_state_protected);
>>> +
>>>       if (!is_long_mode(vcpu))
>>>           return false;
>>>       static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
>>>       return cs_l;
>>>   }
>>> +static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
>>> +{
>>> +    /*
>>> +     * If running with protected guest state, the CS register is not
>>> +     * accessible. The hypercall register values will have had to been
>>> +     * provided in 64-bit mode, so assume the guest is in 64-bit.
>>> +     */
>>> +    return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
>>> +}
>>> +
>>>   static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
>>>   {
>>>   #ifdef CONFIG_X86_64
>>> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>>> index ae17250e1efe..c58f6369e668 100644
>>> --- a/arch/x86/kvm/xen.c
>>> +++ b/arch/x86/kvm/xen.c
>>> @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>>>           kvm_hv_hypercall_enabled(vcpu))
>>>           return kvm_hv_hypercall(vcpu);
>>> -    longmode = is_64_bit_mode(vcpu);
>>> +    longmode = is_64_bit_hypercall(vcpu);
>>>       if (!longmode) {
>>>           params[0] = (u32)kvm_rbx_read(vcpu);
>>>           params[1] = (u32)kvm_rcx_read(vcpu);
>>
>> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>
>> Thanks!
> 
> Paolo,
> 
> This got lost in my stack of work... any comments?
> 
> Thanks,
> Tom

Ping

Thanks,
Tom

> 
>>

Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Tom Lendacky]

On 10/26/21 1:03 PM, Tom Lendacky wrote:
> On 10/1/21 12:06 PM, Tom Lendacky wrote:
>> On 5/25/21 1:25 AM, Vitaly Kuznetsov wrote:
>>> Tom Lendacky <thomas.lendacky@amd.com> writes:
>>>
>>>> When processing a hypercall for a guest with protected state, currently
>>>> SEV-ES guests, the guest CS segment register can't be checked to
>>>> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
>>>> expected that communication between the guest and the hypervisor is
>>>> performed to shared memory using the GHCB. In order to use the GHCB, the
>>>> guest must have been in long mode, otherwise writes by the guest to the
>>>> GHCB would be encrypted and not be able to be comprehended by the
>>>> hypervisor.
>>>>
>>>> Create a new helper function, is_64_bit_hypercall(), that assumes the
>>>> guest is in 64-bit mode when the guest has protected state, and returns
>>>> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
>>>> the hypercall related routines to use is_64_bit_hypercall() instead of
>>>> is_64_bit_mode().
>>>>
>>>> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
>>>> this helper function for a guest running with protected state.
>>>>
>>>> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support 
>>>> intercepts under SEV-ES")
>>>> Reported-by: Sean Christopherson <seanjc@google.com>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---

...

>>>
>>> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>
>>> Thanks!
>>
>> Paolo,
>>
>> This got lost in my stack of work... any comments?
>>
>> Thanks,
>> Tom
> 
> Ping
> 
> Thanks,
> Tom

Another ping on this.

Paolo, you had replied "queued" on the v1, but came up with a suggestion
for a v2 that might have got lost in the replies because it never got
queued. Here are the threads:

v1: https://lore.kernel.org/kvm/d0904f0d049300267665bd4abf96c3d7e7aa4825.1621701837.git.thomas.lendacky@amd.com/

v2: https://lore.kernel.org/kvm/e0b20c770c9d0d1403f23d83e785385104211f74.1621878537.git.thomas.lendacky@amd.com/

Thanks,
Tom

> 
>>
>>>

Re: [PATCH v2] KVM: x86: Assume a 64-bit hypercall for guests with protected state

[Paolo Bonzini]

On 5/24/21 19:48, Tom Lendacky wrote:
> When processing a hypercall for a guest with protected state, currently
> SEV-ES guests, the guest CS segment register can't be checked to
> determine if the guest is in 64-bit mode. For an SEV-ES guest, it is
> expected that communication between the guest and the hypervisor is
> performed to shared memory using the GHCB. In order to use the GHCB, the
> guest must have been in long mode, otherwise writes by the guest to the
> GHCB would be encrypted and not be able to be comprehended by the
> hypervisor.
> 
> Create a new helper function, is_64_bit_hypercall(), that assumes the
> guest is in 64-bit mode when the guest has protected state, and returns
> true, otherwise invoking is_64_bit_mode() to determine the mode. Update
> the hypercall related routines to use is_64_bit_hypercall() instead of
> is_64_bit_mode().
> 
> Add a WARN_ON_ONCE() to is_64_bit_mode() to catch occurences of calls to
> this helper function for a guest running with protected state.
> 
> Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
> Reported-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> 
> Changes since v1:
> - Create a new helper routine, is_64_bit_hypercall(), and use it in place
>    of is_64_bit_mode() in hypercall related areas.
> - Add a WARN_ON_ONCE() to is_64_bit_mode() to issue a warning if invoked
>    for a guest with protected state.
> ---
>   arch/x86/kvm/hyperv.c |  4 ++--
>   arch/x86/kvm/x86.c    |  2 +-
>   arch/x86/kvm/x86.h    | 12 ++++++++++++
>   arch/x86/kvm/xen.c    |  2 +-
>   4 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index f98370a39936..1cdf2b213f41 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1818,7 +1818,7 @@ static void kvm_hv_hypercall_set_result(struct kvm_vcpu *vcpu, u64 result)
>   {
>   	bool longmode;
>   
> -	longmode = is_64_bit_mode(vcpu);
> +	longmode = is_64_bit_hypercall(vcpu);
>   	if (longmode)
>   		kvm_rax_write(vcpu, result);
>   	else {
> @@ -1895,7 +1895,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
>   	}
>   
>   #ifdef CONFIG_X86_64
> -	if (is_64_bit_mode(vcpu)) {
> +	if (is_64_bit_hypercall(vcpu)) {
>   		param = kvm_rcx_read(vcpu);
>   		ingpa = kvm_rdx_read(vcpu);
>   		outgpa = kvm_r8_read(vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..dc72f0a1609a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8403,7 +8403,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   
>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>   
> -	op_64_bit = is_64_bit_mode(vcpu);
> +	op_64_bit = is_64_bit_hypercall(vcpu);
>   	if (!op_64_bit) {
>   		nr &= 0xFFFFFFFF;
>   		a0 &= 0xFFFFFFFF;
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 521f74e5bbf2..3102caf689d2 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -151,12 +151,24 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
>   {
>   	int cs_db, cs_l;
>   
> +	WARN_ON_ONCE(vcpu->arch.guest_state_protected);
> +
>   	if (!is_long_mode(vcpu))
>   		return false;
>   	static_call(kvm_x86_get_cs_db_l_bits)(vcpu, &cs_db, &cs_l);
>   	return cs_l;
>   }
>   
> +static inline bool is_64_bit_hypercall(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * If running with protected guest state, the CS register is not
> +	 * accessible. The hypercall register values will have had to been
> +	 * provided in 64-bit mode, so assume the guest is in 64-bit.
> +	 */
> +	return vcpu->arch.guest_state_protected || is_64_bit_mode(vcpu);
> +}
> +
>   static inline bool is_la57_mode(struct kvm_vcpu *vcpu)
>   {
>   #ifdef CONFIG_X86_64
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index ae17250e1efe..c58f6369e668 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -680,7 +680,7 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu)
>   	    kvm_hv_hypercall_enabled(vcpu))
>   		return kvm_hv_hypercall(vcpu);
>   
> -	longmode = is_64_bit_mode(vcpu);
> +	longmode = is_64_bit_hypercall(vcpu);
>   	if (!longmode) {
>   		params[0] = (u32)kvm_rbx_read(vcpu);
>   		params[1] = (u32)kvm_rcx_read(vcpu);
> 

Queued, thanks.

Paolo