[PATCH 0 of 2] RFC blktap: Small changes to blktap + added a hook to clear ptes on implicit releases of VMAs.

[PATCH 0 of 2] RFC blktap: Small changes to blktap + added a hook to clear ptes on implicit releases of VMAs.

[Geoffrey Lefebvre]

Hi,

These patches contain changes to the kernel blktap code. The main change is the addition of a vma hook to clear ptes when linux implicitly releases a vma. This allows the block tap to release grants when a user level process such as tapdisk crashes. This fixes the problem of Xen killing a domain because of implicit grant unmapping.

The patches should apply cleanly to changeset 15080. Comments are most welcome.

Thanks,

geoffrey

[PATCH 1 of 2] Small changes to blktap: free memory on release, invalidate p2m entry when unmapping grants, check for alloc failure of idx_map, added DONTCOPY flag

[Geoffrey Lefebvre]

# HG changeset patch
# User Geoffrey Lefebvre <geoffrey@cs.ubc.ca>
# Date 1179788512 25200
# Node ID f5218394705702c1aaa02520be2ba5a9f2648f37
# Parent  9cf9027ec568929946a1ee9094821fdec1eb68ff
Small changes to blktap: free memory on release, invalidate p2m entry when unmapping grants, check for alloc failure of idx_map, added DONTCOPY flag.

diff -r 9cf9027ec568 -r f52183947057 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
--- a/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Sat May 19 16:41:48 2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Mon May 21 16:01:52 2007 -0700
@@ -477,6 +477,11 @@ static int blktap_open(struct inode *ino
 	info->idx_map = kmalloc(sizeof(unsigned long) * MAX_PENDING_REQS, 
 				GFP_KERNEL);
 	
+	if (info->idx_map == NULL) {
+	    goto fail_nomem;
+	}
+
+
 	if (idx > 0) {
 		init_waitqueue_head(&info->wait);
 		for (i = 0; i < MAX_PENDING_REQS; i++) 
@@ -510,16 +515,24 @@ static int blktap_release(struct inode *
 		zap_page_range(
 			info->vma, info->vma->vm_start, 
 			info->vma->vm_end - info->vma->vm_start, NULL);
+
+		kfree(info->vma->vm_private_data);
+
 		info->vma = NULL;
 	}
-	
+
+	if (info->idx_map) {
+		kfree(info->idx_map);
+		info->idx_map = NULL;
+	}
+
 	if ( (info->status != CLEANSHUTDOWN) && (info->blkif != NULL) ) {
 		if (info->blkif->xenblkd != NULL) {
 			kthread_stop(info->blkif->xenblkd);
 			info->blkif->xenblkd = NULL;
 		}
 		info->status = CLEANSHUTDOWN;
-	}	
+	}
 	return 0;
 }
 
