| Message ID | 1475673558-30720-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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
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); }
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(-)