Patches for PCI passthrough with modified E820 (v2).

The code libxl_device_pci_parse_bdf calls the libxl__e820_alloc when it has parsed one BDF. Subsequent calls to libxl_device_pci_parse_bdf will
not trigger libxl__e820_alloc being called (unless the first call to libxl__e820_alloc failed).

# HG changeset patch
# User Konrad Rzeszutek Wilk <[email protected]> # Date 1302295108 14400
# Node ID e7057fec103ba69776d157d470d630ce99dbc540 # Parent 0f279f43f41ffa8549e076f2a30f499bd8d6cc1d
x86: make the pv-only e820 array be dynamic.

During creation of the PV domain we allocate the E820 structure to have the amount of E820 entries on the machine, plus the number three.

Quoted 7 lines · Konrad Rzeszutek Wilk

# HG changeset patch
# User Konrad Rzeszutek Wilk <[email protected]> # Date 1302295108 14400
# Node ID e7057fec103ba69776d157d470d630ce99dbc540 # Parent 0f279f43f41ffa8549e076f2a30f499bd8d6cc1d
x86: make the pv-only e820 array be dynamic.

Quoted 2 lines

During creation of the PV domain we allocate the E820 structure to have the amount of E820 entries on the machine, plus the number three.

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

Quoted 4 lines

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

I was thinking about it, but the mm.c code did not have the xen/xmalloc.h header, nor any references to xmalloc_array.

Is it OK to make an xmalloc_array during a hypercall?

Quoted 5 lines · Konrad Rzeszutek Wilk

Quoted 4 lines

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

Quoted 4 lines

I was thinking about it, but the mm.c code did not have the xen/xmalloc.h header, nor any references to xmalloc_array.

Is it OK to make an xmalloc_array during a hypercall?

Yes. I think the toolstack should be able to clean up on the newly-possible ENOMEM return from this hypercall.

Quoted 7 lines · Konrad Rzeszutek Wilk

The code libxl_device_pci_parse_bdf calls the libxl__e820_alloc when it has parsed one BDF. Subsequent calls to libxl_device_pci_parse_bdf will
not trigger libxl__e820_alloc being called (unless the first call to libxl__e820_alloc failed).

That sounds like a very odd non-intuitive location for that allocation. Why not do it in libxl_domain_create or somewhere like that?

I think the e820 map added to the idl should become a simple boolean flag and this should all be taken care of internally based on that.

Quoted 9 lines · Ian Campbell

Quoted 7 lines · Konrad Rzeszutek Wilk

The code libxl_device_pci_parse_bdf calls the libxl__e820_alloc when it has parsed one BDF. Subsequent calls to libxl_device_pci_parse_bdf will
not trigger libxl__e820_alloc being called (unless the first call to libxl__e820_alloc failed).

Quoted 5 lines

That sounds like a very odd non-intuitive location for that allocation. Why not do it in libxl_domain_create or somewhere like that?

I think the e820 map added to the idl should become a simple boolean flag and this should all be taken care of internally based on that.

Like this?

# HG changeset patch
# User Konrad Rzeszutek Wilk <[email protected]>
# Date 1302617081 14400
# Node ID 945a34bcc2bd4bb341296f3ce84be6c42f24a4c9
# Parent 4527044d3d1cebdc393e596072bc02d947a8de51
libxl: Add support for passing in the machine's E820 for PCI passthrough

The code that populates E820 is unconditionally triggered by the guest configuration having "pci=['<BDF>,..']", being a PV guest, and if b_info->u.pv.no_machine_e820 is not set.

The code do_domain_create calls the libxl__e820_alloc when
it notices that the guest is PV, has at least one PCI devices, and does not have the no_machine_e820 flag set..

libxl__e820_alloc calls the xc_get_machine_memory_map to retrieve the systems E820. Then the E820 is sanitized to weed out E820 entries below 16MB, and as well remove any E820_RAM or E820_UNUSED regions as the guest does not need to know about them. The guest only needs the E820_ACPI, E820_NVS, E820_RESERVED to get an idea of where the PCI I/O space is. Mostly.. The Linux kernel assumes that any gap in the E820 is considered PCI I/O space which means that if we pass in the guest 2GB, and the E820_ACPI, and its friend start at 3GB, the gap between 2GB and 3GB will be considered as PCI I/O space. To guard against that we also create an E820_UNUSABLE between the region of 'target_kb' (called ram_end in the code) up to the first E820_[ACPI,NVS,RESERVED] region. Lastly, the xc_domain_set_memory_map is called to install the new E820.

