On 01/10/2011 05:14 PM, Konrad Rzeszutek Wilk wrote:
>> @@ -134,6 +133,7 @@ static void gntdev_add_map(struct gntdev_priv *priv,
>> struct grant_map *add)
>> {
>> struct grant_map *map;
>>
>> + spin_lock(&priv->lock);
>> list_for_each_entry(map, &priv->maps, next) {
>> if (add->index + add->count < map->index) {
>> list_add_tail(&add->next, &map->next);
>> @@ -144,8 +144,10 @@ static void gntdev_add_map(struct gntdev_priv *priv,
>> struct grant_map *add)
>> list_add_tail(&add->next, &priv->maps);
>>
>> done:
>> + add->priv = priv;
>> if (debug)
>> gntdev_print_maps(priv, "[new]", add->index);
>> + spin_unlock(&priv->lock);
>
> That looks like you are also fixing a bug?
The lock is just being pushed inside this function from its caller, to
make it easier to see where the lock was being held.
>> }
>>
>> static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv,
>> int index,
>> @@ -186,9 +188,10 @@ static int gntdev_del_map(struct grant_map *map)
>>
>> if (map->vma)
>> return -EBUSY;
>> - for (i = 0; i < map->count; i++)
>> - if (map->unmap_ops[i].handle)
>> - return -EBUSY;
>> + if (map->is_mapped)
>> + for (i = 0; i < map->count; i++)
>> + if (map->pginfo[i].handle)
>> + return -EBUSY;
>>
>> atomic_sub(map->count, &pages_mapped);
>> list_del(&map->next);
>> @@ -202,15 +205,10 @@ static void gntdev_free_map(struct grant_map *map)
>> if (!map)
>> return;
>>
>> - if (map->pages)
>> - for (i = 0; i < map->count; i++) {
>> - if (map->pages[i])
>> - __free_page(map->pages[i]);
>> - }
>> - kfree(map->pages);
>> - kfree(map->grants);
>> - kfree(map->map_ops);
>> - kfree(map->unmap_ops);
>> + for (i = 0; i < map->count; i++) {
>> + if (map->pages[i])
>> + __free_page(map->pages[i]);
>> + }
>> kfree(map);
>> }
>>
>> @@ -223,53 +221,99 @@ static int find_grant_ptes(pte_t *pte, pgtable_t
>> token, unsigned long addr, void
>> u64 pte_maddr;
>>
>> BUG_ON(pgnr >= map->count);
>> +
>> pte_maddr = arbitrary_virt_to_machine(pte).maddr;
>> + map->pginfo[pgnr].pte_maddr = pte_maddr;
>>
>> - gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr,
>> - GNTMAP_contains_pte | map->flags,
>> - map->grants[pgnr].ref,
>> - map->grants[pgnr].domid);
>> - gnttab_set_unmap_op(&map->unmap_ops[pgnr], pte_maddr,
>> - GNTMAP_contains_pte | map->flags,
>> - 0 /* handle */);
>> return 0;
>> }
>>
>> static int map_grant_pages(struct grant_map *map)
>> {
>> - int i, err = 0;
>> + int i, flags, err = 0;
>> + struct gnttab_map_grant_ref* map_ops = NULL;
>>
>> + flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
>
> I am not sure if the GNTMAP_contains_pte is correct here. Stefano mentioned
> that is used to determine how many arguments to put in the hypercall. Looking
> at the previous usage - it was only done on the unmap_op, while you enforce
> it on map_op too?
GNTMAP_contains_pte is used on PV guests (only) to indicate that the
argument contains PTEs that need to be modified, rather than PFNs to
remap. See patch 6 for the HVM case where this flag is not used.
Previous use had it on both map and unmap. This was clearer prior to
a reshuffling patch that moved it to the gnttab_set_map_op call.
>> + if (map->is_ro)
>> + flags |= GNTMAP_readonly;
>> +
>> + err = -ENOMEM;
>> + map_ops = kzalloc(sizeof(map_ops[0]) * map->count, GFP_TEMPORARY);
>> + if (!map_ops)
>> + goto out;
>> +
>> + for(i=0; i < map->count; i++) {
>> + gnttab_set_map_op(&map_ops[i], map->pginfo[i].pte_maddr, flags,
>> + map->pginfo[i].target.ref,
>> + map->pginfo[i].target.domid);
>> + }
>> if (debug)
>> printk("%s: map %d+%d\n", __FUNCTION__, map->index, map->count);
>> - err = gnttab_map_refs(map->map_ops, map->pages, map->count);
>> +
>> + err = gnttab_map_refs(map_ops, map->pages, map->count);
>> +
>> if (WARN_ON(err))
>> - return err;
>> + goto out;
>>
>> for (i = 0; i < map->count; i++) {
>> - if (map->map_ops[i].status)
>> + if (map_ops[i].status) {
>> + __free_page(map->pages[i]);
>> + map->pages[i] = NULL;
>> err = -EINVAL;
>> - map->unmap_ops[i].handle = map->map_ops[i].handle;
>> + } else {
>> + map->pginfo[i].handle = map_ops[i].handle;
>> + }
>> }
>> +
>> +out:
>> + kfree(map_ops);
>> return err;
>> }
>>
>> -static int unmap_grant_pages(struct grant_map *map, int offset, int pages)
>> +static void unmap_grant_pages(struct grant_map *map, int offset, int pages)
>> {
>> - int i, err = 0;
>> + int i, flags, err = 0;
>> + struct gnttab_unmap_grant_ref *unmap_ops;
>> + struct gnttab_unmap_grant_ref unmap_single;
>> +
>> + if (pages > 1) {
>> + unmap_ops = kzalloc(sizeof(unmap_ops[0]) * pages,
>> + GFP_TEMPORARY);
>> + if (unlikely(!unmap_ops)) {
>> + for(i=0; i < pages; i++)
>> + unmap_grant_pages(map, offset + i, 1);
>> + return;
>> + }
>> + } else {
>> + unmap_ops = &unmap_single;
>> + }
>> +
>> + flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
>> + if (map->is_ro)
>> + flags |= GNTMAP_readonly;
>>
>> + for(i=0; i < pages; i++)
>> + gnttab_set_unmap_op(&unmap_ops[i],
>> + map->pginfo[offset+i].pte_maddr, flags,
>> + map->pginfo[offset+i].handle);
>> if (debug)
>> printk("%s: map %d+%d [%d+%d]\n", __FUNCTION__,
>> map->index, map->count, offset, pages);
>> - err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages, pages);
>> +
>> + err = gnttab_unmap_refs(unmap_ops, map->pages + offset, pages);
>> +
>> if (WARN_ON(err))
>> - return err;
>> + goto out;
>>
>> for (i = 0; i < pages; i++) {
>> - if (map->unmap_ops[offset+i].status)
>> - err = -EINVAL;
>> - map->unmap_ops[offset+i].handle = 0;
>> + WARN_ON(unmap_ops[i].status);
>
> Why change it from err to WARN_ON? I think the caller of this function
> checks for this too so you would end up with two WARN_ON?
No, unmap_ops[i].status is not ever seen by the caller. This function
returns void, so the caller doesn't have anything to warn about.
> Also, you don't add the offset value to i here..
unmap_ops (the local variable) is indexed without using offset.
>> + __free_page(map->pages[offset+i]);
>> + map->pages[offset+i] = NULL;
>> + map->pginfo[offset+i].handle = 0;
>> }
>> - return err;
>> +out:
>> + if (unmap_ops != &unmap_single)
>> + kfree(unmap_ops);
>> }
>>
>> /* ------------------------------------------------------------------ */
>> @@ -308,7 +352,6 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>> struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
>> struct grant_map *map;
>> unsigned long mstart, mend;
>> - int err;
>>
>> spin_lock(&priv->lock);
>> list_for_each_entry(map, &priv->maps, next) {
>> @@ -327,10 +370,9 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>> __FUNCTION__, map->index, map->count,
>> map->vma->vm_start, map->vma->vm_end,
>> start, end, mstart, mend);
>> - err = unmap_grant_pages(map,
>> - (mstart - map->vma->vm_start) >>
>> PAGE_SHIFT,
>> - (mend - mstart) >> PAGE_SHIFT);
>> - WARN_ON(err);
>> + unmap_grant_pages(map,
>> + (mstart - map->vma->vm_start) >> PAGE_SHIFT,
>> + (mend - mstart) >> PAGE_SHIFT);
>
> Ah, so you rememoved the WARN_ON here. What is the reason for doing so?
This makes it clearer where the error was located (in the hypercall return,
or in the status of some page). Since the return value of unmap_grant_pages
was never used except to pass to a WARN_ON, it seemed redundant.
>> }
>> spin_unlock(&priv->lock);
>> }
>> @@ -347,7 +389,6 @@ static void mn_release(struct mmu_notifier *mn,
>> {
>> struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
>> struct grant_map *map;
>> - int err;
>>
>> spin_lock(&priv->lock);
>> list_for_each_entry(map, &priv->maps, next) {
>> @@ -357,8 +398,7 @@ static void mn_release(struct mmu_notifier *mn,
>> printk("%s: map %d+%d (%lx %lx)\n",
>> __FUNCTION__, map->index, map->count,
>> map->vma->vm_start, map->vma->vm_end);
>> - err = unmap_grant_pages(map, 0, map->count);
>> - WARN_ON(err);
>> + unmap_grant_pages(map, 0, map->count);
>> }
>> spin_unlock(&priv->lock);
>> }
>> @@ -427,6 +467,7 @@ static long gntdev_ioctl_map_grant_ref(struct
>> gntdev_priv *priv,
>> {
>> struct ioctl_gntdev_map_grant_ref op;
>> struct grant_map *map;
>> + struct ioctl_gntdev_grant_ref* grants;
>> int err;
>>
>> if (copy_from_user(&op, u, sizeof(op)) != 0)
>> @@ -437,38 +478,45 @@ static long gntdev_ioctl_map_grant_ref(struct
>> gntdev_priv *priv,
>> if (unlikely(op.count <= 0))
>> return -EINVAL;
>>
>> - err = -ENOMEM;
>> - map = gntdev_alloc_map(priv, op.count);
>> - if (!map)
>> - return err;
>> + grants = kmalloc(sizeof(grants[0]) * op.count, GFP_TEMPORARY);
>> + if (!grants)
>> + return -ENOMEM;
>>
>> - if (copy_from_user(map->grants, &u->refs,
>> - sizeof(map->grants[0]) * op.count) != 0) {
>> - gntdev_free_map(map);
>> - return err;
>> + if (copy_from_user(grants, u->refs, sizeof(grants[0]) * op.count)) {
>> + err = -EFAULT;
>> + goto out_free;
>> }
>>
>> + err = -ENOMEM;
>> + map = gntdev_alloc_map(op.count, grants);
>> + if (!map)
>> + goto out_free;
>> +
>> if (unlikely(atomic_add_return(op.count, &pages_mapped) > limit))
>> {
>> if (debug)
>> printk("%s: can't map: over limit\n", __FUNCTION__);
>> - gntdev_free_map(map);
>> - return err;
>> + goto out_free_map;
>
> I just noticed it now, but shouldn't we also free grants? That looks like
> it needs a seperate bug patch thought.
It is getting freed: out_free_map goes to out_free after freeing the map.
>> }
>>
>> - spin_lock(&priv->lock);
>> gntdev_add_map(priv, map);
>> op.index = map->index << PAGE_SHIFT;
>> - spin_unlock(&priv->lock);
>
> Ah, so you moved the spinlock down. I presume it is OK to have op.index be
> unprotected.
Yep, op is on the stack.
>>
>> - if (copy_to_user(u, &op, sizeof(op)) != 0) {
>> - spin_lock(&priv->lock);
>> - gntdev_del_map(map);
>> - spin_unlock(&priv->lock);
>> - gntdev_free_map(map);
>> - return err;
>> + if (copy_to_user(u, &op, sizeof(op))) {
>> + err = -EFAULT;
>> + goto out_remove;
>
> Hmm, should we free the grants on this exit path?
We are; out_remove falls to out_free_map falls to out_free.
>> }
>> - return 0;
>> + err = 0;
>> +out_free:
>> + kfree(grants);
>> + return err;
>> +out_remove:
>> + spin_lock(&priv->lock);
>> + gntdev_del_map(map);
>> + spin_unlock(&priv->lock);
>> +out_free_map:
>> + gntdev_free_map(map);
>> + goto out_free;
>> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home