Re: [Xen-devel] [PATCH 09/18] PVH xen: Support privileged op emulation for PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> @@ -1524,6 +1528,49 @@ static int read_descriptor(unsigned int sel,
>      return 1;
>  }
>  
> +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)
> +{
> +    struct segment_register seg;
> +    unsigned int long_mode = 0;

Pointless initializer and bogus type.

> +
> +    if ( !is_pvh_vcpu(v) )
> +        return read_descriptor(sel, v, regs, base, limit, ar, vm86attr);
> +
> +    hvm_get_segment_register(v, x86_seg_cs, &seg);
> +    long_mode = seg.attr.fields.l;
> +
> +    if ( which_sel != x86_seg_cs )
> +        hvm_get_segment_register(v, which_sel, &seg);
> +
> +    /* "ar" is returned packed as in segment_attributes_t. Fix it up. */
> +    *ar = (unsigned int)seg.attr.bytes;

Is the cast really needed for anything here?

> +    *ar = (*ar & 0xff ) | ((*ar & 0xf00) << 4);
> +    *ar = *ar << 8;

Preferably fold this into the prior expression or use <<=.

> +
> +    if ( long_mode )
> +    {
> +        *limit = ~0UL;
> +
> +        if ( which_sel < x86_seg_fs )
> +        {
> +            *base = 0UL;
> +            return 1;
> +        }
> +   }
> +   else
> +       *limit = (unsigned long)seg.limit;

Again - is the cast really needed for anything here?

> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -4,10 +4,20 @@
>  #include <xen/lib.h>
>  #include <xen/bitops.h>
>  
> -#define read_segment_register(vcpu, regs, name)                 \
> -({  u16 __sel;                                                  \
> -    asm volatile ( "movw %%" STR(name) ",%0" : "=r" (__sel) );  \
> -    __sel;                                                      \
> +/*
> + * We need vcpu because during context switch, going from PVH to PV,
> + * in save_segments(), current has been updated to next, and no longer 
> pointing
> + * to the PVH.
> + */

This is bogus - you shouldn't need any of the {save,load}_segment()
machinery for PVH, and hence this is not a valid reason for adding a
vcpu parameter here.

> +#define read_segment_register(vcpu, regs, name)                   \
> +({  u16 __sel;                                                    \
> +    struct cpu_user_regs *_regs = (regs);                         \

_If_ these changes need to remain, please const-qualify this
pointer to clarify the intention of not modifying anything.

> +                                                                  \
> +    if ( is_pvh_vcpu(vcpu) && guest_mode(regs) )                  \

Need to use _regs here too.

> +        __sel = _regs->name;                                      \

By now (going through the series sequentially) I don't think I've
seen code writing these fields, so reading them can't logically be
done at this point. Or did I overlook anything?

If reordering this would cause a lot of grief, I'd be tolerant of
this provided a note gets added to the commit message.

Jan


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