When tested with another PV guest (NetBSD 5.1) the modified E820 gave it no trouble. The code has also been tested with older "classic" Xen Linux and with the newer "pvops" with success (SLES11, RHEL5, Ubuntu Lucid, Debian Squeeze, 2.6.37, 2.6.38, 2.6.39).

Memory that is slack or for balloon (so 'maxmem' in guest configuration) is put behind the machine E820. Which in most cases is after the 4GB.

The reason for doing the fetching of the E820 using the hypercall in the toolstack (instead of the guest doing it) is that when a guest would do a hypercall to 'XENMEM_machine_memory_map' it would retrieve an E820 with I/O range caps added in. Meaning that the region after 4GB up to end of possible memory would be marked as unusable and the kernel would not have any space to allocate a balloon region.

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

diff -r 4527044d3d1c -r 945a34bcc2bd tools/libxl/libxl.idl

tools/libxl/libxl.idl +1 -0

@@ -117,6 +117,7 @@ libxl_domain_build_info = Struct("domain
                                         ("cmdline", string),
                                         ("ramdisk", libxl_file_reference),
                                         ("features", string, True),
+                                        ("no_machine_e820", bool),
                                         ])),
                  ])),
     ],

diff -r 4527044d3d1c -r 945a34bcc2bd tools/libxl/libxl_create.c

tools/libxl/libxl_create.c +8 -0

@@ -536,6 +536,14 @@ static int do_domain_create(libxl__gc *g
     for (i = 0; i < d_config->num_pcidevs; i++)
         libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
 
+    if (!d_config->c_info.hvm && !d_config->b_info.u.pv.no_machine_e820) {
+        int rc = 0;
+        rc = libxl__e820_alloc(ctx, domid, d_config);
+        if (rc)
+            LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
+                      "Failed while collecting E820 with: %d (errno:%d)\n",
+                      rc, errno);
+    }
     if ( cb && (d_config->c_info.hvm || d_config->b_info.u.pv.bootloader )) {
         if ( (*cb)(ctx, domid, priv) )
             goto error_out;

diff -r 4527044d3d1c -r 945a34bcc2bd tools/libxl/libxl_internal.h

tools/libxl/libxl_internal.h +1 -0

@@ -335,4 +335,5 @@ _hidden int libxl__error_set(libxl__gc *
 _hidden int libxl__file_reference_map(libxl_file_reference *f);
 _hidden int libxl__file_reference_unmap(libxl_file_reference *f);
 
+_hidden int libxl__e820_alloc(libxl_ctx *ctx, uint32_t domid, libxl_domain_config *d_config);
 #endif

diff -r 4527044d3d1c -r 945a34bcc2bd tools/libxl/libxl_pci.c

tools/libxl/libxl_pci.c +154 -0

@@ -1047,3 +1047,157 @@ int libxl_device_pci_shutdown(libxl_ctx 
     free(pcidevs);
     return 0;
 }
+
+static int e820_sanitize(libxl_ctx *ctx, struct e820entry src[],
+                         uint32_t *nr_entries,
+                         unsigned long map_limitkb,
+                         unsigned long balloon_kb)
+{
+    uint64_t delta_kb = 0, start = 0, start_kb = 0, last = 0, ram_end;
+    uint32_t i, idx = 0, nr;
+    struct e820entry e820[E820MAX];
+
+    if (!src || !map_limitkb || !balloon_kb || !nr_entries)
+        return ERROR_INVAL;
+
+    nr = *nr_entries;
+    if (!nr)
+        return ERROR_INVAL;
+
+    if (nr > E820MAX)
+        return ERROR_NOMEM;
+
+    /* Weed out anything under 16MB */
+    for (i = 0; i < nr; i++) {
+      if (src[i].addr > 0x100000)
+        continue;
+
+      src[i].type = 0;
+      src[i].size = 0;
+      src[i].addr = -1ULL;
+    }
+
+    /* Find the lowest and highest entry in E820, skipping over
+     * undersired entries. */
+    start = -1ULL;
+    last = 0;
+    for (i = 0; i < nr; i++) {
+        if ((src[i].type == E820_RAM) ||
+            (src[i].type == E820_UNUSABLE) ||
+            (src[i].type == 0)) 
+		continue;
+
+            start = src[i].addr < start ? src[i].addr : start;
+            last = src[i].addr + src[i].size > last ?
+                    src[i].addr + src[i].size > last : last;
+    }
+    if (start > 1024)
+      start_kb = start >> 10;
+
+    /* Add the memory RAM region for the guest */
+    e820[idx].addr = 0;
+    e820[idx].size = (uint64_t)map_limitkb << 10;
+    e820[idx].type = E820_RAM;
+
+    /* .. and trim if neccessary */
+    if (start_kb && map_limitkb > start_kb) {
+        delta_kb = map_limitkb - start_kb;
+        if (delta_kb)
+          e820[idx].size -= (uint64_t)(delta_kb << 10);
+    }
+    /* Note: We don't touch balloon_kb here. Will add it at the end. */
+    ram_end = e820[idx].addr + e820[idx].size;
+    idx ++;
+
+    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Memory: %ldkB End of RAM: 0x%lx (PFN) " \
+               "Delta: %ldkB, PCI start: %ldkB (0x%lx PFN), Balloon %ldkB\n",
+		map_limitkb, ram_end >> 12, delta_kb, start_kb ,start >> 12,
+                balloon_kb);
+
+    /* Check if there is a region between ram_end and start. */
+    if (start > ram_end) {
+      /* .. and if not present, add it in. This is to guard against
+       the Linux guest assuming that the gap between the end of
+       RAM region and the start of the E820_[ACPI,NVS,RESERVED]
+       is PCI I/O space. Which it certainly is _not_. */
+      e820[idx].type = E820_UNUSABLE;
+      e820[idx].addr = ram_end;
+      e820[idx].size = start - ram_end;
+      idx++;
+    } 
+     /* Almost done: copy them over, ignoring the undesireable ones */
+     for (i = 0; i < nr; i++) {
+            if ((src[i].type == E820_RAM) ||
+	        (src[i].type == E820_UNUSABLE) ||
+	         (src[i].type == 0))
+		continue;
+            e820[idx].type = src[i].type;
+            e820[idx].addr = src[i].addr;
+            e820[idx].size = src[i].size;
+            idx++;
+     }
+
+     /* At this point we have the mapped RAM + E820 entries from src. */
+     if (balloon_kb) {
+        /* and if we truncated the RAM region, then add it to the end. */
+        e820[idx].type = E820_RAM;
+        e820[idx].addr = (uint64_t)(1ULL << 32) > last ? (uint64_t)(1ULL << 32) : last;
+        /* also add the balloon memory to the end. */
+        e820[idx].size = (uint64_t)(delta_kb << 10) + (uint64_t)(balloon_kb << 10);
+        idx++;
+
+    }
+    nr = idx;
+
+    for (i = 0; i < nr; i++) {
+      LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, ":%s\t[%lx -> %lx]",
+	e820[i].type == E820_RAM ? "RAM " :
+	(e820[i].type == E820_RESERVED ? "RSV " :
+	 e820[i].type == E820_ACPI ? "ACPI" :
+         (e820[i].type == E820_NVS ? "NVS " :
+          (e820[i].type == E820_UNUSABLE ? "UNU " : "----"))),
+          e820[i].addr >> 12,
+         (e820[i].addr + e820[i].size) >> 12);
+    }
+
+    /* Done: copy the sanitized version. */
+    *nr_entries = nr;
+    memcpy(src, e820, nr * sizeof(struct e820entry));
+    return 0;
+}
+      
+
+int libxl__e820_alloc(libxl_ctx *ctx, uint32_t domid, libxl_domain_config *d_config)
+{
+    int rc;
+    uint32_t nr;
+    struct e820entry map[E820MAX];
+    libxl_domain_build_info *b_info;
+
+    if (d_config == NULL || d_config->c_info.hvm)
+      return ERROR_INVAL;
+
+    b_info = &d_config->b_info;
+    if (b_info->u.pv.no_machine_e820)
+      return ERROR_INVAL;
+
+    rc = xc_get_machine_memory_map(ctx->xch, map, E820MAX);
+    if (rc < 0) {
+        errno = rc;
+        return ERROR_FAIL;
+    }
+    nr = rc;
+    rc = e820_sanitize(ctx, map, &nr, b_info->target_memkb,
+                       (b_info->max_memkb - b_info->target_memkb) +
+                       b_info->u.pv.slack_memkb);
+    if (rc)
+        return ERROR_FAIL;
+
+    rc = xc_domain_set_memory_map(ctx->xch, domid, map, nr);
+
+    if (rc < 0) {
+        errno  = rc;
+        return ERROR_FAIL;
+    }
+    return 0;
+}

diff -r 4527044d3d1c -r 945a34bcc2bd tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +4 -1

@@ -371,6 +371,7 @@ static void printf_info(int domid,
         printf("\t\t\t(kernel %s)\n", b_info->kernel.path);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);
+        printf("\t\t\t(no_machine_e820 %d)\n", b_info->u.pv.no_machine_e820);
         printf("\t\t)\n");
     }
     printf("\t)\n");

@@ -1025,7 +1026,9 @@ skip_vfb:
             if (!libxl_device_pci_parse_bdf(&ctx, pcidev, buf))
                 d_config->num_pcidevs++;
         }
-    }
+    } else
+      if (!c_info->hvm)
+        b_info->u.pv.no_machine_e820 = 1;
 
     switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
     case 0:

Quoted 13 lines · Konrad Rzeszutek Wilk

Quoted 12 lines · Ian Campbell

Quoted 7 lines · Konrad Rzeszutek Wilk

The code libxl_device_pci_parse_bdf calls the libxl__e820_alloc when it has parsed one BDF. Subsequent calls to libxl_device_pci_parse_bdf will
not trigger libxl__e820_alloc being called (unless the first call to libxl__e820_alloc failed).

That sounds like a very odd non-intuitive location for that allocation. Why not do it in libxl_domain_create or somewhere like that?

Quoted 4 lines

Quoted 2 lines

I think the e820 map added to the idl should become a simple boolean flag and this should all be taken care of internally based on that.

Like this?

Why no_machine_e820 instead of just machine_e820? You can set a non-zero default in the appropriate libxl_foo_init function if need be.

Quoted 19 lines · Ian Campbell

Quoted 17 lines · Konrad Rzeszutek Wilk

Quoted 9 lines · Ian Campbell

Quoted 7 lines · Konrad Rzeszutek Wilk

The code libxl_device_pci_parse_bdf calls the libxl__e820_alloc when it has parsed one BDF. Subsequent calls to libxl_device_pci_parse_bdf will
not trigger libxl__e820_alloc being called (unless the first call to libxl__e820_alloc failed).

Quoted 5 lines

That sounds like a very odd non-intuitive location for that allocation. Why not do it in libxl_domain_create or somewhere like that?

I think the e820 map added to the idl should become a simple boolean flag and this should all be taken care of internally based on that.

Like this?

Quoted 2 lines

Why no_machine_e820 instead of just machine_e820? You can set a non-zero default in the appropriate libxl_foo_init function if need be.

I tried to
("machine_e820", bool, True, "Use machine's E820 for PCI passthrough."),

and it made the the machine_e820 variable be an 'const bool' so that you couldn't change it anymore.

xl_cmdimpl.c:990: error: assignment of read-only member ‘machine_e820’

112             char * cmdline;
113             libxl_file_reference ramdisk;
114             const char * features;
115             /*

116 * Use machine's E820 for PCI passthrough.
117 */
118 const bool machine_e820;
119 } pv;
120 } u;
121 } libxl_domain_build_info;

If I make an 'integer' I get the same thing. Ah, if I modify the python code:

konrad@phenom:~/hg/xen-unstable.hg/tools/libxl$ hg diff
diff -r cc01692f4b41 tools/libxl/gentypes.py

tools/libxl/gentypes.py Tue Apr 12 14:58:24 2011 -0400 renamed from tools/libxl/gentypes.py Tue Apr 12 14:46:05 2011 -0400 +1 -1

@@ -48,7 +48,7 @@ def libxl_C_type_define(ty, indent = "")
                 s += format_comment(4, f.comment)
             x = libxl_C_instance_of(f.type, f.name)
             if f.const:
