| Message ID | 20240813051256.2246612-2-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Check hypercall's exit to userspace generically | expand |
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 > >
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 > > > > >
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?
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__) | ^
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.
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().
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().
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.
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.
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.
>>>>> +{ >>>>> + 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.
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().
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
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(-)