[v2,09/12] KVM: s390: make kvm_s390_get_io_int() aware of GISA

Commit Message

Christian Borntraeger Jan. 25, 2018, 1:28 p.m. UTC

From: Michael Mueller <[e-mail]>

The function returns a pending I/O interrupt with the highest
priority defined by its ISC.

Together with AIV activation, pending adapter interrupts are
managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
inspect the IPM as well when the interrupt with the highest
priority has to be identified.

In case classic and adapter interrupts with the same ISC are
pending, the classic interrupt will be returned first.

Signed-off-by: Michael Mueller <[e-mail]>
Reviewed-by: Halil Pasic <[e-mail]>
Reviewed-by: Christian Borntraeger <[e-mail]>
Signed-off-by: Christian Borntraeger <[e-mail]>
---
 arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 6 deletions(-)

Comments

Cornelia Huck Jan. 26, 2018, 9:41 a.m. UTC | #1

On Thu, 25 Jan 2018 14:28:45 +0100
Christian Borntraeger <[e-mail]> wrote:

> From: Michael Mueller <[e-mail]>
> 
> The function returns a pending I/O interrupt with the highest
> priority defined by its ISC.
> 
> Together with AIV activation, pending adapter interrupts are
> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> inspect the IPM as well when the interrupt with the highest
> priority has to be identified.
> 
> In case classic and adapter interrupts with the same ISC are
> pending, the classic interrupt will be returned first.

Can this lead to starving? Consider a guest that never enables itself
for I/O interrupts, but collects pending interrupts via tpi. It will
always get the intis for an isc, but not the ai, wouldn't it?

> 
> Signed-off-by: Michael Mueller <[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> Reviewed-by: Christian Borntraeger <[e-mail]>
> Signed-off-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)

Christian Borntraeger Jan. 26, 2018, 9:57 a.m. UTC | #2

On 01/26/2018 10:41 AM, Cornelia Huck wrote:
> On Thu, 25 Jan 2018 14:28:45 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>> From: Michael Mueller <[e-mail]>
>>
>> The function returns a pending I/O interrupt with the highest
>> priority defined by its ISC.
>>
>> Together with AIV activation, pending adapter interrupts are
>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
>> inspect the IPM as well when the interrupt with the highest
>> priority has to be identified.
>>
>> In case classic and adapter interrupts with the same ISC are
>> pending, the classic interrupt will be returned first.
> 
> Can this lead to starving? Consider a guest that never enables itself
> for I/O interrupts, but collects pending interrupts via tpi. It will
> always get the intis for an isc, but not the ai, wouldn't it?

Only if it handles the interrupts slower than new ones arrive, in that case
you have a problem anyway. When looking at sane configuration, this priority
makes sense as the classic interrupts are used for configuration type ccw,
while adapter interrupts are for data. You want to get the control changes
quickly. In a sane environment nobody would probably put devices with adapter
interrupts on the same isc as different devices with only classic interrupts.

But looking at your theoretical "tpi only" case. If your statement is correct
then you would also starve interrupts with lets say isc 4 when also interrupts
with isc3 are pending, since isc3 will always be preferred. And it did not
seem to be an issue in the real world. Or did I miss your point?

> 
>>
>> Signed-off-by: Michael Mueller <[e-mail]>
>> Reviewed-by: Halil Pasic <[e-mail]>
>> Reviewed-by: Christian Borntraeger <[e-mail]>
>> Signed-off-by: Christian Borntraeger <[e-mail]>
>> ---
>>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 65 insertions(+), 6 deletions(-)
>

Cornelia Huck Jan. 26, 2018, 11:21 a.m. UTC | #3

