Re: [PATCH v2 3/9] x86/paravirt: Replace paravirt patches with data

[Thomas Gleixner]

On Fri, 29 Mar 2019, Andi Kleen wrote:
> For LTO all top level assembler statements need to be global because
> LTO might put it into a different assembler file than the referencing
> C code.
> 
> To avoid making all the paravirt patch snippets global replace them
> with data containing the patch instructions. Since these are unlikely
> to change this shouldn't be a significant maintenance burden.

s/with data containing/with unparseable, inconsistent and broken mess/

Unparseable:
------------

> +static const unsigned char patch_irq_save_fl[] = { 0x9c, 0x58 };  	/* pushf; pop %eax */
> +static const unsigned char patch_cpu_iret[] = { 0xcf };		  	/* iret */
> +static const unsigned char patch_mmu_read_cr2[] = { 0x0f, 0x20, 0xd0 }; /* mov %cr2, %eax */
> +static const unsigned char patch_mmu_write_cr3[] = { 0x0f, 0x22, 0xd8 };/* mov %eax, %cr3 */
> +static const unsigned char patch_mmu_read_cr3[] = { 0x0f, 0x20, 0xd8 }; /* mov %cr3, %eax */

  Overlong lines, spaces and tabs mixed, no formatting which allows easy
  reading and review.

Inconsistent and unparseable:
-----------------------------

>  #define PATCH_SITE(ops, x)					\
>  	case PARAVIRT_PATCH(ops.x):				\
> -		return paravirt_patch_insns(ibuf, len, start_##ops##_##x, end_##ops##_##x)
> +		return paravirt_patch_insns(ibuf, len, 		\
> +				patch_##ops##_##x, patch_##ops##_##x+sizeof(patch_##ops##_x));

vs.

> +		return paravirt_patch_insns(ibuf, len, start_##ops##_##x, \
> +				patch_##ops##_##x + sizeof(patch_##ops##_##x));

Broken:
-------

> +static const unsigned char patch_irq_restore_fl[] =  { 0x50, 0x9d};		/* pushq %rdi; popfq */

Can you spot the fail?

That probably works because Intel CPUs are so good at executing crappy
code, right? That's at least what you told me recently. If that's the proof
then I should really stop reviewing patches.

Thanks,

	tglx