Re: [Xen-devel] [RFC Patch v4 4/8] hvmloader: boot cpu through broadcast

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Apr 18, 2018 at 02:38:48AM -0600, Jan Beulich wrote:
>>>> On 06.12.17 at 08:50, <chao.gao@xxxxxxxxx> wrote:
>> Intel SDM Extended XAPIC (X2APIC) -> "Initialization by System Software"
>> has the following description:
>> 
>> "The ACPI interfaces for the x2APIC are described in Section 5.2, “ACPI 
>> System
>> Description Tables,” of the Advanced Configuration and Power Interface
>> Specification, Revision 4.0a (http://www.acpi.info/spec.htm). The default
>> behavior for BIOS is to pass the control to the operating system with the
>> local x2APICs in xAPIC mode if all APIC IDs reported by CPUID.0BH:EDX are 
>> less
>> than 255, and in x2APIC mode if there are any logical processor reporting an
>> APIC ID of 255 or greater."
>
>With this you mean ...
>
>> In this patch, hvmloader enables x2apic mode for all vcpus if there are cpus
>> with APIC ID > 255. To wake up processors whose APIC ID is greater than 255,
>
>">= 255" and "greater than 254" here respectively. Please be precise.

Will do.

>
>> the SIPI is broadcasted to all APs. It is the way how Seabios wakes up APs.
>> APs may compete for the stack, thus a lock is introduced to protect the 
>> stack.
>
>
>
>> --- a/tools/firmware/hvmloader/smp.c
>> +++ b/tools/firmware/hvmloader/smp.c
>> @@ -26,7 +26,9 @@
>>  #define AP_BOOT_EIP 0x1000
>>  extern char ap_boot_start[], ap_boot_end[];
>>  
>> -static int ap_callin, ap_cpuid;
>> +static int ap_callin;
>> +static int enable_x2apic;
>> +static bool lock = 1;
>>  
>>  asm (
>>      "    .text                       \n"
>> @@ -47,7 +49,15 @@ asm (
>>      "    mov   %eax,%ds              \n"
>>      "    mov   %eax,%es              \n"
>>      "    mov   %eax,%ss              \n"
>> -    "    movl  $stack_top,%esp       \n"
>> +    "3:  movb  $1, %bl               \n"
>> +    "    mov   $lock,%edx            \n"
>> +    "    movzbl %bl,%eax             \n"
>> +    "    xchg  %al, (%edx)           \n"
>> +    "    test  %al,%al               \n"
>> +    "    je    2f                    \n"
>> +    "    pause                       \n"
>> +    "    jmp   3b                    \n"
>> +    "2:  movl  $stack_top,%esp       \n"
>
>Please be consistent with suffixes: Either add them only when really needed
>(preferred) or add them uniformly everywhere (when permitted of course).
>I also don't understand why you need to use %bl here at all.

will remove %bl stuff.

>
>> @@ -68,14 +78,34 @@ asm (
>>      "    .text                       \n"
>>      );
>>  
>> +unsigned int ap_cpuid(void)
>
>static
>
>> +{
>> +    if ( !(rdmsr(MSR_IA32_APICBASE) & MSR_IA32_APICBASE_EXTD) )
>> +    {
>> +        uint32_t eax, ebx, ecx, edx;
>> +
>> +        cpuid(1, &eax, &ebx, &ecx, &edx);
>> +        return ebx >> 24;
>> +    }
>> +    else
>
>pointless "else"
>
>> +        return rdmsr(MSR_IA32_APICBASE_MSR + (APIC_ID >> 4));
>> +}
>> +
>>  void ap_start(void); /* non-static avoids unused-function compiler warning 
>> */
>>  /*static*/ void ap_start(void)
>>  {
>> -    printf(" - CPU%d ... ", ap_cpuid);
>> +    printf(" - CPU%d ... ", ap_cpuid());
>>      cacheattr_init();
>>      printf("done.\n");
>>      wmb();
>> -    ap_callin = 1;
>> +    ap_callin++;
>> +
>> +    if ( enable_x2apic )
>> +        wrmsr(MSR_IA32_APICBASE, rdmsr(MSR_IA32_APICBASE) |
>> +                                 MSR_IA32_APICBASE_EXTD);
>> +
>> +    /* Release the lock */
>> +    asm volatile ( "xchgb %1, %b0" : : "m" (lock), "r" (0) : "memory" );
>>  }
>
>How can you release the lock here - you've not switched off the stack, and
>you're about to actually use it (for returning from the function)?

    "2:  movl  $stack_top,%esp       \n"
    "    movl  %esp,%ebp             \n"
    "    call  ap_start              \n"
    "1:  hlt                         \n"
    "    jmp  1b                     \n"

Yes. I think it would be right to release the lock following the
"call ap_start" here.
>
>> @@ -125,9 +169,15 @@ void smp_initialise(void)
>>      memcpy((void *)AP_BOOT_EIP, ap_boot_start, ap_boot_end - ap_boot_start);
>>  
>>      printf("Multiprocessor initialisation:\n");
>> +    if ( nr_cpus > MADT_MAX_LOCAL_APIC )
>> +        enable_x2apic = 1;
>> +
>>      ap_start();
>> -    for ( i = 1; i < nr_cpus; i++ )
>> -        boot_cpu(i);
>> +    if ( nr_cpus > MADT_MAX_LOCAL_APIC )
>
>Where does MADT_MAX_LOCAL_APIC come from? I can't find it anywhere, and
>hence can't judge (along the lines of my remark on the description) whether 
>this
>is a correct comparison.

It is defined in patch 5/8. I know it is a mistake.

With regard to this point, I only boot up vCPU-s via broadcasting
INIT-STARTUP signal when the number of vCPU-s is greater than a
given value, otherwise the previous way will be used.

In general, before all APs are up, BSP couldn't know each AP's APIC ID
(currently, we know that because APIC ID can be inferred from vcpu_id) and
whether there is a vCPU with APIC_ID >= 255. I plan to discard the
old way and always use broadcast to boot up APs. And we don't need
MADT_MAX_LOCAL_APIC here. Instead, a global variable can be set if
any vCPU-s finds its APIC ID is greater than 254. Then all CPUs can
switch to x2apic mode if need be. Do you think it is reasonable?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel