Patchwork [RFC,6/8] exec: Clean up unnecessary S390 ifdeffery

login
register
mail settings
Submitter Markus Armbruster
Date June 13, 2013, 7:02 a.m.
Message ID <1371106939-6968-7-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/250989/
State New
Headers show

Comments

Markus Armbruster - June 13, 2013, 7:02 a.m.
Another issue missed in commit fdec991 is -mem-path: it needs to be
rejected only for old S390 KVM, not for any S390.  Not that I
personally care, but the ifdeffery in qemu_ram_alloc_from_ptr() annoys
me.

Note that this doesn't actually make -mem-path work, as the kernel
doesn't (yet?)  support large pages in the host for KVM guests.  Clean
it up anyway.

Thanks to Christian Borntraeger for pointing out the S390 kernel
limitations.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
Paolo Bonzini - June 13, 2013, 10:21 p.m.
Il 13/06/2013 03:02, Markus Armbruster ha scritto:
> +    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
> +        /* some s390/kvm configurations have special constraints */
> +        if (mem_path) {
> +            fprintf(stderr,
> +                    "-mem-path not supported with this version of KVM\n");
> +            exit(1);
> +        }
> +        new_block->host = kvm_arch_ram_alloc(size);
> +        memory_try_enable_merging(new_block->host, size);

Uh oh, now I see why you wanted the hook as a function pointer.  Can you
instead pass the file descriptor to kvm_ram_alloc?

Paolo
Markus Armbruster - June 14, 2013, 8:06 a.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/06/2013 03:02, Markus Armbruster ha scritto:
>> +    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
>> +        /* some s390/kvm configurations have special constraints */
>> +        if (mem_path) {
>> +            fprintf(stderr,
>> +                    "-mem-path not supported with this version of KVM\n");
>> +            exit(1);
>> +        }
>> +        new_block->host = kvm_arch_ram_alloc(size);
>> +        memory_try_enable_merging(new_block->host, size);
>
> Uh oh, now I see why you wanted the hook as a function pointer.  Can you
> instead pass the file descriptor to kvm_ram_alloc?

I'm not sure I understand what you're suggesting.  Here's my best guess.

I made kvm_arch_ram_alloc() an *optional* hook, implemented as function
pointer that may be null.  It's actually non-null only with old S390
KVM.

Non-null value also suppresses -mem-path.  Admittedly not the cleanest
possible solution, but (1) it's better than what we have now, and (2)
the whole thing should go away forever once we stop supporting old S390
kernels.

You seem to suggest to factor the actual allocation of guest memory out
of this part of qemu_ram_alloc_from_ptr():

    if (host) {
        new_block->host = host;
        new_block->flags |= RAM_PREALLOC_MASK;
    } else if (xen_enabled()) {
        if (mem_path) {
            fprintf(stderr, "-mem-path not supported with Xen\n");
            exit(1);
        }
        xen_ram_alloc(new_block->offset, size, mr);
    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
        /* some s390/kvm configurations have special constraints */
        if (mem_path) {
            fprintf(stderr,
                    "-mem-path not supported with this version of KVM\n");
            exit(1);
        }
        new_block->host = kvm_arch_ram_alloc(size);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
        memory_try_enable_merging(new_block->host, size);
    } else {
        if (mem_path) {
            new_block->host = file_ram_alloc(new_block, size, mem_path);
        }
        if (!new_block->host) {
            new_block->host = qemu_anon_ram_alloc(size);
            if (!new_block->host) {
                no_guest_mem(new_block);
            }
            memory_try_enable_merging(new_block->host, size);
        }
    }

Three functions, one each for Xen, S390 KVM, and generic.  Except the
one for Xen has only one caller, so we better leave that one alone.

Put a trivial wrapper around generic into each target-*/kvm.c other than
s390: arm, i386, ppc.

