Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210917135152.5111-1-ubizjak@gmail.com> <YWcyeGk7vOSoQWW4@google.com>
In-Reply-To: <YWcyeGk7vOSoQWW4@google.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Mon, 1 Nov 2021 10:59:20 +0100
Message-ID: <CAFULd4YxctGCbLFSJrr4WK1wS4c_-QwkCQ4V77=8_B0URSeFow@mail.gmail.com>
Subject: Re: [PATCH] KVM: x86: Improve exception safe wrappers in emulate.c
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.