Re: [PATCH 10/12] Add cpu_number and cpuboot

[Samuel Thibault]

Damien Zammit, le mar. 25 oct. 2022 10:56:26 +0000, a ecrit:
> +apboot_gdt:
> +     /* NULL segment */
> +     .quad 0
> +     /* boot CS = 8 . Code Segment*/

? No, it's the actual KERNEL_CS

> +     .word 0xffff /* Segment limit first 0-15 bits*/
> +     .word (-KERNELBASE) & 0xffff /*Base first 0-15 bits*/
> +     .byte ((-KERNELBASE) >> 16) & 0xff /*Base 16-23 bits */
> +     .byte 0x9a /*Access byte */
> +     .byte 0xcf /* High 4 bits */

Rather avoid magic numbers, use ACC_* macros like e.g.
i386/i386/idt_inittab.S does.

> +     .byte ((-KERNELBASE) >> 24) & 0xff /*Base 24-31 bits */
> +     /* boot DS = 8 . Data segment*/

It's the actual KERNEL_DS

> +     .word 0xffff /*Segment limit */
> +     .word (-KERNELBASE) & 0xffff /*Base first 0-15 bits*/
> +     .byte ((-KERNELBASE) >> 16) & 0xff
> +     .byte 0x92 /*Access byte*/
> +     .byte 0xcf /* High 4 bits */
> +     .byte ((-KERNELBASE) >> 24) & 0xff /*Base 24-31 bits */
> +
> +.globl apboot, apbootend
> +.align 16
> +.code16
> +
> +apboot:
> +_apboot:
> +     cli
> +     xorl    %eax, %eax
> +     movl    %eax, %cr3
> +     mov     %ax, %ds
> +     mov     %ax, %es
> +     mov     %ax, %ss
> +
> +     lgdt    M(gdt_descr_tmp)
> +
> +     movl    %cr0, %eax
> +     andl    $~CR0_CLEAR_FLAGS, %eax
> +     orl     $CR0_SET_FLAGS, %eax
> +     movl    %eax, %cr0
> +
> +     ljmp    $KERNEL_CS, $M(0f)
> +0:
> +     .code32
> +     movw    $0, %ax

Rather use a BOOT_CS/DS macro

> +     movw    %ax, %ds
> +     movw    %ax, %es
> +     movw    %ax, %fs
> +     movw    %ax, %gs

Rather set these to BOOT_DS only?

> +     movw    $16, %ax
> +     movw    %ax, %ds
> +     movw    %ax, %es
> +     movw    %ax, %ss
> +
> +     lgdtl   apboot_gdt_descr - KERNELBASE
> +     ljmpl   $KERNEL_CS, $1f
> +1:
> +     movw    $KERNEL_DS, %ax
> +     movw    %ax, %ds
> +     movw    %ax, %es
> +     movw    %ax, %ss

Also set fs and gs to be safe.

> +     /* Load null Interrupt descriptor table */
> +     mov     apboot_idt_ptr, %ebx
> +     lidt    (%ebx)
> +
> +     /* Enable local apic */

Is it needed here, can't we do it in C?

> +     xorl    %eax, %eax
> +     xorl    %edx, %edx
> +     movl    $APIC_MSR, %ecx
> +     rdmsr
> +     orl     $APIC_MSR_ENABLE, %eax
> +     andl    $(~APIC_MSR_BSP), %eax
> +     movl    $APIC_MSR, %ecx
> +     wrmsr
> +
> +     /* Load int_stack_top[cpu] -> esp */
> +     CPU_NUMBER(%edx)
> +     movl    CX(EXT(int_stack_top), %edx), %esp
> +
> +     /* Ensure stack alignment */
> +     andl    $0xfffffff0, %esp
> +
> +     /* Reset EFLAGS to a known state */
> +     pushl $0
> +     popf
> +
> +     /* Finish the cpu configuration */
> +     call EXT(cpu_ap_main)
> +
> +     /* NOT REACHED */
> +     hlt
> +
> +.align 16
> +    .word 0
> +gdt_descr_tmp:
> +    .short 3*8+7
> +    .long M(gdt_tmp)
> +
> +.align 16
> +gdt_tmp:
> +    /* 0 */
> +    .quad 0
> +    /* boot CS = 0x8 */
> +    .word 0xffff
> +    .word 0x0000
> +    .byte 0x00
> +    .byte 0x9a
> +    .byte 0xcf

Ditto for magic numbers.

> +    .byte 0x00
> +    /* boot DS = 0x10 */
> +    .word 0xffff
> +    .word 0x0000
> +    .byte 0x00
> +    .byte 0x92
> +    .byte 0xcf
> +    .byte 0x00
> +
> +_apbootend:
> +apbootend:
> --
> 2.34.1
> 
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.