Re: [PATCH RFC v12 16/21] pvh: Use PV handlers for emulated forced invalid ops, cpuid, and IO

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

On Wed, 18 Sep 2013 16:31:17 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@eu.citrix.com>
> >>> wrote:
> >      case EXIT_REASON_IO_INSTRUCTION:
> > -        exit_qualification = __vmread(EXIT_QUALIFICATION);
> > -        if ( exit_qualification & 0x10 )
> > +        if ( is_pvh_vcpu(v) )
> >          {
> > -            /* INS, OUTS */
> > -            if ( !handle_mmio() )
> > -                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > +            /*
> > +             * Note: A PVH guest sets IOPL natively by setting
> > bits in
> > +             *       the eflags, and not via hypercalls used by a
> > PV.
> > +             */
> > +            struct segment_register seg;
> > +            int requested = (regs->rflags & X86_EFLAGS_IOPL) >> 12;
> > +            int curr_lvl = (regs->rflags & X86_EFLAGS_VM) ? 3 : 0;
> > +            
> > +            if ( curr_lvl == 0 )
> > +            {
> > +                hvm_get_segment_register(current, x86_seg_ss,
> > &seg);
> > +                curr_lvl = seg.attr.fields.dpl;
> > +            }
> > +            if ( requested < curr_lvl
> > || !emulate_privileged_op(regs) )
> > +                hvm_inject_hw_exception(TRAP_gp_fault,
> > regs->error_code);
> 
> Now that I think about it once more, that's actually rather
> questionable. First of all - does a PVH guest see translated or
> untranslated I/O port space? With there not being a PV MMU, the
> former might seem more natural...
> 
> And then for the majority of I/O ports where Xen simply carries
> out the access on behalf of the guest, we could as well allow the
> guest to do the port access itself by clearing the respective flags
> in the bitmap. Once that is done, the question would then be
> whether any legitimate cases remain that require a call to
> emulate_privileged_op() here.

Good idea, I didn't know enough about what ports are allowed access
to do that.

> > @@ -1624,6 +1631,13 @@ 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
> > +     * and so don't need to check again here.
> > +     */
> > +    if ( is_pvh_vcpu(v) )
> > +        return 1;
> > +
> >      if ( !vm86_mode(regs) &&
> >           (v->arch.pv_vcpu.iopl >= (guest_kernel_mode(v, regs) ?
> > 1 : 3)) ) return 1;
> 
> Hmm, am I missing something here? The check in the VMEXIT
> handler is just a privilege level one - where's the bitmap being
> consulted? _If_ the bitmap is being maintained properly for the
> guest (which I don't recall having seen), anything leading here
> would be for ports the guest was not permitted access to. Yet
> we would happily emulate the access for it then.

Not sure I understand which bitmap needs to be consulted. The bitmap
hvm_io_bitmap is used to set the intercepts which PVH also uses, with
HVM defaults.

thanks
mukesh