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

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

[Andrew Cooper]

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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---

This has been tested using my XSA-108 testcase, which verifies that exactly
the same set of MSRs are readable and the same set cause #GP faults.

bloat-o-meter (from a Linux tree) shows:

add/remove: 1/0 grow/shrink: 0/1 up/down: 8/-262 (-254)
function                                     old     new   delta
readable                                       -       8      +8
hvm_x2apic_msr_read                          409     147    -262
---
 xen/arch/x86/hvm/vlapic.c |   50 +++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 72b6509..29bd4b5 100644
--- 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))
+            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);
 
-    if ( !vlapic_x2apic_mode(vlapic) )
+    if ( !vlapic_x2apic_mode(vlapic) ||
+         (reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
         return X86EMUL_UNHANDLEABLE;
 
-    switch ( offset )
-    {
-    case APIC_ICR:
+    if ( offset == APIC_ICR )
         vlapic_read_aligned(vlapic, APIC_ICR2, &high);
-        /* Fallthrough. */
-    case APIC_ID:
-    case APIC_LVR:
-    case APIC_TASKPRI:
-    case APIC_PROCPRI:
-    case APIC_LDR:
-    case APIC_SPIV:
-    case APIC_ISR ... APIC_ISR + 0x70:
-    case APIC_TMR ... APIC_TMR + 0x70:
-    case APIC_IRR ... APIC_IRR + 0x70:
-    case APIC_ESR:
-    case APIC_CMCI:
-    case APIC_LVTT:
-    case APIC_LVTTHMR:
-    case APIC_LVTPC:
-    case APIC_LVT0:
-    case APIC_LVT1:
-    case APIC_LVTERR:
-    case APIC_TMICT:
-    case APIC_TMCCT:
-    case APIC_TDCR:
-        vlapic_read_aligned(vlapic, offset, &low);
-        break;
 
-    default:
-        return X86EMUL_UNHANDLEABLE;
-    }
+    vlapic_read_aligned(vlapic, offset, &low);
 
     *msr_content = (((uint64_t)high) << 32) | low;
 
-- 
1.7.10.4

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

[Jan Beulich]

>>> 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.

Jan

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

[Andrew Cooper]

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