On 12/14/2010 04:15 PM, Jeremy Fitzhardinge wrote:
> On 12/14/2010 06:55 AM, Daniel De Graaf wrote:
>> The entire hypercall argument list isn't required; only selected
>> fields from the hypercall need to be tracked between the ioctl, map,
>> and unmap operations.
>
> Is the rationale of this patch to save memory? If so, how much does it
> save.
>
> (This patch seems sensible in principle, but it doesn't seem to save
> much complexity.)
>
> J
This will also allow easier testing of what pages need to be unmapped
(more obvious in the HVM version). I also find it less confusing to
populate the hypercall arguments immediately before the hypercall, but
that's likely a matter of opinion. It only saves 46 bytes per page, so
if it seems more complex it could be dropped.
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>> drivers/xen/gntdev.c | 195
>> +++++++++++++++++++++++++++++--------------------
>> 1 files changed, 115 insertions(+), 80 deletions(-)
>>
>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>> index 62639af..773b76c 100644
>> --- a/drivers/xen/gntdev.c
>> +++ b/drivers/xen/gntdev.c
>> @@ -55,17 +55,23 @@ struct gntdev_priv {
>> struct mmu_notifier mn;
>> };
>>
>> +struct granted_page {
>> + u64 pte_maddr;
>> + union {
>> + struct ioctl_gntdev_grant_ref target;
>> + grant_handle_t handle;
>> + };
>> +};
>> +
>> struct grant_map {
>> struct list_head next;
>> struct gntdev_priv *priv;
>> struct vm_area_struct *vma;
>> int index;
>> int count;
>> - int flags;
>> - int is_mapped;
>> - struct ioctl_gntdev_grant_ref *grants;
>> - struct gnttab_map_grant_ref *map_ops;
>> - struct gnttab_unmap_grant_ref *unmap_ops;
>> + int is_mapped:1;
>> + int is_ro:1;
>> + struct granted_page pages[0];
>> };
>>
>> /* ------------------------------------------------------------------ */
>> @@ -83,40 +89,28 @@ static void gntdev_print_maps(struct gntdev_priv *priv,
>> map->index == text_index && text ? text : "");
>> }
>>
>> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int
>> count)
>> +static struct grant_map *gntdev_alloc_map(int count,
>> + struct ioctl_gntdev_grant_ref* grants)
>> {
>> struct grant_map *add;
>> + int i;
>>
>> - add = kzalloc(sizeof(struct grant_map), GFP_KERNEL);
>> - if (NULL == add)
>> + add = kzalloc(sizeof(*add) + sizeof(add->pages[0])*count, GFP_KERNEL);
>> + if (!add)
>> return NULL;
>>
>> - add->grants = kzalloc(sizeof(add->grants[0]) * count, GFP_KERNEL);
>> - add->map_ops = kzalloc(sizeof(add->map_ops[0]) * count, GFP_KERNEL);
>> - add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
>> - if (NULL == add->grants ||
>> - NULL == add->map_ops ||
>> - NULL == add->unmap_ops)
>> - goto err;
>> -
>> - add->index = 0;
>> add->count = count;
>> - add->priv = priv;
>> + for(i = 0; i < count; i++)
>> + add->pages[i].target = grants[i];
>>
>> return add;
>> -
>> -err:
>> - kfree(add->grants);
>> - kfree(add->map_ops);
>> - kfree(add->unmap_ops);
>> - kfree(add);
>> - return NULL;
>> }
>>
>> 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);
>> @@ -127,8 +121,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);
>> }
>>
>> static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv,
>> int index,
>> @@ -169,9 +165,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->pages[i].handle)
>> + return -EBUSY;
>>
>> atomic_sub(map->count, &pages_mapped);
>> list_del(&map->next);
>> @@ -182,9 +179,6 @@ static void gntdev_free_map(struct grant_map *map)
>> {
>> if (!map)
>> return;
>> - kfree(map->grants);
>> - kfree(map->map_ops);
>> - kfree(map->unmap_ops);
>> kfree(map);
>> }
>>
>> @@ -199,51 +193,91 @@ static int find_grant_ptes(pte_t *pte, pgtable_t
>> token, unsigned long addr, void
>> BUG_ON(pgnr >= map->count);
>> pte_maddr = (u64)pfn_to_mfn(page_to_pfn(token)) << PAGE_SHIFT;
>> pte_maddr += (unsigned long)pte & ~PAGE_MASK;
>> - gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr, map->flags,
>> - map->grants[pgnr].ref,
>> - map->grants[pgnr].domid);
>> - gnttab_set_unmap_op(&map->unmap_ops[pgnr], pte_maddr, map->flags,
>> - 0 /* handle */);
>> + map->pages[pgnr].pte_maddr = pte_maddr;
>> +
>> 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;
>>
>> + flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
>> + if (map->is_ro)
>> + flags |= GNTMAP_readonly;
>> +
>> + map_ops = kzalloc(sizeof(map_ops[0]) * map->count, GFP_TEMPORARY);
>> + if (!map_ops)
>> + return -ENOMEM;
>> +
>> + for(i=0; i < map->count; i++)
>> + gnttab_set_map_op(&map_ops[i], map->pages[i].pte_maddr, flags,
>> + map->pages[i].target.ref,
>> + map->pages[i].target.domid);
>> if (debug)
>> printk("%s: map %d+%d\n", __FUNCTION__, map->index, map->count);
>> err = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
>> - map->map_ops, map->count);
>> + map_ops, 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) {
>> + map->pages[i].pte_maddr = 0;
>> err = -EINVAL;
>> - map->unmap_ops[i].handle = map->map_ops[i].handle;
>> + } else {
>> + map->pages[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->pages[offset+i].pte_maddr, flags,
>> + map->pages[offset+i].handle);
>> if (debug)
>> printk("%s: map %d+%d [%d+%d]\n", __FUNCTION__,
>> map->index, map->count, offset, pages);
>> err = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref,
>> - map->unmap_ops + offset, pages);
>> + unmap_ops, 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);
>> + map->pages[offset+i].pte_maddr = 0;
>> + map->pages[offset+i].handle = 0;
>> }
>> - return err;
>> +out:
>> + if (unmap_ops != &unmap_single)
>> + kfree(unmap_ops);
>> }
>>
>> /* ------------------------------------------------------------------ */
>> @@ -282,7 +316,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) {
>> @@ -301,10 +334,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);
>> }
>> spin_unlock(&priv->lock);
>> }
>> @@ -321,7 +353,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) {
>> @@ -331,8 +362,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);
>> }
>> @@ -401,6 +431,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)
>> @@ -411,38 +442,45 @@ static long gntdev_ioctl_map_grant_ref(struct
>> gntdev_priv *priv,
>> if (unlikely(op.count <= 0))
>> return -EINVAL;
>>
>> + grants = kmalloc(sizeof(grants[0]) * op.count, GFP_TEMPORARY);
>> + if (!grants)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(grants, u->refs, sizeof(grants[0]) * op.count)) {
>> + err = -EFAULT;
>> + goto out_free;
>> + }
>> +
>> err = -ENOMEM;
>> - map = gntdev_alloc_map(priv, op.count);
>> + map = gntdev_alloc_map(op.count, grants);
>> if (!map)
>> - return err;
>> -
>> - if (copy_from_user(map->grants, &u->refs,
>> - sizeof(map->grants[0]) * op.count) != 0) {
>> - gntdev_free_map(map);
>> - return err;
>> - }
>> + 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;
>> }
>>
>> - spin_lock(&priv->lock);
>> gntdev_add_map(priv, map);
>> op.index = map->index << PAGE_SHIFT;
>> - spin_unlock(&priv->lock);
>>
>> - 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;
>> }
>> - 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;
>> }
>>
>> static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv,
>> @@ -556,10 +594,7 @@ static int gntdev_mmap(struct file *flip, struct
>> vm_area_struct *vma)
>>
>> vma->vm_private_data = map;
>> map->vma = vma;
>> -
>> - map->flags = GNTMAP_host_map | GNTMAP_application_map |
>> GNTMAP_contains_pte;
>> - if (!(vma->vm_flags & VM_WRITE))
>> - map->flags |= GNTMAP_readonly;
>> + map->is_ro = !(vma->vm_flags & VM_WRITE);
>>
>> spin_unlock(&priv->lock);
>>
>
--
Daniel De Graaf
National Security Agency
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home