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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2022 12:59, Andrew Cooper wrote:
> --- a/tools/firmware/hvmloader/smp.c
> +++ b/tools/firmware/hvmloader/smp.c
> @@ -35,9 +35,9 @@ asm (
>      "    mov   %cs,%ax               \n"
>      "    mov   %ax,%ds               \n"
>      "    lgdt  gdt_desr-ap_boot_start\n"
> -    "    xor   %ax, %ax              \n"
> -    "    inc   %ax                   \n"
> -    "    lmsw  %ax                   \n"
> +    "    mov   %cr0, %eax            \n"
> +    "    or    $1, %al               \n"
> +    "    mov   %eax, %cr0            \n"

Hmm, yes, read-modify-write should probably have been used from
the beginning, irrespective of using 286 or 386 insns.

> @@ -68,14 +66,37 @@ asm (
>      "    .text                       \n"
>      );
>  
> -void ap_start(void); /* non-static avoids unused-function compiler warning */
> -/*static*/ void ap_start(void)
> +static void __attribute__((used)) ap_start(void)
>  {
> -    printf(" - CPU%d ... ", ap_cpuid);
> +    unsigned int cpu = ap_cpuid;
> +
> +    printf(" - CPU%d ... ", cpu);
>      cacheattr_init();
>      printf("done.\n");
> -    wmb();

Is there a reason you remove this barrier but not the one in boot_cpu()?

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

> +        "    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).

> +        ".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?

Jan