Reducing LOC (always a good thing) by eliminating duplicated functionality. vmx_platform.c/inst_copy_from_guest() now uses vmx_copy. Also shored up vmx_copy to handle copies when paging is enabled and improved its error handling. Signed-Off-By: Leendert van Doorn <leendert@watson.ibm.com> diff -r 23d8580d56b0 xen/arch/x86/vmx.c --- a/xen/arch/x86/vmx.c Fri Sep 2 18:23:57 2005 +++ b/xen/arch/x86/vmx.c Mon Sep 5 13:43:59 2005 @@ -729,7 +735,7 @@ int vmx_copy(void *buf, unsigned long laddr, int size, int dir) { - unsigned long mfn; + unsigned long gpa, mfn; char *addr; int count; @@ -738,8 +744,14 @@ if (count > size) count = size; - mfn = get_mfn_from_pfn(laddr >> PAGE_SHIFT); - /* XXX check whether laddr is valid */ + if (vmx_paging_enabled(current)) { + gpa = gva_to_gpa(laddr); + mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); + } else + mfn = get_mfn_from_pfn(laddr >> PAGE_SHIFT); + if (mfn == INVALID_MFN) + return 0; + addr = (char *)map_domain_page(mfn) + (laddr & ~PAGE_MASK); if (dir == VMX_COPY_IN) diff -r 23d8580d56b0 xen/arch/x86/vmx_platform.c --- a/xen/arch/x86/vmx_platform.c Fri Sep 2 18:23:57 2005 +++ b/xen/arch/x86/vmx_platform.c Mon Sep 5 13:43:59 2005 @@ -583,49 +583,13 @@ } } -/* XXX use vmx_copy instead */ int inst_copy_from_guest(unsigned char *buf, unsigned long guest_eip, int inst_len) { - unsigned long gpa; - unsigned long mfn; - unsigned char *inst_start; - int remaining = 0; - - if ( (inst_len > MAX_INST_LEN) || (inst_len <= 0) ) + if (inst_len > MAX_INST_LEN || inst_len <= 0) return 0; - - if ( vmx_paging_enabled(current) ) - { - gpa = gva_to_gpa(guest_eip); - mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); - - /* Does this cross a page boundary ? */ - if ( (guest_eip & PAGE_MASK) != ((guest_eip + inst_len) & PAGE_MASK) ) - { - remaining = (guest_eip + inst_len) & ~PAGE_MASK; - inst_len -= remaining; - } - } - else - { - mfn = get_mfn_from_pfn(guest_eip >> PAGE_SHIFT); - } - - inst_start = map_domain_page(mfn); - memcpy((char *)buf, inst_start + (guest_eip & ~PAGE_MASK), inst_len); - unmap_domain_page(inst_start); - - if ( remaining ) - { - gpa = gva_to_gpa(guest_eip+inst_len+remaining); - mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); - - inst_start = map_domain_page(mfn); - memcpy((char *)buf+inst_len, inst_start, remaining); - unmap_domain_page(inst_start); - } - - return inst_len+remaining; + if (!vmx_copy(buf, guest_eip, inst_len, VMX_COPY_IN)) + return 0; + return inst_len; } void send_mmio_req(unsigned char type, unsigned long gpa, _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Leendert van Doorn
2005-Sep-06 10:22 UTC
Re: [Xen-devel] [PATCH] vmx-copy_from_guest.patch
# seems current vmx_copy can not deal with copy over page boundary? # so remove inst_copy_from_guest will cause problem. Are you looking at the correct tree? Both xen-vt-testing and xen-unstable have my newer vmx_copy that was introduced last Friday. It does handle multiple pages. int vmx_copy(void *buf, unsigned long laddr, int size, int dir) { unsigned long gpa, mfn; char *addr; int count; while (size > 0) { count = PAGE_SIZE - (laddr & ~PAGE_MASK); if (count > size) count = size; if (vmx_paging_enabled(current)) { gpa = gva_to_gpa(laddr); mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); } else mfn = get_mfn_from_pfn(laddr >> PAGE_SHIFT); if (mfn == INVALID_MFN) return 0; addr = (char *)map_domain_page(mfn) + (laddr & ~PAGE_MASK); if (dir == VMX_COPY_IN) memcpy(buf, addr, count); else memcpy(addr, buf, count); unmap_domain_page(addr); laddr += count; buf += count; size -= count; } return 1; } - Leendert -- Leendert van Doorn <leendert@watson.ibm.com> IBM T.J. Watson Research Center (914) 784-7831 30 Saw Mill River Road, Hawthorne, NY 10532 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
seems current vmx_copy can not deal with copy over page boundary? so remove inst_copy_from_guest will cause problem. Leendert van Doorn <> wrote:> Reducing LOC (always a good thing) by eliminating duplicated > functionality. > vmx_platform.c/inst_copy_from_guest() now uses vmx_copy. Also shored > up vmx_copy to handle copies when paging is enabled and improved its > error handling. > > Signed-Off-By: Leendert van Doorn <leendert@watson.ibm.com> > > diff -r 23d8580d56b0 xen/arch/x86/vmx.c > --- a/xen/arch/x86/vmx.c Fri Sep 2 18:23:57 2005 > +++ b/xen/arch/x86/vmx.c Mon Sep 5 13:43:59 2005 > @@ -729,7 +735,7 @@ > int > vmx_copy(void *buf, unsigned long laddr, int size, int dir) { > - unsigned long mfn; > + unsigned long gpa, mfn; > char *addr; > int count; > > @@ -738,8 +744,14 @@ > if (count > size) > count = size; > > - mfn = get_mfn_from_pfn(laddr >> PAGE_SHIFT); > - /* XXX check whether laddr is valid */ > + if (vmx_paging_enabled(current)) { > + gpa = gva_to_gpa(laddr); > + mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); > + } else > + mfn = get_mfn_from_pfn(laddr >> PAGE_SHIFT); > + if (mfn == INVALID_MFN) > + return 0; > + > addr = (char *)map_domain_page(mfn) + (laddr & ~PAGE_MASK); > > if (dir == VMX_COPY_IN) > diff -r 23d8580d56b0 xen/arch/x86/vmx_platform.c > --- a/xen/arch/x86/vmx_platform.c Fri Sep 2 18:23:57 2005 > +++ b/xen/arch/x86/vmx_platform.c Mon Sep 5 13:43:59 2005 > @@ -583,49 +583,13 @@ > } > } > > -/* XXX use vmx_copy instead */ > int inst_copy_from_guest(unsigned char *buf, unsigned long > guest_eip, int inst_len) { > - unsigned long gpa; > - unsigned long mfn; > - unsigned char *inst_start; > - int remaining = 0; > - > - if ( (inst_len > MAX_INST_LEN) || (inst_len <= 0) ) > + if (inst_len > MAX_INST_LEN || inst_len <= 0) > return 0; > - > - if ( vmx_paging_enabled(current) ) > - { > - gpa = gva_to_gpa(guest_eip); > - mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); > - > - /* Does this cross a page boundary ? */ > - if ( (guest_eip & PAGE_MASK) != ((guest_eip + inst_len) & > PAGE_MASK) ) > - { > - remaining = (guest_eip + inst_len) & ~PAGE_MASK; > - inst_len -= remaining; > - } > - } > - else > - { > - mfn = get_mfn_from_pfn(guest_eip >> PAGE_SHIFT); > - } > - > - inst_start = map_domain_page(mfn); > - memcpy((char *)buf, inst_start + (guest_eip & ~PAGE_MASK), > inst_len); > - unmap_domain_page(inst_start); > - > - if ( remaining ) > - { > - gpa = gva_to_gpa(guest_eip+inst_len+remaining); > - mfn = get_mfn_from_pfn(gpa >> PAGE_SHIFT); > - > - inst_start = map_domain_page(mfn); > - memcpy((char *)buf+inst_len, inst_start, remaining); > - unmap_domain_page(inst_start); > - } > - > - return inst_len+remaining; > + if (!vmx_copy(buf, guest_eip, inst_len, VMX_COPY_IN)) > + return 0; > + return inst_len; > } > > void send_mmio_req(unsigned char type, unsigned long gpa, > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ling, Xiaofeng <> wrote:> seems current vmx_copy can not deal with copy over page boundary? > so remove inst_copy_from_guest will cause problem. > > Leendert van Doorn <> wrote:The following code is I used to deal with hypercall parameters in vmx. It has no limit with copy length and page boundary. static inline void* map_domain_vaddr(void * guest_vaddr, unsigned long len) { unsigned long gpa, mfn; void * vstart; gpa = gva_to_gpa((unsigned long)guest_vaddr); mfn = get_mfn_from_pfn(gpa>>PAGE_SHIFT); vstart = (void *)map_domain_page(mfn) + ((unsigned long)guest_vaddr & (PAGE_SIZE - 1)); return vstart; } unsigned long copy_from_guest(void *to, const void __user *from, unsigned long n) { void *hfrom; unsigned long ncopy; int nleft; ncopy = (((unsigned long)from + PAGE_SIZE) & PAGE_MASK) - (unsigned long)from; ncopy = ncopy > n ? n : ncopy; for(nleft = n; nleft > 0; ncopy = nleft > PAGE_SIZE ? PAGE_SIZE : nleft) { hfrom = map_domain_vaddr((void*)from, ncopy); if(hfrom) { memcpy(to, hfrom, ncopy); unmap_domain_page((void*)hfrom); } else { printk("error!, copy from guest map error, from:%p, ncopy:%ld\n", from, ncopy); return nleft; } nleft -= ncopy; from += ncopy; to += ncopy; } return nleft; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
This code doesn''t work for non-paging situations. Also, the original inst_copy_from_guest is broken with respect to page-crossing when in "non-paging" mode, so if that can be fixed in any new version of code, that would be great. Having one piece of code that does all the types of copying is probably a good idea as long as the code isn''t hit so often that inlining the actual memcpy is of importance. But all the other stuff going on in either of these functions probably doesn''t make this very likely for the cases I''ve seen so far. -- Mats> -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of > Ling, Xiaofeng > Sent: 06 September 2005 15:42 > To: Leendert van Doorn; xen-devel@lists.xensource.com > Cc: Sharma, Arun; Ian Pratt > Subject: RE: [Xen-devel] [PATCH] vmx-copy_from_guest.patch > > > Ling, Xiaofeng <> wrote: > > seems current vmx_copy can not deal with copy over page boundary? > > so remove inst_copy_from_guest will cause problem. > > > > Leendert van Doorn <> wrote: > The following code is I used to deal with hypercall parameters in vmx. > It has no limit with copy length and page boundary. > > static inline void* map_domain_vaddr(void * guest_vaddr, > unsigned long len) { > unsigned long gpa, mfn; > void * vstart; > > gpa = gva_to_gpa((unsigned long)guest_vaddr); > mfn = get_mfn_from_pfn(gpa>>PAGE_SHIFT); > vstart = (void *)map_domain_page(mfn) + > ((unsigned long)guest_vaddr & (PAGE_SIZE - 1)); > > return vstart; > } > > unsigned long > copy_from_guest(void *to, const void __user *from, unsigned long n) { > void *hfrom; > unsigned long ncopy; > int nleft; > ncopy = (((unsigned long)from + PAGE_SIZE) & PAGE_MASK) - > (unsigned long)from; > ncopy = ncopy > n ? n : ncopy; > > for(nleft = n; nleft > 0; ncopy = nleft > PAGE_SIZE ? > PAGE_SIZE : nleft) > { > hfrom = map_domain_vaddr((void*)from, ncopy); > if(hfrom) > { > memcpy(to, hfrom, ncopy); > unmap_domain_page((void*)hfrom); > } > else > { > printk("error!, copy from guest map error, > from:%p, ncopy:%ld\n", > from, ncopy); > return nleft; > } > nleft -= ncopy; > from += ncopy; > to += ncopy; > } > return nleft; > } > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
leendert@watson.ibm.com <mailto:leendert@watson.ibm.com> wrote:> # seems current vmx_copy can not deal with copy over page boundary? > # so remove inst_copy_from_guest will cause problem. > > Are you looking at the correct tree? Both xen-vt-testing and > xen-unstable have my newer vmx_copy that was introduced last Friday. > It does handle multiple pages.ok. seems I look at the old code:) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel