[PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

[PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

[Uros Bizjak]

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@redhat.com>
Cc: Sean Christopherson  <seanjc@google.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 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.
- *      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;
+}
+
+static __always_inline int safe_fxrstor(struct fxregs_state *fx_state)
+{
+	asm_volatile_goto("1: fxrstor %0\n\t"
+			  _ASM_EXTABLE(1b, %l[fault])
+			  : : "m" (*fx_state) : : fault);
+	return X86EMUL_CONTINUE;
+ fault:
+	return X86EMUL_UNHANDLEABLE;
+}
+
+#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+
+static __always_inline int safe_fxsave(struct fxregs_state *fx_state)
+{
+	asm_volatile_goto("1: fxsave %0\n\t"
+			  _ASM_EXTABLE(1b, %l[fault])
+			  : "=m" (*fx_state) : : : fault);
+	return X86EMUL_CONTINUE;
+ fault:
+	return X86EMUL_UNHANDLEABLE;
+}
+
+#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+
+static __always_inline int safe_fxsave(struct fxregs_state *fx_state)
+{
+	int rc;
+
+	asm volatile("1: fxsave %0\n\t"
+		     "movl %2, %1\n\t"
+		     "2:\n\t"
+	             ".pushsection .fixup, \"ax\"\n\t"
+	             "3: movl %3, %1\n\t"
+	             "jmp 2b\n\t"
+	             ".popsection\n\t"
+	             _ASM_EXTABLE(1b, 3b)
+	             : "=m" (*fx_state), "=rm" (rc)
+		     : "i" (X86EMUL_CONTINUE),
+		       "i" (X86EMUL_UNHANDLEABLE));
+	return rc;
+}
+
+#endif // CONFIG_CC_ASM_GOTO_OUTPUT
 
 static int emulator_check_intercept(struct x86_emulate_ctxt *ctxt,
 				    enum x86_intercept intercept,
@@ -4030,7 +4064,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt)
 
 	kvm_fpu_get();
 
-	rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_state));
+	rc = safe_fxsave (&fx_state);
 
 	kvm_fpu_put();
 
@@ -4054,7 +4088,7 @@ static noinline int fxregs_fixup(struct fxregs_state *fx_state,
 	struct fxregs_state fx_tmp;
 	int rc;
 
-	rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_tmp));
+	rc = safe_fxsave (&fx_tmp);
 	memcpy((void *)fx_state + used_size, (void *)&fx_tmp + used_size,
 	       __fxstate_size(16) - used_size);
 
@@ -4090,7 +4124,7 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
 	}
 
 	if (rc == X86EMUL_CONTINUE)
-		rc = asm_safe("fxrstor %[fx]", : [fx] "m"(fx_state));
+		rc = safe_fxrstor (&fx_state);
 
 out:
 	kvm_fpu_put();
@@ -5342,7 +5376,7 @@ static int flush_pending_x87_faults(struct x86_emulate_ctxt *ctxt)
 	int rc;
 
 	kvm_fpu_get();
-	rc = asm_safe("fwait");
+	rc = safe_fwait();
 	kvm_fpu_put();
 
 	if (unlikely(rc != X86EMUL_CONTINUE))
-- 
2.31.1

Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

[Sean Christopherson]

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@redhat.com>
> Cc: Sean Christopherson  <seanjc@google.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  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.

Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c

[Uros Bizjak]

On Wed, Oct 13, 2021 at 9:24 PM Sean Christopherson <seanjc@google.com> wrote:
>
> 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@redhat.com>
> > Cc: Sean Christopherson  <seanjc@google.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >  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.

After 5.16 x86 FPU handling rewrite, kernel_insn_err and even
fxrstor_safe are now in fpu/legacy.h. Is there a way to share these
with KVM?

Uros.