[PATCH v2 0/2] x86/HVM: fix various aspects of x2APIC emulation

[PATCH v2 0/2] x86/HVM: fix various aspects of x2APIC emulation

[Jan Beulich]

1: fix miscellaneous aspects of x2APIC emulation
2: fix ID handling of x2APIC emulation

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Monolithic patch broken up. Generate #GP on invalid APIC base
    MSR transitions.Correct ID and LDR after domain restore (if
    necessary); as stated previously the only compatibility problem this
    creates is when migrating a VM _to_ an unfixed (i.e. old) hypervisor,
    a scenario which supposedly isn't supported.

[PATCH v2 1/2] x86/HVM: fix miscellaneous aspects of x2APIC emulation

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4321 bytes --]

- generate #GP on invalid APIC base MSR transitions
- fail reads from the self-IPI register (which is write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
  hvm_x2apic_msr_write()

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split from main patch.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
         break;
 
     case MSR_IA32_APICBASE:
-        vlapic_msr_set(vcpu_vlapic(v), msr_content);
+        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
+            goto gp_fault;
         break;
 
     case MSR_IA32_TSC_DEADLINE:
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -602,6 +602,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
         break;
 
     case APIC_ICR2:
+    case APIC_SELF_IPI:
         return 1;
     }
 
@@ -692,9 +693,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_SELF_IPI:
-        rc = vlapic_x2apic_mode(vlapic)
-            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
-            : X86EMUL_UNHANDLEABLE;
+        rc = X86EMUL_UNHANDLEABLE;
         break;
 
     case APIC_ICR:
@@ -704,9 +703,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_ICR2:
-        if ( !vlapic_x2apic_mode(vlapic) )
-            val &= 0xff000000;
-        vlapic_set_reg(vlapic, APIC_ICR2, val);
+        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
         break;
 
     case APIC_LVTT:         /* LVT Timer Reg */
