1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] scsi: support NDOB (no data-out buffer) for WRITE SAME commands

Paolo Bonzini posted 1 patch 6 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180308155145.19315-1-pbonzini@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppcbe passed
Test ppcle passed
Test s390x passed
hw/scsi/scsi-bus.c       | 2 +-
hw/scsi/scsi-disk.c      | 2 +-
tests/virtio-scsi-test.c | 6 ++++++
3 files changed, 8 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] scsi: support NDOB (no data-out buffer) for WRITE SAME commands
Posted by Paolo Bonzini 6 years, 1 month ago 
A NDOB bit set to one specifies that the disk shall not transfer data
from the data-out buffer and shall process the command as if the data-out
buffer contained user data set to all zeroes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c       | 2 +-
 hw/scsi/scsi-disk.c      | 2 +-
 tests/virtio-scsi-test.c | 6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 1eaeffc830..9646743a7d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -944,7 +944,7 @@ static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
         break;
     case WRITE_SAME_10:
     case WRITE_SAME_16:
-        cmd->xfer = dev->blocksize;
+        cmd->xfer = buf[1] & 1 ? 0 : dev->blocksize;
         break;
     case READ_CAPACITY_10:
         cmd->xfer = 8;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c65c1ce56d..5b7a48f5a5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1807,7 +1807,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
         return;
     }
 
-    if (buffer_is_zero(inbuf, s->qdev.blocksize)) {
+    if ((req->cmd.buf[1] & 0x1) || buffer_is_zero(inbuf, s->qdev.blocksize)) {
         int flags = (req->cmd.buf[1] & 0x8) ? BDRV_REQ_MAY_UNMAP : 0;
 
         /* The request is used as the AIO opaque value, so add a ref.  */
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index 7393d69bb2..037872bb98 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -216,6 +216,9 @@ static void test_unaligned_write_same(void)
     const uint8_t write_same_cdb_2[VIRTIO_SCSI_CDB_SIZE] = {
         0x41, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x33, 0x00, 0x00
     };
+    const uint8_t write_same_cdb_ndob[VIRTIO_SCSI_CDB_SIZE] = {
+        0x41, 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, 0x33, 0x00, 0x00
+    };
 
     vs = qvirtio_scsi_pci_init(PCI_SLOT);
 
@@ -225,6 +228,9 @@ static void test_unaligned_write_same(void)
     g_assert_cmphex(0, ==,
         virtio_scsi_do_command(vs, write_same_cdb_2, NULL, 0, buf2, 512, NULL));
 
+    g_assert_cmphex(0, ==,
+        virtio_scsi_do_command(vs, write_same_cdb_ndob, NULL, 0, NULL, 0, NULL));
+
     qvirtio_scsi_pci_free(vs);
 }
 
-- 
2.14.3
Re: [Qemu-devel] [PATCH] scsi: support NDOB (no data-out buffer) for WRITE SAME commands
Posted by Fam Zheng 6 years, 1 month ago 
On Thu, 03/08 16:51, Paolo Bonzini wrote:
> A NDOB bit set to one specifies that the disk shall not transfer data
> from the data-out buffer and shall process the command as if the data-out
> buffer contained user data set to all zeroes.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-bus.c       | 2 +-
>  hw/scsi/scsi-disk.c      | 2 +-
>  tests/virtio-scsi-test.c | 6 ++++++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 1eaeffc830..9646743a7d 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -944,7 +944,7 @@ static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
>          break;
>      case WRITE_SAME_10:
>      case WRITE_SAME_16:
> -        cmd->xfer = dev->blocksize;
> +        cmd->xfer = buf[1] & 1 ? 0 : dev->blocksize;
>          break;
>      case READ_CAPACITY_10:
>          cmd->xfer = 8;
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index c65c1ce56d..5b7a48f5a5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1807,7 +1807,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf)
>          return;
>      }
>  
> -    if (buffer_is_zero(inbuf, s->qdev.blocksize)) {
> +    if ((req->cmd.buf[1] & 0x1) || buffer_is_zero(inbuf, s->qdev.blocksize)) {
>          int flags = (req->cmd.buf[1] & 0x8) ? BDRV_REQ_MAY_UNMAP : 0;

The standard says this bit is only allowed when UNMAP is set. Do we want to
check it here? I.e.

           if (!flags && (req->cmd.buf[1] & 0x1)) {
               scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
               return;
           }

>  
>          /* The request is used as the AIO opaque value, so add a ref.  */
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index 7393d69bb2..037872bb98 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -216,6 +216,9 @@ static void test_unaligned_write_same(void)
>      const uint8_t write_same_cdb_2[VIRTIO_SCSI_CDB_SIZE] = {
>          0x41, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x33, 0x00, 0x00
>      };
> +    const uint8_t write_same_cdb_ndob[VIRTIO_SCSI_CDB_SIZE] = {
> +        0x41, 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, 0x33, 0x00, 0x00
> +    };

If we add a check, maybe a test can be added to.

Fam

>  
>      vs = qvirtio_scsi_pci_init(PCI_SLOT);
>  
> @@ -225,6 +228,9 @@ static void test_unaligned_write_same(void)
>      g_assert_cmphex(0, ==,
>          virtio_scsi_do_command(vs, write_same_cdb_2, NULL, 0, buf2, 512, NULL));
>  
> +    g_assert_cmphex(0, ==,
> +        virtio_scsi_do_command(vs, write_same_cdb_ndob, NULL, 0, NULL, 0, NULL));
> +
>      qvirtio_scsi_pci_free(vs);
>  }
>  
> -- 
> 2.14.3
>
Re: [Qemu-devel] [PATCH] scsi: support NDOB (no data-out buffer) for WRITE SAME commands
Posted by Paolo Bonzini 6 years, 1 month ago 
On 09/03/2018 10:24, Fam Zheng wrote:
> The standard says this bit is only allowed when UNMAP is set. Do we want to
> check it here? I.e.
> 
>            if (!flags && (req->cmd.buf[1] & 0x1)) {
>                scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
>                return;
>            }

Yeah, I had it like that before but my copy of the standard actually
doesn't say that.  It says:

---
To ensure that subsequent read operations return all zeros in a logical
block, use the WRITE SAME (16) command with the NDOB bit set to one. If
the UNMAP bit is set to one, then the device server may unmap the
logical blocks specified by the WRITE SAME (16) command as described in
4.7.3.4.4.

[...]

A NDOB bit set to zero specifies that the device server shall process
the command using logical block data from the Data-Out Buffer. A NDOB
bit set to one specifies that the device server shall not transfer data
from the Data-Out Buffer and shall process the command as if the
Data-Out Buffer contained user data set to all zeroes.
---

This is sbc4r04, and sbc4r15 (the latest) is more or less the same, with
some complications related because LBPRZ has been recently extended.

Paolo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.