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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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