Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 19 Apr 2019 23:26:50 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Andi Kleen <andi@firstfloor.org>
cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v2 3/9] x86/paravirt: Replace paravirt patches with
 data
In-Reply-To: <20190330004743.29541-3-andi@firstfloor.org>
Message-ID: <alpine.DEB.2.21.1904192306120.3174@nanos.tec.linutronix.de>
References: <20190330004743.29541-1-andi@firstfloor.org> <20190330004743.29541-3-andi@firstfloor.org>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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