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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel