Re: [PATCH 10/17] PVH xen: introduce vmx_pvh.c and pvh.c

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

On Fri, 03 May 2013 07:33:50 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 03.05.13 at 02:40, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Thu, 02 May 2013 07:53:18 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >     if ( (tmp_ar & X86_SEG_AR_CS_LM_ACTIVE) && selector <
> > x86_seg_fs  )
> 
> This is still wrong. As said before you need to look as the _CS_
> access rights, not the ones of the selector register you read.

Hmm... unless I'm reading the SDM wrong, it says "for non-code segments
bit 21 is reserved and should always be set to 0". But its prob clearer
to check for _CS_ only. 

> But as also hinted at - do you really need the override at all?

Yes, because of the following check in insn_fetch macro:

     "(eip) > (limit) - (sizeof(_x) - 1)" in the if statment:

   if ( (limit) < sizeof(_x) - 1 || (eip) > (limit) - (sizeof(_x) - 1) )   \
       goto fail;                                                          \

Reading vmcs would return 32bit limit of 0xffffffff. BTW, same override
exists in read_descriptor() (it seems to do the override for FS and
GS also, which I don't understand).


Anyways, thanks to hvm_get_segment_register(), I got rid of the function 
vmx_pvh_read_descriptor():

static int read_descriptor_sel(unsigned int sel,
                               enum x86_segment which_sel,
                               struct vcpu *v,
                               const struct cpu_user_regs *regs,
                               unsigned long *base,
                               unsigned long *limit,
                               unsigned int *ar,
                               unsigned int vm86attr)
{
    if ( is_pvh_vcpu(v) )
    {
        struct segment_register seg;

        hvm_get_segment_register(v, which_sel, &seg);
        *ar = (unsigned int)seg.attr.bytes;

        /* ar is returned packed as in segment_attributes_t. fix it up */
        *ar = (*ar & 0xff ) | ((*ar & 0xf00) << 4);
        *ar = *ar << 8;

        if ( (vm86attr & _SEGMENT_CODE) && (*ar & _SEGMENT_L) &&
             (which_sel < x86_seg_fs) )
        {
            *base = 0UL;
            *limit = ~0UL;
        }
        else
        {
            *base = (unsigned long)seg.base;
            *limit = (unsigned long)seg.limit;
        }

        return 1;
    }

    return read_descriptor(sel, v, regs, base, limit, ar, vm86attr);

}