All of lore.kernel.org
 help / color / mirror / Atom feed
* RFC: PVH set vcpu info context in vmcs....
@ 2013-08-13  1:45 Mukesh Rathor
  2013-08-13  9:16 ` Tim Deegan
  2013-08-13 10:56 ` Jan Beulich
  0 siblings, 2 replies; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-13  1:45 UTC (permalink / raw)
  To: Jan Beulich, Tim Deegan, Keir Fraser, Xen-devel

I just decided to create a new thread... Here's the code I wanted
to propose.

thanks,
Mukesh

/*
 * Set vmcs fields in support of vcpu_op -> VCPUOP_initialise hcall used
 * to initialise a secondary vcpu prior to boot. Called from 
 * arch_set_info_guest() which sets the (PVH relevant) non-vmcs fields.
 *
 * In case of linux:
 *     The call comes from cpu_initialize_context().  (boot vcpu 0 context is
 *     set by the tools via do_domctl -> vcpu_initialise).
 *
 * PVH 32bitfixme: this function needs to be modified for 32bit guest. 
 */
int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
{
    int rc;

    if ( v->vcpu_id == 0 )
        return 0;

    if ( !(ctxtp->flags & VGCF_in_kernel) )
        return -EINVAL;

    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
         (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss || 
         ctxtp->user_regs.es || ctxtp->user_regs.ds )
        return -EINVAL;

    if ( ctxtp->user_regs.cs == 0 )
        return -EINVAL;

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
    __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);

    __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);

    /* IA-32e: ss/es/ds are ignored, we load cs only. */ 
    __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);
    if ( (rc = hvm_load_segment_selector(x86_seg_cs, ctxtp->user_regs.cs)) ) 
        return rc;

    __vmwrite(GUEST_FS_SELECTOR, ctxtp->user_regs.fs);
    __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs);

    if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
    {
        vmx_vmcs_exit(v);
        return rc;
    }
    vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);

    vmx_vmcs_exit(v);
    return 0;
}

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-13  1:45 RFC: PVH set vcpu info context in vmcs Mukesh Rathor
@ 2013-08-13  9:16 ` Tim Deegan
  2013-08-13 12:10   ` Jan Beulich
  2013-08-13 10:56 ` Jan Beulich
  1 sibling, 1 reply; 19+ messages in thread
From: Tim Deegan @ 2013-08-13  9:16 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: Keir Fraser, Xen-devel, Jan Beulich

Hi, 

At 18:45 -0700 on 12 Aug (1376333113), Mukesh Rathor wrote:
> I just decided to create a new thread... Here's the code I wanted
> to propose.
> 
> /*
>  * Set vmcs fields in support of vcpu_op -> VCPUOP_initialise hcall used
>  * to initialise a secondary vcpu prior to boot. Called from 
>  * arch_set_info_guest() which sets the (PVH relevant) non-vmcs fields.
>  *
>  * In case of linux:
>  *     The call comes from cpu_initialize_context().  (boot vcpu 0 context is
>  *     set by the tools via do_domctl -> vcpu_initialise).
>  *
>  * PVH 32bitfixme: this function needs to be modified for 32bit guest. 
>  */
> int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
> {
>     int rc;
> 
>     if ( v->vcpu_id == 0 )
>         return 0;
> 
>     if ( !(ctxtp->flags & VGCF_in_kernel) )
>         return -EINVAL;

Is that a new restriction on PVH?  Should there be a matching check of
the CS RPL or is it safe not to?

>     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>          (ctxtp->user_regs.cs & 4) 

I think you could support LDTs by either taking the LDT range supplied
(with a bit of sanity checking) or asking for a descriptor and using
hvm_load_segment_selector(x86_seg_ldtr).

> || ctxtp->user_regs.ss || 
>          ctxtp->user_regs.es || ctxtp->user_regs.ds )
>         return -EINVAL;
> 
>     if ( ctxtp->user_regs.cs == 0 )
>         return -EINVAL;
> 
>     vmx_vmcs_enter(v);
>     __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
>     __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);
> 
>     __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> 
>     /* IA-32e: ss/es/ds are ignored, we load cs only. */ 
>     __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);
>     if ( (rc = hvm_load_segment_selector(x86_seg_cs, ctxtp->user_regs.cs)) ) 
>         return rc;

This looks good to me, thank you.

>     __vmwrite(GUEST_FS_SELECTOR, ctxtp->user_regs.fs);
>     __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs);

Or we could load these segments from their selectors too, and then
either check that the supplied bases match or mandate that they be zero.
I'm happy enough with this interface, though.  Either way it would be
nice to document which of GDT[fs].base and fs_base is actually loaded
(and likewise for gs_base_*).

Tim.

>     if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
>     {
>         vmx_vmcs_exit(v);
>         return rc;
>     }
>     vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);
> 
>     vmx_vmcs_exit(v);
>     return 0;
> }

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-13  1:45 RFC: PVH set vcpu info context in vmcs Mukesh Rathor
  2013-08-13  9:16 ` Tim Deegan
@ 2013-08-13 10:56 ` Jan Beulich
  2013-08-14  2:12   ` Mukesh Rathor
  1 sibling, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2013-08-13 10:56 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Tim Deegan

