Patchwork [kvm-unit-test,3/3] KVM nVMX: Check VM-exit MSR-store address on vmentry of L2 guests

login
register
mail settings
Submitter Krish Sadhukhan
Date Dec. 6, 2018, 6:20 p.m.
Message ID <20181206182055.16927-4-krish.sadhukhan@oracle.com>
Download mbox | patch
Permalink /patch/674555/
State New
Headers show

Comments

Krish Sadhukhan - Dec. 6, 2018, 6:20 p.m.
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
following checks performed for the VM-exit MSR-store address if the
the VM-exit MSR-store count field is non-zero:

    - The lower 4 bits of the VM-exit MSR-store address must be 0.
      The address should not set any bits beyond the processor’s
      physical-address width.

    - The address of the last byte in the VM-exit MSR-store area
      should not set any bits beyond the processor’s physical-address
      width. The address of this last byte is VM-exit MSR-store address
      + (MSR count * 16) - 1. (The arithmetic used for the computation
      uses more bits than the processor’s physical-address width.)

      If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
      in the range 63:32.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
---
 x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)
Jim Mattson - Dec. 6, 2018, 11:02 p.m.
On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> following checks performed for the VM-exit MSR-store address if the
> the VM-exit MSR-store count field is non-zero:
>
>     - The lower 4 bits of the VM-exit MSR-store address must be 0.
>       The address should not set any bits beyond the processor’s
>       physical-address width.
>
>     - The address of the last byte in the VM-exit MSR-store area
>       should not set any bits beyond the processor’s physical-address
>       width. The address of this last byte is VM-exit MSR-store address
>       + (MSR count * 16) - 1. (The arithmetic used for the computation
>       uses more bits than the processor’s physical-address width.)
>
>       If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
>       in the range 63:32.

I think there are several existing tests that have ignored this
footnote. Perhaps you could fix this omission in a general way in a
separate patch?

> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 900dcf4..af856cf 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -4731,6 +4731,75 @@ static void test_pml(void)
>  }
>
>  /*
> + * The following checks are performed for the VM-exit MSR-store address if
> + * the VM-exit MSR-store count field is non-zero:
> + *
> + *    - The lower 4 bits of the VM-exit MSR-store address must be 0.
> + *      The address should not set any bits beyond the processor’s
> + *      physical-address width.
> + *
> + *    - The address of the last byte in the VM-exit MSR-store area
> + *      should not set any bits beyond the processor’s physical-address
> + *      width. The address of this last byte is VM-exit MSR-store address
> + *      + (MSR count * 16) - 1. (The arithmetic used for the computation
> + *      uses more bits than the processor’s physical-address width.)
> + *
> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
> + * in the range 63:32.
> + *
> + *  [Intel SDM]
> + */
> +static void test_exit_msr_store(void)
> +{
> +       exit_msr_store = alloc_page();
> +       u64 tmp;
> +       u32 exit_msr_st_cnt = 1;
> +       int i;
> +       bool ctrl = true;
> +       u32 addr_len = 64;
> +
> +       vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
> +
> +       /* Check first 4 bits of VM-exit MSR-store address */
> +       for (i = 0; i < 4; i++) {
> +               tmp = (u64)exit_msr_store | 1ull << i;
> +               vmcs_write(EXIT_MSR_ST_ADDR, tmp);
> +               report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
> +                                   tmp & 0xf);
> +               if (i && ctrl == true) {
> +                       ctrl = false;
> +               }
> +               test_vmx_controls(false, false);
> +               report_prefix_pop();
> +       }
> +
> +       if (basic.val & (1ul << 48))
> +               addr_len = 32;
> +
> +       test_vmcs_page_values("VM-exit-MSR-store address",
> +                               EXIT_MSR_ST_ADDR, false, false, 16,
> +                               4, addr_len - 1);
> +
> +       /* Check last byte of VM-exit MSR-store address */
> +       for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {

Why the different initializers? Even when IA32_VMX_BASIC[48] is set,
the *end* of the MSR store buffer doesn't have to fit in 32 bits.

> +               exit_msr_store = (struct vmx_msr_entry *)
> +                       (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
> +                       1ul << i);
> +
> +               vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
> +               test_vmx_controls(false, false);
> +       }

