Re: [Xen-devel] [PATCH] x86/hvm: Set the emulation context correctly in hvmemul_cmpxchg()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/05/2016 04:43 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 15:19, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1031,13 +1031,17 @@ static int hvmemul_cmpxchg(
>>  
>>      if ( unlikely(hvmemul_ctxt->set_context) )
>>      {
>> -        int rc = set_context_data(p_new, bytes);
>> +        int rc = set_context_data(p_old, bytes);
> 
> So this addresses one half of the problem mentioned, but not the
> other: You're still (unlike in all other cases where set_context_data()
> is being used) modifying data owned by the caller, which may end
> in it getting confused. I admit the semantics of the ->cmpxchg()
> callback aren't well defined, but current behavior is clearly to leave
> both buffers untouched even if at least p_new can't be constified.
> And if p_old was meant to get modified in any way, it clearly could
> only be to return back the old value an actual CMPXCHG may have
> found in memory (which afaict could be different from whatever
> override the introspection app may have enforced).

I understand. I'll just remove the set_context code then, and the newly
added comment along with it, and send out V2.

>>          if ( rc != X86EMUL_OKAY )
>>              return rc;
>>      }
>>  
>> -    /* Fix this in case the guest is really relying on r-m-w atomicity. */
>> +    /*
>> +     * Fix this in case the guest is really relying on r-m-w atomicity.
>> +     * Please note that while the set_context code is provided here for
>> +     * consistency, p_old is unused.
>> +     */
>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>  }
> 
> So with this I wonder btw. why your patch (mostly) fixing this
> shortcoming (while adding proper LOCK handling) never made it
> to a version that could be committed.

I was under the impression that your stand on the rwlock patch had
remained that you prefer a stub version to it, for possible performance
reasons, hence I've not pressed the issue. If I've misunderstood I'm
happy to try to rework it for staging.

I thought that the only acceptable solution was adding an actual stub
running on the physical VCPU, and unfortunately I didn't get to work one
out, in part because I had to tackle other issues, and partly because
it's not very clear how to go about that in this case.


Thanks,
Razvan

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