Re: [PATCH v2 4/8] tools/hvmloader: Wake APs with hypercalls and not with INIT+SIPI+SIPI

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 08/05/2024 20:13, Andrew Cooper wrote:
> On 08/05/2024 1:39 pm, Alejandro Vallejo wrote:
>> Removes a needless assembly entry point and simplifies the codebase by 
>> allowing
>> hvmloader to wake APs it doesn't know the APIC ID of.
>>
>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> 
> This is basically independent of the rest of the series, and it would be
> good to pull it in separately.  A few notes.
> 
> The commit message ought to note that this has a side effect of removing
> an LMSW instruction which has fastpath at all on AMD CPUs, and requires
> full emulation, and it gets rid of 13 vLAPIC emulations when 3
> hypercalls would do.
> 
> The point being that this is borderline backport material, although it
> does depend on the 32 vCPU bugfix.
> 
>> ---
>> v2:
>>   * New patch. Replaces adding cpu policy to hvmloader in v1.
>> ---
>>  tools/firmware/hvmloader/smp.c | 111 +++++++++++++--------------------
>>  1 file changed, 44 insertions(+), 67 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/smp.c b/tools/firmware/hvmloader/smp.c
>> index 082b17f13818..a668f15d7e1f 100644
>> --- a/tools/firmware/hvmloader/smp.c
>> +++ b/tools/firmware/hvmloader/smp.c
>> @@ -22,88 +22,68 @@
>>  #include "util.h"
>>  #include "config.h"
>>  #include "apic_regs.h"
>> +#include "hypercall.h"
>>  
>> -#define AP_BOOT_EIP 0x1000
>> -extern char ap_boot_start[], ap_boot_end[];
>> +#include <xen/asm/x86-defns.h>
>> +#include <xen/hvm/hvm_vcpu.h>
>> +
>> +#include <xen/vcpu.h>
>>  
>>  static int ap_callin, ap_cpuid;
>>  
>> -asm (
>> -    "    .text                       \n"
>> -    "    .code16                     \n"
>> -    "ap_boot_start: .code16          \n"
>> -    "    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"
>> -    "    ljmpl $0x08,$1f             \n"
>> -    "gdt_desr:                       \n"
>> -    "    .word gdt_end - gdt - 1     \n"
>> -    "    .long gdt                   \n"
>> -    "ap_boot_end: .code32            \n"
>> -    "1:  mov   $0x10,%eax            \n"
>> -    "    mov   %eax,%ds              \n"
>> -    "    mov   %eax,%es              \n"
>> -    "    mov   %eax,%ss              \n"
>> -    "    movl  $stack_top,%esp       \n"
>> -    "    movl  %esp,%ebp             \n"
>> -    "    call  ap_start              \n"
>> -    "1:  hlt                         \n"
>> -    "    jmp  1b                     \n"
>> -    "                                \n"
>> -    "    .align 8                    \n"
>> -    "gdt:                            \n"
>> -    "    .quad 0x0000000000000000    \n"
>> -    "    .quad 0x00cf9a000000ffff    \n" /* 0x08: Flat code segment */
>> -    "    .quad 0x00cf92000000ffff    \n" /* 0x10: Flat data segment */
>> -    "gdt_end:                        \n"
>> -    "                                \n"
>> -    "    .bss                        \n"
>> -    "    .align    8                 \n"
>> -    "stack:                          \n"
>> -    "    .skip    0x4000             \n"
>> -    "stack_top:                      \n"
>> -    "    .text                       \n"
>> -    );
>> -
>> -void ap_start(void); /* non-static avoids unused-function compiler warning 
>> */
>> -/*static*/ void ap_start(void)
>> +static void ap_start(void)
>>  {
>>      printf(" - CPU%d ... ", ap_cpuid);
>>      cacheattr_init();
>>      printf("done.\n");
>> +
>> +    if ( !ap_cpuid )
>> +        return;
>> +
>>      wmb();
>>      ap_callin = 1;
> 
> /* After this point, the BSP will shut us down. */
> 
>> -}
>>  
>> -static void lapic_wait_ready(void)
>> -{
>> -    while ( lapic_read(APIC_ICR) & APIC_ICR_BUSY )
>> -        cpu_relax();
>> +    while ( 1 )
> 
> For this, we tend to favour "for ( ;; )".
> 
>> +        asm volatile ( "hlt" );
>>  }
>>  
>>  static void boot_cpu(unsigned int cpu)
>>  {
>> -    unsigned int icr2 = SET_APIC_DEST_FIELD(LAPIC_ID(cpu));
>> +    static uint8_t ap_stack[4 * PAGE_SIZE] __attribute__ ((aligned (16)));
> 
> I know you're just copying what was there, but 4 pages is stupidly large
> for something that needs about 4 stack slots.
> 
> 4K is absolutely plenty.
> 
>> +    static struct vcpu_hvm_context ap;
>>  
>>      /* Initialise shared variables. */
>>      ap_cpuid = cpu;
>> -    ap_callin = 0;
>>      wmb();
>>  
>> -    /* Wake up the secondary processor: INIT-SIPI-SIPI... */
>> -    lapic_wait_ready();
>> -    lapic_write(APIC_ICR2, icr2);
>> -    lapic_write(APIC_ICR, APIC_DM_INIT);
>> -    lapic_wait_ready();
>> -    lapic_write(APIC_ICR2, icr2);
>> -    lapic_write(APIC_ICR, APIC_DM_STARTUP | (AP_BOOT_EIP >> 12));
>> -    lapic_wait_ready();
>> -    lapic_write(APIC_ICR2, icr2);
>> -    lapic_write(APIC_ICR, APIC_DM_STARTUP | (AP_BOOT_EIP >> 12));
>> -    lapic_wait_ready();
>> +    /* Wake up the secondary processor */
>> +    ap = (struct vcpu_hvm_context) {
>> +        .mode = VCPU_HVM_MODE_32B,
>> +        .cpu_regs.x86_32 = {
>> +            .eip = (uint32_t)ap_start,
>> +            .esp = (uint32_t)ap_stack + ARRAY_SIZE(ap_stack),
> 
> Not that it really matters, but these want to be unsigned long casts.
> 
>> +
>> +            /* Protected mode with MMU off */
>> +            .cr0 = X86_CR0_PE,
>> +
>> +            /* Prepopulate the GDT */
> 
> /* 32bit Flat Mode */
> 
> This isn't the GDT; it's the segment registers, but "Flat Mode" is the
> more meaningful term to use.
> 
> 
> I'm happy to fix all on commit.
> 
> ~Andrew

All sound ok to me.

Cheers,
Alejandro