>>> On 13.08.13 at 03:45, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
> {
>     int rc;
> 
>     if ( v->vcpu_id == 0 )
>         return 0;

Bogus/pointless.

>     if ( !(ctxtp->flags & VGCF_in_kernel) )
>         return -EINVAL;
> 
>     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>          (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss || 
>          ctxtp->user_regs.es || ctxtp->user_regs.ds )
>         return -EINVAL;

How about FS/GS? If you don't enforce these selectors to be zero
too, then loading only base and selector values below isn't
sufficient (and again potentially inconsistent).

> 
>     if ( ctxtp->user_regs.cs == 0 )
>         return -EINVAL;

Perhaps also check RPL == 0?

>     vmx_vmcs_enter(v);
>     __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
>     __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);
> 
>     __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> 
>     /* IA-32e: ss/es/ds are ignored, we load cs only. */ 
>     __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);
>     if ( (rc = hvm_load_segment_selector(x86_seg_cs, ctxtp->user_regs.cs)) ) 
>         return rc;

You can't use that function here without modification, as it
assumes v == current.

Jan

> 
>     __vmwrite(GUEST_FS_SELECTOR, ctxtp->user_regs.fs);
>     __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs);
> 
>     if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
>     {
>         vmx_vmcs_exit(v);
>         return rc;
>     }
>     vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);
> 
>     vmx_vmcs_exit(v);
>     return 0;
> }

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-13  9:16 ` Tim Deegan
@ 2013-08-13 12:10   ` Jan Beulich
  0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2013-08-13 12:10 UTC (permalink / raw)
  To: Mukesh Rathor, Tim Deegan; +Cc: Keir Fraser, Xen-devel

>>> On 13.08.13 at 11:16, Tim Deegan <tim@xen.org> wrote:
>>     __vmwrite(GUEST_FS_SELECTOR, ctxtp->user_regs.fs);
>>     __vmwrite(GUEST_GS_SELECTOR, ctxtp->user_regs.gs);
> 
> Or we could load these segments from their selectors too, and then
> either check that the supplied bases match or mandate that they be zero.

For FS and GS that would be wrong - you can't load them through
a selector when the base is beyond 4Gb. As said in my other reply,
I think we should require the selectors to be zero, and load just the
bases.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-13 10:56 ` Jan Beulich
@ 2013-08-14  2:12   ` Mukesh Rathor
  2013-08-14  9:12     ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-14  2:12 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Tue, 13 Aug 2013 11:56:36 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 13.08.13 at 03:45, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
> > *ctxtp) {
> >     int rc;
> > 
> >     if ( v->vcpu_id == 0 )
> >         return 0;
> 
> Bogus/pointless.

No, we don't have anything for the boot vcpu. It's totally coming up
on the flat address space. For non boot, the vcpu is coming up on the
kernel GDT. Recall it's a PV guest (coming up in an HVM container).

> >     if ( !(ctxtp->flags & VGCF_in_kernel) )
> >         return -EINVAL;
> > 
> >     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
> >          (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss || 
> >          ctxtp->user_regs.es || ctxtp->user_regs.ds )
> >         return -EINVAL;
> 
> How about FS/GS? If you don't enforce these selectors to be zero
> too, then loading only base and selector values below isn't
> sufficient (and again potentially inconsistent).
> 
> > 
> >     if ( ctxtp->user_regs.cs == 0 )
> >         return -EINVAL;
> 
> Perhaps also check RPL == 0?

OK. I still think we should just remove the check for VGCF_in_kernel,
why do we care for PVH? For 64bit PV, we needed that, but a PVH guest
can come up any RPL it chooses to, right?

Below is updated version.

thanks
Mukesh


/*
 * Set vmcs fields in support of vcpu_op -> VCPUOP_initialise hcall used
 * to initialise a secondary vcpu prior to boot. Called from
 * arch_set_info_guest() which sets the (PVH relevant) non-vmcs fields.
 *
 * In case of linux:
 *     The call comes from cpu_initialize_context().  (boot vcpu 0 context is
 *     set by the tools via do_domctl -> vcpu_initialise).
 *
 * PVH 32bitfixme: this function needs to be modified for 32bit guest.
 */
int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
{
    int rc = 0;

    if ( v->vcpu_id == 0 )
        return 0;

    if ( !(ctxtp->flags & VGCF_in_kernel) )
        return -EINVAL;

    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
         (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss ||
         ctxtp->user_regs.es || ctxtp->user_regs.ds ||
         ctxtp->user_regs.fs || ctxtp->user_regs.gs )
         
        return -EINVAL;

    if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 3) == 3 )
        return -EINVAL;

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
    __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);

    __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);

    /* IA-32e: ss/es/ds are ignored, we load cs only. */
    __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);
    if ( (rc = hvm_load_segment_selector(v, x86_seg_cs, ctxtp->user_regs.cs)) )
        goto out;

    if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
        goto out;
    vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);

 out:
    vmx_vmcs_exit(v);
    return 0;
}

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-14  2:12   ` Mukesh Rathor
@ 2013-08-14  9:12     ` Jan Beulich
  2013-08-14  9:57       ` George Dunlap
  2013-08-15  0:25       ` Mukesh Rathor
  0 siblings, 2 replies; 19+ messages in thread
From: Jan Beulich @ 2013-08-14  9:12 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Tim Deegan