I could just be confused, but I don't think this is testing what you
think it's testing.

The constraints are that the entire msr_store buffer must reside
within the bounds of physically addressable memory. Thus, with
EXI_MSR_ST_CNT set to 1, it might be interesting to test
EXIT_MSR_ST_ADDR values of, say:
(1ULL << cpuid_maxphyaddr()) - 15 [FAIL]
(1ULL << cpuid_maxphyaddr()) - 16 [PASS]
(1ULL << cpuid_maxphyaddr()) - 17 [PASS]

> +}
> +
> +/*
> + * Tests for VMX exit controls
> + */
> +static void test_exit_ctls(void)
> +{
> +       test_exit_msr_store();
> +}
> +
> +/*
>   * Check that the virtual CPU checks all of the VMX controls as
>   * documented in the Intel SDM.
>   */
> @@ -4756,6 +4825,7 @@ static void vmx_controls_test(void)
>         test_invalid_event_injection();
>         test_vpid();
>         test_eptp();
> +       test_exit_ctls();
>  }
>
>  static bool valid_vmcs_for_vmentry(void)
> --
> 2.9.5
>
Krish Sadhukhan - Dec. 7, 2018, 12:51 a.m.
On 12/06/2018 03:02 PM, Jim Mattson wrote:
> On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
>> following checks performed for the VM-exit MSR-store address if the
>> the VM-exit MSR-store count field is non-zero:
>>
>>      - The lower 4 bits of the VM-exit MSR-store address must be 0.
>>        The address should not set any bits beyond the processor’s
>>        physical-address width.
>>
>>      - The address of the last byte in the VM-exit MSR-store area
>>        should not set any bits beyond the processor’s physical-address
>>        width. The address of this last byte is VM-exit MSR-store address
>>        + (MSR count * 16) - 1. (The arithmetic used for the computation
>>        uses more bits than the processor’s physical-address width.)
>>
>>        If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
>>        in the range 63:32.
> I think there are several existing tests that have ignored this
> footnote. Perhaps you could fix this omission in a general way in a
> separate patch?

Will do.

>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>> ---
>>   x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 900dcf4..af856cf 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -4731,6 +4731,75 @@ static void test_pml(void)
>>   }
>>
>>   /*
>> + * The following checks are performed for the VM-exit MSR-store address if
>> + * the VM-exit MSR-store count field is non-zero:
>> + *
>> + *    - The lower 4 bits of the VM-exit MSR-store address must be 0.
>> + *      The address should not set any bits beyond the processor’s
>> + *      physical-address width.
>> + *
>> + *    - The address of the last byte in the VM-exit MSR-store area
>> + *      should not set any bits beyond the processor’s physical-address
>> + *      width. The address of this last byte is VM-exit MSR-store address
>> + *      + (MSR count * 16) - 1. (The arithmetic used for the computation
>> + *      uses more bits than the processor’s physical-address width.)
>> + *
>> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
>> + * in the range 63:32.
>> + *
>> + *  [Intel SDM]
>> + */
>> +static void test_exit_msr_store(void)
>> +{
>> +       exit_msr_store = alloc_page();
>> +       u64 tmp;
>> +       u32 exit_msr_st_cnt = 1;
>> +       int i;
>> +       bool ctrl = true;
>> +       u32 addr_len = 64;
>> +
>> +       vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
>> +
>> +       /* Check first 4 bits of VM-exit MSR-store address */
>> +       for (i = 0; i < 4; i++) {
>> +               tmp = (u64)exit_msr_store | 1ull << i;
>> +               vmcs_write(EXIT_MSR_ST_ADDR, tmp);
>> +               report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
>> +                                   tmp & 0xf);
>> +               if (i && ctrl == true) {
>> +                       ctrl = false;
>> +               }
>> +               test_vmx_controls(false, false);
>> +               report_prefix_pop();
>> +       }
>> +
>> +       if (basic.val & (1ul << 48))
>> +               addr_len = 32;
>> +
>> +       test_vmcs_page_values("VM-exit-MSR-store address",
>> +                               EXIT_MSR_ST_ADDR, false, false, 16,
>> +                               4, addr_len - 1);
>> +
>> +       /* Check last byte of VM-exit MSR-store address */
>> +       for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {
> Why the different initializers? Even when IA32_VMX_BASIC[48] is set,
> the *end* of the MSR store buffer doesn't have to fit in 32 bits.

I am not sure I follow it...

According to the footnote, setting any bit in [63:32] in the address of 
the last byte is illegal if IA32_VMX_BASIC[48] is set and according to 
appendix A-1 in SDM vol 3D:

     "If the bit is 0, these addresses are limited to the processor’s 
physical-address width. If the bit is 1, these addresses are limited to 
32 bits. This bit is always 0 for processors that support Intel 64 
architecture."

  Therefore, on x86_64, the addresses can set any bits in [40:0] but on 
x86_32, they are limited to [31:0].

Am I missing something ?

>
>> +               exit_msr_store = (struct vmx_msr_entry *)
>> +                       (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
>> +                       1ul << i);
>> +
>> +               vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
>> +               test_vmx_controls(false, false);
>> +       }
> I could just be confused, but I don't think this is testing what you
> think it's testing.

