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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/02/18 16:49, Jan Beulich wrote:
>>>> 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?

Ok.

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

cmpxchg_local() would be better than with a trailing underscore.

Seeing as it matches the Linux naming scheme, using exactly
cmpxchg_local() would be the logical move.

~Andrew

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