Re: [Xen-devel] [PATCH V2] x86/emulate: synchronize LOCKed instruction emulation

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/15/2017 06:57 PM, Jan Beulich wrote:
>>>> On 15.03.17 at 17:46, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 03/15/2017 06:30 PM, Jan Beulich wrote:
>>>>>> On 15.03.17 at 17:04, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> ---
>>>> Changes since V1:
>>>>  - Added Andrew Cooper's credit, as he's kept the patch current
>>>>    througout non-trivial code changes since the initial patch.
>>>>  - Significantly more patch testing (with XenServer).
>>>>  - Restricted lock scope.
>>>
>>> Not by much, as it seems. In particular you continue to take the
>>> lock even for instructions not accessing memory at all.
>>
>> I'll take a closer look.
>>
>>> Also, by "reworked" I did assume you mean converted to at least the
>>> cmpxchg based model.
>>
>> I haven't been able to follow the latest emulator changes closely, could
>> you please clarify what you mean by "the cmpxchg model"? Thanks.
> 
> This is unrelated to any recent changes. The idea is to make the
> ->cmpxchg() hook actually behave like what its name says. It's
> being used for LOCKed insn writeback already, and it could
> therefore simply force a retry of the full instruction if the compare
> part of it fails. It may need to be given another parameter, to
> allow the hook function to tell LOCKed from "normal" uses.

I assume this is what you have in mind?


static int hvmemul_cmpxchg(
    enum x86_segment seg,
    unsigned long offset,
    void *p_old,
    void *p_new,
    unsigned int bytes,
    struct x86_emulate_ctxt *ctxt)
{
    /* Fix this in case the guest is really relying on r-m-w atomicity. */
    uint64_t read;
    int rc;

    rc = hvmemul_read(seg, offset, &read, bytes, ctxt);

    if ( rc != X86EMUL_OKAY )
        return rc;

    switch( bytes )
    {
    case 1:
        if ( *(uint8_t *)read != *(uint8_t *)p_old )
        {
            *(uint8_t *)p_old = *(uint8_t *)&read;
            return X86EMUL_RETRY;
        }
        break;
    case 2:
        if ( *(uint16_t *)read != *(uint16_t *)p_old )
        {
            *(uint16_t *)p_old = *(uint16_t *)&read;
            return X86EMUL_RETRY;
        }
        break;
    case 4:
        if ( *(uint32_t *)&read != *(uint32_t *)p_old )
        {
            *(uint32_t *)p_old = *(uint32_t *)&read;
            return X86EMUL_RETRY;
        }
        break;
    case 8:
        if ( read != *(uint64_t *)p_old )
        {
            *(uint64_t *)p_old = read;
            return X86EMUL_RETRY;
        }
        break;
    }

    return hvmemul_write(seg, offset, p_new, bytes, ctxt);
}

I can rip this off and send this one small patch independently of the
larger LOCK one if you think it's a good idea.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel