Re: [Xen-devel] [PATCH v3 22/25] x86/HVM: do actual CMPXCHG in hvmemul_cmpxchg()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.18 at 17:09, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/02/18 08:32, Jan Beulich wrote:
>>>>> On 02.02.18 at 17:36, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 07/12/17 14:16, Jan Beulich wrote:
>>>> +    case 16:
>>>> +        if ( cpu_has_cx16 )
>>>> +        {
>>>> +            __uint128_t *old = p_old, cur;
>>>> +
>>>> +            if ( lock )
>>>> +                cur = __cmpxchg16b(mapping, old, p_new);
>>>> +            else
>>>> +                cur = cmpxchg16b_local_(mapping, old, p_new);
>>>> +            if ( cur != *old )
>>>> +            {
>>>> +                *old = cur;
>>>> +                rc = X86EMUL_CMPXCHG_FAILED;
>>>> +            }
>>>> +            break;
>>>> +        }
>>>> +        /* fall through */
>>>> +    default:
>>> ASSERT_UNREACHABLE() ?
>>>
>>>> +        rc = X86EMUL_UNHANDLEABLE;
>>>> +        break;
>>>> +    }
>> I'm not sure - from an abstract POV cpu_has_cx16 and the guest
>> seeing the feature available in its CPUID policy could differ. Granted
>> we're unlikely to want to try to emulate CMPXCHG16B without having
>> the instruction available ourselves, but it still wouldn't seem entirely
>> correct to assert here. I could remove the fall-through and _then_
>> assert in the default case only. Let me know.
> 
> The point was to catch bad sizes from being passed in.  There is only a
> single ancient range of 64bit processors which don't have CX16, but I'd
> still argue that it would be a bug for the emulator to pass 16 down in
> such cases.

So - are you fine then with my earlier suggestion towards an actual
change to make here?

>>>> --- a/xen/include/asm-x86/system.h
>>>> +++ b/xen/include/asm-x86/system.h
>>>> @@ -110,6 +110,38 @@ static always_inline unsigned long __cmp
>>>>      return old;
>>>>  }
>>>>  
>>>> +static always_inline unsigned long cmpxchg_local_(
>>> unlocked_cmpxchg() ?
>> Not in line with our current naming scheme.
> 
> Its rather more in line than introducing a local_ suffix.  Trailing
> underscores are almost non-existant, and as far as I can tell, used
> exclusively in the internals of the compat code.

Well, the name choice started from Linux'es cmpxchg_local(), of
which the function introduced here would be a helper. I'd like to
stick to the Linux inherited naming scheme (read: I want to keep
the "cmpxchg_local" part), but I don't insist on the trailing
underscore (which I only use here [and elsewhere] in preference
of name space violating leading ones). I'd just need a suggestion
towards an alternative you could live with, and fitting the outlined
criteria.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel