Re: [Qemu-devel] [PATCH 1/1] s390/kvm: implement clearing part of IPL clear

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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 <borntraeger@de.ibm.com>
> >>> ---
> >>>  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.

> The right thing here would be to actually terminate the postcopy migrate but
> return it as "successful" (since we are going to clear that RAM anyway). Do 
> you see a good way to achieve that?

There's no current mechanism to do it; I think it would have to involve
some interaction with the source as well though to tell it that you
didn't need that area of RAM anyway.

However, there are more problems:
  a) Even forgetting the userfault problem, this is racy since during
postcopy you're still receiving blocks from the source at the same time;
so some of the area that you've discarded might get overwritten by data
from the source.

  b) Your release_all_rams seems to do all RAM Blocks - won't that nuke
any ROMs as well? Or maybe even flash?

  c) In a normal precopy migration, I think you may also get old data;
Paolo said that an MADV_DONTNEED won't cause the dirty flags to be set,
so if the migrate has already sent the data for a page, and then this
happens, before the CPUs are stopped during the migration, when you
restart on the destination you'll have the old data.

Dave

> 
> > 
> > Dave
> > 
> >>> +}
> >>> +
> >>>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>>  {
> >>>      S390CPU *cpu = S390_CPU(cs);
> >>> @@ -1853,6 +1864,14 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>>              ret = handle_intercept(cpu);
> >>>              break;
> >>>          case KVM_EXIT_S390_RESET:
> >>> +            if (run->s390_reset_flags & KVM_S390_RESET_CLEAR) {
> >>> +                /*
> >>> +                 * We will stop other CPUs anyway, avoid spurious crashes and
> >>> +                 * get all CPUs out. The reset will take care of the resume.
> >>> +                 */
> >>> +                pause_all_vcpus();
> >>> +                release_all_rams();
> >>> +            }
> >>>              s390_reipl_request();
> >>>              break;
> >>>          case KVM_EXIT_S390_TSCH:
> >>>
> >>
> >> Apart from the cosmetic nits, patch looks good to me.
> >>
> >>  Thomas
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK