Re: [PATCH gnumach] smp: Remove hardcoded AP_BOOT_ADDR

[Jessica Clarke]

On 29 Jan 2024, at 10:20, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> 
> Damien Zammit, le lun. 29 janv. 2024 10:07:30 +0000, a ecrit:
>> - ljmp $BOOT_CS, $M(0f)
>> + xorl %eax, %eax
>> + mov %cs, %ax
>> + shll $4, %eax
>> + addl $M(0f), %eax
>> + movl %eax, M(ljmp_offset32)
> 
> This won't work with pipelined processors, which assume a complete
> separation between code and data, and will thus have already loaded
> the jmp instruction before your modify it.

That’s true of most architectures, but not x86. It architecturally
guarantees that self-modifying code works, at least without using
different virtual addresses that physically alias (though in practice
it seems at least Intel’s implementations match on physical not virtual
address so even that works).

> Rather either perform the relocation from the C code,

Were your statement true, that wouldn’t fix the problem, just make it
less likely due to the larger number of instructions between
modification and execution.

Jess

> or use a variable,
> which you can refer from the ljmp instruction.
> 
>> + /* ljmpl with relocation */
>> + .byte 0x66
>> + .byte 0xea
>> +ljmp_offset32:
>> + .long 0xffffffff
>> + .word BOOT_CS
>> +
>> 0:
>> .code32
>> + /* Protected mode! */
>> movw $BOOT_DS, %ax
>> movw %ax, %ds
>> movw %ax, %es
>> diff --git a/i386/i386/mp_desc.c b/i386/i386/mp_desc.c
>> index 467f2728..76b4a79c 100644
>> --- a/i386/i386/mp_desc.c
>> +++ b/i386/i386/mp_desc.c
>> @@ -99,6 +99,7 @@ interrupt_stack_alloc(void)
>>  */
>> int bspdone;
>> 
>> +phys_addr_t apboot_addr;
>> extern void *apboot, *apbootend;
>> extern volatile ApicLocalUnit* lapic;
>> 
>> @@ -296,7 +297,7 @@ cpu_start(int cpu)
>> 
>>     printf("Trying to enable: %d\n", apic_id);
>> 
>> -    smp_startup_cpu(apic_id, AP_BOOT_ADDR);
>> +    smp_startup_cpu(apic_id, apboot_addr);
>> 
>>     printf("Started cpu %d (lapic id %04x)\n", cpu, apic_id);
>> 
>> @@ -309,7 +310,7 @@ start_other_cpus(void)
>> int ncpus = smp_get_numcpus();
>> 
>> //Copy cpu initialization assembly routine
>> - memcpy((void*)phystokv(AP_BOOT_ADDR), (void*) &apboot,
>> + memcpy((void*) phystokv(apboot_addr), (void*) &apboot,
>>        (uint32_t)&apbootend - (uint32_t)&apboot);
>> 
>> #ifndef APIC
>> diff --git a/i386/i386/mp_desc.h b/i386/i386/mp_desc.h
>> index fea42cd3..bcc68662 100644
>> --- a/i386/i386/mp_desc.h
>> +++ b/i386/i386/mp_desc.h
>> @@ -46,8 +46,6 @@
>> #include "gdt.h"
>> #include "ldt.h"
>> 
>> -#define AP_BOOT_ADDR 0x7000
>> -
>> /*
>>  * The descriptor tables are together in a structure
>>  * allocated one per processor (except for the boot processor).
>> @@ -78,6 +76,11 @@ extern uint8_t solid_intstack[];
>> 
>> extern int bspdone;
>> 
>> +/*
>> + * Address to hold AP boot code, held in ASM
>> + */
>> +extern phys_addr_t apboot_addr;
>> +
>> /*
>>  * Each CPU calls this routine to set up its descriptor tables.
>>  */
>> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
>> index b0a55754..fb95029f 100644
>> --- a/i386/i386at/model_dep.c
>> +++ b/i386/i386at/model_dep.c
>> @@ -66,6 +66,7 @@
>> #include <i386/locore.h>
>> #include <i386/model_dep.h>
>> #include <i386/smp.h>
>> +#include <i386/seg.h>
>> #include <i386at/acpi_parse_apic.h>
>> #include <i386at/autoconf.h>
>> #include <i386at/biosmem.h>
>> @@ -125,6 +126,9 @@ char *kernel_cmdline = "";
>> 
>> extern char version[];
>> 
>> +/* Realmode temporary GDT */
>> +extern struct pseudo_descriptor gdt_descr_tmp;
>> +
>> /* If set, reboot the system on ctrl-alt-delete.  */
>> boolean_t rebootflag = FALSE; /* exported to kdintr */
>> 
>> @@ -201,6 +205,20 @@ void machine_init(void)
>>  */
>> pmap_unmap_page_zero();
>> #endif
>> +
>> +#ifdef APIC
>> + /*
>> +  * Grab an early page for AP boot code
>> +  */
>> + /* FIXME: this may not allocate from below 1MB, if within first 16MB */
>> + apboot_addr = vm_page_to_pa(vm_page_grab_contig(PAGE_SIZE, 
>> VM_PAGE_SEL_DMA));
>> + assert (apboot_addr < 0x100000);
>> +
>> + /*
>> +  * Patch the realmode gdt with the correct offset
>> +  */
>> + gdt_descr_tmp.linear_base += apboot_addr;
>> +#endif
>> }
>> 
>> /* Conserve power on processor CPU.  */
>> -- 
>> 2.43.0
>> 
>> 
>> 
> 
> -- 
> Samuel
> ---
> Pour une évaluation indépendante, transparente et rigoureuse !
> Je soutiens la Commission d'Évaluation de l'Inria.