Re: [PATCH v2 09/18] xen: add PVH boot entry code

[Daniel Kiper]

On Tue, Oct 09, 2018 at 01:03:08PM +0200, Juergen Gross wrote:
> Add the code for the Xen PVH mode boot entry.
>
> Signed-off-by: Juergen Gross <address@hidden>
> ---
>  grub-core/kern/i386/xen/startup_pvh.S | 50 
> +++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/grub-core/kern/i386/xen/startup_pvh.S 
> b/grub-core/kern/i386/xen/startup_pvh.S
> index e18ee5b31..0ddb63b31 100644
> --- a/grub-core/kern/i386/xen/startup_pvh.S
> +++ b/grub-core/kern/i386/xen/startup_pvh.S
> @@ -19,11 +19,61 @@
>
>  #include <config.h>
>  #include <grub/symbol.h>
> +#include <grub/machine/memory.h>
>
>       .file   "startup_pvh.S"
>       .text
> +     .globl  start, _start
> +     .code32
>
> +start:
> +_start:
> +     cld
> +     lgdt    gdtdesc
> +     ljmp    $GRUB_MEMORY_MACHINE_PROT_MODE_CSEG, $1f
> +1:
> +     movl    $GRUB_MEMORY_MACHINE_PROT_MODE_DSEG, %eax
> +     mov     %eax, %ds
> +     mov     %eax, %es
> +     mov     %eax, %ss

Should not you load null descriptor into %fs and %gs?
Just in case...

> +     leal    LOCAL(stack_end), %esp
> +
> +     /* Save address of start info structure. */
> +     mov     %ebx, pvh_start_info
> +     call    EXT_C(grub_main)
> +     /* Doesn't return. */
> +
> +     .p2align        3
> +gdt:
> +     .word   0, 0
> +     .byte   0, 0, 0, 0
> +
> +     /* -- code segment --
> +      * base = 0x00000000, limit = 0xFFFFF (4 KiB Granularity), present
> +      * type = 32bit code execute/read, DPL = 0
> +      */
> +     .word   0xFFFF, 0
> +     .byte   0, 0x9A, 0xCF, 0
> +
> +     /* -- data segment --
> +      * base = 0x00000000, limit 0xFFFFF (4 KiB Granularity), present
> +      * type = 32 bit data read/write, DPL = 0
> +      */
> +     .word   0xFFFF, 0
> +     .byte   0, 0x92, 0xCF, 0
> +
> +     .p2align        3
> +/* this is the GDT descriptor */
> +gdtdesc:
> +     .word   0x17            /* limit */
> +     .long   gdt             /* addr */
> +
> +     .p2align        2
>  /* Saved pointer to start info structure. */
>       .globl  pvh_start_info
>  pvh_start_info:
>       .long   0
> +
> +     .bss
> +     .space  (1 << 22)

Hmmm... Why do we need 4 MiB here? If this is really needed then it begs for
a comment. And I would like to see a constant instead of plain number here.

Daniel