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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.01.15 at 18:08, <andrew.cooper3@xxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel