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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.04.18 at 18:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Well, okay then:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



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