[kvm-unit-tests PATCH] x86/msr.c generalize to any input msr

[kvm-unit-tests PATCH] x86/msr.c generalize to any input msr

[yqwfh]

If an MSR description is provided as input by the user,
run the test against that MSR. This allows the user to
run tests on custom MSR's.

Otherwise run all default tests.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
---
 x86/msr.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 7a551c4..554014e 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -3,6 +3,7 @@
 #include "libcflat.h"
 #include "processor.h"
 #include "msr.h"
+#include <stdlib.h>
 
 struct msr_info {
 	int index;
@@ -77,25 +78,44 @@ static void test_rdmsr_fault(struct msr_info *msr)
 	       "Expected #GP on RDSMR(%s), got vector %d", msr->name, vector);
 }
 
+static void test_msr(struct msr_info *msr, bool is_64bit_host)
+{
+	if (is_64bit_host || !msr->is_64bit_only) {
+		test_msr_rw(msr, msr->value);
+
+		/*
+		 * The 64-bit only MSRs that take an address always perform
+		 * canonical checks on both Intel and AMD.
+		 */
+		if (msr->is_64bit_only &&
+		    msr->value == addr_64)
+			test_wrmsr_fault(msr, NONCANONICAL);
+	} else {
+		test_wrmsr_fault(msr, msr->value);
+		test_rdmsr_fault(msr);
+	}
+}
+
 int main(int ac, char **av)
 {
 	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
 	int i;
 
-	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
-		if (is_64bit_host || !msr_info[i].is_64bit_only) {
-			test_msr_rw(&msr_info[i], msr_info[i].value);
-
-			/*
-			 * The 64-bit only MSRs that take an address always perform
-			 * canonical checks on both Intel and AMD.
-			 */
-			if (msr_info[i].is_64bit_only &&
-			    msr_info[i].value == addr_64)
-				test_wrmsr_fault(&msr_info[i], NONCANONICAL);
-		} else {
-			test_wrmsr_fault(&msr_info[i], msr_info[i].value);
-			test_rdmsr_fault(&msr_info[i]);
+	if (ac == 4) {
+		char msr_name[16];
+		int index = strtoul(av[1], NULL, 0x10);
+		snprintf(msr_name, sizeof(msr_name), "MSR:0x%x", index);
+
+		struct msr_info msr = {
+			.index = index,
+			.name = msr_name,
+			.is_64bit_only = !strcmp(av[3], "0"),
+			.value = strtoul(av[2], NULL, 0x10)
+		};
+		test_msr(&msr, is_64bit_host);
+	} else {
+		for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
+			test_msr(&msr_info[i], is_64bit_host);
 		}
 	}
 
-- 
2.20.1

Re: [kvm-unit-tests PATCH] x86/msr.c generalize to any input msr

[Alexander Graf]

Hi Daniele!

On 10.08.21 16:31, yqwfh wrote:
> If an MSR description is provided as input by the user,
> run the test against that MSR. This allows the user to
> run tests on custom MSR's.
> 
> Otherwise run all default tests.

This patch description is missing the rationale. It comes through 
lightly where you say "This allows the user to run tests on custom 
MSRs", but that still doesn't explain why you would need that functionality.

The most important piece to transmit in the patch description is always 
the "Why". Only after that it sorted, you move on to a quick "How".

> 
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>

Please send the email from the same account that your SoB line quotes :)

It usually also helps to CC people that worked on the same file before. 
Usually, get_maintainers.pl should extract that list automatically for 
you but I realized that there is no such file in the kvm-unit-tests tree 
even though we have a MAINTAINERS one.

Paolo, what is the method you'd prefer to determine who to CC on 
kvm-unit-tests submissions?

> ---
>   x86/msr.c | 48 ++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/x86/msr.c b/x86/msr.c
> index 7a551c4..554014e 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -3,6 +3,7 @@
>   #include "libcflat.h"
>   #include "processor.h"
>   #include "msr.h"
> +#include <stdlib.h>
>   
>   struct msr_info {
>   	int index;
> @@ -77,25 +78,44 @@ static void test_rdmsr_fault(struct msr_info *msr)
>   	       "Expected #GP on RDSMR(%s), got vector %d", msr->name, vector);
>   }
>   
> +static void test_msr(struct msr_info *msr, bool is_64bit_host)
> +{
> +	if (is_64bit_host || !msr->is_64bit_only) {
> +		test_msr_rw(msr, msr->value);
> +
> +		/*
> +		 * The 64-bit only MSRs that take an address always perform
> +		 * canonical checks on both Intel and AMD.
> +		 */
> +		if (msr->is_64bit_only &&
> +		    msr->value == addr_64)
> +			test_wrmsr_fault(msr, NONCANONICAL);
> +	} else {
> +		test_wrmsr_fault(msr, msr->value);
> +		test_rdmsr_fault(msr);
> +	}
> +}
> +

I would prefer if you separate the "extract individual MSR logic into 
function" part from the "Add a new mode of operation to test a 
particular MSR" part into two separate patches. That way it's easier to 
review.

>   int main(int ac, char **av)
>   {
>   	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
>   	int i;
>   
> -	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
> -		if (is_64bit_host || !msr_info[i].is_64bit_only) {
> -			test_msr_rw(&msr_info[i], msr_info[i].value);
> -
> -			/*
> -			 * The 64-bit only MSRs that take an address always perform
> -			 * canonical checks on both Intel and AMD.
> -			 */
> -			if (msr_info[i].is_64bit_only &&
> -			    msr_info[i].value == addr_64)
> -				test_wrmsr_fault(&msr_info[i], NONCANONICAL);
> -		} else {
> -			test_wrmsr_fault(&msr_info[i], msr_info[i].value);
> -			test_rdmsr_fault(&msr_info[i]);
> +	if (ac == 4) {

This means you require 3 completely undocumented command line arguments, 
no? I'm sure even you as the author of this patch will forget what they 
are in 2 years :).

Shouldn't there be some documentation that explains users that and how 
they can use this special mode of operation somewhere?

> +		char msr_name[16];
> +		int index = strtoul(av[1], NULL, 0x10);
> +		snprintf(msr_name, sizeof(msr_name), "MSR:0x%x", index);
> +
> +		struct msr_info msr = {
> +			.index = index,
> +			.name = msr_name,
> +			.is_64bit_only = !strcmp(av[3], "0"),

Why do you need to pass this when you invoke the test manually?

> +			.value = strtoul(av[2], NULL, 0x10)

Overall, the command line parsing is pretty ad hoc and wouldn't scale 
well with updates. Paolo, is there any common theme on command line 
argument passing? Do we do things like command line options? Help texts? 
Do we have something even remotely similar to getopt?


Thanks,

Alex

> +		};
> +		test_msr(&msr, is_64bit_host);
> +	} else {
> +		for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
> +			test_msr(&msr_info[i], is_64bit_host);
>   		}
>   	}
>   
> 




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [kvm-unit-tests PATCH] x86/msr.c generalize to any input msr

[Paolo Bonzini]

On 23/09/21 11:11, Alexander Graf wrote:
> Usually, get_maintainers.pl should extract that list automatically for 
> you but I realized that there is no such file in the kvm-unit-tests tree 
> even though we have a MAINTAINERS one.
> 
> Paolo, what is the method you'd prefer to determine who to CC on 
> kvm-unit-tests submissions?

Let's add a get_maintainers.pl script.  We might also start accepting 
merge requests on gitlab though...

Paolo

[kvm-unit-tests PATCH v2 1/3] lib/string: Add stroull and strtoll

[ahmeddan]

From: Daniele Ahmed <ahmeddan@amazon.com>

Add the two functions strtoull and strtoll.
This is in preparation for an update
in x86/msr.c to write 64b values
that are given as inputs as strings by
a user.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
---
 lib/stdlib.h |  2 ++
 lib/string.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/lib/stdlib.h b/lib/stdlib.h
