Re: [PATCH 1/6] add support for booting from grub with x86_64

[Samuel Thibault]

Hello,

Luca Dariz, le ven. 28 janv. 2022 19:24:04 +0100, a ecrit:
> * link kernel at 0x4000000 as the xen version, higher values causes
>   linker errors.
> * we can't use full segmentation in long mode, so we need to create a
>   temporary mapping during early boot to be able to jump to high
>   addresses
> * build direct map for first 4G in boothdr (seems required by Linux
>   drivers
> * enable also write page access check in kernel mode

Please document all changes.

This is also changing ./configure behavior, also please document:

> @@ -73,7 +73,7 @@
>  /* This is the kernel address range in linear addresses.  */
>  #ifdef __x86_64__
>  #define LINEAR_MIN_KERNEL_ADDRESS    VM_MIN_KERNEL_ADDRESS
> -#define LINEAR_MAX_KERNEL_ADDRESS    (0x00007fffffffffffUL)
> +#define LINEAR_MAX_KERNEL_ADDRESS    (0xffffffffffffffffUL)

why this is needed (is it perhaps even a bugfix?), why the impact it has
is not a problem. (same for INTEL_PTE_PFN)

> diff --git a/i386/intel/pmap.c b/i386/intel/pmap.c
> index 3bf00659..91835b30 100644
> --- a/i386/intel/pmap.c
> +++ b/i386/intel/pmap.c
> @@ -655,7 +655,7 @@ void pmap_bootstrap(void)
>                                 pa_to_pte(_kvtophys((void *) kernel_page_dir
>                                                     + i * INTEL_PGBYTES))
>                                 | INTEL_PTE_VALID
> -#ifdef       MACH_PV_PAGETABLES
> +#if defined(MACH_PV_PAGETABLES) || defined(__x86_64__)

AIUI it'd rather be a

> +#if !defined(MACH_HYP) || defined(MACH_PV_PAGETABLES)

?


> +     movl    $p3table,%eax
> +     or      $0b11,%eax

Please avoid magic numbers, use macros from headers so we know what 0b11
means.

> +.map_p2_table:
> +        mov     $0x200000,%eax   // 2MiB page, should be always available

(this one is fine however for instance: the comment says it all)

> diff --git a/x86_64/interrupt.S b/x86_64/interrupt.S
> index fccf6e28..eab643a5 100644
> --- a/x86_64/interrupt.S
> +++ b/x86_64/interrupt.S
> @@ -41,12 +41,12 @@ ENTRY(interrupt)
>       movl    8(%esp),%edx            /* set irq number as 3rd arg */
>       movl    %edx,%ebx               /* copy irq number */
>       shll    $2,%ebx                 /* irq * 4 */
> -     movl    EXT(iunit)(%ebx),%edi   /* get device unit number as 1st arg */
> +     movl    EXT(iunit)(%rbx),%edi   /* get device unit number as 1st arg */

Why do we need to switch to rbx?

> diff --git a/x86_64/locore.S b/x86_64/locore.S
> index 612fc493..a7266dab 100644
> --- a/x86_64/locore.S
> +++ b/x86_64/locore.S
> @@ -1136,7 +1136,7 @@ syscall_native:
>  #endif
>       shll    $5,%eax                 /* manual indexing of mach_trap_t */
>       xorq    %r10,%r10
> -     movl    EXT(mach_trap_table)(%eax),%r10d
> +     mov     EXT(mach_trap_table)(%rax),%r10

? mach_trap_arg_count is an int, not a long. We wouldn't need a long
anyway. Note that writing into the r10d register zero-clears the upper
part of r10.

>                                       /* get number of arguments */
>       andq    %r10,%r10
>       jz      mach_call_call          /* skip argument copy if none */
> @@ -1184,7 +1184,7 @@ mach_call_call:
>  0:
>  #endif /* DEBUG */
>  
> -     call    *EXT(mach_trap_table)+8(%eax)
> +     call    *EXT(mach_trap_table)+8(%rax)

Similarly, why do we need to switch to rax?

Samuel