Re: [Xen-devel] [PATCH v3 10/24] x86/emul: Always use fault semantics for software events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.16 at 14:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -1242,27 +1242,38 @@ static void svm_inject_event(const struct x86_event 
> *event)
>      eventinj.fields.v = 1;
>      eventinj.fields.vector = _event.vector;
>  
> -    /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */
> +    /*
> +     * Refer to AMD Vol 2: System Programming, 15.20 Event Injection.
> +     *
> +     * On hardware lacking NextRIP support, and all hardware in the case of
> +     * icebp, software events with trap semantics need emulating, so %eip in
> +     * the trap frame points after the instruction.
> +     *
> +     * The x86 emulator (if requested by the x86_swint_emulate_* choice) will
> +     * have performed checks such as presence/dpl/etc and believes that the
> +     * event injection will succeed without faulting.
> +     *
> +     * The x86 emulator will always provide fault semantics for software
> +     * events, with _trap.insn_len set appropriately.  If the injection
> +     * requires emulation, move %eip forwards at this point.
> +     */
>      switch ( _event.type )
>      {
>      case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
> -        /*
> -         * Software interrupts (type 4) cannot be properly injected if the
> -         * processor doesn't support NextRIP.  Without NextRIP, the emulator
> -         * will have performed DPL and presence checks for us, and will have
> -         * moved eip forward if appropriate.
> -         */
>          if ( cpu_has_svm_nrips )
>              vmcb->nextrip = regs->eip + _event.insn_len;
> +        else
> +            regs->eip += _event.insn_len;

Please use ->rip here and below (and perhaps also in the comment).

>          eventinj.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>          break;
>  
>      case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>          /*
> -         * icebp's injection must always be emulated.  Software injection 
> help
> -         * in x86_emulate has moved eip forward, but NextRIP (if used) still
> -         * needs setting or execution will resume from 0.
> +         * icebp's injection must always be emulated, as hardware does not
> +         * special case HW_EXCEPTION with vector 1 (#DB) as having trap
> +         * semantics.
>           */
> +        regs->eip += _event.insn_len;
>          if ( cpu_has_svm_nrips )
>              vmcb->nextrip = regs->eip;
>          eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> @@ -1270,16 +1281,13 @@ static void svm_inject_event(const struct x86_event 
> *event)
>  
>      case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
>          /*
> -         * The AMD manual states that .type=3 (HW exception), .vector=3 or 4,
> -         * will perform DPL checks.  Experimentally, DPL and presence checks
> -         * are indeed performed, even without NextRIP support.
> -         *
> -         * However without NextRIP support, the event injection still needs
> -         * fully emulating to get the correct eip in the trap frame, yet get
> -         * the correct faulting eip should a fault occur.
> +         * Hardware special cases HW_EXCEPTION with vectors 3 and 4 as having
> +         * trap semantics, and will perform DPL checks.
>           */
>          if ( cpu_has_svm_nrips )
>              vmcb->nextrip = regs->eip + _event.insn_len;
> +        else
> +            regs->eip += _event.insn_len;
>          eventinj.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>          break;

By moving the adding to reg->rip here, you bypass x86_emulate()'s
zeroing of the upper 32 bits outside of 64-bit mode, so you now need
to replicate it below here.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1694,8 +1694,6 @@ static int inject_swint(enum x86_swint_type type,
>                      goto raise_exn;
>              }
>          }
> -
> -        ctxt->regs->eip += insn_len;
>      }
>  
>      rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt);

I think for the patch description to be correct, this call's return value
needs to be altered, for inject_swint() to return EXCEPTION when
OKAY comes back here (which iirc you do in a later patch when you
eliminate this hook).

> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -64,7 +64,13 @@ enum x86_swint_type {
>      x86_swint_int,   /* 0xcd $n */
>  };
>  
> -/* How much help is required with software event injection? */
> +/*
> + * How much help is required with software event injection?
> + *
> + * All software events return from x86_emulate() with X86EMUL_EXCEPTION and
> + * fault-like semantics.  This just controls whether the emulator performs
> + * presence/dpl/etc checks and possibly raises excepions instead.

exceptions

Jan

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