qemu_ram_alloc_from_ptr() then looks roughly like this:

    if (host) {
        new_block->host = host;
        new_block->flags |= RAM_PREALLOC_MASK;
    } else if (xen_enabled()) {
        if (mem_path) {
            fprintf(stderr, "-mem-path not supported with Xen\n");
            exit(1);
        }
        xen_ram_alloc(new_block->offset, size, mr);
    } else if (kvm_enabled()) {
        new->block->host = kvm_arch_ram_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    } else {
        new_block->host = qemu_guest_mem_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    }

Except new_block->fd still needs to be set.  Need to split
file_ram_alloc() into two: first part yields the file descriptor, to be
called by qemu_ram_alloc_from_ptr(), second part does the actual
allocation, to be called by the allocation functions.  The code in
qemu_ram_alloc_from_ptr() becomes:

    if (host) {
        new_block->host = host;
        new_block->flags |= RAM_PREALLOC_MASK;
    } else if (xen_enabled()) {
        if (mem_path) {
            fprintf(stderr, "-mem-path not supported with Xen\n");
            exit(1);
        }
        xen_ram_alloc(new_block->offset, size, mr);
    } else {
        new->block->fd = file_ram_fd(mem_path);
        new->block->host = kvm_enabled()
            ? kvm_arch_ram_alloc(size, new_block->fd);
            : qemu_guest_mem_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    }

Is that what you have in mind?

It's a lot of restructuring and notational overhead just for this stupid
hack to support old S390 kernels.

Here's another way to keep the KVM hooks regular: make my function
pointers exec.c hooks instead of KVM hooks ;-P
Paolo Bonzini - June 15, 2013, 5:26 p.m.
Il 14/06/2013 04:06, Markus Armbruster ha scritto:
> Here's another way to keep the KVM hooks regular: make my function
> pointers exec.c hooks instead of KVM hooks ;-P

I can buy this one!

Paolo

Patch

diff --git a/exec.c b/exec.c
index 8810d33..9f0355b 100644
--- a/exec.c
+++ b/exec.c
@@ -849,7 +849,7 @@  void qemu_mutex_unlock_ramlist(void)
     qemu_mutex_unlock(&ram_list.mutex);
 }
 
-#if defined(__linux__) && !defined(TARGET_S390X)
+#ifdef __linux__
 
 #include <sys/vfs.h>
 
@@ -952,6 +952,14 @@  static void *file_ram_alloc(RAMBlock *block,
     block->fd = fd;
     return area;
 }
+#else
+static void *file_ram_alloc(RAMBlock *block,
+                            ram_addr_t memory,
+                            const char *path)
+{
+    fprintf(stderr, "-mem-path not supported on this host\n");
+    exit(1);
+}
 #endif
 
 static ram_addr_t find_ram_offset(ram_addr_t size)
@@ -1088,22 +1096,21 @@  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
             exit(1);
         }
         xen_ram_alloc(new_block->offset, size, mr);
+    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
+        /* some s390/kvm configurations have special constraints */
+        if (mem_path) {
+            fprintf(stderr,
+                    "-mem-path not supported with this version of KVM\n");
+            exit(1);
+        }
+        new_block->host = kvm_arch_ram_alloc(size);
+        memory_try_enable_merging(new_block->host, size);
     } else {
         if (mem_path) {
-#if defined (__linux__) && !defined(TARGET_S390X)
             new_block->host = file_ram_alloc(new_block, size, mem_path);
-#else
-            fprintf(stderr, "-mem-path option unsupported\n");
-            exit(1);
-#endif
         }
         if (!new_block->host) {
-            if (kvm_enabled() && kvm_arch_ram_alloc) {
-                /* some s390/kvm configurations have special constraints */
-                new_block->host = kvm_arch_ram_alloc(size);
-            } else {
-                new_block->host = qemu_anon_ram_alloc(size);
-            }
+            new_block->host = qemu_anon_ram_alloc(size);
             memory_try_enable_merging(new_block->host, size);
         }
     }