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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Apr 20, 2017 at 07:34:30AM -0600, Jan Beulich wrote:
>>>> On 19.04.17 at 22:22, <chao.gao@xxxxxxxxx> wrote:
>> 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@xxxxxxxxx>
>
>Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
>> 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).
>
>Julien,
>
>I'm slightly in favor of taking it now, but I won't object if you decide
>otherwise.
>
>Jan

I just realize that we also need reset bsp field, otherwise the BSP field
may be cleared in vlapic_reset(). Really Sorry for this. 

Jan, do you think this following change is ok? Could you help add this
part when committing? 

Thanks
Chao

---8<---
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 4ac9f46..cf8ee50 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1290,6 +1290,8 @@ void vlapic_reset(struct vlapic *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;
 
     vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
     vlapic_do_init(vlapic);
@@ -1509,9 +1511,6 @@ int vlapic_init(struct vcpu *v)
 
     vlapic_reset(vlapic);
 
-    if ( v->vcpu_id == 0 )
-        vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_BSP;
-
     spin_lock_init(&vlapic->esr_lock);
 
     tasklet_init(&vlapic->init_sipi.tasklet,

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