[Qemu-devel] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Qemu-devel] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Peter Maydell]

Hi; Coverity has I think enabled a new warning recently, which
is triggering on virtio_ccw_notify() in hw/s390x/virtio-ccw.c
(CID 1390619).

This function does
    indicators |= 1ULL << vector;
but the code is guarded only by
    if (vector < VIRTIO_QUEUE_MAX) {

That used to be OK when VIRTIO_QUEUE_MAX was 64, but in
commit b829c2a98f1 it was raised to 1024, and this is no longer
a useful guard. The commit message for b829c2a98f1 suggests that
this is a "can't happen" case -- is that so? If so then the
else {} part of the code and an earlier check on
"if (vector >= VIRTIO_QUEUE_MAX + 64)" are dead code.
However it looks like the device_plugged method is also
checking VIRTIO_QUEUE_MAX, rather than 64.

If this is a false positive, then an assert() in
virtio_ccw_notify() and cleaning up the dead code would
help placate coverity.

(Other odd code in that function:
    vector = 0;
    [...]
    indicators |= 1ULL << vector;
is that really supposed to ignore the input vector number?)

thanks
-- PMM

Re: [Qemu-devel] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Cornelia Huck]

On Mon, 14 May 2018 19:12:27 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> Hi; Coverity has I think enabled a new warning recently, which
> is triggering on virtio_ccw_notify() in hw/s390x/virtio-ccw.c
> (CID 1390619).
> 
> This function does
>     indicators |= 1ULL << vector;
> but the code is guarded only by
>     if (vector < VIRTIO_QUEUE_MAX) {
> 
> That used to be OK when VIRTIO_QUEUE_MAX was 64, but in
> commit b829c2a98f1 it was raised to 1024, and this is no longer
> a useful guard. The commit message for b829c2a98f1 suggests that
> this is a "can't happen" case -- is that so? 

That is outdated; we bumped max virtqueues for ccw in b1914b824ade.

> If so then the
> else {} part of the code and an earlier check on
> "if (vector >= VIRTIO_QUEUE_MAX + 64)" are dead code.
> However it looks like the device_plugged method is also
> checking VIRTIO_QUEUE_MAX, rather than 64.
> 
> If this is a false positive, then an assert() in
> virtio_ccw_notify() and cleaning up the dead code would
> help placate coverity.

It is a false positive as far as I can see; this is the notification
code for classical interrupts, and we fence off more than 64 virtqueues
already when the guest tries to set it up (introduced in 797b608638c5).
As that code flow is basically impossible to deduce by a static code
checker, adding an assert() seems like a good idea. Halil, what do you
think?

> 
> (Other odd code in that function:
>     vector = 0;
>     [...]
>     indicators |= 1ULL << vector;
> is that really supposed to ignore the input vector number?)

Yes; this as a configuration change notification done via secondary
indicators (different from either the classical indicators or the
adapter interrupt indicators). We always set the same bit, and it is
always triggered by doing a notification with a number one above the
maximum possible virtqueue number. (I agree that it does look odd, we
should maybe add a comment.)

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Halil Pasic]

On 05/15/2018 10:32 AM, Cornelia Huck wrote:
> On Mon, 14 May 2018 19:12:27 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
>> Hi; Coverity has I think enabled a new warning recently, which
>> is triggering on virtio_ccw_notify() in hw/s390x/virtio-ccw.c
>> (CID 1390619).
>>
>> This function does
>>      indicators |= 1ULL << vector;
>> but the code is guarded only by
>>      if (vector < VIRTIO_QUEUE_MAX) {
>>
>> That used to be OK when VIRTIO_QUEUE_MAX was 64, but in
>> commit b829c2a98f1 it was raised to 1024, and this is no longer
>> a useful guard. The commit message for b829c2a98f1 suggests that
>> this is a "can't happen" case -- is that so?

The logic behind this condition is the following. Vector either
a) stands for an used buffer notification and holds a virtqueue index, or
b) it stands for configuration change notification and holds VIRTIO_QUEUE_MAX.

The valid values for a virtqueue index are [0 .. VIRTIO_QUEUE_MAX -1]. For
a particular device only a subset of this set may be valid, but anything
outside is an invalid virtqueue index QEMU-wide.

