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;
Date:   Thu, 4 Aug 2022 18:52:19 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Uros Bizjak <ubizjak@gmail.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>
Subject: Re: [PATCH] x86/acrn: Improve ACRN hypercalls
Message-ID: <YuwVY8iGkifExuli@google.com>
References: <20220804180358.32944-1-ubizjak@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220804180358.32944-1-ubizjak@gmail.com>
Precedence: bulk

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).

E.g. adding a pr_warn_ratelimited() after the register declaration generates code
that doesn't even load r8.

Dump of assembler code for function fake_acrn_example:
Address range 0xffffffff8105a1e0 to 0xffffffff8105a216:
   0xffffffff8105a1e0 <+0>:	call   0xffffffff8104a8c0 <__fentry__>
   0xffffffff8105a1e5 <+5>:	push   %rbp
   0xffffffff8105a1e6 <+6>:	mov    $0xffffffff81e08b40,%rsi
   0xffffffff8105a1ed <+13>:	mov    %rsp,%rbp
   0xffffffff8105a1f0 <+16>:	push   %r12
   0xffffffff8105a1f2 <+18>:	movslq %edi,%r12
   0xffffffff8105a1f5 <+21>:	mov    $0xffffffff822229e0,%rdi
   0xffffffff8105a1fc <+28>:	call   0xffffffff81545e30 <___ratelimit>
   0xffffffff8105a201 <+33>:	test   %eax,%eax
   0xffffffff8105a203 <+35>:	jne    0xffffffff81843bf0 <fake_acrn_example.cold>
   0xffffffff8105a209 <+41>:	vmcall
   0xffffffff8105a20c <+44>:	mov    -0x8(%rbp),%r12
   0xffffffff8105a210 <+48>:	leave
   0xffffffff8105a211 <+49>:	jmp    0xffffffff81c01a40 <__x86_return_thunk>


diff --git a/arch/x86/include/asm/acrn.h b/arch/x86/include/asm/acrn.h
index 601867085b95..adc867a4a3d6 100644
--- a/arch/x86/include/asm/acrn.h
+++ b/arch/x86/include/asm/acrn.h
@@ -35,6 +35,9 @@ static inline long acrn_hypercall0(unsigned long hcall_id)
        long result;

        register unsigned long r8 asm ("r8") = hcall_id;
+
+       pr_warn_ratelimited("acrn: issuing hypercall = %ld\n", hcall_id);
+
        asm volatile("vmcall"
                     : "=a" (result)
                     : "r" (r8)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index fad8faa29d04..1b6e13a8bb9d 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -33,6 +33,7 @@
 #include <asm/kvm_para.h>              /* kvm_handle_async_pf          */
 #include <asm/vdso.h>                  /* fixup_vdso_exception()       */
 #include <asm/irq_stack.h>
+#include <asm/acrn.h>

 #define CREATE_TRACE_POINTS
 #include <asm/trace/exceptions.h>
@@ -1542,3 +1543,9 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)

        irqentry_exit(regs, state);
 }
+
+
+long fake_acrn_example(int hcall)
+{
+       return acrn_hypercall0(hcall);
+}

Compile tested, but the below appears to generate sane code, e.g. reloads r8 when
given:

long fake_acrn_example(long hcall)
{
	pr_warn_ratelimited("acrn: attempting hcall2 for %ld\n", hcall);
	if (acrn_hypercall2(hcall, 0, 0))
		return -EINVAL;

	pr_warn_ratelimited("acrn: attempting hcall1 for %ld\n", hcall);
	if (acrn_hypercall1(hcall, 0))
		return -EINVAL;

	pr_warn_ratelimited("acrn: attempting hcall0 for %ld\n", hcall);
	return acrn_hypercall0(hcall);
}

---
From: Uros Bizjak <ubizjak@gmail.com>
Date: Thu, 4 Aug 2022 20:03:58 +0200
Subject: [PATCH] x86/acrn: Use "register" input constraint to set r8 for ACRN
 hypercalls

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;

Use the above approach instead of explicit MOV to R8 at the beginning
of the asm. The relaxed assignment allows compiler to optimize and
shrink drivers/virt/acrn.o for 181 bytes:

   text    data     bss     dec     hex filename
   4284     208       0    4492    118c hsm-new.o
   4465     208       0    4673    1241 hsm-old.o

Wrap the register approach in a macro as using "register" for inputs is
subtly dangerous.  If the compiler detects that the register (r8) would
be clobbered between setting it (declaration) and consuming it (asm), the
compiler is apparently allowed to ignore the input.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/acrn.h | 50 +++++++++++++++----------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/acrn.h b/arch/x86/include/asm/acrn.h
index e003a01b7c67..0817415f6d74 100644
--- a/arch/x86/include/asm/acrn.h
+++ b/arch/x86/include/asm/acrn.h
@@ -21,6 +21,23 @@ static inline u32 acrn_cpuid_base(void)
 	return 0;
 }

+/*
+  * Do not put code between the r8 register declaration and its usage in asm(),
+  * the compiler is allowed to ignore the register input if r8 would be
+  * clobbered before the asm() blob, e.g. by a function call.
+  */
+#define ACRN_HYPERCALL(id, params...)			\
+({							\
+	long result;					\
+	register unsigned long r8 asm ("r8") = id;	\
+							\
+	asm volatile("vmcall"				\
+		     : "=a" (result)			\
+		     : "r" (r8), ##params		\
+		     : "memory");			\
+	result;						\
+})
+
 /*
  * Hypercalls for ACRN
  *
@@ -29,50 +46,23 @@ static inline u32 acrn_cpuid_base(void)
  *   - Hypercall number is passed in R8 register.
  *   - Up to 2 arguments are passed in RDI, RSI.
  *   - Return value will be placed in RAX.
- *
- * Because GCC doesn't support R8 register as direct register constraints, use
- * supported constraint as input with a explicit MOV to R8 in beginning of asm.
  */
 static inline long acrn_hypercall0(unsigned long hcall_id)
 {
-	long result;
-
-	asm volatile("movl %1, %%r8d\n\t"
-		     "vmcall\n\t"
-		     : "=a" (result)
-		     : "g" (hcall_id)
-		     : "r8", "memory");
-
-	return result;
+	return ACRN_HYPERCALL(hcall_id);
 }

 static inline long acrn_hypercall1(unsigned long hcall_id,
 				   unsigned long param1)
 {
-	long result;
-
-	asm volatile("movl %1, %%r8d\n\t"
-		     "vmcall\n\t"
-		     : "=a" (result)
-		     : "g" (hcall_id), "D" (param1)
-		     : "r8", "memory");
-
-	return result;
+	return ACRN_HYPERCALL(hcall_id, "D" (param1));
 }

 static inline long acrn_hypercall2(unsigned long hcall_id,
 				   unsigned long param1,
 				   unsigned long param2)
 {
-	long result;
-
-	asm volatile("movl %1, %%r8d\n\t"
-		     "vmcall\n\t"
-		     : "=a" (result)
-		     : "g" (hcall_id), "D" (param1), "S" (param2)
-		     : "r8", "memory");
-
-	return result;
+	return ACRN_HYPERCALL(hcall_id, "D" (param1), "S" (param2));
 }

 #endif /* _ASM_X86_ACRN_H */

base-commit: 91dd4df51571bbea4d31e265cfbd59a85e0876be
--