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: <20220804180358.32944-1-ubizjak@gmail.com> <YuwVY8iGkifExuli@google.com>
In-Reply-To: <YuwVY8iGkifExuli@google.com>
From:   Uros Bizjak <ubizjak@gmail.com>
Date:   Thu, 4 Aug 2022 21:05:57 +0200
Message-ID: <CAFULd4a3oY6HPtccj0tAH0H9h=mhZkbUUimqq4tWBZU2E6oS2g@mail.gmail.com>
Subject: Re: [PATCH] x86/acrn: Improve ACRN hypercalls
To:     Sean Christopherson <seanjc@google.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 4, 2022 at 8:52 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Aug 04, 2022, Uros Bizjak wrote:
> > As explained in section 6.47.5.2, "Specifying Registers for Local Variables"
> > of the GCC info documentation, the correct way to specify register for
> > input operands when calling Extended 'asm' is to define a local register
> > variable and associate it with a specified register:
> >
> >       register unsigned long r8 asm ("r8") = hcall_id;
>
> Using "register" for input operands is subtly dangerous.  The compiler (at least
> some versions of GCC) isn't smart enough to realize that it must not clobber the
> register between setting the register and passing it into asm().  Or maybe that's
> the designed/intended behavior, but if so, IMO it's a terrible design.
>
> And since kernel style is to put declarations at the beginning of functions, it's
> quite easy for a developer/debugger to mess this up (speaking from multiple
> experiences).

All you said there is true, and these cases are even covered in the
GCC documentation (please see section 6.47.5.2, "Specifying Registers
for Local Variables"). But in this particular case, we have a static
inline function, which encapsulates low-level access, so we *can* add
the definition just above the asm.

Uros.