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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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