[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 09/08/13 07:54, Jan Beulich wrote:
On 09.08.13 at 03:32, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
On Thu, 8 Aug 2013 15:18:56 +0100
George Dunlap <George.Dunlap@xxxxxxxxxxxxx> 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?
It's worse performance wise, but better maintenance wise, so I
guess I don't really object (but also am not too happy with it).

Is this a really hot path?

It does mean going through a bit of extra code in the simple version. In theory one could do something with arrays or something to make that to avoid it.

In any case, I think the interface in the patch is really ugly, but I'll leave it up to Keir and Jan what they want to do.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.