Re: [PATCH 1/4] x86/hvmloader: SMP improvements

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/08/2022 3:21 pm, Jan Beulich wrote:
> On 24.08.2022 12:59, Andrew Cooper wrote:
>
>> -    ap_callin = 1;
>> +
>> +    /*
>> +     * Call in to the BSP.  For APs, take ourselves offline.
>> +     *
>> +     * We must not use the stack after calling in to the BSP.
>> +     */
>> +    asm volatile (
>> +        "    movb $1, ap_callin          \n"
>> +
>> +        "    test %[icr2], %[icr2]       \n"
>> +        "    jz   .Lbsp                  \n"
>
> Are we intending to guarantee going forward that the BSP always has
> APIC ID zero?

It's currently true, and I doubt that will change, but I prefer the
suggestion to not call this at all on the BSP.

>
>> +        "    movl %[icr2], %[ICR2]       \n"
>> +        "    movl %[init], %[ICR1]       \n"
>> +        "1:  hlt                         \n"
>> +        "    jmp  1b                     \n"
>
> The use of the function for the BSP is questionable anyway. What is
> really needed is the call to cacheattr_init(). I'm inclined to
> suggest to move to something like
>
> void smp_initialise(void)
> {
>    unsigned int i, nr_cpus = hvm_info->nr_vcpus;
>
>    cacheattr_init();
>
>    if ( nr_cpus <= 1 )
>        return;
>
>    memcpy((void *)AP_BOOT_EIP, ap_boot_start, ap_boot_end -
> ap_boot_start);
>
>    printf("Multiprocessor initialisation:\n");
>    for ( i = 1; i < nr_cpus; i++ )
>        boot_cpu(i);
> }
>
> thus eliminating bogus output when there's just one vCPU.
> Then the function here can become noreturn (which I was about to suggest
> until spotting that for the BSP the function actually does return).

Dropping the printk() isn't nice, because you'll then get unqualified
information from cacheattr_init().

I'll see if I can rearrange this a bit more nicely.

>
>> +        ".Lbsp:                          \n"
>> +        :
>> +        : [icr2] "r" (SET_APIC_DEST_FIELD(LAPIC_ID(cpu))),
>> +          [init] "i" (APIC_DM_INIT),
>> +          [ICR1] "m" (*(uint32_t *)(LAPIC_BASE_ADDRESS + APIC_ICR)),
>> +          [ICR2] "m" (*(uint32_t *)(LAPIC_BASE_ADDRESS + APIC_ICR2))
>> +        : "memory" );
>
> Can't you use APIC_DEST_SELF now, avoiding the need to fiddle
> with ICR2?

No.  Fixed is the only message type which can use self or all-inc-self. 
All others are only permitted to use the all-excluding-self.

This makes sense as a consequence of likely shortcuts taking when
integrating the LAPIC into the core.  Either way, it's documented
behaviour now, however inconvenient this is for this case.

~Andrew