Yes, that works for me. An alternative I've toyed with is modifying
nested_get_vmcs12_pages to generate a VM-exit for "illegal guest
state" rather than a VM-instruction failure for "VM entry with invalid
control field(s)" when kvm_vcpu_gpa_to_page(vcpu,
vmcs12->virtual_apic_page_addr) returns an error page. Still wrong,
but the kvm-unit-test isn't yet smart enough to notice the error.
On Fri, Sep 15, 2017 at 9:40 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> On 15/09/2017 01:31, Jim Mattson wrote:
>> When the APIC virtualization address references unbacked memory, kvm
>> incorrectly fails a VM-entry with "invalid control field(s)." Until
>> this can be fixed, just skip the VMX control field tests that populate
>> a VMCS field with a physical address that isn't backed.
>>
>> Tested: <multi line explanation of how the code was tested>
>>
>> Effort: <Your effort group>
>> Google-Bug-Id: <buganizer-id>
>> Change-Id: I624091cb5cd7cfaae887de8f017e18b5f95d8f61
>> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>> ---
>> x86/vmx_tests.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 088d32aaee0b..e82bc04d72e2 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -3359,6 +3359,12 @@ static void test_vmcs_page_addr(const char *name,
>> bool ignored,
>> u64 addr)
>> {
>> + /*
>> + * Skip tests that reference unbacked memory for now, because
>> + * kvm doesn't always handle this situation correctly.
>
> Since in practice it's only 16 tests that fail, it would be nice to only
> XFAIL those. The following is a bit intrusive, but it works. What do
> you think?
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 729e85f..13b45dd 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -3177,14 +3177,14 @@ success:
> /*
> * Try to launch the current VMCS.
> */
> -static void test_vmx_controls(bool controls_valid)
> +static void test_vmx_controls(bool controls_valid, bool xfail)
> {
> bool success = vmlaunch_succeeds();
> u32 vmx_inst_err;
>
> - report("vmlaunch %s", success == controls_valid,
> - controls_valid ? "succeeds" : "fails");
> - if (!controls_valid) {
> + report_xfail("vmlaunch %s", xfail, success == controls_valid,
> + controls_valid ? "succeeds" : "fails");
> + if (!success) {
> vmx_inst_err = vmcs_read(VMX_INST_ERROR);
> report("VMX inst error is %d (actual %d)",
> vmx_inst_err == VMXERR_ENTRY_INVALID_CONTROL_FIELD,
> @@ -3226,7 +3226,7 @@ static void test_rsvd_ctl_bit_value(const char *name, union vmx_ctrl_msr msr,
> vmcs_write(encoding, msr.set & ~mask);
> expected = !(msr.set & mask);
> }
> - test_vmx_controls(expected);
> + test_vmx_controls(expected, false);
> vmcs_write(encoding, controls);
> report_prefix_pop();
> }
> @@ -3322,7 +3322,7 @@ static void try_cr3_target_count(unsigned i, unsigned max)
> {
> report_prefix_pushf("CR3 target count 0x%x", i);
> vmcs_write(CR3_TARGET_COUNT, i);
> - test_vmx_controls(i <= max);
> + test_vmx_controls(i <= max, false);
> report_prefix_pop();
> }
>
> @@ -3357,13 +3357,21 @@ static void test_cr3_targets(void)
> static void test_vmcs_page_addr(const char *name,
> enum Encoding encoding,
> bool ignored,
> + bool xfail_beyond_mapped_ram,
> u64 addr)
> {
> + bool xfail =
> + (xfail_beyond_mapped_ram &&
> + addr > fwcfg_get_u64(FW_CFG_RAM_SIZE) - PAGE_SIZE &&
> + addr < (1ul << cpuid_maxphyaddr()));
> +
> report_prefix_pushf("%s = %lx", name, addr);
> vmcs_write(encoding, addr);
> test_vmx_controls(ignored || (IS_ALIGNED(addr, PAGE_SIZE) &&
> - addr < (1ul << cpuid_maxphyaddr())));
> + addr < (1ul << cpuid_maxphyaddr())),
> + xfail);
> report_prefix_pop();
> + xfail = false;
> }
>
> /*
> @@ -3371,19 +3379,26 @@ static void test_vmcs_page_addr(const char *name,
> */
> static void test_vmcs_page_values(const char *name,
> enum Encoding encoding,
> - bool ignored)
> + bool ignored,
> + bool xfail_beyond_mapped_ram)
> {
> unsigned i;
> u64 orig_val = vmcs_read(encoding);
>
> for (i = 0; i < 64; i++)
> - test_vmcs_page_addr(name, encoding, ignored, 1ul << i);
> + test_vmcs_page_addr(name, encoding, ignored,
> + xfail_beyond_mapped_ram, 1ul << i);
>
> - test_vmcs_page_addr(name, encoding, ignored, PAGE_SIZE - 1);
> - test_vmcs_page_addr(name, encoding, ignored, PAGE_SIZE);
> + test_vmcs_page_addr(name, encoding, ignored,
> + xfail_beyond_mapped_ram, PAGE_SIZE - 1);
> + test_vmcs_page_addr(name, encoding, ignored,
> + xfail_beyond_mapped_ram, PAGE_SIZE);
> test_vmcs_page_addr(name, encoding, ignored,
> + xfail_beyond_mapped_ram,
> (1ul << cpuid_maxphyaddr()) - PAGE_SIZE);
> - test_vmcs_page_addr(name, encoding, ignored, -1ul);
> + test_vmcs_page_addr(name, encoding, ignored,
> + xfail_beyond_mapped_ram,
> + -1ul);
>
> vmcs_write(encoding, orig_val);
> }
> @@ -3394,7 +3409,8 @@ static void test_vmcs_page_values(const char *name,
> */
> static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
> const char *field_name,
> - const char *control_name)
> + const char *control_name,
> + bool xfail_beyond_mapped_ram)
> {
> u32 primary = vmcs_read(CPU_EXEC_CTRL0);
> u64 page_addr;
> @@ -3406,12 +3422,12 @@ static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
>
> report_prefix_pushf("%s enabled", control_name);
> vmcs_write(CPU_EXEC_CTRL0, primary | control_bit);
> - test_vmcs_page_values(field_name, field, false);
> + test_vmcs_page_values(field_name, field, false, xfail_beyond_mapped_ram);
> report_prefix_pop();
>
> report_prefix_pushf("%s disabled", control_name);
> vmcs_write(CPU_EXEC_CTRL0, primary & ~control_bit);
> - test_vmcs_page_values(field_name, field, true);
> + test_vmcs_page_values(field_name, field, true, false);
> report_prefix_pop();
>
> vmcs_write(field, page_addr);
> @@ -3427,9 +3443,9 @@ static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
> static void test_io_bitmaps(void)
> {
> test_vmcs_page_reference(CPU_IO_BITMAP, IO_BITMAP_A,
> - "I/O bitmap A", "Use I/O bitmaps");
> + "I/O bitmap A", "Use I/O bitmaps", false);
> test_vmcs_page_reference(CPU_IO_BITMAP, IO_BITMAP_B,
> - "I/O bitmap B", "Use I/O bitmaps");
> + "I/O bitmap B", "Use I/O bitmaps", false);
> }
>
> /*
> @@ -3441,7 +3457,7 @@ static void test_io_bitmaps(void)
> static void test_msr_bitmap(void)
> {
> test_vmcs_page_reference(CPU_MSR_BITMAP, MSR_BITMAP,
> - "MSR bitmap", "Use MSR bitmaps");
> + "MSR bitmap", "Use MSR bitmaps", false);
> }
>
> /*
> @@ -3455,7 +3471,7 @@ static void test_msr_bitmap(void)
> static void test_apic_virt_addr(void)
> {
> test_vmcs_page_reference(CPU_TPR_SHADOW, APIC_VIRT_ADDR,
> - "virtual-APIC address", "Use TPR shadow");
> + "virtual-APIC address", "Use TPR shadow", true);
> }
>
> static void try_tpr_threshold(unsigned val)
> @@ -3468,7 +3484,7 @@ static void try_tpr_threshold(unsigned val)
> valid = !(val >> 4);
> report_prefix_pushf("TPR threshold 0x%x", val);
> vmcs_write(TPR_THRESHOLD, val);
> - test_vmx_controls(valid);
> + test_vmx_controls(valid, false);
> report_prefix_pop();
> }
>
>
> Paolo
