On 07/03/19 23:05, Sean Christopherson wrote:
> On Fri, Feb 22, 2019 at 09:37:34PM +0100, Paolo Bonzini wrote:
>> On 18/02/19 01:57, Krish Sadhukhan wrote:
>>> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
>>> following checks are performed on vmentry of L2 guests:
>>>
>>> - The CR0 field must not set any bit to a value not supported in VMX
>>> operation.
>>> - The CR4 field must not set any bit to a value not supported in VMX
>>> operation.
>>> - On processors that support Intel 64 architecture, the CR3 field must
>>> be such that bits 63:52 and bits in the range 51:32 beyond the
>>> processor’s physical-address width must be 0.
>>>
>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
>>> Reviewed-by: Liam Merwick <liam.merwick@xxxxxxxxxx>
>>> ---
>>> lib/x86/processor.h | 1 +
>>> x86/vmx_tests.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 95 insertions(+)
>>>
>>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>>> index 15237a5..916e67d 100644
>>> --- a/lib/x86/processor.h
>>> +++ b/lib/x86/processor.h
>>> @@ -40,6 +40,7 @@
>>> #define X86_CR4_UMIP 0x00000800
>>> #define X86_CR4_VMXE 0x00002000
>>> #define X86_CR4_PCIDE 0x00020000
>>> +#define X86_CR4_SMEP 0x00100000
>>> #define X86_CR4_SMAP 0x00200000
>>> #define X86_CR4_PKE 0x00400000
>>>
>>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>>> index 66a87f6..f5324ad 100644
>>> --- a/x86/vmx_tests.c
>>> +++ b/x86/vmx_tests.c
>>> @@ -4995,6 +4995,98 @@ static void test_sysenter_field(u32 field, const char *name)
>>> vmcs_write(field, addr_saved);
>>> }
>>>
>>> +static void test_ctl_reg(const char *cr_name, u64 cr, u64 fixed0, u64 fixed1)
>>> +{
>>> + u64 val;
>>> + u64 cr_saved = vmcs_read(cr);
>>> + int i;
>>> +
>>> + val = fixed0 & fixed1;
>>> + if (cr == HOST_CR4)
>>> + vmcs_write(cr, val | X86_CR4_PAE);
>>> + else
>>> + vmcs_write(cr, val);
>>> + report_prefix_pushf("%s %lx", cr_name, val);
>>> + if (val == fixed0)
>>> + test_vmx_vmlaunch(0, false);
>>> + else
>>> + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> + false);
>>> + report_prefix_pop();
>>> +
>>> + for (i = 0; i < 64; i++) {
>>> +
>>> + /* Set a bit when the corresponding bit in fixed1 is 0 */
>>> + if ((fixed1 & (1ull << i)) == 0) {
>>> + if (cr == HOST_CR4 && ((1ull << i) & X86_CR4_SMEP ||
>>> + (1ull << i) & X86_CR4_SMAP))
>>> + continue;
>>> +
>>> + vmcs_write(cr, cr_saved | (1ull << i));
>>> + report_prefix_pushf("%s %llx", cr_name,
>>> + cr_saved | (1ull << i));
>>> + test_vmx_vmlaunch(
>>> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> + false);
>>> + report_prefix_pop();
>>> + }
>>> +
>>> + /* Unset a bit when the corresponding bit in fixed0 is 1 */
>>> + if (fixed0 & (1ull << i)) {
>>> + vmcs_write(cr, cr_saved & ~(1ull << i));
>>> + report_prefix_pushf("%s %llx", cr_name,
>>> + cr_saved & ~(1ull << i));
>>> + test_vmx_vmlaunch(
>>> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> + false);
>>> + report_prefix_pop();
>>> + }
>>> + }
>>> +
>>> + vmcs_write(cr, cr_saved);
>>> +}
>>> +
>>> +/*
>>> + * 1. The CR0 field must not set any bit to a value not supported in VMX
>>> + * operation.
>>> + * 2. The CR4 field must not set any bit to a value not supported in VMX
>>> + * operation.
>>> + * 3. On processors that support Intel 64 architecture, the CR3 field must
>>> + * be such that bits 63:52 and bits in the range 51:32 beyond the
>>> + * processor’s physical-address width must be 0.
>>> + *
>>> + * [Intel SDM]
>>> + */
>>> +static void test_host_ctl_regs(void)
>>> +{
>>> + u64 fixed0, fixed1, cr3, cr3_saved;
>>> + int i;
>>> +
>>> + /* Test CR0 */
>>> + fixed0 = rdmsr(MSR_IA32_VMX_CR0_FIXED0);
>>> + fixed1 = rdmsr(MSR_IA32_VMX_CR0_FIXED1);
>>> + test_ctl_reg("HOST_CR0", HOST_CR0, fixed0, fixed1);
>>> +
>>> + /* Test CR4 */
>>> + fixed0 = rdmsr(MSR_IA32_VMX_CR4_FIXED0);
>>> + fixed1 = rdmsr(MSR_IA32_VMX_CR4_FIXED1) &
>>> + ~(X86_CR4_SMEP | X86_CR4_SMAP);
>>> + test_ctl_reg("HOST_CR4", HOST_CR4, fixed0, fixed1);
>>> +
>>> + /* Test CR3 */
>>> + cr3_saved = vmcs_read(HOST_CR3);
>>> + for (i = cpuid_maxphyaddr(); i < 64; i++) {
>>> + cr3 = cr3_saved | (1ul << i);
>>> + vmcs_write(HOST_CR3, cr3);
>>> + report_prefix_pushf("HOST_CR3 %lx", cr3);
>>> + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>>> + false);
>>> + report_prefix_pop();
>>> + }
>>> +
>>> + vmcs_write(HOST_CR3, cr3_saved);
>>> +}
>>> +
>>> /*
>>> * Check that the virtual CPU checks the VMX Host State Area as
>>> * documented in the Intel SDM.
>>> @@ -5008,6 +5100,8 @@ static void vmx_host_state_area_test(void)
>>> */
>>> vmcs_write(GUEST_RFLAGS, 0);
>>>
>>> + test_host_ctl_regs();
>>> +
>>> test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP");
>>> test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
>>> }
>>>
>>
>> Applied, thanks.
>>
>> Paolo
>
> The entirety of test_ctl_reg() was dropped when this was applied. It's
> still the head commit, maybe you can just do a fixup?
>
> x86/vmx_tests.c: In function ‘test_host_ctl_regs’:
> x86/vmx_tests.c:5111:2: error: implicit declaration of function ‘test_ctl_reg’ [-Werror=implicit-function-declaration]
> test_ctl_reg("HOST_CR0", HOST_CR0, fixed0, fixed1);
> ^
> x86/vmx_tests.c: At top level:
> cc1: error: unrecognized command line option ‘-Wno-frame-address’ [-Werror]
>
Yup, amended.
Paolo
