Re: [PATCH v1 1/2] s390x/kvm: Get rid of legacy_s390_alloc()

From: Igor Mammedov <imammedo@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: qemu-devel@nongnu.org, Thomas Huth <thuth@redhat.com>,
	kvm@vger.kernel.org, Cornelia Huck <cohuck@redhat.com>,
	Peter Xu <peterx@redhat.com>, Halil Pasic <pasic@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	qemu-s390x@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v1 1/2] s390x/kvm: Get rid of legacy_s390_alloc()
Date: Wed, 3 Mar 2021 17:13:14 +0100	[thread overview]
Message-ID: <20210303171314.7a24b29e@MiWiFi-RA69-srv> (raw)
In-Reply-To: <20210303130916.22553-2-david@redhat.com>

On Wed,  3 Mar 2021 14:09:15 +0100
David Hildenbrand <david@redhat.com> wrote:

> legacy_s390_alloc() was required for dealing with the absence of the ESOP
> feature -- on old HW (< gen 10) and old z/VM versions (< 6.3).
> 
> As z/VM v6.2 (and even v6.3) is no longer supported since 2017 [1]
> and we don't expect to have real users on such old hardware, let's drop
> legacy_s390_alloc().
> 
> Still check+report an error just in case someone still runs on
> such old z/VM environments, or someone runs under weird nested KVM
> setups (where we can manually disable ESOP via the CPU model).
> 
> No need to check for KVM_CAP_GMAP - that should always be around on
> kernels that also have KVM_CAP_DEVICE_CTRL (>= v3.15).
> 
> [1] https://www.ibm.com/support/lifecycle/search?q=z%2FVM
> 
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/s390x/kvm.c | 43 +++++--------------------------------------
>  1 file changed, 5 insertions(+), 38 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 7a892d663d..84b40572f2 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -161,8 +161,6 @@ static int cap_protected;
>  
>  static int active_cmma;
>  
> -static void *legacy_s390_alloc(size_t size, uint64_t *align, bool shared);
> -
>  static int kvm_s390_query_mem_limit(uint64_t *memory_limit)
>  {
>      struct kvm_device_attr attr = {
> @@ -349,6 +347,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>                       "please use kernel 3.15 or newer");
>          return -1;
>      }
> +    if (!kvm_check_extension(s, KVM_CAP_S390_COW)) {
> +        error_report("KVM is missing capability KVM_CAP_S390_COW - "
> +                     "unsupported environment");
> +        return -1;
> +    }
>  
>      cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
> @@ -357,11 +360,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>      cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>  
> -    if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
> -        || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> -        phys_mem_set_alloc(legacy_s390_alloc);
> -    }
> -
>      kvm_vm_enable_cap(s, KVM_CAP_S390_USER_SIGP, 0);
>      kvm_vm_enable_cap(s, KVM_CAP_S390_VECTOR_REGISTERS, 0);
>      kvm_vm_enable_cap(s, KVM_CAP_S390_USER_STSI, 0);
> @@ -889,37 +887,6 @@ int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
>      return ret;
>  }
>  
> -/*
> - * Legacy layout for s390:
> - * Older S390 KVM requires the topmost vma of the RAM to be
> - * smaller than an system defined value, which is at least 256GB.
> - * Larger systems have larger values. We put the guest between
> - * the end of data segment (system break) and this value. We
> - * use 32GB as a base to have enough room for the system break
> - * to grow. We also have to use MAP parameters that avoid
> - * read-only mapping of guest pages.
> - */
> -static void *legacy_s390_alloc(size_t size, uint64_t *align, bool shared)
> -{
> -    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);
> -    if (mem == MAP_FAILED) {
> -        mem = NULL;
> -    }
> -    if (mem && align) {
> -        *align = QEMU_VMALLOC_ALIGN;
> -    }
> -    return mem;
> -}
> -
>  static uint8_t const *sw_bp_inst;
>  static uint8_t sw_bp_ilen;
>