On Fri, Sep 17, 2021, Uros Bizjak wrote:
> Improve exception safe wrappers in emulate.c by converting them to
> ASM GOTO (and ASM GOTO OUTPUT when supported) statements. Also, convert
> wrappers to inline functions to avoid statement as expression
> GNU extension and to remove weird requirement where user must know
> where the asm argument is being expanded.
>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Signed-off-by: Uros Bizjak <ubizjak@xxxxxxxxx>
> ---
> arch/x86/kvm/emulate.c | 80 ++++++++++++++++++++++++++++++------------
> 1 file changed, 57 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2837110e66ed..2197a3ecc55b 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -464,25 +464,59 @@ FOP_FUNC(salc)
> FOP_RET(salc)
> FOP_END;
>
> -/*
> - * XXX: inoutclob user must know where the argument is being expanded.
I 100% agree that this is a weird requirement, but I actually like the side
effect of forcing the caller to define a name for the input/output.
> - * Relying on CONFIG_CC_HAS_ASM_GOTO would allow us to remove _fault.
> - */
> -#define asm_safe(insn, inoutclob...) \
> -({ \
> - int _fault = 0; \
> - \
> - asm volatile("1:" insn "\n" \
> - "2:\n" \
> - ".pushsection .fixup, \"ax\"\n" \
> - "3: movl $1, %[_fault]\n" \
> - " jmp 2b\n" \
> - ".popsection\n" \
> - _ASM_EXTABLE(1b, 3b) \
> - : [_fault] "+qm"(_fault) inoutclob ); \
> - \
> - _fault ? X86EMUL_UNHANDLEABLE : X86EMUL_CONTINUE; \
> -})
> +static __always_inline int safe_fwait(void)
> +{
> + asm_volatile_goto("1: fwait\n\t"
> + _ASM_EXTABLE(1b, %l[fault])
> + : : : : fault);
> + return X86EMUL_CONTINUE;
> + fault:
> + return X86EMUL_UNHANDLEABLE;
> +}
Rather than defining a bunch of safe_() variants, what about providing a generic
helper/macro similar to the existing asm_safe()? Not just for KVM, but for the
kernel at large. Asm with output is problematic due to the CONFIG_CC_HAS_ASM_GOTO
dependency, but it wouldn't be the end of the world to state that simply isn't
supported until the min compiler version is raised.
__wrmsr(), native_write_msr_safe(), cpu_vmxoff(), kvm_cpu_vmxon(), and probably
others could use a generic generic asm_safe(). I wouldn't be surprised if there
are other places in the kernel that could take advantage of such a helper, e.g.
kvm_load_ldt() could use a "safe" variant instead of crashing if the sel is bad.
