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 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.

Jan


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