Re: [PULL 3/5] hw/ufs: Support for Query Transfer Requests

[Jeuk Kim <jeuk20.kim@gmail.com>]

On 23. 9. 14. 23:40, Peter Maydell wrote:
> On Thu, 7 Sept 2023 at 19:17, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> From: Jeuk Kim <jeuk20.kim@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.kim@samsung.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Message-id: ff7a5f0fd26761936a553ffb89d3df0ba62844e9.1693980783.git.jeuk20.kim@gmail.com
>> Signed-off-by: Stefan Hajnoczi <stefanha@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
>

Thank you for letting me know about the coverity issue with a detailed 
description!

I have checked all the coverity issues related to ufs.
(cid 1519042, cid 1519043, cid 1519050, cid 1519051)

I will fix them with an additional patch as soon as possible.

Thank you!

Jeuk