Re: [PATCH v4 1/2] SVM: support data breakpoint extension registers

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 04/22/2014 10:04 AM, Jan Beulich wrote:
>>>> On 22.04.14 at 16:04, <boris.ostrovsky@oracle.com> wrote:
>> On 04/16/2014 10:33 AM, Jan Beulich wrote:
>>> @@ -1526,6 +1628,21 @@ static int svm_msr_read_intercept(unsign
>>>            vpmu_do_rdmsr(msr, msr_content);
>>>            break;
>>>    
>>> +    case MSR_AMD64_DR0_ADDRESS_MASK:
>>> +        hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>> +        if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>> +            goto gpf;
>>> +        *msr_content = v->arch.hvm_svm.dr_mask[0];
>>> +        break;
>>> +
>>> +    case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
>>> +        hvm_cpuid(0x80000001, NULL, NULL, &ecx, NULL);
>>> +        if ( !test_bit(X86_FEATURE_DBEXT & 31, &ecx) )
>>> +            goto gpf;
>>> +        *msr_content =
>>> +            v->arch.hvm_svm.dr_mask[msr - MSR_AMD64_DR1_ADDRESS_MASK + 1];
>>> +        break;
>>> +
>> Can you merge these two cases (here and in write_intercept)? They are
>> doing logically the same thing, the only difference is index calculation
>> (and you can have a macro or something similar for that).
> Do you really think this would be better - since the MSR numbers
> aren't contiguous (and since one might want to consider adding
> support for Fam15 supporting just the DR0 extension), I think they
> would better be kept separate.

These MSR are tied to CPUID bit so if they are going to change then it 
will require a new bit anyway.

But it's a nit really so

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



-boris