>>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Tue, 13 Aug 2013 11:56:36 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 13.08.13 at 03:45, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
>> > *ctxtp) {
>> >     int rc;
>> > 
>> >     if ( v->vcpu_id == 0 )
>> >         return 0;
>> 
>> Bogus/pointless.
> 
> No, we don't have anything for the boot vcpu. It's totally coming up
> on the flat address space. For non boot, the vcpu is coming up on the
> kernel GDT. Recall it's a PV guest (coming up in an HVM container).

No, that's the wrong perspective. You either should never get here
for vCPU 0, or you should refuse this for all already initialized
vCPU-s.

>> >     if ( !(ctxtp->flags & VGCF_in_kernel) )
>> >         return -EINVAL;
>> > 
>> >     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>> >          (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss || 
>> >          ctxtp->user_regs.es || ctxtp->user_regs.ds )
>> >         return -EINVAL;
>> 
>> How about FS/GS? If you don't enforce these selectors to be zero
>> too, then loading only base and selector values below isn't
>> sufficient (and again potentially inconsistent).
>> 
>> > 
>> >     if ( ctxtp->user_regs.cs == 0 )
>> >         return -EINVAL;
>> 
>> Perhaps also check RPL == 0?
> 
> OK. I still think we should just remove the check for VGCF_in_kernel,
> why do we care for PVH? For 64bit PV, we needed that, but a PVH guest
> can come up any RPL it chooses to, right?

Again, no - if you drop the checking of the flag, you need the
function to behave correctly even if it is not set (and do
consistency checks for _both_ cases). So to keep the code
simple _and_ correct, checking the flag to be clear and doing
only the kernel mode validation is likely the better route.

> int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
> {
>     int rc = 0;
> 
>     if ( v->vcpu_id == 0 )
>         return 0;
> 
>     if ( !(ctxtp->flags & VGCF_in_kernel) )
>         return -EINVAL;
> 
>     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>          (ctxtp->user_regs.cs & 4) || ctxtp->user_regs.ss ||
>          ctxtp->user_regs.es || ctxtp->user_regs.ds ||
>          ctxtp->user_regs.fs || ctxtp->user_regs.gs )
>          
>         return -EINVAL;

Stray blank line before the return statement.

> 
>     if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 3) == 3 )
>         return -EINVAL;

Perhaps better check for any non-zero RPL, as the RPL needs
to match of the (enforced) CS hidden fields?

Also, I'd very much prefer all the CS related checks to be together.

>     vmx_vmcs_enter(v);
>     __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
>     __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);
> 
>     __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> 
>     /* IA-32e: ss/es/ds are ignored, we load cs only. */
>     __vmwrite(GUEST_CS_SELECTOR, ctxtp->user_regs.cs);

Pointlessly duplicating what the function call below will also do.

>     if ( (rc = hvm_load_segment_selector(v, x86_seg_cs, ctxtp->user_regs.cs)) )
>         goto out;

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-14  9:12     ` Jan Beulich
@ 2013-08-14  9:57       ` George Dunlap
  2013-08-15  0:25       ` Mukesh Rathor
  1 sibling, 0 replies; 19+ messages in thread
From: George Dunlap @ 2013-08-14  9:57 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Wed, Aug 14, 2013 at 10:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>> On Tue, 13 Aug 2013 11:56:36 +0100
>> "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> >>> On 13.08.13 at 03:45, Mukesh Rathor <mukesh.rathor@oracle.com>
>>> >>> wrote:
>>> > int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
>>> > *ctxtp) {
>>> >     int rc;
>>> >
>>> >     if ( v->vcpu_id == 0 )
>>> >         return 0;
>>>
>>> Bogus/pointless.
>>
>> No, we don't have anything for the boot vcpu. It's totally coming up
>> on the flat address space. For non boot, the vcpu is coming up on the
>> kernel GDT. Recall it's a PV guest (coming up in an HVM container).
>
> No, that's the wrong perspective. You either should never get here
> for vCPU 0, or you should refuse this for all already initialized
> vCPU-s.

+1, FWIW.

 -George

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-14  9:12     ` Jan Beulich
  2013-08-14  9:57       ` George Dunlap
@ 2013-08-15  0:25       ` Mukesh Rathor
  2013-08-15  1:58         ` Mukesh Rathor
  2013-08-15  6:31         ` Jan Beulich
  1 sibling, 2 replies; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-15  0:25 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Wed, 14 Aug 2013 10:12:18 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Tue, 13 Aug 2013 11:56:36 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
...
> >> >     if ( v->vcpu_id == 0 )
> >> >         return 0;
> >> 
> >> Bogus/pointless.
> > 
> > No, we don't have anything for the boot vcpu. It's totally coming up
> > on the flat address space. For non boot, the vcpu is coming up on
> > the kernel GDT. Recall it's a PV guest (coming up in an HVM
> > container).
> 
> No, that's the wrong perspective. You either should never get here
> for vCPU 0, or you should refuse this for all already initialized
> vCPU-s.

Ok, i'll move the check to caller. FWIW, the vcpu->is_initialised is
inapplicable here because this is relevant for non-boot vcpus only.

>> 
>>     if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 3) == 3 )
>>         return -EINVAL;  

>Perhaps better check for any non-zero RPL, as the RPL needs
>to match of the (enforced) CS hidden fields?

