Re: [PATCH] hw/ufs: Modify lu.c to share codes with SCSI subsystem

2023-10-30 Thread Jeuk Kim



On 10/30/2023 1:11 PM, Philippe Mathieu-Daudé wrote:

Hi Jeuk,

On 20/10/23 03:51, Jeuk Kim wrote:

This patch removes the code that ufs-lu was duplicating from
scsi-hd and allows them to share code.

It makes ufs-lu have a virtual scsi-bus and scsi-hd internally.
This allows scsi related commands to be passed thorugh to the scsi-hd.
The query request and nop command work the same as the existing logic.

Well-known lus do not have a virtual scsi-bus and scsi-hd, and
handle the necessary scsi commands by emulating them directly.

Signed-off-by: Jeuk Kim 
---
  hw/ufs/lu.c    | 1473 +++-


I liked this patch intent, but almost 1500 lines changed in a single
patch make it impossible to review. Ideally each patch shouldn't modify
more than 100 lines, otherwise reviewers are either exhausted or can't
be careful enough and miss possible bugs or design issues.

Regards,

Phil.


Hi Phil,

Thanks for the comment.
I thought that since most of the code fixes were code removals and
were functionally bundled together, it was right to submit them as a 
single patch.


However, I agree that it was inconsiderate of the reviewers, and I 
apologize for that.


Next time, I'll size the patch appropriately as you suggest.

Thanks,
Jeuk



Re: [PATCH] hw/ufs: Modify lu.c to share codes with SCSI subsystem

2023-10-29 Thread Philippe Mathieu-Daudé

Hi Jeuk,

On 20/10/23 03:51, Jeuk Kim wrote:

This patch removes the code that ufs-lu was duplicating from
scsi-hd and allows them to share code.

It makes ufs-lu have a virtual scsi-bus and scsi-hd internally.
This allows scsi related commands to be passed thorugh to the scsi-hd.
The query request and nop command work the same as the existing logic.

Well-known lus do not have a virtual scsi-bus and scsi-hd, and
handle the necessary scsi commands by emulating them directly.

Signed-off-by: Jeuk Kim 
---
  hw/ufs/lu.c| 1473 +++-


I liked this patch intent, but almost 1500 lines changed in a single
patch make it impossible to review. Ideally each patch shouldn't modify
more than 100 lines, otherwise reviewers are either exhausted or can't
be careful enough and miss possible bugs or design issues.

Regards,

Phil.


  hw/ufs/trace-events|   25 -
  hw/ufs/ufs.c   |  202 +-
  hw/ufs/ufs.h   |   36 +-
  include/block/ufs.h|2 +-
  tests/qtest/ufs-test.c |   37 +-
  6 files changed, 315 insertions(+), 1460 deletions(-)





[PATCH] hw/ufs: Modify lu.c to share codes with SCSI subsystem

2023-10-19 Thread Jeuk Kim
This patch removes the code that ufs-lu was duplicating from
scsi-hd and allows them to share code.

It makes ufs-lu have a virtual scsi-bus and scsi-hd internally.
This allows scsi related commands to be passed thorugh to the scsi-hd.
The query request and nop command work the same as the existing logic.

Well-known lus do not have a virtual scsi-bus and scsi-hd, and
handle the necessary scsi commands by emulating them directly.

Signed-off-by: Jeuk Kim 
---
 hw/ufs/lu.c| 1473 +++-
 hw/ufs/trace-events|   25 -
 hw/ufs/ufs.c   |  202 +-
 hw/ufs/ufs.h   |   36 +-
 include/block/ufs.h|2 +-
 tests/qtest/ufs-test.c |   37 +-
 6 files changed, 315 insertions(+), 1460 deletions(-)

diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
index 13b5e37b53..81bfff9b4e 100644
--- a/hw/ufs/lu.c
+++ b/hw/ufs/lu.c
@@ -19,57 +19,117 @@
 #include "trace.h"
 #include "ufs.h"
 
-/*
- * The code below handling SCSI commands is copied from hw/scsi/scsi-disk.c,
- * with minor adjustments to make it work for UFS.
- */
+#define SCSI_COMMAND_FAIL (-1)
 
