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

login
register
mail settings
Submitter Chao Gao
Date April 20, 2017, 6:59 a.m.
Message ID <20170420065949.GA90527@skl-2s3.sh.intel.com>
Download mbox | patch
Permalink /patch/226975/
State New
Headers show

Comments

Chao Gao - April 20, 2017, 6:59 a.m.
On Thu, Apr 20, 2017 at 07:34:30AM -0600, Jan Beulich wrote:
>>>> On 19.04.17 at 22:22, <chao.gao@intel.com> 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@intel.com>
>
>Reviewed-by: Jan Beulich <jbeulich@suse.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).
>
>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<---
Chao Gao - April 20, 2017, 7:37 a.m.
On Thu, Apr 20, 2017 at 08:15:49AM -0600, Jan Beulich wrote:
>>>> On 20.04.17 at 08:59, <chao.gao@intel.com> wrote:
>> On Thu, Apr 20, 2017 at 07:34:30AM -0600, Jan Beulich wrote:
>>>>>> On 19.04.17 at 22:22, <chao.gao@intel.com> 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@intel.com>
>>>
>>>Reviewed-by: Jan Beulich <jbeulich@suse.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).
>>>
>>>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? 
>
>I could certainly fold it in, but I did indeed think about this bit while
>reviewing, and I'm not convinced it needs to be kept. Aiui its value
>is being established (on real hardware) very early using arbitration
>between CPUs. Forcing the bit on for vCPU0 would probably be in
>line with the vlapic_reset() use by hvm_s3_suspend(), but I'm
>rather uncertain about the use in vlapic_msr_set() in this regard.

I check SDM again. In "MODEL-SPECIFIC REGISTERS" -> "Architechural MSRs",
we can know the BSP field is R/W. So in vlapic_msr_set(), clearing BSP
is allowable. In "Advanced Programmable Interrupt Controller" -> "Local APIC"
-> "Local APIC Status and Location", it says "Following a power-up or reset,
the flag is set to 1 for the processor selected as the BSP and set to 0 for
the remaining processors". Which specific problem you think we may have if
we add this part? I just don't want this patch fixes one bug, incurring
another.

Thanks
Chao
Jan Beulich - April 20, 2017, 2:15 p.m.
>>> On 20.04.17 at 08:59, <chao.gao@intel.com> wrote:
> On Thu, Apr 20, 2017 at 07:34:30AM -0600, Jan Beulich wrote:
>>>>> On 19.04.17 at 22:22, <chao.gao@intel.com> 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@intel.com>
>>
>>Reviewed-by: Jan Beulich <jbeulich@suse.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).
>>
>>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? 

I could certainly fold it in, but I did indeed think about this bit while
reviewing, and I'm not convinced it needs to be kept. Aiui its value
is being established (on real hardware) very early using arbitration
between CPUs. Forcing the bit on for vCPU0 would probably be in
line with the vlapic_reset() use by hvm_s3_suspend(), but I'm
rather uncertain about the use in vlapic_msr_set() in this regard.

Jan
Jan Beulich - April 20, 2017, 3:05 p.m.
>>> On 20.04.17 at 09:37, <chao.gao@intel.com> wrote:
> On Thu, Apr 20, 2017 at 08:15:49AM -0600, Jan Beulich wrote:
>>>>> On 20.04.17 at 08:59, <chao.gao@intel.com> wrote:
>>> On Thu, Apr 20, 2017 at 07:34:30AM -0600, Jan Beulich wrote:
>>>>>>> On 19.04.17 at 22:22, <chao.gao@intel.com> 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@intel.com>
>>>>
>>>>Reviewed-by: Jan Beulich <jbeulich@suse.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).
>>>>
>>>>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? 
>>
>>I could certainly fold it in, but I did indeed think about this bit while
>>reviewing, and I'm not convinced it needs to be kept. Aiui its value
>>is being established (on real hardware) very early using arbitration
>>between CPUs. Forcing the bit on for vCPU0 would probably be in
>>line with the vlapic_reset() use by hvm_s3_suspend(), but I'm
>>rather uncertain about the use in vlapic_msr_set() in this regard.
> 
> I check SDM again. In "MODEL-SPECIFIC REGISTERS" -> "Architechural MSRs",
> we can know the BSP field is R/W. So in vlapic_msr_set(), clearing BSP
> is allowable. In "Advanced Programmable Interrupt Controller" -> "Local APIC"
> -> "Local APIC Status and Location", it says "Following a power-up or reset,
> the flag is set to 1 for the processor selected as the BSP and set to 0 for
> the remaining processors". Which specific problem you think we may have if
> we add this part? I just don't want this patch fixes one bug, incurring
> another.

My primary concern is with the guest possibly offlining its vCPU0.
But having looked again, the additional change you suggested is
benign to vlapic_msr_set(), as the full guest specified value is
being written to vlapic->hw.apic_base_msr soon after the call to
vlapic_reset() anyway. IOW - yes, that addendum should be fine.

Jan

Patch

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,