Well, we now load the descriptor provided by the guest in the GDT
so that's kinda un needed now.. thats why i thought we don't need to
enforce RPL or check VGCF_in_kernel. But, that would work too...

thanks,
mukesh

Final(hopefully) version:

/*
 * Set vmcs fields in support of vcpu_op -> VCPUOP_initialise hcall used
 * to initialise a secondary vcpu prior to boot. (boot vcpu 0 context is
 * set by the tools via do_domctl -> vcpu_initialise).  Called from
 * arch_set_info_guest() which sets the (PVH relevant) non-vmcs fields.
 *
 * In case of linux:
 *     The call comes from cpu_initialize_context().  
 *
 * PVH 32bitfixme: this function needs to be modified for 32bit guest.
 */
int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
{
    int rc;

    if ( !(ctxtp->flags & VGCF_in_kernel) )
        return -EINVAL;

    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
         ctxtp->user_regs.ss || ctxtp->user_regs.es || ctxtp->user_regs.ds ||
         ctxtp->user_regs.fs || ctxtp->user_regs.gs )
        return -EINVAL;

    if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 7) )
        return -EINVAL;

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
    __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);

    /* IA-32e: ss/es/ds are ignored. */
    if ( (rc = hvm_load_segment_selector(v, x86_seg_cs, ctxtp->user_regs.cs)) )
        goto out;

    __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);

    if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
        goto out;
    vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);

 out:
    vmx_vmcs_exit(v);
    return rc;
}

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-15  0:25       ` Mukesh Rathor
@ 2013-08-15  1:58         ` Mukesh Rathor
  2013-08-15  6:34           ` Jan Beulich
  2013-08-15  6:31         ` Jan Beulich
  1 sibling, 1 reply; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-15  1:58 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Wed, 14 Aug 2013 17:25:15 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Wed, 14 Aug 2013 10:12:18 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
.......
> int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
> *ctxtp) {
>     int rc;
> 
>     if ( !(ctxtp->flags & VGCF_in_kernel) )
>         return -EINVAL;
> 
>     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>          ctxtp->user_regs.ss || ctxtp->user_regs.es ||
> ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs )
>         return -EINVAL;
> 
>     if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 7) )
>         return -EINVAL;
> 
>     vmx_vmcs_enter(v);
>     __vmwrite(GUEST_GDTR_BASE, ctxtp->gdt.pvh.addr);
>     __vmwrite(GUEST_GDTR_LIMIT, ctxtp->gdt.pvh.limit);
> 
>     /* IA-32e: ss/es/ds are ignored. */
>     if ( (rc = hvm_load_segment_selector(v, x86_seg_cs,
> ctxtp->user_regs.cs)) ) goto out;
> 
>     __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> 
>     if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
>         goto out;
>     vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);
> 

Jan,

Thinking about this more, I realized we are unnecessarily creating
a vmcs intercept for gs_base_user. How about we not allow that either?

thanks
mukesh

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-15  0:25       ` Mukesh Rathor
  2013-08-15  1:58         ` Mukesh Rathor
@ 2013-08-15  6:31         ` Jan Beulich
  2013-08-16  2:26           ` Mukesh Rathor
  1 sibling, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2013-08-15  6:31 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Tim Deegan

>>> On 15.08.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Wed, 14 Aug 2013 10:12:18 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > On Tue, 13 Aug 2013 11:56:36 +0100
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
> ...
>> >> >     if ( v->vcpu_id == 0 )
>> >> >         return 0;
>> >> 
>> >> Bogus/pointless.
>> > 
>> > No, we don't have anything for the boot vcpu. It's totally coming up
>> > on the flat address space. For non boot, the vcpu is coming up on
>> > the kernel GDT. Recall it's a PV guest (coming up in an HVM
>> > container).
>> 
>> No, that's the wrong perspective. You either should never get here
>> for vCPU 0, or you should refuse this for all already initialized
>> vCPU-s.
> 
> Ok, i'll move the check to caller. FWIW, the vcpu->is_initialised is
> inapplicable here because this is relevant for non-boot vcpus only.

That won't make the check any better. Can you please explain in
detail why this check is necessary, and why the ->is_initialised one
would be wrong?

>>>     if ( ctxtp->user_regs.cs == 0 || (ctxtp->user_regs.cs & 3) == 3 )
>>>         return -EINVAL;  
> 
>>Perhaps better check for any non-zero RPL, as the RPL needs
>>to match of the (enforced) CS hidden fields?
> 
> Well, we now load the descriptor provided by the guest in the GDT
> so that's kinda un needed now.. thats why i thought we don't need to
> enforce RPL or check VGCF_in_kernel. But, that would work too...

Of course you don't need to duplicate anything that
hvm_load_segment_selector() does; anything that's not obvious
would of course warrant a code comment to that effect.

> Final(hopefully) version:

Looks okay, but we'll certainly need to see the changes to
hvm_load_segment_selector() too.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-15  1:58         ` Mukesh Rathor
@ 2013-08-15  6:34           ` Jan Beulich
  0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2013-08-15  6:34 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Tim Deegan

