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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.18 at 13:20, <chao.gao@xxxxxxxxx> wrote:
> On Wed, Apr 18, 2018 at 02:38:48AM -0600, Jan Beulich wrote:
>>>>> On 06.12.17 at 08:50, <chao.gao@xxxxxxxxx> wrote:
>>>  /*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.

Strictly speaking even that's too early, as an NMI might arrive at the HLT
(before actually executing it). Otoh that's going to be deadly anyway, so
perhaps with a suitable comment that might be accptable.

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

At the first glance - why not.

Jan



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