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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/12/16 10:53, Jan Beulich wrote:
>
>>          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.

Hmm - this was actually already broken with the nextrip adjustment.  I
will introduce a fixup at the end of this switch statement covering both
fields.

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

At the moment, the sole user is

    swint:
        rc = inject_swint(swint_type, (uint8_t)src.val,
                          _regs.eip - ctxt->regs->eip,
                          ctxt, ops) ? : X86EMUL_EXCEPTION;
        goto done;

so the description is correct.

We currently have a uniform pattern of the injection functions returning
OKAY, and being converted to EXCEPTION by the caller.  If this wants
fixing at all, it wants fixing later when switching to using
x86_emul_software_event().

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

I had already noticed and fixed this.

~Andrew

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