@@ -865,16 +862,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
 
     switch ( offset )
     {
-        int rc;
-
     case APIC_ICR:
-        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
-        if ( rc )
-            return rc;
+        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
         break;
 
     case APIC_ICR2:
         return X86EMUL_UNHANDLEABLE;
+
+    case APIC_SELF_IPI:
+        offset = APIC_ICR;
+        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
+        break;
     }
 
     return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -893,10 +891,12 @@ const struct hvm_mmio_handler vlapic_mmi
     .write_handler = vlapic_write
 };
 
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
 {
     if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
     {
+        if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
+            return 0;
         if ( value & MSR_IA32_APICBASE_ENABLE )
         {
             vlapic_reset(vlapic);
@@ -905,10 +905,15 @@ void vlapic_msr_set(struct vlapic *vlapi
         }
         else
         {
+            if ( unlikely(vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
+                return 0;
             vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
             pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
         }
     }
+    else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
+              unlikely(value & MSR_IA32_APICBASE_EXTD) )
+        return 0;
 
     vlapic->hw.apic_base_msr = value;
 
@@ -923,6 +928,8 @@ void vlapic_msr_set(struct vlapic *vlapi
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                 "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
+
+    return 1;
 }
 
 uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)
@@ -1206,6 +1213,10 @@ static int lapic_load_hidden(struct doma
     if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
         return -EINVAL;
 
+    if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
+         unlikely(s->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
+        return -EINVAL;
+
     vmx_vlapic_msr_changed(v);
 
     return 0;
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
 
 void vlapic_reset(struct vlapic *vlapic);
 
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
 void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
 uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
 



[-- Attachment #2: x86-HVM-x2APIC-misc.patch --]
[-- Type: text/plain, Size: 4375 bytes --]

x86/HVM: fix miscellaneous aspects of x2APIC emulation

- generate #GP on invalid APIC base MSR transitions
- fail reads from the self-IPI register (which is write-only)
- handle self-IPI writes and the ICR2 half of ICR writes largely in
  hvm_x2apic_msr_write()

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split from main patch.

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
         break;
 
     case MSR_IA32_APICBASE:
-        vlapic_msr_set(vcpu_vlapic(v), msr_content);
+        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
+            goto gp_fault;
         break;
 
     case MSR_IA32_TSC_DEADLINE:
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -602,6 +602,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
         break;
 
     case APIC_ICR2:
+    case APIC_SELF_IPI:
         return 1;
     }
 
@@ -692,9 +693,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_SELF_IPI:
-        rc = vlapic_x2apic_mode(vlapic)
-            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
-            : X86EMUL_UNHANDLEABLE;
+        rc = X86EMUL_UNHANDLEABLE;
         break;
 
     case APIC_ICR:
@@ -704,9 +703,7 @@ static int vlapic_reg_write(struct vcpu 
         break;
 
     case APIC_ICR2:
-        if ( !vlapic_x2apic_mode(vlapic) )
-            val &= 0xff000000;
-        vlapic_set_reg(vlapic, APIC_ICR2, val);
+        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
         break;
 
     case APIC_LVTT:         /* LVT Timer Reg */
@@ -865,16 +862,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
 
     switch ( offset )
     {
-        int rc;
-
     case APIC_ICR:
-        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
-        if ( rc )
-            return rc;
+        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
         break;
 
     case APIC_ICR2:
         return X86EMUL_UNHANDLEABLE;
+
+    case APIC_SELF_IPI:
+        offset = APIC_ICR;
+        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
+        break;
     }
 
     return vlapic_reg_write(v, offset, (uint32_t)msr_content);
@@ -893,10 +891,12 @@ const struct hvm_mmio_handler vlapic_mmi
     .write_handler = vlapic_write
 };
 
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
 {
     if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
     {
+        if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
+            return 0;
         if ( value & MSR_IA32_APICBASE_ENABLE )
         {
             vlapic_reset(vlapic);
@@ -905,10 +905,15 @@ void vlapic_msr_set(struct vlapic *vlapi
         }
         else
         {
+            if ( unlikely(vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
+                return 0;
             vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
             pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
         }
     }
+    else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
+              unlikely(value & MSR_IA32_APICBASE_EXTD) )
+        return 0;
 
     vlapic->hw.apic_base_msr = value;
 
@@ -923,6 +928,8 @@ void vlapic_msr_set(struct vlapic *vlapi
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
                 "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
+
+    return 1;
 }
 
 uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)
@@ -1206,6 +1213,10 @@ static int lapic_load_hidden(struct doma
     if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
         return -EINVAL;
 
+    if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
+         unlikely(s->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
+        return -EINVAL;
+
     vmx_vlapic_msr_changed(v);
 
     return 0;
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
 
 void vlapic_reset(struct vlapic *vlapic);
 
-void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
+bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
 void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
 uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 1/2] x86/HVM: fix miscellaneous aspects of x2APIC emulation

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4957 bytes --]

On 10/09/14 14:43, Jan Beulich wrote:
> - generate #GP on invalid APIC base MSR transitions
> - fail reads from the self-IPI register (which is write-only)
> - handle self-IPI writes and the ICR2 half of ICR writes largely in
>   hvm_x2apic_msr_write()
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Split from main patch.
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4499,7 +4499,8 @@ int hvm_msr_write_intercept(unsigned int
>          break;
>  
>      case MSR_IA32_APICBASE:
> -        vlapic_msr_set(vcpu_vlapic(v), msr_content);
> +        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
> +            goto gp_fault;
>          break;
>  
>      case MSR_IA32_TSC_DEADLINE:
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -602,6 +602,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
>          break;
>  
>      case APIC_ICR2:
> +    case APIC_SELF_IPI:

APIC_EOI is also write-only, generates #GP(0) on on rdmsr, and isn't
caught by vlapic_read_aligned().

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>          return 1;
>      }
>  
> @@ -692,9 +693,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_SELF_IPI:
> -        rc = vlapic_x2apic_mode(vlapic)
> -            ? vlapic_reg_write(v, APIC_ICR, 0x40000 | (val & 0xff))
> -            : X86EMUL_UNHANDLEABLE;
> +        rc = X86EMUL_UNHANDLEABLE;
>          break;
>  
>      case APIC_ICR:
> @@ -704,9 +703,7 @@ static int vlapic_reg_write(struct vcpu 
>          break;
>  
>      case APIC_ICR2:
> -        if ( !vlapic_x2apic_mode(vlapic) )
> -            val &= 0xff000000;
> -        vlapic_set_reg(vlapic, APIC_ICR2, val);
> +        vlapic_set_reg(vlapic, APIC_ICR2, val & 0xff000000);
>          break;
>  
>      case APIC_LVTT:         /* LVT Timer Reg */
> @@ -865,16 +862,17 @@ int hvm_x2apic_msr_write(struct vcpu *v,
>  
>      switch ( offset )
>      {
> -        int rc;
> -
>      case APIC_ICR:
> -        rc = vlapic_reg_write(v, APIC_ICR2, (uint32_t)(msr_content >> 32));
> -        if ( rc )
> -            return rc;
> +        vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>          break;
>  
>      case APIC_ICR2:
>          return X86EMUL_UNHANDLEABLE;
> +
> +    case APIC_SELF_IPI:
> +        offset = APIC_ICR;
> +        msr_content = APIC_DEST_SELF | (uint8_t)msr_content;
> +        break;
>      }
>  
>      return vlapic_reg_write(v, offset, (uint32_t)msr_content);
> @@ -893,10 +891,12 @@ const struct hvm_mmio_handler vlapic_mmi
>      .write_handler = vlapic_write
>  };
>  
> -void vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
> +bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
>  {
>      if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
>      {
> +        if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
> +            return 0;
>          if ( value & MSR_IA32_APICBASE_ENABLE )
>          {
>              vlapic_reset(vlapic);
> @@ -905,10 +905,15 @@ void vlapic_msr_set(struct vlapic *vlapi
>          }
>          else
>          {
> +            if ( unlikely(vlapic->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
> +                return 0;
>              vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
>              pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
>          }
>      }
> +    else if ( !(value & MSR_IA32_APICBASE_ENABLE) &&
> +              unlikely(value & MSR_IA32_APICBASE_EXTD) )
> +        return 0;
>  
>      vlapic->hw.apic_base_msr = value;
>  
> @@ -923,6 +928,8 @@ void vlapic_msr_set(struct vlapic *vlapi
>  
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
>                  "apic base msr is 0x%016"PRIx64, vlapic->hw.apic_base_msr);
> +
> +    return 1;
>  }
>  
>  uint64_t  vlapic_tdt_msr_get(struct vlapic *vlapic)
> @@ -1206,6 +1213,10 @@ static int lapic_load_hidden(struct doma
>      if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
>          return -EINVAL;
>  
> +    if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
> +         unlikely(s->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD) )
> +        return -EINVAL;
> +
>      vmx_vlapic_msr_changed(v);
>  
>      return 0;
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -106,7 +106,7 @@ void vlapic_destroy(struct vcpu *v);
>  
>  void vlapic_reset(struct vlapic *vlapic);
>  
> -void vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
> +bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value);
>  void vlapic_tdt_msr_set(struct vlapic *vlapic, uint64_t value);
>  uint64_t vlapic_tdt_msr_get(struct vlapic *vlapic);
>  
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5579 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 1/2] x86/HVM: fix miscellaneous aspects of x2APIC emulation

[Jan Beulich]

>>> On 11.09.14 at 17:39, <andrew.cooper3@citrix.com> wrote:
> On 10/09/14 14:43, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -602,6 +602,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
>>          break;
>>  
>>      case APIC_ICR2:
>> +    case APIC_SELF_IPI:
> 
> APIC_EOI is also write-only, generates #GP(0) on on rdmsr, and isn't
> caught by vlapic_read_aligned().

Of course - not sure how I managed to overlook that.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, but together with the above I actually realized I should do
some further changes in v3.

Jan

[PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9389 bytes --]

- properly change ID when switching into x2APIC mode (instead of
  mimicking necessary behavior in hvm_x2apic_msr_read())
- correctly (meaningfully) set LDR (so far it ended up being 1 on all
  vCPU-s)
- even if we don't support more than 128 vCPU-s in a HVM guest for now,
  we should properly handle IDs as 32-bit values (i.e. not ignore the
  top 24 bits)
- with that, properly do cluster ID and bit mask check in
  vlapic_match_logical_addr()
- slightly adjust other parameter types of vlapic_match_dest() and
  vlapic_lowest_prio() (and related local variable ones)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Some changes broken out to separate patch. Correct ID and
    LDR after domain restore (if necessary); as stated previously the
    only compatibility problem this creates is when migrating a VM _to_
    an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
    supported.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,18 +173,17 @@ uint32_t vlapic_set_ppr(struct vlapic *v
    return ppr;
 }
 
-static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
+static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
 {
     int result = 0;
-    uint32_t logical_id;
+    uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
 
     if ( vlapic_x2apic_mode(vlapic) )
-    {
-        logical_id = vlapic_get_reg(vlapic, APIC_LDR);
-        return !!(logical_id & mda);
-    }
+        return ((logical_id >> 16) == (mda >> 16)) &&
+               (uint16_t)(logical_id & mda);
 
-    logical_id = GET_xAPIC_LOGICAL_ID(vlapic_get_reg(vlapic, APIC_LDR));
+    logical_id = GET_xAPIC_LOGICAL_ID(logical_id);
+    mda = (uint8_t)mda;
 
     switch ( vlapic_get_reg(vlapic, APIC_DFR) )
     {
@@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
 }
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "target %p, source %p, dest %#x, "
                 "dest_mode %#x, short_hand %#x",
@@ -219,7 +218,8 @@ bool_t vlapic_match_dest(
     case APIC_DEST_NOSHORT:
         if ( dest_mode )
             return vlapic_match_logical_addr(target, dest);
-        return ((dest == 0xFF) || (dest == VLAPIC_ID(target)));
+        return (dest == _VLAPIC_ID(target, 0xffffffff)) ||
+               (dest == VLAPIC_ID(target));
 
     case APIC_DEST_SELF:
         return (target == source);
@@ -286,7 +286,7 @@ static void vlapic_init_sipi_action(unsi
     uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
     uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
     uint32_t short_hand = icr & APIC_SHORT_MASK;
-    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
+    bool_t dest_mode  = !!(icr & APIC_DEST_MASK);
     struct vcpu *v;
 
     if ( icr == 0 )
@@ -352,8 +352,8 @@ static void vlapic_accept_irq(struct vcp
 }
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
     uint32_t ppr, target_ppr = UINT_MAX;
@@ -414,13 +414,11 @@ void vlapic_ipi(
 {
     unsigned int dest;
     unsigned int short_hand = icr_low & APIC_SHORT_MASK;
-    unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
+    bool_t dest_mode = !!(icr_low & APIC_DEST_MASK);
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
 
-    dest = (vlapic_x2apic_mode(vlapic)
-            ? icr_high
-            : GET_xAPIC_DEST_FIELD(icr_high));
+    dest = _VLAPIC_ID(vlapic, icr_high);
 
     switch ( icr_low & APIC_MODE_MASK )
     {
@@ -593,10 +591,6 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
     vlapic_read_aligned(vlapic, offset, &low);
     switch ( offset )
     {
-    case APIC_ID:
-        low = GET_xAPIC_ID(low);
-        break;
-
     case APIC_ICR:
         vlapic_read_aligned(vlapic, APIC_ICR2, &high);
         break;
@@ -891,6 +885,15 @@ const struct hvm_mmio_handler vlapic_mmi
     .write_handler = vlapic_write
 };
 
+static void set_x2apic_id(struct vlapic *vlapic)
+{
+    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
+    u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
+
+    vlapic_set_reg(vlapic, APIC_ID, id * 2);
+    vlapic_set_reg(vlapic, APIC_LDR, ldr);
+}
+
 bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
 {
     if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
@@ -918,11 +921,7 @@ bool_t vlapic_msr_set(struct vlapic *vla
     vlapic->hw.apic_base_msr = value;
 
     if ( vlapic_x2apic_mode(vlapic) )
-    {
-        u32 id = vlapic_get_reg(vlapic, APIC_ID);
-        u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
-        vlapic_set_reg(vlapic, APIC_LDR, ldr);
-    }
+        set_x2apic_id(vlapic);
 
     vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
 
@@ -1209,6 +1208,7 @@ static int lapic_load_hidden(struct doma
         return -EINVAL;
     }
     s = vcpu_vlapic(v);
+    s->loaded = 1;
     
     if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
         return -EINVAL;
@@ -1237,6 +1237,7 @@ static int lapic_load_regs(struct domain
         return -EINVAL;
     }
     s = vcpu_vlapic(v);
+    s->loaded = 1;
     
     if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 ) 
         return -EINVAL;
@@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
 HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
                           1, HVMSR_PER_VCPU);
 
+void vlapic_domain_unpause(const struct domain *d)
+{
+    /*
+     * Following lapic_load_hidden()/lapic_load_regs() we may need to
+     * correct ID and LDR when they come from an old, broken hypervisor.
+     */
+    struct vcpu *v;
+
+    for_each_vcpu ( d, v )
+    {
+        struct vlapic *vlapic = vcpu_vlapic(v);
+        u32 id = vlapic_get_reg(vlapic, APIC_ID);
+
+        if ( vlapic->loaded && vlapic_x2apic_mode(vlapic) &&
+             id && GET_xAPIC_ID(id) == v->vcpu_id * 2 &&
+             id == SET_xAPIC_ID(GET_xAPIC_ID(id)) &&
+             vlapic_get_reg(vlapic, APIC_LDR) == 1 )
+            set_x2apic_id(vlapic);
+
+        vlapic->loaded = 0;
+    }
+}
+
 int vlapic_init(struct vcpu *v)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -952,8 +952,11 @@ void domain_unpause(struct domain *d)
     struct vcpu *v;
 
     if ( atomic_dec_and_test(&d->pause_count) )
+    {
+        arch_domain_unpause(d);
         for_each_vcpu( d, v )
             vcpu_wake(v);
+    }
 }
 
 int __domain_pause_by_systemcontroller(struct domain *d,
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -241,6 +241,8 @@ struct arch_vcpu
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
+#define arch_domain_unpause(d) ((void)(d))
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -500,6 +500,12 @@ void domain_cpuid(struct domain *d,
                   unsigned int  *ecx,
                   unsigned int  *edx);
 
+#define arch_domain_unpause(d) ({  \
+    const struct domain *d_ = (d); \
+    if ( is_hvm_domain(d_) )       \
+        vlapic_domain_unpause(d_); \
+})
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -30,8 +30,9 @@
 #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
 #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
 
-#define VLAPIC_ID(vlapic)   \
-    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
+#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
+                                ? (id) : GET_xAPIC_ID(id))
+#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))
 
 /*
  * APIC can be disabled in two ways:
@@ -70,6 +71,7 @@
 struct vlapic {
     struct hvm_hw_lapic      hw;
     struct hvm_hw_lapic_regs *regs;
+    bool_t                   loaded;
     struct periodic_time     pt;
     s_time_t                 timer_last_update;
     struct page_info         *regs_page;
@@ -123,11 +125,13 @@ void vlapic_ipi(struct vlapic *vlapic, u
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
+
+void vlapic_domain_unpause(const struct domain *);
 
 #endif /* __ASM_X86_HVM_VLAPIC_H__ */



[-- Attachment #2: x86-HVM-x2APIC-id.patch --]
[-- Type: text/plain, Size: 9433 bytes --]

x86/HVM: fix ID handling of x2APIC emulation

- properly change ID when switching into x2APIC mode (instead of
  mimicking necessary behavior in hvm_x2apic_msr_read())
- correctly (meaningfully) set LDR (so far it ended up being 1 on all
  vCPU-s)
- even if we don't support more than 128 vCPU-s in a HVM guest for now,
  we should properly handle IDs as 32-bit values (i.e. not ignore the
  top 24 bits)
- with that, properly do cluster ID and bit mask check in
  vlapic_match_logical_addr()
- slightly adjust other parameter types of vlapic_match_dest() and
  vlapic_lowest_prio() (and related local variable ones)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Some changes broken out to separate patch. Correct ID and
    LDR after domain restore (if necessary); as stated previously the
    only compatibility problem this creates is when migrating a VM _to_
    an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
    supported.

--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -173,18 +173,17 @@ uint32_t vlapic_set_ppr(struct vlapic *v
    return ppr;
 }
 
-static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
+static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
 {
     int result = 0;
-    uint32_t logical_id;
+    uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
 
     if ( vlapic_x2apic_mode(vlapic) )
-    {
-        logical_id = vlapic_get_reg(vlapic, APIC_LDR);
-        return !!(logical_id & mda);
-    }
+        return ((logical_id >> 16) == (mda >> 16)) &&
+               (uint16_t)(logical_id & mda);
 
-    logical_id = GET_xAPIC_LOGICAL_ID(vlapic_get_reg(vlapic, APIC_LDR));
+    logical_id = GET_xAPIC_LOGICAL_ID(logical_id);
+    mda = (uint8_t)mda;
 
     switch ( vlapic_get_reg(vlapic, APIC_DFR) )
     {
@@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
 }
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "target %p, source %p, dest %#x, "
                 "dest_mode %#x, short_hand %#x",
@@ -219,7 +218,8 @@ bool_t vlapic_match_dest(
     case APIC_DEST_NOSHORT:
         if ( dest_mode )
             return vlapic_match_logical_addr(target, dest);
-        return ((dest == 0xFF) || (dest == VLAPIC_ID(target)));
+        return (dest == _VLAPIC_ID(target, 0xffffffff)) ||
+               (dest == VLAPIC_ID(target));
 
     case APIC_DEST_SELF:
         return (target == source);
@@ -286,7 +286,7 @@ static void vlapic_init_sipi_action(unsi
     uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
     uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
     uint32_t short_hand = icr & APIC_SHORT_MASK;
-    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
+    bool_t dest_mode  = !!(icr & APIC_DEST_MASK);
     struct vcpu *v;
 
     if ( icr == 0 )
@@ -352,8 +352,8 @@ static void vlapic_accept_irq(struct vcp
 }
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode)
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode)
 {
     int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
     uint32_t ppr, target_ppr = UINT_MAX;
@@ -414,13 +414,11 @@ void vlapic_ipi(
 {
     unsigned int dest;
     unsigned int short_hand = icr_low & APIC_SHORT_MASK;
-    unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
+    bool_t dest_mode = !!(icr_low & APIC_DEST_MASK);
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
 
-    dest = (vlapic_x2apic_mode(vlapic)
-            ? icr_high
-            : GET_xAPIC_DEST_FIELD(icr_high));
+    dest = _VLAPIC_ID(vlapic, icr_high);
 
     switch ( icr_low & APIC_MODE_MASK )
     {
@@ -593,10 +591,6 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
     vlapic_read_aligned(vlapic, offset, &low);
     switch ( offset )
     {
-    case APIC_ID:
-        low = GET_xAPIC_ID(low);
-        break;
-
     case APIC_ICR:
         vlapic_read_aligned(vlapic, APIC_ICR2, &high);
         break;
@@ -891,6 +885,15 @@ const struct hvm_mmio_handler vlapic_mmi
     .write_handler = vlapic_write
 };
 
+static void set_x2apic_id(struct vlapic *vlapic)
+{
+    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
+    u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
+
+    vlapic_set_reg(vlapic, APIC_ID, id * 2);
+    vlapic_set_reg(vlapic, APIC_LDR, ldr);
+}
+
 bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
 {
     if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
@@ -918,11 +921,7 @@ bool_t vlapic_msr_set(struct vlapic *vla
     vlapic->hw.apic_base_msr = value;
 
     if ( vlapic_x2apic_mode(vlapic) )
-    {
-        u32 id = vlapic_get_reg(vlapic, APIC_ID);
-        u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
-        vlapic_set_reg(vlapic, APIC_LDR, ldr);
-    }
+        set_x2apic_id(vlapic);
 
     vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
 
@@ -1209,6 +1208,7 @@ static int lapic_load_hidden(struct doma
         return -EINVAL;
     }
     s = vcpu_vlapic(v);
+    s->loaded = 1;
     
     if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
         return -EINVAL;
@@ -1237,6 +1237,7 @@ static int lapic_load_regs(struct domain
         return -EINVAL;
     }
     s = vcpu_vlapic(v);
+    s->loaded = 1;
     
     if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 ) 
         return -EINVAL;
@@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
 HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
                           1, HVMSR_PER_VCPU);
 
+void vlapic_domain_unpause(const struct domain *d)
+{
+    /*
+     * Following lapic_load_hidden()/lapic_load_regs() we may need to
+     * correct ID and LDR when they come from an old, broken hypervisor.
+     */
+    struct vcpu *v;
+
+    for_each_vcpu ( d, v )
+    {
+        struct vlapic *vlapic = vcpu_vlapic(v);
+        u32 id = vlapic_get_reg(vlapic, APIC_ID);
+
+        if ( vlapic->loaded && vlapic_x2apic_mode(vlapic) &&
+             id && GET_xAPIC_ID(id) == v->vcpu_id * 2 &&
+             id == SET_xAPIC_ID(GET_xAPIC_ID(id)) &&
+             vlapic_get_reg(vlapic, APIC_LDR) == 1 )
+            set_x2apic_id(vlapic);
+
+        vlapic->loaded = 0;
+    }
+}
+
 int vlapic_init(struct vcpu *v)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -952,8 +952,11 @@ void domain_unpause(struct domain *d)
     struct vcpu *v;
 
     if ( atomic_dec_and_test(&d->pause_count) )
+    {
+        arch_domain_unpause(d);
         for_each_vcpu( d, v )
             vcpu_wake(v);
+    }
 }
 
 int __domain_pause_by_systemcontroller(struct domain *d,
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -241,6 +241,8 @@ struct arch_vcpu
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
+#define arch_domain_unpause(d) ((void)(d))
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -500,6 +500,12 @@ void domain_cpuid(struct domain *d,
                   unsigned int  *ecx,
                   unsigned int  *edx);
 
+#define arch_domain_unpause(d) ({  \
+    const struct domain *d_ = (d); \
+    if ( is_hvm_domain(d_) )       \
+        vlapic_domain_unpause(d_); \
+})
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -30,8 +30,9 @@
 #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
 #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
 
-#define VLAPIC_ID(vlapic)   \
-    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
+#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
+                                ? (id) : GET_xAPIC_ID(id))
+#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))
 
 /*
  * APIC can be disabled in two ways:
@@ -70,6 +71,7 @@
 struct vlapic {
     struct hvm_hw_lapic      hw;
     struct hvm_hw_lapic_regs *regs;
+    bool_t                   loaded;
     struct periodic_time     pt;
     s_time_t                 timer_last_update;
     struct page_info         *regs_page;
@@ -123,11 +125,13 @@ void vlapic_ipi(struct vlapic *vlapic, u
 int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
 
 struct vlapic *vlapic_lowest_prio(
-    struct domain *d, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct domain *d, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
 
 bool_t vlapic_match_dest(
-    struct vlapic *target, struct vlapic *source,
-    int short_hand, uint8_t dest, uint8_t dest_mode);
+    struct vlapic *target, const struct vlapic *source,
+    int short_hand, uint32_t dest, bool_t dest_mode);
+
+void vlapic_domain_unpause(const struct domain *);
 
 #endif /* __ASM_X86_HVM_VLAPIC_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 10920 bytes --]

On 10/09/14 14:44, Jan Beulich wrote:
> - properly change ID when switching into x2APIC mode (instead of
>   mimicking necessary behavior in hvm_x2apic_msr_read())
> - correctly (meaningfully) set LDR (so far it ended up being 1 on all
>   vCPU-s)
> - even if we don't support more than 128 vCPU-s in a HVM guest for now,
>   we should properly handle IDs as 32-bit values (i.e. not ignore the
>   top 24 bits)
> - with that, properly do cluster ID and bit mask check in
>   vlapic_match_logical_addr()
> - slightly adjust other parameter types of vlapic_match_dest() and
>   vlapic_lowest_prio() (and related local variable ones)

I think the addition of arch_domain_unpause() at least needs mentioning
in the commit message, although...

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Some changes broken out to separate patch. Correct ID and
>     LDR after domain restore (if necessary); as stated previously the
>     only compatibility problem this creates is when migrating a VM _to_
>     an unfixed (i.e. old) hypervisor, a scenario which supposedly isn't
>     supported.
>
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -173,18 +173,17 @@ uint32_t vlapic_set_ppr(struct vlapic *v
>     return ppr;
>  }
>  
> -static int vlapic_match_logical_addr(struct vlapic *vlapic, uint8_t mda)
> +static int vlapic_match_logical_addr(struct vlapic *vlapic, uint32_t mda)
>  {
>      int result = 0;
> -    uint32_t logical_id;
> +    uint32_t logical_id = vlapic_get_reg(vlapic, APIC_LDR);
>  
>      if ( vlapic_x2apic_mode(vlapic) )
> -    {
> -        logical_id = vlapic_get_reg(vlapic, APIC_LDR);
> -        return !!(logical_id & mda);
> -    }
> +        return ((logical_id >> 16) == (mda >> 16)) &&
> +               (uint16_t)(logical_id & mda);
>  
> -    logical_id = GET_xAPIC_LOGICAL_ID(vlapic_get_reg(vlapic, APIC_LDR));
> +    logical_id = GET_xAPIC_LOGICAL_ID(logical_id);
> +    mda = (uint8_t)mda;
>  
>      switch ( vlapic_get_reg(vlapic, APIC_DFR) )
>      {
> @@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
>  }
>  
>  bool_t vlapic_match_dest(
> -    struct vlapic *target, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode)
> +    struct vlapic *target, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode)

target should be const as well, and looks as if it can be by pushing
const-ness down into vlapic_match_logical_addr() and vlapic_get_reg().

>  {
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "target %p, source %p, dest %#x, "
>                  "dest_mode %#x, short_hand %#x",
> @@ -219,7 +218,8 @@ bool_t vlapic_match_dest(
>      case APIC_DEST_NOSHORT:
>          if ( dest_mode )
>              return vlapic_match_logical_addr(target, dest);
> -        return ((dest == 0xFF) || (dest == VLAPIC_ID(target)));
> +        return (dest == _VLAPIC_ID(target, 0xffffffff)) ||
> +               (dest == VLAPIC_ID(target));
>  
>      case APIC_DEST_SELF:
>          return (target == source);
> @@ -286,7 +286,7 @@ static void vlapic_init_sipi_action(unsi
>      uint32_t icr = vcpu_vlapic(origin)->init_sipi.icr;
>      uint32_t dest = vcpu_vlapic(origin)->init_sipi.dest;
>      uint32_t short_hand = icr & APIC_SHORT_MASK;
> -    uint32_t dest_mode  = !!(icr & APIC_DEST_MASK);
> +    bool_t dest_mode  = !!(icr & APIC_DEST_MASK);
>      struct vcpu *v;
>  
>      if ( icr == 0 )
> @@ -352,8 +352,8 @@ static void vlapic_accept_irq(struct vcp
>  }
>  
>  struct vlapic *vlapic_lowest_prio(
> -    struct domain *d, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode)
> +    struct domain *d, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode)
>  {
>      int old = d->arch.hvm_domain.irq.round_robin_prev_vcpu;
>      uint32_t ppr, target_ppr = UINT_MAX;
> @@ -414,13 +414,11 @@ void vlapic_ipi(
>  {
>      unsigned int dest;
>      unsigned int short_hand = icr_low & APIC_SHORT_MASK;
> -    unsigned int dest_mode  = !!(icr_low & APIC_DEST_MASK);
> +    bool_t dest_mode = !!(icr_low & APIC_DEST_MASK);
>  
>      HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "icr = 0x%08x:%08x", icr_high, icr_low);
>  
> -    dest = (vlapic_x2apic_mode(vlapic)
> -            ? icr_high
> -            : GET_xAPIC_DEST_FIELD(icr_high));
> +    dest = _VLAPIC_ID(vlapic, icr_high);
>  
>      switch ( icr_low & APIC_MODE_MASK )
>      {
> @@ -593,10 +591,6 @@ int hvm_x2apic_msr_read(struct vcpu *v, 
>      vlapic_read_aligned(vlapic, offset, &low);
>      switch ( offset )
>      {
> -    case APIC_ID:
> -        low = GET_xAPIC_ID(low);
> -        break;
> -
>      case APIC_ICR:
>          vlapic_read_aligned(vlapic, APIC_ICR2, &high);
>          break;
> @@ -891,6 +885,15 @@ const struct hvm_mmio_handler vlapic_mmi
>      .write_handler = vlapic_write
>  };
>  
> +static void set_x2apic_id(struct vlapic *vlapic)
> +{
> +    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
> +    u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
> +
> +    vlapic_set_reg(vlapic, APIC_ID, id * 2);

I know this mimics the existing behaviour, but I should point out that
it is an Intel-ism (which assumes no hyperthreads) which is wrong on AMD
systems, and confuses algorithms which following the BIOS/Systems
guides.  I do plan to fix it as part of my cpuid/feature levelling fixes.

> +    vlapic_set_reg(vlapic, APIC_LDR, ldr);
> +}
> +
>  bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
>  {
>      if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
> @@ -918,11 +921,7 @@ bool_t vlapic_msr_set(struct vlapic *vla
>      vlapic->hw.apic_base_msr = value;
>  
>      if ( vlapic_x2apic_mode(vlapic) )
> -    {
> -        u32 id = vlapic_get_reg(vlapic, APIC_ID);
> -        u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
> -        vlapic_set_reg(vlapic, APIC_LDR, ldr);
> -    }
> +        set_x2apic_id(vlapic);
>  
>      vmx_vlapic_msr_changed(vlapic_vcpu(vlapic));
>  
> @@ -1209,6 +1208,7 @@ static int lapic_load_hidden(struct doma
>          return -EINVAL;
>      }
>      s = vcpu_vlapic(v);
> +    s->loaded = 1;
>      
>      if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) 
>          return -EINVAL;
> @@ -1237,6 +1237,7 @@ static int lapic_load_regs(struct domain
>          return -EINVAL;
>      }
>      s = vcpu_vlapic(v);
> +    s->loaded = 1;
>      
>      if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 ) 
>          return -EINVAL;
> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
>                            1, HVMSR_PER_VCPU);
>  
> +void vlapic_domain_unpause(const struct domain *d)
> +{
> +    /*
> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
> +     * correct ID and LDR when they come from an old, broken hypervisor.
> +     */

This seems like aweful overhead for the domain_{,un}pause() path.  Why
can't it be fixed up once in lapic_load_{regs,hidden}(), or possibly
deferred to the end of hvm_load()?

> +    struct vcpu *v;
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        struct vlapic *vlapic = vcpu_vlapic(v);
> +        u32 id = vlapic_get_reg(vlapic, APIC_ID);
> +
> +        if ( vlapic->loaded && vlapic_x2apic_mode(vlapic) &&
> +             id && GET_xAPIC_ID(id) == v->vcpu_id * 2 &&
> +             id == SET_xAPIC_ID(GET_xAPIC_ID(id)) &&
> +             vlapic_get_reg(vlapic, APIC_LDR) == 1 )
> +            set_x2apic_id(vlapic);
> +
> +        vlapic->loaded = 0;
> +    }
> +}
> +
>  int vlapic_init(struct vcpu *v)
>  {
>      struct vlapic *vlapic = vcpu_vlapic(v);
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -952,8 +952,11 @@ void domain_unpause(struct domain *d)
>      struct vcpu *v;
>  
>      if ( atomic_dec_and_test(&d->pause_count) )
> +    {
> +        arch_domain_unpause(d);
>          for_each_vcpu( d, v )
>              vcpu_wake(v);
> +    }
>  }
>  
>  int __domain_pause_by_systemcontroller(struct domain *d,
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -241,6 +241,8 @@ struct arch_vcpu
>  void vcpu_show_execution_state(struct vcpu *);
>  void vcpu_show_registers(const struct vcpu *);
>  
> +#define arch_domain_unpause(d) ((void)(d))
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -500,6 +500,12 @@ void domain_cpuid(struct domain *d,
>                    unsigned int  *ecx,
>                    unsigned int  *edx);
>  
> +#define arch_domain_unpause(d) ({  \
> +    const struct domain *d_ = (d); \
> +    if ( is_hvm_domain(d_) )       \
> +        vlapic_domain_unpause(d_); \
> +})
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> --- a/xen/include/asm-x86/hvm/vlapic.h
> +++ b/xen/include/asm-x86/hvm/vlapic.h
> @@ -30,8 +30,9 @@
>  #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
>  #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
>  
> -#define VLAPIC_ID(vlapic)   \
> -    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
> +#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
> +                                ? (id) : GET_xAPIC_ID(id))
> +#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))

Some comment regarding the difference between these two?

~Andrew

>  
>  /*
>   * APIC can be disabled in two ways:
> @@ -70,6 +71,7 @@
>  struct vlapic {
>      struct hvm_hw_lapic      hw;
>      struct hvm_hw_lapic_regs *regs;
> +    bool_t                   loaded;
>      struct periodic_time     pt;
>      s_time_t                 timer_last_update;
>      struct page_info         *regs_page;
> @@ -123,11 +125,13 @@ void vlapic_ipi(struct vlapic *vlapic, u
>  int vlapic_apicv_write(struct vcpu *v, unsigned int offset);
>  
>  struct vlapic *vlapic_lowest_prio(
> -    struct domain *d, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode);
> +    struct domain *d, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode);
>  
>  bool_t vlapic_match_dest(
> -    struct vlapic *target, struct vlapic *source,
> -    int short_hand, uint8_t dest, uint8_t dest_mode);
> +    struct vlapic *target, const struct vlapic *source,
> +    int short_hand, uint32_t dest, bool_t dest_mode);
> +
> +void vlapic_domain_unpause(const struct domain *);
>  
>  #endif /* __ASM_X86_HVM_VLAPIC_H__ */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 11821 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Jan Beulich]

>>> On 11.09.14 at 18:28, <andrew.cooper3@citrix.com> wrote:
> On 10/09/14 14:44, Jan Beulich wrote:
>> - properly change ID when switching into x2APIC mode (instead of
>>   mimicking necessary behavior in hvm_x2apic_msr_read())
>> - correctly (meaningfully) set LDR (so far it ended up being 1 on all
>>   vCPU-s)
>> - even if we don't support more than 128 vCPU-s in a HVM guest for now,
>>   we should properly handle IDs as 32-bit values (i.e. not ignore the
>>   top 24 bits)
>> - with that, properly do cluster ID and bit mask check in
>>   vlapic_match_logical_addr()
>> - slightly adjust other parameter types of vlapic_match_dest() and
>>   vlapic_lowest_prio() (and related local variable ones)
> 
> I think the addition of arch_domain_unpause() at least needs mentioning
> in the commit message, although...

Hmm, not sure - I tried to describe _what_ gets fixed/changed
here rather than _how_.

>> @@ -207,8 +206,8 @@ static int vlapic_match_logical_addr(str
>>  }
>>  
>>  bool_t vlapic_match_dest(
>> -    struct vlapic *target, struct vlapic *source,
>> -    int short_hand, uint8_t dest, uint8_t dest_mode)
>> +    struct vlapic *target, const struct vlapic *source,
>> +    int short_hand, uint32_t dest, bool_t dest_mode)
> 
> target should be const as well, and looks as if it can be by pushing
> const-ness down into vlapic_match_logical_addr() and vlapic_get_reg().

I specifically didn't want to mess with unrelated functions here. But
yes, this could be further follow-up cleanup.

>> @@ -891,6 +885,15 @@ const struct hvm_mmio_handler vlapic_mmi
>>      .write_handler = vlapic_write
>>  };
>>  
>> +static void set_x2apic_id(struct vlapic *vlapic)
>> +{
>> +    u32 id = vlapic_vcpu(vlapic)->vcpu_id;
>> +    u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf));
>> +
>> +    vlapic_set_reg(vlapic, APIC_ID, id * 2);
> 
> I know this mimics the existing behaviour, but I should point out that
> it is an Intel-ism (which assumes no hyperthreads) which is wrong on AMD
> systems, and confuses algorithms which following the BIOS/Systems
> guides.  I do plan to fix it as part of my cpuid/feature levelling fixes.

Yeah, this certainly should be dealt with better, but again not
something to change in this patch (I'm relatively certain there
are assumptions based on this elsewhere in the code base).

>> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
>>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
>>                            1, HVMSR_PER_VCPU);
>>  
>> +void vlapic_domain_unpause(const struct domain *d)
>> +{
>> +    /*
>> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
>> +     * correct ID and LDR when they come from an old, broken hypervisor.
>> +     */
> 
> This seems like aweful overhead for the domain_{,un}pause() path.

It's the best I could think of (and domain un-pausing shouldn't be that
frequent an operation I would hope), since ...

>  Why can't it be fixed up once in lapic_load_{regs,hidden}(),

... both of these would have to have got carried out and ...

> or possibly deferred to the end of hvm_load()?

... there's apparently no requirement for all state to be loaded in
one go.

>> --- a/xen/include/asm-x86/hvm/vlapic.h
>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>> @@ -30,8 +30,9 @@
>>  #define vlapic_vcpu(x)   (container_of((x), struct vcpu, arch.hvm_vcpu.vlapic))
>>  #define vlapic_domain(x) (vlapic_vcpu(x)->domain)
>>  
>> -#define VLAPIC_ID(vlapic)   \
>> -    (GET_xAPIC_ID(vlapic_get_reg((vlapic), APIC_ID)))
>> +#define _VLAPIC_ID(vlapic, id) (vlapic_x2apic_mode(vlapic) \
>> +                                ? (id) : GET_xAPIC_ID(id))
>> +#define VLAPIC_ID(vlapic) _VLAPIC_ID(vlapic, vlapic_get_reg(vlapic, APIC_ID))
> 
> Some comment regarding the difference between these two?

Not sure what would need explaining here - one operates on a passed
in ID while the other uses the active one.

Jan

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Tim Deegan]

At 08:57 +0100 on 12 Sep (1410508628), Jan Beulich wrote:
> >> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
> >>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
> >>                            1, HVMSR_PER_VCPU);
> >>  
> >> +void vlapic_domain_unpause(const struct domain *d)
> >> +{
> >> +    /*
> >> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
> >> +     * correct ID and LDR when they come from an old, broken hypervisor.
> >> +     */
> > 
> > This seems like aweful overhead for the domain_{,un}pause() path.
> 
> It's the best I could think of (and domain un-pausing shouldn't be that
> frequent an operation I would hope), since ...
> 
> >  Why can't it be fixed up once in lapic_load_{regs,hidden}(),
> 
> ... both of these would have to have got carried out and ...

You're already tracking whether either of them happens so you could
track whether both have happened and do the fixup then.  Loading a VM
with only the hidden state and not the register state is not something
we care about (and in that case, by construction, LDR won't be 1).

Tim.

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Jan Beulich]

>>> On 18.09.14 at 12:53, <tim@xen.org> wrote:
> At 08:57 +0100 on 12 Sep (1410508628), Jan Beulich wrote:
>> >> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
>> >>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
>> >>                            1, HVMSR_PER_VCPU);
>> >>  
>> >> +void vlapic_domain_unpause(const struct domain *d)
>> >> +{
>> >> +    /*
>> >> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
>> >> +     * correct ID and LDR when they come from an old, broken hypervisor.
>> >> +     */
>> > 
>> > This seems like aweful overhead for the domain_{,un}pause() path.
>> 
>> It's the best I could think of (and domain un-pausing shouldn't be that
>> frequent an operation I would hope), since ...
>> 
>> >  Why can't it be fixed up once in lapic_load_{regs,hidden}(),
>> 
>> ... both of these would have to have got carried out and ...
> 
> You're already tracking whether either of them happens so you could
> track whether both have happened and do the fixup then.  Loading a VM
> with only the hidden state and not the register state is not something
> we care about (and in that case, by construction, LDR won't be 1).

I had considered that, but dropped the idea because the two pieces
are independent, and hence I don't think we should (implicitly) disallow
it. Nor do I think we should (implicitly) disallow restoring the same
state more than once before resuming the guest (or vCPU). I.e. if I
was to go that route, we should then first formally (even if just
verbally) tighten the requirements on these context load operations.

Jan

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Tim Deegan]

At 13:20 +0100 on 18 Sep (1411042805), Jan Beulich wrote:
> >>> On 18.09.14 at 12:53, <tim@xen.org> wrote:
> > At 08:57 +0100 on 12 Sep (1410508628), Jan Beulich wrote:
> >> >> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
> >> >>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
> >> >>                            1, HVMSR_PER_VCPU);
> >> >>  
> >> >> +void vlapic_domain_unpause(const struct domain *d)
> >> >> +{
> >> >> +    /*
> >> >> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
> >> >> +     * correct ID and LDR when they come from an old, broken hypervisor.
> >> >> +     */
> >> > 
> >> > This seems like aweful overhead for the domain_{,un}pause() path.
> >> 
> >> It's the best I could think of (and domain un-pausing shouldn't be that
> >> frequent an operation I would hope), since ...
> >> 
> >> >  Why can't it be fixed up once in lapic_load_{regs,hidden}(),
> >> 
> >> ... both of these would have to have got carried out and ...
> > 
> > You're already tracking whether either of them happens so you could
> > track whether both have happened and do the fixup then.  Loading a VM
> > with only the hidden state and not the register state is not something
> > we care about (and in that case, by construction, LDR won't be 1).
> 
> I had considered that, but dropped the idea because the two pieces
> are independent, and hence I don't think we should (implicitly) disallow
> it.

We wouldn't be disallowing it; we'd just fail to magically fix vlapic
state for that one case.  And since the fix assumes that it's dealing
with a save record from an older Xen I think that's OK.  After all,
the fix also implicitly disallows deliberately setting up a vcpu that
looks like an old-fashioned HVM vcpu.

It would, I think, be reasonable to abandon the fix altogether and
just let upgraded VMs continue with their bogus state -- after all it
can't be that much of a problem for them or they'd have failed on the
old box.

> Nor do I think we should (implicitly) disallow restoring the same
> state more than once before resuming the guest (or vCPU).

True, but that's true whether we do the fix at unpause or beforehand.

> I.e. if I
> was to go that route, we should then first formally (even if just
> verbally) tighten the requirements on these context load operations.

I have no objection to that; I think a comment in save.h would
suffice.

Tim.

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Jan Beulich]

>>> On 18.09.14 at 14:59, <tim@xen.org> wrote:
> At 13:20 +0100 on 18 Sep (1411042805), Jan Beulich wrote:
>> >>> On 18.09.14 at 12:53, <tim@xen.org> wrote:
>> > At 08:57 +0100 on 12 Sep (1410508628), Jan Beulich wrote:
>> >> >> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
>> >> >>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
>> >> >>                            1, HVMSR_PER_VCPU);
>> >> >>  
>> >> >> +void vlapic_domain_unpause(const struct domain *d)
>> >> >> +{
>> >> >> +    /*
>> >> >> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
>> >> >> +     * correct ID and LDR when they come from an old, broken hypervisor.
>> >> >> +     */
>> >> > 
>> >> > This seems like aweful overhead for the domain_{,un}pause() path.
>> >> 
>> >> It's the best I could think of (and domain un-pausing shouldn't be that
>> >> frequent an operation I would hope), since ...
>> >> 
>> >> >  Why can't it be fixed up once in lapic_load_{regs,hidden}(),
>> >> 
>> >> ... both of these would have to have got carried out and ...
>> > 
>> > You're already tracking whether either of them happens so you could
>> > track whether both have happened and do the fixup then.  Loading a VM
>> > with only the hidden state and not the register state is not something
>> > we care about (and in that case, by construction, LDR won't be 1).
>> 
>> I had considered that, but dropped the idea because the two pieces
>> are independent, and hence I don't think we should (implicitly) disallow
>> it.
> 
> We wouldn't be disallowing it; we'd just fail to magically fix vlapic
> state for that one case.  And since the fix assumes that it's dealing
> with a save record from an older Xen I think that's OK.  After all,
> the fix also implicitly disallows deliberately setting up a vcpu that
> looks like an old-fashioned HVM vcpu.
> 
> It would, I think, be reasonable to abandon the fix altogether and
> just let upgraded VMs continue with their bogus state -- after all it
> can't be that much of a problem for them or they'd have failed on the
> old box.

No, we can't drop it, because it doesn't fix only LDR. It also changes
the ID representation (since in particular the GET_xAPIC_ID() use
gets dropped from hvm_x2apic_msr_read()'s APIC_ID case), and this
one must be transparent to the guest.

>> Nor do I think we should (implicitly) disallow restoring the same
>> state more than once before resuming the guest (or vCPU).
> 
> True, but that's true whether we do the fix at unpause or beforehand.

No, it's not: If the first load resulted in x2APIC mode and the
second one not, we'd have done an adjustment already which we
would better not have done.

But I think I can see another alternative: Latch the most recently
loaded or live ID and LDR values into struct vlapic and restore
them into register state when hidden state got updated, applying
the fix immediately afterwards. Any write to those two registers
(or perhaps just any live register access) would then clear the
"loaded" state.

Jan

Re: [PATCH v2 2/2] x86/HVM: fix ID handling of x2APIC emulation

[Tim Deegan]

At 14:22 +0100 on 18 Sep (1411046578), Jan Beulich wrote:
> > It would, I think, be reasonable to abandon the fix altogether and
> > just let upgraded VMs continue with their bogus state -- after all it
> > can't be that much of a problem for them or they'd have failed on the
> > old box.
> 
> No, we can't drop it, because it doesn't fix only LDR. It also changes
> the ID representation (since in particular the GET_xAPIC_ID() use
> gets dropped from hvm_x2apic_msr_read()'s APIC_ID case), and this
> one must be transparent to the guest.

Ah, right.

> >> Nor do I think we should (implicitly) disallow restoring the same
> >> state more than once before resuming the guest (or vCPU).
> > 
> > True, but that's true whether we do the fix at unpause or beforehand.
> 
> No, it's not: If the first load resulted in x2APIC mode and the
> second one not, we'd have done an adjustment already which we
> would better not have done.

What I mean is: we also ought to handle multiple restores even with
unpauses between them (like COLO, though IIRC that won't actually
unpause), or else explicitly disallow them.

> But I think I can see another alternative: Latch the most recently
> loaded or live ID and LDR values into struct vlapic and restore
> them into register state when hidden state got updated, applying
> the fix immediately afterwards. Any write to those two registers
> (or perhaps just any live register access) would then clear the
> "loaded" state.

Yes, I think that makes sense.

Tim.