x86/msr: Rework wrmsr_safe() using asm goto()

[PATCH] x86/msr: Rework wrmsr_safe() using asm goto()

Posted by Andrew Cooper 5 months, 1 week ago

This avoids needing to hold rc in a register across the WRMSR, and in most
cases removes direct testing and branching based on rc, as the fault label can
be rearranged to directly land on the out-of-line block.

No functional change.

Resolves: https://gitlab.com/xen-project/xen/-/work_items/214
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/msr.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
index 0d3b1d637488..4c4f18b3a54d 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -69,20 +69,20 @@ static inline void wrmsr_ns(uint32_t msr, uint32_t lo, uint32_t hi)
 /* wrmsr with exception handling */
 static inline int wrmsr_safe(unsigned int msr, uint64_t val)
 {
-    int rc;
-    uint32_t lo, hi;
-    lo = (uint32_t)val;
-    hi = (uint32_t)(val >> 32);
-
-    __asm__ __volatile__(
-        "1: wrmsr\n2:\n"
-        ".section .fixup,\"ax\"\n"
-        "3: movl %5,%0\n; jmp 2b\n"
-        ".previous\n"
-        _ASM_EXTABLE(1b, 3b)
-        : "=&r" (rc)
-        : "c" (msr), "a" (lo), "d" (hi), "0" (0), "i" (-EFAULT));
-    return rc;
+    uint32_t lo = val, hi = val >> 32;
+
+    asm_inline goto (
+        "1: wrmsr\n\t"
+        _ASM_EXTABLE(1b, %l[fault])
+        :
+        : "a" (lo), "c" (msr), "d" (hi)
+        :
+        : fault );
+
+    return 0;
+
+ fault:
+    return -EFAULT;
 }
 
 static inline uint64_t msr_fold(const struct cpu_user_regs *regs)
-- 
2.39.5

[Eclair false positive] Re: [PATCH] x86/msr: Rework wrmsr_safe() using asm goto()

Posted by Andrew Cooper 5 months, 1 week ago

On 21/05/2025 3:36 pm, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index 0d3b1d637488..4c4f18b3a54d 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -69,20 +69,20 @@ static inline void wrmsr_ns(uint32_t msr, uint32_t lo, uint32_t hi)
>  /* wrmsr with exception handling */
>  static inline int wrmsr_safe(unsigned int msr, uint64_t val)
>  {
> -    int rc;
> -    uint32_t lo, hi;
> -    lo = (uint32_t)val;
> -    hi = (uint32_t)(val >> 32);
> -
> -    __asm__ __volatile__(
> -        "1: wrmsr\n2:\n"
> -        ".section .fixup,\"ax\"\n"
> -        "3: movl %5,%0\n; jmp 2b\n"
> -        ".previous\n"
> -        _ASM_EXTABLE(1b, 3b)
> -        : "=&r" (rc)
> -        : "c" (msr), "a" (lo), "d" (hi), "0" (0), "i" (-EFAULT));
> -    return rc;
> +    uint32_t lo = val, hi = val >> 32;
> +
> +    asm_inline goto (
> +        "1: wrmsr\n\t"
> +        _ASM_EXTABLE(1b, %l[fault])
> +        :
> +        : "a" (lo), "c" (msr), "d" (hi)
> +        :
> +        : fault );
> +
> +    return 0;
> +
> + fault:
> +    return -EFAULT;
>  }

It turns out this is the first piece of Eclair-scanned code using asm goto.

https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/10108558677
(The run also contained an equivalent change to xsetbv())

We're getting R1.1 and R2.6 violations.

R1.1 complains about [STD.adrslabl] "label address" not being valid C99.

R2.6 complains about unused labels.

I expect this means that Eclair doesn't know how to interpret asm goto()
yet.  The labels listed are reachable from inside the asm block.

From a qualification point of view, this allows for some extensive
optimisations dropping emitted code.

~Andrew