Re: [V10 PATCH 12/23] PVH xen: Support privileged op emulation for PVH

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

On Thu, 8 Aug 2013 15:18:56 +0100
George Dunlap <George.Dunlap@eu.citrix.com> wrote:

> On Thu, Aug 8, 2013 at 2:59 AM, Mukesh Rathor
> <mukesh.rathor@oracle.com> wrote:
> > On Wed, 7 Aug 2013 14:49:50 +0100
> > George Dunlap <dunlapg@umich.edu> wrote:
> >
> >> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
> >> <mukesh.rathor@oracle.com> wrote:
.......
> The problem I have is that you still pass in *both* the value of
> regs->$SEGMENT_REGISTER, *and* an enum of a segment register, and use
> one in one case, and another in a different case.  That's just a
> really ugly interface.
> 
> What I'd like to see is for read_descriptor_sel() to *just* take
> which_sel (perhaps renamed sreg or something, since it's referring to
> a segment register), and in the PV case, read the appropriate segment
> register, then calling read_descriptor().  Then you don't have this
> crazy thing where you set two variables (sel and which_cs) all over
> the place.


Hmm... lemme make sure I understand precisely, what you mean is
something like:

static int read_descriptor_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)

{
    uint sel;
    if (!pvh)
    {
        sel = read_pv_segreg(which_sel)
        return read_descriptor(sel, v, regs, base, limit, ar, vm86attr);
    }
}

where read_pv_segreg() has one long case statment:
   case x86_seg_cs
       return read_segment_register(v, regs, cs);
   case x86_seg_cs
       return read_segment_register(v, regs, ds);
       .....


Then emulate_privileged_op() will not be setting data_sel, but 
only which_sel, except for one place:

....
            if ( lm_ovr == lm_seg_none || data_sel < 4 )
            {
                switch ( lm_ovr )
                {
                case lm_seg_none:
...

That sounds like a good change to me. Jan, you OK with this?


-mukesh