| Message ID | 20210115231046.31785-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Initial Trenchboot/SKINIT support | expand |
On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote: > A subsequent change is going to introduce SKINIT support, wherein the APs will > be already be in the wait-for-SIPI state, and an INIT must not be sent. > > Introduce a send_INIT boolean, so we can control sending an INIT IPI > separately from sending SIPIs. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> I'm not opposed to introduce this, but maybe it would be better to move it to a separate helper? send_init(unsigned int apicid); or some such? Would reduce one level of indentation. Thanks, Roger.
On 19/01/2021 14:58, Roger Pau Monné wrote: > On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote: >> A subsequent change is going to introduce SKINIT support, wherein the APs will >> be already be in the wait-for-SIPI state, and an INIT must not be sent. >> >> Introduce a send_INIT boolean, so we can control sending an INIT IPI >> separately from sending SIPIs. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > I'm not opposed to introduce this, but maybe it would be better to > move it to a separate helper? send_init(unsigned int apicid); or some > such? > > Would reduce one level of indentation. I've got a lot of cleanup planned for 4.16, but splitting this up INIT-SIPI-SIPI is specifically not one of them. This will get more complicated with Intel TXT Intel, and I also want to integrate it more nicely with the virtualised AP boot logic. I suspect we'll end up with a function pointer per platform&configuration, but that's too much work at this point in 4.15. ~Andrew
On Tue, Jan 19, 2021 at 03:05:38PM +0000, Andrew Cooper wrote: > On 19/01/2021 14:58, Roger Pau Monné wrote: > > On Fri, Jan 15, 2021 at 11:10:45PM +0000, Andrew Cooper wrote: > >> A subsequent change is going to introduce SKINIT support, wherein the APs will > >> be already be in the wait-for-SIPI state, and an INIT must not be sent. > >> > >> Introduce a send_INIT boolean, so we can control sending an INIT IPI > >> separately from sending SIPIs. > >> > >> No functional change. > >> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > I'm not opposed to introduce this, but maybe it would be better to > > move it to a separate helper? send_init(unsigned int apicid); or some > > such? > > > > Would reduce one level of indentation. > > I've got a lot of cleanup planned for 4.16, but splitting this up > INIT-SIPI-SIPI is specifically not one of them. > > This will get more complicated with Intel TXT Intel, and I also want to > integrate it more nicely with the virtualised AP boot logic. I suspect > we'll end up with a function pointer per platform&configuration, but > that's too much work at this point in 4.15. Ack, I'm fine with this, albeit I would also be fine with dropping the variable and just open-coding the ap_boot_method check (but I guess there will be further use of this variable in newer patches): Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index 9eca452ce1..195e3681b4 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -424,6 +424,7 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip) { unsigned long send_status = 0, accept_status = 0; int maxlvt, timeout, i; + bool send_INIT = true; /* * Some versions of tboot might be able to handle the entire wake sequence @@ -438,49 +439,52 @@ static int wakeup_secondary_cpu(int phys_apicid, unsigned long start_eip) apic_write(APIC_ESR, 0); apic_read(APIC_ESR); - Dprintk("Asserting INIT.\n"); + if ( send_INIT ) + { + Dprintk("Asserting INIT.\n"); - /* - * Turn INIT on target chip via IPI - */ - apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, - phys_apicid); + /* + * Turn INIT on target chip via IPI + */ + apic_icr_write(APIC_INT_LEVELTRIG | APIC_INT_ASSERT | APIC_DM_INIT, + phys_apicid); - if ( !x2apic_enabled ) - { - Dprintk("Waiting for send to finish...\n"); - timeout = 0; - do { - Dprintk("+"); - udelay(100); - send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY; - } while ( send_status && (timeout++ < 1000) ); + if ( !x2apic_enabled ) + { + Dprintk("Waiting for send to finish...\n"); + timeout = 0; + do { + Dprintk("+"); + udelay(100); + send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY; + } while ( send_status && (timeout++ < 1000) ); - mdelay(10); + mdelay(10); - Dprintk("Deasserting INIT.\n"); + Dprintk("Deasserting INIT.\n"); - apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid); + apic_icr_write(APIC_INT_LEVELTRIG | APIC_DM_INIT, phys_apicid); - Dprintk("Waiting for send to finish...\n"); - timeout = 0; - do { - Dprintk("+"); - udelay(100); - send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY; - } while ( send_status && (timeout++ < 1000) ); - } - else if ( tboot_in_measured_env() ) - { - /* - * With tboot AP is actually spinning in a mini-guest before - * receiving INIT. Upon receiving INIT ipi, AP need time to VMExit, - * update VMCS to tracking SIPIs and VMResume. - * - * While AP is in root mode handling the INIT the CPU will drop - * any SIPIs - */ - udelay(10); + Dprintk("Waiting for send to finish...\n"); + timeout = 0; + do { + Dprintk("+"); + udelay(100); + send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY; + } while ( send_status && (timeout++ < 1000) ); + } + else if ( tboot_in_measured_env() ) + { + /* + * With tboot AP is actually spinning in a mini-guest before + * receiving INIT. Upon receiving INIT ipi, AP need time to VMExit, + * update VMCS to tracking SIPIs and VMResume. + * + * While AP is in root mode handling the INIT the CPU will drop + * any SIPIs + */ + udelay(10); + } } maxlvt = get_maxlvt();
A subsequent change is going to introduce SKINIT support, wherein the APs will be already be in the wait-for-SIPI state, and an INIT must not be sent. Introduce a send_INIT boolean, so we can control sending an INIT IPI separately from sending SIPIs. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Marek Kasiewicz <marek.kasiewicz@3mdeb.com> CC: Norbert Kamiński <norbert.kaminski@3mdeb.com> CC: Michal Zygowski <michal.zygowski@3mdeb.com> CC: Piotr Krol <piotr.krol@3mdeb.co> CC: Krystian Hebel <krystian.hebel@3mdeb.com> CC: Daniel P. Smith <dpsmith@apertussolutions.com> CC: Rich Persaud <persaur@gmail.com> CC: Christopher Clark <christopher.w.clark@gmail.com> --- xen/arch/x86/smpboot.c | 78 ++++++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 37 deletions(-)