[Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0

[Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0

[Andrew Cooper]

From: Sergey Dyasli <sergey.dyasli@citrix.com>

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

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

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(-)

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);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0

[Roger Pau Monné]

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

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> 
> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0

[Andrew Cooper]

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 <sergey.dyasli@citrix.com>
>>
>> Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2
>> and vice-versa).
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0

[Tian, Kevin]

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

Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel