RE: [Xen-devel] [PATCH] xc_map_foreign_pages(), a convenient alternative to xc_map_foreign_batch()

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

Just curious why this can't be fixed in kernel driver? Is there case that 
caller is tolerable with partial mapping?

Also why do you need an extra allocation and copy to arr?

Thanks,
Kevin

>From: Markus Armbruster
>Sent: 2007年9月6日 0:37
>
>xc_map_foreign_batch() can succeed partially.  It is awkward to use
>when you're only interested in complete success.  Provide new
>xc_map_foreign_pages() convenience function for that kind of use.
>Also convert two obvious calls to use it.
>
>Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
>
>---
>The patch converts only those uses of xc_map_foreign_batch() to
>xc_map_foreign_pages() where I'm 100% sure it's safe.  For the others,
>I'd rather have a second opinion from somebody familiar with the code.
>
>Discussion of uses:
>
>* qemu_remap_bucket() in tools/ioemu/vl.c
>
>  Tests for mapping failure, although in a slightly sloppy way:
>
>      !(pfns[i + --j] & 0xF0000000UL)
>
>  This is true when any of the four bits is set.  But mapping failed
>  only when all of them are set, so maybe it should do this instead:
>
>      (pfns[i + --j] & 0xF0000000UL) != 0xF0000000UL
>
>* main() in tools/ioemu/vl.c, in __ia64__ code
>
>  Doesn't test for mapping failure.  Likely candidate for
>  xc_map_foreign_pages().
>
>* set_vram_mapping() in tools/ioemu/hw/cirrus_vga.c
>
>  Doesn't test for mapping failure.  Patch converts this one to
>  xc_map_foreign_pages().
>
>* copy_from_GFW_to_nvram() in tools/libxc/ia64/xc_ia64_hvm_build.c
>
>  Doesn't test for mapping failure.  Likely candidate for
>  xc_map_foreign_pages(), but I have no way to test it.
>
>* xc_core_arch_map_p2m() in tools/libxc/xc_core_x86.c
>
>  This maps a two level page directory: first a a page of pfns for the
>  page directory (live_p2m_frame_list_list), using that pages of pfns
>  for the pages themselves (live_p2m_frame_list), and using that the
>  actual pages.
>
>  Doesn't test for mapping failure.  Likely candidate for
>  xc_map_foreign_pages().
>
>* xc_domain_restore() in tools/libxc/xc_domain_restore.c
>
>  Three uses, none of them tests for mapping failure.
>
>  I don't understand this code.
>
>  The first use of xc_map_foreign_batch() appears to deliberately pass
>  invalid PFNs.
>
>* map_and_save_p2m_table() in tools/libxc/xc_domain_save.c
>
>  Similar to xc_core_arch_map_p2m().
>
>* xc_domain_save() in tools/libxc/xc_domain_save.c
>
>  Doesn't test for mapping failure.  Likely candidate for
>  xc_map_foreign_pages().
>
>* xc_domain_resume_any() in tools/libxc/xc_resume.c
>
>  Similar to xc_core_arch_map_p2m().
>
>* xenfb_map_fb() in tools/xenfb/xenfb.c
>
>  Another map through a two level page directory, roughly similar to
>  xc_core_arch_map_p2m().
>
>  Doesn't test for mapping failure.  Patch converts this one to
>  xc_map_foreign_pages().
>
>
>diff -r 256160ff19b7 tools/ioemu/hw/cirrus_vga.c
>--- a/tools/ioemu/hw/cirrus_vga.c      Thu Aug 16 13:27:59 2007 +0100
>+++ b/tools/ioemu/hw/cirrus_vga.c      Wed Sep 05 17:38:50 2007 +0200
>@@ -2561,7 +2561,7 @@ static void *set_vram_mapping(unsigned l
>
>     set_mm_mapping(xc_handle, domid, nr_extents, 0, extent_start);
>
>-    vram_pointer = xc_map_foreign_batch(xc_handle, domid,
>+    vram_pointer = xc_map_foreign_pages(xc_handle, domid,
>
>PROT_READ|PROT_WRITE,
>                                         extent_start, nr_extents);
>     if (vram_pointer == NULL) {
>diff -r 256160ff19b7 tools/libxc/xc_misc.c
>--- a/tools/libxc/xc_misc.c    Thu Aug 16 13:27:59 2007 +0100
>+++ b/tools/libxc/xc_misc.c    Wed Sep 05 18:31:46 2007 +0200
>@@ -224,6 +224,39 @@ int xc_hvm_set_pci_link_route(
>     unlock_pages(&arg, sizeof(arg));
>
>     return rc;
>+}
>+
>+void *xc_map_foreign_pages(int xc_handle, uint32_t dom, int prot,
>+                           const xen_pfn_t *arr, int num)
>+{
>+    xen_pfn_t *pfn;
>+    void *res;
>+    int i;
>+
>+    pfn = malloc(num * sizeof(*pfn));
>+    if (!pfn)
>+        return NULL;
>+    memcpy(pfn, arr, num * sizeof(*pfn));
>+
>+    res = xc_map_foreign_batch(xc_handle, dom, prot, pfn, num);
>+    if (res) {
>+        for (i = 0; i < num; i++) {
>+            if ((pfn[i] & 0xF0000000UL) == 0xF0000000UL) {
>+                /*
>+                 * xc_map_foreign_batch() doesn't give us an error
>+                 * code, so we have to make one up.  May not be
>the
>+                 * appropriate one.
>+                 */
>+                errno = EINVAL;
>+                munmap(res, num * PAGE_SIZE);
>+                res = NULL;
>+                break;
>+            }
>+        }
>+    }
>+
>+    free(pfn);
>+    return res;
> }
>
> /*
>diff -r 256160ff19b7 tools/libxc/xenctrl.h
>--- a/tools/libxc/xenctrl.h    Thu Aug 16 13:27:59 2007 +0100
>+++ b/tools/libxc/xenctrl.h    Mon Sep 03 09:18:50 2007 +0200
>@@ -646,6 +646,14 @@ void *xc_map_foreign_range(int xc_handle
>                             int size, int prot,
>                             unsigned long mfn );
>
>+void *xc_map_foreign_pages(int xc_handle, uint32_t dom, int prot,
>+                           const xen_pfn_t *arr, int num );
>+
>+/**
>+ * Like xc_map_foreign_pages(), except it can succeeed partially.
>+ * When a page cannot be mapped, its PFN in @arr is or'ed with
>+ * 0xF0000000 to indicate the error.
>+ */
> void *xc_map_foreign_batch(int xc_handle, uint32_t dom, int prot,
>                            xen_pfn_t *arr, int num );
>
>diff -r 256160ff19b7 tools/xenfb/xenfb.c
>--- a/tools/xenfb/xenfb.c      Thu Aug 16 13:27:59 2007 +0100
>+++ b/tools/xenfb/xenfb.c      Wed Sep 05 17:39:05 2007 +0200
>@@ -398,21 +398,15 @@ static int xenfb_map_fb(struct xenfb_pri
>       if (!pgmfns || !fbmfns)
>               goto out;
>
>-      /*
>-       * Bug alert: xc_map_foreign_batch() can fail partly and
>-       * return a non-null value.  This is a design flaw.  When it
>-       * happens, we happily continue here, and later crash on
>-       * access.
>-       */
>       xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
>-      map = xc_map_foreign_batch(xenfb->xc, domid,
>+      map = xc_map_foreign_pages(xenfb->xc, domid,
>                                  PROT_READ, pgmfns, n_fbdirs);
>       if (map == NULL)
>               goto out;
>       xenfb_copy_mfns(mode, n_fbmfns, fbmfns, map);
>       munmap(map, n_fbdirs * XC_PAGE_SIZE);
>
>-      xenfb->pub.pixels = xc_map_foreign_batch(xenfb->xc, domid,
>+      xenfb->pub.pixels = xc_map_foreign_pages(xenfb->xc, domid,
>                               PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
>       if (xenfb->pub.pixels == NULL)
>               goto out;
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@xxxxxxxxxxxxxxxxxxx
>http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel