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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.04.17 at 09:37, <chao.gao@xxxxxxxxx> wrote:
> On Thu, Apr 20, 2017 at 08:15:49AM -0600, Jan Beulich wrote:
>>>>> On 20.04.17 at 08:59, <chao.gao@xxxxxxxxx> wrote:
>>> 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? 
>>
>>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


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