+-- On Sat, 22 Oct 2016, Peter Maydell wrote --+ | Secondly, it's almost the same as this cleanup | patch from Thomas Huth that's already in qemu-trivial: | http://patchwork.ozlabs.org/patch/681349/ | | except that your version is removing the ! | negations from the return value. | | Can you explain in a bit more detail what the bug | is here and why this is the right fix for it?
nvme_check_sqid(NvmeCtrl *n, uint16_t sqid) { return sqid < n->num_queues && n->sq[sqid] != NULL ? 0 : -1; } Both 'nvme_check_sqid' and 'nvme_check_cqid' return zero(0) when the respective queue id's are valid and -1 when they are invalid. The '!' was causing it to return invalid QID error when the check function returned zero. The code seems incosistent quite a bit. The current check returns invalid QID error for '!cqid' and '!sqid', but these IDs are used as index into 'n->cq' array, not sure why index zero(0) is invalid. But then I found this in the NVME specification[0] "...One entry in each queue is not available for use due to Head and Tail entry pointer definition." Not sure if this one entry is at index zero(0). If so, there are other calls to the 'nvme_check' functions which are not acompanyed with '!cqid' or '!sqid' checks. Ex. nvme_process_db qid = (addr - 0x1000) >> 3; if (nvme_check_sqid(n, qid)) { return; } nvme_del_sq if (!nvme_check_cqid(n, sq->cqid)) { cq = n->cq[sq->cqid]; I haven't check these in further details yet. When 'cqid' is invalid it returns 'NVME_INVALID_CQID' but when 'sqid' is invalid it returns 'NVME_INVALID_QID'. Not sure why not use 'NVME_INVALID_QID' for both. [0] http://www.nvmexpress.org/wp-content/uploads/NVM-Express-1_2-Gold-20141209.pdf Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F