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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 09/08/13 07:54, Jan Beulich wrote:
>>>> On 09.08.13 at 03:32, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>> On Thu, 8 Aug 2013 15:18:56 +0100
>> George Dunlap <George.Dunlap@eu.citrix.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?
> 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