[PATCH v2 0/2] KVM: x86: Check hypercall's exit to userspace generically

[PATCH v2 0/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

Currently in kvm_emulate_hypercall, KVM_HC_MAP_GPA_RANGE is checked
specifically to decide whether a KVM hypercall needs to exit to userspace
or not.  Do the check based on the hypercall_exit_enabled field of
struct kvm_arch.

Also use the API is_kvm_hc_exit_enabled() to replace the opencode.

---
v2:
- Check the return value of __kvm_emulate_hypercall() before checking
  hypercall_exit_enabled to avoid an invalid KVM hypercall nr.
  https://lore.kernel.org/kvm/184d90a8-14a0-494a-9112-365417245911@linux.intel.com/
- Add a warning if a hypercall nr out of the range of hypercall_exit_enabled
  can express.

Binbin Wu (2):
  KVM: x86: Check hypercall's exit to userspace generically
  KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

 arch/x86/kvm/svm/sev.c | 4 ++--
 arch/x86/kvm/x86.c     | 6 +++---
 arch/x86/kvm/x86.h     | 7 +++++++
 3 files changed, 12 insertions(+), 5 deletions(-)


base-commit: 332d2c1d713e232e163386c35a3ba0c1b90df83f
-- 
2.43.2

[PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

Check whether a KVM hypercall needs to exit to userspace or not based on
hypercall_exit_enabled field of struct kvm_arch.

Userspace can request a hypercall to exit to userspace for handling by
enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
hypercall_exit_enabled.  Make the check code generic based on it.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 arch/x86/kvm/x86.h | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index af6c8cf6a37a..6e16c9751af7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	cpl = kvm_x86_call(get_cpl)(vcpu);
 
 	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-		/* MAP_GPA tosses the request to the user space. */
+	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
+		/* The hypercall is requested to exit to userspace. */
 		return 0;
 
 	if (!op_64_bit)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 50596f6f8320..0cbec76b42e6 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
 			 unsigned int port, void *data,  unsigned int count,
 			 int in);
 
+static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
+{
+	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
+		return false;
+
+	return kvm->arch.hypercall_exit_enabled & (1 << hc_nr);
+}
 #endif
-- 
2.43.2

[PATCH v2 2/2] KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

[Binbin Wu]

Use is_kvm_hc_exit_enabled() instead of opencode.

No functional change intended.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 arch/x86/kvm/svm/sev.c | 4 ++--
 arch/x86/kvm/x86.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a16c873b3232..d622aab8351d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3635,7 +3635,7 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr)
 		return 1; /* resume guest */
 	}
 
-	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
+	if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
 		set_ghcb_msr(svm, GHCB_MSR_PSC_RESP_ERROR);
 		return 1; /* resume guest */
 	}
@@ -3718,7 +3718,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 	bool huge;
 	u64 gfn;
 
-	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
+	if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
 		snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
 		return 1;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e16c9751af7..9857c1984ef7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10171,7 +10171,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 		u64 gpa = a0, npages = a1, attrs = a2;
 
 		ret = -KVM_ENOSYS;
-		if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE)))
+		if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
 			break;
 
 		if (!PAGE_ALIGNED(gpa) || !npages ||
-- 
2.43.2

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Yuan Yao]

On Tue, Aug 13, 2024 at 01:12:55PM +0800, Binbin Wu wrote:
> Check whether a KVM hypercall needs to exit to userspace or not based on
> hypercall_exit_enabled field of struct kvm_arch.
>
> Userspace can request a hypercall to exit to userspace for handling by
> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> hypercall_exit_enabled.  Make the check code generic based on it.
>
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++--
>  arch/x86/kvm/x86.h | 7 +++++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index af6c8cf6a37a..6e16c9751af7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  	cpl = kvm_x86_call(get_cpl)(vcpu);
>
>  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> +		/* The hypercall is requested to exit to userspace. */
>  		return 0;
>
>  	if (!op_64_bit)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 50596f6f8320..0cbec76b42e6 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>  			 unsigned int port, void *data,  unsigned int count,
>  			 int in);
>
> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> +{
> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))

How about:

if (!(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK))

