Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

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

On 13/04/18 09:31, Jan Beulich wrote:
>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>> do_get_debugreg() has several bugs:
>>
>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>    %cr4.de is disabled.
>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>    with zero.
>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>    near the top of the address space.
>>
>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>> add HVM support) which separates its success/failure indication from the data
>> value, and have do_get_debugreg() call into the handler.
> The HVM part here is sort of questionable because of your use of
> curr->arch.pv_vcpu.ctrlreg[4].

That is what the "needs further plumbing" refers to, as well as needing
hooks to get/modify %dr6/7 from the VMCB/VMCS.

However, we are gaining an increasing amount of common x86 code which
needs to read control register values, and I've got a plan to refactor
across the board to v->arch.cr4 (and similar).  There is no point having
identical information in different parts of sub-unions.

> This is appropriate for the NULL ctxt case,
> but it's already a layering violation for the use of the function in
> priv_op_ops, where the read_cr() hook should be used instead.

Hmm - doing this, while probably the better long temr course of action,
would require passing the ops structures down into the callbacks.

>
>> +int x86emul_read_dr(unsigned int reg, unsigned long *val,
>> +                    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    struct vcpu *curr = current;
>> +
>> +    /* HVM support requires a bit more plumbing before it will work. */
>> +    ASSERT(is_pv_vcpu(curr));
>> +
>> +    switch ( reg )
>> +    {
>> +    case 0 ... 3:
>> +    case 6:
>> +        *val = curr->arch.debugreg[reg];
>> +        break;
>> +
>> +    case 7:
>> +        *val = (curr->arch.debugreg[7] |
>> +                curr->arch.debugreg[5]);
>> +        break;
>> +
>> +    case 4 ... 5:
>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>> +        {
>> +            *val = curr->arch.debugreg[reg + 2];
>> +            break;
> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
> DR5) value here?

[5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
patch), as IO breakpoints are only valid to use when %cr4.de is enabled.

I haven't yet investigated the native behaviour of selecting an IO
breakpoint, then disabling Debug Extensions.

That said, one of my TODO items is to allow PV guests to use General
Detect (because its trivial to implement), at which point [5] will turn
from an IO breakpoint shadow, into a more general %dr7 shadow, and ORing
it in here will matter.

~Andrew

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