Hi Keir,
I did an almost identical patch to yours for xsd_kva mmap. You''ll find
it below. I just saw the conflict when updating, and think that we
may want to merge approaches. Main difference is in populating the
ops, and making xsd_kva rdwr. The first is just cleanup, the second I
don''t believe is functional in your version. Can''t open rdwr
and mmap
PROT_READ|PROT_WRITE and 0400 file. Also, I just drop the read interface
entirely, as it''s no longer needed. I''ll follow this up with
the actual
diff between the two.
thanks,
-chris
--
Make xsd_kva provide its own mmap interface. Eliminates reliance on
/dev/kmem which is not installed by default in some distros.
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c | 28 +++++++++--------
tools/xenstore/xenstored_domain.c | 23 +++----------
2 files changed, 21 insertions(+), 30 deletions(-)
diff -r 1c46091df7ce linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c Fri Mar 3 19:06:50
2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c Sat Mar 4 17:31:03
2006 -0800
@@ -951,18 +951,22 @@
}
+static int xsd_kva_mmap (struct file *filp, struct vm_area_struct *vma)
+{
+ unsigned long size = vma->vm_end - vma->vm_start;
+ unsigned long pfn = mfn_to_pfn(xen_start_info->store_mfn);
+
+ if (vma->vm_pgoff || size > PAGE_SIZE)
+ return -EINVAL;
+
+ return remap_pfn_range(vma, vma->vm_start, pfn , size,
vma->vm_page_prot);
+}
+
+static struct file_operations xsd_kva_fops = {
+ .mmap = xsd_kva_mmap,
+};
static struct proc_dir_entry *xsd_kva_intf;
static struct proc_dir_entry *xsd_port_intf;
-
-
-static int xsd_kva_read(char *page, char **start, off_t off,
- int count, int *eof, void *data)
-{
- int len;
- len = sprintf(page, "0x%p",
mfn_to_virt(xen_start_info->store_mfn));
- *eof = 1;
- return len;
-}
static int xsd_port_read(char *page, char **start, off_t off,
int count, int *eof, void *data)
@@ -1027,8 +1031,8 @@
xen_start_info->store_evtchn = op.u.alloc_unbound.port;
/* And finally publish the above info in /proc/xen */
- if((xsd_kva_intf = create_xen_proc_entry("xsd_kva", 0400)))
- xsd_kva_intf->read_proc = xsd_kva_read;
+ if((xsd_kva_intf = create_xen_proc_entry("xsd_kva", 0600)))
+ xsd_kva_intf->proc_fops = &xsd_kva_fops;
if((xsd_port_intf = create_xen_proc_entry("xsd_port", 0400)))
xsd_port_intf->read_proc = xsd_port_read;
}
diff -r 1c46091df7ce tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c Fri Mar 3 19:06:50 2006 +0100
+++ b/tools/xenstore/xenstored_domain.c Sat Mar 4 17:31:03 2006 -0800
@@ -466,21 +466,8 @@
{
int rc, fd;
evtchn_port_t port;
- unsigned long kva;
char str[20];
struct domain *dom0;
-
- fd = open(XENSTORED_PROC_KVA, O_RDONLY);
- if (fd == -1)
- return -1;
-
- rc = read(fd, str, sizeof(str));
- if (rc == -1)
- goto outfd;
- str[rc] = ''\0'';
- kva = strtoul(str, NULL, 0);
-
- close(fd);
fd = open(XENSTORED_PROC_PORT, O_RDONLY);
if (fd == -1)
@@ -494,14 +481,14 @@
close(fd);
- dom0 = new_domain(NULL, 0, port);
-
- fd = open(_PATH_KMEM, O_RDWR);
+ fd = open(XENSTORED_PROC_KVA, O_RDWR);
if (fd == -1)
return -1;
- dom0->interface = mmap(NULL, getpagesize(), PROT_READ|PROT_WRITE,
- MAP_SHARED, fd, kva);
+ dom0 = new_domain(NULL, 0, port);
+
+ dom0->interface = mmap(NULL, getpagesize(), PROT_READ|PROT_WRITE,
MAP_SHARED,
+ fd, 0);
if (dom0->interface == MAP_FAILED)
goto outfd;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 6 Mar 2006, at 18:30, Chris Wright wrote:> I did an almost identical patch to yours for xsd_kva mmap. You''ll find > it below. I just saw the conflict when updating, and think that we > may want to merge approaches. Main difference is in populating the > ops, and making xsd_kva rdwr. The first is just cleanup, the second I > don''t believe is functional in your version. Can''t open rdwr and mmap > PROT_READ|PROT_WRITE and 0400 file. Also, I just drop the read > interface > entirely, as it''s no longer needed. I''ll follow this up with the > actual > diff between the two.We''ll keep the current method for populating fops, as we want to keep the read function on xsd_kva. I''ve checked in the fix for permissions (to 0600). The existing code seemed to work okay anyway, probably because xenstored runs as root and so permissions get ignored on open(). But it''s a good fix to get in the tree anyway. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 6 Mar 2006, at 18:30, Chris Wright wrote:> I did an almost identical patch to yours for xsd_kva mmap. You''ll find > it below. I just saw the conflict when updating, and think that we > may want to merge approaches. Main difference is in populating the > ops, and making xsd_kva rdwr. The first is just cleanup, the second I > don''t believe is functional in your version. Can''t open rdwr and mmap > PROT_READ|PROT_WRITE and 0400 file. Also, I just drop the read > interface > entirely, as it''s no longer needed. I''ll follow this up with the > actual > diff between the two.Also, your patch would need to set vma->vm_pgoff before calling remap_pfn_range(), or the unmap code will incorrectly mess with page reference counts (because the skanky VM_PFNMAP check in vm_normal_page would not trigger and so that function will return non-NULL). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:> > On 6 Mar 2006, at 18:30, Chris Wright wrote: > > >I did an almost identical patch to yours for xsd_kva mmap. You''ll find > >it below. I just saw the conflict when updating, and think that we > >may want to merge approaches. Main difference is in populating the > >ops, and making xsd_kva rdwr. The first is just cleanup, the second I > >don''t believe is functional in your version. Can''t open rdwr and mmap > >PROT_READ|PROT_WRITE and 0400 file. Also, I just drop the read > >interface > >entirely, as it''s no longer needed. I''ll follow this up with the > >actual > >diff between the two. > > Also, your patch would need to set vma->vm_pgoff before calling > remap_pfn_range(), or the unmap code will incorrectly mess with page > reference counts (because the skanky VM_PFNMAP check in vm_normal_page > would not trigger and so that function will return non-NULL).Thanks for the eyes, esp. considering that''s what the original fix was all about ;-) thanks, -chris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 6 Mar 2006, at 19:11, Chris Wright wrote:>> Also, your patch would need to set vma->vm_pgoff before calling >> remap_pfn_range(), or the unmap code will incorrectly mess with page >> reference counts (because the skanky VM_PFNMAP check in vm_normal_page >> would not trigger and so that function will return non-NULL). > > Thanks for the eyes, esp. considering that''s what the original fix was > all about ;-)Actually, now I look again, it appears that remap_pfn_range() deals with the re-setting of vm_pgoff itself (conditional on is_cow_mapping(), and prefixed with an enormous comment). So re-setting in the caller is unnecessary and your patch is just fine. I think there are one or two other places in Xen code that I munge vm_pgoff, so I guess I can clean up a little now. :-) Sorry about that... -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:> > On 6 Mar 2006, at 19:11, Chris Wright wrote: > > >>Also, your patch would need to set vma->vm_pgoff before calling > >>remap_pfn_range(), or the unmap code will incorrectly mess with page > >>reference counts (because the skanky VM_PFNMAP check in vm_normal_page > >>would not trigger and so that function will return non-NULL). > > > >Thanks for the eyes, esp. considering that''s what the original fix was > >all about ;-) > > Actually, now I look again, it appears that remap_pfn_range() deals > with the re-setting of vm_pgoff itself (conditional on > is_cow_mapping(), and prefixed with an enormous comment). So re-setting > in the caller is unnecessary and your patch is just fine.Well, I did test it for the page count problem, and it passed. But started to question my read of the code based on your original comments above.> I think there are one or two other places in Xen code that I munge > vm_pgoff, so I guess I can clean up a little now. :-) > > Sorry about that...No problem, thanks for the clarification. -chris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel