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

[Chao Gao]

On Wed, Apr 18, 2018 at 02:38:48AM -0600, Jan Beulich wrote:
 On 06.12.17 at 08:50,  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"
>> +"je2f\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, , , , );
>> +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 

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

[Jan Beulich]

>>> On 06.12.17 at 08:50,  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"
> +"je2f\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, , , , );
> +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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 08:33:23PM +0800, Chao Gao wrote:
> On Mon, Feb 26, 2018 at 01:28:07AM -0700, Jan Beulich wrote:
>  On 24.02.18 at 06:49,  wrote:
> >> On Fri, Feb 23, 2018 at 04:42:10PM +, Roger Pau Monné wrote:
> >>>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.
> >>>
> >>>Hm, how are we going to deal with this on PVH? hvmloader doesn't run
> >>>for PVH guests, hence it seems like switching to x2APIC mode should be
> >>>done somewhere else that shared between HVM and PVH.
> >>>
> >>>Maybe the hypercall that sets the number of vCPUs should change the
> >>>APIC mode?
> >> 
> >> Yes. A flag can be passed when setting the maximum number of vCPUs. Xen
> >> will switch all local APICs to x2APIC mode or xAPIC mode according to
> >> the flag.
> >
> >A flag? Where? Why isn't 257+ vCPU-s on its own sufficient to tell
> >that the mode needs to be switched?
> 
> In struct xen_domctl_max_vcpus, a flag, like SWITCH_TO_X2APIC_MODE, can
> be used to instruct Xen to initialize vlapic and do this switch.
> 
> Yes, it is another option: Xen can do this switch when need. This
> solution leads to smaller code change compared with introducing a new
> flag when setting the maximum number of vCPUs.

Since APIC ID is currently hardcoded in guest_cpuid as vcpu_id * 2,
IMO Xen should switch to x2APIC mode when it detects that vCPUs > 128,
like Jan has suggest. Then you won't need to modify hvmloader at all,
and the same would work for PVH I assume?

Thanks, Roger.

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

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

[Chao Gao]

On Mon, Feb 26, 2018 at 01:28:07AM -0700, Jan Beulich wrote:
 On 24.02.18 at 06:49,  wrote:
>> On Fri, Feb 23, 2018 at 04:42:10PM +, Roger Pau Monné wrote:
>>>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.
>>>
>>>Hm, how are we going to deal with this on PVH? hvmloader doesn't run
>>>for PVH guests, hence it seems like switching to x2APIC mode should be
>>>done somewhere else that shared between HVM and PVH.
>>>
>>>Maybe the hypercall that sets the number of vCPUs should change the
>>>APIC mode?
>> 
>> Yes. A flag can be passed when setting the maximum number of vCPUs. Xen
>> will switch all local APICs to x2APIC mode or xAPIC mode according to
>> the flag.
>
>A flag? Where? Why isn't 257+ vCPU-s on its own sufficient to tell
>that the mode needs to be switched?

In struct xen_domctl_max_vcpus, a flag, like SWITCH_TO_X2APIC_MODE, can
be used to instruct Xen to initialize vlapic and do this switch.

Yes, it is another option: Xen can do this switch when need. This
solution leads to smaller code change compared with introducing a new
flag when setting the maximum number of vCPUs.

Thanks
Chao

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

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

[Jan Beulich]

>>> On 24.02.18 at 06:49,  wrote:
> On Fri, Feb 23, 2018 at 04:42:10PM +, Roger Pau Monné wrote:
>>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.
>>
>>Hm, how are we going to deal with this on PVH? hvmloader doesn't run
>>for PVH guests, hence it seems like switching to x2APIC mode should be
>>done somewhere else that shared between HVM and PVH.
>>
>>Maybe the hypercall that sets the number of vCPUs should change the
>>APIC mode?
> 
> Yes. A flag can be passed when setting the maximum number of vCPUs. Xen
> will switch all local APICs to x2APIC mode or xAPIC mode according to
> the flag.

A flag? Where? Why isn't 256+ vCPU-s on its own sufficient to tell
that the mode needs to be switched?

Jan

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

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

[Chao Gao]

On Fri, Feb 23, 2018 at 04:42:10PM +, Roger Pau Monné wrote:
>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.
>
>Hm, how are we going to deal with this on PVH? hvmloader doesn't run
>for PVH guests, hence it seems like switching to x2APIC mode should be
>done somewhere else that shared between HVM and PVH.
>
>Maybe the hypercall that sets the number of vCPUs should change the
>APIC mode?

Yes. A flag can be passed when setting the maximum number of vCPUs. Xen
will switch all local APICs to x2APIC mode or xAPIC mode according to
the flag.

Thanks
Chao

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

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

[Roger Pau Monné]

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.

Hm, how are we going to deal with this on PVH? hvmloader doesn't run
for PVH guests, hence it seems like switching to x2APIC mode should be
done somewhere else that shared between HVM and PVH.

Maybe the hypercall that sets the number of vCPUs should change the
APIC mode?

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 22.02.18 at 19:44,  wrote:
> On Wed, Dec 06, 2017 at 03:50:10PM +0800, Chao Gao wrote:
>> +"mov   $lock,%edx\n"
> 
> Not really an expert in x86 asm -- shouldn't this be lea instead?

No, when MOV can be used it's preferable as having a 1 byte shorter
encoding. There are a few cases where MOV may not be appropriate,
but I don't think the one here is.

Jan


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

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

[Wei Liu]

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 
> ---
> 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_APICBASE0x1b
> +#define MSR_IA32_APICBASE_EXTD   (1<<10)
> +#define MSR_IA32_APICBASE_MSR0x800
> +
>  #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"
> +"je2f\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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel