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

["Wei, Gang" <gang.wei@xxxxxxxxx>]

Jan Beulich wrote on 2012-01-05:
>>>> 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.

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



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