Re: [Fastboot] [RFC] [PATCH 10/16] x86_64: 64bit PIC ACPI wakeup

[Pavel Machek <pavel@suse.cz>]

Hi!

> > Ok. In the new code NX bit protection feature is not being enabled and that
> > seems to be causing the problem. I checked and enabled the NX bit feature
> > in EFER in wakeup.S and it starts working.
> > 
> > I think my new machine supports NX bit protection feature and if while
> > resuming if I don't enable that feature back probably it must have caused
> > a GPF while loading the page tables which have got NX bit set. (A guess).
> > 
> > I know that previous machine I was testing on does not support NX bit
> > feature and that could be the reason that previous machine did not run into
> > the problems.
> 
> Fixed the resume problem happening on my second box which supported NX
> protection bit. Please find attached the regenerated patch.
> 
> - Killed lots of dead code

Cleanup. (a)

> - Improve the cpu sanity checks to verify long mode
>   is enabled when we wake up.

Change. (b). I'm not sure if we really need this one. I do not think
replacing cpu while suspended is supported operation.

> - Removed the need for modifying any existing kernel page table.

Unrelated change, probably good one. (c).

> - Moved wakeup_level4_pgt into the wakeup routine so we can
>   run the kernel above 4G.

The change you really wanted to do in the first place. (d).

> - Increased the size of the wakeup routine to 8K.

You want bigger stack or what? (e)

> - Renamed the variables to use the 64bit register names.

Cleanup. (a)

> - Lots of misc cleanups to match trampoline.S

More cleanups. (a).

Can we at least get (a) (b) (c) (d) and (e) separated?

Oh and please drop the whitespace changes.

> I don't have a configuration I can test this but it compiles cleanly
> and it should work, the code is very similar to the SMP trampoline,

I assume you have configuration for test now?

> @@ -60,17 +60,6 @@ extern char wakeup_start, wakeup_end;
>  
>  extern unsigned long FASTCALL(acpi_copy_wakeup_routine(unsigned long));
>  
> -static pgd_t low_ptr;
> -
> -static void init_low_mapping(void)
> -{
> -	pgd_t *slot0 = pgd_offset(current->mm, 0UL);
> -	low_ptr = *slot0;
> -	set_pgd(slot0, *pgd_offset(current->mm, PAGE_OFFSET));
> -	WARN_ON(num_online_cpus() != 1);
> -	local_flush_tlb();
> -}
> -

So you no longer need identity mapping? Is not it specified that when
you transition between modes, you should do that while in identity
mapping?

> @@ -15,7 +16,6 @@
>  # cs = 0x1234, eip = 0x05
>  #
>  
> -
>  ALIGN
>  	.align	16
>  ENTRY(wakeup_start)

Whitespace changes.

> @@ -30,22 +30,25 @@ wakeup_code:
>  	cld
>  	# setup data segment
>  	movw	%cs, %ax
> -	movw	%ax, %ds					# Make ds:0 point to wakeup_start
> +	movw	%ax, %ds			# Make ds:0 point to wakeup_start
>  	movw	%ax, %ss
> -	mov	$(wakeup_stack - wakeup_code), %sp		# Private stack is needed for ASUS board
> +						# Private stack is needed for ASUS board
> +	mov	$(wakeup_stack - wakeup_code), %sp
>  
> -	pushl	$0						# Kill any dangerous flags
> +	pushl	$0				# Kill any dangerous flags
>  	popfl

More whitespace changes.


>  	movl	real_magic - wakeup_code, %eax
>  	cmpl	$0x12345678, %eax
>  	jne	bogus_real_magic
>  
> +	call	verify_cpu			# Verify the cpu supports long mode
> +

Check if cpu supports long mode... but we suspended when running long
mode, why checking again?

>  	testl	$1, video_flags - wakeup_code
>  	jz	1f
>  	lcall   $0xc000,$3
>  	movw	%cs, %ax
> -	movw	%ax, %ds					# Bios might have played with that
> +	movw	%ax, %ds			# Bios might have played with that
>  	movw	%ax, %ss
>  1:

More whitespace changes.

> @@ -228,25 +206,10 @@ wakeup_long64:
>  	.align	64	
>  gdta:
>  	.word	0, 0, 0, 0			# dummy
> -
> -	.word	0, 0, 0, 0			# unused
> -
> -	.word	0xFFFF				# 4Gb - (0x100000*0x1000 = 4Gb)
> -	.word	0				# base address = 0
> -	.word	0x9B00				# code read/exec. ??? Why I need 0x9B00 (as opposed to 0x9A00 in order for this to work?)
> -	.word	0x00CF				# granularity = 4096, 386
> -						#  (+5th nibble of limit)
> -
> -	.word	0xFFFF				# 4Gb - (0x100000*0x1000 = 4Gb)
> -	.word	0				# base address = 0
> -	.word	0x9200				# data read/write
> -	.word	0x00CF				# granularity = 4096, 386
> -						#  (+5th nibble of limit)
> -# this is 64bit descriptor for code
> -	.word	0xFFFF
> -	.word	0
> -	.word	0x9A00				# code read/exec
> -	.word	0x00AF				# as above, but it is long mode and with D=0
> +	/* ??? Why I need the accessed bit set in order for this to work? */
> +	.quad	0x00cf9b000000ffff		# __KERNEL32_CS
> +	.quad	0x00af9b000000ffff		# __KERNEL_CS
> +	.quad	0x00cf93000000ffff		# __KERNEL_DS

Why this change, why did you change the values in here, and why you
did not tell me about it in the changelog?

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html