index 33c00e8..48e10f0 100644
--- a/lib/stdlib.h
+++ b/lib/stdlib.h
@@ -9,5 +9,7 @@
 
 long int strtol(const char *nptr, char **endptr, int base);
 unsigned long int strtoul(const char *nptr, char **endptr, int base);
+long long int strtoll(const char *nptr, char **endptr, int base);
+unsigned long long strtoull(const char *nptr, char **endptr, int base);
 
 #endif /* _STDLIB_H_ */
diff --git a/lib/string.c b/lib/string.c
index ffc7c7e..dacd927 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -242,6 +242,80 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
     return __strtol(nptr, endptr, base, false);
 }
 
+static unsigned long long __strtoll(const char *nptr, char **endptr,
+                              int base, bool is_signed) {
+    unsigned long long acc = 0;
+    const char *s = nptr;
+    int neg, c;
+
+    assert(base == 0 || (base >= 2 && base <= 36));
+
+    while (isspace(*s))
+        s++;
+
+    if (*s == '-') {
+        neg = 1;
+        s++;
+    } else {
+        neg = 0;
+        if (*s == '+')
+            s++;
+    }
+
+    if (base == 0 || base == 16) {
+        if (*s == '0') {
+            s++;
+            if (*s == 'x' || *s == 'X') {
+                 s++;
+                 base = 16;
+            } else if (base == 0)
+                 base = 8;
+        } else if (base == 0)
+            base = 10;
+    }
+
+    while (*s) {
+        if (*s >= '0' && *s < '0' + base && *s <= '9')
+            c = *s - '0';
+        else if (*s >= 'a' && *s < 'a' + base - 10)
+            c = *s - 'a' + 10;
+        else if (*s >= 'A' && *s < 'A' + base - 10)
+            c = *s - 'A' + 10;
+        else
+            break;
+
+        if (is_signed) {
+            long long sacc = (long long)acc;
+            assert(!check_mul_overflow(sacc, base));
+            assert(!check_add_overflow(sacc * base, c));
+        } else {
+            assert(!check_mul_overflow(acc, base));
+            assert(!check_add_overflow(acc * base, c));
+        }
+
+        acc = acc * base + c;
+        s++;
+    }
+
+    if (neg)
+        acc = -acc;
+
+    if (endptr)
+        *endptr = (char *)s;
+
+    return acc;
+}
+
+long long int strtoll(const char *nptr, char **endptr, int base)
+{
+    return __strtoll(nptr, endptr, base, true);
+}
+
+unsigned long long int strtoull(const char *nptr, char **endptr, int base)
+{
+    return __strtoll(nptr, endptr, base, false);
+}
+
 long atol(const char *ptr)
 {
     return strtol(ptr, NULL, 10);
-- 
2.32.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

[kvm-unit-tests PATCH v2 2/3] [kvm-unit-tests PATCH] x86/msr.c refactor out generic test logic

[ahmeddan]

From: Daniele Ahmed <ahmeddan@amazon.com>

Move the generic MSR test logic to its own function.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
---
 x86/msr.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 7a551c4..8931f59 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -77,26 +77,31 @@ static void test_rdmsr_fault(struct msr_info *msr)
 	       "Expected #GP on RDSMR(%s), got vector %d", msr->name, vector);
 }
 
+static void test_msr(struct msr_info *msr, bool is_64bit_host)
+{
+	if (is_64bit_host || !msr->is_64bit_only) {
+		test_msr_rw(msr, msr->value);
+
+		/*
+		 * The 64-bit only MSRs that take an address always perform
+		 * canonical checks on both Intel and AMD.
+		 */
+		if (msr->is_64bit_only &&
+		    msr->value == addr_64)
+			test_wrmsr_fault(msr, NONCANONICAL);
+	} else {
+		test_wrmsr_fault(msr, msr->value);
+		test_rdmsr_fault(msr);
+	}
+}
+
 int main(int ac, char **av)
 {
 	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
 	int i;
 
 	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
-		if (is_64bit_host || !msr_info[i].is_64bit_only) {
-			test_msr_rw(&msr_info[i], msr_info[i].value);
-
-			/*
-			 * The 64-bit only MSRs that take an address always perform
-			 * canonical checks on both Intel and AMD.
-			 */
-			if (msr_info[i].is_64bit_only &&
-			    msr_info[i].value == addr_64)
-				test_wrmsr_fault(&msr_info[i], NONCANONICAL);
-		} else {
-			test_wrmsr_fault(&msr_info[i], msr_info[i].value);
-			test_rdmsr_fault(&msr_info[i]);
-		}
+		test_msr(&msr_info[i], is_64bit_host);
 	}
 
 	return report_summary();
