Message ID | 1492584052-92224-1-git-send-email-chao.gao@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
>>> 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
>>> 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
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
>>> 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
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;
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(-)