Re: [PATCH 1/4] hw/ufs: minor bug fixes related to ufs-test

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

On 8/21/2024 11:32 AM, 정유찬 wrote:
>  From d0ae8e25aec4ae7d222a2ea667d5ddb61f14fe02 Mon Sep 17 00:00:00 2001
> From: Yoochan Jeong <yc01.jeong@samsung.com>
> Date: Wed, 21 Aug 2024 09:03:06 +0900
> Subject: [PATCH 1/4] hw/ufs: minor bug fixes related to ufs-test
>
> Minor bugs and errors related to ufs-test are resolved. Some
> permissions and code implementations that are not synchronized
> with the ufs spec are edited.
>
> Based on: 20240802051902epcms2p319bc095a15eaef8de4e6955f6718371d@epcms2p3
> Signed-off-by: Yoochan Jeong <yc01.jeong@samsung.com>
> ---
>   hw/ufs/ufs.c           | 26 +++++++++++++++++++++-----
>   tests/qtest/ufs-test.c | 12 +++++++++---
>   2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
> index ce2c96aeea..9472a3c14a 100644
> --- a/hw/ufs/ufs.c
> +++ b/hw/ufs/ufs.c
> @@ -971,7 +971,7 @@ static const int attr_permission[UFS_QUERY_ATTR_IDN_COUNT] = {
>           UFS_QUERY_ATTR_READ | UFS_QUERY_ATTR_WRITE,
>       [UFS_QUERY_ATTR_IDN_EE_STATUS] = UFS_QUERY_ATTR_READ,
>       [UFS_QUERY_ATTR_IDN_SECONDS_PASSED] = UFS_QUERY_ATTR_WRITE,
> -    [UFS_QUERY_ATTR_IDN_CNTX_CONF] = UFS_QUERY_ATTR_READ,
> +    [UFS_QUERY_ATTR_IDN_CNTX_CONF] = UFS_QUERY_ATTR_READ | UFS_QUERY_ATTR_WRITE,

Although the spec defines UFS_QUERY_ATTR_IDN_CNTX_CONF as configurable,
the qemu ufs does not yet implement related functionality.
So it seems reasonable to leave it as not configurable to me.


>       [UFS_QUERY_ATTR_IDN_FFU_STATUS] = UFS_QUERY_ATTR_READ,
>       [UFS_QUERY_ATTR_IDN_PSA_STATE] = UFS_QUERY_ATTR_READ | UFS_QUERY_ATTR_WRITE,
>       [UFS_QUERY_ATTR_IDN_PSA_DATA_SIZE] =
> @@ -1038,7 +1038,7 @@ static QueryRespCode ufs_exec_query_flag(UfsRequest *req, int op)
>       }
>   
>       *(((uint8_t *)&u->flags) + idn) = value;
> -    req->rsp_upiu.qr.value = cpu_to_be32(value);
> +    req->rsp_upiu.qr.value = value;

req->rsp_upiu.qr.value uses big endian. Please check the spec


