Re: [PATCH v3 1/1] s390:kvm: diag9c forwarding

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/19/21 5:09 PM, Cornelia Huck wrote:
> On Mon, 15 Feb 2021 17:06:50 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> Make $SUBJECT
> 
> "KVM: s390: diag9c (directed yield) forwarding" ?
> 
>> When we receive intercept a DIAG_9C from the guest we verify
> 
> Either 'receive' or 'intercept', I guess :)

right.

...snip...

>> +DIAG 'X'9C forwarding
>> ++++++++++++++++++++++
>> +
>> +Under KVM, the guest operating system may send a DIAGNOSE code 'X'9C to
>> +the host when it fails to acquire a spinlock for a virtual CPU
>> +and detects that the host CPU on which the virtual guest CPU owner is
>> +assigned to is not running to try to get this host CPU running and
>> +consequently the guest virtual CPU running and freeing the lock.
> 
> What about:
> 
> "The guest may send a DIAGNOSE 0x9c in order to yield to a certain
> other vcpu. An example is a Linux guest that tries to yield to the vcpu
> that is currently holding a spinlock, but not running."

Yes, thanks.

> 
>> +
>> +However, on the logical partition the real CPU on which the previously
>> +targeted host CPU is assign may itself not be running.
> 
> "However, on the host the real cpu backing the vcpu may itself not be
> running."

Yes, better too, thanks.

> 
>> +By forwarding the DIAGNOSE code 'X'9C, initially sent by the guest,
>> +from the host to LPAR hypervisor, this one will hopefully schedule
>> +the host CPU which will let KVM run the target guest CPU.
> 
> "Forwarding the DIAGNOSE 0x9c initially sent by the guest to yield to
> the backing cpu will hopefully cause that cpu, and thus subsequently
> the guest's vcpu, to be scheduled."

yes.

> 
> [I don't think we should explicitly talk about LPAR here, as the same
> should apply if we are running second-or-deeper level, right?]

yes right.

> 
>> +
>> +diag9c_forwarding_hz
>> +    KVM kernel parameter allowing to specify the maximum number of DIAGNOSE
>> +    'X'9C forwarding per second in the purpose of avoiding a DIAGNOSE 'X'9C
>> +    forwarding storm.
> 
> I think 0x9c is the more common way to write the hex code.

yes it is the purpose was to keep it consistent with the existing 
documentation

> 
> Also,
> 
> "A value of 0 turns the forwarding off" ?

yes, I can add this explicitly.

> 
> (...)
> 
>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>> index 27c763014114..15e207a671fd 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);
> 
> EXPORT_SYMBOL_GPL?

OK, clear.

> 
>>   
>>   /*
>>    * Send cpus emergency shutdown signal. This gives the cpus the
> 
> (...)
> 
>> @@ -190,6 +191,11 @@ static bool use_gisa  = true;
>>   module_param(use_gisa, bool, 0644);
>>   MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it.");
>>   
>> +/* maximum diag9c forwarding per second */
>> +unsigned int diag9c_forwarding_hz;
>> +module_param(diag9c_forwarding_hz, uint, 0644);
>> +MODULE_PARM_DESC(diag9c_forwarding_hz, "Maximum diag9c forwarding per second");
> 
> Maybe also add "(0 to turn off forwarding)" here?

OK, I will add it.

Thanks for the comments,
Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen