Re: [patch 3/3] x86/paravirt: Replace paravirt patch asm magic

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Apr 25, 2019 at 08:52:09AM +0200, Ingo Molnar wrote:

> > -# ifdef CONFIG_PARAVIRT_XXL
> > -DEF_NATIVE(irq, irq_disable, "cli");
> > -DEF_NATIVE(irq, irq_enable, "sti");
> > -DEF_NATIVE(irq, restore_fl, "push %eax; popf");
> > -DEF_NATIVE(irq, save_fl, "pushf; pop %eax");
> > -DEF_NATIVE(cpu, iret, "iret");
> > -DEF_NATIVE(mmu, read_cr2, "mov %cr2, %eax");
> > -DEF_NATIVE(mmu, write_cr3, "mov %eax, %cr3");
> > -DEF_NATIVE(mmu, read_cr3, "mov %cr3, %eax");
> 
> > +static const struct patch_xxl patch_data_xxl = {
> > +	.irq_irq_disable	= { 0xfa },		// cli
> > +	.irq_irq_enable		= { 0xfb },		// sti
> > +	.irq_save_fl		= { 0x9c, 0x58 },	// pushf; pop %[re]ax
> > +	.mmu_read_cr2		= { 0x0f, 0x20, 0xd0 },	// mov %cr2, %[re]ax
> > +	.mmu_read_cr3		= { 0x0f, 0x20, 0xd8 },	// mov %cr3, %[re]ax
> > +# ifdef CONFIG_X86_64
> > +	.irq_restore_fl		= { 0x57, 0x9d },	// push %rdi; popfq
> > +	.mmu_write_cr3		= { 0x0f, 0x22, 0xdf },	// mov %rdi, %cr3
> > +	.cpu_wbinvd		= { 0x0f, 0x09 },	// wbinvd
> > +	.cpu_usergs_sysret64	= { 0x0f, 0x01, 0xf8,
> > +				    0x48, 0x0f, 0x07 },	// swapgs; sysretq
> > +	.cpu_swapgs		= { 0x0f, 0x01, 0xf8 },	// swapgs
> > +	.mov64			= { 0x48, 0x89, 0xf8 },	// mov %rdi, %rax
> > +# else
> > +	.irq_restore_fl		= { 0x50, 0x9d },	// push %eax; popf
> > +	.mmu_write_cr3		= { 0x0f, 0x22, 0xd8 },	// mov %eax, %cr3
> > +	.cpu_iret		= { 0xcf },		// iret
> > +# endif
> 
> I think these open-coded hexa versions are somewhat fragile as well, how 
> about putting these into a .S file and controlling the sections in an LTO 
> safe manner there?
> 
> That will also allow us to write proper asm, and global labels can be 
> used to extract the patchlets and their length?

While I'm not fan either; I think that will be worse still, because it
splits the information over multiple files.

The advantage of this form is that it is clear how long the instructions
are, which is important for the patching. These immediates have to be
shorter than 5 bytes because they overwrite the CALL/JMP to the paravirt
function.

/me eyes .cpu_usergs_sysret64 and goes wtf..