>       return UFS_QUERY_RESULT_SUCCESS;
>   }
>   
> @@ -1148,8 +1148,11 @@ static QueryRespCode ufs_exec_query_attr(UfsRequest *req, int op)
>   {
>       UfsHc *u = req->hc;
>       uint8_t idn = req->req_upiu.qr.idn;
> +    uint8_t selector = req->req_upiu.qr.selector;
>       uint32_t value;
>       QueryRespCode ret;
> +    const uint8_t UFS_QUERY_ATTR_CNTX_CONF_SELECTOR = 15;
> +    const uint32_t UFS_QUERY_ATTR_MAXVALUE = 0x0F;
The name UFS_QUERY_ATTR_MAXVALUE does not seem appropriate. Rename it to 
mean the maximum value of the ICC.
>   
>       ret = ufs_attr_check_idn_valid(idn, op);
>       if (ret) {
> @@ -1159,10 +1162,20 @@ static QueryRespCode ufs_exec_query_attr(UfsRequest *req, int op)
>       if (op == UFS_QUERY_ATTR_READ) {
>           value = ufs_read_attr_value(u, idn);
>       } else {
> -        value = be32_to_cpu(req->req_upiu.qr.value);
> +        value = req->req_upiu.qr.value;
> +        if (idn == UFS_QUERY_ATTR_IDN_ACTIVE_ICC_LVL &&
> +            value > UFS_QUERY_ATTR_MAXVALUE) {
Move this condition check inside the ufs_write_attr_value() function
> +            return UFS_QUERY_RESULT_INVALID_VALUE;
> +        }
> +        if (idn == UFS_QUERY_ATTR_IDN_CNTX_CONF) {
Remove it. We haven't implemented the UFS_QUERY_ATTR_IDN_CNTX_CONF 
function yet.
> +            if (selector != UFS_QUERY_ATTR_CNTX_CONF_SELECTOR) {
> +                return UFS_QUERY_RESULT_INVALID_SELECTOR;
> +            } else if (value == 0x00 || value > UFS_QUERY_ATTR_MAXVALUE) {
> +                return UFS_QUERY_RESULT_INVALID_VALUE;
> +            }
> +        }
>           ufs_write_attr_value(u, idn, value);
>       }
> -
>       req->rsp_upiu.qr.value = cpu_to_be32(value);
>       return UFS_QUERY_RESULT_SUCCESS;
>   }
> @@ -1287,9 +1300,12 @@ static QueryRespCode ufs_read_desc(UfsRequest *req)
>       UfsHc *u = req->hc;
>       QueryRespCode status;
>       uint8_t idn = req->req_upiu.qr.idn;
> +    uint8_t selector = req->req_upiu.qr.selector;
>       uint16_t length = be16_to_cpu(req->req_upiu.qr.length);
>       InterconnectDescriptor desc;
> -
> +    if (selector != 0) {
> +        return UFS_QUERY_RESULT_INVALID_SELECTOR;
> +    }
>       switch (idn) {
>       case UFS_QUERY_DESC_IDN_DEVICE:
>           memcpy(&req->rsp_upiu.qr.data, &u->device_desc, sizeof(u->device_desc));
> diff --git a/tests/qtest/ufs-test.c b/tests/qtest/ufs-test.c
> index 82ec3f0671..d70c2ee4a3 100644
> --- a/tests/qtest/ufs-test.c
> +++ b/tests/qtest/ufs-test.c
> @@ -119,8 +119,10 @@ static void ufs_send_nop_out(QUfs *ufs, uint8_t slot,
>   
>   static void ufs_send_query(QUfs *ufs, uint8_t slot, uint8_t query_function,
>                              uint8_t query_opcode, uint8_t idn, uint8_t index,
> +                           uint8_t selector, uint32_t attr_value,

We use ufs_send_query() not only for attributes, but also descriptors 
and flags.

Please rename `attr_value` to `value`.


>                              UtpTransferReqDesc *utrd_out, UtpUpiuRsp *rsp_out)
>   {
> +    const uint16_t UFS_QUERY_DESC_MAXLENGTH = 0x62;
How did you determine that the maximum size of a descriptor is 62?
>       /* Build up utp transfer request descriptor */
>       UtpTransferReqDesc utrd = ufs_build_req_utrd(ufs->cmd_desc_addr, slot,
>                                                    UFS_UTP_NO_DATA_TRANSFER, 0);
> @@ -136,13 +138,16 @@ static void ufs_send_query(QUfs *ufs, uint8_t slot, uint8_t query_function,
>       req_upiu.header.query_func = query_function;
>       req_upiu.header.task_tag = slot;
>       /*
> -     * QEMU UFS does not currently support Write descriptor and Write attribute,
> +     * QEMU UFS does not currently support Write descriptor,

We might need to check condition here that `query_opcode != 
UFS_UPIU_QUERY_OPCODE_WRITE_DESC`, since

it is not implemented yet.

>        * so the value of data_segment_length is always 0.
>        */
>       req_upiu.header.data_segment_length = 0;
>       req_upiu.qr.opcode = query_opcode;
>       req_upiu.qr.idn = idn;
>       req_upiu.qr.index = index;
> +    req_upiu.qr.selector = selector;
> +    req_upiu.qr.value = attr_value;
> +    req_upiu.qr.length = UFS_QUERY_DESC_MAXLENGTH;
>       qtest_memwrite(ufs->dev.bus->qts, req_upiu_addr, &req_upiu,
>                      sizeof(req_upiu));
>   
> @@ -344,7 +349,7 @@ static void ufs_init(QUfs *ufs, QGuestAllocator *alloc)
>       /* Set fDeviceInit flag via query request */
>       ufs_send_query(ufs, 0, UFS_UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST,
>                      UFS_UPIU_QUERY_OPCODE_SET_FLAG,
> -                   UFS_QUERY_FLAG_IDN_FDEVICEINIT, 0, &utrd, &rsp_upiu);
> +                   UFS_QUERY_FLAG_IDN_FDEVICEINIT, 0, 0, 0, &utrd, &rsp_upiu);
>       g_assert_cmpuint(le32_to_cpu(utrd.header.dword_2), ==, UFS_OCS_SUCCESS);
>   
>       /* Wait for device to reset */
> @@ -353,7 +358,8 @@ static void ufs_init(QUfs *ufs, QGuestAllocator *alloc)
>           qtest_clock_step(ufs->dev.bus->qts, 100);
>           ufs_send_query(ufs, 0, UFS_UPIU_QUERY_FUNC_STANDARD_READ_REQUEST,
>                          UFS_UPIU_QUERY_OPCODE_READ_FLAG,
> -                       UFS_QUERY_FLAG_IDN_FDEVICEINIT, 0, &utrd, &rsp_upiu);
> +                       UFS_QUERY_FLAG_IDN_FDEVICEINIT, 0, 0, 0, &utrd,
> +                       &rsp_upiu);
>       } while (be32_to_cpu(rsp_upiu.qr.value) != 0 &&
>                g_get_monotonic_time() < end_time);
>       g_assert_cmpuint(be32_to_cpu(rsp_upiu.qr.value), ==, 0);