From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [PATCH 10/24] PVH xen: introduce pvh_set_vcpu_info() and vmx_pvh_set_vcpu_info() Date: Thu, 18 Jul 2013 11:37:49 -0700 Message-ID: <20130718113749.71d434b1@mantra.us.oracle.com> References: <1374114788-27652-1-git-send-email-mukesh.rathor@oracle.com> <1374114788-27652-11-git-send-email-mukesh.rathor@oracle.com> <51E8065802000078000E5FD7@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Uzt5S-0000gP-RE for xen-devel@lists.xenproject.org; Thu, 18 Jul 2013 18:37:59 +0000 In-Reply-To: <51E8065802000078000E5FD7@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On Thu, 18 Jul 2013 14:14:32 +0100 "Jan Beulich" wrote: > >>> On 18.07.13 at 04:32, Mukesh Rathor > >>> wrote: > > +/* > > + * Set vmcs fields in support of vcpu_op -> VCPUOP_initialise > > hcall. Called > > + * from arch_set_info_guest() which sets the (PVH relevant) > > non-vmcs fields. > > + * > > + * In case of linux: > > + * The boot vcpu calls this to set some context for the non > > boot smp vcpu. > > + * The call comes from cpu_initialize_context(). (boot vcpu 0 > > context is > > + * set by the tools via do_domctl -> vcpu_initialise). > > + * > > + * NOTE: In case of VMCS, loading a selector doesn't cause the > > hidden fields > > + * to be automatically loaded. We load selectors here but > > not the hidden > > + * parts. This means we require the guest to have same > > hidden values > > + * as the default values loaded in the vmcs in > > pvh_construct_vmcs(), ie, > > + * the GDT the vcpu is coming up on should be something like > > following > > + * on linux (for 64bit, CS:0x10 DS/SS:0x18) : > > + * > > + * ffff88007f704000: 0000000000000000 00cf9b000000ffff > > + * ffff88007f704010: 00af9b000000ffff 00cf93000000ffff > > + * ffff88007f704020: 00cffb000000ffff 00cff3000000ffff > > + * > > + */ > > This comment should reflect reality as closely as possible, or else > it'll just cause confusion rather than clarifying things. In > particular, the hidden base fields of FS and GS get set below, and > hence the comment should say so. Ah, right, the FS and GS are different that way. I'll change comment. > > +int vmx_pvh_set_vcpu_info(struct vcpu *v, struct > > vcpu_guest_context *ctxtp) +{ > > + if ( v->vcpu_id == 0 ) > > + return 0; > > + > > + if ( !(ctxtp->flags & VGCF_in_kernel) ) > > + return -EINVAL; > > So you check for kernel mode now, ... > > > + > > + vmx_vmcs_enter(v); > > + __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr); > > + __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit); > > + __vmwrite(GUEST_LDTR_BASE, ctxtp->ldt_base); > > + __vmwrite(GUEST_LDTR_LIMIT, ctxtp->ldt_ents); > > + > > + __vmwrite(GUEST_FS_BASE, ctxtp->fs_base); > > + __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_user); > > ... but then write the user GS base here ... > > > + > > + __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs); > > + __vmwrite(GUEST_SS_SELECTOR, ctxtp->user_regs.ss); > > + __vmwrite(GUEST_ES_SELECTOR, ctxtp->user_regs.es); > > + __vmwrite(GUEST_DS_SELECTOR, ctxtp->user_regs.ds); > > + __vmwrite(GUEST_FS_SELECTOR, ctxtp->user_regs.fs); > > + __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs); > > + > > + if ( vmx_add_guest_msr(MSR_SHADOW_GS_BASE) ) > > + { > > + vmx_vmcs_exit(v); > > + return -EINVAL; > > + } > > + vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_kernel); > > ... and the kernel one here? That looks the wrong way round to me. Yeah, I struggled with that one a lot, and had added to my list to talk to Konrad about. I think the PV code in linux has it backwards. Both values are same when the hcall is made, btw. But in linux baremetal/HVM, the value put in gs_base_user is the value written to MSR_GS_BASE. But, in PV part of linux code, I think it should be switched. Since you also agree, I'll change this code here. thanks Mukesh