[Qemu-devel] [PATCH v1 0/2] s390x/kvm: legacy_s390_alloc() fixes

[Qemu-devel] [PATCH v1 0/2] s390x/kvm: legacy_s390_alloc() fixes

[David Hildenbrand]

Two patches for legacy_s390_alloc(), only relevant under old z/VM versions.

Patch #1 is relevant when trying to allocate memory for more than one
memory region. Patch #2 is only for completeness, to make it behave
more similar to qemu_anon_ram_alloc().

David Hildenbrand (2):
  s390x/kvm: legacy_s390_alloc() only supports one allocation
  s390x/kvm: indicate alignment in legacy_s390_alloc()

 target/s390x/kvm.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 1/2] s390x/kvm: legacy_s390_alloc() only supports one allocation

[David Hildenbrand]

We always allocate at a fixed address, a second allocation can therefore
of course never work. We would simply overwrite mappings.

This can e.g. happen in s390_memory_init(), if trying to allocate more
than > 8TB. Let's just bail out, as there is no need for supporting it
(legacy handling for z/VM).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/kvm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ac370da281..bbac3454e3 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -752,12 +752,20 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
  */
 static void *legacy_s390_alloc(size_t size, uint64_t *align, bool shared)
 {
-    void *mem;
+    static void *mem;
+
+    if (mem) {
+        /* we only support one allocation, which is enough for initial ram */
+        return NULL;
+    }
 
     mem = mmap((void *) 0x800000000ULL, size,
                PROT_EXEC|PROT_READ|PROT_WRITE,
                MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
-    return mem == MAP_FAILED ? NULL : mem;
+    if (mem == MAP_FAILED) {
+        mem = NULL;
+    }
+    return mem;
 }
 
 static uint8_t const *sw_bp_inst;
-- 
2.17.1

[Qemu-devel] [PATCH v1 2/2] s390x/kvm: indicate alignment in legacy_s390_alloc()

[David Hildenbrand]

Let's do this for completeness reason, although we don't support e.g.
PCDIMM/NVDIMM, which would use the alignment for placing the memory
region in guest physical memory. But maybe someday we would want to
support something like this - then we don't forget about this if
allowing multiple allocations in legacy_s390_alloc().

Use the same alignment as we would set in qemu_anon_ram_alloc(). Our
fixed address satisfies this alignment (1MB). This implicitly sets the
alignment of the underlying memory region.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/kvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index bbac3454e3..22e13080a5 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -765,6 +765,9 @@ static void *legacy_s390_alloc(size_t size, uint64_t *align, bool shared)
     if (mem == MAP_FAILED) {
         mem = NULL;
     }
+    if (mem && align) {
+        *align = QEMU_VMALLOC_ALIGN;
+    }
     return mem;
 }
 
-- 
2.17.1

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/2] s390x/kvm: legacy_s390_alloc() only supports one allocation

[Christian Borntraeger]

On 06/28/2018 01:38 PM, David Hildenbrand wrote:
> We always allocate at a fixed address, a second allocation can therefore
> of course never work. We would simply overwrite mappings.
> 
> This can e.g. happen in s390_memory_init(), if trying to allocate more
> than > 8TB. Let's just bail out, as there is no need for supporting it
> (legacy handling for z/VM).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  target/s390x/kvm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ac370da281..bbac3454e3 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -752,12 +752,20 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
>   */
>  static void *legacy_s390_alloc(size_t size, uint64_t *align, bool shared)
>  {
> -    void *mem;
> +    static void *mem;
> +
> +    if (mem) {
> +        /* we only support one allocation, which is enough for initial ram */
> +        return NULL;
> +    }
>  
>      mem = mmap((void *) 0x800000000ULL, size,
>                 PROT_EXEC|PROT_READ|PROT_WRITE,
>                 MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> -    return mem == MAP_FAILED ? NULL : mem;
> +    if (mem == MAP_FAILED) {
> +        mem = NULL;
> +    }
> +    return mem;
>  }
>  
>  static uint8_t const *sw_bp_inst;
> 

Re: [Qemu-devel] [PATCH v1 0/2] s390x/kvm: legacy_s390_alloc() fixes

[Cornelia Huck]

On Thu, 28 Jun 2018 13:38:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> Two patches for legacy_s390_alloc(), only relevant under old z/VM versions.

Those teddy bears are really looking a bit scruffy... but the patches
look sane.

> 
> Patch #1 is relevant when trying to allocate memory for more than one
> memory region. Patch #2 is only for completeness, to make it behave
> more similar to qemu_anon_ram_alloc().
> 
> David Hildenbrand (2):
>   s390x/kvm: legacy_s390_alloc() only supports one allocation
>   s390x/kvm: indicate alignment in legacy_s390_alloc()
> 
>  target/s390x/kvm.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 

Thanks, applied.