[Qemu-devel] [PATCH v4 0/2] hw/scsi: support SCSI-2 passthrough without PI

[Qemu-devel] [PATCH v4 0/2] hw/scsi: support SCSI-2 passthrough without PI

[Paolo Bonzini]

This is my version of Daniel's patch.  In order to keep the
RD/WRPROTECT check for emulated SCSI disks, I've added a new property
to scsi-disk/hd/cd devices as well.  The property, similar to the
earlier versions posted by Daniel, is available to the user, but for
scsi-disk/hd/cd it affects the INQUIRY value returned by the emulated
INQUIRY command.

Daniel, can you please test it?

Thanks,

Paolo

Daniel Henrique Barboza (1):
  hw/scsi: support SCSI-2 passthrough without PI

Paolo Bonzini (1):
  scsi-disk: allow customizing the SCSI version

 hw/scsi/scsi-disk.c    | 29 ++++++++++++++++++++++++-----
 hw/scsi/scsi-generic.c | 48 +++++++++++++++++++++++++++++++++++++-----------
 include/hw/scsi/scsi.h |  2 ++
 3 files changed, 63 insertions(+), 16 deletions(-)

-- 
2.16.2

[Qemu-devel] [PATCH 1/2] scsi-disk: allow customizing the SCSI version

[Paolo Bonzini]

We would like to have different behavior for passthrough devices
depending on the SCSI version they expose.  To prepare for that,
allow the user of emulated devices to specify the desired SCSI
level, and adjust the emulation according to the property value.
The next patch will set the level for scsi-block and scsi-generic
devices.

Based on a patch by Daniel Henrique Barboza
<danielhb@linux.vnet.ibm.com>.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c    | 29 ++++++++++++++++++++++++-----
 hw/scsi/scsi-generic.c |  1 +
 include/hw/scsi/scsi.h |  2 ++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f8ed8cf2b4..9400b97387 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -825,7 +825,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
      * block characteristics VPD page by default.  Not all of SPC-3
      * is actually implemented, but we're good enough.
      */
