Re: [PATCH v1 7/8]: PVH privcmd changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Mon, 8 Oct 2012 10:21:42 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2012-10-05 at 22:22 +0100, Mukesh Rathor wrote:
> > On Fri, 5 Oct 2012 10:21:18 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Thu, 2012-10-04 at 19:20 +0100, Mukesh Rathor wrote:
> > > > On Thu, 4 Oct 2012 09:50:42 +0100
> > > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > > 
> > > > > 
> > > > > Won't that break because on the second call you will pass in
> > > > > the freshly allocated pointer and overwrite the exiting
> > > > > (useful) one with it?
> > > > 
> > > > No, for xlate, I just check for NULL. I didn't think it was big 
> > > > deal to special case xlate in this case. We got so many if
> > > > xlate cases already thru the code. It leaves the semantics easy
> > > > to understand: NULL == avail. 1 == locked PV. PTR == Locked
> > > > PVH. I'll add a comment this time :).
> > > 
> > > The transition from NULL => Locked PVH still needs to be done
> > > atomically and without clobbering any existing non-NULL value,
> > > otherwise it doesn't actually protect against multiple mappings
> > > like it is supposed to.
> 
> > Ok, changed it to, and tested it:
> > 
> > static int privcmd_enforce_singleshot_mapping(struct vm_area_struct
> > *vma) {
> >         if (xen_feature(XENFEAT_auto_translated_physmap)) {
> >                 int sz = sizeof(vma->vm_private_data);
> >                 return (!__cmpxchg(&vma->vm_private_data, NULL,
> > NULL, sz));
> 
> Passing NULL for both old and new values can't be right, can it? Did
> you test with something which tries to map twice?

well, if it's already set to pointer, then __cmpxchg would leave the ptr 
alone and return it. The function would then return false which would 
fail the api. OTOH, if it's NULL, it would continue and get set later.


> Also using cmpxchg instead of __cmpxchg includes the sizeof bit for
> you automatically and IIRC Coding-Style doesn't like () around return
> values.
> 
> So, I think you want:
> 	return !cmpxchg(&vma->vm_private_data, NULL, 1);

This would work also, and is prob better than to special condition
xlated.

thanks
Mukesh