tboot-devel — Developer support

Re: [tboot-devel] [PATCH 2/7] tboot: Add return values for tboot_sleep

[Konrad R. W. <kon...@or...>]

On Tue, Jan 10, 2012 at 08:10:53PM +0000, Cihula, Joseph wrote:
> ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which you are not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its errors through tboot_sleep().

Hey Joe,

Thanks for looking at the patches.

Right now [with this patch applied] the code looks as so:

297 
298         tboot_shutdown(acpi_shutdown_map[sleep_state]);
299         return 0;
300 }


If we do make tboot_shutdown() return an int, there will be a need to modify
other callers: native_machine_emergency_restart, native_machine_halt, 
native_machine_power_off, and native_play_dead as well.

Perhaps that could be done in another patch and leave this one as is
where the 'tboot_sleep' would just return 0 instead of
'return tboot_shutdown(...)' ?

Perhaps another way to do this is to extract the common code of tboot_sleep
and tboot_shutdown?

> 
> Joe
> 
> > -----Original Message-----
> > From: Konrad Rzeszutek Wilk [mailto:kon...@or...]
> > Sent: Friday, December 16, 2011 2:38 PM
> > To: lin...@vg...; rj...@si...; x8...@ke...; Brown, Len; Cihula, Joseph; linux-
> > pm...@li...; tbo...@li...; lin...@vg...;
> > lia...@or...
> > Cc: hp...@zy...; Konrad Rzeszutek Wilk
> > Subject: [PATCH 2/7] tboot: Add return values for tboot_sleep
> > 
> > . as appropiately. As tboot_sleep now returns values.
> > remove tboot_sleep_wrapper.
> > 
> > Suggested-by: "Rafael J. Wysocki" <rj...@si...>
> > CC: Joseph Cihula <jos...@in...>
> > [v1: Return -1/0/+1 instead of ACPI_xx values]
> > Signed-off-by: Konrad Rzeszutek Wilk <kon...@or...>
> > ---
> >  arch/x86/kernel/tboot.c |   13 ++++---------
> >  1 files changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 1a4ab7d..6410744 100644
> > --- a/arch/x86/kernel/tboot.c
> > +++ b/arch/x86/kernel/tboot.c
> > @@ -272,7 +272,7 @@ static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
> >  		offsetof(struct acpi_table_facs, firmware_waking_vector);  }
> > 
> > -void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
> > +static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32
> > +pm1b_control)
> >  {
> >  	static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
> >  		/* S0,1,2: */ -1, -1, -1,
> > @@ -281,7 +281,7 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
> >  		/* S5: */ TB_SHUTDOWN_S5 };
> > 
> >  	if (!tboot_enabled())
> > -		return;
> > +		return 0;
> > 
> >  	tboot_copy_fadt(&acpi_gbl_FADT);
> >  	tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control; @@ -292,15 +292,10 @@ void tboot_sleep(u8
> > sleep_state, u32 pm1a_control, u32 pm1b_control)
> >  	if (sleep_state >= ACPI_S_STATE_COUNT ||
> >  	    acpi_shutdown_map[sleep_state] == -1) {
> >  		pr_warning("unsupported sleep state 0x%x\n", sleep_state);
> > -		return;
> > +		return -1;
> >  	}
> > 
> >  	tboot_shutdown(acpi_shutdown_map[sleep_state]);
> > -}
> > -static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
> > -			       u32 pm1b_control)
> > -{
> > -	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
> >  	return 0;
> >  }
> > 
> > @@ -352,7 +347,7 @@ static __init int tboot_late_init(void)
> >  	atomic_set(&ap_wfs_count, 0);
> >  	register_hotcpu_notifier(&tboot_cpu_notifier);
> > 
> > -	acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
> > +	acpi_os_set_prepare_sleep(&tboot_sleep);
> >  	return 0;
> >  }
> > 
> > --
> > 1.7.7.4

Re: [tboot-devel] [PATCH 2/7] tboot: Add return values for tboot_sleep

[Cihula, J. <jos...@in...>]

ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which you are not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its errors through tboot_sleep().

Joe

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:kon...@or...]
> Sent: Friday, December 16, 2011 2:38 PM
> To: lin...@vg...; rj...@si...; x8...@ke...; Brown, Len; Cihula, Joseph; linux-
> pm...@li...; tbo...@li...; lin...@vg...;
> lia...@or...
> Cc: hp...@zy...; Konrad Rzeszutek Wilk
> Subject: [PATCH 2/7] tboot: Add return values for tboot_sleep
> 
> . as appropiately. As tboot_sleep now returns values.
> remove tboot_sleep_wrapper.
> 
> Suggested-by: "Rafael J. Wysocki" <rj...@si...>
> CC: Joseph Cihula <jos...@in...>
> [v1: Return -1/0/+1 instead of ACPI_xx values]
> Signed-off-by: Konrad Rzeszutek Wilk <kon...@or...>
> ---
>  arch/x86/kernel/tboot.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 1a4ab7d..6410744 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -272,7 +272,7 @@ static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
>  		offsetof(struct acpi_table_facs, firmware_waking_vector);  }
> 
> -void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
> +static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32
> +pm1b_control)
>  {
>  	static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
>  		/* S0,1,2: */ -1, -1, -1,
> @@ -281,7 +281,7 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
>  		/* S5: */ TB_SHUTDOWN_S5 };
> 
>  	if (!tboot_enabled())
> -		return;
> +		return 0;
> 
>  	tboot_copy_fadt(&acpi_gbl_FADT);
>  	tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control; @@ -292,15 +292,10 @@ void tboot_sleep(u8
> sleep_state, u32 pm1a_control, u32 pm1b_control)
>  	if (sleep_state >= ACPI_S_STATE_COUNT ||
>  	    acpi_shutdown_map[sleep_state] == -1) {
>  		pr_warning("unsupported sleep state 0x%x\n", sleep_state);
> -		return;
> +		return -1;
>  	}
> 
>  	tboot_shutdown(acpi_shutdown_map[sleep_state]);
> -}
> -static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
> -			       u32 pm1b_control)
> -{
> -	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
>  	return 0;
>  }
> 
> @@ -352,7 +347,7 @@ static __init int tboot_late_init(void)
>  	atomic_set(&ap_wfs_count, 0);
>  	register_hotcpu_notifier(&tboot_cpu_notifier);
> 
> -	acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
> +	acpi_os_set_prepare_sleep(&tboot_sleep);
>  	return 0;
>  }
> 
> --
> 1.7.7.4

Re: [tboot-devel] [PATCH 1/7] x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling tboot_sleep.

[Cihula, J. <jos...@in...>]

