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.vall...@cloud.com>
>
> 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
