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

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

On Thu, 04 Jul 2013 09:04:48 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 04.07.13 at 04:00, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Wed, 03 Jul 2013 11:21:20 +0100 "Jan Beulich"
> > <JBeulich@suse.com> wrote:
> >> Even if it really is (which I doubt), you still would make PVH
> >> different from both PV and HVM, which both don't populate the
> >> selector fields of the frame (PV obviously has ->cs and ->ss
> >> populated [by the CPU], but HVM avoids even that).
> > 
> > And what's wrong with PVH being little different?
> 
> There's nothing wrong with this as long as it's for a useful purpose,
> and without introducing hidden dependencies (the latter is what is
> happening here). Being different just for the purpose of being
> different is not desirable (and likely not even acceptable, as in any
> case this makes code more difficult to understand).
> 
> >> We'll have to see - at the first glance I don't follow...
> > 
> > Here's what I am talking about:
> > 
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1241,10 +1241,10 @@ static void save_segments(struct vcpu *v)
> >      struct cpu_user_regs *regs = &v->arch.user_regs;
> >      unsigned int dirty_segment_mask = 0;
> >  
> > -    regs->ds = read_segment_register(v, regs, ds);
> > -    regs->es = read_segment_register(v, regs, es);
> > -    regs->fs = read_segment_register(v, regs, fs);
> > -    regs->gs = read_segment_register(v, regs, gs);
> > +    regs->ds = read_segment_register(v, regs, x86_seg_ds);
> > +    regs->es = read_segment_register(v, regs, x86_seg_es);
> > +    regs->fs = read_segment_register(v, regs, x86_seg_fs);
> > +    regs->gs = read_segment_register(v, regs, x86_seg_gs);
> 
> This I think is completely pointless a change if you keep the thing
> being a macro (using token concatenation):

I find the ## to be disgusting because it makes code much harder to
read/understand as one cannot use cscope/grep to find things. But,
I'll make the change as suggested.

thanks
Mukesh