I have verified that the following check in nested_vmx_check_msr_switch(),

         (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr)


passes when a bit in [63:40] is set in the address of the last byte of 
the MSR-store address.

Am I missing anything ?

>
> The constraints are that the entire msr_store buffer must reside
> within the bounds of physically addressable memory. Thus, with
> EXI_MSR_ST_CNT set to 1, it might be interesting to test
> EXIT_MSR_ST_ADDR values of, say:
> (1ULL << cpuid_maxphyaddr()) - 15 [FAIL]
> (1ULL << cpuid_maxphyaddr()) - 16 [PASS]
> (1ULL << cpuid_maxphyaddr()) - 17 [PASS]

I can certainly add these cases. BTW, the last one (17) on your list is 
not a PASS case because it sets the low 4 bits in the MSR-store address 
itself (and not address of the last byte) and that's not legal.

>
>> +}
>> +
>> +/*
>> + * Tests for VMX exit controls
>> + */
>> +static void test_exit_ctls(void)
>> +{
>> +       test_exit_msr_store();
>> +}
>> +
>> +/*
>>    * Check that the virtual CPU checks all of the VMX controls as
>>    * documented in the Intel SDM.
>>    */
>> @@ -4756,6 +4825,7 @@ static void vmx_controls_test(void)
>>          test_invalid_event_injection();
>>          test_vpid();
>>          test_eptp();
>> +       test_exit_ctls();
>>   }
>>
>>   static bool valid_vmcs_for_vmentry(void)
>> --
>> 2.9.5
>>
Jim Mattson - Dec. 7, 2018, 5:57 p.m.
On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
>
> On 12/06/2018 03:02 PM, Jim Mattson wrote:
> > On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> >> following checks performed for the VM-exit MSR-store address if the
> >> the VM-exit MSR-store count field is non-zero:
> >>
> >>      - The lower 4 bits of the VM-exit MSR-store address must be 0.
> >>        The address should not set any bits beyond the processor’s
> >>        physical-address width.
> >>
> >>      - The address of the last byte in the VM-exit MSR-store area
> >>        should not set any bits beyond the processor’s physical-address
> >>        width. The address of this last byte is VM-exit MSR-store address
> >>        + (MSR count * 16) - 1. (The arithmetic used for the computation
> >>        uses more bits than the processor’s physical-address width.)
> >>
> >>        If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
> >>        in the range 63:32.
> > I think there are several existing tests that have ignored this
> > footnote. Perhaps you could fix this omission in a general way in a
> > separate patch?
>
> Will do.
>
> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
> >> ---
> >>   x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 70 insertions(+)
> >>
> >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> >> index 900dcf4..af856cf 100644
> >> --- a/x86/vmx_tests.c
> >> +++ b/x86/vmx_tests.c
> >> @@ -4731,6 +4731,75 @@ static void test_pml(void)
> >>   }
> >>
> >>   /*
> >> + * The following checks are performed for the VM-exit MSR-store address if
> >> + * the VM-exit MSR-store count field is non-zero:
> >> + *
> >> + *    - The lower 4 bits of the VM-exit MSR-store address must be 0.
> >> + *      The address should not set any bits beyond the processor’s
> >> + *      physical-address width.
> >> + *
> >> + *    - The address of the last byte in the VM-exit MSR-store area
> >> + *      should not set any bits beyond the processor’s physical-address
> >> + *      width. The address of this last byte is VM-exit MSR-store address
> >> + *      + (MSR count * 16) - 1. (The arithmetic used for the computation
> >> + *      uses more bits than the processor’s physical-address width.)
> >> + *
> >> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
> >> + * in the range 63:32.
> >> + *
> >> + *  [Intel SDM]
> >> + */
> >> +static void test_exit_msr_store(void)
> >> +{
> >> +       exit_msr_store = alloc_page();
> >> +       u64 tmp;
> >> +       u32 exit_msr_st_cnt = 1;
> >> +       int i;
> >> +       bool ctrl = true;
> >> +       u32 addr_len = 64;
> >> +
> >> +       vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
> >> +
> >> +       /* Check first 4 bits of VM-exit MSR-store address */
> >> +       for (i = 0; i < 4; i++) {
> >> +               tmp = (u64)exit_msr_store | 1ull << i;
> >> +               vmcs_write(EXIT_MSR_ST_ADDR, tmp);
> >> +               report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
> >> +                                   tmp & 0xf);
> >> +               if (i && ctrl == true) {
> >> +                       ctrl = false;
> >> +               }
> >> +               test_vmx_controls(false, false);
> >> +               report_prefix_pop();
> >> +       }
> >> +
> >> +       if (basic.val & (1ul << 48))
> >> +               addr_len = 32;
> >> +
> >> +       test_vmcs_page_values("VM-exit-MSR-store address",
> >> +                               EXIT_MSR_ST_ADDR, false, false, 16,
> >> +                               4, addr_len - 1);
> >> +
> >> +       /* Check last byte of VM-exit MSR-store address */
> >> +       for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {
> > Why the different initializers? Even when IA32_VMX_BASIC[48] is set,
> > the *end* of the MSR store buffer doesn't have to fit in 32 bits.
>
> I am not sure I follow it...
>
> According to the footnote, setting any bit in [63:32] in the address of
> the last byte is illegal if IA32_VMX_BASIC[48] is set and according to
> appendix A-1 in SDM vol 3D:
>
>      "If the bit is 0, these addresses are limited to the processor’s
> physical-address width. If the bit is 1, these addresses are limited to
> 32 bits. This bit is always 0 for processors that support Intel 64
> architecture."
>
>   Therefore, on x86_64, the addresses can set any bits in [40:0] but on
> x86_32, they are limited to [31:0].
>
> Am I missing something ?

My bad. Re-reading the SDM, the 32-bit constraint clearly applies to
both ends of the buffer.

> >
> >> +               exit_msr_store = (struct vmx_msr_entry *)
> >> +                       (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
> >> +                       1ul << i);
> >> +
> >> +               vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
> >> +               test_vmx_controls(false, false);
> >> +       }
> > I could just be confused, but I don't think this is testing what you
> > think it's testing.
>
> I have verified that the following check in nested_vmx_check_msr_switch(),
>
>          (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr)
>
>
> passes when a bit in [63:40] is set in the address of the last byte of
> the MSR-store address.
>
> Am I missing anything ?

