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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -1296,8 +1296,83 @@ static int hvmemul_cmpxchg(
>>>      bool lock,
>>>      struct x86_emulate_ctxt *ctxt)
>>>  {
>>> -    /* Fix this in case the guest is really relying on r-m-w atomicity. */
>>> -    return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>> +    struct vcpu *curr = current;
>>> +    unsigned long addr, reps = 1;
>>> +    uint32_t pfec = PFEC_page_present | PFEC_write_access;
>> I'm fairly certain from my pagetable work that passing PFEC_page_present
>> here is bogus, and I do have (eventual) plans to make the pagewalk
>> reject such values.
> Both here and in the subsequent RMW patch I'm simply following
> what hvmemul_write() does. I'd prefer all three to stay in sync.

Fair enough.

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

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

~Andrew

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