[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH 4/4] linux-2.6.18/gntdev: misc cleanup


  • To: "xen-devel" <xen-devel@xxxxxxxxxxxxx>
  • From: "Jan Beulich" <JBeulich@xxxxxxxx>
  • Date: Thu, 10 May 2012 16:19:06 +0100
  • Delivery-date: Thu, 10 May 2012 15:18:54 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

- eliminate struct gntdev_grant_info's dev_bus_addr member (which was
  used to communicate a value inside the main loop of gntdev_mmap())
- correct types
- use a local variable 'grants' in gntdev_mmap() to improve readability
- avoid re-calculating already calculated values
- properly check for out of range values
- combine both GNTTABOP_unmap_grant_ref calls in gntdev_clear_pte()
  into a single hypercall
- adjust error diagnostic in unmap ioctl to be more useful and better
  legible

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/drivers/xen/gntdev/gntdev.c
+++ b/drivers/xen/gntdev/gntdev.c
@@ -80,7 +80,6 @@ typedef struct gntdev_grant_info {
                        grant_ref_t ref;
                        grant_handle_t kernel_handle;
                        grant_handle_t user_handle;
-                       uint64_t dev_bus_addr;
                } valid;
        } u;
 } gntdev_grant_info_t;
@@ -473,14 +472,14 @@ static int gntdev_mmap (struct file *fli
 {
        struct gnttab_map_grant_ref op;
        unsigned long slot_index = vma->vm_pgoff;
-       unsigned long kernel_vaddr, user_vaddr;
-       uint32_t size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+       unsigned long kernel_vaddr, user_vaddr, mfn;
+       unsigned long size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
        uint64_t ptep;
        int ret, exit_ret;
-       int flags;
-       int i;
+       unsigned int i, flags;
        struct page *page;
        gntdev_file_private_data_t *private_data = flip->private_data;
+       gntdev_grant_info_t *grants;
        struct vm_foreign_map *foreign_map;
 
        if (unlikely(!private_data)) {
@@ -490,17 +489,19 @@ static int gntdev_mmap (struct file *fli
 
        /* Test to make sure that the grants array has been initialised. */
        down_read(&private_data->grants_sem);
-       if (unlikely(!private_data->grants)) {
-               up_read(&private_data->grants_sem);
+       grants = private_data->grants;
+       up_read(&private_data->grants_sem);
+
+       if (unlikely(!grants)) {
                printk(KERN_ERR "Attempted to mmap before ioctl.\n");
                return -EINVAL;
        }
-       up_read(&private_data->grants_sem);
+       grants += slot_index;
 
-       if (unlikely((size <= 0) || 
-                    (size + slot_index) > private_data->grants_size)) {
+       if (unlikely(size + slot_index <= slot_index ||
+                    size + slot_index > private_data->grants_size)) {
                printk(KERN_ERR "Invalid number of pages or offset"
-                      "(num_pages = %d, first_slot = %ld).\n",
+                      "(num_pages = %lu, first_slot = %lu)\n",
                       size, slot_index);
                return -ENXIO;
        }
@@ -521,11 +522,10 @@ static int gntdev_mmap (struct file *fli
        /* Slots must be in the NOT_YET_MAPPED state. */
        down_write(&private_data->grants_sem);
        for (i = 0; i < size; ++i) {
-               if (private_data->grants[slot_index + i].state != 
-                   GNTDEV_SLOT_NOT_YET_MAPPED) {
-                       printk(KERN_ERR "Slot (index = %ld) is in the wrong "
+               if (grants[i].state != GNTDEV_SLOT_NOT_YET_MAPPED) {
+                       printk(KERN_ERR "Slot (index = %lu) is in the wrong "
                               "state (%d).\n", slot_index + i, 
-                              private_data->grants[slot_index + i].state);
+                              grants[i].state);
                        up_write(&private_data->grants_sem);
                        kfree(foreign_map);
                        return -EINVAL;
@@ -565,14 +565,11 @@ static int gntdev_mmap (struct file *fli
                        flags |= GNTMAP_readonly;
 
                kernel_vaddr = get_kernel_vaddr(private_data, slot_index + i);
-               user_vaddr = get_user_vaddr(vma, i);
                page = private_data->foreign_pages[slot_index + i];
 
                gnttab_set_map_op(&op, kernel_vaddr, flags,   
-                                 private_data->grants[slot_index+i]
-                                 .u.valid.ref, 
-                                 private_data->grants[slot_index+i]
-                                 .u.valid.domid);
+                                 grants[i].u.valid.ref,
+                                 grants[i].u.valid.domid);
 
                /* Carry out the mapping of the grant reference. */
                ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, 
@@ -583,10 +580,8 @@ static int gntdev_mmap (struct file *fli
                                printk(KERN_ERR "Error mapping the grant 
reference "
                                       "into the kernel (%d). domid = %d; ref = 
%d\n",
                                       op.status,
-                                      private_data->grants[slot_index+i]
-                                      .u.valid.domid,
-                                      private_data->grants[slot_index+i]
-                                      .u.valid.ref);
+                                      grants[i].u.valid.domid,
+                                      grants[i].u.valid.ref);
                        else
                                /* Propagate eagain instead of trying to fix it 
up */
                                exit_ret = -EAGAIN;
@@ -597,16 +592,14 @@ static int gntdev_mmap (struct file *fli
                SetPageReserved(page);
 
                /* Record the grant handle, for use in the unmap operation. */
-               private_data->grants[slot_index+i].u.valid.kernel_handle = 
-                       op.handle;
-               private_data->grants[slot_index+i].u.valid.dev_bus_addr = 
-                       op.dev_bus_addr;
+               grants[i].u.valid.kernel_handle = op.handle;
                
-               private_data->grants[slot_index+i].state = GNTDEV_SLOT_MAPPED;
-               private_data->grants[slot_index+i].u.valid.user_handle =
-                       GNTDEV_INVALID_HANDLE;
+               grants[i].state = GNTDEV_SLOT_MAPPED;
+               grants[i].u.valid.user_handle = GNTDEV_INVALID_HANDLE;
 
                /* Now perform the mapping to user space. */
+               user_vaddr = get_user_vaddr(vma, i);
+
                if (xen_feature(XENFEAT_auto_translated_physmap)) {
                        /* USING SHADOW PAGE TABLES. */
                        /* In this case, we simply insert the page into the VM
@@ -622,11 +615,11 @@ static int gntdev_mmap (struct file *fli
                /* In this case, we map the grant(s) straight into user
                 * space.
                 */
+               mfn = op.dev_bus_addr >> PAGE_SHIFT;
 
                /* Get the machine address of the PTE for the user page. */
                if ((ret = create_lookup_pte_addr(vma->vm_mm,
-                                                 vma->vm_start
-                                                 + (i << PAGE_SHIFT),
+                                                 user_vaddr,
                                                  &ptep)))
                {
                        printk(KERN_ERR "Error obtaining PTE pointer (%d)\n",
@@ -636,9 +629,6 @@ static int gntdev_mmap (struct file *fli
 
                /* Configure the map operation. */
 
-               /* The reference is to be used by host CPUs. */
-               flags = GNTMAP_host_map;
-
                /* Specifies a user space mapping. */
                flags |= GNTMAP_application_map;
 
@@ -647,14 +637,9 @@ static int gntdev_mmap (struct file *fli
                 */
                flags |= GNTMAP_contains_pte;
 
-               if (!(vma->vm_flags & VM_WRITE))
-                       flags |= GNTMAP_readonly;
-
                gnttab_set_map_op(&op, ptep, flags,
-                                 private_data->grants[slot_index+i]
-                                 .u.valid.ref,
-                                 private_data->grants[slot_index+i]
-                                 .u.valid.domid);
+                                 grants[i].u.valid.ref,
+                                 grants[i].u.valid.domid);
 
                /* Carry out the mapping of the grant reference. */
                ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
@@ -664,10 +649,8 @@ static int gntdev_mmap (struct file *fli
                        printk(KERN_ERR "Error mapping the grant reference "
                               "into user space (%d). domid = %d; ref = %d\n",
                               op.status,
-                              private_data->grants[slot_index+i].u
-                              .valid.domid,
-                              private_data->grants[slot_index+i].u
-                              .valid.ref);
+                              grants[i].u.valid.domid,
+                              grants[i].u.valid.ref);
                        /* This should never happen after we've mapped into
                         * the kernel space. */
                        BUG_ON(op.status == GNTST_eagain);
@@ -677,15 +660,11 @@ static int gntdev_mmap (struct file *fli
                /* Record the grant handle, for use in the unmap
                 * operation.
                 */
-               private_data->grants[slot_index+i].u.valid.user_handle
-                       = op.handle;
+               grants[i].u.valid.user_handle = op.handle;
 
                /* Update p2m structure with the new mapping. */
                set_phys_to_machine(__pa(kernel_vaddr) >> PAGE_SHIFT,
-                                   FOREIGN_FRAME(private_data->
-                                                 grants[slot_index+i]
-                                                 .u.valid.dev_bus_addr
-                                                 >> PAGE_SHIFT));
+                                   FOREIGN_FRAME(mfn));
        }
        exit_ret = 0;
 
@@ -715,9 +694,11 @@ undo_map_out:
 static pte_t gntdev_clear_pte(struct vm_area_struct *vma, unsigned long addr,
                              pte_t *ptep, int is_fullmm)
 {
-       int slot_index, ret;
+       int ret;
+       unsigned int nr;
+       unsigned long slot_index;
        pte_t copy;
-       struct gnttab_unmap_grant_ref op;
+       struct gnttab_unmap_grant_ref op[2];
        gntdev_file_private_data_t *private_data;
 
        /* THIS IS VERY UNPLEASANT: do_mmap_pgoff() will set the vma->vm_file
@@ -750,36 +731,35 @@ static pte_t gntdev_clear_pte(struct vm_
                return copy;
        }
 
-       /* First, we clear the user space mapping, if it has been made.
-        */
+       /* Clear the user space mapping, if it has been made. */
        if (private_data->grants[slot_index].u.valid.user_handle !=
-           GNTDEV_INVALID_HANDLE &&
-           !xen_feature(XENFEAT_auto_translated_physmap)) {
-               /* NOT USING SHADOW PAGE TABLES. */
-               gnttab_set_unmap_op(&op, ptep_to_machine(ptep),
+           GNTDEV_INVALID_HANDLE) {
+               /* NOT USING SHADOW PAGE TABLES (and user handle valid). */
+               gnttab_set_unmap_op(&op[0], ptep_to_machine(ptep),
                                    GNTMAP_contains_pte,
                                    private_data->grants[slot_index].u.valid
                                    .user_handle);
-               ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref,
-                                               &op, 1);
-               BUG_ON(ret);
-               if (op.status != GNTST_okay)
-                       printk("User unmap grant status = %d\n", op.status);
+               nr = 1;
        } else {
-               /* USING SHADOW PAGE TABLES. */
+               /* USING SHADOW PAGE TABLES (or user handle invalid). */
                pte_clear_full(vma->vm_mm, addr, ptep, is_fullmm);
+               nr = 0;
        }
 
-       /* Finally, we unmap the grant from kernel space. */
-       gnttab_set_unmap_op(&op,
+       /* We always unmap the grant from kernel space. */
+       gnttab_set_unmap_op(&op[nr],
                            get_kernel_vaddr(private_data, slot_index),
                            GNTMAP_host_map,
                            private_data->grants[slot_index].u.valid
                            .kernel_handle);
-       ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
+
+       ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, op, nr + 1);
        BUG_ON(ret);
-       if (op.status != GNTST_okay)
-               printk("Kernel unmap grant status = %d\n", op.status);
+
+       if (nr && op[0].status != GNTST_okay)
+               printk("User unmap grant status = %d\n", op[0].status);
+       if (op[nr].status != GNTST_okay)
+               printk("Kernel unmap grant status = %d\n", op[nr].status);
 
        /* Return slot to the not-yet-mapped state, so that it may be
         * mapped again, or removed by a subsequent ioctl.
@@ -915,7 +895,8 @@ private_data_initialised:
                        return -EFAULT;
 
                start_index = op.index >> PAGE_SHIFT;
-               if (start_index + op.count > private_data->grants_size)
+               if (start_index + op.count < start_index ||
+                   start_index + op.count > private_data->grants_size)
                        return -EINVAL;
 
                down_write(&private_data->grants_sem);
@@ -924,28 +905,29 @@ private_data_initialised:
                 * state.
                 */
                for (i = 0; i < op.count; ++i) {
-                       if (unlikely
-                           (private_data->grants[start_index + i].state
-                            != GNTDEV_SLOT_NOT_YET_MAPPED)) {
-                               if (private_data->grants[start_index + i].state
-                                   == GNTDEV_SLOT_INVALID) {
-                                       printk(KERN_ERR
-                                              "Tried to remove an invalid "
-                                              "grant at offset 0x%x.",
-                                              (start_index + i) 
-                                              << PAGE_SHIFT);
-                                       rc = -EINVAL;
-                               } else {
-                                       printk(KERN_ERR
-                                              "Tried to remove a grant which "
-                                              "is currently mmap()-ed at "
-                                              "offset 0x%x.",
-                                              (start_index + i) 
-                                              << PAGE_SHIFT);
-                                       rc = -EBUSY;
-                               }
-                               goto unmap_out;
+                       const char *what;
+
+                       switch (private_data->grants[start_index + i].state) {
+                       case GNTDEV_SLOT_NOT_YET_MAPPED:
+                               continue;
+                       case GNTDEV_SLOT_INVALID:
+                               what = "invalid";
+                               rc = -EINVAL;
+                               break;
+                       case GNTDEV_SLOT_MAPPED:
+                               what = "currently mmap()-ed";
+                               rc = -EBUSY;
+                               break;
+                       default:
+                               what = "in an invalid state";
+                               rc = -ENXIO;
+                               break;
                        }
+                       printk(KERN_ERR "%s[%d] tried to remove a grant"
+                              " which is %s at %#x+%#x\n",
+                              current->comm, current->pid,
+                              what, start_index, i);
+                       goto unmap_out;
                }
 
                down_write(&private_data->free_list_sem);


Attachment: xen-gntdev-cleanup.patch
Description: Text document

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.