KVM_EXIT_HYPERCALL_VALID_MASK is used to guard kvm->arch.hypercall_exit_enabled
on KVM_CAP_EXIT_HYPERCALL, "hc_nr > maximum supported hc" AND "hc_nr <=
bit_count(kvm->arch.hypercall_exit_enabled)" should be treated as invalid yet to
me.

> +		return false;
> +
> +	return kvm->arch.hypercall_exit_enabled & (1 << hc_nr);

BIT(xx) instead of "1 << hc_nr" for better readability.

> +}
>  #endif
> --
> 2.43.2
>
>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Yuan Yao]

On Tue, Aug 13, 2024 at 01:56:14PM +0800, Yuan Yao wrote:
> On Tue, Aug 13, 2024 at 01:12:55PM +0800, Binbin Wu wrote:
> > Check whether a KVM hypercall needs to exit to userspace or not based on
> > hypercall_exit_enabled field of struct kvm_arch.
> >
> > Userspace can request a hypercall to exit to userspace for handling by
> > enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> > hypercall_exit_enabled.  Make the check code generic based on it.
> >
> > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 ++--
> >  arch/x86/kvm/x86.h | 7 +++++++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index af6c8cf6a37a..6e16c9751af7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >  	cpl = kvm_x86_call(get_cpl)(vcpu);
> >
> >  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> > +		/* The hypercall is requested to exit to userspace. */
> >  		return 0;
> >
> >  	if (!op_64_bit)
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index 50596f6f8320..0cbec76b42e6 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
> >  			 unsigned int port, void *data,  unsigned int count,
> >  			 int in);
> >
> > +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> > +{
> > +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>
> How about:
>
> if (!(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK))
>
> KVM_EXIT_HYPERCALL_VALID_MASK is used to guard kvm->arch.hypercall_exit_enabled
> on KVM_CAP_EXIT_HYPERCALL, "hc_nr > maximum supported hc" AND "hc_nr <=
> bit_count(kvm->arch.hypercall_exit_enabled)" should be treated as invalid yet to
> me.

Not real good idea. Rely on hypercall_exit_enabled is good enough, this brings
unnecessary complexity.

>
> > +		return false;
> > +
> > +	return kvm->arch.hypercall_exit_enabled & (1 << hc_nr);
>
> BIT(xx) instead of "1 << hc_nr" for better readability.
>
> > +}
> >  #endif
> > --
> > 2.43.2
> >
> >
>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Isaku Yamahata]

On Tue, Aug 13, 2024 at 01:12:55PM +0800,
Binbin Wu <binbin.wu@linux.intel.com> wrote:

