On Mon, Mar 21, 2016 at 2:00 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
>
> On 21/03/2016 20:59, David Matlack wrote:
>> On Sat, Mar 19, 2016 at 2:52 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>> Check that it doesn't cause an infinite stream of vmexits.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> ---
>>> x86/vmx.h | 9 +++++++--
>>> x86/vmx_tests.c | 26 ++++++++++++++++++++++++++
>>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/x86/vmx.h b/x86/vmx.h
>>> index 0cb995d..146f082 100644
>>> --- a/x86/vmx.h
>>> +++ b/x86/vmx.h
>>> @@ -549,12 +549,17 @@ static inline int vmcs_save(struct vmcs **vmcs)
>>> return ret;
>>> }
>>>
>>> -static inline void invept(unsigned long type, u64 eptp)
>>> +static inline bool invept(unsigned long type, u64 eptp)
>>> {
>>> + bool ret;
>>> + u64 rflags = read_rflags() | X86_EFLAGS_CF | X86_EFLAGS_ZF;
>>
>> Why explicitly set these bits prior to invept?
>
> Cargo culting to some extent, but it does check that a successful INVEPT
> clears the bits.
Good point.
>
>>> +
>>> struct {
>>> u64 eptp, gpa;
>>> } operand = {eptp, 0};
>>> - asm volatile("invept %0, %1\n" ::"m"(operand),"r"(type));
>>> + asm volatile("push %1; popf; invept %2, %3; setbe %0"
>>> + : "=q" (ret) : "r" (rflags), "m"(operand),"r"(type) : "cc");
>>> + return ret;
>>> }
>>>
>>> static inline void invvpid(unsigned long type, u16 vpid, u64 gva)
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index 0145cad..dd3af58 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -1029,6 +1029,28 @@ t1:
>>> // Test EPT access to L1 MMIO
>>> vmx_set_test_stage(6);
>>> report("EPT - MMIO access", *((u32 *)0xfee00030UL) == apic_version);
>>> +
>>> + // Test invalid operand for INVEPT
>>> + vmcall();
>>> + report("EPT - unsupported INVEPT", vmx_get_test_stage() == 7);
>>> +}
>>> +
>>> +bool invept_test(int type, u64 eptp)
>>> +{
>>> + bool ret, supported;
>>> +
>>> + supported = ept_vpid.val & (EPT_CAP_INVEPT_SINGLE >> INVEPT_SINGLE << type);
>>> + ret = invept(type, eptp);
>>> +
>>> + if (ret == !supported)
>>> + return false;
>>
>> Nit: Is there a precedent for using bool as the return type and
>> indicating success with false? I found this a little confusing; the
>> pattern I'm more familiar with is an int return type and 0 for
>> success.
>
> It's modeled against the opcode wrappers in vmx.h (for example invept).
>
> I don't really like bool return values indeed (I've probably complained
> about them several times in the past for KVM code...), but it's the
> common convention in VMX tests, see for example
>
> if (vmx_on()) {
> printf("%s : vmxon failed.\n", __func__);
> return 1;
> }
>
> in vmx.c.
It makes sense to maintain consistency with the rest of the file.
Thanks for the explanation!
>
>
> Paolo
>
>>> +
>>> + if (!supported)
>>> + printf("WARNING: unsupported invept passed!\n");
>>> + else
>>> + printf("WARNING: invept failed!\n");
>>> +
>>> + return true;
>>> }
>>>
>>> static int ept_exit_handler()
>>> @@ -1084,6 +1106,10 @@ static int ept_exit_handler()
>>> data_page1_pte_pte & (~EPT_PRESENT));
>>> ept_sync(INVEPT_SINGLE, eptp);
>>> break;
>>> + case 6:
>>> + if (!invept_test(0, eptp))
>>> + vmx_inc_test_stage();
>>> + break;
>>> // Should not reach here
>>> default:
>>> printf("ERROR - unexpected stage, %d.\n",
>>> --
>>> 2.5.0
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
