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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.04.18 at 13:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/04/18 09:31, Jan Beulich wrote:
>>>>> On 12.04.18 at 18:55, <andrew.cooper3@xxxxxxxxxx> 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.

I agree.

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

That doesn't sound like a problem, though - the hypercall path would
pass NULL there as well.

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

Oh, right you are.

Jan



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