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

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

[Andrew Cooper]

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

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

[Jan Beulich]

On 21.05.2025 16:36, Andrew Cooper wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Andrew Cooper]

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

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

[Nicola Vetrini]

On 2025-05-21 20:00, Andrew Cooper wrote:
> 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.
> 

Hi Andrew,

R1.1 is easy to fix, I'll send a patch myself. On R2.6 you are probably 
right. I suggest marking the rule not clean to unblock while we 
investigate. It should not be hard to fix this FP.

Thanks,
  Nicola
> ~Andrew

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Andrew Cooper]

On 21/05/2025 8:21 pm, Nicola Vetrini wrote:
> On 2025-05-21 20:00, Andrew Cooper wrote:
>> 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.
>>
>
> Hi Andrew,
>
> R1.1 is easy to fix, I'll send a patch myself. On R2.6 you are
> probably right. I suggest marking the rule not clean to unblock while
> we investigate. It should not be hard to fix this FP.

I've not committed the patch yet, so staging is still green.

But, it occurs to me that this is not the first asm goto() to be scanned
by Eclair.  There's wrmsr_amd_safe() in amd.c (c/s b3d8b3e3f3aa from ~6w
ago) using exactly the same pattern, which has been passing just fine.

Clearly something else is relevant in terms of triggering violations.

~Andrew

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

[Nicola Vetrini]

On 2025-05-21 21:43, Andrew Cooper wrote:
> On 21/05/2025 8:21 pm, Nicola Vetrini wrote:
>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>> 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.
>>> 
>> 
>> Hi Andrew,
>> 
>> R1.1 is easy to fix, I'll send a patch myself. On R2.6 you are
>> probably right. I suggest marking the rule not clean to unblock while
>> we investigate. It should not be hard to fix this FP.
> 
> I've not committed the patch yet, so staging is still green.
> 
> But, it occurs to me that this is not the first asm goto() to be 
> scanned
> by Eclair.  There's wrmsr_amd_safe() in amd.c (c/s b3d8b3e3f3aa from 
> ~6w
> ago) using exactly the same pattern, which has been passing just fine.
> 
> Clearly something else is relevant in terms of triggering violations.
> 
> ~Andrew

I think the reason it's simply that file being out of scope due to being 
imported from Linux (whether that is still true I don't know), which is 
unfortunate. We ought to revise that list 
(docs/misra/exclude-list.json).

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Andrew Cooper]

On 21/05/2025 8:48 pm, Nicola Vetrini wrote:
> On 2025-05-21 21:43, Andrew Cooper wrote:
>> On 21/05/2025 8:21 pm, Nicola Vetrini wrote:
>>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>>> 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.
>>>>
>>>
>>> Hi Andrew,
>>>
>>> R1.1 is easy to fix, I'll send a patch myself. On R2.6 you are
>>> probably right. I suggest marking the rule not clean to unblock while
>>> we investigate. It should not be hard to fix this FP.
>>
>> I've not committed the patch yet, so staging is still green.
>>
>> But, it occurs to me that this is not the first asm goto() to be scanned
>> by Eclair.  There's wrmsr_amd_safe() in amd.c (c/s b3d8b3e3f3aa from ~6w
>> ago) using exactly the same pattern, which has been passing just fine.
>>
>> Clearly something else is relevant in terms of triggering violations.
>>
>> ~Andrew
>
> I think the reason it's simply that file being out of scope due to
> being imported from Linux (whether that is still true I don't know),
> which is unfortunate. We ought to revise that list
> (docs/misra/exclude-list.json).
>

Oh, that.  Anyone who takes a cursory look at amd.c will find it has
diverged entirely from Linux.

If I were an examiner, I wouldn't accept such a claim for being out of
scope...

~Andrew

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

[Nicola Vetrini]

On 2025-05-21 20:00, Andrew Cooper wrote:
> 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.
> 

That has been fixed upstream. I'll reach out to you when that fix 
trickles down to the runners, so that you're able to test and push that 
change.

Thanks,
  Nicola

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

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Andrew Cooper]

On 22/05/2025 1:45 pm, Nicola Vetrini wrote:
> On 2025-05-21 20:00, Andrew Cooper wrote:
>> 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.
>>
>
> That has been fixed upstream. I'll reach out to you when that fix
> trickles down to the runners, so that you're able to test and push
> that change.

Oh, fantastic thanks.

I'll hold off pushing until then.

~Andrew

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

[Nicola Vetrini]

On 2025-05-22 15:49, Andrew Cooper wrote:
> On 22/05/2025 1:45 pm, Nicola Vetrini wrote:
>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>> 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.
>>> 
>> 
>> That has been fixed upstream. I'll reach out to you when that fix
>> trickles down to the runners, so that you're able to test and push
>> that change.
> 
> Oh, fantastic thanks.
> 
> I'll hold off pushing until then.
> 
> ~Andrew

Hi Andrew,

both runners are now updated with the new images, so you can rerun the 
tests.

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Andrew Cooper]

On 25/05/2025 8:34 am, Nicola Vetrini wrote:
> On 2025-05-22 15:49, Andrew Cooper wrote:
>> On 22/05/2025 1:45 pm, Nicola Vetrini wrote:
>>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>>> 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.
>>>>
>>>
>>> That has been fixed upstream. I'll reach out to you when that fix
>>> trickles down to the runners, so that you're able to test and push
>>> that change.
>>
>> Oh, fantastic thanks.
>>
>> I'll hold off pushing until then.
>>
>> ~Andrew
>
> Hi Andrew,
>
> both runners are now updated with the new images, so you can rerun the
> tests.

Both Eclair runs are unhappy, and not even completing analysis.

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1835567532

This is the same branch as before, plus your config change for R1.1

~Andrew

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

[Nicola Vetrini]

On 2025-05-25 12:52, Andrew Cooper wrote:
> On 25/05/2025 8:34 am, Nicola Vetrini wrote:
>> On 2025-05-22 15:49, Andrew Cooper wrote:
>>> On 22/05/2025 1:45 pm, Nicola Vetrini wrote:
>>>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>>>> 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.
>>>>> 
>>>> 
>>>> That has been fixed upstream. I'll reach out to you when that fix
>>>> trickles down to the runners, so that you're able to test and push
>>>> that change.
>>> 
>>> Oh, fantastic thanks.
>>> 
>>> I'll hold off pushing until then.
>>> 
>>> ~Andrew
>> 
>> Hi Andrew,
>> 
>> both runners are now updated with the new images, so you can rerun the
>> tests.
> 
> Both Eclair runs are unhappy, and not even completing analysis.
> 
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1835567532
> 
> This is the same branch as before, plus your config change for R1.1
> 
> ~Andrew

There might be something wrong with the image I used, it's erroring on a 
speculative call to the compiler. I rolled back to the previous images 
(without the fix for R2.6). I will investigate tomorrow.

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

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

[Nicola Vetrini]

On 2025-05-25 15:36, Nicola Vetrini wrote:
> On 2025-05-25 12:52, Andrew Cooper wrote:
>> On 25/05/2025 8:34 am, Nicola Vetrini wrote:
>>> On 2025-05-22 15:49, Andrew Cooper wrote:
>>>> On 22/05/2025 1:45 pm, Nicola Vetrini wrote:
>>>>> On 2025-05-21 20:00, Andrew Cooper wrote:
>>>>>> 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.
>>>>>> 
>>>>> 
>>>>> That has been fixed upstream. I'll reach out to you when that fix
>>>>> trickles down to the runners, so that you're able to test and push
>>>>> that change.
>>>> 
>>>> Oh, fantastic thanks.
>>>> 
>>>> I'll hold off pushing until then.
>>>> 
>>>> ~Andrew
>>> 
>>> Hi Andrew,
>>> 
>>> both runners are now updated with the new images, so you can rerun 
>>> the
>>> tests.
>> 
>> Both Eclair runs are unhappy, and not even completing analysis.
>> 
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1835567532
>> 
>> This is the same branch as before, plus your config change for R1.1
>> 
>> ~Andrew
> 
> There might be something wrong with the image I used, it's erroring on 
> a speculative call to the compiler. I rolled back to the previous 
> images (without the fix for R2.6). I will investigate tomorrow.

Fixed, I will let you know when runners are updated. It was a change 
unrelated to Rule 2.6.

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253