[PATCH] x86: Fix code patching for paravirt-alternatives on 486

[PATCH] x86: Fix code patching for paravirt-alternatives on 486

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

As reported in <http://bugs.debian.org/511703> and
<http://bugs.debian.org/515982>, kernels with paravirt-alternatives
enabled crash in text_poke_early() on at least some 486-class
processors.

The problem is that text_poke_early() itself contains paravirt-
alternatives and therefore will modify instructions that have already
been prefetched.  Pentium and later processors will invalidate the
prefetched instructions in this case, but 486-class processors do
not.  We must use a jmp instruction to limit prefetching.

There is then a further problem in that sync_core() uses "cpuid" which
isn't implemented by most 486-class processors.  Since they also do
not perform speculative execution, we can make this conditional on the
processor family.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This has been tested as a change to 2.6.26 by Richard Kettlewell.  This
code doesn't appear to have changed significantly since then, so
hopefully the change is still correct.

Possible the call to sync_core() should be moved above the
local_irq_restore() and should incorporate the dummy jmp?

Ben.

 arch/x86/include/asm/processor.h |    6 ++++++
 arch/x86/kernel/alternative.c    |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index c776826..74ddfce 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -708,6 +708,12 @@ static inline void sync_core(void)
 {
 	int tmp;
 
+#if defined(CONFIG_M386) || defined(CONFIG_M486)
+	/* This is unnecessary on 386- and 486-class processors, most of
+	   which don't even implement CPUID. */
+	if (boot_cpu_data.x86 < 5)
+		return;
+#endif
 	asm volatile("cpuid" : "=a" (tmp) : "0" (1)
 		     : "ebx", "ecx", "edx", "memory");
 }
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 4869351..330ab89 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -498,6 +498,9 @@ static void *__init_or_module text_poke_early(void *addr, const void *opcode,
 	unsigned long flags;
 	local_irq_save(flags);
 	memcpy(addr, opcode, len);
+	/* Force 486-class processors to flush prefetched instructions,
+	   since we may have just patched local_irq_restore(). */
+	asm volatile("jmp 1f\n1:\n" ::: "memory");
 	local_irq_restore(flags);
 	sync_core();
 	/* Could also do a CLFLUSH here to speed up CPU recovery; but
-- 
1.6.3.3


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] x86: Fix code patching for paravirt-alternatives on 486

[H. Peter Anvin]

On 09/08/2009 04:23 PM, Ben Hutchings wrote:
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index c776826..74ddfce 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -708,6 +708,12 @@ static inline void sync_core(void)
>  {
>  	int tmp;
>  
> +#if defined(CONFIG_M386) || defined(CONFIG_M486)
> +	/* This is unnecessary on 386- and 486-class processors, most of
> +	   which don't even implement CPUID. */
> +	if (boot_cpu_data.x86 < 5)
> +		return;
> +#endif
>  	asm volatile("cpuid" : "=a" (tmp) : "0" (1)
>  		     : "ebx", "ecx", "edx", "memory");
>  }
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 4869351..330ab89 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -498,6 +498,9 @@ static void *__init_or_module text_poke_early(void *addr, const void *opcode,
>  	unsigned long flags;
>  	local_irq_save(flags);
>  	memcpy(addr, opcode, len);
> +	/* Force 486-class processors to flush prefetched instructions,
> +	   since we may have just patched local_irq_restore(). */
> +	asm volatile("jmp 1f\n1:\n" ::: "memory");
>  	local_irq_restore(flags);
>  	sync_core();
>  	/* Could also do a CLFLUSH here to speed up CPU recovery; but

I'm wondering if it wouldn't be cleaner to fold the jump into
sync_core() and moving the sync_core() up before local_irq_restore().

	-hpa

Re: [PATCH] x86: Fix code patching for paravirt-alternatives on 486

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Tue, 2009-09-08 at 17:31 -0700, H. Peter Anvin wrote:
[...]
> I'm wondering if it wouldn't be cleaner to fold the jump into
> sync_core() and moving the sync_core() up before local_irq_restore().

Exactly as I suggested below the dashes. :-)

The only reason I didn't do that initially was that I don't know whether
or not there's a good reason for the current placement of sync_core()
after local_irq_restore().

Ben.

-- 
Ben Hutchings
Who are all these weirdos? - David Bowie, about L-Space IRC channel #afp

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] x86: Fix code patching for paravirt-alternatives on 486

[H. Peter Anvin]

On 09/08/2009 05:46 PM, Ben Hutchings wrote:
> On Tue, 2009-09-08 at 17:31 -0700, H. Peter Anvin wrote:
> [...]
>> I'm wondering if it wouldn't be cleaner to fold the jump into
>> sync_core() and moving the sync_core() up before local_irq_restore().
> 
> Exactly as I suggested below the dashes. :-)
> 

Ah, yes, so you did indeed.

> The only reason I didn't do that initially was that I don't know whether
> or not there's a good reason for the current placement of sync_core()
> after local_irq_restore().

I can personally not fathom how moving the sync_core() before
local_irq_restore() could cause problems -- in fact, the current code
seems logically wrong, although probably correct in practice, as the
interrupt itself should perform the necessary synchronization (although
it is not architecturally required to do so.)  Not that I haven't been
wrong before.

	-hpa