-#define SCSI_DMA_BUF_SIZE (128 * KiB)
-#define SCSI_MAX_INQUIRY_LEN 256
-#define SCSI_INQUIRY_DATA_SIZE 36
-#define SCSI_MAX_MODE_LEN 256
-
-typedef struct UfsSCSIReq {
-SCSIRequest req;
-/* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.  */
-uint64_t sector;
-uint32_t sector_count;
-uint32_t buflen;
-bool started;
-bool need_fua_emulation;
-struct iovec iov;
-QEMUIOVector qiov;
-BlockAcctCookie acct;
-} UfsSCSIReq;
-
-static void ufs_scsi_free_request(SCSIRequest *req)
+static void ufs_build_upiu_sense_data(UfsRequest *req, uint8_t *sense,
+  uint32_t sense_len)
 {
-UfsSCSIReq *r = DO_UPCAST(UfsSCSIReq, req, req);
+req->rsp_upiu.sr.sense_data_len = cpu_to_be16(sense_len);
+assert(sense_len <= SCSI_SENSE_LEN);
+memcpy(req->rsp_upiu.sr.sense_data, sense, sense_len);
+}
+
+static void ufs_build_scsi_response_upiu(UfsRequest *req, uint8_t *sense,
+ uint32_t sense_len,
+ uint32_t transfered_len,
+ int16_t status)
+{
+uint32_t expected_len = 
be32_to_cpu(req->req_upiu.sc.exp_data_transfer_len);
+uint8_t flags = 0, response = UFS_COMMAND_RESULT_SUCCESS;
+uint16_t data_segment_length;
+
+if (expected_len > transfered_len) {
+req->rsp_upiu.sr.residual_transfer_count =
+cpu_to_be32(expected_len - transfered_len);
+flags |= UFS_UPIU_FLAG_UNDERFLOW;
+} else if (expected_len < transfered_len) {
+req->rsp_upiu.sr.residual_transfer_count =
+cpu_to_be32(transfered_len - expected_len);
+flags |= UFS_UPIU_FLAG_OVERFLOW;
+}
 
-qemu_vfree(r->iov.iov_base);
+if (status != 0) {
+ufs_build_upiu_sense_data(req, sense, sense_len);
+response = UFS_COMMAND_RESULT_FAIL;
+}
+
+data_segment_length =
+cpu_to_be16(sense_len + sizeof(req->rsp_upiu.sr.sense_data_len));
+ufs_build_upiu_header(req, UFS_UPIU_TRANSACTION_RESPONSE, flags, response,
+  status, data_segment_length);
 }
 
-static void scsi_check_condition(UfsSCSIReq *r, SCSISense sense)
+static void ufs_scsi_command_complete(SCSIRequest *scsi_req, size_t resid)
 {
-trace_ufs_scsi_check_condition(r->req.tag, sense.key, sense.asc,
-   sense.ascq);
-scsi_req_build_sense(&r->req, sense);
-scsi_req_complete(&r->req, CHECK_CONDITION);
+UfsRequest *req = scsi_req->hba_private;
+int16_t status = scsi_req->status;
+
+uint32_t transfered_len = scsi_req->cmd.xfer - resid;
+
+ufs_build_scsi_response_upiu(req, scsi_req->sense, scsi_req->sense_len,
+ transfered_len, status);
+
+ufs_complete_req(req, UFS_REQUEST_SUCCESS);
+
+scsi_req->hba_private = NULL;
+scsi_req_unref(scsi_req);
 }
 
-static int ufs_scsi_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf,
- uint32_t outbuf_len)
+static QEMUSGList *ufs_get_sg_list(SCSIRequest *scsi_req)
 {
-UfsHc *u = UFS(req->bus->qbus.parent);
-UfsLu *lu = DO_UPCAST(UfsLu, qdev, req->dev);
-uint8_t page_code = req->cmd.buf[2];
-int start, buflen = 0;
+UfsRequest *req = scsi_req->hba_private;
+return req->sg;
+}
+
+static const struct SCSIBusInfo ufs_scsi_info = {
+.tcq = true,
+.max_target = 0,
+.max_lun = UFS_MAX_LUS,
+.max_channel = 0,
+
+.get_sg_list = ufs_get_sg_list,
+.complete = ufs_scsi_command_complete,
+};
+
+static int ufs_emulate_report_luns(UfsRequest *req, uint8_t *outbuf,
+   uint32_t outbuf_len)
+{
+UfsHc *u = req->hc;
+int len = 0;
 
-if (outbuf_len < SCSI_INQUIRY_DATA_SIZE) {
-return -1;
+/* TODO: Support for cases where SELECT REPORT is