Re: [Xen-devel] RFC: PVH set vcpu info context in vmcs....

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 13 Aug 2013 11:56:36 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 13.08.13 at 03:45, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> 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


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