Re: [PATCH 1/1] KVM: s390: diag9c forwarding

From: Christian Borntraeger <borntraeger@de.ibm.com>
To: Janosch Frank <frankja@linux.ibm.com>,
	Janosch Frank <frankja@linux.vnet.ibm.com>
Cc: KVM <kvm@vger.kernel.org>, Cornelia Huck <cohuck@redhat.com>,
	David Hildenbrand <david@redhat.com>,
	linux-s390 <linux-s390@vger.kernel.org>,
	Pierre Morel <pmorel@linux.ibm.com>,
	Thomas Huth <thuth@redhat.com>
Subject: Re: [PATCH 1/1] KVM: s390: diag9c forwarding
Date: Mon, 18 Jan 2021 14:59:50 +0100	[thread overview]
Message-ID: <1533de16-0426-d33f-45ad-576cc2348f13@de.ibm.com> (raw)
In-Reply-To: <db1d2a6e-1947-321b-bdc2-019eee5780f4@linux.ibm.com>

On 18.01.21 14:45, Janosch Frank wrote:
> On 1/18/21 2:17 PM, Christian Borntraeger wrote:
>> From: Pierre Morel <pmorel@linux.ibm.com>
>>
>> When we receive intercept a DIAG_9C from the guest we verify
>> that the target real CPU associated with the virtual CPU
>> designated by the guest is running and if not we forward the
>> DIAG_9C to the target real CPU.
>>
>> To avoid a diag9c storm we allow a maximal rate of diag9c forwarding.
>>
>> The rate is calculated as a count per second defined as a
>> new parameter of the s390 kvm module: diag9c_forwarding_hz .
>>
>> The default value is to not forward diag9c.
> 
> Before Conny starts yelling I'll do it myself:
> Documentation
> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/include/asm/kvm_host.h |  1 +
>>  arch/s390/include/asm/smp.h      |  1 +
>>  arch/s390/kernel/smp.c           |  1 +
>>  arch/s390/kvm/diag.c             | 31 ++++++++++++++++++++++++++++---
>>  arch/s390/kvm/kvm-s390.c         |  6 ++++++
>>  arch/s390/kvm/kvm-s390.h         |  8 ++++++++
>>  6 files changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 74f9a036bab2..98ae55f79620 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -455,6 +455,7 @@ struct kvm_vcpu_stat {
>>  	u64 diagnose_44;
>>  	u64 diagnose_9c;
>>  	u64 diagnose_9c_ignored;
>> +	u64 diagnose_9c_forward;
>>  	u64 diagnose_258;
>>  	u64 diagnose_308;
>>  	u64 diagnose_500;
>> diff --git a/arch/s390/include/asm/smp.h b/arch/s390/include/asm/smp.h
>> index 01e360004481..e317fd4866c1 100644
>> --- a/arch/s390/include/asm/smp.h
>> +++ b/arch/s390/include/asm/smp.h
>> @@ -63,5 +63,6 @@ extern void __noreturn cpu_die(void);
>>  extern void __cpu_die(unsigned int cpu);
>>  extern int __cpu_disable(void);
>>  extern void schedule_mcck_handler(void);
>> +void notrace smp_yield_cpu(int cpu);
>>  
>>  #endif /* __ASM_SMP_H */
>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>> index c5abbb94ac6e..32622e9c15f0 100644
>> --- a/arch/s390/kernel/smp.c
>> +++ b/arch/s390/kernel/smp.c
>> @@ -422,6 +422,7 @@ void notrace smp_yield_cpu(int cpu)
>>  	asm volatile("diag %0,0,0x9c"
>>  		     : : "d" (pcpu_devices[cpu].address));
>>  }
>> +EXPORT_SYMBOL(smp_yield_cpu);
>>  
>>  /*
>>   * Send cpus emergency shutdown signal. This gives the cpus the
>> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
>> index 5b8ec1c447e1..fc1ec4aa81ed 100644
>> --- a/arch/s390/kvm/diag.c
>> +++ b/arch/s390/kvm/diag.c
>> @@ -150,6 +150,19 @@ static int __diag_time_slice_end(struct kvm_vcpu *vcpu)
>>  	return 0;
>>  }
>>  
>> +static unsigned int forward_cnt;
> 
> This is not per CPU, so we could have one CPU making forwards impossible
> for all others, right? Would this be a possible improvement or doesn't
> that happen in real world workloads?

this is mostly a system limit to avoid non-root users being able to trigger
gazillions of diag9c hypercalls. 