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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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.
 -George

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