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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx> 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?

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 will set vma->vm_private_data to 1 iff it is currently NULL and
returns true success iff the old values was NULL (although you might
want to double check my logic on the return value).

As Konrad said though using a symbolic constant for the 1 would be a
good idea.

I'm not sure if the cmpxchg is so expensive to be worth special casing
XENFEAT_auto_translated_physmap. It'd probably be fine to just
unconditionally use cmpxchg even in the other case, I don't think this
this path is so hot that it would matter.

>         }
>         return (xchg(&vma->vm_private_data, (void *)1) == NULL);
> }
> 
> Then in pvh_privcmd_resv_pfns():
> 
>         BUG_ON(vma->vm_private_data);
>       vma->vm_private_data = pvhp;

With the above this becomes:
        BUG_ON(vma->vm_private_data != 1);
        vma->vm_private_data = pvhp;

I previously thought this assignment was unsafe, but
privcmd_enforce_singleshot_mapping is always called first and ensures
that only one thread ever gets to this part and that v_p_d is always 1
if that happens, so I think it is likely be OK.  A comment to that
effect would be helpful.

Ian.


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