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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> 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.

> @@ -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)?

> @@ -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.

Jan


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