> Check whether a KVM hypercall needs to exit to userspace or not based on
> hypercall_exit_enabled field of struct kvm_arch.
> 
> Userspace can request a hypercall to exit to userspace for handling by
> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> hypercall_exit_enabled.  Make the check code generic based on it.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++--
>  arch/x86/kvm/x86.h | 7 +++++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index af6c8cf6a37a..6e16c9751af7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  	cpl = kvm_x86_call(get_cpl)(vcpu);
>  
>  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> +		/* The hypercall is requested to exit to userspace. */
>  		return 0;
>  
>  	if (!op_64_bit)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 50596f6f8320..0cbec76b42e6 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>  			 unsigned int port, void *data,  unsigned int count,
>  			 int in);
>  
> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> +{
> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
> +		return false;

Is this to detect potential bug? Maybe
BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
             !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
Overkill?
-- 
Isaku Yamahata <isaku.yamahata@intel.com>

Re: [PATCH v2 2/2] KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

[Isaku Yamahata]

On Tue, Aug 13, 2024 at 01:12:56PM +0800,
Binbin Wu <binbin.wu@linux.intel.com> wrote:

> Use is_kvm_hc_exit_enabled() instead of opencode.
> 
> No functional change intended.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
>  arch/x86/kvm/svm/sev.c | 4 ++--
>  arch/x86/kvm/x86.c     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a16c873b3232..d622aab8351d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3635,7 +3635,7 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr)
>  		return 1; /* resume guest */
>  	}
>  
> -	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
> +	if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
>  		set_ghcb_msr(svm, GHCB_MSR_PSC_RESP_ERROR);
>  		return 1; /* resume guest */
>  	}
> @@ -3718,7 +3718,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
>  	bool huge;
>  	u64 gfn;
>  
> -	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
> +	if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
>  		snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
>  		return 1;
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6e16c9751af7..9857c1984ef7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10171,7 +10171,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  		u64 gpa = a0, npages = a1, attrs = a2;
>  
>  		ret = -KVM_ENOSYS;
> -		if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE)))
> +		if (!is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
>  			break;
>  
>  		if (!PAGE_ALIGNED(gpa) || !npages ||
> -- 
> 2.43.2
> 
> 

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
-- 
Isaku Yamahata <isaku.yamahata@intel.com>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On 14/08/2024 5:50 am, Isaku Yamahata wrote:
> On Tue, Aug 13, 2024 at 01:12:55PM +0800,
> Binbin Wu <binbin.wu@linux.intel.com> wrote:
> 
>> Check whether a KVM hypercall needs to exit to userspace or not based on
>> hypercall_exit_enabled field of struct kvm_arch.
>>
>> Userspace can request a hypercall to exit to userspace for handling by
>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>> hypercall_exit_enabled.  Make the check code generic based on it.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>>   arch/x86/kvm/x86.c | 4 ++--
>>   arch/x86/kvm/x86.h | 7 +++++++
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index af6c8cf6a37a..6e16c9751af7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>>   
>>   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>> -		/* MAP_GPA tosses the request to the user space. */
>> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
>> +		/* The hypercall is requested to exit to userspace. */
>>   		return 0;
>>   
>>   	if (!op_64_bit)
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 50596f6f8320..0cbec76b42e6 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>>   			 unsigned int port, void *data,  unsigned int count,
>>   			 int in);
>>   
>> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
>> +{
>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>> +		return false;
> 
> Is this to detect potential bug? Maybe
> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>               !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
> Overkill?

I don't think this is the correct way to use __builtin_constant_p(), 
i.e. it doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().

IIUC you need some build time guarantee here, but __builtin_constant_p() 
can return false, in which case the above BUILD_BUG_ON() does nothing, 
which defeats the purpose.

On the other hand, albeit WARN_ON_ONCE() is runtime check, but it is 
always there.

In fact, the @hc_nr (or @nr) in the kvm_emulate_hypercall() is read from 
the RAX register:

         nr = kvm_rax_read(vcpu);

So I don't see how the compiler can be smart enough to determine the 
value at compile time.

In fact, I tried to build by removing the __builtin_constant_p() but got 
below error (sorry for text wrap but you can see the error I believe).

root@server:/home/kai/tdx/linux# make M=arch/x86/kvm/ W=1
   CC [M]  arch/x86/kvm/x86.o
In file included from <command-line>:
In function ‘is_kvm_hc_exit_enabled’,
     inlined from ‘kvm_emulate_hypercall’ at arch/x86/kvm/x86.c:10254:14:
././include/linux/compiler_types.h:510:45: error: call to 
‘__compiletime_assert_3873’ declared with attribute error: BUILD_BUG_ON 
failed: !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK)
   510 |         _compiletime_assert(condition, msg, 
__compiletime_assert_, __COUNTER__)
       |                                             ^

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On 13/08/2024 5:12 pm, Binbin Wu wrote:
> Check whether a KVM hypercall needs to exit to userspace or not based on
> hypercall_exit_enabled field of struct kvm_arch.
> 
> Userspace can request a hypercall to exit to userspace for handling by
> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> hypercall_exit_enabled.  Make the check code generic based on it.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

One nitpicking below:

> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   arch/x86/kvm/x86.h | 7 +++++++
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index af6c8cf6a37a..6e16c9751af7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>   
>   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> +		/* The hypercall is requested to exit to userspace. */
>   		return 0;

I believe you put "!ret" check first for a reason?  Perhaps you can add 
a comment.

Re: [PATCH v2 2/2] KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

[Huang, Kai]

On 13/08/2024 5:12 pm, Binbin Wu wrote:
> Use is_kvm_hc_exit_enabled() instead of opencode.
> 
> No functional change intended.

It would be helpful to mention currently hypercall_exit_enabled  can 
only have KVM_HC_MAP_GPA_RANGE bit set (so that there will be no 
functional change).

> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Isaku Yamahata]

On Wed, Aug 14, 2024 at 11:11:29AM +1200,
"Huang, Kai" <kai.huang@intel.com> wrote:

> 
> 
> On 14/08/2024 5:50 am, Isaku Yamahata wrote:
> > On Tue, Aug 13, 2024 at 01:12:55PM +0800,
> > Binbin Wu <binbin.wu@linux.intel.com> wrote:
> > 
> > > Check whether a KVM hypercall needs to exit to userspace or not based on
> > > hypercall_exit_enabled field of struct kvm_arch.
> > > 
> > > Userspace can request a hypercall to exit to userspace for handling by
> > > enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> > > hypercall_exit_enabled.  Make the check code generic based on it.
> > > 
> > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> > > ---
> > >   arch/x86/kvm/x86.c | 4 ++--
> > >   arch/x86/kvm/x86.h | 7 +++++++
> > >   2 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index af6c8cf6a37a..6e16c9751af7 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> > >   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> > > +		/* The hypercall is requested to exit to userspace. */
> > >   		return 0;
> > >   	if (!op_64_bit)
> > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > > index 50596f6f8320..0cbec76b42e6 100644
> > > --- a/arch/x86/kvm/x86.h
> > > +++ b/arch/x86/kvm/x86.h
> > > @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
> > >   			 unsigned int port, void *data,  unsigned int count,
> > >   			 int in);
> > > +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> > > +{
> > > +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
> > > +		return false;
> > 
> > Is this to detect potential bug? Maybe
> > BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
> >               !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
> > Overkill?
> 
> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
> 
> IIUC you need some build time guarantee here, but __builtin_constant_p() can
> return false, in which case the above BUILD_BUG_ON() does nothing, which
> defeats the purpose.

It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
can detect the usage in the patch 2/2,
is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).

Although this version doesn't help for the one in kvm_emulate_hypercall(),
!ret check is done first to avoid WARN_ON_ONCE() to hit here.

Maybe we can just drop this WARN_ON_ONCE().
-- 
Isaku Yamahata <isaku.yamahata@intel.com>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Isaku Yamahata]

On Wed, Aug 14, 2024 at 11:16:44AM +1200,
"Huang, Kai" <kai.huang@intel.com> wrote:

> > ---
> >   arch/x86/kvm/x86.c | 4 ++--
> >   arch/x86/kvm/x86.h | 7 +++++++
> >   2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index af6c8cf6a37a..6e16c9751af7 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> >   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> > +		/* The hypercall is requested to exit to userspace. */
> >   		return 0;
> 
> I believe you put "!ret" check first for a reason?  Perhaps you can add a
> comment.

