[Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

[Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

[Christian Borntraeger]

KVM does not allow memory regions > KVM_MEM_MAX_NR_PAGES, basically
limiting the memory per slot to 8TB-4k. Lets start a new memory region
if we cross that boundary.

With that (and optimistic overcommitment in the kernel) I was able to
start  a 24TB guest on a 1TB system.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8425534..3630f6a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -154,14 +154,41 @@ static void virtio_ccw_register_hcalls(void)
                                    virtio_ccw_hcall_early_printk);
 }
 
+/*
+ * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
+ * as the dirty bitmap must be managed by bitops that take an int as
+ * position indicator. If we have a guest beyond that we will split off
+ * new subregions.
+ */
+#define KVM_SLOT_MAX ((((1UL << 31) - 1) * 4096) & ~0xfffffUL )
+
 static void s390_memory_init(ram_addr_t mem_size)
 {
     MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
+    ram_addr_t chunk, offset;
+    unsigned int number;
+    gchar *name;
 
     /* allocate RAM for core */
-    memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
-    memory_region_add_subregion(sysmem, 0, ram);
+    offset = 0;
+    number = 0;
+    name = g_strdup_printf("s390.ram");
+    while (mem_size) {
+        MemoryRegion *ram = g_new(MemoryRegion, 1);
+        chunk = mem_size;
+        /* KVM does not allow memslots >= 8 TB */
+        if (chunk > KVM_SLOT_MAX) {
+            chunk = KVM_SLOT_MAX;
+        }
+        memory_region_allocate_system_memory(ram, NULL, name, chunk);
+        memory_region_add_subregion(sysmem, offset, ram);
+        mem_size -= chunk;
+        offset += chunk;
+	number++;
+        g_free(name);
+        name = g_strdup_printf("s390.ram.%u", number);
+    }
+    g_free(name);
 
     /* Initialize storage key device */
     s390_skeys_init();
-- 
2.9.4

Re: [Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

[Cornelia Huck]

On Thu,  7 Dec 2017 15:58:16 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> KVM does not allow memory regions > KVM_MEM_MAX_NR_PAGES, basically
> limiting the memory per slot to 8TB-4k. Lets start a new memory region

s/Lets/Let's/ :)

> if we cross that boundary.
> 
> With that (and optimistic overcommitment in the kernel) I was able to
> start  a 24TB guest on a 1TB system.

extra whitespace after 'start'

> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8425534..3630f6a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -154,14 +154,41 @@ static void virtio_ccw_register_hcalls(void)
>                                     virtio_ccw_hcall_early_printk);
>  }
>  
> +/*
> + * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
> + * as the dirty bitmap must be managed by bitops that take an int as
> + * position indicator. If we have a guest beyond that we will split off
> + * new subregions.
> + */
> +#define KVM_SLOT_MAX ((((1UL << 31) - 1) * 4096) & ~0xfffffUL )

extra space before closing bracket

What feels a bit awkward here is that we are working from a value that
is not externalized by KVM. If we expect that value to never get
smaller than it is now, we should be fine, though.

Another slightly awkward thing is that tcg is influenced by a kvm
detail. Should not matter, though, as it simply means that we may have
more memory regions. And I really don't expect 8TB+ tcg guests to
become a common use case :)

