* [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c
@ 2021-09-17 13:51 Uros Bizjak
2021-10-13 19:24 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Uros Bizjak @ 2021-09-17 13:51 UTC (permalink / raw)
To: kvm, linux-kernel; +Cc: Uros Bizjak, Paolo Bonzini, Sean Christopherson
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c
2021-09-17 13:51 [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c Uros Bizjak
@ 2021-10-13 19:24 ` Sean Christopherson
2021-11-01 9:59 ` Uros Bizjak
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2021-10-13 19:24 UTC (permalink / raw)
To: Uros Bizjak; +Cc: kvm, linux-kernel, Paolo Bonzini
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c
2021-10-13 19:24 ` Sean Christopherson
@ 2021-11-01 9:59 ` Uros Bizjak
0 siblings, 0 replies; 3+ messages in thread
From: Uros Bizjak @ 2021-11-01 9:59 UTC (permalink / raw)
To: Sean Christopherson; +Cc: kvm, LKML, Paolo Bonzini
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-01 10:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 13:51 [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c Uros Bizjak
2021-10-13 19:24 ` Sean Christopherson
2021-11-01 9:59 ` Uros Bizjak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).