On Fri, 26 Jan 2018 10:57:32 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 01/26/2018 10:41 AM, Cornelia Huck wrote:
> > On Thu, 25 Jan 2018 14:28:45 +0100
> > Christian Borntraeger <[e-mail]> wrote:
> >   
> >> From: Michael Mueller <[e-mail]>
> >>
> >> The function returns a pending I/O interrupt with the highest
> >> priority defined by its ISC.
> >>
> >> Together with AIV activation, pending adapter interrupts are
> >> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> >> inspect the IPM as well when the interrupt with the highest
> >> priority has to be identified.
> >>
> >> In case classic and adapter interrupts with the same ISC are
> >> pending, the classic interrupt will be returned first.  
> > 
> > Can this lead to starving? Consider a guest that never enables itself
> > for I/O interrupts, but collects pending interrupts via tpi. It will
> > always get the intis for an isc, but not the ai, wouldn't it?  
> 
> Only if it handles the interrupts slower than new ones arrive, in that case
> you have a problem anyway. When looking at sane configuration, this priority
> makes sense as the classic interrupts are used for configuration type ccw,
> while adapter interrupts are for data. You want to get the control changes
> quickly. In a sane environment nobody would probably put devices with adapter
> interrupts on the same isc as different devices with only classic interrupts.

But if you have a lot of devices, all using the same isc, you might
have a lot of classic interrupts (for example, due to firing a volley
of channel programs at all subchannels) and they could starve out the
device(s) that are waiting for adapter interrupts.

It's probably not a problem with today's guests (due to the control vs.
data semantics you pointed out above), especially as the only guest I
know that does not enable interrupts is the s390-ccw bios. But maybe
add a comment?

> 
> But looking at your theoretical "tpi only" case. If your statement is correct
> then you would also starve interrupts with lets say isc 4 when also interrupts
> with isc3 are pending, since isc3 will always be preferred. And it did not
> seem to be an issue in the real world. Or did I miss your point?

That's how it supposed to work with different iscs. If you have a very
chatty device on isc 3 and enable iscs 3 and 4 in cr6, it may well
drone out a device on isc 4. But that's an issue with the setup done by
the guest; it needs to put the devices on sensible iscs and manipulate
cr6, if needed.

That said, the hypothetical tpi-only guest might work around the issue
by assigning different iscs for classic and adapter interrupts.

Christian Borntraeger Jan. 26, 2018, 11:25 a.m. UTC | #4

On 01/26/2018 12:21 PM, Cornelia Huck wrote:
> On Fri, 26 Jan 2018 10:57:32 +0100
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 01/26/2018 10:41 AM, Cornelia Huck wrote:
>>> On Thu, 25 Jan 2018 14:28:45 +0100
>>> Christian Borntraeger <[e-mail]> wrote:
>>>   
>>>> From: Michael Mueller <[e-mail]>
>>>>
>>>> The function returns a pending I/O interrupt with the highest
>>>> priority defined by its ISC.
>>>>
>>>> Together with AIV activation, pending adapter interrupts are
>>>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
>>>> inspect the IPM as well when the interrupt with the highest
>>>> priority has to be identified.
>>>>
>>>> In case classic and adapter interrupts with the same ISC are
>>>> pending, the classic interrupt will be returned first.  
>>>
>>> Can this lead to starving? Consider a guest that never enables itself
>>> for I/O interrupts, but collects pending interrupts via tpi. It will
>>> always get the intis for an isc, but not the ai, wouldn't it?  
>>
>> Only if it handles the interrupts slower than new ones arrive, in that case
>> you have a problem anyway. When looking at sane configuration, this priority
>> makes sense as the classic interrupts are used for configuration type ccw,
>> while adapter interrupts are for data. You want to get the control changes
>> quickly. In a sane environment nobody would probably put devices with adapter
>> interrupts on the same isc as different devices with only classic interrupts.
> 
> But if you have a lot of devices, all using the same isc, you might
> have a lot of classic interrupts (for example, due to firing a volley
> of channel programs at all subchannels) and they could starve out the
> device(s) that are waiting for adapter interrupts.
> 
> It's probably not a problem with today's guests (due to the control vs.
> data semantics you pointed out above), especially as the only guest I
> know that does not enable interrupts is the s390-ccw bios. But maybe
> add a comment?

Do you have some proposal for a comment? Then I can certainly add that.

Cornelia Huck Jan. 26, 2018, 11:40 a.m. UTC | #5

