From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52927) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1erNVB-0000x6-1C for qemu-devel@nongnu.org; Thu, 01 Mar 2018 07:40:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1erNV7-0001e2-QX for qemu-devel@nongnu.org; Thu, 01 Mar 2018 07:40:01 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:43546 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1erNV7-0001dm-Ew for qemu-devel@nongnu.org; Thu, 01 Mar 2018 07:39:57 -0500 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w21CdCue026684 for ; Thu, 1 Mar 2018 07:39:56 -0500 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0b-001b2d01.pphosted.com with ESMTP id 2geg5fupjy-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 01 Mar 2018 07:39:56 -0500 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 1 Mar 2018 12:39:54 -0000 From: Christian Borntraeger References: <20180228195320.165230-1-borntraeger@de.ibm.com> <79f7059b-f2d3-a758-6bb9-29433b31b313@redhat.com> <20180301092442.GA2994@work-vm> <20180301114543.GC2994@work-vm> <69654fb2-f5ba-c23b-f6f5-1b559692cf37@de.ibm.com> <20180301122854.GD2994@work-vm> <416dbf71-c311-1f40-baa6-414cdf220f4d@de.ibm.com> Date: Thu, 1 Mar 2018 13:39:50 +0100 MIME-Version: 1.0 In-Reply-To: <416dbf71-c311-1f40-baa6-414cdf220f4d@de.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: Subject: Re: [Qemu-devel] [PATCH 1/1] s390/kvm: implement clearing part of IPL clear List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Thomas Huth , qemu-s390x , qemu-devel , Cornelia Huck , David Hildenbrand , Halil Pasic , Janosch Frank , Paolo Bonzini On 03/01/2018 01:35 PM, Christian Borntraeger wrote: > > > On 03/01/2018 01:28 PM, Dr. David Alan Gilbert wrote: >> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: >>> >>> >>> On 03/01/2018 12:45 PM, Dr. David Alan Gilbert wrote: >>>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote: >>>>> >>>>> >>>>> On 03/01/2018 10:24 AM, Dr. David Alan Gilbert wrote: >>>>>> * Thomas Huth (thuth@redhat.com) wrote: >>>>>>> On 28.02.2018 20:53, Christian Borntraeger wrote: >>>>>>>> When a guests reboots with diagnose 308 subcode 3 it requests the memory >>>>>>>> to be cleared. We did not do it so far. This does not only violate the >>>>>>>> architecture, it also misses the chance to free up that memory on >>>>>>>> reboot, which would help on host memory over commitment. By using >>>>>>>> ram_block_discard_range we can cover both cases. >>>>>>> >>>>>>> Sounds like a good idea. I wonder whether that release_all_ram() >>>>>>> function should maybe rather reside in exec.c, so that other machines >>>>>>> that want to clear all RAM at reset time can use it, too? >>>>>>> >>>>>>>> Signed-off-by: Christian Borntraeger >>>>>>>> --- >>>>>>>> target/s390x/kvm.c | 19 +++++++++++++++++++ >>>>>>>> 1 file changed, 19 insertions(+) >>>>>>>> >>>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>>>>>> index 8f3a422288..2e145ad5c3 100644 >>>>>>>> --- a/target/s390x/kvm.c >>>>>>>> +++ b/target/s390x/kvm.c >>>>>>>> @@ -34,6 +34,8 @@ >>>>>>>> #include "qapi/error.h" >>>>>>>> #include "qemu/error-report.h" >>>>>>>> #include "qemu/timer.h" >>>>>>>> +#include "qemu/rcu_queue.h" >>>>>>>> +#include "sysemu/cpus.h" >>>>>>>> #include "sysemu/sysemu.h" >>>>>>>> #include "sysemu/hw_accel.h" >>>>>>>> #include "hw/boards.h" >>>>>>>> @@ -41,6 +43,7 @@ >>>>>>>> #include "sysemu/device_tree.h" >>>>>>>> #include "exec/gdbstub.h" >>>>>>>> #include "exec/address-spaces.h" >>>>>>>> +#include "exec/ram_addr.h" >>>>>>>> #include "trace.h" >>>>>>>> #include "qapi-event.h" >>>>>>>> #include "hw/s390x/s390-pci-inst.h" >>>>>>>> @@ -1841,6 +1844,14 @@ static int kvm_arch_handle_debug_exit(S390CPU *cpu) >>>>>>>> return ret; >>>>>>>> } >>>>>>>> >>>>>>>> +static void release_all_rams(void) >>>>>>> >>>>>>> s/rams/ram/ maybe? >>>>>>> >>>>>>>> +{ >>>>>>>> + struct RAMBlock *rb; >>>>>>>> + >>>>>>>> + QLIST_FOREACH_RCU(rb, &ram_list.blocks, next) >>>>>>>> + ram_block_discard_range(rb, 0, rb->used_length); >>>>>>> >>>>>>> From a coding style point of view, I think there should be curly braces >>>>>>> around ram_block_discard_range() ? >>>>>> >>>>>> I think this might break if it happens during a postcopy migrate. >>>>>> The destination CPU is running, so it can do a reboot at just the wrong >>>>>> time; and then the pages (that are protected by userfaultfd) would get >>>>>> deallocated and trigger userfaultfd requests if accessed. >>>>> >>>>> Yes, userfaultd/postcopy is really fragile and relies on things that are not >>>>> necessarily true (e.g. virito-balloon can also invalidate pages). >>>> >>>> That's why we use qemu_balloon_inhibit around postcopy to stop >>>> ballooning; I'm not aware of anything else that does the same. >>> >>> we also have at least the pte_unused thing in mm/rmap.c that clearly >>> predates userfaultfd. We might need to look into this as well.... >> >> I've not come across that; what does that do? > > It can drop a page on page out if the page is no longer of value. It is used by > the CMMA (guest page hinting) code of s390x. > > see kernel mm/rmap.c > > > static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > unsigned long address, void *arg) > { > [...] > } else if (pte_unused(pteval)) { > /* > * The guest indicated that the page content is of no > * interest anymore. Simply discard the pte, vmscan > * will take care of the rest. > */ > dec_mm_counter(mm, mm_counter(page)); > /* We have to invalidate as we cleared the pte */ > mmu_notifier_invalidate_range(mm, address, > address + PAGE_SIZE); > } else if (IS_ENABLED(CONFIG_MIGRATION) && > (flags & (TTU_MIGRATION|TTU_SPLIT_FREEZE))) { > [...] > > Maybe something like this in the kernel diff --git a/mm/rmap.c b/mm/rmap.c index 47db27f8049e..9bdf4d448987 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1483,7 +1483,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, set_pte_at(mm, address, pvmw.pte, pteval); } - } else if (pte_unused(pteval)) { + } else if (pte_unused(pteval) && !vma->vm_userfaultfd_ctx.ctx) { /* * The guest indicated that the page content is of no * interest anymore. Simply discard the pte, vmscan could help?