>>> On 15.08.13 at 03:58, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>>     __vmwrite(GUEST_FS_BASE, ctxtp->fs_base);
>>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
>> 
>>     if ( (rc = vmx_add_guest_msr(MSR_SHADOW_GS_BASE)) )
>>         goto out;
>>     vmx_write_guest_msr(MSR_SHADOW_GS_BASE, ctxtp->gs_base_user);
>> 
> 
> Thinking about this more, I realized we are unnecessarily creating
> a vmcs intercept for gs_base_user. How about we not allow that either?

Why would requiring this base to be zero avoid the need to create
an intercept?

Also, remember that the user/kernel distinction here is sort of a
mis-naming by the Xen public interface - the CPU architecture
calls them GS base and shadow GS base, so other usage models
by kernels are possible (albeit admittedly unlikely).

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-15  6:31         ` Jan Beulich
@ 2013-08-16  2:26           ` Mukesh Rathor
  2013-08-16  7:28             ` Jan Beulich
  0 siblings, 1 reply; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-16  2:26 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Thu, 15 Aug 2013 07:31:32 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 15.08.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Wed, 14 Aug 2013 10:12:18 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > On Tue, 13 Aug 2013 11:56:36 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> > 
> > ...
> >> >> >     if ( v->vcpu_id == 0 )
> >> >> >         return 0;
> >> >> 
> >> >> Bogus/pointless.
> >> > 
> >> > No, we don't have anything for the boot vcpu. It's totally
> >> > coming up on the flat address space. For non boot, the vcpu is
> >> > coming up on the kernel GDT. Recall it's a PV guest (coming up
> >> > in an HVM container).
> >> 
> >> No, that's the wrong perspective. You either should never get here
> >> for vCPU 0, or you should refuse this for all already initialized
> >> vCPU-s.
> > 
> > Ok, i'll move the check to caller. FWIW, the vcpu->is_initialised is
> > inapplicable here because this is relevant for non-boot vcpus only.
> 
> That won't make the check any better. Can you please explain in
> detail why this check is necessary, and why the ->is_initialised one
> would be wrong?

is_initialised is marked for *each* vcpu upon it being updated, so 
second pass around, things are skipped in arch_set_info_guest().
What we want here is something just for non-boot vcpu. The boot vcpu
should fields mostly null in this function since its coming up flat 
with default descriptors we loaded. Hence, it will return -EINVAL as
the function is. The boot vcpu sets up bunch of things for other vcpus, 
like gdt, per cpu data areas, etc.. and jump starts them. To jump start 
secondary vcpus, its sets few things in this function. For PV guest, all 
things need to be sent to the hypervisor, but for PVH the initialization 
can be done in the guest with very minor change, hence I had proposed 
in the past to make this function even more minimal. Actually, all the 
secondary vcpu needs is gs_base_kernel.  Again, we are not really
creating a new guest type, but booting a PV guest in HVM container.
Hence, we look at it from the perspective of what a PV guest and
accomodate PVH extension.

If I had my way, I'd have this function as follows:

int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
{
        vmx_vmcs_enter(v);
        __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
        vmx_vmcs_exit(v);

        return 0;
}

I realize there are other fields in vcpu_guest_context,  but at the 
risk of repeating myself, they are all needed for PV, but not for PVH.
Yes, they all will need to be used for pvh save/restore, but not here where
we are just powering on a secondary vcpu. Perhaps, rename function to
something like: vmx_pvh_set_boot_secondary_vcpu_fields()? 

thanks much,
Mukesh

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-16  2:26           ` Mukesh Rathor
@ 2013-08-16  7:28             ` Jan Beulich
  2013-08-16 22:28               ` Mukesh Rathor
  0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2013-08-16  7:28 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Tim Deegan

>>> On 16.08.13 at 04:26, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Thu, 15 Aug 2013 07:31:32 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 15.08.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > On Wed, 14 Aug 2013 10:12:18 +0100
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> >> >>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >> >>> wrote:
>> >> > On Tue, 13 Aug 2013 11:56:36 +0100
>> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> > 
>> > ...
>> >> >> >     if ( v->vcpu_id == 0 )
>> >> >> >         return 0;
>> >> >> 
>> >> >> Bogus/pointless.
>> >> > 
>> >> > No, we don't have anything for the boot vcpu. It's totally
>> >> > coming up on the flat address space. For non boot, the vcpu is
>> >> > coming up on the kernel GDT. Recall it's a PV guest (coming up
>> >> > in an HVM container).
>> >> 
>> >> No, that's the wrong perspective. You either should never get here
>> >> for vCPU 0, or you should refuse this for all already initialized
>> >> vCPU-s.
>> > 
>> > Ok, i'll move the check to caller. FWIW, the vcpu->is_initialised is
>> > inapplicable here because this is relevant for non-boot vcpus only.
>> 
>> That won't make the check any better. Can you please explain in
>> detail why this check is necessary, and why the ->is_initialised one
>> would be wrong?
> 
> is_initialised is marked for *each* vcpu upon it being updated, so 
> second pass around, things are skipped in arch_set_info_guest().

So what's the point of the of != 0 check then? The intention is for
this to be a one time thing, i.e. to not be used on already initialized
CPUs.

> What we want here is something just for non-boot vcpu. The boot vcpu
> should fields mostly null in this function since its coming up flat 
> with default descriptors we loaded. Hence, it will return -EINVAL as
> the function is.

But you still didn't explain how you would _incorrectly_ get here for
the boot CPU _at all_.

