Re: [PATCH 07/11] x86/emul: Add pending_dbg field to x86_event

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/06/18 13:34, Jan Beulich wrote:
>>>> On 04.06.18 at 15:59, <andrew.cooper3@citrix.com> wrote:
>> @@ -82,9 +83,16 @@ void pv_inject_event(const struct x86_event *event)
>>              error_code |= PFEC_user_mode;
>>  
>>          trace_pv_page_fault(event->cr2, error_code);
>> -    }
>> -    else
>> +        break;
>> +
>> +    case TRAP_debug:
>> +        curr->arch.dr6 |= event->pending_dbg;
> Considering what you've been telling me over and over, shouldn't
> you mask out the low four bits here before ORing in new state?

Yes, but that is covered in a later patch (9 to be specific) once the
HVM side is fixed to not update %dr6 twice.

Until all paths have been updated to touch %dr6 exactly once,
implementing the overwriting of the bottom 4 bits will lead to losing
all breakpoint information all the time, which is worse guest behaviour
than having them act as if they were sticky (which works in practice as
the #DB handlers reset %dr6 to 0).

>
>> +        /* Fallthrough */
>> +
>> +    default:
>>          trace_pv_trap(vector, regs->rip, use_error_code, error_code);
> I also wonder whether tracing wouldn't benefit from being informed
> about pending_dbg here instead of the error code. Not something
> for this patch (or series) of course, just as a remark.
>
> Since the equivalent HVM code looks to be missing, I take it that's
> going to be addressed in later patches of the series?

Tracing is a mess, and completely inconsistent.  I'm opting for "no
functional change" until I've got time to rationalise it all.

>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -88,7 +88,10 @@ struct x86_event {
>>      uint8_t       type;         /* X86_EVENTTYPE_* */
>>      uint8_t       insn_len;     /* Instruction length */
>>      int32_t       error_code;   /* X86_EVENT_NO_EC if n/a */
>> -    unsigned long cr2;          /* Only for TRAP_page_fault h/w exception */
>> +    union {
>> +        unsigned long cr2;         /* #PF */
>> +        unsigned long pending_dbg; /* #DB (new DR6 bits, positive polarity) */
>> +    };
>>  };
> Seeing that this is the only x86_emulate* change, I don't suppose
> you fancy making the emulator correctly raise X86_DR6_BD at the
> same time?

Access to %dr registers is only implemented for PV guests at the
moment.  HVM guests don't provide {read,write}_dr() hooks.  Furthermore,
PV guests are disallowed from enabling general detect.

I toyed with supporting GD for PV guests, but it complicates the
hypercall access, and I'm not sure whether it is worth supporting.  The
feature exists for In Circuit Emulators, and PV guests have been fine
without the ability to use it for more than a decade now.

For HVM guests, I do plan to hook up full emulation, once I've
rationalised the state handling logic.

~Andrew

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