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

[Fam Zheng]

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 <address@hidden>
> ---
>  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
>