Re: [PULL 3/5] hw/ufs: Support for Query Transfer Requests
On 23. 9. 14. 23:40, Peter Maydell wrote:
On Thu, 7 Sept 2023 at 19:17, Stefan Hajnoczi wrote:
From: Jeuk Kim
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
Reviewed-by: Stefan Hajnoczi
Message-id:
ff7a5f0fd26761936a553ffb89d3df0ba62844e9.1693980783.git.jeuk20@gmail.com
Signed-off-by: Stefan Hajnoczi
---
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
Re: [PULL 3/5] hw/ufs: Support for Query Transfer Requests
On Thu, 7 Sept 2023 at 19:17, Stefan Hajnoczi wrote:
>
> From: Jeuk Kim
>
> 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
> Reviewed-by: Stefan Hajnoczi
> Message-id:
> ff7a5f0fd26761936a553ffb89d3df0ba62844e9.1693980783.git.jeuk20@gmail.com
> Signed-off-by: Stefan Hajnoczi
> ---
> 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
[PULL 3/5] hw/ufs: Support for Query Transfer Requests
From: Jeuk Kim
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
Reviewed-by: Stefan Hajnoczi
Message-id:
ff7a5f0fd26761936a553ffb89d3df0ba62844e9.1693980783.git.jeuk20@gmail.com
Signed-off-by: Stefan Hajnoczi
---
hw/ufs/ufs.h| 46 +++
hw/ufs/ufs.c| 988 +++-
hw/ufs/trace-events | 1 +
3 files changed, 1033 insertions(+), 2 deletions(-)
diff --git a/hw/ufs/ufs.h b/hw/ufs/ufs.h
index d9d195caec..3d1b2cff4e 100644
--- a/hw/ufs/ufs.h
+++ b/hw/ufs/ufs.h
@@ -18,6 +18,32 @@
#define UFS_MAX_LUS 32
#define UFS_BLOCK_SIZE 4096
+typedef enum UfsRequestState {
+UFS_REQUEST_IDLE = 0,
+UFS_REQUEST_READY = 1,
+UFS_REQUEST_RUNNING = 2,
+UFS_REQUEST_COMPLETE = 3,
+UFS_REQUEST_ERROR = 4,
+} UfsRequestState;
+
+typedef enum UfsReqResult {
+UFS_REQUEST_SUCCESS = 0,
+UFS_REQUEST_FAIL = 1,
+} UfsReqResult;
+
+typedef struct UfsRequest {
+struct UfsHc *hc;
+UfsRequestState state;
+int slot;
+
+UtpTransferReqDesc utrd;
+UtpUpiuReq req_upiu;
+UtpUpiuRsp rsp_upiu;
+
+/* for scsi command */
+QEMUSGList *sg;
+} UfsRequest;
+
typedef struct UfsParams {
char *serial;
uint8_t nutrs; /* Number of UTP Transfer Request Slots */
@@ -30,6 +56,12 @@ typedef struct UfsHc {
UfsReg reg;
UfsParams params;
uint32_t reg_size;
+UfsRequest *req_list;
+
+DeviceDescriptor device_desc;
+GeometryDescriptor geometry_desc;
+Attributes attributes;
+Flags flags;
qemu_irq irq;
QEMUBH *doorbell_bh;
@@ -39,4 +71,18 @@ typedef struct UfsHc {
#define TYPE_UFS "ufs"
#define UFS(obj) OBJECT_CHECK(UfsHc, (obj), TYPE_UFS)
+typedef enum UfsQueryFlagPerm {
+UFS_QUERY_FLAG_NONE = 0x0,
+UFS_QUERY_FLAG_READ = 0x1,
+UFS_QUERY_FLAG_SET = 0x2,
+UFS_QUERY_FLAG_CLEAR = 0x4,
+UFS_QUERY_FLAG_TOGGLE = 0x8,
+} UfsQueryFlagPerm;
+
+typedef enum UfsQueryAttrPerm {
+UFS_QUERY_ATTR_NONE = 0x0,
+UFS_QUERY_ATTR_READ = 0x1,
+UFS_QUERY_ATTR_WRITE = 0x2,
+} UfsQueryAttrPerm;
+
#endif /* HW_UFS_UFS_H */
diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
index df87f2a6d5..56a8ec286b 100644
--- a/hw/ufs/ufs.c
+++ b/hw/ufs/ufs.c
@@ -15,10 +15,221 @@
#include "ufs.h"
/* The QEMU-UFS device follows spec version 3.1 */
-#define UFS_SPEC_VER 0x0310
+#define UFS_SPEC_VER 0x0310
#define UFS_MAX_NUTRS 32
#define UFS_MAX_NUTMRS 8
+static MemTxResult ufs_addr_read(UfsHc *u, hwaddr addr, void *buf, int size)
+{
+hwaddr hi = addr + size - 1;
+
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_read(PCI_DEVICE(u), addr, buf, size);
+}
+
+static MemTxResult ufs_addr_write(UfsHc *u, hwaddr addr, const void *buf,
+ int size)
+{
+hwaddr hi = addr + size - 1;
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_write(PCI_DEVICE(u), addr, buf, size);
+}
+
+static void ufs_complete_req(UfsRequest *req, UfsReqResult req_result);
+
+static inline hwaddr ufs_get_utrd_addr(UfsHc *u, uint32_t slot)
+{
+hwaddr utrl_base_addr = (((hwaddr)u->reg.utrlbau) << 32) + u->reg.utrlba;
+hwaddr utrd_addr = utrl_base_addr + slot * sizeof(UtpTransferReqDesc);
+
+return utrd_addr;
+}
+
+static inline hwaddr ufs_get_req_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+uint32_t cmd_desc_base_addr_lo =
+le32_to_cpu(utrd->command_desc_base_addr_lo);
+uint32_t cmd_desc_base_addr_hi =
+le32_to_cpu(utrd->command_desc_base_addr_hi);
+
+return (((hwaddr)cmd_desc_base_addr_hi) << 32) + cmd_desc_base_addr_lo;
+}
+
+static inline hwaddr ufs_get_rsp_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(utrd);
+uint32_t rsp_upiu_byte_off =
+le16_to_cpu(utrd->response_upiu_offset) * sizeof(uint32_t);
+return req_upiu_base_addr + rsp_upiu_byte_off;
+}
+
+static MemTxResult ufs_dma_read_utrd(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr utrd_addr = ufs_get_utrd_addr(u, req->slot);
+MemTxResult ret;
+
+ret = ufs_addr_read(u, utrd_addr, &req->utrd, sizeof(req->utrd));
+if (ret) {
+trace_ufs_err_dma_read_utrd(req->slot, utrd_addr);
+}
+return ret;
+}
+
+static MemTxResult ufs_dma_read_req_upiu(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(&req->utrd);
+UtpUpiuReq *req_upiu = &req->req_upiu;
+uint32_t copy_size;
+uint16_t data_segment_length;
+MemTxResult ret;
+
+/*
+ * To know the size of the req_upiu, we need to read the
+ * data_
