On 23.11.18 09:37, Michael Mueller wrote: > > > On 22.11.18 18:33, Pierre Morel wrote: >> On 22/11/2018 17:38, David Hildenbrand wrote: >>> On 22.11.18 12:21, Michael Mueller wrote: >>>> >>>> >>>> On 21.11.18 22:05, Halil Pasic wrote: >>>>> On Tue, 20 Nov 2018 13:00:03 +0100 >>>>> David Hildenbrand <david@xxxxxxxxxx> wrote: >>>>> >>>>>> On 20.11.18 12:33, Cornelia Huck wrote: >>>>>>> On Mon, 19 Nov 2018 18:25:25 +0100 >>>>>>> Michael Mueller <mimu@xxxxxxxxxxxxx> wrote: >>>>>>> >>>>>>>> Do not call __deliver_io() for adapter interruptions already >>>>>>>> pending in the IPM. That is a double effort. They will >>>>>>>> be processed as soon the vcpu control is given to SIE. >>>>>>>> >>>>>>>> Signed-off-by: Michael Mueller <mimu@xxxxxxxxxxxxx> >>>>>>>> --- >>>>>>>> arch/s390/kvm/interrupt.c | 54 >>>>>>>> ++++++++++++++++++++++------------------------- 1 file changed, 25 >>>>>>>> insertions(+), 29 deletions(-) >>>>>>> I think this patch does what it says on the tin, but I'm a bit lost >>>>>>> as to the why. (Might make more sense with the gib.) >>>>>>> >>>>>>> Currently, we are trying to process any I/O interrupts, even if we'd >>>>>>> get them delivered via the gisa, when we're out of the SIE anyway. >>>>>>> IIRC, while this looks a bit like a belt-and-suspenders approach, it >>>>>>> also prevented performance problems when the vcpu did not go back >>>>>>> into the SIE immediately (it even may exit to userspace). >>>>>>> >>>>>>> Also, if you're ignoring the I/O interrupts pending in the ipm, you >>>>>>> may end up delivering interrupts with a lower priority (higher isc) >>>>>>> first. I'm not sure that's what we want. >>>>>>> >>>>>>> But maybe I'm just missing another bit of the code that makes this >>>>>>> safe. Can you elaborate a bit? >>>>>>> >>>>>> For interrupt priorities to work at least somewhat predictable, we >>>>>> should always try to inject all interrupts even if the HW would be >>>>>> doing it for us. In the order of priority. >>>>>> >>>>>> But we don't have the same thing for external calls injected via SCA. >>>>>> I remember that I once had a patch lying around a couple of years ago >>>>>> to fix that ... it went missing :) >>>>>> >>>>>> IO interrupt almost have lowest priority, so letting the HW inject >>>>>> them could be problematic when mixing IO interrupt priorities between >>>>>> SW and HW injected ones (hat Conny described). >>>>>> >>>>>> There are other corner cases if a e.g. a RESTART interrupt is pending >>>>>> for that CPU. We would deliver eventually the RESTART interrupt before >>>>>> delivering the IO interrupt, which would be wrong. >>>>>> >>>>> I do share David's concern. Could somebody try to explain why this >>>>> RESTART scenario David described is not actually a problem -- AFAIU it >>>>> is a problem. >>>>> >>>>> Regards, >>>>> Halil >>>>> >>>> Before I start arguing why this is *not* a problem I ask you both why >>>> you consider >>>> this being a problem. We are talking about the CPU restart here, right? >>>> >>>> >>> >>> When sending a restart interrupt to a running CPU (e.g. system_reset) an >>> IO interrupt might remain pending and not delivered. >>> >>> One could make a guess how bad that is (depending on the type of guest >>> and use case), however it is guest observable difference to what is >>> documented in the PoP. Restart interrupt has (almost) lowest priority. >>> >> >> I do not see the priority as a problem. >> RESET and IO IRQ are asynchronous from each other anyway. >> >> But there I see another issue that your question point. >> >> Generally, on system RESET all peripheral also get a RESET and all >> interrupt are cleared. >> This is the case for AP VFIO devices. >> This is the case for local interrupts. Please don't confuse SIGP RESTART (e.g. RESTART interrupt any CPU can happily send to another CPU) with system resets. For ordinary SIGP RESTARTs, no floating interrupts are cleared. However, the question is if the asynchronous nature of IO interrupts indeed give no guarantees in respect to priorities against RESTART interrupts (IOW, will it be observerable). I'll have to think about this a little bit longer :) >> >> I miss the GISA reset code to avoid that the adapter IO IRQ is cleared >> on RESET. >> >> May be I did not look at the right place. > > See kvm_s390_clear_float_irqs() and how it as called. > >> >> Regards, >> Pierre >> >> > -- Thanks, David / dhildenb
