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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/21/2017 05:38 PM, Razvan Cojocaru wrote:
> 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;

Sorry, forgot to add & to "read" for the two cases above.


Thanks,
Razvan

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