Re: [Xen-devel] [PATCH 04/18] PVH xen: add params to read_segment_register

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.13 at 03:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Fri, 31 May 2013 11:00:12 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 25.05.13 at 03:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > @@ -240,10 +240,10 @@ void do_double_fault(struct cpu_user_regs
>> > *regs) crs[2] = read_cr2();
>> >      crs[3] = read_cr3();
>> >      crs[4] = read_cr4();
>> > -    regs->ds = read_segment_register(ds);
>> > -    regs->es = read_segment_register(es);
>> > -    regs->fs = read_segment_register(fs);
>> > -    regs->gs = read_segment_register(gs);
>> > +    regs->ds = read_segment_register(current, regs, ds);
>> > +    regs->es = read_segment_register(current, regs, es);
>> > +    regs->fs = read_segment_register(current, regs, fs);
>> > +    regs->gs = read_segment_register(current, regs, gs);
>> 
>> In patch 9 you start using the first parameter of
>> read_segment_register() in ways not compatible with the use of
>> current here - the double fault handler (and in general all host side
>> exception handling code, i.e. the change to show_registers() is
>> questionable too) wants to use the real register value, not what's
>> in regs->. Even more, with the VMEXIT code storing at best
>> a known bad value into these fields, is it really valid to use them
>> at all (i.e. things ought to work much like the if() portion of
>> show_registers() which you _do not_ modify).
> 
> Right, in case of double fault we'd need the real values. 
> The only thing comes to mind:
> 
> #define read_segment_register(vcpu, regs, name)                   \
> ({  u16 __sel;                                                    \
>     struct cpu_user_regs *_regs = (regs);                         \
>                                                                   \
>     if ( guest_mode(regs) && is_pvh_vcpu(vcpu) )         <==========

Might be okay, albeit if so I'd like the two sides of the && switched.
But see below...

>         __sel = _regs->name;                                      \
>     else                                                          \
>         asm volatile ( "movw %%" #name ",%0" : "=r" (__sel) );    \
>     __sel;                                                        \
> })                  
> 
> but let me verify this would work for all possible contect_switch ->
> save_segments() calls.
> 
> BTW, I can't use current in the macro because of call from save_segments(). 
> 
>> at all (i.e. things ought to work much like the if() portion of
>> show_registers() which you _do not_ modify).
> 
> Yeah, it was on hold because I've been investigating guest_cr[] sanity,
> and found that I was missing:
> 
>     v->arch.hvm_vcpu.guest_cr[4] = value;
> 
> So, my next version will add that and update show_registers() for PVH.
> I can scratch off another fixme from my list.

But you don't answer the underlying question that I raised: Is
accessing the struct cpu_user_regs selector register fields valid
at all? Again, the #VMEXIT handling code only stores garbage
into them (in debug builds, in non-debug builds it simply leaves
the fields unaltered).

> BTW, In the process I realized in the cr4 update intercept I am missing:
> 
>     if ( value & HVM_CR4_GUEST_RESERVED_BITS(v) )
>     {
>         HVM_DBG_LOG(DBG_LEVEL_1,
>                     "Guest attempts to set reserved bit in CR4: %lx",
>                     value);
>         goto gpf;
>     }
> 
>     if ( !(value & X86_CR4_PAE) && hvm_long_mode_enabled(v) )
>     {
>         HVM_DBG_LOG(DBG_LEVEL_1, "Guest cleared CR4.PAE while "
>                     "EFER.LMA is set");
>         goto gpf;
>     }
> 
> I can't recall now whether I somehow concluded I didn't need to worry about
> it for PVH since I was only thinking 64bit, or just missed it. 
> I guess I should have the check even if I expect the guest to always
> be in LME, right?

Yes, I think this puts you on the safe side.

Jan


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