ACK

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:kon...@or...]
> Sent: Tuesday, January 03, 2012 9:13 AM
> To: lin...@vg...; rj...@si...; x8...@ke...; Brown, Len; Cihula, Joseph; linux-
> pm...@li...; tbo...@li...; lin...@vg...;
> lia...@or...
> Cc: hp...@zy...; Thomas Gleixner; Wang, Shane; xen...@li...
> Subject: Re: [PATCH 1/7] x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling
> tboot_sleep.
> 
> On Fri, Dec 16, 2011 at 05:38:13PM -0500, Konrad Rzeszutek Wilk wrote:
> > From: Tang Liang <lia...@or...>
> 
> Hey Joseph,
> 
> I remember you acked the initial rfc patches I had posted.
> But I was wondering if these ones are to your liking (I think they are - they aren't that much
> different, but I didn't want to blindly sticking 'Acked-by' as they did change a bit).
> 
> Thanks!
> >
> > The ACPI suspend path makes a call to tboot_sleep right before it
> > writes the PM1A, PM1B values. We replace the direct call to tboot via
> > an registration callback similar to __acpi_register_gsi.
> >
> > CC: Thomas Gleixner <tg...@li...>
> > CC: "H. Peter Anvin" <hp...@zy...>
> > CC: x8...@ke...
> > CC: Len Brown <len...@in...>
> > CC: Joseph Cihula <jos...@in...>
> > CC: Shane Wang <sha...@in...>
> > CC: xen...@li...
> > CC: lin...@li...
> > CC: tbo...@li...
> > CC: lin...@vg...
> > [v1: Added __attribute__ ((unused))]
> > [v2: Introduced a wrapper instead of changing tboot_sleep return
> > values]
> > [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare]
> > Signed-off-by: Tang Liang <lia...@or...>
> > [v1: Fix compile issues on IA64 and PPC64]
> > [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep
> > properly]
> > Signed-off-by: Konrad Rzeszutek Wilk <kon...@or...>
> > ---
> >  arch/x86/kernel/tboot.c       |    8 ++++++++
> >  drivers/acpi/acpica/hwsleep.c |   10 +++++++---
> >  drivers/acpi/osl.c            |   24 ++++++++++++++++++++++++
> >  include/acpi/acexcep.h        |    1 +
> >  include/linux/acpi.h          |   10 ++++++++++
> >  include/linux/tboot.h         |    1 -
> >  6 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index
> > e2410e2..1a4ab7d 100644
> > --- a/arch/x86/kernel/tboot.c
> > +++ b/arch/x86/kernel/tboot.c
> > @@ -297,6 +297,12 @@ void tboot_sleep(u8 sleep_state, u32
> > pm1a_control, u32 pm1b_control)
> >
> >  	tboot_shutdown(acpi_shutdown_map[sleep_state]);
> >  }
> > +static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
> > +			       u32 pm1b_control)
> > +{
> > +	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
> > +	return 0;
> > +}
> >
> >  static atomic_t ap_wfs_count;
> >
> > @@ -345,6 +351,8 @@ static __init int tboot_late_init(void)
> >
> >  	atomic_set(&ap_wfs_count, 0);
> >  	register_hotcpu_notifier(&tboot_cpu_notifier);
> > +
> > +	acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/acpi/acpica/hwsleep.c
> > b/drivers/acpi/acpica/hwsleep.c index d52da30..992359a 100644
> > --- a/drivers/acpi/acpica/hwsleep.c
> > +++ b/drivers/acpi/acpica/hwsleep.c
> > @@ -43,9 +43,9 @@
> >   */
> >
> >  #include <acpi/acpi.h>
> > +#include <linux/acpi.h>
> >  #include "accommon.h"
> >  #include "actables.h"
> > -#include <linux/tboot.h>
> >  #include <linux/module.h>
> >
> >  #define _COMPONENT          ACPI_HARDWARE
> > @@ -344,8 +344,12 @@ acpi_status asmlinkage acpi_enter_sleep_state(u8
> > sleep_state)
> >
> >  	ACPI_FLUSH_CPU_CACHE();
> >
> > -	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
> > -
> > +	status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
> > +				       pm1b_control);
> > +	if (ACPI_SKIP(status))
> > +		return_ACPI_STATUS(AE_OK);
> > +	if (ACPI_FAILURE(status))
> > +		return_ACPI_STATUS(status);
> >  	/* Write #2: Write both SLP_TYP + SLP_EN */
> >
> >  	status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control); diff
> > --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index f31c5c5..f3aae4b
> > 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -76,6 +76,9 @@ EXPORT_SYMBOL(acpi_in_debugger);  extern char
> > line_buf[80];
> >  #endif				/*ENABLE_DEBUGGER */
> >
> > +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
> > +				      u32 pm1b_ctrl);
> > +
> >  static acpi_osd_handler acpi_irq_handler;  static void
> > *acpi_irq_context;  static struct workqueue_struct *kacpid_wq; @@
> > -1659,3 +1662,24 @@ acpi_status acpi_os_terminate(void)
> >
> >  	return AE_OK;
> >  }
> > +
> > +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
> > +				  u32 pm1b_control)
> > +{
> > +	int rc = 0;
> > +	if (__acpi_os_prepare_sleep)
> > +		rc = __acpi_os_prepare_sleep(sleep_state,
> > +					     pm1a_control, pm1b_control);
> > +	if (rc < 0)
> > +		return AE_ERROR;
> > +	else if (rc > 0)
> > +		return AE_CTRL_SKIP;
> > +
> > +	return AE_OK;
> > +}
> > +
> > +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> > +			       u32 pm1a_ctrl, u32 pm1b_ctrl)) {
> > +	__acpi_os_prepare_sleep = func;
> > +}
> > diff --git a/include/acpi/acexcep.h b/include/acpi/acexcep.h index
> > 5b6c391..fa0d22c 100644
> > --- a/include/acpi/acexcep.h
> > +++ b/include/acpi/acexcep.h
> > @@ -57,6 +57,7 @@
> >  #define ACPI_SUCCESS(a)                 (!(a))
> >  #define ACPI_FAILURE(a)                 (a)
> >
> > +#define ACPI_SKIP(a)                    (a == AE_CTRL_SKIP)
> >  #define AE_OK                           (acpi_status) 0x0000
> >
> >  /*
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
> > 6001b4da..fccd017 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -359,4 +359,14 @@ static inline int suspend_nvs_register(unsigned
> > long a, unsigned long b)  }  #endif
> >
> > +#ifdef CONFIG_ACPI
> > +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> > +			       u32 pm1a_ctrl,  u32 pm1b_ctrl));
> > +
> > +acpi_status acpi_os_prepare_sleep(u8 sleep_state,
> > +				  u32 pm1a_control, u32 pm1b_control); #else #define
> > +acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while
> > +(0) #endif
> > +
> >  #endif	/*_LINUX_ACPI_H*/
> > diff --git a/include/linux/tboot.h b/include/linux/tboot.h index
> > 1dba6ee..c75128b 100644
> > --- a/include/linux/tboot.h
> > +++ b/include/linux/tboot.h
> > @@ -143,7 +143,6 @@ static inline int tboot_enabled(void)
> >
> >  extern void tboot_probe(void);
> >  extern void tboot_shutdown(u32 shutdown_type); -extern void
> > tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control);
> > extern struct acpi_table_header *tboot_get_dmar_table(
> >  				      struct acpi_table_header *dmar_tbl);  extern int
> > tboot_force_iommu(void);
> > --
> > 1.7.7.4

