On Thu, 7 Sept 2023 at 19:17, Stefan Hajnoczi <stefa...@redhat.com> wrote: > > From: Jeuk Kim <jeuk20....@samsung.com> > > This commit makes the UFS device support query > and nop out transfer requests. > > The next patch would be support for UFS logical > unit and scsi command transfer request. > > Signed-off-by: Jeuk Kim <jeuk20....@samsung.com> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> > Message-id: > ff7a5f0fd26761936a553ffb89d3df0ba62844e9.1693980783.git.jeuk20....@gmail.com > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > hw/ufs/ufs.h | 46 +++ > hw/ufs/ufs.c | 988 +++++++++++++++++++++++++++++++++++++++++++- > hw/ufs/trace-events | 1 + > 3 files changed, 1033 insertions(+), 2 deletions(-)
Hi; Coverity isn't happy about the code in this function (CID 1519050). The code isn't strictly wrong, but it's probably possible to make it a bit more clearly correct. > +static void ufs_process_db(UfsHc *u, uint32_t val) > +{ > + unsigned long doorbell; > + uint32_t slot; > + uint32_t nutrs = u->params.nutrs; > + UfsRequest *req; > + > + val &= ~u->reg.utrldbr; > + if (!val) { > + return; > + } > + > + doorbell = val; > + slot = find_first_bit(&doorbell, nutrs); Here we pass the address of a single 'unsigned long' to find_first_bit(). That function operates on arrays, so unless nutrs is guaranteed to be less than 32 this might walk off the end of memory. There is a check on params.nutrs in ufs_check_constraints(), which checks for "> UFS_MAX_NUTRS" and that value is 32, so this won't actually overflow, but Coverity can't see that check and in any case what it really doesn't like here is the passing of the address of a 'long' variable to a function that is prototyped as taking an array of longs. You can probably make Coverity happy by defining doorbell here as a 1 element array, and asserting that nutrs is 32 or less. Alternatively, we have ctz32() for working through bits in a uint32_t, though that is a bit lower-level than find_first_bit/find_next_bit. > + > + while (slot < nutrs) { > + req = &u->req_list[slot]; > + if (req->state == UFS_REQUEST_ERROR) { > + trace_ufs_err_utrl_slot_error(req->slot); > + return; > + } > + > + if (req->state != UFS_REQUEST_IDLE) { > + trace_ufs_err_utrl_slot_busy(req->slot); > + return; > + } > + > + trace_ufs_process_db(slot); > + req->state = UFS_REQUEST_READY; > + slot = find_next_bit(&doorbell, nutrs, slot + 1); > + } > + > + qemu_bh_schedule(u->doorbell_bh); > +} thanks -- PMM