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 Mon, 12 Aug 2013 10:00:36 +0100
Tim Deegan <tim@xen.org> wrote:

> At 16:41 -0700 on 09 Aug (1376066498), Mukesh Rathor wrote:
> > 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.
> > 
> > Ugh, to find the G bit, I need to walk the GDT to find the LDT
> > descriptor.
> 
> Why so?  The caller supplies you with the LDT base and range, not a
> segment selector.  I don't think you could find the right LDT selector
> by scanning the GDT anyway -- what if there were two that matched?

Because the range is interpreted in bytes or pages depending on the G
bit in the descriptor. But, it seems acceptable to require it be null,
so i'll just do that.

Mukesh