On Thu, 2008-03-27 at 13:31 -0700, Linus Torvalds wrote:
quoted text
> 
> On Thu, 27 Mar 2008, Ingo Molnar wrote:
> >
> > Ingo Molnar (1):
> >       x86: fix prefetch workaround
> ...
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index fdc6674..c0c82bc 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -92,7 +92,8 @@ static int is_prefetch(struct pt_regs *regs, unsigned long addr,
> >  	unsigned char *max_instr;
> >  
> >  #ifdef CONFIG_X86_32
> > -	if (!(__supported_pte_mask & _PAGE_NX))
> > +	/* Catch an obscure case of prefetch inside an NX page: */
> > +	if ((__supported_pte_mask & _PAGE_NX) && (error_code & 16))
> >  		return 0;
> >  #endif
> 
> Ingo, this patch makes no sense.
> 
> Two reasons:
> 
>  - "error_code & 16" is senseless. Use PF_INSTR instead, which actually 
>    tells the reader something.
> 
>  - this piece of crap code is immediately followed by
> 
> 	/* If it was a exec fault on NX page, ignore */
> 	if (error_code & PF_INSTR)
> 		return 0;
> 
>    which uses that *right* and readable PF_INSTR #define, and also shows 
>    that the newly modified code is totally insane (ie: if the new code 
>    triggers, then it would have returned 0 later _anyway_)
> 
> So I think it's just crap. I think it's duplication from the merging of 
> the x86 code, and I think that the fact that the new code didn't use the 
> right #define helper means that people didn't see that it was crap.
> 
> I pulled it, but this needs some resolution. The code makes no sense. 
> Should that #ifdef and the code inside of it just be removed entirely?
> 

Sorry, this was my fault, I eliminated the ifdef within the X86_32 block
but didn't go further.  Think the below is correct:

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index c0c82bc..6f5df93 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -91,14 +91,8 @@ static int is_prefetch(struct pt_regs *regs, unsigned long addr,
 	int prefetch = 0;
 	unsigned char *max_instr;
 
-#ifdef CONFIG_X86_32
 	/* Catch an obscure case of prefetch inside an NX page: */
-	if ((__supported_pte_mask & _PAGE_NX) && (error_code & 16))
-		return 0;
-#endif
-
-	/* If it was a exec fault on NX page, ignore */
-	if (error_code & PF_INSTR)
+	if ((__supported_pte_mask & _PAGE_NX) && (error_code & PF_INSTR))
 		return 0;
 
 	instr = (unsigned char *)convert_ip_to_linear(current, regs);



--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/