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 <address@hidden> wrote:
>
>
> > --------------------------------8<------------------------------------------------
> > From: Halil Pasic <address@hidden>
> > Date: Tue, 15 May 2018 13:57:44 +0200
> > Subject: [PATCH] WIP: cleanup virtio notify
> >
> > Signed-off-by: Halil Pasic <address@hidden>
> > ---
> >    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