> +
>  static void s390_memory_init(ram_addr_t mem_size)
>  {
>      MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    ram_addr_t chunk, offset;
> +    unsigned int number;
> +    gchar *name;
>  
>      /* allocate RAM for core */
> -    memory_region_allocate_system_memory(ram, NULL, "s390.ram", mem_size);
> -    memory_region_add_subregion(sysmem, 0, ram);
> +    offset = 0;
> +    number = 0;
> +    name = g_strdup_printf("s390.ram");
> +    while (mem_size) {
> +        MemoryRegion *ram = g_new(MemoryRegion, 1);
> +        chunk = mem_size;
> +        /* KVM does not allow memslots >= 8 TB */
> +        if (chunk > KVM_SLOT_MAX) {
> +            chunk = KVM_SLOT_MAX;
> +        }
> +        memory_region_allocate_system_memory(ram, NULL, name, chunk);
> +        memory_region_add_subregion(sysmem, offset, ram);
> +        mem_size -= chunk;
> +        offset += chunk;
> +	number++;

odd indentation

> +        g_free(name);
> +        name = g_strdup_printf("s390.ram.%u", number);
> +    }
> +    g_free(name);
>  
>      /* Initialize storage key device */
>      s390_skeys_init();

The approach looks reasonable to me. I don't know how I can test it
(beyond sanity checking) on my machines, though.

Feedback from others would be appreciated.

Re: [Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

[David Hildenbrand]

[resending as I noticed that I dropped the ccs when I sent this last week]

On 07.12.2017 15:58, Christian Borntraeger wrote:
> KVM does not allow memory regions > KVM_MEM_MAX_NR_PAGES, basically
> limiting the memory per slot to 8TB-4k. Lets start a new memory region
> if we cross that boundary.
>
> With that (and optimistic overcommitment in the kernel) I was able to
> start  a 24TB guest on a 1TB system.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---

This should not harm migration I guess. And it's easier than the other
alternatives we discussed.

>  hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8425534..3630f6a 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -154,14 +154,41 @@ static void virtio_ccw_register_hcalls(void)
>                                     virtio_ccw_hcall_early_printk);
>  }
>
> +/*
> + * KVM does only support memory slots up to KVM_MEM_MAX_NR_PAGES pages
> + * as the dirty bitmap must be managed by bitops that take an int as
> + * position indicator. If we have a guest beyond that we will split off
> + * new subregions.
> + */
> +#define KVM_SLOT_MAX ((((1UL << 31) - 1) * 4096) & ~0xfffffUL )

KVM_SLOT_MAX_BYTES ?

(4096 -> TARGET_PAGE_SIZE, 0xfffff -> ? )

> +
>  static void s390_memory_init(ram_addr_t mem_size)
>  {
>      MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *ram = g_new(MemoryRegion, 1);
> +    ram_addr_t chunk, offset;
> +    unsigned int number;
> +    gchar *name;
>
>      /* allocate RAM for core */
> -    memory_region_allocate_system_memory(ram, NULL, "s390.ram",
mem_size);
> -    memory_region_add_subregion(sysmem, 0, ram);
> +    offset = 0;
> +    number = 0;

you can initialize these directly

> +    name = g_strdup_printf("s390.ram");
> +    while (mem_size) {
> +        MemoryRegion *ram = g_new(MemoryRegion, 1);
> +        chunk = mem_size;
> +        /* KVM does not allow memslots >= 8 TB */
> +        if (chunk > KVM_SLOT_MAX) {
> +            chunk = KVM_SLOT_MAX;
> +        }

chunk = MIN() ...

> +        memory_region_allocate_system_memory(ram, NULL, name, chunk);
> +        memory_region_add_subregion(sysmem, offset, ram);
> +        mem_size -= chunk;
> +        offset += chunk;
> +	number++;
> +        g_free(name);
> +        name = g_strdup_printf("s390.ram.%u", number);

you could directly use number++ here, when initializing number to 1.
(then also the strange indentation of number++ above is gone  )

> +    }
> +    g_free(name);
>
>      /* Initialize storage key device */
>      s390_skeys_init();
>


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20171207145816.87347-1-borntraeger@de.ibm.com
Subject: [Qemu-devel] [PATCH/RFC v2] s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20171208005825.14587-1-marcandre.lureau@redhat.com -> patchew/20171208005825.14587-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
28ebdee9e8 s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES

=== OUTPUT BEGIN ===
Checking PATCH 1/1: s390x: start a new memory region if the old one exceeds KVM_MEM_MAX_NR_PAGES...
ERROR: space prohibited before that close parenthesis ')'
#31: FILE: hw/s390x/s390-virtio-ccw.c:161:
+#define KVM_SLOT_MAX ((((1UL << 31) - 1) * 4096) & ~0xfffffUL )

ERROR: code indent should never use tabs
#58: FILE: hw/s390x/s390-virtio-ccw.c:185:
+^Inumber++;$

total: 2 errors, 0 warnings, 44 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org