Re: [PATCH 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

Re: [PATCH 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

[Cornelia Huck]

On Thu, 25 Oct 2018 14:37:50 +0200
Michael Mueller <mimu@linux.ibm.com> wrote:

> The patch implements a handler for GIB alert interruptions
> on the host. Its task is to alert storage backed guests that
> interrupts are pending for them.
> 
> A GIB alert interrupt statistic counter is added as well:
> 
> $ cat /proc/interrupts
>           CPU0       CPU1
>   ...
>   GAL:       0          0   [I/O] GIB Alert
>   ...
> 
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>  arch/s390/include/asm/irq.h |  1 +
>  arch/s390/include/asm/isc.h |  1 +
>  arch/s390/kernel/irq.c      |  1 +
>  arch/s390/kvm/interrupt.c   | 45 +++++++++++++++++++++++++++++++++----
>  arch/s390/kvm/kvm-s390.c    |  5 +++++
>  5 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/irq.h b/arch/s390/include/asm/irq.h
> index 2f7f27e5493f..afaf5e3c57fd 100644
> --- a/arch/s390/include/asm/irq.h
> +++ b/arch/s390/include/asm/irq.h
> @@ -62,6 +62,7 @@ enum interruption_class {
>  	IRQIO_MSI,
>  	IRQIO_VIR,
>  	IRQIO_VAI,
> +	IRQIO_GAL,
>  	NMI_NMI,
>  	CPU_RST,
>  	NR_ARCH_IRQS
> diff --git a/arch/s390/include/asm/isc.h b/arch/s390/include/asm/isc.h
> index 6cb9e2ed05b6..b2cc1ec78d06 100644
> --- a/arch/s390/include/asm/isc.h
> +++ b/arch/s390/include/asm/isc.h
> @@ -21,6 +21,7 @@
>  /* Adapter interrupts. */
>  #define QDIO_AIRQ_ISC IO_SCH_ISC	/* I/O subchannel in qdio mode */
>  #define PCI_ISC 2			/* PCI I/O subchannels */
> +#define GAL_ISC 5			/* GIB alert */

Dumb question: iscs are ordered by priority. What are the semantics
here? Are gib alerts only for ap-style adapter interrupts (at least
currently, it seems to me like that)? Is there a requirement to use a
distinct isc?

>  #define AP_ISC 6			/* adjunct processor (crypto) devices */
>  
>  /* Functions for registration of I/O interruption subclasses */

Re: [PATCH 09/10] KVM: s390: add and wire function gib_alert_irq_handler()

[Pierre Morel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

On 07/11/2018 17:25, Michael Mueller wrote:
> 
> 
> On 06.11.18 19:42, Pierre Morel wrote:
>> On 05/11/2018 18:41, Michael Mueller wrote:
>>>
>>>
>>> On 31.10.18 13:10, Pierre Morel wrote:
>>>>> atomic_inc(&vcpu->kvm->arch.vcpus_in_sie);
>>>>> +��� if (vcpu->kvm->arch.gib_in_use)
>>>>> +������� vcpu->kvm->arch.gisa->iam = 0;
>>>>
>>>> I am not sure that this is race free....
>>>
>>> It is, I looked at the generated instructions.
>>
>> Here we clear IAM, so an IPM can be set and should trigger an 
>> interrupt in the guest
> 
> Setting or clearing the IAM has no effect whether a ISC can be set or 
> not in the IPM.
> It is cleared because we are about to enter the SIE and which will 
> evaluate the IPM and
> process the interruption possibly.
> 
> Clearing the IAM has the effect, that the fallback path (GIB alert) will 
> not be used.
> 
>>
>>>>
>>>>> vcpu->arch.sie_block->icptcode = 0;
>>>>> ����� cpuflags = atomic_read(&vcpu->arch.sie_block->cpuflags);
>>>>> @@ -3421,6 +3423,9 @@ static int vcpu_post_run(struct kvm_vcpu 
>>>>> *vcpu, int exit_reason)
>>>>> ����� vcpu->run->s.regs.gprs[15] = vcpu->arch.sie_block->gg15;
>>>>> � ����� atomic_dec(&vcpu->kvm->arch.vcpus_in_sie);
>>>>> +��� if (vcpu->kvm->arch.gib_in_use &&
>>>>> +������� !atomic_fetch_andnot(0, &vcpu->kvm->arch.vcpus_in_sie))
>>>>> +������� vcpu->kvm->arch.gisa->iam = vcpu->kvm->arch.iam;
>>>>
>>>> ... with this. Is it?
>>>
>>> the same here
>>
>> No, here we set the IAM if the interrupt comes after the vCPU sleeps 
>> and before we set the IAM we have a race window.
>>
>> What I mean is: here we should check the IPM bit to be sure not to 
>> loose an interrupt in case the IPM bit is also set.
> 
> The fallback path will be activated by setting the IAM here if no vCPU 
> of the guest is in SIE.
> 
> Yes, the ISC can be set in the IPM in the short window between exiting 
> the SIE and setting the IAM here.
> Do we loose the interruption because of that or is it just delayed? It 
> will be delivered as soon vCPU enters
> the SIE again. Can we speed up this? Maybe yes, that is further tuning.

OK, if the interruptions are delivered to the guest even if AIM is set 
then there is not problem.


> 
>>
>>>>
>>>>
>>>>> � ����� if (exit_reason == -EINTR) {
>>>>> ��������� VCPU_EVENT(vcpu, 3, "%s", "machine check");
>>>>>
>>>
>>
>>
> 


-- 
Pierre Morel
Linux/KVM/QEMU in B�blingen - Germany