On Mon, 14 May 2018 19:12:27 +0100 Peter Maydell <peter.mayd...@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.)