Re: [Xen-devel] [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/06/18 14:00, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1437,19 +1437,49 @@ static void svm_inject_event(const struct x86_event 
>> *event)
>>      switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) )
>>      {
>>      case TRAP_debug:
>> -        if ( regs->eflags & X86_EFLAGS_TF )
>> -        {
>> -            __restore_debug_registers(vmcb, curr);
>> -            vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP);
>> -        }
>> +        /*
>> +         * On AMD hardware, a #DB exception:
>> +         *  1) Merges new status bits into %dr6
>> +         *  2) Clears %dr7.gd and MSR_DEBUGCTL.{LBR,BTF}
>> +         *
>> +         * Item 1 is done by hardware before a #DB intercepted vmexit, but 
>> we
>> +         * may end up here from emulation so have to repeat it ourselves.
>> +         * Item 2 is done by hardware when injecting a #DB exception.
>> +         */
>> +        __restore_debug_registers(vmcb, curr);
>> +        vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->pending_dbg);
>> +
>>          /* fall through */
>>      case TRAP_int3:
>>          if ( curr->domain->debugger_attached )
>>          {
>>              /* Debug/Int3: Trap to debugger. */
>> +            if ( _event.vector == TRAP_int3 )
>> +            {
>> +                /* N.B. Can't use __update_guest_eip() for risk of 
>> recusion. */
>> +                regs->rip += _event.insn_len;
> Not all callers provide a non-zero insn length. Is that a potential
> problem here (and in the equivalent VMX code)?

TRAP_int3 is strictly a software exception (and/or software interrupt if
started via `int $n`), so should always have a nonzero length.

We should be able to assert this property, and I was considering some
extra checks in the {pv,hvm}_inject_event() logic, although it would
require XEN_DMOP_inject_event gaining more sanity checking than it
currently has (which is probably a good thing).

I'll fold something suitable into v2, which is already quite a bit
longer to help with monitor problem identified here.

>> @@ -2775,67 +2805,46 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>  
>>      case VMEXIT_ICEBP:
>>      case VMEXIT_EXCEPTION_DB:
>> -        if ( !v->domain->debugger_attached )
>> +    case VMEXIT_EXCEPTION_BP:
>> +    {
>> +        unsigned int vec, type, len, extra;
>> +
>> +        switch ( exit_reason )
>>          {
>> -            int rc;
>> -            unsigned int trap_type;
>> +        case VMEXIT_ICEBP:
> I don't understand this structuring of the code: The outer switch()
> has the exact same control expression, and there's no fall through
> - why the nesting? Move the variable declarations to the beginning
> of the outer switch() and drop the inner one? You may not even
> need all of the variables if you replicated the little bit of code
> currently shared by the three (immediately after the inner switch()).

Good question - I think this layout is a side effect of me changing my
mind several times during its development.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel