Re: [PATCH 1/1 v2] nSVM: Test effect of host RFLAGS.TF on VMRUN

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 2/5/21 12:14 AM, Paolo Bonzini wrote:
> On 05/02/21 00:29, Krish Sadhukhan wrote:
>>
>> +static void host_rflags_prepare(struct svm_test *test)
>> +{
>> +    default_prepare(test);
>> +    handle_exception(DB_VECTOR, host_rflags_db_handler);
>> +    set_test_stage(test, 0);
>> +    /*
>> +     * We trigger a #UD in order to find out the RIP of VMRUN 
>> instruction
>> +     */
>> +    wrmsr(MSR_EFER, rdmsr(MSR_EFER) & ~EFER_SVME);
>> +    handle_exception(UD_VECTOR, host_rflags_ud_handler);
>> +}
>> +
>
> I think you'd get the RIP of VMLOAD, not VMRUN.
>
> Maybe something like:
>
> diff --git a/x86/svm.c b/x86/svm.c
> index a1808c7..88d8452 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -208,14 +208,14 @@ struct regs get_regs(void)
>
>  struct svm_test *v2_test;
>
> -#define ASM_VMRUN_CMD                           \
> +#define ASM_PRE_VMRUN                           \
>                  "vmload %%rax\n\t"              \
>                  "mov regs+0x80, %%r15\n\t"      \
>                  "mov %%r15, 0x170(%%rax)\n\t"   \
>                  "mov regs, %%r15\n\t"           \
>                  "mov %%r15, 0x1f8(%%rax)\n\t"   \
> -                LOAD_GPR_C                      \
> -                "vmrun %%rax\n\t"               \
> +                LOAD_GPR_C
> +#define ASM_POST_VMRUN                          \
>                  SAVE_GPR_C                      \
>                  "mov 0x170(%%rax), %%r15\n\t"   \
>                  "mov %%r15, regs+0x80\n\t"      \
> @@ -232,7 +232,9 @@ int svm_vmrun(void)
>      regs.rdi = (ulong)v2_test;
>
>      asm volatile (
> -        ASM_VMRUN_CMD
> +        ASM_PRE_VMRUN
> +                "vmrun %%rax\n\t"
> +        ASM_POST_VMRUN
>          :
>          : "a" (virt_to_phys(vmcb))
>          : "memory", "r15");
> @@ -240,6 +242,7 @@ int svm_vmrun(void)
>      return (vmcb->control.exit_code);
>  }
>
> +extern unsigned char vmrun_rip;
>  static void test_run(struct svm_test *test)
>  {
>      u64 vmcb_phys = virt_to_phys(vmcb);
> @@ -258,7 +261,9 @@ static void test_run(struct svm_test *test)
>              "sti \n\t"
>              "call *%c[PREPARE_GIF_CLEAR](%[test]) \n \t"
>              "mov %[vmcb_phys], %%rax \n\t"
> -            ASM_VMRUN_CMD
> +                ASM_PRE_VMRUN
> +                        "vmrun_rip: vmrun %%rax\n\t"
> +                ASM_POST_VMRUN
>              "cli \n\t"
>              "stgi"
>              : // inputs clobbered by the guest:
>
> (untested)
>
> Paolo
>

Thanks for the suggestion. I have implemented it in v3 that I have sent out.

The reason why my test (v2) had passed, was because virtual 
VMLOAD/VMSAVE is enabled by default and no #VMEXIT happens due to 
intercepts being disabled (via init_vmcb()). So my #UD handler didn't 
get invoked on VMLOAD but got invoked on VMRUN.

This brings out an interesting point. When intercept for VMLOAD/VMSAVE 
is disabled, there is no effect of unsetting EFER.SVME on VMLOAD/VMLOAD, 
even thought APM vol 2 says it should generate a #UD. I couldn't find 
any text in the APM that describes the effect of unsetting EFER.SVME on 
the virtual VMLOAD/VMSAVE. Is this the expected behavior of the SVM 
hardware or is this a bug in KVM and KVM should handle this ?

I plan to add some more tests based on the correct behavior of virtual 
VMLOAD/VMSAVE.