@@ -590,6 +603,7 @@ static int blktap_mmap(struct file *filp
     
 	vma->vm_private_data = map;
 	vma->vm_flags |= VM_FOREIGN;
+	vma->vm_flags |= VM_DONTCOPY;
 
 	info->vma = vma;
 	info->ring_ok = 1;
@@ -885,6 +899,10 @@ static void fast_flush_area(pending_req_
 					    idx_to_kaddr(mmap_idx, k_idx, i),
 					    GNTMAP_host_map, khandle->kernel);
 			invcount++;
+
+			set_phys_to_machine(
+			    __pa(idx_to_kaddr(mmap_idx, k_idx, i))
+			    >> PAGE_SHIFT, INVALID_P2M_ENTRY);
 		}
 
 		if (khandle->user != INVALID_GRANT_HANDLE) {

[PATCH 2 of 2] Added a hook for clearing ptes when a vma is freed by the kernel. Adapted from gntdev

[Geoffrey Lefebvre]

# HG changeset patch
# User Geoffrey Lefebvre <geoffrey@cs.ubc.ca>
# Date 1179788664 25200
# Node ID 236647990e986bca16f150c2fca87c0ed1ebd729
# Parent  f5218394705702c1aaa02520be2ba5a9f2648f37
Added a hook for clearing ptes when a vma is freed by the kernel. Adapted from gntdev.

diff -r f52183947057 -r 236647990e98 linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c
--- a/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Mon May 21 16:01:52 2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/blktap/blktap.c	Mon May 21 16:04:24 2007 -0700
@@ -269,6 +269,16 @@ static inline int GET_NEXT_REQ(unsigned 
 	return INVALID_REQ;
 }
 
+static inline int OFFSET_TO_USR_IDX(int offset)
+{
+	return offset / BLKIF_MAX_SEGMENTS_PER_REQUEST;
+}
+
+static inline int OFFSET_TO_SEG(int offset)
+{
+	return offset % BLKIF_MAX_SEGMENTS_PER_REQUEST;
+}
+
 
 #define BLKTAP_INVALID_HANDLE(_g) \
     (((_g->kernel) == INVALID_GRANT_HANDLE) &&  \
@@ -295,8 +305,94 @@ static struct page *blktap_nopage(struct
 	return NOPAGE_SIGBUS;
 }
 
+static pte_t blktap_clear_pte(struct vm_area_struct *vma,
+			      unsigned long uvaddr,
+			      pte_t *ptep, int is_fullmm)
+{
+	pte_t copy = *ptep;
+	tap_blkif_t *info;
+	int offset, seg, usr_idx, pending_idx, mmap_idx;
+	unsigned long uvstart = vma->vm_start + (RING_PAGES << PAGE_SHIFT);
+	unsigned long kvaddr;
+	struct page **map;
+	struct page *pg;
+	struct grant_handle_pair *khandle;
+	struct gnttab_unmap_grant_ref unmap[2];
+	int count = 0;
+	static int print_warning = 1;
+
+	/*print a warning message once, if the hook gets called*/
+	if (print_warning) {
+		WPRINTK("Clear pte hook called!\n");
+		print_warning = 0;
+	}
+
+	/* If the address is before the start of the grant mapped region
+	 * or if vm_file is NULL (meaning mmap failed and we have nothing to do)
+	 */
+	if (uvaddr < uvstart || vma->vm_file == NULL)
+		return copy;
+
+	info = vma->vm_file->private_data;
+	map = vma->vm_private_data;
+
+	/* TODO Should these be changed to if statements? */
+	BUG_ON(!info);
+	BUG_ON(!info->idx_map);
+	BUG_ON(!map);
+
+	offset = (int) ((uvaddr - uvstart) >> PAGE_SHIFT);
+	usr_idx = OFFSET_TO_USR_IDX(offset);
+	seg = OFFSET_TO_SEG(offset);
+
+	pending_idx = MASK_PEND_IDX(ID_TO_IDX(info->idx_map[usr_idx]));
+	mmap_idx = ID_TO_MIDX(info->idx_map[usr_idx]);
+
+	kvaddr = idx_to_kaddr(mmap_idx, pending_idx, seg);
+	pg = pfn_to_page(__pa(kvaddr) >> PAGE_SHIFT);
+	ClearPageReserved(pg);
+	map[offset] = NULL;
+
+	khandle = &pending_handle(mmap_idx, pending_idx, seg);
+
+	if (khandle->kernel != INVALID_GRANT_HANDLE) {
+	
+		gnttab_set_unmap_op(&unmap[count], kvaddr, 
+				    GNTMAP_host_map, khandle->kernel);
+		count++;
+
+		set_phys_to_machine(__pa(kvaddr) >> PAGE_SHIFT, 
+				    INVALID_P2M_ENTRY);
+	}
+
+	if (khandle->user != INVALID_GRANT_HANDLE) {
+
+		BUG_ON(xen_feature(XENFEAT_auto_translated_physmap));
+
+		gnttab_set_unmap_op(&unmap[count], virt_to_machine(ptep), 
+				    GNTMAP_host_map 
+				    | GNTMAP_application_map 
+				    | GNTMAP_contains_pte,
+				    khandle->user);
+		count++;
+	}
+
+	if (count) {
+		int ret;
+
+		BLKTAP_INVALIDATE_HANDLE(khandle);
+
+		ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref,
+						unmap, count);
+		BUG_ON(ret);
+	}
+
+	return copy;
+}
+
 struct vm_operations_struct blktap_vm_ops = {
 	nopage:   blktap_nopage,
+	zap_pte:    blktap_clear_pte,
 };
 
 /******************************************************************
@@ -604,6 +700,10 @@ static int blktap_mmap(struct file *filp
 	vma->vm_private_data = map;
 	vma->vm_flags |= VM_FOREIGN;
 	vma->vm_flags |= VM_DONTCOPY;
+
+#ifdef CONFIG_X86
+	vma->vm_mm->context.has_foreign_mappings = 1;
+#endif
 
 	info->vma = vma;
 	info->ring_ok = 1;

Re: [PATCH 2 of 2] Added a hook for clearing ptes when a vma is freed by the kernel. Adapted from gntdev

[Isaku Yamahata]

Hi.

On Tue, May 22, 2007 at 07:17:56AM -0700, Geoffrey Lefebvre wrote:
> +static pte_t blktap_clear_pte(struct vm_area_struct *vma,
> +			      unsigned long uvaddr,
> +			      pte_t *ptep, int is_fullmm)
   [ snip ]
> +	if (khandle->user != INVALID_GRANT_HANDLE) {
> +
> +		BUG_ON(xen_feature(XENFEAT_auto_translated_physmap));
> +
> +		gnttab_set_unmap_op(&unmap[count], virt_to_machine(ptep), 
> +				    GNTMAP_host_map 
> +				    | GNTMAP_application_map 
> +				    | GNTMAP_contains_pte,
> +				    khandle->user);
> +		count++;
> +	}

'else pte_clear_full(...)' is necessary like gntdev for 
auto translated physmap mode.

-- 
yamahata

Re: [PATCH 2 of 2] Added a hook for clearing ptes when a vma is freed by the kernel. Adapted from gntdev

[Geoffrey Lefebvre]

Hi,

Thanks for the comment. I will update the patch.

geoffrey

On 5/23/07, Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> Hi.
>
> On Tue, May 22, 2007 at 07:17:56AM -0700, Geoffrey Lefebvre wrote:
> > +static pte_t blktap_clear_pte(struct vm_area_struct *vma,
> > +                           unsigned long uvaddr,
> > +                           pte_t *ptep, int is_fullmm)
>    [ snip ]
> > +     if (khandle->user != INVALID_GRANT_HANDLE) {
> > +
> > +             BUG_ON(xen_feature(XENFEAT_auto_translated_physmap));
> > +
> > +             gnttab_set_unmap_op(&unmap[count], virt_to_machine(ptep),
> > +                                 GNTMAP_host_map
> > +                                 | GNTMAP_application_map
> > +                                 | GNTMAP_contains_pte,
> > +                                 khandle->user);
> > +             count++;
> > +     }
>
> 'else pte_clear_full(...)' is necessary like gntdev for
> auto translated physmap mode.
>
> --
> yamahata
>