Re: [PATCH kvm-unit-tests] nSVM: test canonicalization of segment bases in VMLOAD

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

On 05/08/21 10:57, Lara Lazier wrote:
> APM2 states that VMRUN and VMLOAD should canonicalize all base
> addresses in the segment registers that have been loaded respectively.
>
> Split up in test_canonicalization the TEST_CANONICAL for VMLOAD and
> VMRUN. Added the respective test for KERNEL_GS.
>
> Signed-off-by: Lara Lazier <laramglazier@xxxxxxxxx>
> ---
>   x86/svm_tests.c | 51 +++++++++++++++++++++++++++++++++----------------
>   1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/x86/svm_tests.c b/x86/svm_tests.c
> index 7c7b19d..f6bccb7 100644
> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -2460,32 +2460,51 @@ static void test_msrpm_iopm_bitmap_addrs(void)
>   	vmcb->control.intercept = saved_intercept;
>   }
>   
> -#define TEST_CANONICAL(seg_base, msg)					\
> -	saved_addr = seg_base;						\
> +#define TEST_CANONICAL_VMRUN(seg_base, msg)					\
> +	saved_addr = seg_base;					\
>   	seg_base = (seg_base & ((1ul << addr_limit) - 1)) | noncanonical_mask; \
> -	report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test %s.base for canonical form: %lx", msg, seg_base);							\
> +	return_value = svm_vmrun(); \
> +	report(return_value == SVM_EXIT_VMMCALL, \
> +			"Successful VMRUN with noncanonical %s.base", msg); \
> +	report(is_canonical(seg_base), \
> +			"Test %s.base for canonical form: %lx", msg, seg_base); \
> +	seg_base = saved_addr;

Interesting, processors seem not to write back the canonicalized form on 
VMRUN.  They probably remember it has not changed and avoid the 
writeback.  Oh well.

Note that you do not need to reproduce this behavior for QEMU/TCG, as it 
is not even documented.

I removed the second "report" and pushed the patch.

Thanks!

Paolo

> +
> +#define TEST_CANONICAL_VMLOAD(seg_base, msg)					\
> +	saved_addr = seg_base;					\
> +	seg_base = (seg_base & ((1ul << addr_limit) - 1)) | noncanonical_mask; \
> +	asm volatile ("vmload %0" : : "a"(vmcb_phys) : "memory"); \
> +	asm volatile ("vmsave %0" : : "a"(vmcb_phys) : "memory"); \
> +	report(is_canonical(seg_base), \
> +			"Test %s.base for canonical form: %lx", msg, seg_base); \
>   	seg_base = saved_addr;
>   
>   /*
>    * VMRUN canonicalizes (i.e., sign-extend to bit 63) all base addresses
>    • in the segment registers that have been loaded.
>    */
> -static void test_vmrun_canonicalization(void)
> +static void test_canonicalization(void)
>   {
>   	u64 saved_addr;
> -	u8 addr_limit = cpuid_maxphyaddr();
> +	u64 return_value;
> +	u64 addr_limit;
> +	u64 vmcb_phys = virt_to_phys(vmcb);
> +
> +	addr_limit = (this_cpu_has(X86_FEATURE_LA57)) ? 57 : 48;
>   	u64 noncanonical_mask = NONCANONICAL & ~((1ul << addr_limit) - 1);
>   
> -	TEST_CANONICAL(vmcb->save.es.base, "ES");
> -	TEST_CANONICAL(vmcb->save.cs.base, "CS");
> -	TEST_CANONICAL(vmcb->save.ss.base, "SS");
> -	TEST_CANONICAL(vmcb->save.ds.base, "DS");
> -	TEST_CANONICAL(vmcb->save.fs.base, "FS");
> -	TEST_CANONICAL(vmcb->save.gs.base, "GS");
> -	TEST_CANONICAL(vmcb->save.gdtr.base, "GDTR");
> -	TEST_CANONICAL(vmcb->save.ldtr.base, "LDTR");
> -	TEST_CANONICAL(vmcb->save.idtr.base, "IDTR");
> -	TEST_CANONICAL(vmcb->save.tr.base, "TR");
> +	TEST_CANONICAL_VMLOAD(vmcb->save.fs.base, "FS");
> +	TEST_CANONICAL_VMLOAD(vmcb->save.gs.base, "GS");
> +	TEST_CANONICAL_VMLOAD(vmcb->save.ldtr.base, "LDTR");
> +	TEST_CANONICAL_VMLOAD(vmcb->save.tr.base, "TR");
> +	TEST_CANONICAL_VMLOAD(vmcb->save.kernel_gs_base, "KERNEL GS");
> +	TEST_CANONICAL_VMRUN(vmcb->save.es.base, "ES");
> +	TEST_CANONICAL_VMRUN(vmcb->save.cs.base, "CS");
> +	TEST_CANONICAL_VMRUN(vmcb->save.ss.base, "SS");
> +	TEST_CANONICAL_VMRUN(vmcb->save.ds.base, "DS");
> +	TEST_CANONICAL_VMRUN(vmcb->save.gdtr.base, "GDTR");
> +	TEST_CANONICAL_VMRUN(vmcb->save.idtr.base, "IDTR");