[PATCH v3] x86/vlapic: Don't reset APIC ID when handling INIT signal

* [PATCH v3] x86/vlapic: Don't reset APIC ID when handling INIT signal
@ 2017-04-19 20:22 Chao Gao
  2017-04-20 13:34 ` Jan Beulich
  0 siblings, 1 reply; 9+ messages in thread
From: Chao Gao @ 2017-04-19 20:22 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Julien Grall, Jan Beulich, Chao Gao

According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
"EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
and APIC ID are preserved when handling INIT signal and a reset places
APIC to xAPIC mode and APIC base address to 0xFEE00000h (this part
is in "Local APIC" -> "Local APIC Status and Location"). So there are
two problems in current code:
1. Using reset logic (aka vlapic_reset) to handle INIT signal.
2. Forgetting resetting APIC mode and base address in vlapic_reset()

This patch introduces a new function vlapic_do_init() and replaces the
wrongly used vlapic_reset(). Also reset APIC mode and APIC base address
in vlapic_reset().

Note that: LDR is read only in x2APIC mode. Resetting it to zero in x2APIC
mode is unreasonable. This patch also doesn't reset LDR when handling INIT
signal in x2APIC mode.

Signed-off-by: Chao Gao <chao.gao@intel.com>

---
I regard this patch as a bug fix. But I haven't seen issues caused by
this bug and am not sure of the existance of such issues. Anyhow Cc
Julien and leave the decision to you (Julien and Jan).

---
v3:
- Reset APIC base address in vlapic_reset() and accordingly change the
commit message
- Reset APIC mode before other APIC registers.

v2:
- rename vlapic_INIT to vlapic_do_init
- reset APIC to xAPIC mode in vlapic_reset()
- constify the struct vcpu in vlapic_reset()
- subject, commit message, comment changes   
- properly handle LDR in x2apic mode

---
 xen/arch/x86/hvm/vlapic.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 2653ba8..4ac9f46 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -83,6 +83,8 @@ static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
     ((vlapic_get_reg(vlapic, APIC_LVTT) & APIC_TIMER_MODE_MASK) \
      == APIC_TIMER_MODE_TSC_DEADLINE)
 
+static void vlapic_do_init(struct vlapic *vlapic);
+
 static int vlapic_find_highest_vector(const void *bitmap)
 {
     const uint32_t *word = bitmap;
@@ -281,7 +283,7 @@ static void vlapic_init_sipi_one(struct vcpu *target, uint32_t icr)
         rc = vcpu_reset(target);
         ASSERT(!rc);
         target->fpu_initialised = fpu_initialised;
-        vlapic_reset(vcpu_vlapic(target));
+        vlapic_do_init(vcpu_vlapic(target));
         domain_unlock(target->domain);
         break;
     }
@@ -1237,16 +1239,14 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic *vlapic)
             !(vlapic_get_reg(vlapic, APIC_LVTPC) & APIC_LVT_MASKED));
 }
 
-/* Reset the VLPAIC back to its power-on/reset state. */
-void vlapic_reset(struct vlapic *vlapic)
+/* Reset the VLAPIC back to its init state. */
+static void vlapic_do_init(struct vlapic *vlapic)
 {
-    struct vcpu *v = vlapic_vcpu(vlapic);
     int i;
 
-    if ( !has_vlapic(v->domain) )
+    if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
 
-    vlapic_set_reg(vlapic, APIC_ID,  (v->vcpu_id * 2) << 24);
     vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
 
     for ( i = 0; i < 8; i++ )
@@ -1257,7 +1257,12 @@ void vlapic_reset(struct vlapic *vlapic)
     }
     vlapic_set_reg(vlapic, APIC_ICR,     0);
     vlapic_set_reg(vlapic, APIC_ICR2,    0);
-    vlapic_set_reg(vlapic, APIC_LDR,     0);
+    /*
+     * LDR is read-only in x2APIC mode. Preserve its value when handling
+     * INIT signal in x2APIC mode.
+     */
+    if ( !vlapic_x2apic_mode(vlapic) )
+        vlapic_set_reg(vlapic, APIC_LDR, 0);
     vlapic_set_reg(vlapic, APIC_TASKPRI, 0);
     vlapic_set_reg(vlapic, APIC_TMICT,   0);
     vlapic_set_reg(vlapic, APIC_TMCCT,   0);
@@ -1275,6 +1280,21 @@ void vlapic_reset(struct vlapic *vlapic)
     destroy_periodic_time(&vlapic->pt);
 }
 
+/* Reset the VLAPIC back to its power-on/reset state. */
+void vlapic_reset(struct vlapic *vlapic)
+{
+    const struct vcpu *v = vlapic_vcpu(vlapic);
+
+    if ( !has_vlapic(v->domain) )
+        return;
+
+    vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
+                                APIC_DEFAULT_PHYS_BASE);
+
+    vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
+    vlapic_do_init(vlapic);
+}
+
 /* rearm the actimer if needed, after a HVM restore */
 static void lapic_rearm(struct vlapic *s)
 {
@@ -1489,8 +1509,6 @@ int vlapic_init(struct vcpu *v)
 
     vlapic_reset(vlapic);
 
-    vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
-                                APIC_DEFAULT_PHYS_BASE);
     if ( v->vcpu_id == 0 )
         vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_BSP;
 
-- 
1.8.3.1


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread