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

[Cornelia Huck <cohuck@redhat.com>]

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.