Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220817144045.3206-1-ubizjak@gmail.com> <Yv0QFZUdePurfjKh@google.com>
In-Reply-To: <Yv0QFZUdePurfjKh@google.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Wed, 31 Aug 2022 09:10:26 +0200
Message-ID: <CAFULd4bVQ73Cur85Oj=oXHiMRvfrxkAVy=V4TfHcbtNWbqOQzw@mail.gmail.com>
Subject: Re: [PATCH] KVM/VMX: Do not declare vmread_error asmlinkage
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 17, 2022 at 5:58 PM Sean Christopherson <seanjc@google.com> wrote:
>
> +PeterZ
>
> On Wed, Aug 17, 2022, Uros Bizjak wrote:
> > There is no need to declare vmread_error asmlinkage, its arguments
> > can be passed via registers for both, 32-bit and 64-bit targets.
> > Function argument registers are considered call-clobbered registers,
> > they are saved in the trampoline just before the function call and
> > restored afterwards.
>
> I'm officially confused.  What's the purpose of asmlinkage when used in the kernel?
> Is it some historical wart that's no longer truly necessary and only causes pain?
>
> When I wrote this code, I thought that the intent was that it should be applied to
> any and all asm => C function calls.  But that's obviously not required as there
> are multiple instances of asm code calling C functions without annotations of any
> kind.

It is the other way around. As written in coding-style.rst:

Large, non-trivial assembly functions should go in .S files, with corresponding
C prototypes defined in C header files.  The C prototypes for assembly
functions should use ``asmlinkage``.

So, prototypes for *assembly functions* should use asmlinkage.

That said, asmlinkage for i386 just switches ABI to the default
stack-passing ABI. However, we are calling assembly files, so the
argument handling in the callee is totally under our control and there
is no need to switch ABIs. It looks to me that besides syscalls,
asmlinkage is and should be used only for a large imported body of asm
functions that use standard stack-passing ABI (e.g. x86 crypto and
math-emu functions), otherwise it is just a burden to push and pop
registers to/from stack for no apparent benefit.

> And vmread_error() isn't the only case where asmlinkage appears to be a burden, e.g.
> schedule_tail_wrapper() => schedule_tail() seems to exist purely to deal with the
> side affect of asmlinkage generating -regparm=0 on 32-bit kernels.

schedule_tail is external to the x86 arch directory, and for some
reason marked asmlinkage. So, the call from asm must follow asmlinkage
ABI.

FYI, there is some discussion about asmlinkage on stackoverflow:

https://stackoverflow.com/questions/61635931/does-asmlinkage-mean-stack-or-registers

Uros.