-- 
2.32.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

[kvm-unit-tests PATCH v2 3/3] x86/msr.c generalize to any input msr

[ahmeddan]

From: Daniele Ahmed <ahmeddan@amazon.com>

If an MSR description is provided as input by the user,
run the test against that MSR. This allows the user to
run tests on custom MSR's.

Otherwise run all default tests.

This is to validate custom MSR handling in user space
with an easy-to-use tool. This kvm-unit-test submodule
is a perfect fit. I'm extending it with a mode that
takes an MSR index and a value to test arbitrary MSR accesses.

Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
---
 x86/msr.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/x86/msr.c b/x86/msr.c
index 8931f59..1a2d791 100644
--- a/x86/msr.c
+++ b/x86/msr.c
@@ -3,6 +3,17 @@
 #include "libcflat.h"
 #include "processor.h"
 #include "msr.h"
+#include <stdlib.h>
+
+/**
+ * This test allows two modes:
+ * 1. Default: the `msr_info' array contains the default test configurations
+ * 2. Custom: by providing command line arguments it is possible to test any MSR and value
+ *	Parameters order:
+ *		1. msr index as a base 16 number
+ *		2. value as a base 16 number
+ *		3. "0" if the msr is available only in 64b hosts, any other string otherwise
+ */
 
 struct msr_info {
 	int index;
@@ -100,8 +111,22 @@ int main(int ac, char **av)
 	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
 	int i;
 
-	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
-		test_msr(&msr_info[i], is_64bit_host);
+	if (ac == 4) {
+		char msr_name[16];
+		int index = strtoul(av[1], NULL, 0x10);
+		snprintf(msr_name, sizeof(msr_name), "MSR:0x%x", index);
+
+		struct msr_info msr = {
+			.index = index,
+			.name = msr_name,
+			.is_64bit_only = !strcmp(av[3], "0"),
+			.value = strtoull(av[2], NULL, 0x10)
+		};
+		test_msr(&msr, is_64bit_host);
+	} else {
+		for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
+			test_msr(&msr_info[i], is_64bit_host);
+		}
 	}
 
 	return report_summary();
-- 
2.32.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [kvm-unit-tests PATCH v2 2/3] [kvm-unit-tests PATCH] x86/msr.c refactor out generic test logic

[Alexander Graf]

On 27.09.21 17:30, ahmeddan@amazon.com wrote:
> From: Daniele Ahmed <ahmeddan@amazon.com>
> 
> Move the generic MSR test logic to its own function.
> 
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>

Reviewed-by: Alexander Graf <graf@amazon.com>

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [kvm-unit-tests PATCH v2 3/3] x86/msr.c generalize to any input msr

[Paolo Bonzini]

On 27/09/21 17:30, ahmeddan@amazon.com wrote:
> From: Daniele Ahmed <ahmeddan@amazon.com>
> 
> If an MSR description is provided as input by the user,
> run the test against that MSR. This allows the user to
> run tests on custom MSR's.
> 
> Otherwise run all default tests.
> 
> This is to validate custom MSR handling in user space
> with an easy-to-use tool. This kvm-unit-test submodule
> is a perfect fit. I'm extending it with a mode that
> takes an MSR index and a value to test arbitrary MSR accesses.
> 
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>

I don't understand; is this a debugging tool, or is it meant to be 
driven by another test suite?

