[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] libxl, hvmloader: Don't relocate memory for MMIO hole

On 06/17/2013 06:15 PM, Ian Jackson wrote:
George Dunlap writes ("[PATCH] libxl,hvmloader: Don't relocate memory for MMIO 
At the moment, qemu-xen can't handle memory being relocated by
hvmloader.  This may happen if a device with a large enough memory
region is passed through to the guest.  At the moment, if this
happens, then at some point in the future qemu will crash and the
domain will hang.  (qemu-traditional is fine.)

I think the approach is good.  Arguably the two things should be in
two patches.

You mean one in libxl to set allow_memory_relocate, one to do something about it?

+    const char *s;
+    uint8_t allow_memory_relocate=1;
+    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
+    if (s)
+        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);

Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
the cast.  (Also, spaces round `='.)

Remember that hvmloader doesn't actually have a libc; this is a locally implemented function, and AFAICT the only one implemented is strtoll.

+    printf("allow_memory_relocate %u\n",
+           __func__, allow_memory_relocate);

Missing %s argument.  This shows that -Wformat isn't working.

No, my bad -- I had a script which I thought was compiling all the tools, but in fact was *only* compiling libxl/xl. -Wformat is indeed working.

-    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
-            ((pci_mem_start << 1) != 0) )
+    /*
+     * At the moment qemu-xen can't deal with relocated memory regions.
+     * It's too close to the release to make a proper fix; for now,
+     * if we're using qemu-xen, just crash and tell people to switch
+     * to qemu-traditional.
+     */
+    while ((mmio_total > (pci_mem_end - pci_mem_start))
+           && ((pci_mem_start << 1) != 0)
+           && ( allow_memory_relocate

I think this is an unnecessarily confusing way of writing it.
allow_memory_relocate does not change.  I would lift it out of the
loop into an if.  That would also mean that "git-diff -b" can show
more clearly whether the change is truly what you intend.

But the behavior we want is this:
* If allow_memory_relocate, increase the size of the memory hole until it's big enough, or it reaches 2GiB * If !allow_memory_relocate, increase the size of the memory hole until it's big enough, or it reaches 2GiB, or it overlaps guest memory.

If we pulled "allow_memory_relocate" out, we'd have to make two separate while() loops. That might make it more clear what was going on, but would risk introducing errors between the two copies for the other conditions.

+                || (((pci_mem_start << 1)>>PAGE_SHIFT 
          pci_mem_start <<= 1;

-    if ( (pci_mem_start << 1) != 0 )
+    if (mmio_total > (pci_mem_end - pci_mem_start))
          bar64_relocate = 1;

Missing spaces.

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ac1f90e..5167ee0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1154,6 +1154,13 @@ void libxl__spawn_local_dm(libxl__egc *egc, 
libxl__dm_spawn_state *dmss)
          libxl__xs_write(gc, XBT_NULL,
                          libxl__sprintf(gc, "%s/hvmloader/bios", path),
                          "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
+        /* Disable relocating memory to make the MMIO hole larger unless we'
+         * running qemu-traditional */

This whole hunk could do with more aggressive wrapping.  See what it
looks like in my MUA.  Can you keep it down to 70-75 columns ?


+        libxl__xs_write(gc, XBT_NULL,
+                        libxl__sprintf(gc, "%s/hvmloader/allow-memory-reloca
te", path),
+                        "%s",
+                        (b_info->device_model_version==LIBXL_DEVICE_MODEL_VE
+                        "1":"0");

Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
value from libxl__xs_write).

Once again, I disagree with mixing different code styles in the same function. Using GCSPRINTF and doing an error check here will make it seem like it's doing something different than the line immediately above it (which is what I copied to get here), which will confuse people.

In theory the "fix it up as we go along" sounds good, but in practice it's awful. How many times have you had to say exactly this kind of thing, when all I did was copy some bit of code that was nearby and do exactly the same thing? It just annoys you and frustrates me.

I shouldn't have to keep in my head, "This is the new way of doing things". It's a total waste of mental energy; I have better things to spend my brain capacity on. If you want things to change, I think we're going to have to have a "flag day" where we go through and change all the code, at least file-by-file. I'd be willing to help with this cleanup once the 4.4 development window opens.

Instead of "%s", blah?"1":"0" use "%d" and the == expression itself.

That's a good idea.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.