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

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

[Razvan Cojocaru]

hvmemul_cmpxchg() sets the read emulation context in p_new instead
of p_old, which is inconsistent (and wrong). We are now setting
p_old (even though it's unused) and adding a comment explaining
the change.

Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index d759d3f..0cbb16e 100644
--- 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);
 
         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);
 }
 
-- 
1.9.1


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

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

[Jan Beulich]

>>> On 05.10.16 at 15:19, <rcojocaru@bitdefender.com> 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).

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

Jan


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

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

[Razvan Cojocaru]

On 10/05/2016 04:43 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 15:19, <rcojocaru@bitdefender.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 05.10.16 at 15:54, <rcojocaru@bitdefender.com> wrote:
> On 10/05/2016 04:43 PM, Jan Beulich wrote:
>> 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.

Hmm, I have to admit I don't recall any stubs to be in the picture
here. What I recall is that the locked region was too large, and
covered cases which don't need a lock in the first place.

Jan


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

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

[Razvan Cojocaru]

On 10/05/2016 05:05 PM, Jan Beulich wrote:
>>>> On 05.10.16 at 15:54, <rcojocaru@bitdefender.com> wrote:
>> On 10/05/2016 04:43 PM, Jan Beulich wrote:
>>> 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.
> 
> Hmm, I have to admit I don't recall any stubs to be in the picture
> here. What I recall is that the locked region was too large, and
> covered cases which don't need a lock in the first place.

Andrew I think suggested a stub first:

https://lists.xenproject.org/archives/html/xen-devel/2016-04/msg02050.html

then George brought it up again:

https://lists.xenproject.org/archives/html/xen-devel/2016-04/msg03294.html

Andrew also talked about XenServer's performance testing with the
original patch:

https://lists.xenproject.org/archives/html/xen-devel/2016-04/msg03354.html

There's actually a version of it in XenServer's patch queue for 4.7:

https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-emulate-syncrhonise-LOCKed-instruction-emulation.patch

If there's no impediment, I'm happy to start working on it again.


Thanks,
Razvan

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