I think he'd like to avoid to hit WARN_ON_ONCE().
-- 
Isaku Yamahata <isaku.yamahata@intel.com>

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 8/14/2024 7:16 AM, Huang, Kai wrote:
>
>
> On 13/08/2024 5:12 pm, Binbin Wu wrote:
>> Check whether a KVM hypercall needs to exit to userspace or not based on
>> hypercall_exit_enabled field of struct kvm_arch.
>>
>> Userspace can request a hypercall to exit to userspace for handling by
>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>> hypercall_exit_enabled.  Make the check code generic based on it.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
>
> One nitpicking below:
>
>> ---
>>   arch/x86/kvm/x86.c | 4 ++--
>>   arch/x86/kvm/x86.h | 7 +++++++
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index af6c8cf6a37a..6e16c9751af7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>       cpl = kvm_x86_call(get_cpl)(vcpu);
>>         ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, 
>> op_64_bit, cpl);
>> -    if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>> -        /* MAP_GPA tosses the request to the user space. */
>> +    if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
>> +        /* The hypercall is requested to exit to userspace. */
>>           return 0;
>
> I believe you put "!ret" check first for a reason?  Perhaps you can 
> add a comment.
>
>
Yes, check "!ret" first to make sure the input of 
is_kvm_hc_exit_enabled() is a valid KVM_HC_*.
Will add a comment.
Thanks.

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 8/14/2024 8:52 AM, Isaku Yamahata wrote:
> On Wed, Aug 14, 2024 at 11:11:29AM +1200,
> "Huang, Kai" <kai.huang@intel.com> wrote:
>
>>
>> On 14/08/2024 5:50 am, Isaku Yamahata wrote:
>>> On Tue, Aug 13, 2024 at 01:12:55PM +0800,
>>> Binbin Wu <binbin.wu@linux.intel.com> wrote:
>>>
>>>> Check whether a KVM hypercall needs to exit to userspace or not based on
>>>> hypercall_exit_enabled field of struct kvm_arch.
>>>>
>>>> Userspace can request a hypercall to exit to userspace for handling by
>>>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>>>> hypercall_exit_enabled.  Make the check code generic based on it.
>>>>
>>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>>>> ---
>>>>    arch/x86/kvm/x86.c | 4 ++--
>>>>    arch/x86/kvm/x86.h | 7 +++++++
>>>>    2 files changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index af6c8cf6a37a..6e16c9751af7 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>>    	cpl = kvm_x86_call(get_cpl)(vcpu);
>>>>    	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>>> -		/* MAP_GPA tosses the request to the user space. */
>>>> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
>>>> +		/* The hypercall is requested to exit to userspace. */
>>>>    		return 0;
>>>>    	if (!op_64_bit)
>>>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>>>> index 50596f6f8320..0cbec76b42e6 100644
>>>> --- a/arch/x86/kvm/x86.h
>>>> +++ b/arch/x86/kvm/x86.h
>>>> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>>>>    			 unsigned int port, void *data,  unsigned int count,
>>>>    			 int in);
>>>> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
>>>> +{
>>>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>>>> +		return false;
>>> Is this to detect potential bug? Maybe
>>> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>>>                !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
>>> Overkill?
My intention was to catch issue when KVM_HC_* grows and exceeds 32.
I was looking a compile time check, but didn't find a proper one.

>> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
>> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
>>
>> IIUC you need some build time guarantee here, but __builtin_constant_p() can
>> return false, in which case the above BUILD_BUG_ON() does nothing, which
>> defeats the purpose.
> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
> can detect the usage in the patch 2/2,
> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
>
> Although this version doesn't help for the one in kvm_emulate_hypercall(),
> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
Even !ret is checked first, it's still possible to hit the warning
if KVM_HC_furture_hypercall >=32.

>
> Maybe we can just drop this WARN_ON_ONCE().

Agree that a warning may not be a good option.
What I wanted to guarantee was that "KVM_HC_* < 32" when
hypercall_exit_enabled is u32.

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Tue, Aug 13, 2024, Isaku Yamahata wrote:
> On Wed, Aug 14, 2024 at 11:11:29AM +1200,
> Kai Huang <kai.huang@intel.com> wrote:
> 
> > 
> > 
> > On 14/08/2024 5:50 am, Isaku Yamahata wrote:
> > > On Tue, Aug 13, 2024 at 01:12:55PM +0800,
> > > Binbin Wu <binbin.wu@linux.intel.com> wrote:
> > > 
> > > > Check whether a KVM hypercall needs to exit to userspace or not based on
> > > > hypercall_exit_enabled field of struct kvm_arch.
> > > > 
> > > > Userspace can request a hypercall to exit to userspace for handling by
> > > > enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> > > > hypercall_exit_enabled.  Make the check code generic based on it.
> > > > 
> > > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> > > > ---
> > > >   arch/x86/kvm/x86.c | 4 ++--
> > > >   arch/x86/kvm/x86.h | 7 +++++++
> > > >   2 files changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index af6c8cf6a37a..6e16c9751af7 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > > >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> > > >   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > > -		/* MAP_GPA tosses the request to the user space. */
> > > > +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
> > > > +		/* The hypercall is requested to exit to userspace. */
> > > >   		return 0;
> > > >   	if (!op_64_bit)
> > > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > > > index 50596f6f8320..0cbec76b42e6 100644
> > > > --- a/arch/x86/kvm/x86.h
> > > > +++ b/arch/x86/kvm/x86.h
> > > > @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
> > > >   			 unsigned int port, void *data,  unsigned int count,
> > > >   			 int in);
> > > > +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)

I would rather have "hypercall" in the name, "hc" never jumps out to me as being
"hypercall". Maybe is_hypercall_exit_enabled(), user_exit_on_hypercall(), or just
exit_on_hypercall()?

