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

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 31 May 2013 11:00:12 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.05.13 at 03:25, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> 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) )         <==========
        __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.

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?

thanks
mukesh