[tboot-devel] [PATCH v3] x86: add a new SMP bring up way for tboot case

[Wei, G. <gan...@in...>]

tboot may be trying to put APs waiting in MWAIT loops before launching Xen. Xen could check the new flag field in v6 tboot shared page for the hint. If TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP have to write the monitored memory(g_tboot_shared->ap_wake_trigger) to bring APs out of MWAIT loops. The sipi vector should be written in g_tboot_shared->ap_wake_addr before waking up APs.

Signed-off-by: Joseph Cihula <jos...@in...>
Signed-off-by: Shane Wang <sha...@in...>
Signed-off-by: Gang Wei <gan...@in...>

diff -r 8087674cceb9 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/arch/x86/smpboot.c	Tue Jan 10 09:01:25 2012 +0800
@@ -586,7 +586,13 @@
     smpboot_setup_warm_reset_vector(start_eip);
 
     /* Starting actual IPI sequence... */
-    boot_error = wakeup_secondary_cpu(apicid, start_eip);
+    if ( tboot_in_measured_env() )
+        boot_error = tboot_wake_ap(apicid, start_eip);
+    else
+        boot_error = 1;
+
+    if ( boot_error )
+        boot_error = wakeup_secondary_cpu(apicid, start_eip);
 
     if ( !boot_error )
     {
diff -r 8087674cceb9 xen/arch/x86/tboot.c
--- a/xen/arch/x86/tboot.c	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/arch/x86/tboot.c	Tue Jan 10 09:01:25 2012 +0800
@@ -123,6 +123,8 @@
     printk("  shutdown_entry: 0x%08x\n", tboot_shared->shutdown_entry);
     printk("  tboot_base: 0x%08x\n", tboot_shared->tboot_base);
     printk("  tboot_size: 0x%x\n", tboot_shared->tboot_size);
+    if ( tboot_shared->version >= 6 )
+        printk("  flags: 0x%08x\n", tboot_shared->flags);
 
     /* these will be needed by tboot_protect_mem_regions() and/or
        tboot_parse_dmar_table(), so get them now */
@@ -529,6 +531,18 @@
     panic("Memory integrity was lost on resume (%d)\n", error);
 }
 
+int tboot_wake_ap(int apicid, unsigned long sipi_vec)
+{
+    if ( g_tboot_shared->version >= 6 &&
+         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
+    {
+        g_tboot_shared->ap_wake_addr = sipi_vec;
+        g_tboot_shared->ap_wake_trigger = apicid;
+        return 0;
+    }
+    return 1;
+}
+
 /*
  * Local variables:
  * mode: C
diff -r 8087674cceb9 xen/include/asm-x86/tboot.h
--- a/xen/include/asm-x86/tboot.h	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/include/asm-x86/tboot.h	Tue Jan 10 09:01:25 2012 +0800
@@ -85,7 +85,7 @@
 typedef struct __packed {
     /* version 3+ fields: */
     uuid_t    uuid;              /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
-    uint32_t  version;           /* Version number; currently supports 0.4 */
+    uint32_t  version;           /* Version number; currently supports 0.6 */
     uint32_t  log_addr;          /* physical addr of tb_log_t log */
     uint32_t  shutdown_entry;    /* entry point for tboot shutdown */
     uint32_t  shutdown_type;     /* type of shutdown (TB_SHUTDOWN_*) */
@@ -99,6 +99,13 @@
     /* version 4+ fields: */
                                  /* populated by tboot; will be encrypted */
     uint8_t   s3_key[TB_KEY_SIZE];
+    /* version 5+ fields: */
+    uint8_t   reserved_align[3]; /* used to 4byte-align num_in_wfs */
+    uint32_t  num_in_wfs;        /* number of processors in wait-for-SIPI */
+    /* version 6+ fields: */
+    uint32_t  flags;
+    uint64_t  ap_wake_addr;      /* phys addr of kernel/VMM SIPI vector */
+    uint32_t  ap_wake_trigger;   /* kernel/VMM writes APIC ID to wake AP */
 } tboot_shared_t;
 
 #define TB_SHUTDOWN_REBOOT      0
@@ -107,6 +114,9 @@
 #define TB_SHUTDOWN_S3          3
 #define TB_SHUTDOWN_HALT        4
 
+#define TB_FLAG_AP_WAKE_SUPPORT   0x00000001  /* kernel/VMM use INIT-SIPI-SIPI
+                                                 if clear, ap_wake_* if set */
+
 /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
 #define TBOOT_SHARED_UUID    { 0x663c8dff, 0xe8b3, 0x4b82, 0xaabf, \
                                { 0x19, 0xea, 0x4d, 0x5, 0x7a, 0x8 } };
@@ -120,6 +130,7 @@
 int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
 int tboot_s3_resume(void);
 void tboot_s3_error(int error);
+int tboot_wake_ap(int apicid, unsigned long sipi_vec);
 
 #endif /* __TBOOT_H__ */

Re: [tboot-devel] [PATCH v2] x86: add a new SMP bring up way for tboot case

[Wei, G. <gan...@in...>]

Jan Beulich wrote on 2012-01-10:
>>>> On 09.01.12 at 17:01, "Wei, Gang" <gan...@in...> wrote:
>> tboot may be trying to put APs waiting in MWAIT loops before launching Xen.
>> Xen could check the new flag field in v6 tboot shared page for the
>> hint. If TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP
>> have to write the monitored memory(g_tboot_shared->ap_wake_trigger)
>> to bring APs out of MWAIT loops. The sipi vector should be written
>> in g_tboot_shared->ap_wake_addr before waking up APs.
>> 
>> Signed-off-by: Joseph Cihula <jos...@in...>
>> Signed-off-by: Shane Wang <sha...@in...>
>> Signed-off-by: Gang Wei <gan...@in...>
>> 
>> diff -r 8087674cceb9 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c	Thu Jan 05 22:42:02 2012 +0800
>> +++ b/xen/arch/x86/smpboot.c	Mon Jan 09 23:55:41 2012 +0800
>> @@ -586,7 +586,10 @@
>>      smpboot_setup_warm_reset_vector(start_eip);
>>      
>>      /* Starting actual IPI sequence... */
>> -    boot_error = wakeup_secondary_cpu(apicid, start_eip);
>> +    if ( tboot_in_measured_env() )
>> +        boot_error = tboot_wake_ap(apicid, start_eip);
>> +    else
>> +        boot_error = wakeup_secondary_cpu(apicid, start_eip);
> 
> I'm afraid this is broken now for older tboot - you want to call
> wakeup_secondary_cpu() if tboot_wake_ap() failed. All I had asked in
> the first review round was that you don't call
> tboot_wake_ap() without tboot_in_measured_env() returning true.