On Fri, 26 Jan 2018 12:25:03 +0100
Christian Borntraeger <[e-mail]> wrote:

> On 01/26/2018 12:21 PM, Cornelia Huck wrote:
> > On Fri, 26 Jan 2018 10:57:32 +0100
> > Christian Borntraeger <[e-mail]> wrote:
> >   
> >> On 01/26/2018 10:41 AM, Cornelia Huck wrote:  
> >>> On Thu, 25 Jan 2018 14:28:45 +0100
> >>> Christian Borntraeger <[e-mail]> wrote:
> >>>     
> >>>> From: Michael Mueller <[e-mail]>
> >>>>
> >>>> The function returns a pending I/O interrupt with the highest
> >>>> priority defined by its ISC.
> >>>>
> >>>> Together with AIV activation, pending adapter interrupts are
> >>>> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> >>>> inspect the IPM as well when the interrupt with the highest
> >>>> priority has to be identified.
> >>>>
> >>>> In case classic and adapter interrupts with the same ISC are
> >>>> pending, the classic interrupt will be returned first.    
> >>>
> >>> Can this lead to starving? Consider a guest that never enables itself
> >>> for I/O interrupts, but collects pending interrupts via tpi. It will
> >>> always get the intis for an isc, but not the ai, wouldn't it?    
> >>
> >> Only if it handles the interrupts slower than new ones arrive, in that case
> >> you have a problem anyway. When looking at sane configuration, this priority
> >> makes sense as the classic interrupts are used for configuration type ccw,
> >> while adapter interrupts are for data. You want to get the control changes
> >> quickly. In a sane environment nobody would probably put devices with adapter
> >> interrupts on the same isc as different devices with only classic interrupts.  
> > 
> > But if you have a lot of devices, all using the same isc, you might
> > have a lot of classic interrupts (for example, due to firing a volley
> > of channel programs at all subchannels) and they could starve out the
> > device(s) that are waiting for adapter interrupts.
> > 
> > It's probably not a problem with today's guests (due to the control vs.
> > data semantics you pointed out above), especially as the only guest I
> > know that does not enable interrupts is the s390-ccw bios. But maybe
> > add a comment?  
> 
> Do you have some proposal for a comment? Then I can certainly add that.
> 

/*
 * Note that for a guest that does not enable I/O interrupts
 * but relies on TPI, a flood of classic interrupts may starve
 * out adapter interrupts on the same isc. Linux does not do
 * that, and it is possible to work around the issue by configuring
 * different iscs for classic and adapter interrupts in the guest,
 * but we may want to revisit this in the future.
 */

I think we don't really need to spend more time on that right now (and
probably complicate the code), so with the comment

Reviewed-by: Cornelia Huck <[e-mail]>

Christian Borntraeger Jan. 26, 2018, 1:13 p.m. UTC | #6

