Re: [Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: Thursday, June 27, 2019 3:02 AM > > From: Sergey Dyasli > > Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2 > and vice-versa). > > Signed-off-by: Sergey Dyasli > Reviewed-by: Andrew Cooper Acked-by: Kevin Tian ___ 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
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 >> >> Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2 >> and vice-versa). >> >> Signed-off-by: Sergey Dyasli >> Reviewed-by: Andrew Cooper > Reviewed-by: Roger Pau Monné > >> --- >> CC: Jan Beulich >> CC: Wei Liu >> CC: Roger Pau Monné >> CC: Jun Nakajima >> CC: Kevin Tian >> >> 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
On Wed, Jun 26, 2019 at 08:02:12PM +0100, Andrew Cooper wrote: > From: Sergey Dyasli > > Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2 > and vice-versa). > > Signed-off-by: Sergey Dyasli > Reviewed-by: Andrew Cooper Reviewed-by: Roger Pau Monné > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > CC: Jun Nakajima > CC: Kevin Tian > > 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
[Xen-devel] [PATCH] x86/vvmx: set CR4 before CR0
From: Sergey Dyasli Otherwise hvm_set_cr0() will check the wrong CR4 bits (L1 instead of L2 and vice-versa). Signed-off-by: Sergey Dyasli Reviewed-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Jun Nakajima CC: Kevin Tian 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
