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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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