Oh, yes. I should not do this in mid-night. V3 patch will fix it.

Jimmy

Re: [tboot-devel] [PATCH v2] x86: add a new SMP bring up way for tboot case

[Jan B. <JBe...@su...>]

>>> On 09.01.12 at 17:01, "Wei, Gang" <gan...@in...> wrote:
> tboot may be trying to put APs waiting in MWAIT loops before launching Xen. 
> Xen could check the new flag field in v6 tboot shared page for the hint. If 
> TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP have to write the 
> monitored memory(g_tboot_shared->ap_wake_trigger) to bring APs out of MWAIT 
> loops. The sipi vector should be written in g_tboot_shared->ap_wake_addr before 
> waking up APs.
> 
> Signed-off-by: Joseph Cihula <jos...@in...>
> Signed-off-by: Shane Wang <sha...@in...>
> Signed-off-by: Gang Wei <gan...@in...>
> 
> diff -r 8087674cceb9 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c	Thu Jan 05 22:42:02 2012 +0800
> +++ b/xen/arch/x86/smpboot.c	Mon Jan 09 23:55:41 2012 +0800
> @@ -586,7 +586,10 @@
>      smpboot_setup_warm_reset_vector(start_eip);
>  
>      /* Starting actual IPI sequence... */
> -    boot_error = wakeup_secondary_cpu(apicid, start_eip);
> +    if ( tboot_in_measured_env() )
> +        boot_error = tboot_wake_ap(apicid, start_eip);
> +    else
> +        boot_error = wakeup_secondary_cpu(apicid, start_eip);

I'm afraid this is broken now for older tboot - you want to call
wakeup_secondary_cpu() if tboot_wake_ap() failed. All I had
asked in the first review round was that you don't call
tboot_wake_ap() without tboot_in_measured_env() returning
true.

Jan

>      if ( !boot_error )
>      {
> diff -r 8087674cceb9 xen/arch/x86/tboot.c
> --- a/xen/arch/x86/tboot.c	Thu Jan 05 22:42:02 2012 +0800
> +++ b/xen/arch/x86/tboot.c	Mon Jan 09 23:55:41 2012 +0800
> @@ -123,6 +123,8 @@
>      printk("  shutdown_entry: 0x%08x\n", tboot_shared->shutdown_entry);
>      printk("  tboot_base: 0x%08x\n", tboot_shared->tboot_base);
>      printk("  tboot_size: 0x%x\n", tboot_shared->tboot_size);
> +    if ( tboot_shared->version >= 6 )
> +        printk("  flags: 0x%08x\n", tboot_shared->flags);
>  
>      /* these will be needed by tboot_protect_mem_regions() and/or
>         tboot_parse_dmar_table(), so get them now */
> @@ -529,6 +531,18 @@
>      panic("Memory integrity was lost on resume (%d)\n", error);
>  }
>  
> +int tboot_wake_ap(int apicid, unsigned long sipi_vec)
> +{
> +    if ( g_tboot_shared->version >= 6 &&
> +         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
> +    {
> +        g_tboot_shared->ap_wake_addr = sipi_vec;
> +        g_tboot_shared->ap_wake_trigger = apicid;
> +        return 0;
> +    }
> +    return -1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff -r 8087674cceb9 xen/include/asm-x86/tboot.h
> --- a/xen/include/asm-x86/tboot.h	Thu Jan 05 22:42:02 2012 +0800
> +++ b/xen/include/asm-x86/tboot.h	Mon Jan 09 23:55:41 2012 +0800
> @@ -85,7 +85,7 @@
>  typedef struct __packed {
>      /* version 3+ fields: */
>      uuid_t    uuid;              /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
> -    uint32_t  version;           /* Version number; currently supports 0.4 
> */
> +    uint32_t  version;           /* Version number; currently supports 0.6 
> */
>      uint32_t  log_addr;          /* physical addr of tb_log_t log */
>      uint32_t  shutdown_entry;    /* entry point for tboot shutdown */
>      uint32_t  shutdown_type;     /* type of shutdown (TB_SHUTDOWN_*) */
> @@ -99,6 +99,13 @@
>      /* version 4+ fields: */
>                                   /* populated by tboot; will be encrypted 
> */
>      uint8_t   s3_key[TB_KEY_SIZE];
> +    /* version 5+ fields: */
> +    uint8_t   reserved_align[3]; /* used to 4byte-align num_in_wfs */
> +    uint32_t  num_in_wfs;        /* number of processors in wait-for-SIPI */
> +    /* version 6+ fields: */
> +    uint32_t  flags;
> +    uint64_t  ap_wake_addr;      /* phys addr of kernel/VMM SIPI vector */
> +    uint32_t  ap_wake_trigger;   /* kernel/VMM writes APIC ID to wake AP */
>  } tboot_shared_t;
>  
>  #define TB_SHUTDOWN_REBOOT      0
> @@ -107,6 +114,9 @@
>  #define TB_SHUTDOWN_S3          3
>  #define TB_SHUTDOWN_HALT        4
>  
> +#define TB_FLAG_AP_WAKE_SUPPORT   0x00000001  /* kernel/VMM use 
> INIT-SIPI-SIPI
> +                                                 if clear, ap_wake_* if set 
> */
> +
>  /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
>  #define TBOOT_SHARED_UUID    { 0x663c8dff, 0xe8b3, 0x4b82, 0xaabf, \
>                                 { 0x19, 0xea, 0x4d, 0x5, 0x7a, 0x8 } };
> @@ -120,6 +130,7 @@
>  int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
>  int tboot_s3_resume(void);
>  void tboot_s3_error(int error);
> +int tboot_wake_ap(int apicid, unsigned long sipi_vec);
>  
>  #endif /* __TBOOT_H__ */

[tboot-devel] [PATCH v2] x86: add a new SMP bring up way for tboot case

[Wei, G. <gan...@in...>]

tboot may be trying to put APs waiting in MWAIT loops before launching Xen. Xen could check the new flag field in v6 tboot shared page for the hint. If TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP have to write the monitored memory(g_tboot_shared->ap_wake_trigger) to bring APs out of MWAIT loops. The sipi vector should be written in g_tboot_shared->ap_wake_addr before waking up APs.

Signed-off-by: Joseph Cihula <jos...@in...>
Signed-off-by: Shane Wang <sha...@in...>
Signed-off-by: Gang Wei <gan...@in...>

diff -r 8087674cceb9 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/arch/x86/smpboot.c	Mon Jan 09 23:55:41 2012 +0800
@@ -586,7 +586,10 @@
     smpboot_setup_warm_reset_vector(start_eip);
 
     /* Starting actual IPI sequence... */
-    boot_error = wakeup_secondary_cpu(apicid, start_eip);
+    if ( tboot_in_measured_env() )
+        boot_error = tboot_wake_ap(apicid, start_eip);
+    else
+        boot_error = wakeup_secondary_cpu(apicid, start_eip);
 
     if ( !boot_error )
     {
diff -r 8087674cceb9 xen/arch/x86/tboot.c
--- a/xen/arch/x86/tboot.c	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/arch/x86/tboot.c	Mon Jan 09 23:55:41 2012 +0800
@@ -123,6 +123,8 @@
     printk("  shutdown_entry: 0x%08x\n", tboot_shared->shutdown_entry);
     printk("  tboot_base: 0x%08x\n", tboot_shared->tboot_base);
     printk("  tboot_size: 0x%x\n", tboot_shared->tboot_size);
+    if ( tboot_shared->version >= 6 )
+        printk("  flags: 0x%08x\n", tboot_shared->flags);
 
     /* these will be needed by tboot_protect_mem_regions() and/or
        tboot_parse_dmar_table(), so get them now */
@@ -529,6 +531,18 @@
     panic("Memory integrity was lost on resume (%d)\n", error);
 }
 
+int tboot_wake_ap(int apicid, unsigned long sipi_vec)
+{
+    if ( g_tboot_shared->version >= 6 &&
+         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
+    {
+        g_tboot_shared->ap_wake_addr = sipi_vec;
+        g_tboot_shared->ap_wake_trigger = apicid;
+        return 0;
+    }
+    return -1;
+}
+
 /*
  * Local variables:
  * mode: C
diff -r 8087674cceb9 xen/include/asm-x86/tboot.h
--- a/xen/include/asm-x86/tboot.h	Thu Jan 05 22:42:02 2012 +0800
+++ b/xen/include/asm-x86/tboot.h	Mon Jan 09 23:55:41 2012 +0800
@@ -85,7 +85,7 @@
 typedef struct __packed {
     /* version 3+ fields: */
     uuid_t    uuid;              /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
-    uint32_t  version;           /* Version number; currently supports 0.4 */
+    uint32_t  version;           /* Version number; currently supports 0.6 */
     uint32_t  log_addr;          /* physical addr of tb_log_t log */
     uint32_t  shutdown_entry;    /* entry point for tboot shutdown */
     uint32_t  shutdown_type;     /* type of shutdown (TB_SHUTDOWN_*) */
@@ -99,6 +99,13 @@
     /* version 4+ fields: */
                                  /* populated by tboot; will be encrypted */
     uint8_t   s3_key[TB_KEY_SIZE];
+    /* version 5+ fields: */
+    uint8_t   reserved_align[3]; /* used to 4byte-align num_in_wfs */
+    uint32_t  num_in_wfs;        /* number of processors in wait-for-SIPI */
+    /* version 6+ fields: */
+    uint32_t  flags;
+    uint64_t  ap_wake_addr;      /* phys addr of kernel/VMM SIPI vector */
+    uint32_t  ap_wake_trigger;   /* kernel/VMM writes APIC ID to wake AP */
 } tboot_shared_t;
 
 #define TB_SHUTDOWN_REBOOT      0
@@ -107,6 +114,9 @@
 #define TB_SHUTDOWN_S3          3
 #define TB_SHUTDOWN_HALT        4
 
+#define TB_FLAG_AP_WAKE_SUPPORT   0x00000001  /* kernel/VMM use INIT-SIPI-SIPI
+                                                 if clear, ap_wake_* if set */
+
 /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
 #define TBOOT_SHARED_UUID    { 0x663c8dff, 0xe8b3, 0x4b82, 0xaabf, \
                                { 0x19, 0xea, 0x4d, 0x5, 0x7a, 0x8 } };
