x86/vvmx: set CR4 before CR0

Commit Message

Andrew Cooper June 26, 2019, 7:02 p.m. UTC

From: Sergey Dyasli <[e-mail]>

Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2
and vice-versa).

Signed-off-by: Sergey Dyasli <[e-mail]>
Reviewed-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Wei Liu <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
CC: Jun Nakajima <[e-mail]>
CC: Kevin Tian <[e-mail]>

I found this patch languishing in the XenServer patchqueue, and Sergey is OoO
so I'm submitting it on his behalf.

Without this change, nested virt is broken when L1 and L2 differ in their use
of PCID.

This is only a stopgap solution - it resolves the PCID issue without
introducing other issues, but the proper fix needs to consider all control
bits at once, rather than considering a vmentry/exit as a sequence of changes
of discrete registers.
---
 xen/arch/x86/hvm/vmx/vvmx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Roger Pau Monné June 27, 2019, 8:37 a.m. UTC | #1

On Wed, Jun 26, 2019 at 08:02:12PM +0100, Andrew Cooper wrote:
> From: Sergey Dyasli <[e-mail]>
> 
> Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2
> and vice-versa).
> 
> Signed-off-by: Sergey Dyasli <[e-mail]>
> Reviewed-by: Andrew Cooper <[e-mail]>

Reviewed-by: Roger Pau Monné <[e-mail]>

> ---
> CC: Jan Beulich <[e-mail]>
> CC: Wei Liu <[e-mail]>
> CC: Roger Pau Monné <[e-mail]>
> CC: Jun Nakajima <[e-mail]>
> CC: Kevin Tian <[e-mail]>
> 
> I found this patch languishing in the XenServer patchqueue, and Sergey is OoO
> so I'm submitting it on his behalf.
> 
> Without this change, nested virt is broken when L1 and L2 differ in their use
> of PCID.
> 
> This is only a stopgap solution - it resolves the PCID issue without
> introducing other issues, but the proper fix needs to consider all control
> bits at once, rather than considering a vmentry/exit as a sequence of changes
> of discrete registers.

The current approach seems prone to such ordering issues, and I don't
see a way to make it more robust while keeping the current approach,
so I guess setting all the registers state and then evaluating them
would make more sense and prevent this kind of mistakes.

Thanks.

Andrew Cooper June 27, 2019, 1:15 p.m. UTC | #2

On 27/06/2019 09:37, Roger Pau Monné wrote:
> On Wed, Jun 26, 2019 at 08:02:12PM +0100, Andrew Cooper wrote:
>> From: Sergey Dyasli <[e-mail]>
>>
>> Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2
>> and vice-versa).
>>
>> Signed-off-by: Sergey Dyasli <[e-mail]>
>> Reviewed-by: Andrew Cooper <[e-mail]>
> Reviewed-by: Roger Pau Monné <[e-mail]>
>
>> ---
>> CC: Jan Beulich <[e-mail]>
>> CC: Wei Liu <[e-mail]>
>> CC: Roger Pau Monné <[e-mail]>
>> CC: Jun Nakajima <[e-mail]>
>> CC: Kevin Tian <[e-mail]>
>>
>> I found this patch languishing in the XenServer patchqueue, and Sergey is OoO
>> so I'm submitting it on his behalf.
>>
>> Without this change, nested virt is broken when L1 and L2 differ in their use
>> of PCID.
>>
>> This is only a stopgap solution - it resolves the PCID issue without
>> introducing other issues, but the proper fix needs to consider all control
>> bits at once, rather than considering a vmentry/exit as a sequence of changes
>> of discrete registers.
> The current approach seems prone to such ordering issues, and I don't
> see a way to make it more robust while keeping the current approach,
> so I guess setting all the registers state and then evaluating them
> would make more sense and prevent this kind of mistakes.

I'm pretty sure that when we start doing all the checks that we should
be doing, there will be combinations which can't be expressed as a
non-faulting sequence of writes to cr0, cr4 and efer.

Unfortunately, there is a load of nested virt prep work to do before
implementing an approach like this becomes viable.

Hence the stopgap solution in the meantime.

~Andrew

Tian, Kevin June 28, 2019, 1:49 a.m. UTC | #3

> From: Andrew Cooper [mailto:[e-mail]]
> Sent: Thursday, June 27, 2019 3:02 AM
> 
> From: Sergey Dyasli <[e-mail]>
> 
> Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2
> and vice-versa).
> 
> Signed-off-by: Sergey Dyasli <[e-mail]>
> Reviewed-by: Andrew Cooper <[e-mail]>

Acked-by: Kevin Tian <[e-mail]>

Patch

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 7bca572d88..332623d006 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1024,11 +1024,11 @@  static void load_shadow_guest_state(struct vcpu *v)
     nvcpu->guest_cr[0] = get_vvmcs(v, CR0_READ_SHADOW);
     nvcpu->guest_cr[4] = get_vvmcs(v, CR4_READ_SHADOW);
 
-    rc = hvm_set_cr0(get_vvmcs(v, GUEST_CR0), true);
+    rc = hvm_set_cr4(get_vvmcs(v, GUEST_CR4), true);
     if ( rc == X86EMUL_EXCEPTION )
         hvm_inject_hw_exception(TRAP_gp_fault, 0);
 
-    rc = hvm_set_cr4(get_vvmcs(v, GUEST_CR4), true);
+    rc = hvm_set_cr0(get_vvmcs(v, GUEST_CR0), true);
     if ( rc == X86EMUL_EXCEPTION )
         hvm_inject_hw_exception(TRAP_gp_fault, 0);
 
@@ -1238,11 +1238,11 @@  static void load_vvmcs_host_state(struct vcpu *v)
         __vmwrite(vmcs_h2g_field[i].guest_field, r);
     }
 
-    rc = hvm_set_cr0(get_vvmcs(v, HOST_CR0), true);
+    rc = hvm_set_cr4(get_vvmcs(v, HOST_CR4), true);
     if ( rc == X86EMUL_EXCEPTION )
         hvm_inject_hw_exception(TRAP_gp_fault, 0);
 
-    rc = hvm_set_cr4(get_vvmcs(v, HOST_CR4), true);
+    rc = hvm_set_cr0(get_vvmcs(v, HOST_CR0), true);
     if ( rc == X86EMUL_EXCEPTION )
         hvm_inject_hw_exception(TRAP_gp_fault, 0);