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

[Alexander Graf <graf@amazon.com>]

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