> The boot vcpu sets up bunch of things for other vcpus, 
> like gdt, per cpu data areas, etc.. and jump starts them. To jump start 
> secondary vcpus, its sets few things in this function.

Nor did you explain why a second call, with is_initialized already
set, would be valid.

> If I had my way, I'd have this function as follows:
> 
> int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context *ctxtp)
> {
>         vmx_vmcs_enter(v);
>         __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
>         vmx_vmcs_exit(v);
> 
>         return 0;
> }
> 
> I realize there are other fields in vcpu_guest_context,  but at the 
> risk of repeating myself, they are all needed for PV, but not for PVH.
> Yes, they all will need to be used for pvh save/restore, but not here where
> we are just powering on a secondary vcpu. Perhaps, rename function to
> something like: vmx_pvh_set_boot_secondary_vcpu_fields()? 

I think you're heading the completely wrong direction here. The
function, just like its PV and HVM counterparts, should be
generically handling both booting and restoring of a guest.

And any fields documented as unused for PVH in _both_ of these
scenarios should be validated to be zero (to allow eventual later
use for whatever purpose).

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-16  7:28             ` Jan Beulich
@ 2013-08-16 22:28               ` Mukesh Rathor
  2013-08-17  1:37                 ` Mukesh Rathor
  0 siblings, 1 reply; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-16 22:28 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Fri, 16 Aug 2013 08:28:12 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 16.08.13 at 04:26, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Thu, 15 Aug 2013 07:31:32 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 15.08.13 at 02:25, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > On Wed, 14 Aug 2013 10:12:18 +0100
...
> > is_initialised is marked for *each* vcpu upon it being updated, so 
> > second pass around, things are skipped in arch_set_info_guest().
> 
> So what's the point of the of != 0 check then? The intention is for
> this to be a one time thing, i.e. to not be used on already
> initialized CPUs.
> 
> > What we want here is something just for non-boot vcpu. The boot vcpu
> > should fields mostly null in this function since its coming up flat 
> > with default descriptors we loaded. Hence, it will return -EINVAL as
> > the function is.
> 
> But you still didn't explain how you would _incorrectly_ get here for
> the boot CPU _at all_.

Like I said, arch_set_info_guest() is called for all vcpus, boot or
non-boot. arch_set_info_guest() calls pvh_set_vcpu_info() for all vcpus,
boot or nonboot, hence the check for vcpuid. Anyways, with
latest change below, i think it doesn't matter now anyways.

> > The boot vcpu sets up bunch of things for other vcpus, 
> > like gdt, per cpu data areas, etc.. and jump starts them. To jump
> > start secondary vcpus, its sets few things in this function.
> 
> Nor did you explain why a second call, with is_initialized already
> set, would be valid.

pvh_set_vcpu_info() gets called once for each vcpu. is_initialized doesn't
affect it, neither we can use is_initialized to find whether a vcpu
is boot vcpu or not.

> > If I had my way, I'd have this function as follows:
> > 
> > int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
> > *ctxtp) {
> >         vmx_vmcs_enter(v);
> >         __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> >         vmx_vmcs_exit(v);
> > 
> >         return 0;
> > }
> > 
> > I realize there are other fields in vcpu_guest_context,  but at the 
> > risk of repeating myself, they are all needed for PV, but not for
> > PVH. Yes, they all will need to be used for pvh save/restore, but
> > not here where we are just powering on a secondary vcpu. Perhaps,
> > rename function to something like:
> > vmx_pvh_set_boot_secondary_vcpu_fields()? 
> 
> I think you're heading the completely wrong direction here. The
> function, just like its PV and HVM counterparts, should be
> generically handling both booting and restoring of a guest.

I don't think so :). Like I said in another thread, PVH save/restore
is intended to be done the HVM way. If you are not familiar with HVM
way:
   Boot:  do_vcpu_op->VCPUOP_initialise->arch_set_info_guest().
   Save/Restore:  do_domctl->XEN_DOMCTL_sethvmcontext -> hvm_load/save().

Same for PVH, thus, the function above is called for pvh boot path only,
like it says in the prolog.

> And any fields documented as unused for PVH in _both_ of these
> scenarios should be validated to be zero (to allow eventual later
> use for whatever purpose).

Ok, I'll have to change tools to make sure they are zeroed in the
create path. I can do that. I'll also take that to mean you are ok with 
the above short function with checks for zero...  I'll have it that way in 
the next version. Thank you very much :), I'm glad this is finally resolved.

Mukesh

vmx_pvh_vcpu_boot_set_info(...) (renamed from vmx_pvh_set_vcpu_info())
{
    check all other fields are zero

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
    vmx_vmcs_exit(v);
}

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-16 22:28               ` Mukesh Rathor
@ 2013-08-17  1:37                 ` Mukesh Rathor
  2013-08-17 10:22                   ` Tim Deegan
  2013-08-19  9:34                   ` Jan Beulich
  0 siblings, 2 replies; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-17  1:37 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Tim Deegan

On Fri, 16 Aug 2013 15:28:37 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Fri, 16 Aug 2013 08:28:12 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
......... 
> Ok, I'll have to change tools to make sure they are zeroed in the
> create path. I can do that. I'll also take that to mean you are ok
> with the above short function with checks for zero...  I'll have it
> that way in the next version. Thank you very much :), I'm glad this
> is finally resolved.

Ok, changed the tools to clear fields, tested and everything works:

/*
 * Set vmcs fields during boot of a vcpu. Called from arch_set_info_guest.
 *
 * Boot vcpu call is from tools via:
 *     do_domctl -> XEN_DOMCTL_setvcpucontext -> arch_set_info_guest
 *
 * Secondary vcpu's are brought up by the guest itself via:
 *     do_vcpu_op -> VCPUOP_initialise -> arch_set_info_guest
 *     (In case of linux, the call comes from cpu_initialize_context()).
 *
 * Note, PVH save/restore is expected to happen the HVM way, ie,
 *        do_domctl -> XEN_DOMCTL_sethvmcontext -> hvm_load/save
 * and not get here.
 *
 * PVH 32bitfixme: this function needs to be modified for 32bit guest.
 */
int vmx_pvh_vcpu_boot_set_info(struct vcpu *v, 
                               struct vcpu_guest_context *ctxtp)
{
    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
         ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
         ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
         ctxtp->gdt.pvh.addr || ctxtp->gdt.pvh.limit ||
         ctxtp->fs_base || ctxtp->gs_base_user )
        return -EINVAL;

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
    vmx_vmcs_exit(v);

    return 0;
}

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-17  1:37                 ` Mukesh Rathor
@ 2013-08-17 10:22                   ` Tim Deegan
  2013-08-19 21:45                     ` Mukesh Rathor
  2013-08-19  9:34                   ` Jan Beulich
  1 sibling, 1 reply; 19+ messages in thread
From: Tim Deegan @ 2013-08-17 10:22 UTC (permalink / raw)
  To: Mukesh Rathor; +Cc: xen-devel, Keir Fraser, Jan Beulich

At 18:37 -0700 on 16 Aug (1376678229), Mukesh Rathor wrote:
> Ok, changed the tools to clear fields, tested and everything works:
> 
> /*
>  * Set vmcs fields during boot of a vcpu. Called from arch_set_info_guest.
>  *
>  * Boot vcpu call is from tools via:
>  *     do_domctl -> XEN_DOMCTL_setvcpucontext -> arch_set_info_guest
>  *
>  * Secondary vcpu's are brought up by the guest itself via:
>  *     do_vcpu_op -> VCPUOP_initialise -> arch_set_info_guest
>  *     (In case of linux, the call comes from cpu_initialize_context()).
>  *
>  * Note, PVH save/restore is expected to happen the HVM way, ie,
>  *        do_domctl -> XEN_DOMCTL_sethvmcontext -> hvm_load/save
>  * and not get here.
>  *
>  * PVH 32bitfixme: this function needs to be modified for 32bit guest.

What's the 32-bit interface going to be like?  Presumably it will have
to load segment state, so this is just postponing that work until then.

>  */
> int vmx_pvh_vcpu_boot_set_info(struct vcpu *v, 
>                                struct vcpu_guest_context *ctxtp)
> {
>     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>          ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
>          ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
>          ctxtp->gdt.pvh.addr || ctxtp->gdt.pvh.limit ||
>          ctxtp->fs_base || ctxtp->gs_base_user )
>         return -EINVAL;
> 
>     vmx_vmcs_enter(v);
>     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
>     vmx_vmcs_exit(v);

Why does this one field get loaded?  Couldn't it be zero too?

Tim.

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-17  1:37                 ` Mukesh Rathor
  2013-08-17 10:22                   ` Tim Deegan
@ 2013-08-19  9:34                   ` Jan Beulich
  2013-08-19 21:46                     ` Mukesh Rathor
  1 sibling, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2013-08-19  9:34 UTC (permalink / raw)
  To: mukesh.rathor; +Cc: xen-devel, keir.xen, tim

>>> Mukesh Rathor <mukesh.rathor@oracle.com> 08/17/13 3:37 AM >>>
>/*
>* Set vmcs fields during boot of a vcpu. Called from arch_set_info_guest.
>*
>* Boot vcpu call is from tools via:
>*     do_domctl -> XEN_DOMCTL_setvcpucontext -> arch_set_info_guest
>*
>* Secondary vcpu's are brought up by the guest itself via:
>*     do_vcpu_op -> VCPUOP_initialise -> arch_set_info_guest
>*     (In case of linux, the call comes from cpu_initialize_context()).

So here you describe clearly that the function is to be called for each
vCPU exactly once. No vCPU == 0 special casing needed (also not in the
caller, as I understood you moved the check there - if not, all is fine).

>int vmx_pvh_vcpu_boot_set_info(struct vcpu *v, 
>struct vcpu_guest_context *ctxtp)
>{
>if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
>ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
>ctxtp->gdt.pvh.addr || ctxtp->gdt.pvh.limit ||
>ctxtp->fs_base || ctxtp->gs_base_user )
>return -EINVAL;

I assume there's be a place where these restrictions are both documented and
rationalized; otherwise this looks pretty arbitrary.

Jan

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-17 10:22                   ` Tim Deegan
@ 2013-08-19 21:45                     ` Mukesh Rathor
  0 siblings, 0 replies; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-19 21:45 UTC (permalink / raw)
  To: Tim Deegan; +Cc: xen-devel, Keir Fraser, Jan Beulich

On Sat, 17 Aug 2013 11:22:20 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:37 -0700 on 16 Aug (1376678229), Mukesh Rathor wrote:
> > Ok, changed the tools to clear fields, tested and everything works:
> > 
> > /*
> >  * Set vmcs fields during boot of a vcpu. Called from
> > arch_set_info_guest. *
> >  * Boot vcpu call is from tools via:
> >  *     do_domctl -> XEN_DOMCTL_setvcpucontext -> arch_set_info_guest
> >  *
> >  * Secondary vcpu's are brought up by the guest itself via:
> >  *     do_vcpu_op -> VCPUOP_initialise -> arch_set_info_guest
> >  *     (In case of linux, the call comes from
> > cpu_initialize_context()). *
> >  * Note, PVH save/restore is expected to happen the HVM way, ie,
> >  *        do_domctl -> XEN_DOMCTL_sethvmcontext -> hvm_load/save
> >  * and not get here.
> >  *
> >  * PVH 32bitfixme: this function needs to be modified for 32bit
> > guest.
> 
> What's the 32-bit interface going to be like?  Presumably it will have
> to load segment state, so this is just postponing that work until
> then.

Right. I think by tweaking guest mostly and xen some, taking 
advantage of the hvm container, we might be able to get 32bit down to 
bare minimum.

> >  */
> > int vmx_pvh_vcpu_boot_set_info(struct vcpu *v, 
> >                                struct vcpu_guest_context *ctxtp)
> > {
> >     if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
> >          ctxtp->user_regs.cs || ctxtp->user_regs.ss ||
> > ctxtp->user_regs.es || ctxtp->user_regs.ds || ctxtp->user_regs.fs
> > || ctxtp->user_regs.gs || ctxtp->gdt.pvh.addr ||
> > ctxtp->gdt.pvh.limit || ctxtp->fs_base || ctxtp->gs_base_user )
> >         return -EINVAL;
> > 
> >     vmx_vmcs_enter(v);
> >     __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> >     vmx_vmcs_exit(v);
> 
> Why does this one field get loaded?  Couldn't it be zero too?

With this both xen and guest can get rid of some code. Without it, 
the guest would need ugly hack, and vcpu bringup will be serialized.
So, this seems to be the most optimal solution. 

thanks
mukesh

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: RFC: PVH set vcpu info context in vmcs....
  2013-08-19  9:34                   ` Jan Beulich
