RFC: PVH set vcpu info context in vmcs....

RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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;
}

Re: RFC: PVH set vcpu info context in vmcs....

[Tim Deegan]

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;
> }

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

>>> 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;
> }

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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;
}

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[George Dunlap]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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;
}

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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);
}

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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;
}

Re: RFC: PVH set vcpu info context in vmcs....

[Tim Deegan]

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.

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Jan Beulich]

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

Re: RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor]

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