@@ -120,6 +130,7 @@
 int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
 int tboot_s3_resume(void);
 void tboot_s3_error(int error);
+int tboot_wake_ap(int apicid, unsigned long sipi_vec);
 
 #endif /* __TBOOT_H__ */

Re: [tboot-devel] [PATCH] x86: add a new SMP bring up way for tboot case

[Wei, G. <gan...@in...>]

Jan Beulich wrote on 2012-01-05:
>>>> On 05.01.12 at 15:53, "Wei, Gang" <gan...@in...> wrote:
>> tboot may be trying to put APs waiting in MWAIT loops before launching Xen.
>> Xen could check the new flag field in v6 tboot shared page for the
>> hint. If TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP
>> have to write the monitored memory(g_tboot_shared->ap_wake_trigger)
>> to bring APs out of MWAIT loops. The sipi vector should be written
>> in g_tboot_shared->ap_wake_addr before waking up APs.
>> 
>> Signed-off-by: Joseph Cihula <jos...@in...>
>> Signed-off-by: Shane Wang <sha...@in...>
>> Signed-off-by: Gang Wei <gan...@in...>
>> 
>> diff -r cbf1ce3afd74 xen/arch/x86/smpboot.c
>> --- a/xen/arch/x86/smpboot.c	Sat Dec 31 16:18:55 2011 +0800
>> +++ b/xen/arch/x86/smpboot.c	Sat Dec 31 18:50:14 2011 +0800
>> @@ -586,7 +586,9 @@
>>      smpboot_setup_warm_reset_vector(start_eip);
>>      
>>      /* Starting actual IPI sequence... */
>> -    boot_error = wakeup_secondary_cpu(apicid, start_eip);
>> +    boot_error = tboot_wake_ap(apicid, start_eip);
> 
> As tboot.h is being included here anyway, it would seem more clean to
> me to guard the call with tboot_in_measured_env() here rather than in
> the called function.

Ok, to be updated in next version.

