Re: [kvm-unit-tests PATCH v3 15/27] svm: move some svm support functions into lib/x86/svm_lib.h

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2022-12-01 at 14:59 +0100, Emanuele Giuseppe Esposito wrote:
> 
> Am 22/11/2022 um 17:11 schrieb Maxim Levitsky:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  lib/x86/svm_lib.h | 53 +++++++++++++++++++++++++++++++++++++++++++++++
> >  x86/svm.c         | 36 +-------------------------------
> >  x86/svm.h         | 18 ----------------
> >  x86/svm_npt.c     |  1 +
> >  x86/svm_tests.c   |  1 +
> >  5 files changed, 56 insertions(+), 53 deletions(-)
> >  create mode 100644 lib/x86/svm_lib.h
> > 
> > diff --git a/lib/x86/svm_lib.h b/lib/x86/svm_lib.h
> > new file mode 100644
> > index 00000000..04910281
> > --- /dev/null
> > +++ b/lib/x86/svm_lib.h
> > @@ -0,0 +1,53 @@
> > +#ifndef SRC_LIB_X86_SVM_LIB_H_
> > +#define SRC_LIB_X86_SVM_LIB_H_
> > +
> > +#include <x86/svm.h>
> > +#include "processor.h"
> > +
> > +static inline bool npt_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_NPT);
> > +}
> > +
> > +static inline bool vgif_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_VGIF);
> > +}
> > +
> > +static inline bool lbrv_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_LBRV);
> > +}
> > +
> > +static inline bool tsc_scale_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_TSCRATEMSR);
> > +}
> > +
> > +static inline bool pause_filter_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_PAUSEFILTER);
> > +}
> > +
> > +static inline bool pause_threshold_supported(void)
> > +{
> > +	return this_cpu_has(X86_FEATURE_PFTHRESHOLD);
> > +}
> > +
> > +static inline void vmmcall(void)
> > +{
> > +	asm volatile ("vmmcall" : : : "memory");
> > +}
> > +
> > +static inline void stgi(void)
> > +{
> > +	asm volatile ("stgi");
> > +}
> > +
> > +static inline void clgi(void)
> > +{
> > +	asm volatile ("clgi");
> > +}
> > +
> Not an expert at all on this, but sti() and cli() in patch 1 are in
> processor.h and stgi (g stansd for global?) and clgi are in a different
> header? What about maybe moving them together?

Well the GI (global interrupt flag) is AMD specific, and even more correctly
SVM specific as well. Same for VMMCALL (Intel has VMCALL instead).

Best regards,
	Maxim Levitsky
> 
> > +
> > +#endif /* SRC_LIB_X86_SVM_LIB_H_ */
> > diff --git a/x86/svm.c b/x86/svm.c
> > index 0b2a1d69..8d90a242 100644
> > --- a/x86/svm.c
> > +++ b/x86/svm.c
> > @@ -14,6 +14,7 @@
> >  #include "alloc_page.h"
> >  #include "isr.h"
> >  #include "apic.h"
> > +#include "svm_lib.h"
> >  
> >  /* for the nested page table*/
> >  u64 *pml4e;
> > @@ -54,32 +55,6 @@ bool default_supported(void)
> >  	return true;
> >  }
> >  
> > -bool vgif_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_VGIF);
> > -}
> > -
> > -bool lbrv_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_LBRV);
> > -}
> > -
> > -bool tsc_scale_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_TSCRATEMSR);
> > -}
> > -
> > -bool pause_filter_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_PAUSEFILTER);
> > -}
> > -
> > -bool pause_threshold_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_PFTHRESHOLD);
> > -}
> > -
> > -
> >  void default_prepare(struct svm_test *test)
> >  {
> >  	vmcb_ident(vmcb);
> > @@ -94,10 +69,6 @@ bool default_finished(struct svm_test *test)
> >  	return true; /* one vmexit */
> >  }
> >  
> > -bool npt_supported(void)
> > -{
> > -	return this_cpu_has(X86_FEATURE_NPT);
> > -}
> >  
> >  int get_test_stage(struct svm_test *test)
> >  {
> > @@ -128,11 +99,6 @@ static void vmcb_set_seg(struct vmcb_seg *seg, u16 selector,
> >  	seg->base = base;
> >  }
> >  
> > -inline void vmmcall(void)
> > -{
> > -	asm volatile ("vmmcall" : : : "memory");
> > -}
> > -
> >  static test_guest_func guest_main;
> >  
> >  void test_set_guest(test_guest_func func)
> > diff --git a/x86/svm.h b/x86/svm.h
> > index 3cd7ce8b..7cb1b898 100644
> > --- a/x86/svm.h
> > +++ b/x86/svm.h
> > @@ -53,21 +53,14 @@ u64 *npt_get_pdpe(u64 address);
> >  u64 *npt_get_pml4e(void);
> >  bool smp_supported(void);
> >  bool default_supported(void);
> > -bool vgif_supported(void);
> > -bool lbrv_supported(void);
> > -bool tsc_scale_supported(void);
> > -bool pause_filter_supported(void);
> > -bool pause_threshold_supported(void);
> >  void default_prepare(struct svm_test *test);
> >  void default_prepare_gif_clear(struct svm_test *test);
> >  bool default_finished(struct svm_test *test);
> > -bool npt_supported(void);
> >  int get_test_stage(struct svm_test *test);
> >  void set_test_stage(struct svm_test *test, int s);
> >  void inc_test_stage(struct svm_test *test);
> >  void vmcb_ident(struct vmcb *vmcb);
> >  struct regs get_regs(void);
> > -void vmmcall(void);
> >  int __svm_vmrun(u64 rip);
> >  void __svm_bare_vmrun(void);
> >  int svm_vmrun(void);
> > @@ -75,17 +68,6 @@ void test_set_guest(test_guest_func func);
> >  
> >  extern struct vmcb *vmcb;
> >  
> > -static inline void stgi(void)
> > -{
> > -    asm volatile ("stgi");
> > -}
> > -
> > -static inline void clgi(void)
> > -{
> > -    asm volatile ("clgi");
> > -}
> > -
> > -
> >  
> >  #define SAVE_GPR_C                              \
> >          "xchg %%rbx, regs+0x8\n\t"              \
> > diff --git a/x86/svm_npt.c b/x86/svm_npt.c
> > index b791f1ac..8aac0bb6 100644
> > --- a/x86/svm_npt.c
> > +++ b/x86/svm_npt.c
> > @@ -2,6 +2,7 @@
> >  #include "vm.h"
> >  #include "alloc_page.h"
> >  #include "vmalloc.h"
> > +#include "svm_lib.h"
> >  
> >  static void *scratch_page;
> >  
> > diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> > index 202e9271..f86c2fa4 100644
> > --- a/x86/svm_tests.c
> > +++ b/x86/svm_tests.c
> > @@ -12,6 +12,7 @@
> >  #include "delay.h"
> >  #include "x86/usermode.h"
> >  #include "vmalloc.h"
> > +#include "svm_lib.h"
> >  
> >  #define SVM_EXIT_MAX_DR_INTERCEPT 0x3f
> >  
> >