On 01/25/2018 02:28 PM, Christian Borntraeger wrote:
> From: Michael Mueller <[e-mail]>
> 
> The function returns a pending I/O interrupt with the highest
> priority defined by its ISC.
> 
> Together with AIV activation, pending adapter interrupts are
> managed by the GISA IPM. Thus kvm_s390_get_io_int() needs to
> inspect the IPM as well when the interrupt with the highest
> priority has to be identified.
> 
> In case classic and adapter interrupts with the same ISC are
> pending, the classic interrupt will be returned first.
> 
> Signed-off-by: Michael Mueller <[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> Reviewed-by: Christian Borntraeger <[e-mail]>
> Signed-off-by: Christian Borntraeger <[e-mail]>
> ---
>  arch/s390/kvm/interrupt.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 0edf97a..5c48930 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1471,12 +1471,8 @@ static struct kvm_s390_interrupt_info *get_io_int(struct kvm *kvm,
>  	return NULL;
>  }
> 
> -/*
> - * Dequeue and return an I/O interrupt matching any of the interruption
> - * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
> - */
> -struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
> -						    u64 isc_mask, u32 schid)
> +static struct kvm_s390_interrupt_info *get_top_io_int(struct kvm *kvm,
> +						      u64 isc_mask, u32 schid)
>  {
>  	struct kvm_s390_interrupt_info *inti = NULL;
>  	int isc;
> @@ -1488,6 +1484,69 @@ struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
>  	return inti;
>  }
> 
> +static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
> +{
> +	unsigned long active_mask;
> +	int isc;
> +
> +	if (schid)
> +		goto out;
> +	if (!kvm->arch.gisa)
> +		goto out;
> +
> +	active_mask = (isc_mask & kvm_s390_gisa_get_ipm(kvm->arch.gisa) << 24) << 32;
> +	while (active_mask) {
> +		isc = __flogr(active_mask);

Sebastian Ott noticed that newer binutils will complain about that when
compiling for z990 or older.

I will use the equivalent

-               isc = __flogr(active_mask);
+               isc = __fls(active_mask) ^ (BITS_PER_LONG - 1);

Patch

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 0edf97a..5c48930 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1471,12 +1471,8 @@  static struct kvm_s390_interrupt_info *get_io_int(struct kvm *kvm,
 	return NULL;
 }
 
-/*
- * Dequeue and return an I/O interrupt matching any of the interruption
- * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
- */
-struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
-						    u64 isc_mask, u32 schid)
+static struct kvm_s390_interrupt_info *get_top_io_int(struct kvm *kvm,
+						      u64 isc_mask, u32 schid)
 {
 	struct kvm_s390_interrupt_info *inti = NULL;
 	int isc;
@@ -1488,6 +1484,69 @@  struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
 	return inti;
 }
 
+static int get_top_gisa_isc(struct kvm *kvm, u64 isc_mask, u32 schid)
+{
+	unsigned long active_mask;
+	int isc;
+
+	if (schid)
+		goto out;
+	if (!kvm->arch.gisa)
+		goto out;
+
+	active_mask = (isc_mask & kvm_s390_gisa_get_ipm(kvm->arch.gisa) << 24) << 32;
+	while (active_mask) {
+		isc = __flogr(active_mask);
+		if (kvm_s390_gisa_tac_ipm_gisc(kvm->arch.gisa, isc))
+			return isc;
+		clear_bit_inv(isc, &active_mask);
+	}
+out:
+	return -EINVAL;
+}
+
+/*
+ * Dequeue and return an I/O interrupt matching any of the interruption
+ * subclasses as designated by the isc mask in cr6 and the schid (if != 0).
+ * Take into account the interrupts pending in the interrupt list and in GISA.
+ */
+struct kvm_s390_interrupt_info *kvm_s390_get_io_int(struct kvm *kvm,
+						    u64 isc_mask, u32 schid)
+{
+	struct kvm_s390_interrupt_info *inti, *tmp_inti;
+	int isc;
+
+	inti = get_top_io_int(kvm, isc_mask, schid);
+
+	isc = get_top_gisa_isc(kvm, isc_mask, schid);
+	if (isc < 0)
+		/* no AI in GISA */
+		goto out;
+
+	if (!inti)
+		/* AI in GISA but no classical IO int */
+		goto gisa_out;
+
+	/* both types of interrupts present */
+	if (int_word_to_isc(inti->io.io_int_word) <= isc) {
+		/* classical IO int with higher priority */
+		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+		goto out;
+	}
+gisa_out:
+	tmp_inti = kzalloc(sizeof(*inti), GFP_KERNEL);
+	if (tmp_inti) {
+		tmp_inti->type = KVM_S390_INT_IO(1, 0, 0, 0);
+		tmp_inti->io.io_int_word = isc_to_int_word(isc);
+		if (inti)
+			kvm_s390_reinject_io_int(kvm, inti);
+		inti = tmp_inti;
+	} else
+		kvm_s390_gisa_set_ipm_gisc(kvm->arch.gisa, isc);
+out:
+	return inti;
+}
+
 #define SCCB_MASK 0xFFFFFFF8
 #define SCCB_EVENT_PENDING 0x3