>> +    if ( boot_error )
>> +        boot_error = wakeup_secondary_cpu(apicid, start_eip);
>> 
>>      if ( !boot_error )
>>      {
>> diff -r cbf1ce3afd74 xen/arch/x86/tboot.c
>> --- a/xen/arch/x86/tboot.c	Sat Dec 31 16:18:55 2011 +0800
>> +++ b/xen/arch/x86/tboot.c	Sat Dec 31 18:50:14 2011 +0800
>> @@ -123,6 +123,10 @@
>>      printk("  shutdown_entry: 0x%08x\n",
>>      tboot_shared->shutdown_entry); printk("  tboot_base: 0x%08x\n",
>>      tboot_shared->tboot_base); printk("  tboot_size: 0x%x\n",
>>      tboot_shared->tboot_size);
>> +    if ( tboot_shared->version >= 5 )
>> +        printk("  num_in_wfs: %u\n", tboot_shared->num_in_wfs);
> 
> You're printing this field, but aren't making any other use of it?

This field is just used by Linux kernel. I will remove this printk.

>> +    if ( tboot_shared->version >= 6 )
>> +        printk("  flags: 0x%08x\n", tboot_shared->flags);
>> 
>>      /* these will be needed by tboot_protect_mem_regions() and/or
>>         tboot_parse_dmar_table(), so get them now */ @@ -529,6
> +533,19
>> @@
>>      panic("Memory integrity was lost on resume (%d)\n", error);  }
>> +int tboot_wake_ap(int apicid, unsigned long sipi_vec) {
>> +    if ( tboot_in_measured_env() && g_tboot_shared->version >= 6 &&
>> +         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
>> +    {
>> +        printk("waking AP %d w/ monitor write\n", apicid);
> 
> Please, no once-per-CPU printk()-s, especially if they don't depend on
> per-CPU properties.

Ok, remove it in next version.

>> +        g_tboot_shared->ap_wake_addr = sipi_vec;
>> +        g_tboot_shared->ap_wake_trigger = apicid;
>> +        return 0;
>> +    }
>> +    return -1;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C

Thanks for comments.

Jimmy

Re: [tboot-devel] [PATCH] x86: add a new SMP bring up way for tboot case

[Jan B. <JBe...@su...>]

>>> On 05.01.12 at 15:53, "Wei, Gang" <gan...@in...> wrote:
> tboot may be trying to put APs waiting in MWAIT loops before launching Xen. 
> Xen could check the new flag field in v6 tboot shared page for the hint. If 
> TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP have to write the 
> monitored memory(g_tboot_shared->ap_wake_trigger) to bring APs out of MWAIT 
> loops. The sipi vector should be written in g_tboot_shared->ap_wake_addr before 
> waking up APs.
> 
> Signed-off-by: Joseph Cihula <jos...@in...>
> Signed-off-by: Shane Wang <sha...@in...>
> Signed-off-by: Gang Wei <gan...@in...>
> 
> diff -r cbf1ce3afd74 xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c	Sat Dec 31 16:18:55 2011 +0800
> +++ b/xen/arch/x86/smpboot.c	Sat Dec 31 18:50:14 2011 +0800
> @@ -586,7 +586,9 @@
>      smpboot_setup_warm_reset_vector(start_eip);
>  
>      /* Starting actual IPI sequence... */
> -    boot_error = wakeup_secondary_cpu(apicid, start_eip);
> +    boot_error = tboot_wake_ap(apicid, start_eip);

As tboot.h is being included here anyway, it would seem more clean
to me to guard the call with tboot_in_measured_env() here rather
than in the called function.

> +    if ( boot_error )
> +        boot_error = wakeup_secondary_cpu(apicid, start_eip);
>  
>      if ( !boot_error )
>      {
> diff -r cbf1ce3afd74 xen/arch/x86/tboot.c
> --- a/xen/arch/x86/tboot.c	Sat Dec 31 16:18:55 2011 +0800
> +++ b/xen/arch/x86/tboot.c	Sat Dec 31 18:50:14 2011 +0800
> @@ -123,6 +123,10 @@
>      printk("  shutdown_entry: 0x%08x\n", tboot_shared->shutdown_entry);
>      printk("  tboot_base: 0x%08x\n", tboot_shared->tboot_base);
>      printk("  tboot_size: 0x%x\n", tboot_shared->tboot_size);
> +    if ( tboot_shared->version >= 5 )
> +        printk("  num_in_wfs: %u\n", tboot_shared->num_in_wfs);

You're printing this field, but aren't making any other use of it?

> +    if ( tboot_shared->version >= 6 )
> +        printk("  flags: 0x%08x\n", tboot_shared->flags);
>  
>      /* these will be needed by tboot_protect_mem_regions() and/or
>         tboot_parse_dmar_table(), so get them now */
> @@ -529,6 +533,19 @@
>      panic("Memory integrity was lost on resume (%d)\n", error);
>  }
>  
> +int tboot_wake_ap(int apicid, unsigned long sipi_vec)
> +{
> +    if ( tboot_in_measured_env() && g_tboot_shared->version >= 6 &&
> +         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
> +    {
> +        printk("waking AP %d w/ monitor write\n", apicid);

Please, no once-per-CPU printk()-s, especially if they don't depend on
per-CPU properties.

> +        g_tboot_shared->ap_wake_addr = sipi_vec;
> +        g_tboot_shared->ap_wake_trigger = apicid;
> +        return 0;
> +    }
> +    return -1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff -r cbf1ce3afd74 xen/include/asm-x86/tboot.h
> --- a/xen/include/asm-x86/tboot.h	Sat Dec 31 16:18:55 2011 +0800
> +++ b/xen/include/asm-x86/tboot.h	Sat Dec 31 18:50:14 2011 +0800
> @@ -85,7 +85,7 @@
>  typedef struct __packed {
>      /* version 3+ fields: */
>      uuid_t    uuid;              /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
> -    uint32_t  version;           /* Version number; currently supports 0.4 
> */
> +    uint32_t  version;           /* Version number; currently supports 0.6 
> */
>      uint32_t  log_addr;          /* physical addr of tb_log_t log */
>      uint32_t  shutdown_entry;    /* entry point for tboot shutdown */
>      uint32_t  shutdown_type;     /* type of shutdown (TB_SHUTDOWN_*) */
> @@ -99,6 +99,13 @@
>      /* version 4+ fields: */
>                                   /* populated by tboot; will be encrypted 
> */
>      uint8_t   s3_key[TB_KEY_SIZE];
> +    /* version 5+ fields: */
> +    uint8_t   reserved_align[3]; /* used to 4byte-align num_in_wfs */
> +    uint32_t  num_in_wfs;        /* number of processors in wait-for-SIPI */
> +    /* version 6+ fields: */
> +    uint32_t  flags;
> +    uint64_t  ap_wake_addr;      /* phys addr of kernel/VMM SIPI vector */
> +    uint32_t  ap_wake_trigger;   /* kernel/VMM writes APIC ID to wake AP */
>  } tboot_shared_t;
>  
>  #define TB_SHUTDOWN_REBOOT      0
> @@ -107,6 +114,9 @@
>  #define TB_SHUTDOWN_S3          3
>  #define TB_SHUTDOWN_HALT        4
>  
> +#define TB_FLAG_AP_WAKE_SUPPORT   0x00000001  /* kernel/VMM use 
> INIT-SIPI-SIPI
> +                                                 if clear, ap_wake_* if set 
> */
> +
>  /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
>  #define TBOOT_SHARED_UUID    { 0x663c8dff, 0xe8b3, 0x4b82, 0xaabf, \
>                                 { 0x19, 0xea, 0x4d, 0x5, 0x7a, 0x8 } };
> @@ -120,6 +130,7 @@
>  int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
>  int tboot_s3_resume(void);
>  void tboot_s3_error(int error);
> +int tboot_wake_ap(int apicid, unsigned long sipi_vec);
>  
>  #endif /* __TBOOT_H__ */

[tboot-devel] [PATCH] x86: add a new SMP bring up way for tboot case

[Wei, G. <gan...@in...>]

tboot may be trying to put APs waiting in MWAIT loops before launching Xen. Xen could check the new flag field in v6 tboot shared page for the hint. If TB_FLAG_AP_WAKE_SUPPORT bit in flag field is set, Xen BSP have to write the monitored memory(g_tboot_shared->ap_wake_trigger) to bring APs out of MWAIT loops. The sipi vector should be written in g_tboot_shared->ap_wake_addr before waking up APs.

Signed-off-by: Joseph Cihula <jos...@in...>
Signed-off-by: Shane Wang <sha...@in...>
Signed-off-by: Gang Wei <gan...@in...>

diff -r cbf1ce3afd74 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Sat Dec 31 16:18:55 2011 +0800
+++ b/xen/arch/x86/smpboot.c	Sat Dec 31 18:50:14 2011 +0800
@@ -586,7 +586,9 @@
     smpboot_setup_warm_reset_vector(start_eip);
 
     /* Starting actual IPI sequence... */
-    boot_error = wakeup_secondary_cpu(apicid, start_eip);
+    boot_error = tboot_wake_ap(apicid, start_eip);
+    if ( boot_error )
+        boot_error = wakeup_secondary_cpu(apicid, start_eip);
 
     if ( !boot_error )
     {
diff -r cbf1ce3afd74 xen/arch/x86/tboot.c
--- a/xen/arch/x86/tboot.c	Sat Dec 31 16:18:55 2011 +0800
+++ b/xen/arch/x86/tboot.c	Sat Dec 31 18:50:14 2011 +0800
@@ -123,6 +123,10 @@
     printk("  shutdown_entry: 0x%08x\n", tboot_shared->shutdown_entry);
     printk("  tboot_base: 0x%08x\n", tboot_shared->tboot_base);
     printk("  tboot_size: 0x%x\n", tboot_shared->tboot_size);
+    if ( tboot_shared->version >= 5 )
+        printk("  num_in_wfs: %u\n", tboot_shared->num_in_wfs);
+    if ( tboot_shared->version >= 6 )
+        printk("  flags: 0x%08x\n", tboot_shared->flags);
 
     /* these will be needed by tboot_protect_mem_regions() and/or
        tboot_parse_dmar_table(), so get them now */
@@ -529,6 +533,19 @@
     panic("Memory integrity was lost on resume (%d)\n", error);
 }
 
+int tboot_wake_ap(int apicid, unsigned long sipi_vec)
+{
+    if ( tboot_in_measured_env() && g_tboot_shared->version >= 6 &&
+         (g_tboot_shared->flags & TB_FLAG_AP_WAKE_SUPPORT) )
+    {
+        printk("waking AP %d w/ monitor write\n", apicid);
+        g_tboot_shared->ap_wake_addr = sipi_vec;
+        g_tboot_shared->ap_wake_trigger = apicid;
+        return 0;
+    }
+    return -1;
+}
+
 /*
  * Local variables:
  * mode: C
diff -r cbf1ce3afd74 xen/include/asm-x86/tboot.h
--- a/xen/include/asm-x86/tboot.h	Sat Dec 31 16:18:55 2011 +0800
+++ b/xen/include/asm-x86/tboot.h	Sat Dec 31 18:50:14 2011 +0800
@@ -85,7 +85,7 @@
 typedef struct __packed {
     /* version 3+ fields: */
     uuid_t    uuid;              /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
-    uint32_t  version;           /* Version number; currently supports 0.4 */
+    uint32_t  version;           /* Version number; currently supports 0.6 */
     uint32_t  log_addr;          /* physical addr of tb_log_t log */
     uint32_t  shutdown_entry;    /* entry point for tboot shutdown */
     uint32_t  shutdown_type;     /* type of shutdown (TB_SHUTDOWN_*) */
@@ -99,6 +99,13 @@
     /* version 4+ fields: */
                                  /* populated by tboot; will be encrypted */
     uint8_t   s3_key[TB_KEY_SIZE];
+    /* version 5+ fields: */
+    uint8_t   reserved_align[3]; /* used to 4byte-align num_in_wfs */
+    uint32_t  num_in_wfs;        /* number of processors in wait-for-SIPI */
+    /* version 6+ fields: */
+    uint32_t  flags;
+    uint64_t  ap_wake_addr;      /* phys addr of kernel/VMM SIPI vector */
+    uint32_t  ap_wake_trigger;   /* kernel/VMM writes APIC ID to wake AP */
 } tboot_shared_t;
 
 #define TB_SHUTDOWN_REBOOT      0
@@ -107,6 +114,9 @@
 #define TB_SHUTDOWN_S3          3
 #define TB_SHUTDOWN_HALT        4
 
+#define TB_FLAG_AP_WAKE_SUPPORT   0x00000001  /* kernel/VMM use INIT-SIPI-SIPI
+                                                 if clear, ap_wake_* if set */
+
 /* {663C8DFF-E8B3-4b82-AABF-19EA4D057A08} */
 #define TBOOT_SHARED_UUID    { 0x663c8dff, 0xe8b3, 0x4b82, 0xaabf, \
                                { 0x19, 0xea, 0x4d, 0x5, 0x7a, 0x8 } };
@@ -120,6 +130,7 @@
 int tboot_parse_dmar_table(acpi_table_handler dmar_handler);
 int tboot_s3_resume(void);
 void tboot_s3_error(int error);
+int tboot_wake_ap(int apicid, unsigned long sipi_vec);
 
 #endif /* __TBOOT_H__ */

Re: [tboot-devel] [PATCH 1/7] x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling tboot_sleep.

[Konrad R. W. <kon...@or...>]

On Fri, Dec 16, 2011 at 05:38:13PM -0500, Konrad Rzeszutek Wilk wrote:
> From: Tang Liang <lia...@or...>

Hey Joseph,

I remember you acked the initial rfc patches I had posted.
But I was wondering if these ones are to your liking (I think they are -
they aren't that much different, but I didn't want to blindly sticking
'Acked-by' as they did change a bit).

Thanks!
> 
> The ACPI suspend path makes a call to tboot_sleep right before
> it writes the PM1A, PM1B values. We replace the direct call to
> tboot via an registration callback similar to __acpi_register_gsi.
> 
> CC: Thomas Gleixner <tg...@li...>
> CC: "H. Peter Anvin" <hp...@zy...>
> CC: x8...@ke...
> CC: Len Brown <len...@in...>
> CC: Joseph Cihula <jos...@in...>
> CC: Shane Wang <sha...@in...>
> CC: xen...@li...
> CC: lin...@li...
> CC: tbo...@li...
> CC: lin...@vg...
> [v1: Added __attribute__ ((unused))]
> [v2: Introduced a wrapper instead of changing tboot_sleep return values]
> [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare]
> Signed-off-by: Tang Liang <lia...@or...>
> [v1: Fix compile issues on IA64 and PPC64]
> [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep properly]
> Signed-off-by: Konrad Rzeszutek Wilk <kon...@or...>
> ---
>  arch/x86/kernel/tboot.c       |    8 ++++++++
>  drivers/acpi/acpica/hwsleep.c |   10 +++++++---
>  drivers/acpi/osl.c            |   24 ++++++++++++++++++++++++
>  include/acpi/acexcep.h        |    1 +
>  include/linux/acpi.h          |   10 ++++++++++
>  include/linux/tboot.h         |    1 -
>  6 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index e2410e2..1a4ab7d 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -297,6 +297,12 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
>  
>  	tboot_shutdown(acpi_shutdown_map[sleep_state]);
>  }
> +static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
> +			       u32 pm1b_control)
> +{
> +	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
> +	return 0;
> +}
>  
>  static atomic_t ap_wfs_count;
>  
> @@ -345,6 +351,8 @@ static __init int tboot_late_init(void)
>  
>  	atomic_set(&ap_wfs_count, 0);
>  	register_hotcpu_notifier(&tboot_cpu_notifier);
> +
> +	acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
>  	return 0;
>  }
>  
> diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> index d52da30..992359a 100644
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -43,9 +43,9 @@
>   */
>  
>  #include <acpi/acpi.h>
> +#include <linux/acpi.h>
>  #include "accommon.h"
>  #include "actables.h"
> -#include <linux/tboot.h>
>  #include <linux/module.h>
>  
>  #define _COMPONENT          ACPI_HARDWARE
> @@ -344,8 +344,12 @@ acpi_status asmlinkage acpi_enter_sleep_state(u8 sleep_state)
>  
>  	ACPI_FLUSH_CPU_CACHE();
>  
> -	tboot_sleep(sleep_state, pm1a_control, pm1b_control);
> -
> +	status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
> +				       pm1b_control);
> +	if (ACPI_SKIP(status))
> +		return_ACPI_STATUS(AE_OK);
> +	if (ACPI_FAILURE(status))
> +		return_ACPI_STATUS(status);
>  	/* Write #2: Write both SLP_TYP + SLP_EN */
>  
>  	status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index f31c5c5..f3aae4b 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -76,6 +76,9 @@ EXPORT_SYMBOL(acpi_in_debugger);
>  extern char line_buf[80];
>  #endif				/*ENABLE_DEBUGGER */
>  
> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
> +				      u32 pm1b_ctrl);
> +
>  static acpi_osd_handler acpi_irq_handler;
>  static void *acpi_irq_context;
>  static struct workqueue_struct *kacpid_wq;
> @@ -1659,3 +1662,24 @@ acpi_status acpi_os_terminate(void)
>  
>  	return AE_OK;
>  }
> +
> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
> +				  u32 pm1b_control)
> +{
> +	int rc = 0;
> +	if (__acpi_os_prepare_sleep)
> +		rc = __acpi_os_prepare_sleep(sleep_state,
> +					     pm1a_control, pm1b_control);
> +	if (rc < 0)
> +		return AE_ERROR;
> +	else if (rc > 0)
> +		return AE_CTRL_SKIP;
> +
> +	return AE_OK;
> +}
> +
> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> +			       u32 pm1a_ctrl, u32 pm1b_ctrl))
> +{
> +	__acpi_os_prepare_sleep = func;
> +}
> diff --git a/include/acpi/acexcep.h b/include/acpi/acexcep.h
> index 5b6c391..fa0d22c 100644
> --- a/include/acpi/acexcep.h
> +++ b/include/acpi/acexcep.h
> @@ -57,6 +57,7 @@
>  #define ACPI_SUCCESS(a)                 (!(a))
>  #define ACPI_FAILURE(a)                 (a)
>  
> +#define ACPI_SKIP(a)                    (a == AE_CTRL_SKIP)
>  #define AE_OK                           (acpi_status) 0x0000
>  
>  /*
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6001b4da..fccd017 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -359,4 +359,14 @@ static inline int suspend_nvs_register(unsigned long a, unsigned long b)
>  }
>  #endif
>  
> +#ifdef CONFIG_ACPI
> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> +			       u32 pm1a_ctrl,  u32 pm1b_ctrl));
> +
> +acpi_status acpi_os_prepare_sleep(u8 sleep_state,
> +				  u32 pm1a_control, u32 pm1b_control);
> +#else
> +#define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
> +#endif
> +
>  #endif	/*_LINUX_ACPI_H*/
> diff --git a/include/linux/tboot.h b/include/linux/tboot.h
> index 1dba6ee..c75128b 100644
> --- a/include/linux/tboot.h
> +++ b/include/linux/tboot.h
> @@ -143,7 +143,6 @@ static inline int tboot_enabled(void)
>  
>  extern void tboot_probe(void);
>  extern void tboot_shutdown(u32 shutdown_type);
> -extern void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control);
>  extern struct acpi_table_header *tboot_get_dmar_table(
>  				      struct acpi_table_header *dmar_tbl);
>  extern int tboot_force_iommu(void);
> -- 
> 1.7.7.4

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

[Jan B. <jbe...@su...>]

>>> "Wei, Gang"  12/27/11 11:07 AM >>>
>Second way, I tested it. The minimum value I tested is udelay(1). So I think udelay(10) should give enough margin.

Ah, that's what I was after. Worth stating in the code comment perhaps.

Jan

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

[Wei, G. <gan...@in...>]

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

[tboot-devel] two csets(288, 289) in bughost tree got removed and re-checked in

[Wei, G. <gan...@in...>]

Please clone a new local tree to avoid accumulating two more obsolete csets in your old local tree.

Sorry for inconvenience brought by this.

Jimmy

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

[Tim D. <ti...@xe...>]

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.

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

[Wei, G. <gan...@in...>]

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

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

[Tim D. <ti...@xe...>]

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.

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

[Wei, G. <gan...@in...>]

Jan Beulich wrote on 2011-12-23:
>>>> On 23.12.11 at 04:14, "Wei, Gang" <gan...@in...> 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 <gan...@in...>
>> Acked-by: Keir Fraser <ke...@xe...>
>> Acked-by: Tim Deegan <tim...@ci...>
>> 
>> 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

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

[Jan B. <JBe...@su...>]

>>> On 23.12.11 at 04:14, "Wei, Gang" <gan...@in...> 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 <gan...@in...>
> Acked-by: Keir Fraser <ke...@xe...>
> Acked-by: Tim Deegan <tim...@ci...>
> 
> 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 ?

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

[Wei, G. <gan...@in...>]

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 <gan...@in...>
Acked-by: Keir Fraser <ke...@xe...>
Acked-by: Tim Deegan <tim...@ci...>

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 ?

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

[Wei, G. <gan...@in...>]

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

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

[Tim D. <ti...@xe...>]

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 ?

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

[Wei, G. <gan...@in...>]

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 ?

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

[Tim D. <ti...@xe...>]

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...@li...
> > Cc: tbo...@li...; 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" <gan...@in...> 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

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

[Wei, G. <gan...@in...>]

(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 <gan...@in...>
Acked-by: Keir Fraser <ke...@xe...>

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 ?