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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Dec 06, 2017 at 03:50:10PM +0800, Chao Gao 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."
> 
> 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,
> 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.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v4:
>  - new
> ---
>  tools/firmware/hvmloader/apic_regs.h |  4 +++
>  tools/firmware/hvmloader/smp.c       | 64 
> ++++++++++++++++++++++++++++++++----
>  2 files changed, 61 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/apic_regs.h 
> b/tools/firmware/hvmloader/apic_regs.h
> index f737b47..bc39ecd 100644
> --- a/tools/firmware/hvmloader/apic_regs.h
> +++ b/tools/firmware/hvmloader/apic_regs.h
> @@ -105,6 +105,10 @@
>  #define     APIC_TDR_DIV_64          0x9
>  #define     APIC_TDR_DIV_128         0xA
>  
> +#define MSR_IA32_APICBASE            0x1b
> +#define MSR_IA32_APICBASE_EXTD       (1<<10)
> +#define MSR_IA32_APICBASE_MSR        0x800
> +
>  #endif
>  
>  /*
> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
> index 082b17f..e3dade4 100644
> --- 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"

I would rather you label this 1 and increment the number as you go.

> +    "    mov   $lock,%edx            \n"

Not really an expert in x86 asm -- shouldn't this be lea instead?

> +    "    movzbl %bl,%eax             \n"
> +    "    xchg  %al, (%edx)           \n"

Why not just use %bl directly?

> +    "    test  %al,%al               \n"
> +    "    je    2f                    \n"
> +    "    pause                       \n"
> +    "    jmp   3b                    \n"
> +    "2:  movl  $stack_top,%esp       \n"
>      "    movl  %esp,%ebp             \n"
>      "    call  ap_start              \n"
>      "1:  hlt                         \n"
> @@ -68,14 +78,34 @@ asm (
>      "    .text                       \n"
>      );
>  
[...]
>  
> +static void boot_cpu_broadcast_x2apic(unsigned int nr_cpus)
> +{
> +    wrmsr(MSR_IA32_APICBASE_MSR + (APIC_ICR >> 4),
> +          APIC_DEST_ALLBUT | APIC_DM_INIT);
> +
> +    wrmsr(MSR_IA32_APICBASE_MSR + (APIC_ICR >> 4),
> +          APIC_DEST_ALLBUT | APIC_DM_STARTUP | (AP_BOOT_EIP >> 12));
> +
> +    while ( ap_callin != nr_cpus )
> +        cpu_relax();
> +
> +    wrmsr(MSR_IA32_APICBASE_MSR + (APIC_ICR >> 4),
> +          APIC_DEST_ALLBUT | APIC_DM_INIT);
> +}
> +
>  void smp_initialise(void)
>  {
>      unsigned int i, nr_cpus = hvm_info->nr_vcpus;
> @@ -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 )

Use enable_x2apic here instead.

Wei.

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