I'd probably vote for user_exit_on_hypercall(), as that clarifies it's all about
exiting to userspace, not from the guest.

> > > > +{
> > > > +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
> > > > +		return false;
> > > 
> > > Is this to detect potential bug? Maybe
> > > BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
> > >               !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
> > > Overkill?
> > 
> > I don't think this is the correct way to use __builtin_constant_p(), i.e. it
> > doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().

KVM does use __builtin_constant_p() to effectively disable some assertions when
it's allowed (by KVM's arbitrary rules) to pass in a non-constant value.  E.g.
see all the vmcs_checkNN() helpers.  If we didn't waive the assertion for values
that aren't constant at compile-time, all of the segmentation code would need to
be unwound into switch statements.

But for things like guest_cpuid_has(), the rule is that the input must be a
compile-time constant.

> > IIUC you need some build time guarantee here, but __builtin_constant_p() can
> > return false, in which case the above BUILD_BUG_ON() does nothing, which
> > defeats the purpose.
> 
> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
> can detect the usage in the patch 2/2,
> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
> 
> Although this version doesn't help for the one in kvm_emulate_hypercall(),
> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
> 
> Maybe we can just drop this WARN_ON_ONCE().

Yeah, I think it makes sense to drop the WARN, otherwise I suspect we'll end up
dancing around the helper just to avoid the warning.

I'm 50/50 on the BUILD_BUG_ON().  One one hand, it's kinda overkill.  On the other
hand, it's zero generated code.

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

>>>>> +{
>>>>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>>>>> +		return false;
>>>>
>>>> Is this to detect potential bug? Maybe
>>>> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>>>>                !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
>>>> Overkill?
>>>
>>> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
>>> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
> 
> KVM does use __builtin_constant_p() to effectively disable some assertions when
> it's allowed (by KVM's arbitrary rules) to pass in a non-constant value.  E.g.
> see all the vmcs_checkNN() helpers.  If we didn't waive the assertion for values
> that aren't constant at compile-time, all of the segmentation code would need to
> be unwound into switch statements.

Yeah I saw vmcs_checkNN(), but I think __builtin_constant_p() makes 
sense for vmcs_checkNN()s because they are widely called.  But 
is_kvm_hc_exit_enabled() doesn't seem so.  But no hard opinion here.  As 
you said, it's kinda overkill (or abused to use) but zero-generated code.

> 
> But for things like guest_cpuid_has(), the rule is that the input must be a
> compile-time constant.
> 
>>> IIUC you need some build time guarantee here, but __builtin_constant_p() can
>>> return false, in which case the above BUILD_BUG_ON() does nothing, which
>>> defeats the purpose.
>>
>> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
>> can detect the usage in the patch 2/2,
>> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
>> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
>>
>> Although this version doesn't help for the one in kvm_emulate_hypercall(),
>> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
>>
>> Maybe we can just drop this WARN_ON_ONCE().
> 
> Yeah, I think it makes sense to drop the WARN, otherwise I suspect we'll end up
> dancing around the helper just to avoid the warning.

Agreed, given @nr is from guest.

Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 8/14/2024 9:31 AM, Sean Christopherson wrote:
> On Tue, Aug 13, 2024, Isaku Yamahata wrote:
>> On Wed, Aug 14, 2024 at 11:11:29AM +1200,
>> Kai Huang <kai.huang@intel.com> wrote:
>>
>>>
>>> On 14/08/2024 5:50 am, Isaku Yamahata wrote:
>>>> On Tue, Aug 13, 2024 at 01:12:55PM +0800,
>>>> Binbin Wu <binbin.wu@linux.intel.com> wrote:
>>>>
>>>>> Check whether a KVM hypercall needs to exit to userspace or not based on
>>>>> hypercall_exit_enabled field of struct kvm_arch.
>>>>>
>>>>> Userspace can request a hypercall to exit to userspace for handling by
>>>>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>>>>> hypercall_exit_enabled.  Make the check code generic based on it.
>>>>>
>>>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>>>>> ---
>>>>>    arch/x86/kvm/x86.c | 4 ++--
>>>>>    arch/x86/kvm/x86.h | 7 +++++++
>>>>>    2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>> index af6c8cf6a37a..6e16c9751af7 100644
>>>>> --- a/arch/x86/kvm/x86.c
>>>>> +++ b/arch/x86/kvm/x86.c
>>>>> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>>>    	cpl = kvm_x86_call(get_cpl)(vcpu);
>>>>>    	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>>>> -		/* MAP_GPA tosses the request to the user space. */
>>>>> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
>>>>> +		/* The hypercall is requested to exit to userspace. */
>>>>>    		return 0;
>>>>>    	if (!op_64_bit)
>>>>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>>>>> index 50596f6f8320..0cbec76b42e6 100644
>>>>> --- a/arch/x86/kvm/x86.h
>>>>> +++ b/arch/x86/kvm/x86.h
>>>>> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>>>>>    			 unsigned int port, void *data,  unsigned int count,
>>>>>    			 int in);
>>>>> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> I would rather have "hypercall" in the name, "hc" never jumps out to me as being
> "hypercall". Maybe is_hypercall_exit_enabled(), user_exit_on_hypercall(), or just
> exit_on_hypercall()?
>
> I'd probably vote for user_exit_on_hypercall(), as that clarifies it's all about
> exiting to userspace, not from the guest.
user_exit_on_hypercall() looks good to me.
Thanks!


