* [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
@ 2017-04-19 6:40 Chao Gao
2017-04-19 14:17 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Chao Gao @ 2017-04-19 6:40 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, 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. So there are two problems in current code:
1. Using reset logic (aka vlapic_reset) to handle INIT signal.
2. Forgetting resetting APIC to xAPIC mode in vlapic_reset()
This patch introduces a new function vlapic_do_init() and replaces the
wrongly used vlapic_reset(). Also reset APIC to xAPIC mode 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>
---
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 | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 2653ba8..6be101f 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,22 @@ 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_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
+ vlapic_set_reg(vlapic, APIC_LDR, 0);
+ vlapic_do_init(vlapic);
+
+ vlapic->hw.apic_base_msr &= ~(uint64_t)MSR_IA32_APICBASE_EXTD;
+ vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_ENABLE;
+}
+
/* rearm the actimer if needed, after a HVM restore */
static void lapic_rearm(struct vlapic *s)
{
@@ -1489,8 +1510,7 @@ int vlapic_init(struct vcpu *v)
vlapic_reset(vlapic);
- vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
- APIC_DEFAULT_PHYS_BASE);
+ vlapic->hw.apic_base_msr |= 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] 6+ messages in thread
* Re: [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
2017-04-19 14:17 ` Jan Beulich
@ 2017-04-19 7:41 ` Chao Gao
2017-04-19 15:04 ` Jan Beulich
2017-04-19 19:33 ` Chao Gao
0 siblings, 2 replies; 6+ messages in thread
From: Chao Gao @ 2017-04-19 7:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Wed, Apr 19, 2017 at 08:17:14AM -0600, Jan Beulich wrote:
>>>> On 19.04.17 at 08:40, <chao.gao@intel.com> wrote:
>> @@ -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) )
>
>In order for this to work you need to ...
>
>> + 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,22 @@ 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_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
>> + vlapic_set_reg(vlapic, APIC_LDR, 0);
>> + vlapic_do_init(vlapic);
>> +
>> + vlapic->hw.apic_base_msr &= ~(uint64_t)MSR_IA32_APICBASE_EXTD;
>
>... do this before calling vlapic_do_init() afaict (and I see no strong
>reason why you would need a cast here)). Likely it is a good idea
>then to also ...
Got it and will fix. The APIC_LDR is also cleared in vlapic_reset.
Do you mean it will do a cast automatically? I don't know that before.
I was afraid that the higher qword of ~MSR_IA32_APICBASE_EXTD will is
zero.
>
>> + vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_ENABLE;
>
>... do this up front.
>
>> @@ -1489,8 +1510,7 @@ int vlapic_init(struct vcpu *v)
>>
>> vlapic_reset(vlapic);
>>
>> - vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
>> - APIC_DEFAULT_PHYS_BASE);
>> + vlapic->hw.apic_base_msr |= APIC_DEFAULT_PHYS_BASE;
>
>Perhaps better move this ahead of the call to vlapic_reset() then
>too.
>
>Remains the question (not answered by the SDM afaics): What
>happens to the base address during reset?
Actually, I don't know and that's also why I don't touch apic_base_msr in the
first verson. Will try to get a confirmation from hardware guys.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
2017-04-19 6:40 [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal Chao Gao
@ 2017-04-19 14:17 ` Jan Beulich
2017-04-19 7:41 ` Chao Gao
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-04-19 14:17 UTC (permalink / raw)
To: Chao Gao; +Cc: Andrew Cooper, xen-devel
>>> On 19.04.17 at 08:40, <chao.gao@intel.com> wrote:
> @@ -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) )
In order for this to work you need to ...
> + 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,22 @@ 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_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
> + vlapic_set_reg(vlapic, APIC_LDR, 0);
> + vlapic_do_init(vlapic);
> +
> + vlapic->hw.apic_base_msr &= ~(uint64_t)MSR_IA32_APICBASE_EXTD;
... do this before calling vlapic_do_init() afaict (and I see no strong
reason why you would need a cast here)). Likely it is a good idea
then to also ...
> + vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_ENABLE;
... do this up front.
> @@ -1489,8 +1510,7 @@ int vlapic_init(struct vcpu *v)
>
> vlapic_reset(vlapic);
>
> - vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
> - APIC_DEFAULT_PHYS_BASE);
> + vlapic->hw.apic_base_msr |= APIC_DEFAULT_PHYS_BASE;
Perhaps better move this ahead of the call to vlapic_reset() then
too.
Remains the question (not answered by the SDM afaics): What
happens to the base address during reset?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
2017-04-19 7:41 ` Chao Gao
@ 2017-04-19 15:04 ` Jan Beulich
2017-04-19 19:33 ` Chao Gao
1 sibling, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-04-19 15:04 UTC (permalink / raw)
To: Chao Gao; +Cc: Andrew Cooper, xen-devel
>>> On 19.04.17 at 09:41, <chao.gao@intel.com> wrote:
> On Wed, Apr 19, 2017 at 08:17:14AM -0600, Jan Beulich wrote:
>>>>> On 19.04.17 at 08:40, <chao.gao@intel.com> wrote:
>>> @@ -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) )
>>
>>In order for this to work you need to ...
>>
>>> + 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,22 @@ 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_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
>>> + vlapic_set_reg(vlapic, APIC_LDR, 0);
>>> + vlapic_do_init(vlapic);
>>> +
>>> + vlapic->hw.apic_base_msr &= ~(uint64_t)MSR_IA32_APICBASE_EXTD;
>>
>>... do this before calling vlapic_do_init() afaict (and I see no strong
>>reason why you would need a cast here)). Likely it is a good idea
>>then to also ...
>
> Got it and will fix. The APIC_LDR is also cleared in vlapic_reset.
> Do you mean it will do a cast automatically? I don't know that before.
> I was afraid that the higher qword of ~MSR_IA32_APICBASE_EXTD will is
> zero.
Depends on whether the value is signed (in which case it will be
sign extended) or unsigned (resulting in zero-extension). The
constant in this case is signed, so the high bits wouldn't be unduly
chopped off.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
2017-04-19 7:41 ` Chao Gao
2017-04-19 15:04 ` Jan Beulich
@ 2017-04-19 19:33 ` Chao Gao
2017-04-20 9:55 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Chao Gao @ 2017-04-19 19:33 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: xen-devel
On Wed, Apr 19, 2017 at 03:41:41PM +0800, Chao Gao wrote:
>>> - vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
>>> - APIC_DEFAULT_PHYS_BASE);
>>> + vlapic->hw.apic_base_msr |= APIC_DEFAULT_PHYS_BASE;
>>
>>Perhaps better move this ahead of the call to vlapic_reset() then
>>too.
>>
>>Remains the question (not answered by the SDM afaics): What
>>happens to the base address during reset?
>
>Actually, I don't know and that's also why I don't touch apic_base_msr in the
>first verson. Will try to get a confirmation from hardware guys.
According to the description about APIC base address in
"ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
"LOCAL APIC" -> "Local APIC Status and Location", the APIC base
address is set to 0xFEE00000H as a result of reset or power-up.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal
2017-04-19 19:33 ` Chao Gao
@ 2017-04-20 9:55 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-04-20 9:55 UTC (permalink / raw)
To: Chao Gao; +Cc: Andrew Cooper, xen-devel
>>> On 19.04.17 at 21:33, <chao.gao@intel.com> wrote:
> On Wed, Apr 19, 2017 at 03:41:41PM +0800, Chao Gao wrote:
>>>> - vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
>>>> - APIC_DEFAULT_PHYS_BASE);
>>>> + vlapic->hw.apic_base_msr |= APIC_DEFAULT_PHYS_BASE;
>>>
>>>Perhaps better move this ahead of the call to vlapic_reset() then
>>>too.
>>>
>>>Remains the question (not answered by the SDM afaics): What
>>>happens to the base address during reset?
>>
>>Actually, I don't know and that's also why I don't touch apic_base_msr in the
>>first verson. Will try to get a confirmation from hardware guys.
>
> According to the description about APIC base address in
> "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
> "LOCAL APIC" -> "Local APIC Status and Location", the APIC base
> address is set to 0xFEE00000H as a result of reset or power-up.
Ah, yes - not very helpful for the different pieces of reset state to
be scattered across the document.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-20 9:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-19 6:40 [PATCH v2] x86/vlapic: Don't reset APIC ID when handling INIT signal Chao Gao
2017-04-19 14:17 ` Jan Beulich
2017-04-19 7:41 ` Chao Gao
2017-04-19 15:04 ` Jan Beulich
2017-04-19 19:33 ` Chao Gao
2017-04-20 9:55 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.