thr3ads.net - Xen devel - [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case [Dec 2011]

If this information is useful, please help other people find it:
Share via:

Wei, Gang

2011-Dec-21 11:22 UTC

[patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Without this delay, Xen could not bring APs up while working with TXT/tboot,
because tboot need some time in APs to handle INIT before becoming ready for
receiving SIPIs. (this delay was removed as part of c/s 23724 by Tim Deegan)

Signed-off-by: Gang Wei <gang.wei@intel.com>

diff -r d1aefee43af1 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
+++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
@@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
             send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
         } while ( send_status && (timeout++ < 1000) );
     }
+    else
+    {
+        mdelay(10);
+    }

     /*
      * Should we send STARTUP IPIs ?


------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev
_______________________________________________
tboot-devel mailing list
tboot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tboot-devel

Jan Beulich

2011-Dec-21 11:49 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

>>> On 21.12.11 at 12:22, "Wei, Gang"
<gang.wei@intel.com> wrote:
> Without this delay, Xen could not bring APs up while working with
TXT/tboot,
> because tboot need some time in APs to handle INIT before becoming ready
for
> receiving SIPIs. (this delay was removed as part of c/s 23724 by Tim
Deegan)
> 
> Signed-off-by: Gang Wei <gang.wei@intel.com>
> 
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
> @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>          } while ( send_status && (timeout++ < 1000) );
>      }
> +    else
> +    {
> +        mdelay(10);
> +    }
Does it really need to be this long then (even in the non-TBOOT
case)?

Jan
> 
>      /*
>       * Should we send STARTUP IPIs ?

Keir Fraser

2011-Dec-21 12:06 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

On 21/12/2011 11:22, "Wei, Gang" <gang.wei@intel.com> wrote:
> Without this delay, Xen could not bring APs up while working with
TXT/tboot,
> because tboot need some time in APs to handle INIT before becoming ready
for
> receiving SIPIs. (this delay was removed as part of c/s 23724 by Tim
Deegan)
Of course Tim will need to review this himself, but a mdelay() right here,
only on the x2apic path just looks bizarre and fragile.

Could we make the !x2apic_enabled conditionals that Tim added be
!(x2apic_enabled || tboot_in_measured_env()) instead? At least that is
somewhat self-documenting and clearly only affects tboot!

 -- Keir
> Signed-off-by: Gang Wei <gang.wei@intel.com>
> 
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
> @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>          } while ( send_status && (timeout++ < 1000) );
>      }
> +    else
> +    {
> +        mdelay(10);
> +    }
> 
>      /*
>       * Should we send STARTUP IPIs ?

Wei, Gang

2011-Dec-21 12:07 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Jan Beulich wrote on 2011-12-21:>>>> On 21.12.11 at 12:22, "Wei, Gang"
<gang.wei@intel.com> wrote:
>> Without this delay, Xen could not bring APs up while working with
>> TXT/tboot, because tboot need some time in APs to handle INIT before
>> becoming ready for receiving SIPIs. (this delay was removed as part
>> of c/s 23724 by Tim Deegan)
>> 
>> Signed-off-by: Gang Wei <gang.wei@intel.com>
>> 
>> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
>> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
>> @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
>>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>>          } while ( send_status && (timeout++ < 1000) );
>>      }
>> +    else
>> +    {
>> +        mdelay(10);
>> +    }
> 
> Does it really need to be this long then (even in the non-TBOOT case)?
No, it could be shorter. I just take a used value back here. If it does matter,
we could use a tested working value here: udelay(10), and for tboot case only.

Jimmy
> 
> Jan
> 
>> 
>>      /*
>>       * Should we send STARTUP IPIs ?
> 
>
------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-21 12:17 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Keir Fraser wrote on 2011-12-21:> On 21/12/2011 11:22, "Wei, Gang" <gang.wei@intel.com>
wrote:
> 
>> Without this delay, Xen could not bring APs up while working with
>> TXT/tboot, because tboot need some time in APs to handle INIT before
>> becoming ready for receiving SIPIs. (this delay was removed as part
>> of c/s 23724 by Tim Deegan)
> 
> Of course Tim will need to review this himself, but a mdelay() right
> here, only on the x2apic path just looks bizarre and fragile.
> 
> Could we make the !x2apic_enabled conditionals that Tim added be
> !(x2apic_enabled || tboot_in_measured_env()) instead? At least that is
> somewhat self-documenting and clearly only affects tboot!
Does below patch make more sense?

diff -r d1aefee43af1 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
+++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
@@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
             send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
         } while ( send_status && (timeout++ < 1000) );
     }
+    else if ( tboot_in_measured_env() )
+    {
+        udelay(10);
+    }

     /*
      * Should we send STARTUP IPIs ?

Jimmy

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-21 12:29 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Resent:

Without this delay, Xen could not bring APs up while working with TXT/tboot,
because tboot need some time in APs to handle INIT before becoming ready for
receiving SIPIs. (this delay was removed as part of c/s 23724 by Tim Deegan)

diff -r d1aefee43af1 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
+++ b/xen/arch/x86/smpboot.c    Wed Dec 21 20:26:39 2011 +0800
@@ -42,6 +42,7 @@
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 #include <asm/time.h>
+#include <asm/tboot.h>
 #include <mach_apic.h>
 #include <mach_wakecpu.h>
 #include <smpboot_hooks.h>
@@ -463,6 +464,10 @@ static int wakeup_secondary_cpu(int phys
             send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
         } while ( send_status && (timeout++ < 1000) );
     }
+    else if ( tboot_in_measured_env() )
+    {
+        udelay(10);
+    }

     /*
      * Should we send STARTUP IPIs ?

Jimmy

> -----Original Message-----
> From: Wei, Gang
> Sent: Wednesday, December 21, 2011 8:18 PM
> To: Keir Fraser; xen-devel@lists.xensource.com
> Cc: tboot-devel@lists.sourceforge.net; Jan Beulich; Tim Deegan; Cihula,
> Joseph; Wei, Gang
> Subject: RE: [patch] x86: Add a delay between INIT & SIPIs for AP
bring-up in
> X2APIC case
> 
> Keir Fraser wrote on 2011-12-21:
> > On 21/12/2011 11:22, "Wei, Gang" <gang.wei@intel.com>
wrote:
> >
> >> Without this delay, Xen could not bring APs up while working with
> >> TXT/tboot, because tboot need some time in APs to handle INIT
before
> >> becoming ready for receiving SIPIs. (this delay was removed as
part
> >> of c/s 23724 by Tim Deegan)
> >
> > Of course Tim will need to review this himself, but a mdelay() right
> > here, only on the x2apic path just looks bizarre and fragile.
> >
> > Could we make the !x2apic_enabled conditionals that Tim added be
> > !(x2apic_enabled || tboot_in_measured_env()) instead? At least that is
> > somewhat self-documenting and clearly only affects tboot!
> 
> Does below patch make more sense?
> 
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
> @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>          } while ( send_status && (timeout++ < 1000) );
>      }
> +    else if ( tboot_in_measured_env() )
> +    {
> +        udelay(10);
> +    }
> 
>      /*
>       * Should we send STARTUP IPIs ?
> 
> Jimmy
------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Keir Fraser

2011-Dec-21 13:13 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Please add a code comment explaining why the delay is needed. Apart from
that:

Acked-by: Keir Fraser <keir@xen.org>


On 21/12/2011 12:29, "Wei, Gang" <gang.wei@intel.com> wrote:
> Resent:
> 
> Without this delay, Xen could not bring APs up while working with
TXT/tboot,
> because tboot need some time in APs to handle INIT before becoming ready
for
> receiving SIPIs. (this delay was removed as part of c/s 23724 by Tim
Deegan)
> 
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 20:26:39 2011 +0800
> @@ -42,6 +42,7 @@
>  #include <asm/msr.h>
>  #include <asm/mtrr.h>
>  #include <asm/time.h>
> +#include <asm/tboot.h>
>  #include <mach_apic.h>
>  #include <mach_wakecpu.h>
>  #include <smpboot_hooks.h>
> @@ -463,6 +464,10 @@ static int wakeup_secondary_cpu(int phys
>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>          } while ( send_status && (timeout++ < 1000) );
>      }
> +    else if ( tboot_in_measured_env() )
> +    {
> +        udelay(10);
> +    }
> 
>      /*
>       * Should we send STARTUP IPIs ?
> 
> Jimmy
> 
> 
>> -----Original Message-----
>> From: Wei, Gang
>> Sent: Wednesday, December 21, 2011 8:18 PM
>> To: Keir Fraser; xen-devel@lists.xensource.com
>> Cc: tboot-devel@lists.sourceforge.net; Jan Beulich; Tim Deegan; Cihula,
>> Joseph; Wei, Gang
>> Subject: RE: [patch] x86: Add a delay between INIT & SIPIs for AP
bring-up in
>> X2APIC case
>> 
>> Keir Fraser wrote on 2011-12-21:
>>> On 21/12/2011 11:22, "Wei, Gang"
<gang.wei@intel.com> wrote:
>>> 
>>>> Without this delay, Xen could not bring APs up while working
with
>>>> TXT/tboot, because tboot need some time in APs to handle INIT
before
>>>> becoming ready for receiving SIPIs. (this delay was removed as
part
>>>> of c/s 23724 by Tim Deegan)
>>> 
>>> Of course Tim will need to review this himself, but a mdelay()
right
>>> here, only on the x2apic path just looks bizarre and fragile.
>>> 
>>> Could we make the !x2apic_enabled conditionals that Tim added be
>>> !(x2apic_enabled || tboot_in_measured_env()) instead? At least that
is
>>> somewhat self-documenting and clearly only affects tboot!
>> 
>> Does below patch make more sense?
>> 
>> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
>> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
>> @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
>>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>>          } while ( send_status && (timeout++ < 1000) );
>>      }
>> +    else if ( tboot_in_measured_env() )
>> +    {
>> +        udelay(10);
>> +    }
>> 
>>      /*
>>       * Should we send STARTUP IPIs ?
>> 
>> Jimmy


------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-21 16:28 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

(resend)

Without this delay, Xen could not bring APs up while working with TXT/tboot,
because tboot need some time in APs to handle INIT before becoming ready for
receiving SIPIs.(this delay was removed as part of c/s 23724 by Tim Deegan)

Signed-off-by: Gang Wei <gang.wei@intel.com>
Acked-by: Keir Fraser <keir@xen.org>

diff -r d1aefee43af1 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Wed Dec 21 18:51:31 2011 +0800
+++ b/xen/arch/x86/smpboot.c	Thu Dec 22 00:26:52 2011 +0800
@@ -42,6 +42,7 @@
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 #include <asm/time.h>
+#include <asm/tboot.h>
 #include <mach_apic.h>
 #include <mach_wakecpu.h>
 #include <smpboot_hooks.h>
@@ -463,6 +464,14 @@
             send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
         } while ( send_status && (timeout++ < 1000) );
     }
+    else if ( tboot_in_measured_env() )
+    {
+        /*
+         * give AP time to handle INIT before ready to receive SIPIs
+         * otherwise SIPIs may be skipped and AP will fail to up
+         */
+        udelay(10);
+    }
 
     /*
      * Should we send STARTUP IPIs ?


------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev
_______________________________________________
tboot-devel mailing list
tboot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tboot-devel

Tim Deegan

2011-Dec-21 17:05 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

At 12:29 +0000 on 21 Dec (1324470582), Wei, Gang wrote:> Without this delay, Xen could not bring APs up while working with
> TXT/tboot, because tboot need some time in APs to handle INIT before
> becoming ready for receiving SIPIs. (this delay was removed as part of
> c/s 23724 by Tim Deegan)
It was removed because I was seeing the opposite problem -- if there was
a delay, the AP did not come up.  Unfortunately I don;''t have sucah a
machine available any more, so I can''t check whether this breaks boot
there. 

Is this something that can be fixed in tboot?  If not, than this patch
is OK, provided it gets a code comment explaining _why_ tboot needs the
delay. 

Cheers,

Tim.
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 20:26:39 2011 +0800
> @@ -42,6 +42,7 @@
>  #include <asm/msr.h>
>  #include <asm/mtrr.h>
>  #include <asm/time.h>
> +#include <asm/tboot.h>
>  #include <mach_apic.h>
>  #include <mach_wakecpu.h>
>  #include <smpboot_hooks.h>
> @@ -463,6 +464,10 @@ static int wakeup_secondary_cpu(int phys
>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>          } while ( send_status && (timeout++ < 1000) );
>      }
> +    else if ( tboot_in_measured_env() )
> +    {
> +        udelay(10);
> +    }
> 
>      /*
>       * Should we send STARTUP IPIs ?
> 
> Jimmy
> 
> 
> > -----Original Message-----
> > From: Wei, Gang
> > Sent: Wednesday, December 21, 2011 8:18 PM
> > To: Keir Fraser; xen-devel@lists.xensource.com
> > Cc: tboot-devel@lists.sourceforge.net; Jan Beulich; Tim Deegan;
Cihula,
> > Joseph; Wei, Gang
> > Subject: RE: [patch] x86: Add a delay between INIT & SIPIs for AP
bring-up in
> > X2APIC case
> > 
> > Keir Fraser wrote on 2011-12-21:
> > > On 21/12/2011 11:22, "Wei, Gang"
<gang.wei@intel.com> wrote:
> > >
> > >> Without this delay, Xen could not bring APs up while working
with
> > >> TXT/tboot, because tboot need some time in APs to handle INIT
before
> > >> becoming ready for receiving SIPIs. (this delay was removed
as part
> > >> of c/s 23724 by Tim Deegan)
> > >
> > > Of course Tim will need to review this himself, but a mdelay()
right
> > > here, only on the x2apic path just looks bizarre and fragile.
> > >
> > > Could we make the !x2apic_enabled conditionals that Tim added be
> > > !(x2apic_enabled || tboot_in_measured_env()) instead? At least
that is
> > > somewhat self-documenting and clearly only affects tboot!
> > 
> > Does below patch make more sense?
> > 
> > diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> > --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> > +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 19:08:57 2011 +0800
> > @@ -463,6 +463,10 @@ static int wakeup_secondary_cpu(int phys
> >              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
> >          } while ( send_status && (timeout++ < 1000) );
> >      }
> > +    else if ( tboot_in_measured_env() )
> > +    {
> > +        udelay(10);
> > +    }
> > 
> >      /*
> >       * Should we send STARTUP IPIs ?
> > 
> > Jimmy

Wei, Gang

2011-Dec-22 01:32 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Tim Deegan wrote on 2011-12-22:> At 12:29 +0000 on 21 Dec (1324470582), Wei, Gang wrote:
>> Without this delay, Xen could not bring APs up while working with
>> TXT/tboot, because tboot need some time in APs to handle INIT before
>> becoming ready for receiving SIPIs. (this delay was removed as part
>> of c/s 23724 by Tim Deegan)
> 
> It was removed because I was seeing the opposite problem -- if there
> was a delay, the AP did not come up.  Unfortunately I don;''t have
> sucah a machine available any more, so I can''t check whether this
breaks boot there.
> 
> Is this something that can be fixed in tboot?  If not, than this patch
> is OK, provided it gets a code comment explaining _why_ tboot needs the
delay.
First, Linux kernel always place a delay between INIT & SIPIs.

Second, 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. After that SIPI is able to be trapped by APs. This is MUST
if giving no change to current AP bring-up method(INIT-SIPI-SIPI) for tboot.

I have provided a resent patch with code comment in a previous reply.

Jimmy
> 
> Cheers,
> 
> Tim.
> 
>> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
>> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 20:26:39 2011 +0800
>> @@ -42,6 +42,7 @@
>>  #include <asm/msr.h> #include <asm/mtrr.h> #include
<asm/time.h>
>>  +#include <asm/tboot.h> #include <mach_apic.h> #include
>>  <mach_wakecpu.h> #include <smpboot_hooks.h>
>> @@ -463,6 +464,10 @@ static int wakeup_secondary_cpu(int phys
>>              send_status = apic_read(APIC_ICR) & APIC_ICR_BUSY;
>>          } while ( send_status && (timeout++ < 1000) );
>>      }
>> +    else if ( tboot_in_measured_env() )
>> +    {
>> +        udelay(10);
>> +    }
>> 
>>      /*
>>       * Should we send STARTUP IPIs ?
------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Tim Deegan

2011-Dec-22 10:01 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

At 01:32 +0000 on 22 Dec (1324517520), Wei, Gang wrote:> Tim Deegan wrote on 2011-12-22:
> > At 12:29 +0000 on 21 Dec (1324470582), Wei, Gang wrote:
> >> Without this delay, Xen could not bring APs up while working with
> >> TXT/tboot, because tboot need some time in APs to handle INIT
before
> >> becoming ready for receiving SIPIs. (this delay was removed as
part
> >> of c/s 23724 by Tim Deegan)
> > 
> > It was removed because I was seeing the opposite problem -- if there
> > was a delay, the AP did not come up.  Unfortunately I don;''t
have
> > sucah a machine available any more, so I can''t check whether
this breaks boot there.
> > 
> > Is this something that can be fixed in tboot?  If not, than this patch
> > is OK, provided it gets a code comment explaining _why_ tboot needs
the delay.
> 
> First, Linux kernel always place a delay between INIT & SIPIs.
Linux does a lot of things. :)  That delay is suggested in the manuals
too, but AIUI it''s for older hardware that doesn''t
synchronously deliver
the INIT.  And there are machines where the delay causes the AP not to
come up.  But I suppose on such a machine tboot will already have
brought up the APs (or failed to) so there''s no harm in the delay.
> Second, 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. After that SIPI is able to
> be trapped by APs. This is MUST if giving no change to current AP
> bring-up method(INIT-SIPI-SIPI) for tboot.
Oh I see - and the CPU blocks SIPIs in root mode; how inconvenient. 
Does tboot also need a delay between the two SIPIs then?
> I have provided a resent patch with code comment in a previous reply.
Your comment should say _why_ -- in particular that while tboot is in
root mode handling the INIT the CPU will drop any SIPIs. 

With that change, Ack. 

Tim.
> Jimmy
> 
> > 
> > Cheers,
> > 
> > Tim.
> > 
> >> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> >> --- a/xen/arch/x86/smpboot.c    Wed Dec 21 18:51:31 2011 +0800
> >> +++ b/xen/arch/x86/smpboot.c    Wed Dec 21 20:26:39 2011 +0800
> >> @@ -42,6 +42,7 @@
> >>  #include <asm/msr.h> #include <asm/mtrr.h> #include
<asm/time.h>
> >>  +#include <asm/tboot.h> #include <mach_apic.h>
#include
> >>  <mach_wakecpu.h> #include <smpboot_hooks.h>
> >> @@ -463,6 +464,10 @@ static int wakeup_secondary_cpu(int phys
> >>              send_status = apic_read(APIC_ICR) &
APIC_ICR_BUSY;
> >>          } while ( send_status && (timeout++ < 1000) );
> >>      }
> >> +    else if ( tboot_in_measured_env() )
> >> +    {
> >> +        udelay(10);
> >> +    }
> >> 
> >>      /*
> >>       * Should we send STARTUP IPIs ?
------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-23 02:48 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Tim Deegan wrote on 2011-12-22:>> Second, 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. After that SIPI is able
>> to be trapped by APs. This is MUST if giving no change to current AP
>> bring-up method(INIT-SIPI-SIPI) for tboot.
> 
> Oh I see - and the CPU blocks SIPIs in root mode; how inconvenient.
> Does tboot also need a delay between the two SIPIs then?
No. Tboot actually skip the second SIPI.
>> I have provided a resent patch with code comment in a previous reply.
> 
> Your comment should say _why_ -- in particular that while tboot is in
> root mode handling the INIT the CPU will drop any SIPIs.
Ok, I will add more words and resend it again.
> With that change, Ack.
> 
> Tim.
Jimmy

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-23 03:14 UTC

head link

[patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Without this delay, Xen could not bring APs up while working with TXT/tboot,
because tboot need some time in APs to handle INIT before becoming ready for
receiving SIPIs.(this delay was removed as part of c/s 23724 by Tim Deegan)

Signed-off-by: Gang Wei <gang.wei@intel.com>
Acked-by: Keir Fraser <keir@xen.org>
Acked-by: Tim Deegan <tim.deegan@citrix.com>

diff -r d1aefee43af1 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Wed Dec 21 18:51:31 2011 +0800
+++ b/xen/arch/x86/smpboot.c	Fri Dec 23 11:01:10 2011 +0800
@@ -42,6 +42,7 @@
 #include <asm/msr.h>
 #include <asm/mtrr.h>
 #include <asm/time.h>
+#include <asm/tboot.h>
 #include <mach_apic.h>
 #include <mach_wakecpu.h>
 #include <smpboot_hooks.h>
@@ -463,6 +464,18 @@
             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);
+    }
 
     /*
      * Should we send STARTUP IPIs ?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Dec-23 08:11 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

>>> On 23.12.11 at 04:14, "Wei, Gang"
<gang.wei@intel.com> wrote:
> Without this delay, Xen could not bring APs up while working with
TXT/tboot,
> because tboot need some time in APs to handle INIT before becoming ready
for
> receiving SIPIs.(this delay was removed as part of c/s 23724 by Tim Deegan)
> 
> Signed-off-by: Gang Wei <gang.wei@intel.com>
> Acked-by: Keir Fraser <keir@xen.org>
> Acked-by: Tim Deegan <tim.deegan@citrix.com>
> 
> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c	Wed Dec 21 18:51:31 2011 +0800
> +++ b/xen/arch/x86/smpboot.c	Fri Dec 23 11:01:10 2011 +0800
> @@ -42,6 +42,7 @@
>  #include <asm/msr.h>
>  #include <asm/mtrr.h>
>  #include <asm/time.h>
> +#include <asm/tboot.h>
>  #include <mach_apic.h>
>  #include <mach_wakecpu.h>
>  #include <smpboot_hooks.h>
> @@ -463,6 +464,18 @@
>              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);
So you jumped from 100ms to 10us - how was that value established?
Or in other words, how certain is it that this (or any other) timeout is
sufficient for all current and future systems implementing tboot?

Jan
> +    }
>  
>      /*
>       * Should we send STARTUP IPIs ?



------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-27 10:05 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Jan Beulich wrote on 2011-12-23:>>>> On 23.12.11 at 04:14, "Wei, Gang"
<gang.wei@intel.com> wrote:
>> Without this delay, Xen could not bring APs up while working with
>> TXT/tboot, because tboot need some time in APs to handle INIT before
>> becoming ready for receiving SIPIs.(this delay was removed as part
>> of c/s 23724 by Tim Deegan)
>> 
>> Signed-off-by: Gang Wei <gang.wei@intel.com>
>> Acked-by: Keir Fraser <keir@xen.org>
>> Acked-by: Tim Deegan <tim.deegan@citrix.com>
>> 
>> diff -r d1aefee43af1 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c	Wed Dec 21 18:51:31 2011 +0800
>> +++ b/xen/arch/x86/smpboot.c	Fri Dec 23 11:01:10 2011 +0800
>> @@ -42,6 +42,7 @@
>>  #include <asm/msr.h> #include <asm/mtrr.h> #include
<asm/time.h>
>>  +#include <asm/tboot.h> #include <mach_apic.h> #include
>>  <mach_wakecpu.h> #include <smpboot_hooks.h>
>> @@ -463,6 +464,18 @@
>>              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);
> 
> So you jumped from 100ms to 10us - how was that value established?
> Or in other words, how certain is it that this (or any other) timeout
> is sufficient for all current and future systems implementing tboot?
First way, code analysis. Tboot VMExitHandler only judge the condition, enable
trapping SIPI, then VMResume. 10us means more than 10,000 cycles in Intel
processors supporting TXT, it should be enough for this simple code path.

Second way, I tested it. The minimum value I tested is udelay(1). So I think
udelay(10) should give enough margin.

Further, I am working on changing the SMP bring-up sequence for tboot path from
INIT-SIPI-SIPI to MWAIT-memwrite style. It means tboot APs will wait in MWAIT
loops and Xen BSP will try to write the monitored memory to bring APs out of
MWAIT loops.

Jimmy



------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Tim Deegan

2011-Dec-27 10:47 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

At 10:05 +0000 on 27 Dec (1324980346), Wei, Gang wrote:> > So you jumped from 100ms to 10us - how was that value established?
> > Or in other words, how certain is it that this (or any other) timeout
> > is sufficient for all current and future systems implementing tboot?
> 
> First way, code analysis. Tboot VMExitHandler only judge the
> condition, enable trapping SIPI, then VMResume. 10us means more than
> 10,000 cycles in Intel processors supporting TXT, it should be enough
> for this simple code path.
Unless the BIOS does clever things in SMM of course. :)
> Further, I am working on changing the SMP bring-up sequence for tboot
> path from INIT-SIPI-SIPI to MWAIT-memwrite style. It means tboot APs
> will wait in MWAIT loops and Xen BSP will try to write the monitored
> memory to bring APs out of MWAIT loops.
That sounds like a very good idea.

Cheers,

Tim.

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Wei, Gang

2011-Dec-28 01:22 UTC

head link

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Tim Deegan wrote on 2011-12-27:> At 10:05 +0000 on 27 Dec (1324980346), Wei, Gang wrote:
>>> So you jumped from 100ms to 10us - how was that value established?
>>> Or in other words, how certain is it that this (or any other)
>>> timeout is sufficient for all current and future systems
implementing tboot?
>> 
>> First way, code analysis. Tboot VMExitHandler only judge the
>> condition, enable trapping SIPI, then VMResume. 10us means more than
>> 10,000 cycles in Intel processors supporting TXT, it should be
>> enough for this simple code path.
> 
> Unless the BIOS does clever things in SMM of course. :)
If no further question on this patch from anyone else, could you help to check
it in?
> 
>> Further, I am working on changing the SMP bring-up sequence for
>> tboot path from INIT-SIPI-SIPI to MWAIT-memwrite style. It means
>> tboot APs will wait in MWAIT loops and Xen BSP will try to write the
>> monitored memory to bring APs out of MWAIT loops.
> 
> That sounds like a very good idea.
I will send out the patch for MWAIT SMP bring-up soon.

Jimmy

------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create 
new or port existing apps to sell to consumers worldwide. Explore the 
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev

Tim Deegan

2011-Dec-29 10:11 UTC

head link

Re: [Xen-devel] [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

At 01:22 +0000 on 28 Dec (1325035368), Wei, Gang wrote:> If no further question on this patch from anyone else, could you help
> to check it in?
Done.

Tim.

------------------------------------------------------------------------------
Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don''t need a
complex
infrastructure or vast IT resources to deliver seamless, secure access to
virtual desktops. With this all-in-one solution, easily deploy virtual 
desktops for less than the cost of PCs and save 60% on VDI infrastructure 
costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox

Wei, Gang

2011-Dec-29 11:25 UTC

head link

Re: [Xen-devel] [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Tim Deegan wrote on 2011-12-29:> At 01:22 +0000 on 28 Dec (1325035368), Wei, Gang wrote:
>> If no further question on this patch from anyone else, could you
>> help to check it in?
> 
> Done.
Thanks. And I have already be ready to send the MWAIT AP bring-up patch for
tboot case, but it might be better to send it out after the New Year,
isn''t it?

Jimmy



------------------------------------------------------------------------------
Ridiculously easy VDI. With Citrix VDI-in-a-Box, you don''t need a
complex
infrastructure or vast IT resources to deliver seamless, secure access to
virtual desktops. With this all-in-one solution, easily deploy virtual 
desktops for less than the cost of PCs and save 60% on VDI infrastructure 
costs. Try it free! http://p.sf.net/sfu/Citrix-VDIinabox

Xen devel - Dec 2011 - [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

[patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for AP bring-up in X2APIC case

[patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [Xen-devel] [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case

Re: [Xen-devel] [patch] x86: Add a delay between INIT & SIPIs for tboot AP bring-up in X2APIC case