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

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 03/01/2018 04:58 AM, Thomas Huth 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?

You already added Paolo, David - good.
I am open to that. As an alternative we can certainly move this function
from s390x/kvm.c to exec.c at a later point in time if a 2nd user comes along.

> 
>> 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?

yes.
> 
>> +{
>> +    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() ?

yes.
> 
>> +}
>> +
>>  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.

Thanks. Will wait with the resend till Paolo,David have some comments.