Re: [RFCv2 15/37] KVM: s390: protvirt: Implement interruption injection

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

On 05.02.20 12:25, Cornelia Huck wrote:
> On Wed, 5 Feb 2020 11:48:42 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 05.02.20 10:51, Thomas Huth wrote:
>>> On 03/02/2020 14.19, Christian Borntraeger wrote:  
>>>> From: Michael Mueller <mimu@linux.ibm.com>
>>>>
>>>> The patch implements interruption injection for the following
>>>> list of interruption types:
>>>>
>>>>   - I/O
>>>>     __deliver_io (III)
>>>>
>>>>   - External
>>>>     __deliver_cpu_timer (IEI)
>>>>     __deliver_ckc (IEI)
>>>>     __deliver_emergency_signal (IEI)
>>>>     __deliver_external_call (IEI)
>>>>
>>>>   - cpu restart
>>>>     __deliver_restart (IRI)
>>>>
>>>> The service interrupt is handled in a followup patch.
>>>>
>>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> [fixes]
>>>> ---
>>>>  arch/s390/include/asm/kvm_host.h |  8 +++
>>>>  arch/s390/kvm/interrupt.c        | 93 ++++++++++++++++++++++----------
>>>>  2 files changed, 74 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>> index a45d10d87a8a..989cea7a5591 100644
>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>> @@ -563,6 +563,14 @@ enum irq_types {
>>>>  #define IRQ_PEND_MCHK_MASK ((1UL << IRQ_PEND_MCHK_REP) | \
>>>>  			    (1UL << IRQ_PEND_MCHK_EX))
>>>>  
>>>> +#define IRQ_PEND_MCHK_REP_MASK (1UL << IRQ_PEND_MCHK_REP)
>>>> +
>>>> +#define IRQ_PEND_EXT_II_MASK ((1UL << IRQ_PEND_EXT_CPU_TIMER)  | \
>>>> +			      (1UL << IRQ_PEND_EXT_CLOCK_COMP) | \
>>>> +			      (1UL << IRQ_PEND_EXT_EMERGENCY)  | \
>>>> +			      (1UL << IRQ_PEND_EXT_EXTERNAL)   | \
>>>> +			      (1UL << IRQ_PEND_EXT_SERVICE))
>>>> +
>>>>  struct kvm_s390_interrupt_info {
>>>>  	struct list_head list;
>>>>  	u64	type;
>>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>>> index c06c89d370a7..ecdec6960a60 100644
>>>> --- a/arch/s390/kvm/interrupt.c
>>>> +++ b/arch/s390/kvm/interrupt.c
>>>> @@ -387,6 +387,12 @@ static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
>>>>  		__clear_bit(IRQ_PEND_EXT_SERVICE, &active_mask);
>>>>  	if (psw_mchk_disabled(vcpu))
>>>>  		active_mask &= ~IRQ_PEND_MCHK_MASK;
>>>> +	/* PV guest cpus can have a single interruption injected at a time. */
>>>> +	if (kvm_s390_pv_is_protected(vcpu->kvm) &&
>>>> +	    vcpu->arch.sie_block->iictl != IICTL_CODE_NONE)
>>>> +		active_mask &= ~(IRQ_PEND_EXT_II_MASK |
>>>> +				 IRQ_PEND_IO_MASK |
>>>> +				 IRQ_PEND_MCHK_REP_MASK);  
>>>
>>> I don't quite understand why there is a difference between
>>> IRQ_PEND_MCHK_REP and IRQ_PEND_MCHK_EX here? Why not simply use
>>> IRQ_PEND_MCHK_MASK here? Could you elaborate? (and maybe add a sentence
>>> to the patch description)  
>>
>> I added that part. 
>> My idea was that an exigent machine check would be kind of fatal that it can override
>> the previous interrupt. Now we do not implement the override (kill the previous interrupt)
>> so I agree, maybe lets use IRQ_PEND_MCHK_MASK
> 
> This makes me wonder about interrupt priorities in general. Under which
> circumstances can we have an interrupt with a lower priority already
> injected (but not delivered) in the injection area when a
> higher-priority interrupt comes along? I'm a bit confused here.

This is not an issue. If the interrupt is already in the injection area, then you can consider
that as delivered. And even highest priority interrupt cannot overtake an already delivered one.