Re: [V10 PATCH 09/23] PVH xen: introduce pvh_set_vcpu_info() and vmx_pvh_set_vcpu_info()

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

On Thu, 08 Aug 2013 07:56:41 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 08.08.13 at 03:05, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 05 Aug 2013 12:10:15 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 24.07.13 at 03:59, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > +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;
> >> > +
> >> > +    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);
> >> 
> >> Just noticed: Aren't you mixing up entries and bytes here?
> > 
> > Right:
> > 
> > __vmwrite(GUEST_LDTR_LIMIT, (ctxtp->ldt_ents * 8 - 1) );
> > 
> > Any formatting issues here? I don't see in coding style, and see
> > both code where there is a space around '*' and not.
> 
> The inner parentheses are superfluous.
> 
> CODING_STYLE is pretty explicit about there needing to be white
> space around operators: "Spaces are placed [...], and around
> binary operators (except the structure access operators, '.' and
> '->')."
> 
> > Also, when setting the limit, do we need to worry about the G flag?
> > or for that matter, D/B whether segment is growing up or down?
> > It appears we don't need to worry about that for LDT, but not sure
> > reading the SDMs..
> 
> The D/B bit doesn't matter for LDT (and TSS), but the G bit would.
> However - now that you're intending to require trivial state (64-bit
> CS, all other selectors zero), it would only be logical to also
> require a zero LDT selector (and hence base and entry count to be
> zero).

No, Tim would like to see the hcall set the hidden fields. Since, you 
are also OK with that, I'm just doing that now. So, we just build 
default VMCS like we do now, and then this hcall will let them set
the selectors, and all hidden fields (implicitly).

thanks
mukesh