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,
     [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;
     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;
 
     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) {
+            return UFS_QUERY_RESULT_INVALID_VALUE;
+        }
+        if (idn == UFS_QUERY_ATTR_IDN_CNTX_CONF) {
+            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,
                            UtpTransferReqDesc *utrd_out, UtpUpiuRsp *rsp_out)
 {
+    const uint16_t UFS_QUERY_DESC_MAXLENGTH = 0x62;
     /* 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,
      * 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);
-- 
2.34.1

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);

On 8/22/2024, Jeuk Kim wrote:
> 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.
>

Okay. Some testcases in patch 3/4 also involve this attribute,
so I will also edit them in the next version.

> 
>>      [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

It seems that the original code is correct, sorry for the confusion.
I will edit some testcases in test functions to check be32_to_cpu values.

> 
> 
>>      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.

I named it this way because CNTX_CONF attribute also had the same range.
But since that attribute is not writable now, it seems that changing its
name including ICC would be better.
Also, I will move this constant to ufs.h file.

>> 
>>      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

I will change ufs_write_attr_value to return QueryRespCode and
edit this function slightly.

>> +            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.

I will edit it that way.

>> +            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`.
> 
>
 
I think this parameter name is okay, because this "value" in UPIU is
only used when writing attributes. Writing flags do not require an
actual value, and descriptor data will be stored in data segmentation
area.
 
>>                              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?

It would be better to use UFS_QUERY_DESC_MAX_SIZE in ufs.h.
I will edit it that way.

>>      /* 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.
> 

in ufs_exec_query_write function in ufs.c, it already checks
if it is trying to write a descriptor. Is there any particular
reason that we should check it here in advance?

>>        * 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);

On 8/22/2024 3:00 PM, Yoochan Jeong wrote:
> On 8/22/2024, Jeuk Kim wrote:
>> On 8/21/2024 11:32 AM, 정유찬 wrote:
>>>    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`.
>>
>>
>   
> I think this parameter name is okay, because this "value" in UPIU is
> only used when writing attributes. Writing flags do not require an
> actual value, and descriptor data will be stored in data segmentation
> area.

Okay. That's reasonable.


>   
>>>                            
>> We might need to check condition here that `query_opcode !=
>> UFS_UPIU_QUERY_OPCODE_WRITE_DESC`, since
>>
>> it is not implemented yet.
>>
> in ufs_exec_query_write function in ufs.c, it already checks
> if it is trying to write a descriptor. Is there any particular
> reason that we should check it here in advance?

You're right. We don't need to check it here.