From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes Date: Fri, 16 Aug 2013 17:11:21 +0100 Message-ID: <520E6B4902000078000ECA4B@nat28.tlf.novell.com> References: <1374631171-15224-1-git-send-email-mukesh.rathor@oracle.com> <1374631171-15224-11-git-send-email-mukesh.rathor@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , Mukesh Rathor , Tim Deegan Cc: "keir.xen@gmail.com" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> On 16.08.13 at 17:32, George Dunlap wrote: > On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor > wrote: >> This patch mostly contains changes to arch/x86/domain.c to allow for a PVH >> domain creation. The new function pvh_set_vcpu_info(), introduced in the >> previous patch, is called here to set some guest context in the VMCS. >> This patch also changes the context_switch code in the same file to follow >> HVM behaviour for PVH. >> >> Changes in V2: >> - changes to read_segment_register() moved to this patch. >> - The other comment was to create NULL functions for pvh_set_vcpu_info >> and pvh_read_descriptor which are implemented in later patch, but since >> I disable PVH creation until all patches are checked in, it is not > needed. >> But it helps breaking down of patches. >> >> Changes in V3: >> - Fix read_segment_register() macro to make sure args are evaluated once, >> and use # instead of STR for name in the macro. >> >> Changes in V4: >> - Remove pvh substruct in the hvm substruct, as the vcpu_info_mfn has been >> moved out of pv_vcpu struct. >> - rename hvm_pvh_* functions to hvm_*. >> >> Changes in V5: >> - remove pvh_read_descriptor(). >> >> Changes in V7: >> - remove hap_update_cr3() and read_segment_register changes from here. >> >> Signed-off-by: Mukesh Rathor >> Reviewed-by: Jan Beulich >> --- >> xen/arch/x86/domain.c | 56 ++++++++++++++++++++++++++++++++---------------- >> xen/arch/x86/mm.c | 3 ++ >> 2 files changed, 40 insertions(+), 19 deletions(-) >> >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >> index c361abf..fccb4ee 100644 >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -385,7 +385,7 @@ int vcpu_initialise(struct vcpu *v) >> >> vmce_init_vcpu(v); >> >> - if ( is_hvm_domain(d) ) >> + if ( !is_pv_domain(d) ) >> { >> rc = hvm_vcpu_initialise(v); >> goto done; >> @@ -452,7 +452,7 @@ void vcpu_destroy(struct vcpu *v) >> >> vcpu_destroy_fpu(v); >> >> - if ( is_hvm_vcpu(v) ) >> + if ( !is_pv_vcpu(v) ) >> hvm_vcpu_destroy(v); >> else >> xfree(v->arch.pv_vcpu.trap_ctxt); >> @@ -464,7 +464,7 @@ int arch_domain_create(struct domain *d, unsigned int > domcr_flags) >> int rc = -ENOMEM; >> >> d->arch.hvm_domain.hap_enabled = >> - is_hvm_domain(d) && >> + !is_pv_domain(d) && >> hvm_funcs.hap_supported && >> (domcr_flags & DOMCRF_hap); >> d->arch.hvm_domain.mem_sharing_enabled = 0; >> @@ -512,7 +512,7 @@ int arch_domain_create(struct domain *d, unsigned int > domcr_flags) >> mapcache_domain_init(d); >> >> HYPERVISOR_COMPAT_VIRT_START(d) = >> - is_hvm_domain(d) ? ~0u : __HYPERVISOR_COMPAT_VIRT_START; >> + is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u; >> >> if ( (rc = paging_domain_init(d, domcr_flags)) != 0 ) >> goto fail; >> @@ -555,7 +555,7 @@ int arch_domain_create(struct domain *d, unsigned int > domcr_flags) >> } >> spin_lock_init(&d->arch.e820_lock); >> >> - if ( is_hvm_domain(d) ) >> + if ( !is_pv_domain(d) ) >> { >> if ( (rc = hvm_domain_initialise(d)) != 0 ) >> { >> @@ -651,7 +651,7 @@ int arch_set_info_guest( >> #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld)) >> flags = c(flags); >> >> - if ( !is_hvm_vcpu(v) ) >> + if ( is_pv_vcpu(v) ) >> { >> if ( !compat ) >> { >> @@ -704,7 +704,7 @@ int arch_set_info_guest( >> v->fpu_initialised = !!(flags & VGCF_I387_VALID); >> >> v->arch.flags &= ~TF_kernel_mode; >> - if ( (flags & VGCF_in_kernel) || is_hvm_vcpu(v)/*???*/ ) >> + if ( (flags & VGCF_in_kernel) || !is_pv_vcpu(v)/*???*/ ) >> v->arch.flags |= TF_kernel_mode; >> >> v->arch.vgc_flags = flags; >> @@ -719,7 +719,7 @@ int arch_set_info_guest( >> if ( !compat ) >> { >> memcpy(&v->arch.user_regs, &c.nat->user_regs, sizeof(c.nat->user_regs)); >> - if ( !is_hvm_vcpu(v) ) >> + if ( is_pv_vcpu(v) ) >> memcpy(v->arch.pv_vcpu.trap_ctxt, c.nat->trap_ctxt, >> sizeof(c.nat->trap_ctxt)); >> } >> @@ -735,10 +735,13 @@ int arch_set_info_guest( >> >> v->arch.user_regs.eflags |= 2; >> >> - if ( is_hvm_vcpu(v) ) >> + if ( !is_pv_vcpu(v) ) >> { >> hvm_set_info_guest(v); >> - goto out; >> + if ( is_hvm_vcpu(v) || v->is_initialised ) >> + goto out; >> + else >> + goto pvh_skip_pv_stuff; >> } >> >> init_int80_direct_trap(v); >> @@ -853,6 +856,7 @@ int arch_set_info_guest( >> >> set_bit(_VPF_in_reset, &v->pause_flags); >> >> + pvh_skip_pv_stuff: > > Any idea what this set_bit(_VPF_in_reset) stuff is? It looks like > it's set above, and cleared down near the bottom of the function if > nothing gets screwed up. This is related to the preemptible vCPU reset (which arch_set_info_guest() just re-uses), making sure that while there is an incomplete state update for a vCPU 8because it may have got preempted) the vCPU can't be unpaused. > It seems like if that set/clear pair is important, then PVH should do > them both as well, shouldn't it? I thought I had checked this once - does it now bypass one of the two? But then again, this is all about PV memory management, so perhaps it was that way when I checked, and I decided it was fine. Jan