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(-)

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();
-- 
2.11.0

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.