Re: [PATCH v1 2/3] x86/msr: Switch between WRMSRNS and WRMSR with the alternatives mechanism

[H. Peter Anvin]

On 8/16/24 11:40, Andrew Cooper wrote:
> > As the CALL instruction is 5-byte long, and we need to pad nop for both
> > WRMSR and WRMSRNS, what about not using segment prefix at all?

You can use up to 4 prefixes of any kind (which includes opcode prefixes 
before 0F) before most decoders start hurting, so we can pad it out to 5 
bytes by doing 3f 3f .. .. ..

> My suggestion, not that I've had time to experiment, was to change
> paravirt to use a non-C ABI and have asm_xen_write_msr() recombine
> edx:eax into rsi.  That way the top level wrmsr() retains sensible
> codegen for native even when paravirt is active.

I have attached what should be an "obvious" example... famous last words.

	-hpa
/*
 * Input in %rax, MSR number in %ecx
 * %rdx is clobbered, as the native stub is assumed to do
 *
 * Change xen_do_write_msr to return its error code instead
 * of passing a pointer	- this gives the extra benefit that
 * we can get the *actual invocation address* in the error
 * messages.
 *
 * ex_handler_msr() should be changed to get the MSR data
 * from %rax regardless of Xen vs native; alternatively the
 * Xen handler can set up %edx as well.
 *
 * Let the native pattern look like:
 *
 * 48 89 c2                mov    %rax,%rdx
 * 48 c1 ea 20             shr    $32,%rdx
 * 3e 0f 30                ds wrmsr		<--- trap point
 *
 * ... which can be replaced with ...
 * 48 89 c2                mov    %rax,%rdx
 * 48 c1 ea 20             shr    $32,%rdx
 * 0f 01 c6                wrmsrns		<--- trap point
 *
 * ... or ...
 * e8 xx xx xx xx          call asm_xen_write_msr
 * 74 02                   jz 1f
 * 3e 0f 0b                ds ud2                  <--- trap point
 *                      1:
 * ds ud2 can of course be replaced with any other 3-byte trapping
 * instruction.
 *
 * This also removes the need for Xen to maintain different safe and
 * unsafe MSR routines, as the difference is handled by the same
 * trap handler as is used natively.
 */
 SYM_FUNC_START(asm_xen_write_msr)
	FRAME_BEGIN
	push %rax		/* Save in case of error */
	push %rcx
	push %rsi
	push %rdi
	push %r8
	push %r9
	push %r10
	push %r11
	mov %rax,%rdi
	mov %ecx,%esi
	call xen_do_write_msr
	test %eax,%eax		/* ZF=0 on error */
	pop %r11
	pop %r10
	pop %r9
	pop %r8
	pop %rdi
	pop %rsi
	pop %rcx
#ifdef OPTIONAL
	mov 4(%rsp),%edx
#endif
	pop %rax
	FRAME_END
	RET
SYM_FUNC_END(asm_xen_write_msr)