Re: [PATCH] hvm/vlapic: Express x2apic msr readability with a bitmap

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/01/15 09:39, Jan Beulich wrote:
>>>> On 21.01.15 at 18:08, <andrew.cooper3@citrix.com> wrote:
>> The x2apic MSR space is currently defined between 0x800 and 0x83f, which
>> conveniently fits in a 64 bit wide bitmap.  This is far more efficient than
>> the cascade comparisons generated by the switch statement, which can't be
>> optimised because of the case ranges used for the ISR, TMR and IRR blocks.
> Nice idea.
>
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -643,43 +643,31 @@ static int vlapic_read(
>>  
>>  int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
>>  {
>> +    static const unsigned long readable[] =
>> +        {
>> +#define REG(x) (1UL << ((APIC_ ## x) >> 4))
>> +            REG(ID)    | REG(LVR)  | REG(TASKPRI) | REG(PROCPRI) |
>> +            REG(LDR)   | REG(SPIV) | REG(ESR)     | REG(ICR)     |
>> +            REG(CMCI)  | REG(LVTT) | REG(LVTTHMR) | REG(LVTPC)   |
>> +            REG(LVT0)  | REG(LVT1) | REG(LVTERR)  | REG(TMICT)   |
>> +            REG(TMCCT) | REG(TDCR) |
>> +#undef REG
>> +#define REGBLOCK(x) (0xffUL << ((APIC_ ## x) >> 4))
> I'd much prefer the 0xffUL to be replaced by a proper expression,
> e.g. ((1UL << (NR_VECTORS / 32)) - 1). Also parenthesizing
> the operands of ## is pretty strange.
>
>> +            REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
>> +#undef REGBLOCK
>> +        };
>>      struct vlapic *vlapic = vcpu_vlapic(v);
>> -    uint32_t low, high = 0, offset = (msr - MSR_IA32_APICBASE_MSR) << 4;
>> +    uint32_t low, high = 0, reg = (msr - MSR_IA32_APICBASE_MSR),
>> +        offset = (reg << 4);
> Nor can I see a need for the parentheses here.
>
> If you agree, I could fix up all of these while committing.

Certainly.

~Andrew