Re: [PULL v2 2/2] hw/ufs: Add support MCQ of UFSHCI 4.0

[Peter Maydell]

On Mon, 3 Jun 2024 at 09:38, Jeuk Kim <jeuk20.kim@gmail.com> wrote:
>
> From: Minwoo Im <minwoo.im@samsung.com>
>
> This patch adds support for MCQ defined in UFSHCI 4.0.  This patch
> utilized the legacy I/O codes as much as possible to support MCQ.
>
> MCQ operation & runtime register is placed at 0x1000 offset of UFSHCI
> register statically with no spare space among four registers (48B):
>
>         UfsMcqSqReg, UfsMcqSqIntReg, UfsMcqCqReg, UfsMcqCqIntReg
>
> The maxinum number of queue is 32 as per spec, and the default
> MAC(Multiple Active Commands) are 32 in the device.
>
> Example:
>         -device ufs,serial=foo,id=ufs0,mcq=true,mcq-maxq=8
>
> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
> Reviewed-by: Jeuk Kim <jeuk20.kim@samsung.com>
> Message-Id: <20240528023106.856777-3-minwoo.im@samsung.com>
> Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com>

Hi; Coverity reported a potential issue with this code.
I don't think it's an actual bug, but it would be nice to
clean it up and keep Coverity happy. (CID 1546866).

>  static uint64_t ufs_mmio_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      UfsHc *u = (UfsHc *)opaque;
> -    uint8_t *ptr = (uint8_t *)&u->reg;
> +    uint8_t *ptr;
>      uint64_t value;
> -
> -    if (addr > sizeof(u->reg) - size) {

Before this change, we checked addr against (sizeof(u->reg) - size).

> +    uint64_t offset;
> +
> +    if (addr < sizeof(u->reg)) {

Now we changed to check it against sizeof(u->reg).
That means Coverity thinks it's possible that we could
have addr = sizeof(u->reg) - 1...

> +        offset = addr;
> +        ptr = (uint8_t *)&u->reg;
> +    } else if (ufs_is_mcq_reg(u, addr)) {
> +        offset = addr - ufs_mcq_reg_addr(u, 0);
> +        ptr = (uint8_t *)&u->mcq_reg;
> +    } else if (ufs_is_mcq_op_reg(u, addr)) {
> +        offset = addr - ufs_mcq_op_reg_addr(u, 0);
> +        ptr = (uint8_t *)&u->mcq_op_reg;
> +    } else {
>          trace_ufs_err_invalid_register_offset(addr);
>          return 0;
>      }
>
> -    value = *(uint32_t *)(ptr + addr);
> +    value = *(uint32_t *)(ptr + offset);

...so Coverity thinks that this write of a 32-bit value
might overrun the end of the array.

>      trace_ufs_mmio_read(addr, value, size);
>      return value;

Side note: why use uint8_t* for the type of "ptr" in these
functions? We know it must be a uint32_t* (it comes either from
the u->reg or from one of these u_mcq_reg or u->mcq_op_reg
fields, and they must all be uint32_t), and using the right
type would reduce the number of casts you need to do.

It also helps the reader a little, because using a uint8_t
implies that you're indexing into an array-of-bytes, and
if you were doing that it would be a bug (because both of
it not handling endianness correctly and because of it
not handling alignment correctly).

>  }
> @@ -423,13 +802,17 @@ static void ufs_mmio_write(void *opaque, hwaddr addr, 
> uint64_t data,
>  {
>      UfsHc *u = (UfsHc *)opaque;
>
> -    if (addr > sizeof(u->reg) - size) {
> +    trace_ufs_mmio_write(addr, data, size);
> +
> +    if (addr < sizeof(u->reg)) {

Similarly here we changed the bounds check we were doing.

> +        ufs_write_reg(u, addr, data, size);
> +    } else if (ufs_is_mcq_reg(u, addr)) {
> +        ufs_write_mcq_reg(u, addr - ufs_mcq_reg_addr(u, 0), data, size);
> +    } else if (ufs_is_mcq_op_reg(u, addr)) {
> +        ufs_write_mcq_op_reg(u, addr - ufs_mcq_op_reg_addr(u, 0), data, 
> size);
> +    } else {
>          trace_ufs_err_invalid_register_offset(addr);
> -        return;
>      }
> -
> -    trace_ufs_mmio_write(addr, data, size);
> -    ufs_write_reg(u, addr, data, size);

thanks
-- PMM