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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/04/18 12:39, Jan Beulich wrote:
>>>> 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.

So, are your comments suitably addressed?  It is unclear whether you
want any changes to be made.

~Andrew

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