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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@xxxxxxxxxxxxx> 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.
 
> +void propagate_page_fault(unsigned long addr, u16 error_code)
> +{
> +    is_pvh_vcpu(current)
> +        ? hvm_inject_page_fault(error_code, addr)
> +        : pv_inject_page_fault(addr, error_code);

Even if not written down in CODINGSTYLE, the majority of other
cases in the code has the operators last thing on a line rather than
first.

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

Jan


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