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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/04/18 13:32, Jan Beulich wrote:
>>>> On 16.04.18 at 14:18, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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.
> This ...
>
>>>>>> +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.
> ... is what I'd prefer to be taken care of without delaying to the time when
> we make this work for HVM as well. Unless you feel really strongly about it
> being better the way you have it, in which case you may feel free to add
> my ack.

In all PV cases (hypercall and emulation), the current code functions
correctly, because DE is active in context.

In principle, the emulation case would be better if it used the
read_cr() hook, but that is invasive to arrange (which is why I chose
not to at this point), and still needs special casing for the hypercall
case anyway.

~Andrew

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