@ 2013-08-19 21:46                     ` Mukesh Rathor
  0 siblings, 0 replies; 19+ messages in thread
From: Mukesh Rathor @ 2013-08-19 21:46 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, keir.xen, tim

On Mon, 19 Aug 2013 10:34:49 +0100
"Jan Beulich" <jbeulich@suse.com> wrote:

> >>> Mukesh Rathor <mukesh.rathor@oracle.com> 08/17/13 3:37 AM >>>
> >/*
> >* Set vmcs fields during boot of a vcpu. Called from
> >arch_set_info_guest. *
> >* Boot vcpu call is from tools via:
> >*     do_domctl -> XEN_DOMCTL_setvcpucontext -> arch_set_info_guest
> >*
> >* Secondary vcpu's are brought up by the guest itself via:
> >*     do_vcpu_op -> VCPUOP_initialise -> arch_set_info_guest
> >*     (In case of linux, the call comes from
> >cpu_initialize_context()).
> 
> So here you describe clearly that the function is to be called for
> each vCPU exactly once. No vCPU == 0 special casing needed (also not
> in the caller, as I understood you moved the check there - if not,
> all is fine).

No more check for vcpu==0 needed now.

> >int vmx_pvh_vcpu_boot_set_info(struct vcpu *v, 
> >struct vcpu_guest_context *ctxtp)
> >{
> >if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
> >ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
> >ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
> >ctxtp->gdt.pvh.addr || ctxtp->gdt.pvh.limit ||
> >ctxtp->fs_base || ctxtp->gs_base_user )
> >return -EINVAL;
> 
> I assume there's be a place where these restrictions are both
> documented and rationalized; otherwise this looks pretty arbitrary.

Yes, I'll document the API.

thanks
mukesh

^ permalink raw reply	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-08-19 21:47 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-13  1:45 RFC: PVH set vcpu info context in vmcs Mukesh Rathor
2013-08-13  9:16 ` Tim Deegan
2013-08-13 12:10   ` Jan Beulich
2013-08-13 10:56 ` Jan Beulich
2013-08-14  2:12   ` Mukesh Rathor
2013-08-14  9:12     ` Jan Beulich
2013-08-14  9:57       ` George Dunlap
2013-08-15  0:25       ` Mukesh Rathor
2013-08-15  1:58         ` Mukesh Rathor
2013-08-15  6:34           ` Jan Beulich
2013-08-15  6:31         ` Jan Beulich
2013-08-16  2:26           ` Mukesh Rathor
2013-08-16  7:28             ` Jan Beulich
2013-08-16 22:28               ` Mukesh Rathor
2013-08-17  1:37                 ` Mukesh Rathor
2013-08-17 10:22                   ` Tim Deegan
2013-08-19 21:45                     ` Mukesh Rathor
2013-08-19  9:34                   ` Jan Beulich
2013-08-19 21:46                     ` Mukesh Rathor

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.