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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.01.12 at 15:53, "Wei, Gang" <gang.wei@xxxxxxxxx> 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 <joseph.cihula@xxxxxxxxx>
> Signed-off-by: Shane Wang <shane.wang@xxxxxxxxx>
> Signed-off-by: Gang Wei <gang.wei@xxxxxxxxx>
> 
> 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__ */



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