| Message ID | 1555333816-17181-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] vmx: do not XFAIL for virtual-APIC address beyond RAM | expand |
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); > } > > /*
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
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>
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); } /*
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(-)