* [PATCH v2 0/2] SVM: guest state handling adjustments @ 2018-04-30 10:17 Jan Beulich 2018-04-30 11:37 ` [PATCH v2 1/2] SVM: re-work VMCB sync-ing Jan Beulich 2018-04-30 11:37 ` [PATCH v2 2/2] SVM: introduce a VM entry helper Jan Beulich 0 siblings, 2 replies; 14+ messages in thread From: Jan Beulich @ 2018-04-30 10:17 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Andrew Cooper, Boris Ostrovsky, Suravee Suthikulpanit Only patch 1 is clearly meant for 4.11. The second patch, however, eliminates a (theoretical) window the first patch still leaves, so should at least be considered. 1: re-work VMCB sync-ing 2: introduce a VM entry helper Signed-off-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 10:17 [PATCH v2 0/2] SVM: guest state handling adjustments Jan Beulich @ 2018-04-30 11:37 ` Jan Beulich 2018-04-30 15:30 ` Boris Ostrovsky 2018-04-30 17:07 ` Andrew Cooper 2018-04-30 11:37 ` [PATCH v2 2/2] SVM: introduce a VM entry helper Jan Beulich 1 sibling, 2 replies; 14+ messages in thread From: Jan Beulich @ 2018-04-30 11:37 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Andrew Cooper, Boris Ostrovsky, Suravee Suthikulpanit While the main problem to be addressed here is the issue of what so far was named "vmcb_in_sync" starting out with the wrong value (should have been true instead of false, to prevent performing a VMSAVE without ever having VMLOADed the vCPU's state), go a step further and make the sync-ed state a tristate: CPU and memory may be in sync or an update may be required in either direction. Rename the field and introduce an enum. Callers of svm_sync_vmcb() now indicate the intended new state (with a slight "anomaly" when requesting VMLOAD: we could store vmcb_needs_vmsave in those cases as the callers request, but the VMCB really is in sync at that point, and hence there's no need to VMSAVE in case we don't make it out to guest context), and all syncing goes through that function. With that, there's no need to VMLOAD the state perhaps multiple times; all that's needed is loading it once before VM entry. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum vmcb_sync_state. --- I've been considering to put the VMLOAD invocation in svm_asid_handle_vmrun() (instead of the two copies in svm_do_resume() and svm_vmexit_handler()), but that seemed a little too abusive of the function. See patch 2. I'm also not really certain about svm_vmexit_do_vmload(): All I'm doing here is a 1:1 change from previous behavior, but I'm unconvinced this was/is really correct. --- a/xen/arch/x86/hvm/svm/entry.S +++ b/xen/arch/x86/hvm/svm/entry.S @@ -112,7 +112,6 @@ UNLIKELY_END(svm_trace) /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ mov VCPU_svm_vmcb(%rbx),%rcx - movb $0,VCPU_svm_vmcb_in_sync(%rbx) mov VMCB_rax(%rcx),%rax mov %rax,UREGS_rax(%rsp) mov VMCB_rip(%rcx),%rax --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -680,16 +680,24 @@ static void svm_cpuid_policy_changed(str cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW); } -static void svm_sync_vmcb(struct vcpu *v) +static void svm_sync_vmcb(struct vcpu *v, enum vmcb_sync_state new_state) { struct arch_svm_struct *arch_svm = &v->arch.hvm_svm; - if ( arch_svm->vmcb_in_sync ) - return; - - arch_svm->vmcb_in_sync = 1; + if ( new_state == vmcb_needs_vmsave ) + { + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); + svm_vmload(arch_svm->vmcb); + arch_svm->vmcb_sync_state = vmcb_in_sync; + } + else + { + if ( arch_svm->vmcb_sync_state == vmcb_needs_vmsave ) + svm_vmsave(arch_svm->vmcb); - svm_vmsave(arch_svm->vmcb); + if ( arch_svm->vmcb_sync_state != vmcb_needs_vmload ) + arch_svm->vmcb_sync_state = new_state; + } } static unsigned int svm_get_cpl(struct vcpu *v) @@ -707,7 +715,7 @@ static void svm_get_segment_register(str switch ( seg ) { case x86_seg_fs ... x86_seg_gs: - svm_sync_vmcb(v); + svm_sync_vmcb(v, vmcb_in_sync); /* Fallthrough. */ case x86_seg_es ... x86_seg_ds: @@ -718,7 +726,7 @@ static void svm_get_segment_register(str break; case x86_seg_tr: - svm_sync_vmcb(v); + svm_sync_vmcb(v, vmcb_in_sync); *reg = vmcb->tr; break; @@ -731,7 +739,7 @@ static void svm_get_segment_register(str break; case x86_seg_ldtr: - svm_sync_vmcb(v); + svm_sync_vmcb(v, vmcb_in_sync); *reg = vmcb->ldtr; break; @@ -746,7 +754,6 @@ static void svm_set_segment_register(str struct segment_register *reg) { struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; - bool sync = false; ASSERT((v == current) || !vcpu_runnable(v)); @@ -768,7 +775,8 @@ static void svm_set_segment_register(str case x86_seg_gs: case x86_seg_tr: case x86_seg_ldtr: - sync = (v == current); + if ( v == current ) + svm_sync_vmcb(v, vmcb_needs_vmload); break; default: @@ -777,9 +785,6 @@ static void svm_set_segment_register(str return; } - if ( sync ) - svm_sync_vmcb(v); - switch ( seg ) { case x86_seg_ss: @@ -813,9 +818,6 @@ static void svm_set_segment_register(str ASSERT_UNREACHABLE(); break; } - - if ( sync ) - svm_vmload(vmcb); } static unsigned long svm_get_shadow_gs_base(struct vcpu *v) @@ -1086,7 +1088,7 @@ static void svm_ctxt_switch_from(struct svm_lwp_save(v); svm_tsc_ratio_save(v); - svm_sync_vmcb(v); + svm_sync_vmcb(v, vmcb_needs_vmload); svm_vmload_pa(per_cpu(host_vmcb, cpu)); /* Resume use of ISTs now that the host TR is reinstated. */ @@ -1114,7 +1116,6 @@ static void svm_ctxt_switch_to(struct vc svm_restore_dr(v); svm_vmsave_pa(per_cpu(host_vmcb, cpu)); - svm_vmload(vmcb); vmcb->cleanbits.bytes = 0; svm_lwp_load(v); svm_tsc_ratio_load(v); @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc hvm_do_resume(v); + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) + svm_sync_vmcb(v, vmcb_needs_vmsave); + reset_stack_and_jump(svm_asm_do_resume); } @@ -1895,7 +1899,7 @@ static int svm_msr_read_intercept(unsign case MSR_FS_BASE: case MSR_GS_BASE: case MSR_SHADOW_GS_BASE: - svm_sync_vmcb(v); + svm_sync_vmcb(v, vmcb_in_sync); break; } @@ -2067,7 +2071,6 @@ static int svm_msr_write_intercept(unsig int ret, result = X86EMUL_OKAY; struct vcpu *v = current; struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; - bool sync = false; switch ( msr ) { @@ -2081,13 +2084,10 @@ static int svm_msr_write_intercept(unsig case MSR_FS_BASE: case MSR_GS_BASE: case MSR_SHADOW_GS_BASE: - sync = true; + svm_sync_vmcb(v, vmcb_needs_vmload); break; } - if ( sync ) - svm_sync_vmcb(v); - switch ( msr ) { case MSR_IA32_SYSENTER_ESP: @@ -2261,9 +2261,6 @@ static int svm_msr_write_intercept(unsig break; } - if ( sync ) - svm_vmload(vmcb); - return result; gpf: @@ -2413,7 +2410,7 @@ svm_vmexit_do_vmload(struct vmcb_struct put_page(page); /* State in L1 VMCB is stale now */ - v->arch.hvm_svm.vmcb_in_sync = 0; + v->arch.hvm_svm.vmcb_sync_state = vmcb_needs_vmsave; __update_guest_eip(regs, inst_len); } @@ -2623,6 +2620,7 @@ void svm_vmexit_handler(struct cpu_user_ bool_t vcpu_guestmode = 0; struct vlapic *vlapic = vcpu_vlapic(v); + v->arch.hvm_svm.vmcb_sync_state = vmcb_needs_vmsave; hvm_invalidate_regs_fields(regs); if ( paging_mode_hap(v->domain) ) @@ -3109,6 +3107,9 @@ void svm_vmexit_handler(struct cpu_user_ } out: + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) + svm_sync_vmcb(v, vmcb_needs_vmsave); + if ( vcpu_guestmode || vlapic_hw_disabled(vlapic) ) return; @@ -3117,6 +3118,7 @@ void svm_vmexit_handler(struct cpu_user_ intr.fields.tpr = (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4; vmcb_set_vintr(vmcb, intr); + ASSERT(v->arch.hvm_svm.vmcb_sync_state != vmcb_needs_vmload); } void svm_trace_vmentry(void) --- a/xen/arch/x86/hvm/svm/vmcb.c +++ b/xen/arch/x86/hvm/svm/vmcb.c @@ -84,6 +84,8 @@ static int construct_vmcb(struct vcpu *v CR_INTERCEPT_CR8_READ | CR_INTERCEPT_CR8_WRITE); + arch_svm->vmcb_sync_state = vmcb_needs_vmload; + /* I/O and MSR permission bitmaps. */ arch_svm->msrpm = alloc_xenheap_pages(get_order_from_bytes(MSRPM_SIZE), 0); if ( arch_svm->msrpm == NULL ) --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -102,7 +102,6 @@ void __dummy__(void) OFFSET(VCPU_svm_vmcb_pa, struct vcpu, arch.hvm_svm.vmcb_pa); OFFSET(VCPU_svm_vmcb, struct vcpu, arch.hvm_svm.vmcb); - OFFSET(VCPU_svm_vmcb_in_sync, struct vcpu, arch.hvm_svm.vmcb_in_sync); BLANK(); OFFSET(VCPU_vmx_launched, struct vcpu, arch.hvm_vmx.launched); --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -495,12 +495,28 @@ struct vmcb_struct { struct svm_domain { }; +/* + * VMRUN doesn't switch fs/gs/tr/ldtr and SHADOWGS/SYSCALL/SYSENTER state. + * Therefore, guest state is in the hardware registers when servicing a + * VMExit. + * + * Immediately after a VMExit, the vmcb is stale, and needs to be brought + * into sync by VMSAVE. If state in the vmcb is modified, a VMLOAD is + * needed before the following VMRUN. + */ +enum vmcb_sync_state { + vmcb_in_sync, + vmcb_needs_vmsave, /* VMCB out of sync (VMSAVE needed)? */ + vmcb_needs_vmload /* VMCB dirty (VMLOAD needed)? */ +}; + struct arch_svm_struct { struct vmcb_struct *vmcb; u64 vmcb_pa; unsigned long *msrpm; int launch_core; - bool_t vmcb_in_sync; /* VMCB sync'ed with VMSAVE? */ + + uint8_t vmcb_sync_state; /* enum vmcb_sync_state */ /* VMCB has a cached instruction from #PF/#NPF Decode Assist? */ uint8_t cached_insn_len; /* Zero if no cached instruction. */ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 11:37 ` [PATCH v2 1/2] SVM: re-work VMCB sync-ing Jan Beulich @ 2018-04-30 15:30 ` Boris Ostrovsky 2018-04-30 15:50 ` Jan Beulich 2018-04-30 17:07 ` Andrew Cooper 1 sibling, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2018-04-30 15:30 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Juergen Gross, Andrew Cooper, Suravee Suthikulpanit On 04/30/2018 07:37 AM, Jan Beulich wrote: > @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc > > hvm_do_resume(v); > > + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) > + svm_sync_vmcb(v, vmcb_needs_vmsave); Is it not possible (or advisable) to move the test into svm_sync_vmcb() (and drop the ASSERT there)? -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 15:30 ` Boris Ostrovsky @ 2018-04-30 15:50 ` Jan Beulich 2018-04-30 15:56 ` Boris Ostrovsky 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2018-04-30 15:50 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Andrew Cooper, Suravee Suthikulpanit, xen-devel >>> On 30.04.18 at 17:30, <boris.ostrovsky@oracle.com> wrote: > On 04/30/2018 07:37 AM, Jan Beulich wrote: >> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc >> >> hvm_do_resume(v); >> >> + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) >> + svm_sync_vmcb(v, vmcb_needs_vmsave); > > > Is it not possible (or advisable) to move the test into svm_sync_vmcb() > (and drop the ASSERT there)? It is possible; I'm not sure myself if it's advisable, but I take you asking the question as you thinking it is, so I'll change it. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 15:50 ` Jan Beulich @ 2018-04-30 15:56 ` Boris Ostrovsky 2018-04-30 16:01 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2018-04-30 15:56 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Andrew Cooper, Suravee Suthikulpanit, xen-devel On 04/30/2018 11:50 AM, Jan Beulich wrote: >>>> On 30.04.18 at 17:30, <boris.ostrovsky@oracle.com> wrote: >> On 04/30/2018 07:37 AM, Jan Beulich wrote: >>> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc >>> >>> hvm_do_resume(v); >>> >>> + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) >>> + svm_sync_vmcb(v, vmcb_needs_vmsave); >> >> Is it not possible (or advisable) to move the test into svm_sync_vmcb() >> (and drop the ASSERT there)? > It is possible; I'm not sure myself if it's advisable, but I take you asking the > question as you thinking it is, so I'll change it. That was really the main reason for me asking to move svm_vmload into svm_sync_vmcb() --- to hide all logic related to the sync state machine there. -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 15:56 ` Boris Ostrovsky @ 2018-04-30 16:01 ` Jan Beulich 0 siblings, 0 replies; 14+ messages in thread From: Jan Beulich @ 2018-04-30 16:01 UTC (permalink / raw) To: Boris Ostrovsky Cc: Juergen Gross, Andrew Cooper, Suravee Suthikulpanit, xen-devel >>> On 30.04.18 at 17:56, <boris.ostrovsky@oracle.com> wrote: > On 04/30/2018 11:50 AM, Jan Beulich wrote: >>>>> On 30.04.18 at 17:30, <boris.ostrovsky@oracle.com> wrote: >>> On 04/30/2018 07:37 AM, Jan Beulich wrote: >>>> @@ -1168,6 +1169,9 @@ static void noreturn svm_do_resume(struc >>>> >>>> hvm_do_resume(v); >>>> >>>> + if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) >>>> + svm_sync_vmcb(v, vmcb_needs_vmsave); >>> >>> Is it not possible (or advisable) to move the test into svm_sync_vmcb() >>> (and drop the ASSERT there)? >> It is possible; I'm not sure myself if it's advisable, but I take you asking the >> question as you thinking it is, so I'll change it. > > That was really the main reason for me asking to move svm_vmload into > svm_sync_vmcb() --- to hide all logic related to the sync state machine > there. Well, there's still the code in svm_vmexit_do_vmload(). Depending on your opinion on the post-commit-message question in patch 2, the one at the top of svm_vmexit_handler() might go away in that patch. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 11:37 ` [PATCH v2 1/2] SVM: re-work VMCB sync-ing Jan Beulich 2018-04-30 15:30 ` Boris Ostrovsky @ 2018-04-30 17:07 ` Andrew Cooper 2018-04-30 17:50 ` Boris Ostrovsky 1 sibling, 1 reply; 14+ messages in thread From: Andrew Cooper @ 2018-04-30 17:07 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Juergen Gross, Boris Ostrovsky, Suravee Suthikulpanit On 30/04/18 12:37, Jan Beulich wrote: > While the main problem to be addressed here is the issue of what so far > was named "vmcb_in_sync" starting out with the wrong value (should have > been true instead of false, to prevent performing a VMSAVE without ever > having VMLOADed the vCPU's state), go a step further and make the > sync-ed state a tristate: CPU and memory may be in sync or an update > may be required in either direction. Rename the field and introduce an > enum. Callers of svm_sync_vmcb() now indicate the intended new state > (with a slight "anomaly" when requesting VMLOAD: we could store > vmcb_needs_vmsave in those cases as the callers request, but the VMCB > really is in sync at that point, and hence there's no need to VMSAVE in > case we don't make it out to guest context), and all syncing goes > through that function. > > With that, there's no need to VMLOAD the state perhaps multiple times; > all that's needed is loading it once before VM entry. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum > vmcb_sync_state. -1 from me. This is even more confusing to use than v1. It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); means "vmload", and its actively wrong that the state doesn't remain in-sync. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 17:07 ` Andrew Cooper @ 2018-04-30 17:50 ` Boris Ostrovsky 2018-05-02 7:11 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2018-04-30 17:50 UTC (permalink / raw) To: Andrew Cooper, Jan Beulich, xen-devel Cc: Juergen Gross, Suravee Suthikulpanit On 04/30/2018 01:07 PM, Andrew Cooper wrote: > On 30/04/18 12:37, Jan Beulich wrote: >> While the main problem to be addressed here is the issue of what so far >> was named "vmcb_in_sync" starting out with the wrong value (should have >> been true instead of false, to prevent performing a VMSAVE without ever >> having VMLOADed the vCPU's state), go a step further and make the >> sync-ed state a tristate: CPU and memory may be in sync or an update >> may be required in either direction. Rename the field and introduce an >> enum. Callers of svm_sync_vmcb() now indicate the intended new state >> (with a slight "anomaly" when requesting VMLOAD: we could store >> vmcb_needs_vmsave in those cases as the callers request, but the VMCB >> really is in sync at that point, and hence there's no need to VMSAVE in >> case we don't make it out to guest context), and all syncing goes >> through that function. >> >> With that, there's no need to VMLOAD the state perhaps multiple times; >> all that's needed is loading it once before VM entry. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum >> vmcb_sync_state. > -1 from me. This is even more confusing to use than v1. > > It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); > means "vmload", and its actively wrong that the state doesn't remain > in-sync. It does become in-sync: + if ( new_state == vmcb_needs_vmsave ) + { + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); + svm_vmload(arch_svm->vmcb); + arch_svm->vmcb_sync_state = vmcb_in_sync; + } + else (although Jan is questioning whether to drop that change in the comments to patch 2, if I understood him correctly) -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-04-30 17:50 ` Boris Ostrovsky @ 2018-05-02 7:11 ` Jan Beulich 2018-05-02 14:45 ` Boris Ostrovsky 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2018-05-02 7:11 UTC (permalink / raw) To: Andrew Cooper, Boris Ostrovsky Cc: Juergen Gross, xen-devel, Suravee Suthikulpanit >>> On 30.04.18 at 19:50, <boris.ostrovsky@oracle.com> wrote: > On 04/30/2018 01:07 PM, Andrew Cooper wrote: >> On 30/04/18 12:37, Jan Beulich wrote: >>> While the main problem to be addressed here is the issue of what so far >>> was named "vmcb_in_sync" starting out with the wrong value (should have >>> been true instead of false, to prevent performing a VMSAVE without ever >>> having VMLOADed the vCPU's state), go a step further and make the >>> sync-ed state a tristate: CPU and memory may be in sync or an update >>> may be required in either direction. Rename the field and introduce an >>> enum. Callers of svm_sync_vmcb() now indicate the intended new state >>> (with a slight "anomaly" when requesting VMLOAD: we could store >>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB >>> really is in sync at that point, and hence there's no need to VMSAVE in >>> case we don't make it out to guest context), and all syncing goes >>> through that function. >>> >>> With that, there's no need to VMLOAD the state perhaps multiple times; >>> all that's needed is loading it once before VM entry. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum >>> vmcb_sync_state. >> -1 from me. This is even more confusing to use than v1. >> >> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); >> means "vmload", and its actively wrong that the state doesn't remain >> in-sync. > > It does become in-sync: > > > + if ( new_state == vmcb_needs_vmsave ) > + { > + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); > + svm_vmload(arch_svm->vmcb); > + arch_svm->vmcb_sync_state = vmcb_in_sync; > + } > + else > > (although Jan is questioning whether to drop that change in the comments to > patch 2, if I understood him correctly) Indeed - in patch 2 this could be made go away. Hence the posting of patch 2 at this point in time in the first place (otherwise I would have waited until 4.12 has opened). In any event - I need some sort of indication of a way forward here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-05-02 7:11 ` Jan Beulich @ 2018-05-02 14:45 ` Boris Ostrovsky 2018-05-03 14:43 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Boris Ostrovsky @ 2018-05-02 14:45 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper Cc: Juergen Gross, xen-devel, Suravee Suthikulpanit On 05/02/2018 03:11 AM, Jan Beulich wrote: >>>> On 30.04.18 at 19:50, <boris.ostrovsky@oracle.com> wrote: >> On 04/30/2018 01:07 PM, Andrew Cooper wrote: >>> On 30/04/18 12:37, Jan Beulich wrote: >>>> While the main problem to be addressed here is the issue of what so far >>>> was named "vmcb_in_sync" starting out with the wrong value (should have >>>> been true instead of false, to prevent performing a VMSAVE without ever >>>> having VMLOADed the vCPU's state), go a step further and make the >>>> sync-ed state a tristate: CPU and memory may be in sync or an update >>>> may be required in either direction. Rename the field and introduce an >>>> enum. Callers of svm_sync_vmcb() now indicate the intended new state >>>> (with a slight "anomaly" when requesting VMLOAD: we could store >>>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB >>>> really is in sync at that point, and hence there's no need to VMSAVE in >>>> case we don't make it out to guest context), and all syncing goes >>>> through that function. >>>> >>>> With that, there's no need to VMLOAD the state perhaps multiple times; >>>> all that's needed is loading it once before VM entry. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum >>>> vmcb_sync_state. >>> -1 from me. This is even more confusing to use than v1. >>> >>> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); >>> means "vmload", and its actively wrong that the state doesn't remain >>> in-sync. >> It does become in-sync: >> >> >> + if ( new_state == vmcb_needs_vmsave ) >> + { >> + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); >> + svm_vmload(arch_svm->vmcb); >> + arch_svm->vmcb_sync_state = vmcb_in_sync; >> + } >> + else >> >> (although Jan is questioning whether to drop that change in the comments to >> patch 2, if I understood him correctly) > Indeed - in patch 2 this could be made go away. Hence the posting of patch 2 > at this point in time in the first place (otherwise I would have waited until 4.12 > has opened). > > In any event - I need some sort of indication of a way forward here. I think the extra optimization that you suggested in patch 2 would make things a bit less obvious so I'd be inclined not to do that (but maybe a comment in svm_sync_vmcb() that we are doing it only for clarity might be useful.) I also see a point in Andrew's observation that vmcb_needs_vmsave implying a vmload may not be not immediately obvious so if he feels strongly about that I will be OK with going back to v1. -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-05-02 14:45 ` Boris Ostrovsky @ 2018-05-03 14:43 ` Jan Beulich 2018-05-03 18:34 ` Boris Ostrovsky 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2018-05-03 14:43 UTC (permalink / raw) To: Andrew Cooper, Boris Ostrovsky Cc: Juergen Gross, xen-devel, Suravee Suthikulpanit >>> On 02.05.18 at 16:45, <boris.ostrovsky@oracle.com> wrote: > On 05/02/2018 03:11 AM, Jan Beulich wrote: >>>>> On 30.04.18 at 19:50, <boris.ostrovsky@oracle.com> wrote: >>> On 04/30/2018 01:07 PM, Andrew Cooper wrote: >>>> On 30/04/18 12:37, Jan Beulich wrote: >>>>> While the main problem to be addressed here is the issue of what so far >>>>> was named "vmcb_in_sync" starting out with the wrong value (should have >>>>> been true instead of false, to prevent performing a VMSAVE without ever >>>>> having VMLOADed the vCPU's state), go a step further and make the >>>>> sync-ed state a tristate: CPU and memory may be in sync or an update >>>>> may be required in either direction. Rename the field and introduce an >>>>> enum. Callers of svm_sync_vmcb() now indicate the intended new state >>>>> (with a slight "anomaly" when requesting VMLOAD: we could store >>>>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB >>>>> really is in sync at that point, and hence there's no need to VMSAVE in >>>>> case we don't make it out to guest context), and all syncing goes >>>>> through that function. >>>>> >>>>> With that, there's no need to VMLOAD the state perhaps multiple times; >>>>> all that's needed is loading it once before VM entry. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> --- >>>>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum >>>>> vmcb_sync_state. >>>> -1 from me. This is even more confusing to use than v1. >>>> >>>> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); >>>> means "vmload", and its actively wrong that the state doesn't remain >>>> in-sync. >>> It does become in-sync: >>> >>> >>> + if ( new_state == vmcb_needs_vmsave ) >>> + { >>> + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); >>> + svm_vmload(arch_svm->vmcb); >>> + arch_svm->vmcb_sync_state = vmcb_in_sync; >>> + } >>> + else >>> >>> (although Jan is questioning whether to drop that change in the comments to >>> patch 2, if I understood him correctly) >> Indeed - in patch 2 this could be made go away. Hence the posting of patch 2 >> at this point in time in the first place (otherwise I would have waited > until 4.12 >> has opened). >> >> In any event - I need some sort of indication of a way forward here. > > I think the extra optimization that you suggested in patch 2 would make > things a bit less obvious so I'd be inclined not to do that (but maybe a > comment in svm_sync_vmcb() that we are doing it only for clarity might > be useful.) Hmm, interesting. To me it would seem to improve things. > I also see a point in Andrew's observation that vmcb_needs_vmsave > implying a vmload may not be not immediately obvious so if he feels > strongly about that I will be OK with going back to v1. How that? Switching to vmcb_needs_vmload also implies a VMSAVE, after all (if none has happened before). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] SVM: re-work VMCB sync-ing 2018-05-03 14:43 ` Jan Beulich @ 2018-05-03 18:34 ` Boris Ostrovsky 0 siblings, 0 replies; 14+ messages in thread From: Boris Ostrovsky @ 2018-05-03 18:34 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper Cc: Juergen Gross, xen-devel, Suravee Suthikulpanit On 05/03/2018 10:43 AM, Jan Beulich wrote: >>>> On 02.05.18 at 16:45, <boris.ostrovsky@oracle.com> wrote: >> On 05/02/2018 03:11 AM, Jan Beulich wrote: >>>>>> On 30.04.18 at 19:50, <boris.ostrovsky@oracle.com> wrote: >>>> On 04/30/2018 01:07 PM, Andrew Cooper wrote: >>>>> On 30/04/18 12:37, Jan Beulich wrote: >>>>>> While the main problem to be addressed here is the issue of what so far >>>>>> was named "vmcb_in_sync" starting out with the wrong value (should have >>>>>> been true instead of false, to prevent performing a VMSAVE without ever >>>>>> having VMLOADed the vCPU's state), go a step further and make the >>>>>> sync-ed state a tristate: CPU and memory may be in sync or an update >>>>>> may be required in either direction. Rename the field and introduce an >>>>>> enum. Callers of svm_sync_vmcb() now indicate the intended new state >>>>>> (with a slight "anomaly" when requesting VMLOAD: we could store >>>>>> vmcb_needs_vmsave in those cases as the callers request, but the VMCB >>>>>> really is in sync at that point, and hence there's no need to VMSAVE in >>>>>> case we don't make it out to guest context), and all syncing goes >>>>>> through that function. >>>>>> >>>>>> With that, there's no need to VMLOAD the state perhaps multiple times; >>>>>> all that's needed is loading it once before VM entry. >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> --- >>>>>> v2: Also handle VMLOAD in svm_sync_vmcb(). Add comment to enum >>>>>> vmcb_sync_state. >>>>> -1 from me. This is even more confusing to use than v1. >>>>> >>>>> It is not obvious at all that using svm_sync_vmcb(v, vmcb_needs_vmsave); >>>>> means "vmload", and its actively wrong that the state doesn't remain >>>>> in-sync. >>>> It does become in-sync: >>>> >>>> >>>> + if ( new_state == vmcb_needs_vmsave ) >>>> + { >>>> + ASSERT(arch_svm->vmcb_sync_state == vmcb_needs_vmload); >>>> + svm_vmload(arch_svm->vmcb); >>>> + arch_svm->vmcb_sync_state = vmcb_in_sync; >>>> + } >>>> + else >>>> >>>> (although Jan is questioning whether to drop that change in the comments to >>>> patch 2, if I understood him correctly) >>> Indeed - in patch 2 this could be made go away. Hence the posting of patch 2 >>> at this point in time in the first place (otherwise I would have waited >> until 4.12 >>> has opened). >>> >>> In any event - I need some sort of indication of a way forward here. >> I think the extra optimization that you suggested in patch 2 would make >> things a bit less obvious so I'd be inclined not to do that (but maybe a >> comment in svm_sync_vmcb() that we are doing it only for clarity might >> be useful.) > Hmm, interesting. To me it would seem to improve things. > >> I also see a point in Andrew's observation that vmcb_needs_vmsave >> implying a vmload may not be not immediately obvious so if he feels >> strongly about that I will be OK with going back to v1. > How that? Switching to vmcb_needs_vmload also implies a VMSAVE, after > all (if none has happened before). Hmm.. Right. I don't know why it appeared less than obvious to me then when I looked at it last time. -boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] SVM: introduce a VM entry helper 2018-04-30 10:17 [PATCH v2 0/2] SVM: guest state handling adjustments Jan Beulich 2018-04-30 11:37 ` [PATCH v2 1/2] SVM: re-work VMCB sync-ing Jan Beulich @ 2018-04-30 11:37 ` Jan Beulich 2018-04-30 11:51 ` Andrew Cooper 1 sibling, 1 reply; 14+ messages in thread From: Jan Beulich @ 2018-04-30 11:37 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Andrew Cooper, Boris Ostrovsky, Suravee Suthikulpanit The register values copying doesn't need doing in assembly. The VMLOAD invocation can also be further deferred (and centralized). Therefore replace the svm_asid_handle_vmrun() invocation wiht one of the new helper. Similarly move the VM exit side register value copying into svm_vmexit_handler(). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New. --- TBD: Now that we always make it out to guest context after VMLOAD, perhaps svm_sync_vmcb() should no longer override vmcb_needs_vmsave, and svm_vmexit_handler() would then no longer need to to set the field at all. --- a/xen/arch/x86/hvm/svm/entry.S +++ b/xen/arch/x86/hvm/svm/entry.S @@ -61,24 +61,14 @@ UNLIKELY_START(ne, nsvm_hap) jmp .Lsvm_do_resume __UNLIKELY_END(nsvm_hap) - call svm_asid_handle_vmrun + mov %rsp, %rdi + call svm_vmenter_helper cmpb $0,tb_init_done(%rip) UNLIKELY_START(nz, svm_trace) call svm_trace_vmentry UNLIKELY_END(svm_trace) - mov VCPU_svm_vmcb(%rbx),%rcx - mov UREGS_rax(%rsp),%rax - mov %rax,VMCB_rax(%rcx) - mov UREGS_rip(%rsp),%rax - mov %rax,VMCB_rip(%rcx) - mov UREGS_rsp(%rsp),%rax - mov %rax,VMCB_rsp(%rcx) - mov UREGS_eflags(%rsp),%rax - or $X86_EFLAGS_MBS,%rax - mov %rax,VMCB_rflags(%rcx) - mov VCPU_arch_msr(%rbx), %rax mov VCPUMSR_spec_ctrl_raw(%rax), %eax @@ -111,16 +101,6 @@ UNLIKELY_END(svm_trace) SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ - mov VCPU_svm_vmcb(%rbx),%rcx - mov VMCB_rax(%rcx),%rax - mov %rax,UREGS_rax(%rsp) - mov VMCB_rip(%rcx),%rax - mov %rax,UREGS_rip(%rsp) - mov VMCB_rsp(%rcx),%rax - mov %rax,UREGS_rsp(%rsp) - mov VMCB_rflags(%rcx),%rax - mov %rax,UREGS_eflags(%rsp) - STGI GLOBAL(svm_stgi_label) mov %rsp,%rdi --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1169,12 +1169,25 @@ static void noreturn svm_do_resume(struc hvm_do_resume(v); - if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) - svm_sync_vmcb(v, vmcb_needs_vmsave); - reset_stack_and_jump(svm_asm_do_resume); } +void svm_vmenter_helper(const struct cpu_user_regs *regs) +{ + struct vcpu *curr = current; + struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb; + + svm_asid_handle_vmrun(); + + if ( curr->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) + svm_sync_vmcb(curr, vmcb_needs_vmsave); + + vmcb->rax = regs->rax; + vmcb->rip = regs->rip; + vmcb->rsp = regs->rsp; + vmcb->rflags = regs->rflags | X86_EFLAGS_MBS; +} + static void svm_guest_osvw_init(struct vcpu *vcpu) { if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ) @@ -2621,6 +2634,12 @@ void svm_vmexit_handler(struct cpu_user_ struct vlapic *vlapic = vcpu_vlapic(v); v->arch.hvm_svm.vmcb_sync_state = vmcb_needs_vmsave; + + regs->rax = vmcb->rax; + regs->rip = vmcb->rip; + regs->rsp = vmcb->rsp; + regs->rflags = vmcb->rflags; + hvm_invalidate_regs_fields(regs); if ( paging_mode_hap(v->domain) ) @@ -3107,9 +3126,6 @@ void svm_vmexit_handler(struct cpu_user_ } out: - if ( v->arch.hvm_svm.vmcb_sync_state == vmcb_needs_vmload ) - svm_sync_vmcb(v, vmcb_needs_vmsave); - if ( vcpu_guestmode || vlapic_hw_disabled(vlapic) ) return; @@ -3118,7 +3134,6 @@ void svm_vmexit_handler(struct cpu_user_ intr.fields.tpr = (vlapic_get_reg(vlapic, APIC_TASKPRI) & 0xFF) >> 4; vmcb_set_vintr(vmcb, intr); - ASSERT(v->arch.hvm_svm.vmcb_sync_state != vmcb_needs_vmload); } void svm_trace_vmentry(void) --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -119,12 +119,6 @@ void __dummy__(void) OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv); BLANK(); - OFFSET(VMCB_rax, struct vmcb_struct, rax); - OFFSET(VMCB_rip, struct vmcb_struct, rip); - OFFSET(VMCB_rsp, struct vmcb_struct, rsp); - OFFSET(VMCB_rflags, struct vmcb_struct, rflags); - BLANK(); - OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending); OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask); BLANK(); --- a/xen/include/asm-x86/hvm/svm/asid.h +++ b/xen/include/asm-x86/hvm/svm/asid.h @@ -23,6 +23,7 @@ #include <asm/processor.h> void svm_asid_init(const struct cpuinfo_x86 *c); +void svm_asid_handle_vmrun(void); static inline void svm_asid_g_invlpg(struct vcpu *v, unsigned long g_vaddr) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] SVM: introduce a VM entry helper 2018-04-30 11:37 ` [PATCH v2 2/2] SVM: introduce a VM entry helper Jan Beulich @ 2018-04-30 11:51 ` Andrew Cooper 0 siblings, 0 replies; 14+ messages in thread From: Andrew Cooper @ 2018-04-30 11:51 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Juergen Gross, Boris Ostrovsky, Suravee Suthikulpanit On 30/04/18 12:37, Jan Beulich wrote: > --- a/xen/arch/x86/hvm/svm/entry.S > +++ b/xen/arch/x86/hvm/svm/entry.S > @@ -61,24 +61,14 @@ UNLIKELY_START(ne, nsvm_hap) > jmp .Lsvm_do_resume > __UNLIKELY_END(nsvm_hap) > > - call svm_asid_handle_vmrun > + mov %rsp, %rdi > + call svm_vmenter_helper > > cmpb $0,tb_init_done(%rip) > UNLIKELY_START(nz, svm_trace) > call svm_trace_vmentry > UNLIKELY_END(svm_trace) This trace call can also be moved up into C, at which point you can fold svm_trace_vmentry() (which is a 1-liner anyway) into svm_vmenter_helper() which would be its sole caller. Otherwise, Reivewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > - mov VCPU_svm_vmcb(%rbx),%rcx > - mov UREGS_rax(%rsp),%rax > - mov %rax,VMCB_rax(%rcx) > - mov UREGS_rip(%rsp),%rax > - mov %rax,VMCB_rip(%rcx) > - mov UREGS_rsp(%rsp),%rax > - mov %rax,VMCB_rsp(%rcx) > - mov UREGS_eflags(%rsp),%rax > - or $X86_EFLAGS_MBS,%rax > - mov %rax,VMCB_rflags(%rcx) > - > mov VCPU_arch_msr(%rbx), %rax > mov VCPUMSR_spec_ctrl_raw(%rax), %eax > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-05-03 18:31 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-30 10:17 [PATCH v2 0/2] SVM: guest state handling adjustments Jan Beulich 2018-04-30 11:37 ` [PATCH v2 1/2] SVM: re-work VMCB sync-ing Jan Beulich 2018-04-30 15:30 ` Boris Ostrovsky 2018-04-30 15:50 ` Jan Beulich 2018-04-30 15:56 ` Boris Ostrovsky 2018-04-30 16:01 ` Jan Beulich 2018-04-30 17:07 ` Andrew Cooper 2018-04-30 17:50 ` Boris Ostrovsky 2018-05-02 7:11 ` Jan Beulich 2018-05-02 14:45 ` Boris Ostrovsky 2018-05-03 14:43 ` Jan Beulich 2018-05-03 18:34 ` Boris Ostrovsky 2018-04-30 11:37 ` [PATCH v2 2/2] SVM: introduce a VM entry helper Jan Beulich 2018-04-30 11:51 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.