[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] Nested VMX: CR emulation fix up
On 10/08/2013 04:31 AM, Jan Beulich wrote:
On 08.10.13 at 09:29, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1817,6 +1817,9 @@ int hvm_set_cr0(unsigned long value)
}
v->arch.hvm_vcpu.guest_cr[0] = value;
+ if ( !nestedhvm_vmswitch_in_progress(v) &&
+ nestedhvm_vcpu_in_guestmode(v) )
+ v->arch.hvm_vcpu.nvcpu.guest_cr[0] = value;
hvm_update_guest_cr(v, 0);
if ( (value ^ old_value) & X86_CR0_PG ) {
@@ -1899,6 +1902,9 @@ int hvm_set_cr4(unsigned long value)
}
v->arch.hvm_vcpu.guest_cr[4] = value;
+ if ( !nestedhvm_vmswitch_in_progress(v) &&
+ nestedhvm_vcpu_in_guestmode(v) )
+ v->arch.hvm_vcpu.nvcpu.guest_cr[4] = value;
hvm_update_guest_cr(v, 4);
Considering the redundancy - wouldn't all of the above now
become the body of a rather desirable helper function?
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1096,6 +1096,30 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
vmx_update_cpu_exec_control(v);
}
+ if ( nestedhvm_vcpu_in_guestmode(v) )
+ {
+ if ( !nestedhvm_vmswitch_in_progress(v) )
+ {
+ /*
+ * We get here when L2 changed cr0 in a way that did not change
+ * any of L1's shadowed bits (see nvmx_n2_vmexit_handler),
+ * but did change L0 shadowed bits. So we first calculate the
+ * effective cr0 value that L1 would like to write into the
+ * hardware. It consists of the L2-owned bits from the new
+ * value combined with the L1-owned bits from L1's guest cr0.
+ */
+ v->arch.hvm_vcpu.guest_cr[0] &=
+ ~__get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
CR0_GUEST_HOST_MASK);
+ v->arch.hvm_vcpu.guest_cr[0] |=
+ __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, GUEST_CR0) &
+ __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
CR0_GUEST_HOST_MASK);
+ }
+ /* nvcpu.guest_cr[0] is what L2 write to cr0 actually. */
+ __vmwrite(CR0_READ_SHADOW, v->arch.hvm_vcpu.nvcpu.guest_cr[0]);
+ }
+ else
+ __vmwrite(CR0_READ_SHADOW, v->arch.hvm_vcpu.guest_cr[0]);
Please re-phrase this into
if ( !nestedhvm_vcpu_in_guestmode(v) )
...
else if ( !nestedhvm_vmswitch_in_progress(v) )
For one that'll put the "normal" (non-nested) case first. And second
it'll reduce indentation on the main portion of your additions (at once
taking care of the otherwise over-long lines in there).
I'm btw also mildly concerned that the moving ahead of this VMCS
write might have other side effects. I did check that we don't read
the shadow value other than in debugging and nested code, but
I'm nevertheless not quite certain that this is indeed benign.
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1042,6 +1042,8 @@ static void load_shadow_guest_state(struct vcpu *v)
vvmcs_to_shadow_bulk(v, ARRAY_SIZE(vmcs_gstate_field),
vmcs_gstate_field);
+ nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
+ nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
Given that the only time where these get read is in
vmx_update_guest_cr() (for writing CR<n>_READ_SHADOW),
are the writes above really needed? And if they are, aren't there
other updates to these two fields still missing?
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -100,6 +100,9 @@ struct nestedvcpu {
*/
bool_t nv_ioport80;
bool_t nv_ioportED;
+
+ /* L2's control-resgister, just as the L2 sees them. */
+ unsigned long guest_cr[5];
This should be prefixed with nv_: all members of this structure are. In
addition, struct hvm_vcpu has exact same member.
Considering that this touches code common with nested SVM, I'd
expect the SVM maintainers to have to approve of the change in
any case.
In particular I wonder whether this addition isn't obsoleting
SVM's ns_cr0.
I am not sure whether ns_cr0 (replaced with nv_guest_cr[0]) would
then be updated in paths where it currently is not.
For example in nsvm_vmcb_prepare4vmrun():
/* CR0 */
svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
cr0 = nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0;
rc = hvm_set_cr0(cr0); <------ nv_guest_cr[0] will get set here.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
