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

[Luca]

Il 28/01/22 20:56, Samuel Thibault ha scritto:
> 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.

Will do

> 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)

Strictly speaking, this should not be needed now, as we don't use high 
addresses yet; however, this seems to impact only VM_MAX_KERNEL_ADDRESS 
for 64-bit targets, which seems to be used only in pmap.c as a safety limit.

> > 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)
>
> ?

In model_dep.c the CR0_WP bit is set only if MACH_HYP is not defined, I 
don't know if MACH_PV_PAGETABLES enables write protection in some other 
way. Additionally, with this definition it would be enabled also for 
32-bit non-xen, changing the current behavior. I'll test it and if it 
works I'll change the condition.


> > +       movl    $p3table,%eax
> > +       or      $0b11,%eax
>
> Please avoid magic numbers, use macros from headers so we know what 0b11
> means.

will do

> > +.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?

This is needed when using high addresses for KERNEL_MAP_BASE because it 
fails to compile (I think because iunit's address was not fitting 32-bit 
anymore)

> > 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.

This also failed to compile when using high addresses

> >                                         /* 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?

Also failed to compile on high addresses. Maybe I should factor out 
these changes in another patch.


Thanks for the comments!

Luca