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

[Juergen Gross <jgross@suse.com>]

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 <jgross@suse.com>
>> ---
>>  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.

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


Juergen