Re: [PATCH v3 2/3] x86/PV: support data breakpoint extension registers

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/11/2014 04:32 PM, Jan Beulich wrote:
>>>> On 11.04.14 at 16:58, <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, Apr 7, 2014 at 10:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> @@ -854,7 +854,42 @@ long arch_do_domctl(
>>>               evc->vmce.mci_ctl2_bank0 = v->arch.vmce.bank[0].mci_ctl2;
>>>               evc->vmce.mci_ctl2_bank1 = v->arch.vmce.bank[1].mci_ctl2;
>>>
>>> -            ret = 0;
>>> +            i = ret = 0;
>>> +            if ( boot_cpu_has(X86_FEATURE_DBEXT) )
>>> +            {
>>> +                unsigned int j;
>>> +
>>> +                if ( v->arch.pv_vcpu.dr_mask[0] )
>>> +                {
>>> +                    if ( i < evc->msr_count && !ret )
>>> +                    {
>>> +                        msr.index = MSR_AMD64_DR0_ADDRESS_MASK;
>>> +                        msr.reserved = 0;
>>> +                        msr.value = v->arch.pv_vcpu.dr_mask[0];
>>> +                        if ( copy_to_guest_offset(evc->msrs, i, &msr, 1) )
>> Sorry if I missed something, but is there any bounds-checking on this
>> buffer?  I don't see any specification in the header file about how
>> big the buffer needs to be, nor any conceptual limit to the number of
>> elements which might be copied into it -- at least in this path.
> The conceptual limit is 64k (due to evc->msr_count being a 16 bit
> quantity). The limit up to which we may write to the buffer is the
> passed in evc->msr_count (which gets updated at the end to the
> number we would have wanted to write).

Dur -- sorry, the first time I went through this patch (last week) I 
noticed the "i < evc->msr_count", but somehow I missed it the second 
time through.

>
>>> @@ -3628,7 +3660,27 @@ long do_set_trap_table(XEN_GUEST_HANDLE_
>>>       return rc;
>>>   }
>>>
>>> -long set_debugreg(struct vcpu *v, int reg, unsigned long value)
>>> +void activate_debugregs(const struct vcpu *curr)
>>> +{
>>> +    ASSERT(curr == current);
>>> +
>>> +    write_debugreg(0, curr->arch.debugreg[0]);
>>> +    write_debugreg(1, curr->arch.debugreg[1]);
>>> +    write_debugreg(2, curr->arch.debugreg[2]);
>>> +    write_debugreg(3, curr->arch.debugreg[3]);
>>> +    write_debugreg(6, curr->arch.debugreg[6]);
>>> +    write_debugreg(7, curr->arch.debugreg[7]);
>>> +
>>> +    if ( boot_cpu_has(X86_FEATURE_DBEXT) )
>>> +    {
>>> +        wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[0]);
>>> +        wrmsrl(MSR_AMD64_DR1_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[1]);
>>> +        wrmsrl(MSR_AMD64_DR2_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[2]);
>>> +        wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, curr->arch.pv_vcpu.dr_mask[3]);
>>> +    }
>>> +}
>>> +
>>> +long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>>>   {
>>>       int i;
>>>       struct vcpu *curr = current;
>>> @@ -3709,11 +3761,8 @@ long set_debugreg(struct vcpu *v, int re
>>>               if ( (v == curr) &&
>>>                    !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
>>>               {
>>> -                write_debugreg(0, v->arch.debugreg[0]);
>>> -                write_debugreg(1, v->arch.debugreg[1]);
>>> -                write_debugreg(2, v->arch.debugreg[2]);
>>> -                write_debugreg(3, v->arch.debugreg[3]);
>>> -                write_debugreg(6, v->arch.debugreg[6]);
>>> +                activate_debugregs(curr);
>> So in the other place you replaced with activate_debugregs(), it
>> writes DR7; but here it doesn't -- and yet the function will write
>> DR7.  Is that a problem?
> It's not strictly a problem (because right above we checked that
> no breakpoints are active, and right below we write it with the
> intended value), but it shouldn't be done to be on the safe side
> as well as because the double write is needlessly expensive. And
> I do recall that I made a mental note that the DR7 write needs to
> remain separate - only to then forget it. I should clearly have
> written it down. So thanks a lot for spotting this!

Easy to do. :-)

  -George