Re: [PATCH 10/24] PVH xen: introduce pvh_set_vcpu_info() and vmx_pvh_set_vcpu_info()

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 18 Jul 2013 14:14:32 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 18.07.13 at 04:32, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> 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