I'm not sure this fits the purpose of kvm-unit-tests very well, though. 
  An alternative is BITS (https://github.com/biosbits/bits/), which is 
relatively easy to use and comes with Python bindings to RDMSR/WRMSR.

Paolo

Re: [kvm-unit-tests PATCH v2 3/3] x86/msr.c generalize to any input msr

[Ahmed, Daniele]

Hi Paolo,
This would be to test the handling of specific MSR's, both the purposes you mention.
It may not cover all possible cases. I'm extending the kvm unit tests for others who might have similar needs.
I looked up BITS and it seems like it's been unmaintained for many years now (although it could still be useful).
Let me know if you don't think this is the right place for this change. I've seen other tests that take custom
values to be used inside the tests.
Thank you

On 28/09/2021, 17:37, "Paolo Bonzini" <pbonzini@redhat.com> wrote:

    CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



    On 27/09/21 17:30, ahmeddan@amazon.com wrote:
    > From: Daniele Ahmed <ahmeddan@amazon.com>
    >
    > If an MSR description is provided as input by the user,
    > run the test against that MSR. This allows the user to
    > run tests on custom MSR's.
    >
    > Otherwise run all default tests.
    >
    > This is to validate custom MSR handling in user space
    > with an easy-to-use tool. This kvm-unit-test submodule
    > is a perfect fit. I'm extending it with a mode that
    > takes an MSR index and a value to test arbitrary MSR accesses.
    >
    > Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>

    I don't understand; is this a debugging tool, or is it meant to be
    driven by another test suite?

    I'm not sure this fits the purpose of kvm-unit-tests very well, though.
      An alternative is BITS (https://github.com/biosbits/bits/), which is
    relatively easy to use and comes with Python bindings to RDMSR/WRMSR.

    Paolo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [kvm-unit-tests PATCH v2 1/3] lib/string: Add stroull and strtoll

[Andrew Jones]

On Mon, Sep 27, 2021 at 03:30:26PM +0000, ahmeddan@amazon.com wrote:
> From: Daniele Ahmed <ahmeddan@amazon.com>
> 
> Add the two functions strtoull and strtoll.
> This is in preparation for an update
> in x86/msr.c to write 64b values
> that are given as inputs as strings by
> a user.
> 
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>
> ---
>  lib/stdlib.h |  2 ++
>  lib/string.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/lib/stdlib.h b/lib/stdlib.h
> index 33c00e8..48e10f0 100644
> --- a/lib/stdlib.h
> +++ b/lib/stdlib.h
> @@ -9,5 +9,7 @@
>  
>  long int strtol(const char *nptr, char **endptr, int base);
>  unsigned long int strtoul(const char *nptr, char **endptr, int base);
> +long long int strtoll(const char *nptr, char **endptr, int base);
> +unsigned long long strtoull(const char *nptr, char **endptr, int base);
>  
>  #endif /* _STDLIB_H_ */
> diff --git a/lib/string.c b/lib/string.c
> index ffc7c7e..dacd927 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -242,6 +242,80 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>      return __strtol(nptr, endptr, base, false);
>  }
>  
> +static unsigned long long __strtoll(const char *nptr, char **endptr,
> +                              int base, bool is_signed) {
> +    unsigned long long acc = 0;
> +    const char *s = nptr;
> +    int neg, c;
> +
> +    assert(base == 0 || (base >= 2 && base <= 36));
> +
> +    while (isspace(*s))
> +        s++;
> +
> +    if (*s == '-') {
> +        neg = 1;
> +        s++;
> +    } else {
> +        neg = 0;
> +        if (*s == '+')
> +            s++;
> +    }
> +
> +    if (base == 0 || base == 16) {
> +        if (*s == '0') {
> +            s++;
> +            if (*s == 'x' || *s == 'X') {
> +                 s++;
> +                 base = 16;
> +            } else if (base == 0)
> +                 base = 8;
> +        } else if (base == 0)
> +            base = 10;
> +    }
> +
> +    while (*s) {
> +        if (*s >= '0' && *s < '0' + base && *s <= '9')
> +            c = *s - '0';
> +        else if (*s >= 'a' && *s < 'a' + base - 10)
> +            c = *s - 'a' + 10;
> +        else if (*s >= 'A' && *s < 'A' + base - 10)
> +            c = *s - 'A' + 10;
> +        else
> +            break;
> +
> +        if (is_signed) {
> +            long long sacc = (long long)acc;
> +            assert(!check_mul_overflow(sacc, base));
> +            assert(!check_add_overflow(sacc * base, c));
> +        } else {
> +            assert(!check_mul_overflow(acc, base));
> +            assert(!check_add_overflow(acc * base, c));
> +        }
> +
> +        acc = acc * base + c;
> +        s++;
> +    }
> +
> +    if (neg)
> +        acc = -acc;
> +
> +    if (endptr)
> +        *endptr = (char *)s;
> +
> +    return acc;
> +}
> +
> +long long int strtoll(const char *nptr, char **endptr, int base)
> +{
> +    return __strtoll(nptr, endptr, base, true);
> +}
> +
> +unsigned long long int strtoull(const char *nptr, char **endptr, int base)
> +{
> +    return __strtoll(nptr, endptr, base, false);
> +}
> +
>  long atol(const char *ptr)
>  {
>      return strtol(ptr, NULL, 10);
> -- 
> 2.32.0
> 
>

Hi Daniele,

I just sent an alternative to this patch which doesn't requiring
duplicating as much code.

Thanks,
drew

 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
> 
> 

Re: [kvm-unit-tests PATCH v2 3/3] x86/msr.c generalize to any input msr

[Paolo Bonzini]

On 27/09/21 17:30, ahmeddan@amazon.com wrote:
> From: Daniele Ahmed <ahmeddan@amazon.com>
> 
> If an MSR description is provided as input by the user,
> run the test against that MSR. This allows the user to
> run tests on custom MSR's.
> 
> Otherwise run all default tests.
> 
> This is to validate custom MSR handling in user space
> with an easy-to-use tool. This kvm-unit-test submodule
> is a perfect fit. I'm extending it with a mode that
> takes an MSR index and a value to test arbitrary MSR accesses.
> 
> Signed-off-by: Daniele Ahmed <ahmeddan@amazon.com>

Queued, thanks.  I removed the "64-bit only" functionality because, for 
manual invocation, you can just not run the test when running on 32-bit 
targets.

Paolo

> ---
>   x86/msr.c | 29 +++++++++++++++++++++++++++--
>   1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/msr.c b/x86/msr.c
> index 8931f59..1a2d791 100644
> --- a/x86/msr.c
> +++ b/x86/msr.c
> @@ -3,6 +3,17 @@
>   #include "libcflat.h"
>   #include "processor.h"
>   #include "msr.h"
> +#include <stdlib.h>
> +
> +/**
> + * This test allows two modes:
> + * 1. Default: the `msr_info' array contains the default test configurations
> + * 2. Custom: by providing command line arguments it is possible to test any MSR and value
> + *	Parameters order:
> + *		1. msr index as a base 16 number
> + *		2. value as a base 16 number
> + *		3. "0" if the msr is available only in 64b hosts, any other string otherwise
> + */
>   
>   struct msr_info {
>   	int index;
> @@ -100,8 +111,22 @@ int main(int ac, char **av)
>   	bool is_64bit_host = this_cpu_has(X86_FEATURE_LM);
>   	int i;
>   
> -	for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
> -		test_msr(&msr_info[i], is_64bit_host);
> +	if (ac == 4) {
> +		char msr_name[16];
> +		int index = strtoul(av[1], NULL, 0x10);
> +		snprintf(msr_name, sizeof(msr_name), "MSR:0x%x", index);
> +
> +		struct msr_info msr = {
> +			.index = index,
> +			.name = msr_name,
> +			.is_64bit_only = !strcmp(av[3], "0"),
> +			.value = strtoull(av[2], NULL, 0x10)
> +		};
> +		test_msr(&msr, is_64bit_host);
> +	} else {
> +		for (i = 0 ; i < ARRAY_SIZE(msr_info); i++) {
> +			test_msr(&msr_info[i], is_64bit_host);
> +		}
>   	}
>   
>   	return report_summary();
>