On Thu, 2022-10-20 at 18:21 +0000, Sean Christopherson wrote:
> s/suported/supported
>
> On Thu, Oct 20, 2022, Maxim Levitsky wrote:
>
> Please provide a changelog.
>
> > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > ---
> > lib/x86/svm_lib.h | 5 +++++
> > x86/svm.c | 2 +-
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/x86/svm_lib.h b/lib/x86/svm_lib.h
> > index 04910281..2d13b066 100644
> > --- a/lib/x86/svm_lib.h
> > +++ b/lib/x86/svm_lib.h
> > @@ -4,6 +4,11 @@
> > #include <x86/svm.h>
> > #include "processor.h"
> >
> > +static inline bool svm_supported(void)
> > +{
> > + return this_cpu_has(X86_FEATURE_SVM);
>
> Why add a wrapper? The only reason NPT and a few others have wrappers is to
> play nice with svm_test's "bool (*supported)(void)" hook.
For consistency with other code. The other way around as you suggest is also reasonable.
>
> I would rather go the opposite direction and get rid of the wrappers, which IMO
> only make it harder to understand what is being checked.
>
> E.g. add a required_feature to the tests and use that for all X86_FEATURE_*
> checks instead of adding wrappers. And unless there's a supported helper I'm not
> seeing, the .supported hook can go away entirely by adding a dedicated "smp_required"
> flag.
I rather not add the .required_feature, since tests might want to test for more that one feature.
It rather better (and more visible to user) to have the test itself check for all features
it need at the start of it).
So I rather would remove the .supported() at all.
Best regards,
Maxim Levitsky
>
> We'd probaby want helper macros for SMP vs. non-SMP, e.g.
>
> #define SVM_V1_TEST(name, feature, ...)
> { #name, feature, false, ... }
> #define SVM_SMP_V1_TEST(name, feature, ...)
> { #name, feature, true, ... }
>
> diff --git a/x86/svm.c b/x86/svm.c
> index 7aa3ebd2..2a412c27 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -170,6 +170,7 @@ test_wanted(const char *name, char *filters[], int filter_count)
>
> int run_svm_tests(int ac, char **av, struct svm_test *svm_tests)
> {
> + bool smp_supported = cpu_count() > 1;
> int i = 0;
>
> ac--;
> @@ -187,7 +188,10 @@ int run_svm_tests(int ac, char **av, struct svm_test *svm_tests)
> for (; svm_tests[i].name != NULL; i++) {
> if (!test_wanted(svm_tests[i].name, av, ac))
> continue;
> - if (svm_tests[i].supported && !svm_tests[i].supported())
> + if (svm_tests[i].required_feature &&
> + !this_cpu_has(svm_tests[i].required_feature))
> + continue;
> + if (svm_tests[i].smp_required && !smp_supported)
> continue;
> if (svm_tests[i].v2 == NULL) {
> if (svm_tests[i].on_vcpu) {
> diff --git a/x86/svm.h b/x86/svm.h
> index 0c40a086..632287ca 100644
> --- a/x86/svm.h
> +++ b/x86/svm.h
> @@ -9,7 +9,8 @@
>
> struct svm_test {
> const char *name;
> - bool (*supported)(void);
> + u64 required_feature;
> + bool smp_required;
> void (*prepare)(struct svm_test *test);
> void (*prepare_gif_clear)(struct svm_test *test);
> void (*guest_func)(struct svm_test *test);
>
