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