If the initial exit_msr_store address is 0, then you are testing the
following addresses when addr_len ==32:

10000000f
30000001e
70000002d
f0000003c
1f0000004b
3f0000005a
7f00000069
ff00000078
1ff00000087
3ff00000096
7ff000000a5
fff000000b4
1fff000000c3
3fff000000d2
7fff000000e1
ffff000000f0
1ffff000000ff
3ffff0000010e
7ffff0000011d
fffff0000012c
1fffff0000013b
3fffff0000014a
7fffff00000159
ffffff00000168
1ffffff00000177
3ffffff00000186
7ffffff00000195
fffffff000001a4
1fffffff000001b3
3fffffff000001c2
7fffffff000001d1
ffffffff000001e0

These addresses seem strange to me.

If addr_len == 64 and cpuid_maxphyaddr() is 46, you are testing:

40000000000f
c0000000001e
1c0000000002d
3c0000000003c
7c0000000004b
fc0000000005a
1fc00000000069
3fc00000000078
7fc00000000087
ffc00000000096
1ffc000000000a5
3ffc000000000b4
7ffc000000000c3
fffc000000000d2
1fffc000000000e1
3fffc000000000f0
7fffc000000000ff
ffffc0000000010e

These addresses also seem strange to me.

> >
> > The constraints are that the entire msr_store buffer must reside
> > within the bounds of physically addressable memory. Thus, with
> > EXI_MSR_ST_CNT set to 1, it might be interesting to test
> > EXIT_MSR_ST_ADDR values of, say:
> > (1ULL << cpuid_maxphyaddr()) - 15 [FAIL]
> > (1ULL << cpuid_maxphyaddr()) - 16 [PASS]
> > (1ULL << cpuid_maxphyaddr()) - 17 [PASS]
>
> I can certainly add these cases. BTW, the last one (17) on your list is
> not a PASS case because it sets the low 4 bits in the MSR-store address
> itself (and not address of the last byte) and that's not legal.

