On Thu, Dec 6, 2018 at 4:51 PM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
>
> On 12/06/2018 03:02 PM, Jim Mattson wrote:
> > On Thu, Dec 6, 2018 at 10:46 AM Krish Sadhukhan
> > <krish.sadhukhan@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx>
> >> ---
> >> 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