-    outbuf[2] = 5;
+    outbuf[2] = s->qdev.default_scsi_version;
     outbuf[3] = 2 | 0x10; /* Format 2, HiSup */
 
     if (buflen > 36) {
@@ -2193,7 +2193,11 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
     case READ_12:
     case READ_16:
         DPRINTF("Read (sector %" PRId64 ", count %u)\n", r->req.cmd.lba, len);
-        if (r->req.cmd.buf[1] & 0xe0) {
+        /* Protection information is not supported.  For SCSI versions 2 and
+         * older (as determined by snooping the guest's INQUIRY commands),
+         * there is no RD/WR/VRPROTECT, so skip this check in these versions.
+         */
+        if (s->qdev.scsi_version > 2 && (r->req.cmd.buf[1] & 0xe0)) {
             goto illegal_request;
         }
         if (!check_lba_range(s, r->req.cmd.lba, len)) {
@@ -2224,7 +2228,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
          * As far as DMA is concerned, we can treat it the same as a write;
          * scsi_block_do_sgio will send VERIFY commands.
          */
-        if (r->req.cmd.buf[1] & 0xe0) {
+        if (s->qdev.scsi_version > 2 && (r->req.cmd.buf[1] & 0xe0)) {
             goto illegal_request;
         }
         if (!check_lba_range(s, r->req.cmd.lba, len)) {
@@ -2270,6 +2274,8 @@ static void scsi_disk_reset(DeviceState *dev)
     /* reset tray statuses */
     s->tray_locked = 0;
     s->tray_open = 0;
+
+    s->qdev.scsi_version = s->qdev.default_scsi_version;
 }
 
 static void scsi_disk_resize_cb(void *opaque)
@@ -2814,6 +2820,8 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, uint8_t *buf)
 static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIBlockReq *r = (SCSIBlockReq *)req;
+    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
+
     r->cmd = req->cmd.buf[0];
     switch (r->cmd >> 5) {
     case 0:
@@ -2839,8 +2847,11 @@ static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
         abort();
     }
 
-    if (r->cdb1 & 0xe0) {
-        /* Protection information is not supported.  */
+    /* Protection information is not supported.  For SCSI versions 2 and
+     * older (as determined by snooping the guest's INQUIRY commands),
+     * there is no RD/WR/VRPROTECT, so skip this check in these versions.
+     */
+    if (s->qdev.scsi_version > 2 && (req->cmd.buf[1] & 0xe0)) {
         scsi_check_condition(&r->req, SENSE_CODE(INVALID_FIELD));
         return 0;
     }
@@ -2952,6 +2963,8 @@ static Property scsi_hd_properties[] = {
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
     DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
+    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
+                      5),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -2997,6 +3010,8 @@ static Property scsi_cd_properties[] = {
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
+                      5),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3025,6 +3040,8 @@ static Property scsi_block_properties[] = {
     DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
     DEFINE_PROP_BOOL("share-rw", SCSIDiskState, qdev.conf.share_rw, false),
     DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
+    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
+                      5),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3065,6 +3082,8 @@ static Property scsi_disk_properties[] = {
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
+    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
+                      5),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 4753f8738f..1870085762 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -474,6 +474,7 @@ static void scsi_generic_reset(DeviceState *dev)
 {
     SCSIDevice *s = SCSI_DEVICE(dev);
 
+    s->scsi_version = s->default_scsi_version;
     scsi_device_purge_requests(s, SENSE_CODE(RESET));
 }
 
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 7ecaddac9d..e35137ea78 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -85,6 +85,8 @@ struct SCSIDevice
     uint64_t max_lba;
     uint64_t wwn;
     uint64_t port_wwn;
+    int scsi_version;
+    int default_scsi_version;
 };
 
 extern const VMStateDescription vmstate_scsi_device;
-- 
2.16.2

Re: [Qemu-devel] [PATCH 1/2] scsi-disk: allow customizing the SCSI version

[Daniel Henrique Barboza]

On 04/05/2018 01:23 PM, Paolo Bonzini wrote:
> We would like to have different behavior for passthrough devices
> depending on the SCSI version they expose.  To prepare for that,
> allow the user of emulated devices to specify the desired SCSI
> level, and adjust the emulation according to the property value.
> The next patch will set the level for scsi-block and scsi-generic
> devices.
>
> Based on a patch by Daniel Henrique Barboza
> <danielhb@linux.vnet.ibm.com>.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>


>   hw/scsi/scsi-disk.c    | 29 ++++++++++++++++++++++++-----
>   hw/scsi/scsi-generic.c |  1 +
>   include/hw/scsi/scsi.h |  2 ++
>   3 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index f8ed8cf2b4..9400b97387 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -825,7 +825,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>        * block characteristics VPD page by default.  Not all of SPC-3
>        * is actually implemented, but we're good enough.
>        */
> -    outbuf[2] = 5;
> +    outbuf[2] = s->qdev.default_scsi_version;
>       outbuf[3] = 2 | 0x10; /* Format 2, HiSup */
>
>       if (buflen > 36) {
> @@ -2193,7 +2193,11 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
>       case READ_12:
>       case READ_16:
>           DPRINTF("Read (sector %" PRId64 ", count %u)\n", r->req.cmd.lba, len);
> -        if (r->req.cmd.buf[1] & 0xe0) {
> +        /* Protection information is not supported.  For SCSI versions 2 and
> +         * older (as determined by snooping the guest's INQUIRY commands),
> +         * there is no RD/WR/VRPROTECT, so skip this check in these versions.
> +         */
> +        if (s->qdev.scsi_version > 2 && (r->req.cmd.buf[1] & 0xe0)) {
>               goto illegal_request;
>           }
>           if (!check_lba_range(s, r->req.cmd.lba, len)) {
> @@ -2224,7 +2228,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
>            * As far as DMA is concerned, we can treat it the same as a write;
>            * scsi_block_do_sgio will send VERIFY commands.
>            */
> -        if (r->req.cmd.buf[1] & 0xe0) {
> +        if (s->qdev.scsi_version > 2 && (r->req.cmd.buf[1] & 0xe0)) {
>               goto illegal_request;
>           }
>           if (!check_lba_range(s, r->req.cmd.lba, len)) {
> @@ -2270,6 +2274,8 @@ static void scsi_disk_reset(DeviceState *dev)
>       /* reset tray statuses */
>       s->tray_locked = 0;
>       s->tray_open = 0;
> +
> +    s->qdev.scsi_version = s->qdev.default_scsi_version;
>   }
>
>   static void scsi_disk_resize_cb(void *opaque)
> @@ -2814,6 +2820,8 @@ static bool scsi_block_is_passthrough(SCSIDiskState *s, uint8_t *buf)
>   static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
>   {
>       SCSIBlockReq *r = (SCSIBlockReq *)req;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
> +
>       r->cmd = req->cmd.buf[0];
>       switch (r->cmd >> 5) {
>       case 0:
> @@ -2839,8 +2847,11 @@ static int32_t scsi_block_dma_command(SCSIRequest *req, uint8_t *buf)
>           abort();
>       }
>
> -    if (r->cdb1 & 0xe0) {
> -        /* Protection information is not supported.  */
> +    /* Protection information is not supported.  For SCSI versions 2 and
> +     * older (as determined by snooping the guest's INQUIRY commands),
> +     * there is no RD/WR/VRPROTECT, so skip this check in these versions.
> +     */
> +    if (s->qdev.scsi_version > 2 && (req->cmd.buf[1] & 0xe0)) {
>           scsi_check_condition(&r->req, SENSE_CODE(INVALID_FIELD));
>           return 0;
>       }
> @@ -2952,6 +2963,8 @@ static Property scsi_hd_properties[] = {
>       DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
>                          DEFAULT_MAX_IO_SIZE),
>       DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> +    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
> +                      5),
>       DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
>       DEFINE_PROP_END_OF_LIST(),
>   };
> @@ -2997,6 +3010,8 @@ static Property scsi_cd_properties[] = {
>       DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
>       DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
>                          DEFAULT_MAX_IO_SIZE),
> +    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
> +                      5),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> @@ -3025,6 +3040,8 @@ static Property scsi_block_properties[] = {
>       DEFINE_PROP_DRIVE("drive", SCSIDiskState, qdev.conf.blk),
>       DEFINE_PROP_BOOL("share-rw", SCSIDiskState, qdev.conf.share_rw, false),
>       DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
> +    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
> +                      5),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> @@ -3065,6 +3082,8 @@ static Property scsi_disk_properties[] = {
>                          DEFAULT_MAX_UNMAP_SIZE),
>       DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
>                          DEFAULT_MAX_IO_SIZE),
> +    DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
> +                      5),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 4753f8738f..1870085762 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -474,6 +474,7 @@ static void scsi_generic_reset(DeviceState *dev)
>   {
>       SCSIDevice *s = SCSI_DEVICE(dev);
>
> +    s->scsi_version = s->default_scsi_version;
>       scsi_device_purge_requests(s, SENSE_CODE(RESET));
>   }
>
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 7ecaddac9d..e35137ea78 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -85,6 +85,8 @@ struct SCSIDevice
>       uint64_t max_lba;
>       uint64_t wwn;
>       uint64_t port_wwn;
> +    int scsi_version;
> +    int default_scsi_version;
>   };
>
>   extern const VMStateDescription vmstate_scsi_device;

[Qemu-devel] [PATCH 2/2] hw/scsi: support SCSI-2 passthrough without PI

[Paolo Bonzini]

From: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

QEMU SCSI code makes assumptions about how the PROTECT and BYTCHK
works in the protocol, denying support for PI (Protection
Information) in case the guest OS requests it. However, in SCSI versions 2
and older, there is no PI concept in the protocol.

This means that when dealing with such devices:

- there is no PROTECT bit in byte 5 of the standard INQUIRY response. The
whole byte is marked as "Reserved";

- there is no RDPROTECT in byte 2 of READ. We have 'Logical Unit Number'
in this field instead;

- there is no VRPROTECT in byte 2 of VERIFY. We have 'Logical Unit Number'
in this field instead. This also means that the BYTCHK bit in this case
is not related to PI.

Since QEMU does not consider these changes, a SCSI passthrough using
a SCSI-2 device will not work. It will mistake these fields with
PI information and return Illegal Request SCSI SENSE thinking
that the driver is asking for PI support.

This patch fixes it by adding a new attribute called 'scsi_version'
that is read from the standard INQUIRY response of passthrough
devices. This allows for a version verification before applying
conditions related to PI that doesn't apply for older versions.

Reported-by: Dac Nguyen <dacng@us.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
Message-Id: <20180327211451.14647-1-danielhb@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c    |  2 +-
 hw/scsi/scsi-generic.c | 47 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 9400b97387..ded23d36ca 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -3041,7 +3041,7 @@ static Property scsi_block_properties[] = {
     DEFINE_PROP_BOOL("share-rw", SCSIDiskState, qdev.conf.share_rw, false),
     DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
     DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
-                      5),
+                      -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 1870085762..381f04e339 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -194,17 +194,40 @@ static void scsi_read_complete(void * opaque, int ret)
             r->buf[3] |= 0x80;
         }
     }
-    if (s->type == TYPE_DISK &&
-        r->req.cmd.buf[0] == INQUIRY &&
-        r->req.cmd.buf[2] == 0xb0) {
-        uint32_t max_transfer =
-            blk_get_max_transfer(s->conf.blk) / s->blocksize;
-
-        assert(max_transfer);
-        stl_be_p(&r->buf[8], max_transfer);
-        /* Also take care of the opt xfer len. */
-        stl_be_p(&r->buf[12],
-                 MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
+    if (r->req.cmd.buf[0] == INQUIRY) {
+        /*
+         *  EVPD set to zero returns the standard INQUIRY data.
+         *
+         *  Check if scsi_version is unset (-1) to avoid re-defining it
+         *  each time an INQUIRY with standard data is received.
+         *  scsi_version is initialized with -1 in scsi_generic_reset
+         *  and scsi_disk_reset, making sure that we'll set the
+         *  scsi_version after a reset. If the version field of the
+         *  INQUIRY response somehow changes after a guest reboot,
+         *  we'll be able to keep track of it.
+         *
+         *  On SCSI-2 and older, first 3 bits of byte 2 is the
+         *  ANSI-approved version, while on later versions the
+         *  whole byte 2 contains the version. Check if we're dealing
+         *  with a newer version and, in that case, assign the
+         *  whole byte.
+         */
+        if (s->scsi_version == -1 && !(r->req.cmd.buf[1] & 0x01)) {
+            s->scsi_version = r->buf[2] & 0x07;
+            if (s->scsi_version > 2) {
+                s->scsi_version = r->buf[2];
+            }
+        }
+        if (s->type == TYPE_DISK && r->req.cmd.buf[2] == 0xb0) {
+            uint32_t max_transfer =
+                blk_get_max_transfer(s->conf.blk) / s->blocksize;
+
+            assert(max_transfer);
+            stl_be_p(&r->buf[8], max_transfer);
+            /* Also take care of the opt xfer len. */
+            stl_be_p(&r->buf[12],
+                     MIN_NON_ZERO(max_transfer, ldl_be_p(&r->buf[12])));
+        }
     }
     scsi_req_data(&r->req, len);
     scsi_req_unref(&r->req);
@@ -550,6 +573,8 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
 
     DPRINTF("block size %d\n", s->blocksize);
 
+    /* Only used by scsi-block, but initialize it nevertheless to be clean.  */
+    s->default_scsi_version = -1;
     scsi_generic_read_device_identification(s);
 }
 
-- 
2.16.2

Re: [Qemu-devel] [PATCH v4 0/2] hw/scsi: support SCSI-2 passthrough without PI

[Daniel Henrique Barboza]

Hi,


On 04/05/2018 01:23 PM, Paolo Bonzini wrote:
> This is my version of Daniel's patch.  In order to keep the
> RD/WRPROTECT check for emulated SCSI disks, I've added a new property
> to scsi-disk/hd/cd devices as well.  The property, similar to the
> earlier versions posted by Daniel, is available to the user, but for
> scsi-disk/hd/cd it affects the INQUIRY value returned by the emulated
> INQUIRY command.
>
> Daniel, can you please test it?

Emulated case worked as intended: I was able to set the scsi_version of 
the emulated device via command line.

One thing I noticed is that I am also able to set the scsi_version of 
scsi-block devices - if I set scsi_version=6 in a SCSI passthrough 
device that has version=2, the version that ends up being passed to the 
guest is 6. In a quick look I believe that this behavior is caused by 
setting

+    s->qdev.scsi_version = s->qdev.default_scsi_version;

in scsi_disk_reset, which is the reset function that is also used for 
scsi-block devices. So, setting scsi_version=6 in the command line 
overwrites the default_scsi_version = -1 that is being set in patch 2, 
making scsi_version =! -1 and bypassing the scsi_version assignment code.

A quick fix is to get rid of the scsi_version == -1 restriction to set 
the scsi_version. This is something that Fam proposed in one of the 
reviews. Given that a well behaved guest will not spam INQUIRY messages 
(causing the code to always recalculate the same version again), I am 
fine with it.

Another quick fix is to make a new scsi_block_reset function that calls 
scsi_disk_reset and  sets s->scsi_version = -1 right after.

Yet another fix is to fully prohibit the user to set scsi_version 
scsi-block and scsi-generic cases, returning an error message right off 
the start. Not sure how hard this would be - perhaps the above 
alternatives are cleaner.

Another fix is ... no fix. I am not sure how far the design philosophy 
of passthrough devices allows the user to overwrite device parameters in 
despite of their real values, but ....  what if the user wants to 
enforce a scsi_version when using a scsi-block device? The device will 
surely behave badly, but the user explicitly enforced it via command 
line, so perhaps let him/her have at it?


Thanks,


Daniel




>
> Thanks,
>
> Paolo
>
> Daniel Henrique Barboza (1):
>    hw/scsi: support SCSI-2 passthrough without PI
>
> Paolo Bonzini (1):
>    scsi-disk: allow customizing the SCSI version
>
>   hw/scsi/scsi-disk.c    | 29 ++++++++++++++++++++++++-----
>   hw/scsi/scsi-generic.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>   include/hw/scsi/scsi.h |  2 ++
>   3 files changed, 63 insertions(+), 16 deletions(-)
>

Re: [Qemu-devel] [PATCH v4 0/2] hw/scsi: support SCSI-2 passthrough without PI

[Paolo Bonzini]

On 06/04/2018 01:10, Daniel Henrique Barboza wrote:
> Yet another fix is to fully prohibit the user to set scsi_version
> scsi-block and scsi-generic cases, returning an error message right off
> the start. Not sure how hard this would be - perhaps the above
> alternatives are cleaner.
> 
> Another fix is ... no fix. I am not sure how far the design philosophy
> of passthrough devices allows the user to overwrite device parameters in
> despite of their real values, but ....  what if the user wants to
> enforce a scsi_version when using a scsi-block device? The device will
> surely behave badly, but the user explicitly enforced it via command
> line, so perhaps let him/her have at it?

Yeah, that was the idea.  Removing it is as easy as dropping the
DEFINE_PROP_INT32 and initializing to -1 in scsi_block_realize (just
like for scsi_generic_realize), but for now we can keep it.  My usecase
was more to downgrade scsi_version from newer to 2.

Paolo

Re: [Qemu-devel] [PATCH v4 0/2] hw/scsi: support SCSI-2 passthrough without PI

[Daniel Henrique Barboza]

On 04/06/2018 07:12 AM, Paolo Bonzini wrote:
> On 06/04/2018 01:10, Daniel Henrique Barboza wrote:
>> Yet another fix is to fully prohibit the user to set scsi_version
>> scsi-block and scsi-generic cases, returning an error message right off
>> the start. Not sure how hard this would be - perhaps the above
>> alternatives are cleaner.
>>
>> Another fix is ... no fix. I am not sure how far the design philosophy
>> of passthrough devices allows the user to overwrite device parameters in
>> despite of their real values, but ....  what if the user wants to
>> enforce a scsi_version when using a scsi-block device? The device will
>> surely behave badly, but the user explicitly enforced it via command
>> line, so perhaps let him/her have at it?
> Yeah, that was the idea.  Removing it is as easy as dropping the
> DEFINE_PROP_INT32 and initializing to -1 in scsi_block_realize (just
> like for scsi_generic_realize), but for now we can keep it.  My usecase
> was more to downgrade scsi_version from newer to 2.

I agree. +1 for pulling the patch series as is.


Thanks,


Daniel


>
> Paolo
>