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

[Christian Borntraeger]

On 03/01/2018 10:24 AM, Dr. David Alan Gilbert wrote:
> * Thomas Huth (address@hidden) 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 <address@hidden>
>>> ---
>>>  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).

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?


> 
> 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 / address@hidden / Manchester, UK
>