Re: [PATCH] S3 suspend/resume with noexec

[Pavel Machek]

Hi!

> >> @@ -62,6 +62,10 @@ void __save_processor_state(struct saved
> >>  	asm volatile ("movl %%cr0, %0" : "=r" (ctxt->cr0));
> >>  	asm volatile ("movl %%cr2, %0" : "=r" (ctxt->cr2));
> >>  	asm volatile ("movl %%cr3, %0" : "=r" (ctxt->cr3));
> >> +#ifdef CONFIG_X86_PAE
> >> +	if (cpu_has_pae && cpu_has_nx)
> >> +		rdmsr(MSR_EFER, ctxt->efer_lo, ctxt->efer_hi);
> >> +#endif
> >>  	asm volatile ("movl %%cr4, %0" : "=r" (ctxt->cr4));
> >>  }
> >
> >Are those #ifdefs good idea?
> 
> That CONFIG is required. Cpu_has_pae and cpu_has_nx doesn't 
> imply that we are using nx. Only when PAE kernel is configured 
> and these features are available, we use it.
> 
> Looking at the code again, I can remove that CONFIG_X86_PAE,
> if I use an additional check for nx_enabled.

Yes, that would be better.

> >> @@ -59,6 +59,20 @@ wakeup_code:
> >>  	movl	$swapper_pg_dir-__PAGE_OFFSET, %eax
> >>  	movl	%eax, %cr3
> >>  
> >> +	testl	$1, real_efer_save_restore - wakeup_code
> >> +	jz	4f
> >> +	# restore efer setting
> >> +	pushl    %eax
> >> +	pushl    %ecx
> >> +	pushl    %edx
> >> +	movl	real_save_efer_edx - wakeup_code, %edx
> >> +	movl	real_save_efer_eax - wakeup_code, %eax
> >> +	mov     $0xc0000080, %ecx
> >> +	wrmsr
> >> +	popl    %edx
> >> +	popl    %ecx
> >> +	popl    %eax
> >> +4:
> >>  	# make sure %cr4 is set correctly (features, etc)
> >>  	movl	real_save_cr4 - wakeup_code, %eax
> >>  	movl	%eax, %cr4
> >
> >Please analyse surrounding code a bit more. You certainly do not need
> >to push %eax, because it is scratch register.
> 
> Yes. I saw that eax was not used. But, I thought it is clean to save 
> and restore every register that is being touched here. Just in case some
> 
> other code around these place starts using those registers in future.
> 
> If you think that is unnecessary, I will resend the patch with minimal 
> push/pops.

I'd like minimal push/pops. If we pushed/poped at every possible
place, it would get too long and too unreadable too quickly.

> >Is it neccessary to restore efer this soon, btw? We should be running
> >from swapper_pg_dir. Does that use nx?
> 
> It seems necessary for swapper_pg_dir too as we use nx even for kernel
> pages (stack, module, etc).

Ahha, ok.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/