>
>>>>> +{
>>>>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>>>>> +		return false;
>>>> Is this to detect potential bug? Maybe
>>>> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>>>>                !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
>>>> Overkill?
>>> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
>>> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
> KVM does use __builtin_constant_p() to effectively disable some assertions when
> it's allowed (by KVM's arbitrary rules) to pass in a non-constant value.  E.g.
> see all the vmcs_checkNN() helpers.  If we didn't waive the assertion for values
> that aren't constant at compile-time, all of the segmentation code would need to
> be unwound into switch statements.
>
> But for things like guest_cpuid_has(), the rule is that the input must be a
> compile-time constant.
>
>>> IIUC you need some build time guarantee here, but __builtin_constant_p() can
>>> return false, in which case the above BUILD_BUG_ON() does nothing, which
>>> defeats the purpose.
>> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
>> can detect the usage in the patch 2/2,
>> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
>> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
>>
>> Although this version doesn't help for the one in kvm_emulate_hypercall(),
>> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
>>
>> Maybe we can just drop this WARN_ON_ONCE().
> Yeah, I think it makes sense to drop the WARN, otherwise I suspect we'll end up
> dancing around the helper just to avoid the warning.
>
> I'm 50/50 on the BUILD_BUG_ON().  One one hand, it's kinda overkill.  On the other
> hand, it's zero generated code.
>
Will remove the WARN_ON_ONCE().

Re: [PATCH v2 2/2] KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

[Binbin Wu]

On 8/14/2024 7:18 AM, Huang, Kai wrote:
>
>
> On 13/08/2024 5:12 pm, Binbin Wu wrote:
>> Use is_kvm_hc_exit_enabled() instead of opencode.
>>
>> No functional change intended.
>
> It would be helpful to mention currently hypercall_exit_enabled can 
> only have KVM_HC_MAP_GPA_RANGE bit set (so that there will be no 
> functional change).
I think it's not needed, because is_kvm_hc_exit_enabled() takes the input.
It just replaces the opencode with a helper API.

Maybe your comment was for the patch 1?

>
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
>
Thanks for your review.

Re: [PATCH v2 2/2] KVM: x86: Use is_kvm_hc_exit_enabled() instead of opencode

[Huang, Kai]

On 19/08/2024 10:07 pm, Binbin Wu wrote:
> 
> 
> 
> On 8/14/2024 7:18 AM, Huang, Kai wrote:
>>
>>
>> On 13/08/2024 5:12 pm, Binbin Wu wrote:
>>> Use is_kvm_hc_exit_enabled() instead of opencode.
>>>
>>> No functional change intended.
>>
>> It would be helpful to mention currently hypercall_exit_enabled can 
>> only have KVM_HC_MAP_GPA_RANGE bit set (so that there will be no 
>> functional change).
> I think it's not needed, because is_kvm_hc_exit_enabled() takes the input.
> It just replaces the opencode with a helper API.
> 
> Maybe your comment was for the patch 1?
> 

OK.  I guess my brain wasn't very clear, feel free to ignore :-)