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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

And of course your use of read_segment_register(v, regs, cs)
above is all but correct - CS and SS need to be read from regs.

Jan


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