Re: [RFC PATCH 12/16]: PVH xen: return PVH features during creation, etc...

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

On Mon, 14 Jan 2013 12:20:12 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 12.01.13 at 03:07, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -910,6 +915,10 @@ int emulate_forced_invalid_op(struct cpu
> >      /* Check for forced emulation signature: ud2 ; .ascii "xen". */
> >      if ( (rc = copy_from_user(sig, (char *)eip, sizeof(sig))) !=
> > 0 ) {
> > +        /* PVH: fixme: hmm... what do we do for PVH? */
> > +        if ( is_pvh_vcpu(current) )
> 
> The fixme and check ought to sit earlier - the copy_from_user()
> above isn't valid there. And I don't see how you would validly
> get here anyway - you don't need to intercept GP faults to
> emulate guest CPUID invocations.

Yup, I need raw_copy like later. I guess I went back and forth
between supporting XEN_EMULATE_PREFIX or not, since a cpuid can
be trapped via vmexit. But we need to support it from user apps, so I
need to fix this to raw_copy. 

> I don't think so - propagate_page_fault() should do the right thing
> in that case, if you can validly get here for a PVH guest.

Agree, I need to make propgate_page_fault() inject PF into the PVH
guest. Working on it now.

> > @@ -1566,6 +1586,10 @@ static int guest_io_okay(
> >      int user_mode = !(v->arch.flags & TF_kernel_mode);
> >  #define TOGGLE_MODE() if ( user_mode ) toggle_guest_mode(v)
> >  
> > +    /* for PVH we check this in vmexit for
> > EXIT_REASON_IO_INSTRUCTION */
> > +    if (is_pvh_vcpu(v))
> 
> The why would it get here at all?

From, emulate_privileged_op(). I should change the comment to say we 
don't need to check again, as we check at vmexit. We won't get to 
emulate_privileged_op() if check fails. Easier to add that in guest_io_okay()
than to change every place in emulate_privileged_op() where guest_io_okay()
is called and not call it for PVH. 

> > @@ -2132,7 +2157,8 @@ int emulate_privileged_op(struct cpu_use
> >  
> >      case 0xfa: /* CLI */
> >      case 0xfb: /* STI */
> > -        if ( v->arch.pv_vcpu.iopl < (guest_kernel_mode(v, regs) ?
> > 1 : 3) )
> > +        if ( !is_pvh_vcpu(v)  &&
> 
> This ought to be impossible.

You mean call to emulate STI/CLI for PVH. Correct. I could just 
remove it. I went thru looking for places that were using pv_vcpu.iopl.

> > @@ -444,6 +444,8 @@ static long register_guest_callback(stru
> >      long ret = 0;
> >      struct vcpu *v = current;
> >  
> > +    NO_PVH_ASSERT_VCPU(v);
> 
> Either the code is unreachable for a PVH guest (in which case the
> assert is likely superfluous, or you need to return an error here
> rather than asserting.

superfluous, hence it's a debug assert to catch any places I might
have missed. I plan to remove them later when PVH is stable. Hope 
it can stay for a little bit :). 

thanks,
Mukesh