-                x = "const " + x
+                x = " " + x
             x = x.replace("\n", "\n    ")
             s += "    " + x + ";\n"
         if ty.typename is None:

It starts working, but that does not look like the correct fix.

Quoted 1 line

Quoted 1 line · Konrad Rzeszutek Wilk

Quoted 7 lines · Keir Fraser

Quoted 5 lines · Konrad Rzeszutek Wilk

Quoted 4 lines

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

Quoted 7 lines

Quoted 4 lines

I was thinking about it, but the mm.c code did not have the xen/xmalloc.h header, nor any references to xmalloc_array.

Is it OK to make an xmalloc_array during a hypercall?

Yes. I think the toolstack should be able to clean up on the newly-possible ENOMEM return from this hypercall.

Hm, not sure what I am hitting, but I can't seem to be able to copy over the contents to the newly allocated array from the guest (this works fine with the previous version of the patch). This is what I get

(XEN) mm.c:4734:d0 We want 8 E820 entries. We have: 1. (XEN) mm.c:4754:d0 Copying E820 8 entries. (XEN) ----[ Xen-4.2-110412 : 00000000000000a0

(XEN) rdx: 0000000000000000   rsi: 0000000000680004   rdi: 0000000000000000
(XEN) rbp: ffff82c48028fc68   rsp: ffff82c48028fc58   r8:  ffff82c4802c8bd0
(XEN) r9:  0000000000000004   r10: 0000000000000004   r11: 0000000000000001
(XEN) r12: 00000000000000a0   r13: 0000000000000000   r14: 0000000000680004
(XEN) r15: 0000000000000008   cr0: 0000000080050033   cr4: 00000000000026f0
(XEN) cr3: 00000001056af000   cr2: 0000000000000000

(XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: e010 cs: e008
(XEN) Xen stack trace from rsp=ffff82c48028fc58:

(XEN)    00000000000000a0 00000000000000a0 ffff82c48028fc98 ffff82c48017fa7d
(XEN)    0000000000000008 0000000000000000 0000000000000008 ffff88005c51d7c0
(XEN)    ffff82c48028fd38 ffff82c480169857 ffff82c48028fcb8 ffff82c480167d9a
(XEN)    000000008028fd08 ffff82f60214be50 0000000000000001 0000000000000008
(XEN)    0000000000680004 0000000000000001 ffff88005d8aa1f8 ffff8301056e0000
(XEN)    ffff83011f9b3000 0000000000000000 ffff82c48028fe28 ffff8300cf0fd000
(XEN)    000000000000000d 00007fff2d629060 ffff88005c51d7c0 0000000000000008
(XEN)    ffff82c48028fef8 ffff82c48011399c ffff82c4802c9940 000000008028ff18
(XEN)    ffff82c48028fd88 0000000000123412 ffff82f602468250 ffff82c48028ff18
(XEN)    ffff82c48028ff18 ffff82c4802c8e38 000000008028fde8 ffff82f602468250
(XEN)    00000000001068f6 ffff83011f9b3000 0000000000000070 0000000123412000
(XEN)    ffff82f602468240 ffff88005d8a9220 0000000000000000 0000000000682004
(XEN)    ffff82c48028fe28 ffff82c48016aa14 8000000127365065 000000018015fd21
(XEN)    0000000000000000 ffff8300cf0fd1c8 ffff8300cf0fd1c8 ffff82c48028fe88
(XEN)    aaaaaaaaaaaaaaaa ffff88005d8a9220 ffff82c48028fef8 ffff82c480113cb7
(XEN)    ffff8300cf0fd1d0 ffff8300cf0fd000 000000018028fef8 ffff82c48028ff18
(XEN)    ffff82c48028fe68 ffff82c48028ff18 ffff8300cf0fd000 00007fff2d6290b0
(XEN)    0000000000000001 0000000000000008 ffff82c48028fef8 ffff82c4802038c3
(XEN)    0000000000680000 00007fb540012cd0 0000000000000000 0000000000000000
(XEN)    00007fb540012e60 000000000000e033 ffff82c48028fed8 ffff8300cf0fd000
(XEN) Xen call trace:
(XEN)    [<ffff82c48020a3c5>] vmac+0x5da/0x927
(XEN)    [<ffff82c48017fa7d>] copy_from_user+0x72/0x9f
(XEN)    [<ffff82c480169857>] arch_memory_op+0x7cc/0xf05
(XEN)    [<ffff82c48011399c>] do_memory_op+0x1dca/0x1df2
(XEN)    [<ffff82c4802082c8>] syscall_enter+0xc8/0x122

And the patch is:

diff -r 97763efc41f9 xen/arch/x86/domain.c

xen/arch/x86/domain.c +2 -0

@@ -696,6 +696,8 @@ void arch_domain_destroy(struct domain *
 
     if ( is_hvm_domain(d) )
         hvm_domain_destroy(d);
+    else
+      xfree(d->arch.pv_domain.e820);
 
     vmce_destroy_msr(d);
     pci_release_devices(d);

diff -r 97763efc41f9 xen/arch/x86/mm.c

xen/arch/x86/mm.c +2 -1

@@ -100,6 +100,7 @@
 #include <xen/iocap.h>
 #include <xen/guest_access.h>
 #include <xen/pfn.h>
+#include <xen/xmalloc.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>

@@ -4710,7 +4711,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
         if ( copy_from_guest(&fmap, arg, 1) )
             return -EFAULT;
 
-        if ( fmap.map.nr_entries > ARRAY_SIZE(d->arch.pv_domain.e820) )
+        if ( fmap.map.nr_entries > E820MAX )
             return -EINVAL;
 
         rc = rcu_lock_target_domain_by_id(fmap.domid, &d);

@@ -4730,9 +4731,39 @@ long arch_memory_op(int op, XEN_GUEST_HA
             return -EPERM;
         }
 

xen/arch/x86/mm.c +24 -1

+            d->arch.pv_domain.e820 = xmalloc_array(e820entry_t,
+                                                   fmap.map.nr_entries + 1);
+            if ( !d->arch.pv_domain.e820 )
+            {
+                rcu_unlock_domain(d);
+                return -ENOMEM;
+            }
+            gdprintk(XENLOG_INFO, "Allocated E820 for %d entries.\n", fmap.map.nr_entries);
+            memset(d->arch.pv_domain.e820, 0, (fmap.map.nr_entries + 1) * sizeof(e820entry_t));
+        }
+        gdprintk(XENLOG_INFO, "Copying E820 %d entries.\n", fmap.map.nr_entries);
         rc = copy_from_guest(d->arch.pv_domain.e820, fmap.map.buffer,
                              fmap.map.nr_entries) ? -EFAULT : 0;
-        d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
+
+        if ( rc == 0 )
+          d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
+        else {
+          /* Don't free the E820 if it had been set before and we failed. */
+          if ( !d->arch.pv_domain.nr_e820 ) {
+            xfree(d->arch.pv_domain.e820);
+            d->arch.pv_domain.e820 = NULL;
+          }
+        }
 
         rcu_unlock_domain(d);
         return rc;

@@ -4747,6 +4778,9 @@ long arch_memory_op(int op, XEN_GUEST_HA
         if ( d->arch.pv_domain.nr_e820 == 0 )
             return -ENOSYS;
 
+        if ( d->arch.pv_domain.e820 == NULL )
+            return -ENOSYS;
+
         if ( copy_from_guest(&map, arg, 1) )
             return -EFAULT;
 

diff -r 97763efc41f9 xen/include/asm-x86/domain.h

xen/include/asm-x86/domain.h +1 -1

@@ -238,7 +238,7 @@ struct pv_domain
     unsigned long pirq_eoi_map_mfn;
 
     /* Pseudophysical e820 map (XENMEM_memory_map).  */
-    struct e820entry e820[3];
+    struct e820entry *e820;
     unsigned int nr_e820;
 };
 

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Quoted 10 lines · Keir Fraser

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 4 lines

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

I was thinking about it, but the mm.c code did not have the xen/xmalloc.h header, nor any references to xmalloc_array.

Quoted 4 lines

Quoted 1 line

Is it OK to make an xmalloc_array during a hypercall?

Yes. I think the toolstack should be able to clean up on the newly-possible ENOMEM return from this hypercall.

Hm, not sure what I am hitting, but I can't seem to be able to copy over the contents to the newly allocated array from the guest (this works fine with the previous version of the patch). This is what I get

(XEN) mm.c:4734:d0 We want 8 E820 entries. We have: 1. (XEN) mm.c:4754:d0 Copying E820 8 entries. (XEN) ----[ Xen-4.2-110412 : 00000000000000a0

(XEN) rdx: 0000000000000000   rsi: 0000000000680004   rdi: 0000000000000000
(XEN) rbp: ffff82c48028fc68   rsp: ffff82c48028fc58   r8:  ffff82c4802c8bd0
(XEN) r9:  0000000000000004   r10: 0000000000000004   r11: 0000000000000001
(XEN) r12: 00000000000000a0   r13: 0000000000000000   r14: 0000000000680004
(XEN) r15: 0000000000000008   cr0: 0000000080050033   cr4: 00000000000026f0
(XEN) cr3: 00000001056af000   cr2: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen stack trace from rsp=ffff82c48028fc58:
(XEN)    00000000000000a0 00000000000000a0 ffff82c48028fc98 ffff82c48017fa7d
(XEN)    0000000000000008 0000000000000000 0000000000000008 ffff88005c51d7c0
(XEN)    ffff82c48028fd38 ffff82c480169857 ffff82c48028fcb8 ffff82c480167d9a
(XEN)    000000008028fd08 ffff82f60214be50 0000000000000001 0000000000000008
(XEN)    0000000000680004 0000000000000001 ffff88005d8aa1f8 ffff8301056e0000
(XEN)    ffff83011f9b3000 0000000000000000 ffff82c48028fe28 ffff8300cf0fd000
(XEN)    000000000000000d 00007fff2d629060 ffff88005c51d7c0 0000000000000008
(XEN)    ffff82c48028fef8 ffff82c48011399c ffff82c4802c9940 000000008028ff18
(XEN)    ffff82c48028fd88 0000000000123412 ffff82f602468250 ffff82c48028ff18
(XEN)    ffff82c48028ff18 ffff82c4802c8e38 000000008028fde8 ffff82f602468250
(XEN)    00000000001068f6 ffff83011f9b3000 0000000000000070 0000000123412000
(XEN)    ffff82f602468240 ffff88005d8a9220 0000000000000000 0000000000682004
(XEN)    ffff82c48028fe28 ffff82c48016aa14 8000000127365065 000000018015fd21
(XEN)    0000000000000000 ffff8300cf0fd1c8 ffff8300cf0fd1c8 ffff82c48028fe88
(XEN)    aaaaaaaaaaaaaaaa ffff88005d8a9220 ffff82c48028fef8 ffff82c480113cb7
(XEN)    ffff8300cf0fd1d0 ffff8300cf0fd000 000000018028fef8 ffff82c48028ff18
(XEN)    ffff82c48028fe68 ffff82c48028ff18 ffff8300cf0fd000 00007fff2d6290b0
(XEN)    0000000000000001 0000000000000008 ffff82c48028fef8 ffff82c4802038c3
(XEN)    0000000000680000 00007fb540012cd0 0000000000000000 0000000000000000
(XEN)    00007fb540012e60 000000000000e033 ffff82c48028fed8 ffff8300cf0fd000
(XEN) Xen call trace:
(XEN)    [<ffff82c48020a3c5>] vmac+0x5da/0x927
(XEN)    [<ffff82c48017fa7d>] copy_from_user+0x72/0x9f
(XEN)    [<ffff82c480169857>] arch_memory_op+0x7cc/0xf05
(XEN)    [<ffff82c48011399c>] do_memory_op+0x1dca/0x1df2
(XEN)    [<ffff82c4802082c8>] syscall_enter+0xc8/0x122

And the patch is:

diff -r 97763efc41f9 xen/arch/x86/domain.c

xen/arch/x86/domain.c Tue Apr 12 13:18:34 2011 -0400 renamed from xen/arch/x86/domain.c Tue Apr 05 18:23:54 2011 +0100 +2 -0

@@ -696,6 +696,8 @@ void arch_domain_destroy(struct domain *
 
     if ( is_hvm_domain(d) )
         hvm_domain_destroy(d);
+    else
+      xfree(d->arch.pv_domain.e820);
 
     vmce_destroy_msr(d);
     pci_release_devices(d);

diff -r 97763efc41f9 xen/arch/x86/mm.c

xen/arch/x86/mm.c Tue Apr 12 13:18:34 2011 -0400 renamed from xen/arch/x86/mm.c Tue Apr 05 18:23:54 2011 +0100 +36 -2

@@ -100,6 +100,7 @@
 #include <xen/iocap.h>
 #include <xen/guest_access.h>
 #include <xen/pfn.h>
+#include <xen/xmalloc.h>
 #include <asm/paging.h>
 #include <asm/shadow.h>
 #include <asm/page.h>

@@ -4710,7 +4711,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
         if ( copy_from_guest(&fmap, arg, 1) )
             return -EFAULT;
 
-        if ( fmap.map.nr_entries > ARRAY_SIZE(d->arch.pv_domain.e820) )
+        if ( fmap.map.nr_entries > E820MAX )
             return -EINVAL;
 
         rc = rcu_lock_target_domain_by_id(fmap.domid, &d);

@@ -4730,9 +4731,39 @@ long arch_memory_op(int op, XEN_GUEST_HA
             return -EPERM;
         }
 
+        gdprintk(XENLOG_INFO, "We want %d E820 entries. We have: %d.\n",
 fmap.map.nr_entries, d->arch.pv_domain.nr_e820);
+        if ( d->arch.pv_domain.e820 )
+        {
+            if ( fmap.map.nr_entries >= d->arch.pv_domain.nr_e820 )
+            {
+                xfree(d->arch.pv_domain.e820);
+                d->arch.pv_domain.e820 = NULL;
+                d->arch.pv_domain.nr_e820 = 0;
+            }
+        } else {
+            d->arch.pv_domain.e820 = xmalloc_array(e820entry_t,
+                                                   fmap.map.nr_entries + 1);
+            if ( !d->arch.pv_domain.e820 )
+            {
+                rcu_unlock_domain(d);
+                return -ENOMEM;
+            }
+            gdprintk(XENLOG_INFO, "Allocated E820 for %d entries.\n",
 fmap.map.nr_entries);
+            memset(d->arch.pv_domain.e820, 0, (fmap.map.nr_entries + 1) *
 sizeof(e820entry_t));
+        }
+        gdprintk(XENLOG_INFO, "Copying E820 %d entries.\n",
 fmap.map.nr_entries);
         rc = copy_from_guest(d->arch.pv_domain.e820, fmap.map.buffer,
                              fmap.map.nr_entries) ? -EFAULT : 0;
-        d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
+
+        if ( rc == 0 )
+          d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
+        else {
+          /* Don't free the E820 if it had been set before and we failed. */
+          if ( !d->arch.pv_domain.nr_e820 ) {
+            xfree(d->arch.pv_domain.e820);
+            d->arch.pv_domain.e820 = NULL;
+          }
+        }
 
         rcu_unlock_domain(d);
         return rc;

@@ -4747,6 +4778,9 @@ long arch_memory_op(int op, XEN_GUEST_HA
         if ( d->arch.pv_domain.nr_e820 == 0 )
             return -ENOSYS;
 
+        if ( d->arch.pv_domain.e820 == NULL )
+            return -ENOSYS;
+
         if ( copy_from_guest(&map, arg, 1) )
             return -EFAULT;
 

diff -r 97763efc41f9 xen/include/asm-x86/domain.h

... this "else". If you have an e820 table already, and it's too small, you want to not only free it, but also allocate a new one. So instead of an "else" you'll need a second "if" here, recovering from the table possibly having got freed. Since xfree(NULL) is okay, you could actually simply use the inner conditional on the first check.

Quoted 16 lines · Konrad Rzeszutek Wilk

Quoted 14 lines · Keir Fraser

Quoted 9 lines · Konrad Rzeszutek Wilk

Quoted 4 lines

How cunning.

Why wouldn't you just allocate exactly the right size of array in XENMEM_set_memory_map?

I was thinking about it, but the mm.c code did not have the xen/xmalloc.h header, nor any references to xmalloc_array.

Is it OK to make an xmalloc_array during a hypercall?

Yes. I think the toolstack should be able to clean up on the newly-possible ENOMEM return from this hypercall.

Quoted 3 lines

Hm, not sure what I am hitting, but I can't seem to be able to copy over the contents to the newly allocated array from the guest (this works fine with the previous version of the patch). This is what I get

Your patch is not really quite right. I've attached a modified version for you to try.