Okay. Given the multiple constraints, I'd suggest setting the store
count to 2 and testing:

(1ULL << cpuid_maxphyaddr()) - 48
(1ULL << cpuid_maxphyaddr()) - 32
(1ULL << cpuid_maxphyaddr()) - 16
Krish Sadhukhan - Dec. 8, 2018, 1:52 a.m.
On 12/07/2018 09:57 AM, Jim Mattson wrote:
> On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>>
>>
>> On 12/06/2018 03:02 PM, Jim Mattson wrote:
>>> On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
>>> <krish.sadhukhan@oracle.com> wrote:
>>>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
>>>> following checks performed for the VM-exit MSR-store address if the
>>>> the VM-exit MSR-store count field is non-zero:
>>>>
>>>>       - The lower 4 bits of the VM-exit MSR-store address must be 0.
>>>>         The address should not set any bits beyond the processor’s
>>>>         physical-address width.
>>>>
>>>>       - The address of the last byte in the VM-exit MSR-store area
>>>>         should not set any bits beyond the processor’s physical-address
>>>>         width. The address of this last byte is VM-exit MSR-store address
>>>>         + (MSR count * 16) - 1. (The arithmetic used for the computation
>>>>         uses more bits than the processor’s physical-address width.)
>>>>
>>>>         If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
>>>>         in the range 63:32.
>>> I think there are several existing tests that have ignored this
>>> footnote. Perhaps you could fix this omission in a general way in a
>>> separate patch?
>> Will do.
>>
>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com>
>>>> ---
>>>>    x86/vmx_tests.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 70 insertions(+)
>>>>
>>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>>> index 900dcf4..af856cf 100644
>>>> --- a/x86/vmx_tests.c
>>>> +++ b/x86/vmx_tests.c
>>>> @@ -4731,6 +4731,75 @@ static void test_pml(void)
>>>>    }
>>>>
>>>>    /*
>>>> + * The following checks are performed for the VM-exit MSR-store address if
>>>> + * the VM-exit MSR-store count field is non-zero:
>>>> + *
>>>> + *    - The lower 4 bits of the VM-exit MSR-store address must be 0.
>>>> + *      The address should not set any bits beyond the processor’s
>>>> + *      physical-address width.
>>>> + *
>>>> + *    - The address of the last byte in the VM-exit MSR-store area
>>>> + *      should not set any bits beyond the processor’s physical-address
>>>> + *      width. The address of this last byte is VM-exit MSR-store address
>>>> + *      + (MSR count * 16) - 1. (The arithmetic used for the computation
>>>> + *      uses more bits than the processor’s physical-address width.)
>>>> + *
>>>> + * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
>>>> + * in the range 63:32.
>>>> + *
>>>> + *  [Intel SDM]
>>>> + */
>>>> +static void test_exit_msr_store(void)
>>>> +{
>>>> +       exit_msr_store = alloc_page();
>>>> +       u64 tmp;
>>>> +       u32 exit_msr_st_cnt = 1;
>>>> +       int i;
>>>> +       bool ctrl = true;
>>>> +       u32 addr_len = 64;
>>>> +
>>>> +       vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
>>>> +
>>>> +       /* Check first 4 bits of VM-exit MSR-store address */
>>>> +       for (i = 0; i < 4; i++) {
>>>> +               tmp = (u64)exit_msr_store | 1ull << i;
>>>> +               vmcs_write(EXIT_MSR_ST_ADDR, tmp);
>>>> +               report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
>>>> +                                   tmp & 0xf);
>>>> +               if (i && ctrl == true) {
>>>> +                       ctrl = false;
>>>> +               }
>>>> +               test_vmx_controls(false, false);
>>>> +               report_prefix_pop();
>>>> +       }
>>>> +
>>>> +       if (basic.val & (1ul << 48))
>>>> +               addr_len = 32;
>>>> +
>>>> +       test_vmcs_page_values("VM-exit-MSR-store address",
>>>> +                               EXIT_MSR_ST_ADDR, false, false, 16,
>>>> +                               4, addr_len - 1);
>>>> +
>>>> +       /* Check last byte of VM-exit MSR-store address */
>>>> +       for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {
>>> Why the different initializers? Even when IA32_VMX_BASIC[48] is set,
>>> the *end* of the MSR store buffer doesn't have to fit in 32 bits.
>> I am not sure I follow it...
>>
>> According to the footnote, setting any bit in [63:32] in the address of
>> the last byte is illegal if IA32_VMX_BASIC[48] is set and according to
>> appendix A-1 in SDM vol 3D:
>>
>>       "If the bit is 0, these addresses are limited to the processor’s
>> physical-address width. If the bit is 1, these addresses are limited to
>> 32 bits. This bit is always 0 for processors that support Intel 64
>> architecture."
>>
>>    Therefore, on x86_64, the addresses can set any bits in [40:0] but on
>> x86_32, they are limited to [31:0].
>>
>> Am I missing something ?
> My bad. Re-reading the SDM, the 32-bit constraint clearly applies to
> both ends of the buffer.
>
>>>> +               exit_msr_store = (struct vmx_msr_entry *)
>>>> +                       (((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
>>>> +                       1ul << i);
>>>> +
>>>> +               vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
>>>> +               test_vmx_controls(false, false);
>>>> +       }
>>> I could just be confused, but I don't think this is testing what you
>>> think it's testing.
>> I have verified that the following check in nested_vmx_check_msr_switch(),
>>
>>           (addr + count * sizeof(struct vmx_msr_entry) - 1) >> maxphyaddr)
>>
>>
>> passes when a bit in [63:40] is set in the address of the last byte of
>> the MSR-store address.
>>
>> Am I missing anything ?
> If the initial exit_msr_store address is 0, then you are testing the
> following addresses when addr_len ==32:

exit_msr_store is allocated and is not 0 in this part of the test. For 
example, on my x86_64 system, I see the following addresses being tested:

0x1000047900f
0x3000047901e
0x7000047902d
0xf000047903c
0x1f000047904b
0x3f000047905a
0x7f0000479069
0xff0000479078
0x1ff0000479087
0x3ff0000479096
0x7ff00004790a5
0xfff00004790b4
0x1fff00004790c3
0x3fff00004790d2
0x7fff00004790e1
0xffff00004790f0
0x1ffff00004790ff
0x3ffff000047910e
0x7ffff000047911d
0xfffff000047912c
0x1fffff000047913b
0x3fffff000047914a
0x7fffff0000479159
0xffffff0000479168


Here, my goal is to use a real address (returned by alloc_page()) for 
the starting byte of exit_msr_store and then make the address of the 
last byte invalid by setting a bit in [63:40] in the latter.

>
> 10000000f
> 30000001e
> 70000002d
> f0000003c
> 1f0000004b
> 3f0000005a
> 7f00000069
> ff00000078
> 1ff00000087
> 3ff00000096
> 7ff000000a5
> fff000000b4
> 1fff000000c3
> 3fff000000d2
> 7fff000000e1
> ffff000000f0
> 1ffff000000ff
> 3ffff0000010e
> 7ffff0000011d
> fffff0000012c
> 1fffff0000013b
> 3fffff0000014a
> 7fffff00000159
> ffffff00000168
> 1ffffff00000177
> 3ffffff00000186
> 7ffffff00000195
> fffffff000001a4
> 1fffffff000001b3
> 3fffffff000001c2
> 7fffffff000001d1
> ffffffff000001e0
>
> These addresses seem strange to me.
>
> If addr_len == 64 and cpuid_maxphyaddr() is 46, you are testing:
>
> 40000000000f
> c0000000001e
> 1c0000000002d
> 3c0000000003c
> 7c0000000004b
> fc0000000005a
> 1fc00000000069
> 3fc00000000078
> 7fc00000000087
> ffc00000000096
> 1ffc000000000a5
> 3ffc000000000b4
> 7ffc000000000c3
> fffc000000000d2
> 1fffc000000000e1
> 3fffc000000000f0
> 7fffc000000000ff
> ffffc0000000010e
>
> These addresses also seem strange to me.
>
>>> The constraints are that the entire msr_store buffer must reside
>>> within the bounds of physically addressable memory. Thus, with
>>> EXI_MSR_ST_CNT set to 1, it might be interesting to test
>>> EXIT_MSR_ST_ADDR values of, say:
>>> (1ULL << cpuid_maxphyaddr()) - 15 [FAIL]
>>> (1ULL << cpuid_maxphyaddr()) - 16 [PASS]
>>> (1ULL << cpuid_maxphyaddr()) - 17 [PASS]
>> I can certainly add these cases. BTW, the last one (17) on your list is
>> not a PASS case because it sets the low 4 bits in the MSR-store address
>> itself (and not address of the last byte) and that's not legal.
> Okay. Given the multiple constraints, I'd suggest setting the store
> count to 2 and testing:
>
> (1ULL << cpuid_maxphyaddr()) - 48
> (1ULL << cpuid_maxphyaddr()) - 32
> (1ULL << cpuid_maxphyaddr()) - 16

Will add these cases.
Jim Mattson - Dec. 10, 2018, 5:26 p.m.
On Fri, Dec 7, 2018 at 5:52 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:

> exit_msr_store is allocated and is not 0 in this part of the test. For
> example, on my x86_64 system, I see the following addresses being tested:
>
> 0x1000047900f
> 0x3000047901e
> 0x7000047902d
> 0xf000047903c
> 0x1f000047904b
> 0x3f000047905a
> 0x7f0000479069
> 0xff0000479078
> 0x1ff0000479087
> 0x3ff0000479096
> 0x7ff00004790a5
> 0xfff00004790b4
> 0x1fff00004790c3
> 0x3fff00004790d2
> 0x7fff00004790e1
> 0xffff00004790f0
> 0x1ffff00004790ff
> 0x3ffff000047910e
> 0x7ffff000047911d
> 0xfffff000047912c
> 0x1fffff000047913b
> 0x3fffff000047914a
> 0x7fffff0000479159
> 0xffffff0000479168
>
>
> Here, my goal is to use a real address (returned by alloc_page()) for
> the starting byte of exit_msr_store and then make the address of the
> last byte invalid by setting a bit in [63:40] in the latter.

Okay. Given that goal:
1) Why do most of these addresses set more than one bit in [63:40]?
2) Why do most of these addresses set illegal bits in [3:0]?
Krish Sadhukhan - Dec. 10, 2018, 7:07 p.m.
On 12/10/2018 09:26 AM, Jim Mattson wrote:
> On Fri, Dec 7, 2018 at 5:52 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>
>> exit_msr_store is allocated and is not 0 in this part of the test. For
>> example, on my x86_64 system, I see the following addresses being tested:
>>
>> 0x1000047900f
>> 0x3000047901e
>> 0x7000047902d
>> 0xf000047903c
>> 0x1f000047904b
>> 0x3f000047905a
>> 0x7f0000479069
>> 0xff0000479078
>> 0x1ff0000479087
>> 0x3ff0000479096
>> 0x7ff00004790a5
>> 0xfff00004790b4
>> 0x1fff00004790c3
>> 0x3fff00004790d2
>> 0x7fff00004790e1
>> 0xffff00004790f0
>> 0x1ffff00004790ff
>> 0x3ffff000047910e
>> 0x7ffff000047911d
>> 0xfffff000047912c
>> 0x1fffff000047913b
>> 0x3fffff000047914a
>> 0x7fffff0000479159
>> 0xffffff0000479168
>>
>>
>> Here, my goal is to use a real address (returned by alloc_page()) for
>> the starting byte of exit_msr_store and then make the address of the
>> last byte invalid by setting a bit in [63:40] in the latter.
> Okay. Given that goal:
> 1) Why do most of these addresses set more than one bit in [63:40]?
> 2) Why do most of these addresses set illegal bits in [3:0]?

Sorry for the oversight ! Both of these need to be fixed. Thanks !

Patch

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 900dcf4..af856cf 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -4731,6 +4731,75 @@  static void test_pml(void)
 }
 
 /*
+ * The following checks are performed for the VM-exit MSR-store address if
+ * the VM-exit MSR-store count field is non-zero:
+ *
+ *    - The lower 4 bits of the VM-exit MSR-store address must be 0.
+ *      The address should not set any bits beyond the processor’s
+ *      physical-address width.
+ *
+ *    - The address of the last byte in the VM-exit MSR-store area
+ *      should not set any bits beyond the processor’s physical-address
+ *      width. The address of this last byte is VM-exit MSR-store address
+ *      + (MSR count * 16) - 1. (The arithmetic used for the computation
+ *      uses more bits than the processor’s physical-address width.)
+ *
+ * If IA32_VMX_BASIC[48] is read as 1, neither address should set any bits
+ * in the range 63:32.
+ *
+ *  [Intel SDM]
+ */
+static void test_exit_msr_store(void)
+{
+	exit_msr_store = alloc_page();
+	u64 tmp;
+	u32 exit_msr_st_cnt = 1;
+	int i;
+	bool ctrl = true;
+	u32 addr_len = 64;
+
+	vmcs_write(EXI_MSR_ST_CNT, exit_msr_st_cnt);
+
+	/* Check first 4 bits of VM-exit MSR-store address */
+	for (i = 0; i < 4; i++) {
+		tmp = (u64)exit_msr_store | 1ull << i;
+		vmcs_write(EXIT_MSR_ST_ADDR, tmp);
+		report_prefix_pushf("VM-exit MSR-store addr [4:0] %lx",
+				    tmp & 0xf);
+		if (i && ctrl == true) {
+			ctrl = false;
+		}
+		test_vmx_controls(false, false);
+		report_prefix_pop();
+	}
+
+	if (basic.val & (1ul << 48))
+		addr_len = 32;
+
+	test_vmcs_page_values("VM-exit-MSR-store address",
+				EXIT_MSR_ST_ADDR, false, false, 16,
+				4, addr_len - 1);
+
+	/* Check last byte of VM-exit MSR-store address */
+	for (i = (addr_len == 64 ? cpuid_maxphyaddr() : addr_len); i < 64; i++) {
+		exit_msr_store = (struct vmx_msr_entry *)
+			(((u64)exit_msr_store + (exit_msr_st_cnt * 16) - 1) |
+			1ul << i);
+
+		vmcs_write(EXIT_MSR_ST_ADDR, (u64)exit_msr_store);
+		test_vmx_controls(false, false);
+	}
+}
+
+/*
+ * Tests for VMX exit controls
+ */
+static void test_exit_ctls(void)
+{
+	test_exit_msr_store();
+}
+
+/*
  * Check that the virtual CPU checks all of the VMX controls as
  * documented in the Intel SDM.
  */
@@ -4756,6 +4825,7 @@  static void vmx_controls_test(void)
 	test_invalid_event_injection();
 	test_vpid();
 	test_eptp();
+	test_exit_ctls();
 }
 
 static bool valid_vmcs_for_vmentry(void)