> 
> That is outdated; we bumped max virtqueues for ccw in b1914b824ade.
> 

Right. With two stage indicators we can support VIRTIO_QUEUE_MAX queues,
but with classic indicators however we are stuck at 64 vritqueues at most.
We have  check for that (if interested look for
'if (virtio_get_num_queues(vdev) > NR_CLASSIC_INDICATOR_BITS) {').

>> If so then the
>> else {} part of the code and an earlier check on
>> "if (vector >= VIRTIO_QUEUE_MAX + 64)" are dead code.
>> However it looks like the device_plugged method is also
>> checking VIRTIO_QUEUE_MAX, rather than 64.
>>
>> If this is a false positive, then an assert() in
>> virtio_ccw_notify() and cleaning up the dead code would
>> help placate coverity.

The else is definitely not dead code, as I've explained above.

But vector >= VIRTIO_QUEUE_MAX + 64 should never be observed. Although
blame blames me, all I did was get rid of the transport specific limit:
-    if (vector >= VIRTIO_CCW_QUEUE_MAX + 64) {
+    if (vector >= VIRTIO_QUEUE_MAX + 64) {

The check however does not make much sense IMHO (and it did not back
then when I touched the code).

The vector values we can observe are AFAIU determined by:
git grep -e 'vdev->config_vector = ' -e 'virtio_queue_set_vector' -n  -- hw/s390x/virtio-ccw.c
and the possible values are [0..VIRTIO_QUEUE_MAX].

So we should really check for that. If it's better to do the check
via assert or log a warning and carry on without notifying, I'm
not sure.


> 
> It is a false positive as far as I can see; this is the notification
> code for classical interrupts, and we fence off more than 64 virtqueues
> already when the guest tries to set it up (introduced in 797b608638c5).
> As that code flow is basically impossible to deduce by a static code
> checker, adding an assert() seems like a good idea. Halil, what do you
> think?
> 

See all over the place ;)

>>
>> (Other odd code in that function:
>>      vector = 0;
>>      [...]
>>      indicators |= 1ULL << vector;
>> is that really supposed to ignore the input vector number?)

This is why the vector >= VIRTIO_QUEUE_MAX + 64 does not make sense. I
think this should be simplified. Overwriting the vector with zero and
doing the shift is just nonsensical.

To sum it up, my take on the whole is the diff below. I can convert
it to a proper patch if we agree that's the way to go.

Regards,
Halil

> 
> Yes; this as a configuration change notification done via secondary
> indicators (different from either the classical indicators or the
> adapter interrupt indicators). We always set the same bit, and it is
> always triggered by doing a notification with a number one above the
> maximum possible virtqueue number. (I agree that it does look odd, we
> should maybe add a comment.)
> 

----------------------------8<---------------------------------------

From: Halil Pasic <pasic@linux.ibm.com>
Date: Tue, 15 May 2018 13:57:44 +0200
Subject: [PATCH] WIP: cleanup virtio notify

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
  hw/s390x/virtio-ccw.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e51fbefd23..22078605d1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
      SubchDev *sch = ccw_dev->sch;
      uint64_t indicators;

-    /* queue indicators + secondary indicators */
-    if (vector >= VIRTIO_QUEUE_MAX + 64) {
-        return;
-    }
+    /* vector == VIRTIO_QUEUE_MAX means configuration change */
+    assert(vector <= VIRTIO_QUEUE_MAX);

      if (vector < VIRTIO_QUEUE_MAX) {
          if (!dev->indicators) {
@@ -1042,12 +1040,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
          if (!dev->indicators2) {
              return;
          }
-        vector = 0;
          indicators = address_space_ldq(&address_space_memory,
                                         dev->indicators2->addr,
                                         MEMTXATTRS_UNSPECIFIED,
                                         NULL);
-        indicators |= 1ULL << vector;
+        indicators |= 1ULL;
          address_space_stq(&address_space_memory, dev->indicators2->addr,
                            indicators, MEMTXATTRS_UNSPECIFIED, NULL);
          css_conditional_io_interrupt(sch);
-- 

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Peter Maydell]

On 15 May 2018 at 13:00, Halil Pasic <pasic@linux.ibm.com> wrote:
> To sum it up, my take on the whole is the diff below. I can convert
> it to a proper patch if we agree that's the way to go.
>
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Tue, 15 May 2018 13:57:44 +0200
> Subject: [PATCH] WIP: cleanup virtio notify
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index e51fbefd23..22078605d1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d,
> uint16_t vector)
>      SubchDev *sch = ccw_dev->sch;
>      uint64_t indicators;
>
> -    /* queue indicators + secondary indicators */
> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> -        return;
> -    }
> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */
> +    assert(vector <= VIRTIO_QUEUE_MAX);
>
>      if (vector < VIRTIO_QUEUE_MAX) {
>          if (!dev->indicators) {

This will not be sufficient to satisfy Coverity, because
VIRTIO_QUEUE_MAX is 1024, and that is way too big to use as
a shift count, and that's the only check that Coverity can see
on the code path leading into the "1ULL << vector" code.

If it's not possible to get here with a vector that's 64 or more,
you need to
    assert(vector < NR_CLASSIC_INDICATOR_BITS);
somewhere.

thanks
-- PMM

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Halil Pasic]

On 05/15/2018 02:07 PM, Peter Maydell wrote:
> On 15 May 2018 at 13:00, Halil Pasic <pasic@linux.ibm.com> wrote:
>> To sum it up, my take on the whole is the diff below. I can convert
>> it to a proper patch if we agree that's the way to go.
>>
>> From: Halil Pasic <pasic@linux.ibm.com>
>> Date: Tue, 15 May 2018 13:57:44 +0200
>> Subject: [PATCH] WIP: cleanup virtio notify
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> ---
>>   hw/s390x/virtio-ccw.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index e51fbefd23..22078605d1 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d,
>> uint16_t vector)
>>       SubchDev *sch = ccw_dev->sch;
>>       uint64_t indicators;
>>
>> -    /* queue indicators + secondary indicators */
>> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
>> -        return;
>> -    }
>> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */
>> +    assert(vector <= VIRTIO_QUEUE_MAX);
>>
>>       if (vector < VIRTIO_QUEUE_MAX) {
>>           if (!dev->indicators) {
> 
> This will not be sufficient to satisfy Coverity, because
> VIRTIO_QUEUE_MAX is 1024, and that is way too big to use as
> a shift count, and that's the only check that Coverity can see
> on the code path leading into the "1ULL << vector" code.
> 
> If it's not possible to get here with a vector that's 64 or more,
> you need to
>      assert(vector < NR_CLASSIC_INDICATOR_BITS);
> somewhere.

Right. Here is the revised version. It's the second hunk.

Regards,
Halil

> 
> thanks
> -- PMM
> 

--------------------------------8<------------------------------------------------
From: Halil Pasic <pasic@linux.ibm.com>
Date: Tue, 15 May 2018 13:57:44 +0200
Subject: [PATCH] WIP: cleanup virtio notify

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
  hw/s390x/virtio-ccw.c | 10 ++++------
  1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 22df33b509..be433b0336 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
      SubchDev *sch = ccw_dev->sch;
      uint64_t indicators;

-    /* queue indicators + secondary indicators */
-    if (vector >= VIRTIO_QUEUE_MAX + 64) {
-        return;
-    }
+    /* vector == VIRTIO_QUEUE_MAX means configuration change */
+    assert(vector <= VIRTIO_QUEUE_MAX);

      if (vector < VIRTIO_QUEUE_MAX) {
          if (!dev->indicators) {
@@ -1029,6 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
                  css_adapter_interrupt(CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc);
              }
          } else {
+            assert(vector < NR_CLASSIC_INDICATOR_BITS);
              indicators = address_space_ldq(&address_space_memory,
                                             dev->indicators->addr,
                                             MEMTXATTRS_UNSPECIFIED,
@@ -1042,12 +1041,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
          if (!dev->indicators2) {
              return;
          }
-        vector = 0;
          indicators = address_space_ldq(&address_space_memory,
                                         dev->indicators2->addr,
                                         MEMTXATTRS_UNSPECIFIED,
                                         NULL);
-        indicators |= 1ULL << vector;
+        indicators |= 1ULL;
          address_space_stq(&address_space_memory, dev->indicators2->addr,
                            indicators, MEMTXATTRS_UNSPECIFIED, NULL);
          css_conditional_io_interrupt(sch);

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Cornelia Huck]

On Tue, 15 May 2018 15:17:51 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:


> --------------------------------8<------------------------------------------------
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Tue, 15 May 2018 13:57:44 +0200
> Subject: [PATCH] WIP: cleanup virtio notify
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>   hw/s390x/virtio-ccw.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 22df33b509..be433b0336 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>       SubchDev *sch = ccw_dev->sch;
>       uint64_t indicators;
> 
> -    /* queue indicators + secondary indicators */
> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> -        return;
> -    }
> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */
> +    assert(vector <= VIRTIO_QUEUE_MAX);
> 
>       if (vector < VIRTIO_QUEUE_MAX) {
>           if (!dev->indicators) {
> @@ -1029,6 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>                   css_adapter_interrupt(CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc);
>               }
>           } else {
> +            assert(vector < NR_CLASSIC_INDICATOR_BITS);
>               indicators = address_space_ldq(&address_space_memory,
>                                              dev->indicators->addr,
>                                              MEMTXATTRS_UNSPECIFIED,
> @@ -1042,12 +1041,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>           if (!dev->indicators2) {
>               return;
>           }
> -        vector = 0;
>           indicators = address_space_ldq(&address_space_memory,
>                                          dev->indicators2->addr,
>                                          MEMTXATTRS_UNSPECIFIED,
>                                          NULL);
> -        indicators |= 1ULL << vector;
> +        indicators |= 1ULL;
>           address_space_stq(&address_space_memory, dev->indicators2->addr,
>                             indicators, MEMTXATTRS_UNSPECIFIED, NULL);
>           css_conditional_io_interrupt(sch);
> 

Looks sane.

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Halil Pasic]

On 05/15/2018 04:01 PM, Cornelia Huck wrote:
> On Tue, 15 May 2018 15:17:51 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> 
>> --------------------------------8<------------------------------------------------
>> From: Halil Pasic <pasic@linux.ibm.com>
>> Date: Tue, 15 May 2018 13:57:44 +0200
>> Subject: [PATCH] WIP: cleanup virtio notify
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> ---
>>    hw/s390x/virtio-ccw.c | 10 ++++------
>>    1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index 22df33b509..be433b0336 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>>        SubchDev *sch = ccw_dev->sch;
>>        uint64_t indicators;
>>
>> -    /* queue indicators + secondary indicators */
>> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
>> -        return;
>> -    }
>> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */

I guess you still prefer the verbose comment, or? (I mean
"vector < VIRTIO_QUEUE_MAX: notification for a virtqueue
vector == VIRTIO_QUEUE_MAX: configuration change notification
bits beyond that are unused and should never be notified for")

I can incorporate it for the proper patch.

>> +    assert(vector <= VIRTIO_QUEUE_MAX);

I knew changing return to assert was dangerous, and that I forgot
something. :/

For this to actually work I need:

  
-    /* queue indicators + secondary indicators */
-    if (vector >= VIRTIO_QUEUE_MAX + 64) {
+    if (vector == VIRTIO_NO_VECTOR) {
          return;
      }
+    /* vector == VIRTIO_QUEUE_MAX means configuration change */
+    assert(vector <= VIRTIO_QUEUE_MAX);

Do you prefer keeping the assert, or would you prefer a simple
if (vector > VIRTIO_QUEUE_MAX) {
     return;
}

I think I prefer  handling the VIRTIO_NO_VECTOR separately and keeping
the assert.

>>
>>        if (vector < VIRTIO_QUEUE_MAX) {
>>            if (!dev->indicators) {
>> @@ -1029,6 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>>                    css_adapter_interrupt(CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc);
>>                }
>>            } else {
>> +            assert(vector < NR_CLASSIC_INDICATOR_BITS);

I think this assert is legit though.

>>                indicators = address_space_ldq(&address_space_memory,
>>                                               dev->indicators->addr,
>>                                               MEMTXATTRS_UNSPECIFIED,
>> @@ -1042,12 +1041,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
>>            if (!dev->indicators2) {
>>                return;
>>            }
>> -        vector = 0;
>>            indicators = address_space_ldq(&address_space_memory,
>>                                           dev->indicators2->addr,
>>                                           MEMTXATTRS_UNSPECIFIED,
>>                                           NULL);
>> -        indicators |= 1ULL << vector;
>> +        indicators |= 1ULL;
>>            address_space_stq(&address_space_memory, dev->indicators2->addr,
>>                              indicators, MEMTXATTRS_UNSPECIFIED, NULL);
>>            css_conditional_io_interrupt(sch);
>>
> 
> Looks sane.
> 

Also any tags for the proper patch (e.g. Reported-by: Peter or similar). I
guess I should mention the Coverity CID as 'Fixes:' to, or?

Thanks for having a look!

Regards,
Halil

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Cornelia Huck]

On Tue, 15 May 2018 17:30:23 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/15/2018 04:01 PM, Cornelia Huck wrote:
> > On Tue, 15 May 2018 15:17:51 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> > 
> >   
> >> --------------------------------8<------------------------------------------------
> >> From: Halil Pasic <pasic@linux.ibm.com>
> >> Date: Tue, 15 May 2018 13:57:44 +0200
> >> Subject: [PATCH] WIP: cleanup virtio notify
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >> ---
> >>    hw/s390x/virtio-ccw.c | 10 ++++------
> >>    1 file changed, 4 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >> index 22df33b509..be433b0336 100644
> >> --- a/hw/s390x/virtio-ccw.c
> >> +++ b/hw/s390x/virtio-ccw.c
> >> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> >>        SubchDev *sch = ccw_dev->sch;
> >>        uint64_t indicators;
> >>
> >> -    /* queue indicators + secondary indicators */
> >> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> >> -        return;
> >> -    }
> >> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */  
> 
> I guess you still prefer the verbose comment, or? (I mean
> "vector < VIRTIO_QUEUE_MAX: notification for a virtqueue
> vector == VIRTIO_QUEUE_MAX: configuration change notification
> bits beyond that are unused and should never be notified for")

I think it's a good idea to spell it out, before people are confused
again next year.

> 
> I can incorporate it for the proper patch.
> 
> >> +    assert(vector <= VIRTIO_QUEUE_MAX);  
> 
> I knew changing return to assert was dangerous, and that I forgot
> something. :/
> 
> For this to actually work I need:
> 
>   
> -    /* queue indicators + secondary indicators */
> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> +    if (vector == VIRTIO_NO_VECTOR) {
>           return;
>       }
> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */
> +    assert(vector <= VIRTIO_QUEUE_MAX);
> 
> Do you prefer keeping the assert, or would you prefer a simple
> if (vector > VIRTIO_QUEUE_MAX) {
>      return;
> }
> 
> I think I prefer  handling the VIRTIO_NO_VECTOR separately and keeping
> the assert.

Ah, good old NO_VECTOR :( Yes, let's handle it explicitly.

> 
> >>
> >>        if (vector < VIRTIO_QUEUE_MAX) {
> >>            if (!dev->indicators) {
> >> @@ -1029,6 +1027,7 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> >>                    css_adapter_interrupt(CSS_IO_ADAPTER_VIRTIO, dev->thinint_isc);
> >>                }
> >>            } else {
> >> +            assert(vector < NR_CLASSIC_INDICATOR_BITS);  
> 
> I think this assert is legit though.

Nod.

> 
> >>                indicators = address_space_ldq(&address_space_memory,
> >>                                               dev->indicators->addr,
> >>                                               MEMTXATTRS_UNSPECIFIED,
> >> @@ -1042,12 +1041,11 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> >>            if (!dev->indicators2) {
> >>                return;
> >>            }
> >> -        vector = 0;
> >>            indicators = address_space_ldq(&address_space_memory,
> >>                                           dev->indicators2->addr,
> >>                                           MEMTXATTRS_UNSPECIFIED,
> >>                                           NULL);
> >> -        indicators |= 1ULL << vector;
> >> +        indicators |= 1ULL;
> >>            address_space_stq(&address_space_memory, dev->indicators2->addr,
> >>                              indicators, MEMTXATTRS_UNSPECIFIED, NULL);
> >>            css_conditional_io_interrupt(sch);
> >>  
> > 
> > Looks sane.
> >   
> 
> Also any tags for the proper patch (e.g. Reported-by: Peter or similar). I
> guess I should mention the Coverity CID as 'Fixes:' to, or?

Yes, that makes sense.

Re: [Qemu-devel] [qemu-s390x] virtio-ccw.c vs larger VIRTIO_QUEUE_MAX (coverity warning CID 1390619)

[Cornelia Huck]

On Tue, 15 May 2018 14:00:30 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On 05/15/2018 10:32 AM, Cornelia Huck wrote:
> > On Mon, 14 May 2018 19:12:27 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:

> >> (Other odd code in that function:
> >>      vector = 0;
> >>      [...]
> >>      indicators |= 1ULL << vector;
> >> is that really supposed to ignore the input vector number?)  
> 
> This is why the vector >= VIRTIO_QUEUE_MAX + 64 does not make sense. I
> think this should be simplified. Overwriting the vector with zero and
> doing the shift is just nonsensical.

Yes, that would only make sense if we did a vector -= VIRTIO_QUEUE_MAX
instead.

History time :)

Originally, we defined a single 64 bit area for notifications, matching
virtqueues bit-for-queue, and simply saved the bit/virtqueue number in
the vector value. Then, we realized that we were missing configuration
notifications... solution was to define a second notification area,
also 64 bit for convenience (and in case we had missed yet another
notification mechanism). So, we had the following 'vector':

primary (queue) indicators   secondary indicators
0          ..           63   64       ..      127

with the two parts registered separately, and 65..127 unused. This
persisted through introduction of adapter interrupts (making the first
part 64 bit in a bitfield somewhere) and increasing the number of
virtqueues (making the first part even more bits in a bitfield
somewhere).

The rationale for the 'max virtqueues + 64' check is that the
guest always registers the full 64 bit for the second part (and that we
might want some more bits in there in the future -- which never
happened). In reality, a notification beyond max virtqueues + 1 means
that qemu has lost its way somewhere and is doing weird things.

> 
> To sum it up, my take on the whole is the diff below. I can convert
> it to a proper patch if we agree that's the way to go.

> ----------------------------8<---------------------------------------
> 
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Tue, 15 May 2018 13:57:44 +0200
> Subject: [PATCH] WIP: cleanup virtio notify
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>   hw/s390x/virtio-ccw.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index e51fbefd23..22078605d1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1003,10 +1003,8 @@ static void virtio_ccw_notify(DeviceState *d,
> uint16_t vector) SubchDev *sch = ccw_dev->sch;
>       uint64_t indicators;
> 
> -    /* queue indicators + secondary indicators */
> -    if (vector >= VIRTIO_QUEUE_MAX + 64) {
> -        return;
> -    }
> +    /* vector == VIRTIO_QUEUE_MAX means configuration change */

"vector < VIRTIO_QUEUE_MAX: notification for a virtqueue
vector == VIRTIO_QUEUE_MAX: configuration change notification
bits beyond that are unused and should never be notified for"

?

> +    assert(vector <= VIRTIO_QUEUE_MAX);
> 
>       if (vector < VIRTIO_QUEUE_MAX) {
>           if (!dev->indicators) {
> @@ -1042,12 +1040,11 @@ static void virtio_ccw_notify(DeviceState *d,
> uint16_t vector) if (!dev->indicators2) {
>               return;
>           }
> -        vector = 0;
>           indicators = address_space_ldq(&address_space_memory,
>                                          dev->indicators2->addr,
>                                          MEMTXATTRS_UNSPECIFIED,
>                                          NULL);
> -        indicators |= 1ULL << vector;
> +        indicators |= 1ULL;
>           address_space_stq(&address_space_memory,
> dev->indicators2->addr, indicators, MEMTXATTRS_UNSPECIFIED, NULL);
>           css_conditional_io_interrupt(sch);

I'm ok with that; but as Peter mentioned, you also need to assert on
vector < 64 in the classic indicator case.