[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 09/18] PVH xen: Support privileged op emulation for PVH
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |