[PATCH kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM

[PATCH kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM

[Paolo Bonzini]

We will allow this behavior for KVM in some specific cases
(CR8 load/store exits enabled, virtualize APIC accesses
disabled).  Ensure these specific values of the controls
are there in the VMCS, and remove the XFAIL.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/vmx_tests.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index ab7e8cc..0ca5363 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -3669,9 +3669,17 @@ static void test_msr_bitmap(void)
  */
 static void test_apic_virt_addr(void)
 {
+	/*
+	 * Ensure the processor will never use the virtual-APIC page, since
+	 * we will point it to invalid RAM.  Otherwise KVM is puzzled about
+	 * what we're trying to achieve and fails vmentry.
+	 */
+	u32 cpu_ctrls0 = vmcs_read(CPU_EXEC_CTRL0);
+	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0 | CPU_CR8_LOAD | CPU_CR8_STORE);
 	test_vmcs_addr_reference(CPU_TPR_SHADOW, APIC_VIRT_ADDR,
 				 "virtual-APIC address", "Use TPR shadow",
-				 PAGE_SIZE, true, true);
+				 PAGE_SIZE, false, true);
+	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0);
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM

[Krish Sadhukhan]

On 04/15/2019 06:10 AM, Paolo Bonzini wrote:
> We will allow this behavior for KVM in some specific cases
> (CR8 load/store exits enabled, virtualize APIC accesses
> disabled).

I am wondering whether it will be useful to mention here the reason for 
allowing such an exception.

>   Ensure these specific values of the controls
> are there in the VMCS, and remove the XFAIL.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   x86/vmx_tests.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index ab7e8cc..0ca5363 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -3669,9 +3669,17 @@ static void test_msr_bitmap(void)
>    */
>   static void test_apic_virt_addr(void)
>   {
> +	/*
> +	 * Ensure the processor will never use the virtual-APIC page, since
> +	 * we will point it to invalid RAM.  Otherwise KVM is puzzled about
> +	 * what we're trying to achieve and fails vmentry.
> +	 */
> +	u32 cpu_ctrls0 = vmcs_read(CPU_EXEC_CTRL0);
> +	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0 | CPU_CR8_LOAD | CPU_CR8_STORE);

Even though "virtualize APIC accesses" is not set by default, it might 
be cleaner to unset it explicitly here in the test, in case someone sets 
it prior to the execution of this piece of VMX controls test and forgets 
to unset it.

>   	test_vmcs_addr_reference(CPU_TPR_SHADOW, APIC_VIRT_ADDR,
>   				 "virtual-APIC address", "Use TPR shadow",
> -				 PAGE_SIZE, true, true);
> +				 PAGE_SIZE, false, true);
> +	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0);
>   }
>   
>   /*

Re: [PATCH kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM

[Paolo Bonzini]

On 17/04/19 02:11, Krish Sadhukhan wrote:
>> We will allow this behavior for KVM in some specific cases
>> (CR8 load/store exits enabled, virtualize APIC accesses
>> disabled).
> 
> I am wondering whether it will be useful to mention here the reason for allowing such an exception.

It's mentioned in the test: these are the only cases where it's actually
possible to emulate a virtual-APIC address that points outside RAM
(because in this case the processor will never use it and L0 can simply
ignore that L1 has set the TPR-shadow execution control).

>>   static void test_apic_virt_addr(void)
>>   {
>> +    /*
>> +     * Ensure the processor will never use the virtual-APIC page, since
>> +     * we will point it to invalid RAM.  Otherwise KVM is puzzled about
>> +     * what we're trying to achieve and fails vmentry.
>> +     */
>> +    u32 cpu_ctrls0 = vmcs_read(CPU_EXEC_CTRL0);
>> +    vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0 | CPU_CR8_LOAD |
>> CPU_CR8_STORE);
> 
> Even though "virtualize APIC accesses" is not set by default, it might
> be cleaner to unset it explicitly here in the test, in case someone sets
> it prior to the execution of this piece of VMX controls test and forgets
> to unset it.

If that happened, it would break a lot of things, I think.  You're right
that it's not very clean, but the right way to do it would be to start
each test from scratch with an entirely new VMCS.  Until then, I'm more
inclined to make things fail loudly if somebody doesn't respect the
invariants and leaves dirty control bits at the end of a test.

Thanks,

Paolo

Re: [PATCH kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM

[Krish Sadhukhan]

On 4/17/19 5:00 AM, Paolo Bonzini wrote:
> On 17/04/19 02:11, Krish Sadhukhan wrote:
>>> We will allow this behavior for KVM in some specific cases
>>> (CR8 load/store exits enabled, virtualize APIC accesses
>>> disabled).
>> I am wondering whether it will be useful to mention here the reason for allowing such an exception.
> It's mentioned in the test: these are the only cases where it's actually
> possible to emulate a virtual-APIC address that points outside RAM
> (because in this case the processor will never use it and L0 can simply
> ignore that L1 has set the TPR-shadow execution control).


Slightly unrelated topic. This test was the only one that was setting 
'xfail'. With your changes, 'xfail' will not be used anywhere anymore.  
Should we remove it then ?


>
>>>    static void test_apic_virt_addr(void)
>>>    {
>>> +    /*
>>> +     * Ensure the processor will never use the virtual-APIC page, since
>>> +     * we will point it to invalid RAM.  Otherwise KVM is puzzled about
>>> +     * what we're trying to achieve and fails vmentry.
>>> +     */
>>> +    u32 cpu_ctrls0 = vmcs_read(CPU_EXEC_CTRL0);
>>> +    vmcs_write(CPU_EXEC_CTRL0, cpu_ctrls0 | CPU_CR8_LOAD |
>>> CPU_CR8_STORE);
>> Even though "virtualize APIC accesses" is not set by default, it might
>> be cleaner to unset it explicitly here in the test, in case someone sets
>> it prior to the execution of this piece of VMX controls test and forgets
>> to unset it.
> If that happened, it would break a lot of things, I think.  You're right
> that it's not very clean, but the right way to do it would be to start
> each test from scratch with an entirely new VMCS.  Until then, I'm more
> inclined to make things fail loudly if somebody doesn't respect the
> invariants and leaves dirty control bits at the end of a test.
>
> Thanks,
>
> Paolo


Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>