Re: [PULL v2 2/2] hw/ufs: Add support MCQ of UFSHCI 4.0
On 6/8/2024 12:02 AM, Peter Maydell wrote:
On Mon, 3 Jun 2024 at 09:38, Jeuk Kim wrote:
From: Minwoo Im
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
Reviewed-by: Jeuk Kim
Message-Id: <20240528023106.856777-3-minwoo...@samsung.com>
Signed-off-by: Jeuk Kim
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.
This is probably to make the offset calculation easier (since we can
write `addr + offset` instead of `addr + offset/4`). But I agree with
you that it can be semantically confusing. I'll fix it.
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
Thanks for the detailed explanation! I will prepare a patch to fix the
coverity issue.
Thanks,
Jeuk
Re: [PULL v2 2/2] hw/ufs: Add support MCQ of UFSHCI 4.0
On Mon, 3 Jun 2024 at 09:38, Jeuk Kim wrote:
>
> From: Minwoo Im
>
> 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
> Reviewed-by: Jeuk Kim
> Message-Id: <20240528023106.856777-3-minwoo...@samsung.com>
> Signed-off-by: Jeuk Kim
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
[PULL v2 2/2] hw/ufs: Add support MCQ of UFSHCI 4.0
From: Minwoo Im 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 Reviewed-by: Jeuk Kim Message-Id: <20240528023106.856777-3-minwoo...@samsung.com> Signed-off-by: Jeuk Kim --- hw/ufs/trace-events | 17 ++ hw/ufs/ufs.c| 475 ++-- hw/ufs/ufs.h| 98 - include/block/ufs.h | 23 ++- 4 files changed, 593 insertions(+), 20 deletions(-) diff --git a/hw/ufs/trace-events b/hw/ufs/trace-events index 665e1a942b..531dcfc686 100644 --- a/hw/ufs/trace-events +++ b/hw/ufs/trace-events @@ -11,13 +11,18 @@ ufs_exec_nop_cmd(uint32_t slot) "UTRLDBR slot %"PRIu32"" ufs_exec_scsi_cmd(uint32_t slot, uint8_t lun, uint8_t opcode) "slot %"PRIu32", lun 0x%"PRIx8", opcode 0x%"PRIx8"" ufs_exec_query_cmd(uint32_t slot, uint8_t opcode) "slot %"PRIu32", opcode 0x%"PRIx8"" ufs_process_uiccmd(uint32_t uiccmd, uint32_t ucmdarg1, uint32_t ucmdarg2, uint32_t ucmdarg3) "uiccmd 0x%"PRIx32", ucmdarg1 0x%"PRIx32", ucmdarg2 0x%"PRIx32", ucmdarg3 0x%"PRIx32"" +ufs_mcq_complete_req(uint8_t qid) "sqid %"PRIu8"" +ufs_mcq_create_sq(uint8_t sqid, uint8_t cqid, uint64_t addr, uint16_t size) "mcq create sq sqid %"PRIu8", cqid %"PRIu8", addr 0x%"PRIx64", size %"PRIu16"" +ufs_mcq_create_cq(uint8_t cqid, uint64_t addr, uint16_t size) "mcq create cq cqid %"PRIu8", addr 0x%"PRIx64", size %"PRIu16"" # error condition ufs_err_dma_read_utrd(uint32_t slot, uint64_t addr) "failed to read utrd. UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64"" ufs_err_dma_read_req_upiu(uint32_t slot, uint64_t addr) "failed to read req upiu. UTRLDBR slot %"PRIu32", request upiu addr %"PRIu64"" ufs_err_dma_read_prdt(uint32_t slot, uint64_t addr) "failed to read prdt. UTRLDBR slot %"PRIu32", prdt addr %"PRIu64"" +ufs_err_dma_read_sq(uint8_t sqid, uint64_t addr) "failed to read sq entry. sqid %"PRIu8", hwaddr %"PRIu64"" ufs_err_dma_write_utrd(uint32_t slot, uint64_t addr) "failed to write utrd. UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64"" ufs_err_dma_write_rsp_upiu(uint32_t slot, uint64_t addr) "failed to write rsp upiu. UTRLDBR slot %"PRIu32", response upiu addr %"PRIu64"" +ufs_err_dma_write_cq(uint8_t cqid, uint64_t addr) "failed to write cq entry. cqid %"PRIu8", hwaddr %"PRIu64"" ufs_err_utrl_slot_error(uint32_t slot) "UTRLDBR slot %"PRIu32" is in error" ufs_err_utrl_slot_busy(uint32_t slot) "UTRLDBR slot %"PRIu32" is busy" ufs_err_unsupport_register_offset(uint32_t offset) "Register offset 0x%"PRIx32" is not yet supported" @@ -31,3 +36,15 @@ ufs_err_query_invalid_opcode(uint8_t opcode) "query request has invalid opcode. ufs_err_query_invalid_idn(uint8_t opcode, uint8_t idn) "query request has invalid idn. opcode: 0x%"PRIx8", idn 0x%"PRIx8"" ufs_err_query_invalid_index(uint8_t opcode, uint8_t index) "query request has invalid index. opcode: 0x%"PRIx8", index 0x%"PRIx8"" ufs_err_invalid_trans_code(uint32_t slot, uint8_t trans_code) "request upiu has invalid transaction code. slot: %"PRIu32", trans_code: 0x%"PRIx8"" +ufs_err_mcq_db_wr_invalid_sqid(uint8_t qid) "invalid mcq sqid %"PRIu8"" +ufs_err_mcq_db_wr_invalid_db(uint8_t qid, uint32_t db) "invalid mcq doorbell sqid %"PRIu8", db %"PRIu32"" +ufs_err_mcq_create_sq_invalid_sqid(uint8_t qid) "invalid mcq sqid %"PRIu8"" +ufs_err_mcq_create_sq_invalid_cqid(uint8_t qid) "invalid mcq cqid %"PRIu8"" +ufs_err_mcq_create_sq_already_exists(uint8_t qid) "mcq sqid %"PRIu8 "already exists" +ufs_err_mcq_delete_sq_invalid_sqid(uint8_t qid) "invalid mcq sqid %"PRIu8"" +ufs_err_mcq_delete_sq_not_exists(uint8_t qid) "mcq sqid %"PRIu8 "not exists" +ufs_err_mcq_create_cq_invalid_cqid(uint8_t qid) "invalid mcq cqid %"PRIu8"" +ufs_err_mcq_create_cq_already_exists(uint8_t qid) "mcq cqid %"PRIu8 "already exists" +ufs_err_mcq_delete_cq_invalid_cqid(uint8_t qid) "invalid mcq cqid %"PRIu8"" +ufs_err_mcq_delete_cq_not_exists(uint8_t qid) "mcq cqid %"PRIu8 "not exists" +ufs_err_mcq_delete_cq_sq_not_deleted(uint8_t sqid, uint8_t cqid) "mcq sq %"PRIu8" still has cq %"PRIu8"" diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c index bac78a32bb..71a88d221c 100644 --- a/hw/ufs/ufs.c +++ b/hw/ufs/ufs.c @@ -9,7 +9,7 @@ */ /** - * Reference Specs: https://www.jedec.org/, 3.1 + * Reference Specs: https://www.jedec.org/, 4.0 * * Usage * - @@ -28,10 +28,45 @@ #include "trace.h" #include "ufs.h" -/* The QEMU-UFS device follows spec version 3.1 */ -#define UFS_SPEC_VER 0x0310 +/* The QEMU-UFS device follows spec version 4.0 */ +#define UFS_SPEC_VER 0x0400 #define
