Ping: Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
Dear Paolo This is a ping for the following. If you don't mind, could you give me some feedback? Thank you very much. Jeuk On 23. 9. 21. 17:38, Jeuk Kim wrote: Dear Paolo Hi. I've been looking into how ufs-lu can share code with scsi-hd. I have verified that ufs-lu can use scsi-hd's code, and I would like to modify it to do so. I've validated two possible fixes. I'd like to hear your thoughts. Option1. As you mentioned, using ufsbus, which inherits from scsibus, removes the ufs-lu device type and use scsi-hd. (like -device ufs,id=ufs0 -device scsi-hd,bus=ufs0) I've verified that this method is implementable, except for one problem. Because we are using the scsi-hd type instead of the ufs-lu type, the ufs has to manage all the ufs-lu specific data (such as the unit descriptor). However, since there is no ufs_lu_realize() function, we need a way to notify the ufs when a new scsi-hd device is added. Would there be a way to let the ufs know that a new scsi-hd has been added at scsi_hd_realize() time? Option 2. Use qdev_new() & qdev_realize() to make ufs-lu have a virtual scsi bus and scsi-hd. The ufs-lu can pass through SCSI commands to the virtual scsi-hd. This is similar to the method used by the device "usb-storage". With this method, I can keep the ufs-lu device type (ufs_lu_realize() makes it convenient to manage ufs-lu related data) and avoid duplicating code with scsi-hd. So I prefer this approach, but the annotation for "usb-storage" is marked with a "Hack alert", so I'm not sure if this is the right way. The code can be found in usb_msd_storage_realize() (hw/usb/dev-storage-classic.c:51). I am wondering if you could give me some advice on this and your preferred direction for fixing it. Thank you so much. Jeuk
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 2023-09-15 4:59 PM, Paolo Bonzini wrote: On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo Dear Paolo Hi. I've been looking into how ufs-lu can share code with scsi-hd. I have verified that ufs-lu can use scsi-hd's code, and I would like to modify it to do so. I've validated two possible fixes. I'd like to hear your thoughts. Option1. As you mentioned, using ufsbus, which inherits from scsibus, removes the ufs-lu device type and use scsi-hd. (like -device ufs,id=ufs0 -device scsi-hd,bus=ufs0) I've verified that this method is implementable, except for one problem. Because we are using the scsi-hd type instead of the ufs-lu type, the ufs has to manage all the ufs-lu specific data (such as the unit descriptor). However, since there is no ufs_lu_realize() function, we need a way to notify the ufs when a new scsi-hd device is added. Would there be a way to let the ufs know that a new scsi-hd has been added at scsi_hd_realize() time? Option 2. Use qdev_new() & qdev_realize() to make ufs-lu have a virtual scsi bus and scsi-hd. The ufs-lu can pass through SCSI commands to the virtual scsi-hd. This is similar to the method used by the device "usb-storage". With this method, I can keep the ufs-lu device type (ufs_lu_realize() makes it convenient to manage ufs-lu related data) and avoid duplicating code with scsi-hd. So I prefer this approach, but the annotation for "usb-storage" is marked with a "Hack alert", so I'm not sure if this is the right way. The code can be found in usb_msd_storage_realize() (hw/usb/dev-storage-classic.c:51). I am wondering if you could give me some advice on this and your preferred direction for fixing it. Thank you so much. Jeuk
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 2023-09-18 오후 1:41, Jeuk Kim wrote: On 2023-09-15 16:59, Paolo Bonzini wrote: On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo Thanks for the comment. ufs-lu took most of its code from scsi-hd, so I completely agree that we should make scsi-hd code shareable to reduce code redundancy and make it better. Sorry about it. This sentence is misleading. I meant to say "I completely agree that ufs-lu should be made to reuse scsi-hd code to reduce code redundancy and improve code quality." I will fix it and get back to you soon. Thank you. Sincerely, Jeuk Thanks
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 2023-09-15 16:59, Paolo Bonzini wrote: On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo Thanks for the comment. ufs-lu took most of its code from scsi-hd, so I completely agree that we should make scsi-hd code shareable to reduce code redundancy and make it better. I will fix it and get back to you soon. Thank you. Sincerely, Jeuk
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 9/15/23 00:19, Jeuk Kim wrote: First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. This looks like something that can be implemented in the UFS subsystem. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. This can also be implemented in UfsBus. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. These should be implemented in scsi-hd as well. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. Some of these don't have much justification, and others (such as the control page) could be done in scsi-hd as well. We should look into cleaning this up and making ufs-lu share a lot more code with scsi-hd; possibly even supporting -device scsi-hd with UFS devices. I am not going to ask you for a revert, but if this is not done before 8.2 is out, I will ask you to disable it by default in hw/ufs/Kconfig. In the future, please Cc the SCSI maintainers for UFS patches. Paolo
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 23. 9. 15. 02:31, Paolo Bonzini wrote: On 9/7/23 20:16, Stefan Hajnoczi wrote: From: Jeuk Kim This commit adds support for ufs logical unit. The LU handles processing for the SCSI command, unit descriptor query request. This commit enables the UFS device to process IO requests. Signed-off-by: Jeuk Kim Reviewed-by: Stefan Hajnoczi Message-id:beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20@gmail.com Signed-off-by: Stefan Hajnoczi --- Jeuk, can you explain the differences between scsi-hd and ufs-lu, apart from the different bus type? Ideally, the UFS controller would be in hw/scsi/ufs.c and there would be no need for ufs-lu at all. Would it make sense to allow any SCSI device into a UFS bus without the need to have duplicate code? Thanks! Paolo Hi Paolo, While ufs does use the SCSI specification to communicate with the driver, it doesn't behave exactly like a typical scsi device. First, ufs-lu has a feature called "unit descriptor". This feature shows the status of the ufs-lu and only works with UFS-specific "query request" commands, not SCSI commands. UFS also has something called a well-known lu. Unlike typical SCSI devices, where each lu is independent, UFS can control other lu's through the well-known lu. Finally, UFS-LU will have features that SCSI-HD does not have, such as the zone block command. In addition to this, I wanted some scsi commands to behave differently from scsi-hd, for example, the Inquiry command should read "QEMU UFS" instead of "QEMU HARDDISK", and the mode_sense_page command should have a different result. For these reasons, I chose to generate the ufs-lu code separately. Please let me know if you have any comments on this. Thanks! Jeuk
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On 9/7/23 20:16, Stefan Hajnoczi wrote: From: Jeuk Kim This commit adds support for ufs logical unit. The LU handles processing for the SCSI command, unit descriptor query request. This commit enables the UFS device to process IO requests. Signed-off-by: Jeuk Kim Reviewed-by: Stefan Hajnoczi Message-id:beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20@gmail.com Signed-off-by: Stefan Hajnoczi --- Jeuk, can you explain the differences between scsi-hd and ufs-lu, apart from the different bus type? Ideally, the UFS controller would be in hw/scsi/ufs.c and there would be no need for ufs-lu at all. Would it make sense to allow any SCSI device into a UFS bus without the need to have duplicate code? Thanks! Paolo
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On Thu, 7 Sept 2023 at 19:18, Stefan Hajnoczi wrote:
>
> From: Jeuk Kim
>
> This commit adds support for ufs logical unit.
> The LU handles processing for the SCSI command,
> unit descriptor query request.
>
> This commit enables the UFS device to process
> IO requests.
>
> Signed-off-by: Jeuk Kim
> Reviewed-by: Stefan Hajnoczi
> Message-id:
> beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20@gmail.com
> Signed-off-by: Stefan Hajnoczi
> ---
Hi; Coverity points out a NULL pointer dereference issue in
this code (CID 1519043):
> +static void ufs_lu_realize(SCSIDevice *dev, Error **errp)
> +{
> +UfsLu *lu = DO_UPCAST(UfsLu, qdev, dev);
> +BusState *s = qdev_get_parent_bus(>qdev);
> +UfsHc *u = UFS(s->parent);
> +AioContext *ctx = NULL;
> +uint64_t nb_sectors, nb_blocks;
> +
> +if (!ufs_lu_check_constraints(lu, errp)) {
> +return;
> +}
> +
> +if (lu->qdev.conf.blk) {
Here we check whether lu->qdev.conf.blk is non-NULL, implying
that it can be NULL at this point...
> +ctx = blk_get_aio_context(lu->qdev.conf.blk);
> +aio_context_acquire(ctx);
> +if (!blkconf_blocksizes(>qdev.conf, errp)) {
> +goto out;
> +}
> +}
> +lu->qdev.blocksize = UFS_BLOCK_SIZE;
> +blk_get_geometry(lu->qdev.conf.blk, _sectors);
...but here we pass it to blk_get_geometry(), which will
unconditionally dereference it, and crashes if it is NULL.
Either the NULL check above is unnecessary, or else this
bit of the code needs to do something else for NULL.
> +nb_blocks = nb_sectors / (lu->qdev.blocksize / BDRV_SECTOR_SIZE);
> +if (nb_blocks > UINT32_MAX) {
> +nb_blocks = UINT32_MAX;
> +}
> +lu->qdev.max_lba = nb_blocks;
> +lu->qdev.type = TYPE_DISK;
> +
> +ufs_init_lu(lu);
> +if (!ufs_add_lu(u, lu, errp)) {
> +goto out;
> +}
> +
> +ufs_lu_brdv_init(lu, errp);
> +out:
> +if (ctx) {
> +aio_context_release(ctx);
> +}
> +}
thanks
-- PMM
Re: [PULL 4/5] hw/ufs: Support for UFS logical unit
On Thu, 7 Sept 2023 at 19:18, Stefan Hajnoczi wrote:
>
> From: Jeuk Kim
>
> This commit adds support for ufs logical unit.
> The LU handles processing for the SCSI command,
> unit descriptor query request.
>
> This commit enables the UFS device to process
> IO requests.
>
> Signed-off-by: Jeuk Kim
> Reviewed-by: Stefan Hajnoczi
> Message-id:
> beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20@gmail.com
> Signed-off-by: Stefan Hajnoczi
Hi; Coverity points out a buffer overrun (CID 1519051) in
this commit:
> @@ -52,12 +76,18 @@ typedef struct UfsParams {
>
> typedef struct UfsHc {
> PCIDevice parent_obj;
> +UfsBus bus;
> MemoryRegion iomem;
> UfsReg reg;
> UfsParams params;
> uint32_t reg_size;
> UfsRequest *req_list;
>
> +UfsLu *lus[UFS_MAX_LUS];
The array lus[] is UFS_MAX_LUS in size...
> +UfsWLu *report_wlu;
> +UfsWLu *dev_wlu;
> +UfsWLu *boot_wlu;
> +UfsWLu *rpmb_wlu;
> DeviceDescriptor device_desc;
> GeometryDescriptor geometry_desc;
> Attributes attributes;
> @@ -716,9 +834,11 @@ static const RpmbUnitDescriptor rpmb_unit_desc = {
>
> static QueryRespCode ufs_read_unit_desc(UfsRequest *req)
> {
> +UfsHc *u = req->hc;
> uint8_t lun = req->req_upiu.qr.index;
>
> -if (lun != UFS_UPIU_RPMB_WLUN && lun > UFS_MAX_LUS) {
> +if (lun != UFS_UPIU_RPMB_WLUN &&
> +(lun > UFS_MAX_LUS || u->lus[lun] == NULL)) {
...but here the guard is "> UFS_MAX_LUS", which permits
lun == UFS_MAX_LUS, which will index off the end of the array here...
> trace_ufs_err_query_invalid_index(req->req_upiu.qr.opcode, lun);
> return UFS_QUERY_RESULT_INVALID_INDEX;
> }
> @@ -726,8 +846,8 @@ static QueryRespCode ufs_read_unit_desc(UfsRequest *req)
> if (lun == UFS_UPIU_RPMB_WLUN) {
> memcpy(>rsp_upiu.qr.data, _unit_desc,
> rpmb_unit_desc.length);
> } else {
> -/* unit descriptor is not yet supported */
> -return UFS_QUERY_RESULT_INVALID_INDEX;
> +memcpy(>rsp_upiu.qr.data, >lus[lun]->unit_desc,
> + sizeof(u->lus[lun]->unit_desc));
...and here.
> }
>
> return UFS_QUERY_RESULT_SUCCESS;
Either the array needs to be larger, or the guard should be ">=".
thanks
-- PMM
[PULL 4/5] hw/ufs: Support for UFS logical unit
From: Jeuk Kim
This commit adds support for ufs logical unit.
The LU handles processing for the SCSI command,
unit descriptor query request.
This commit enables the UFS device to process
IO requests.
Signed-off-by: Jeuk Kim
Reviewed-by: Stefan Hajnoczi
Message-id:
beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20@gmail.com
Signed-off-by: Stefan Hajnoczi
---
hw/ufs/ufs.h | 43 ++
include/scsi/constants.h |1 +
hw/ufs/lu.c | 1445 ++
hw/ufs/ufs.c | 252 ++-
hw/ufs/meson.build |2 +-
hw/ufs/trace-events | 25 +
6 files changed, 1761 insertions(+), 7 deletions(-)
create mode 100644 hw/ufs/lu.c
diff --git a/hw/ufs/ufs.h b/hw/ufs/ufs.h
index 3d1b2cff4e..f244228617 100644
--- a/hw/ufs/ufs.h
+++ b/hw/ufs/ufs.h
@@ -18,6 +18,18 @@
#define UFS_MAX_LUS 32
#define UFS_BLOCK_SIZE 4096
+typedef struct UfsBusClass {
+BusClass parent_class;
+bool (*parent_check_address)(BusState *bus, DeviceState *dev, Error
**errp);
+} UfsBusClass;
+
+typedef struct UfsBus {
+SCSIBus parent_bus;
+} UfsBus;
+
+#define TYPE_UFS_BUS "ufs-bus"
+DECLARE_OBJ_CHECKERS(UfsBus, UfsBusClass, UFS_BUS, TYPE_UFS_BUS)
+
typedef enum UfsRequestState {
UFS_REQUEST_IDLE = 0,
UFS_REQUEST_READY = 1,
@@ -29,6 +41,7 @@ typedef enum UfsRequestState {
typedef enum UfsReqResult {
UFS_REQUEST_SUCCESS = 0,
UFS_REQUEST_FAIL = 1,
+UFS_REQUEST_NO_COMPLETE = 2,
} UfsReqResult;
typedef struct UfsRequest {
@@ -44,6 +57,17 @@ typedef struct UfsRequest {
QEMUSGList *sg;
} UfsRequest;
+typedef struct UfsLu {
+SCSIDevice qdev;
+uint8_t lun;
+UnitDescriptor unit_desc;
+} UfsLu;
+
+typedef struct UfsWLu {
+SCSIDevice qdev;
+uint8_t lun;
+} UfsWLu;
+
typedef struct UfsParams {
char *serial;
uint8_t nutrs; /* Number of UTP Transfer Request Slots */
@@ -52,12 +76,18 @@ typedef struct UfsParams {
typedef struct UfsHc {
PCIDevice parent_obj;
+UfsBus bus;
MemoryRegion iomem;
UfsReg reg;
UfsParams params;
uint32_t reg_size;
UfsRequest *req_list;
+UfsLu *lus[UFS_MAX_LUS];
+UfsWLu *report_wlu;
+UfsWLu *dev_wlu;
+UfsWLu *boot_wlu;
+UfsWLu *rpmb_wlu;
DeviceDescriptor device_desc;
GeometryDescriptor geometry_desc;
Attributes attributes;
@@ -71,6 +101,12 @@ typedef struct UfsHc {
#define TYPE_UFS "ufs"
#define UFS(obj) OBJECT_CHECK(UfsHc, (obj), TYPE_UFS)
+#define TYPE_UFS_LU "ufs-lu"
+#define UFSLU(obj) OBJECT_CHECK(UfsLu, (obj), TYPE_UFS_LU)
+
+#define TYPE_UFS_WLU "ufs-wlu"
+#define UFSWLU(obj) OBJECT_CHECK(UfsWLu, (obj), TYPE_UFS_WLU)
+
typedef enum UfsQueryFlagPerm {
UFS_QUERY_FLAG_NONE = 0x0,
UFS_QUERY_FLAG_READ = 0x1,
@@ -85,4 +121,11 @@ typedef enum UfsQueryAttrPerm {
UFS_QUERY_ATTR_WRITE = 0x2,
} UfsQueryAttrPerm;
+static inline bool is_wlun(uint8_t lun)
+{
+return (lun == UFS_UPIU_REPORT_LUNS_WLUN ||
+lun == UFS_UPIU_UFS_DEVICE_WLUN || lun == UFS_UPIU_BOOT_WLUN ||
+lun == UFS_UPIU_RPMB_WLUN);
+}
+
#endif /* HW_UFS_UFS_H */
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 6a8bad556a..9b98451912 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -231,6 +231,7 @@
#define MODE_PAGE_FLEXIBLE_DISK_GEOMETRY 0x05
#define MODE_PAGE_CACHING 0x08
#define MODE_PAGE_AUDIO_CTL 0x0e
+#define MODE_PAGE_CONTROL 0x0a
#define MODE_PAGE_POWER 0x1a
#define MODE_PAGE_FAULT_FAIL 0x1c
#define MODE_PAGE_TO_PROTECT 0x1d
diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
new file mode 100644
index 00..e1c46bddb1
--- /dev/null
+++ b/hw/ufs/lu.c
@@ -0,0 +1,1445 @@
+/*
+ * QEMU UFS Logical Unit
+ *
+ * Copyright (c) 2023 Samsung Electronics Co., Ltd. All rights reserved.
+ *
+ * Written by Jeuk Kim
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "qemu/memalign.h"
+#include "hw/scsi/scsi.h"
+#include "scsi/constants.h"
+#include "sysemu/block-backend.h"
+#include "qemu/cutils.h"
+#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_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)
+{
+UfsSCSIReq *r =
