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

[Daniel Kiper]

On Fri, Oct 19, 2018 at 04:50:25PM +0200, Juergen Gross wrote:
> On 19/10/2018 14:17, Daniel Kiper wrote:
> > 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...
>
> Hmm, if you want I can do that, sure.

Please do.

> >> +  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.
>
> This is just copied from xen/startup.S
>
> I can reduce it to something near GRUB_MEMORY_MACHINE_PROT_STACK_SIZE
> (about 64kB).

GRUB_MEMORY_MACHINE_PROT_STACK_SIZE works for me.

Daniel