On Wed, Oct 13, 2021 at 9:24 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@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.
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.
