[Qemu-devel] [PATCHv2 0/4] scsi: enclosure support

[Qemu-devel] [PATCHv2 0/4] scsi: enclosure support

[Hannes Reinecke]

Hi all,

due to a customer issue I've added simple subenclosure support
to the SCSI emulation. By setting the 'enclosure' option to a
SCSI device we will now present an enclosure device on LUN0
(if LUN0 is otherwise unassigned) or setting the 'EncServ' bit
in the inquiry data if LUN0 is assigned to a device.

Changes to v1:
- Add patch to clarify sense code responses
- Add 'enclosure' option for SCSI devices

Hannes Reinecke (4):
  scsi: Make LUN 0 a simple enclosure
  scsi: use qemu_uuid to generate logical identifier for SES
  scsi: clarify sense codes for LUN0 emulation
  scsi: Add 'enclosure' option for scsi devices

 hw/scsi/scsi-bus.c     | 85 ++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/scsi/scsi-disk.c    |  4 ++-
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 87 insertions(+), 3 deletions(-)

-- 
1.8.5.6

[Qemu-devel] [PATCHv2 1/4] scsi: Make LUN 0 a simple enclosure

[Hannes Reinecke]

Instead of having an 'invisible' LUN0 (in case LUN 0 is not connected)
this patch maks LUN0 a enclosure service, exposing it to the OS.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 hw/scsi/scsi-bus.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 23c51de..c89e82d 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -493,10 +493,11 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     if (r->req.lun != 0) {
         r->buf[0] = TYPE_NO_LUN;
     } else {
-        r->buf[0] = TYPE_NOT_PRESENT | TYPE_INACTIVE;
+        r->buf[0] = TYPE_ENCLOSURE;
         r->buf[2] = 5; /* Version */
         r->buf[3] = 2 | 0x10; /* HiSup, response data format */
         r->buf[4] = r->len - 5; /* Additional Length = (Len - 1) - 4 */
+        r->buf[6] = 0x40; /* Enclosure service */
         r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ.  */
         memcpy(&r->buf[8], "QEMU    ", 8);
         memcpy(&r->buf[16], "QEMU TARGET     ", 16);
@@ -505,6 +506,54 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     return true;
 }
 
+static bool scsi_target_emulate_receive_diagnostic(SCSITargetReq *r)
+{
+    uint8_t page_code = r->req.cmd.buf[2];
+    unsigned char *enc_desc, *type_desc;
+
+    assert(r->req.dev->lun != r->req.lun);
+
+    scsi_target_alloc_buf(&r->req, 0x38);
+
+    switch (page_code) {
+    case 0x00:
+        r->buf[r->len++] = page_code ; /* this page */
+        r->buf[r->len++] = 0x00;
+        r->buf[r->len++] = 0x00;
+        r->buf[r->len++] = 0x03;
+        r->buf[r->len++] = 0x00;
+        r->buf[r->len++] = 0x01;
+        r->buf[r->len++] = 0x08;
+        break;
+    case 0x01:
+        memset(r->buf, 0, 0x38);
+        r->buf[0] = page_code;
+        r->buf[3] = 0x30;
+        enc_desc = &r->buf[8];
+        enc_desc[0] = 0x09;
+        enc_desc[2] = 1;
+        enc_desc[3] = 0x24;
+        memcpy(&enc_desc[12], "QEMU    ", 8);
+        memcpy(&enc_desc[20], "QEMU TARGET     ", 16);
+        pstrcpy((char *)&enc_desc[36], 4, qemu_hw_version());
+        type_desc = &r->buf[48];
+        type_desc[1] = 1;
+        r->len = 0x38;
+        break;
+    case 0x08:
+        r->buf[0] = page_code;
+        r->buf[1] = 0x00;
+        r->buf[2] = 0x00;
+        r->buf[3] = 0x00;
+        r->len = 4;
+        break;
+    default:
+        return false;
+    }
+    r->len = MIN(r->req.cmd.xfer, r->len);
+    return true;
+}
+
 static size_t scsi_sense_len(SCSIRequest *req)
 {
     if (req->dev->type == TYPE_SCANNER)
@@ -528,6 +577,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
             goto illegal_request;
         }
         break;
+    case RECEIVE_DIAGNOSTIC:
+        if (!scsi_target_emulate_receive_diagnostic(r)) {
+            goto illegal_request;
+        }
+        break;
     case REQUEST_SENSE:
         scsi_target_alloc_buf(&r->req, scsi_sense_len(req));
         r->len = scsi_device_get_sense(r->req.dev, r->buf,
-- 
1.8.5.6

[Qemu-devel] [PATCHv2 2/4] scsi: use qemu_uuid to generate logical identifier for SES

[Hannes Reinecke]

The SES enclosure descriptor requires a logical identifier,
so generate one using the qemu_uuid and the Qumranet OUI.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 hw/scsi/scsi-bus.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index c89e82d..8419c75 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -10,6 +10,7 @@
 #include "trace.h"
 #include "sysemu/dma.h"
 #include "qemu/cutils.h"
+#include "qemu/crc32c.h"
 
 static char *scsibus_get_dev_path(DeviceState *dev);
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
@@ -533,6 +534,22 @@ static bool scsi_target_emulate_receive_diagnostic(SCSITargetReq *r)
         enc_desc[0] = 0x09;
         enc_desc[2] = 1;
         enc_desc[3] = 0x24;
+        if (qemu_uuid_set) {
+            uint32_t crc;
+
+            /*
+             * Make this a NAA IEEE Registered identifier
+             * using Qumranet OUI (0x001A4A) and the
+             * crc32 from the system UUID.
+             */
+            enc_desc[4] = 0x50;
+            enc_desc[5] = 0x01;
+            enc_desc[6] = 0xa4;
+            enc_desc[7] = 0xa0;
+            crc = crc32c(0xffffffff, qemu_uuid.data, 16);
+            cpu_to_le32s(&crc);
+            memcpy(&enc_desc[8], &crc, 4);
+        }
         memcpy(&enc_desc[12], "QEMU    ", 8);
         memcpy(&enc_desc[20], "QEMU TARGET     ", 16);
         pstrcpy((char *)&enc_desc[36], 4, qemu_hw_version());
-- 
1.8.5.6

[Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Hannes Reinecke]

The LUN0 emulation is just that, an emulation for a non-existing
LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
coming from any other LUN.
And we should be aborting unhandled commands with INVALID OPCODE,
not LUN NOT SUPPORTED.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 hw/scsi/scsi-bus.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 8419c75..79a222f 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
 
+    if (req->lun != 0) {
+        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
+        scsi_req_complete(req, CHECK_CONDITION);
+        return 0;
+    }
     switch (buf[0]) {
     case REPORT_LUNS:
         if (!scsi_target_emulate_report_luns(r)) {
@@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
     case TEST_UNIT_READY:
         break;
     default:
-        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
+        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
         scsi_req_complete(req, CHECK_CONDITION);
         return 0;
     illegal_request:
-- 
1.8.5.6

Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Paolo Bonzini]

On 04/08/2017 10:36, Hannes Reinecke wrote:
> The LUN0 emulation is just that, an emulation for a non-existing
> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
> coming from any other LUN.
> And we should be aborting unhandled commands with INVALID OPCODE,
> not LUN NOT SUPPORTED.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  hw/scsi/scsi-bus.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 8419c75..79a222f 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>  {
>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>  
> +    if (req->lun != 0) {
> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
> +        scsi_req_complete(req, CHECK_CONDITION);
> +        return 0;
> +    }
>      switch (buf[0]) {
>      case REPORT_LUNS:
>          if (!scsi_target_emulate_report_luns(r)) {
> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>      case TEST_UNIT_READY:
>          break;
>      default:
> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>          scsi_req_complete(req, CHECK_CONDITION);
>          return 0;
>      illegal_request:
> 

I am queuing this one since it's an independent bugfix.

Paolo

Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Laszlo Ersek]

On 08/04/17 12:49, Paolo Bonzini wrote:
> On 04/08/2017 10:36, Hannes Reinecke wrote:
>> The LUN0 emulation is just that, an emulation for a non-existing
>> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
>> coming from any other LUN.
>> And we should be aborting unhandled commands with INVALID OPCODE,
>> not LUN NOT SUPPORTED.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  hw/scsi/scsi-bus.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>> index 8419c75..79a222f 100644
>> --- a/hw/scsi/scsi-bus.c
>> +++ b/hw/scsi/scsi-bus.c
>> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>  {
>>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>>  
>> +    if (req->lun != 0) {
>> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>> +        scsi_req_complete(req, CHECK_CONDITION);
>> +        return 0;
>> +    }
>>      switch (buf[0]) {
>>      case REPORT_LUNS:
>>          if (!scsi_target_emulate_report_luns(r)) {
>> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>      case TEST_UNIT_READY:
>>          break;
>>      default:
>> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>>          scsi_req_complete(req, CHECK_CONDITION);
>>          return 0;
>>      illegal_request:
>>
> 
> I am queuing this one since it's an independent bugfix.

This patch (ded6ddc5a7b9, "scsi: clarify sense codes for LUN0
emulation", 2017-08-04) seems to confuse the media detection in edk2's
"MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c".

Namely, when it enumerates the {targets}x{LUNs} matrix on the
virtio-scsi HBA, it now reports the following message, for each
(target,LUN) pair to which no actual SCSI device (like disk or CD-ROM)
is assigned on the command line:

ScsiDisk: Sense Key = 0x5 ASC = 0x25!

Unfortunately, this is not all that happens -- the ScsiDiskDxe driver
even installs a BlockIo protocol instance on the handle (again, there is
no media, and no actual SCSI device), on which further protocols are
stacked, such as BlockIo2:

ScsiDisk: Sense Key = 0x5 ASC = 0x25!
InstallProtocolInterface: [EfiBlockIoProtocol] 13A59A3A8
InstallProtocolInterface: [EfiBlockIo2Protocol] 13A59A3D8
InstallProtocolInterface: [EfiDiskInfoProtocol] 13A59A4D0

In turn, in BDS, UEFI boot options are auto-generated for these devices,
which is not nice, given that this procedure in BDS is very
pflash-intensive, and pflash access is remarkably slow on aarch64 KVM.

For example, if I use one virtio-scsi HBA, and put a CD-ROM on target 0,
LUN 0, and a disk on target 1, LUN 0, then edk2 will create protocol
interfaces, and matching boot options, for

  2 targets * 7 LUNs/target = 14 LUNs

of which only 2 make sense.


If I revert the patch (on top of v2.10.0-rc3), then everything works as
before -- BlockIo protocol instances are produced only for actual
devices (with media).

I guess the path forward is to fix the ScsiDiskDxe driver in edk2; the
new ASC should be recognized.

My question is, how *exactly* did this patch change the reported sense
key and ASC? That is, what did they use to be *before*? INVALID_OPCODE?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Laszlo Ersek]

On 08/17/17 22:57, Laszlo Ersek wrote:
> On 08/04/17 12:49, Paolo Bonzini wrote:
>> On 04/08/2017 10:36, Hannes Reinecke wrote:
>>> The LUN0 emulation is just that, an emulation for a non-existing
>>> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
>>> coming from any other LUN.
>>> And we should be aborting unhandled commands with INVALID OPCODE,
>>> not LUN NOT SUPPORTED.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  hw/scsi/scsi-bus.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>>> index 8419c75..79a222f 100644
>>> --- a/hw/scsi/scsi-bus.c
>>> +++ b/hw/scsi/scsi-bus.c
>>> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>  {
>>>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>>>  
>>> +    if (req->lun != 0) {
>>> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>> +        scsi_req_complete(req, CHECK_CONDITION);
>>> +        return 0;
>>> +    }
>>>      switch (buf[0]) {
>>>      case REPORT_LUNS:
>>>          if (!scsi_target_emulate_report_luns(r)) {
>>> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>      case TEST_UNIT_READY:
>>>          break;
>>>      default:
>>> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>>>          scsi_req_complete(req, CHECK_CONDITION);
>>>          return 0;
>>>      illegal_request:
>>>
>>
>> I am queuing this one since it's an independent bugfix.
> 
> This patch (ded6ddc5a7b9, "scsi: clarify sense codes for LUN0
> emulation", 2017-08-04) seems to confuse the media detection in edk2's
> "MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c".
> 
> Namely, when it enumerates the {targets}x{LUNs} matrix on the
> virtio-scsi HBA, it now reports the following message, for each
> (target,LUN) pair to which no actual SCSI device (like disk or CD-ROM)
> is assigned on the command line:
> 
> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
> 
> Unfortunately, this is not all that happens -- the ScsiDiskDxe driver
> even installs a BlockIo protocol instance on the handle (again, there is
> no media, and no actual SCSI device), on which further protocols are
> stacked, such as BlockIo2:
> 
> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
> InstallProtocolInterface: [EfiBlockIoProtocol] 13A59A3A8
> InstallProtocolInterface: [EfiBlockIo2Protocol] 13A59A3D8
> InstallProtocolInterface: [EfiDiskInfoProtocol] 13A59A4D0
> 
> In turn, in BDS, UEFI boot options are auto-generated for these devices,
> which is not nice, given that this procedure in BDS is very
> pflash-intensive, and pflash access is remarkably slow on aarch64 KVM.
> 
> For example, if I use one virtio-scsi HBA, and put a CD-ROM on target 0,
> LUN 0, and a disk on target 1, LUN 0, then edk2 will create protocol
> interfaces, and matching boot options, for
> 
>   2 targets * 7 LUNs/target = 14 LUNs
> 
> of which only 2 make sense.
> 
> 
> If I revert the patch (on top of v2.10.0-rc3), then everything works as
> before -- BlockIo protocol instances are produced only for actual
> devices (with media).
> 
> I guess the path forward is to fix the ScsiDiskDxe driver in edk2; the
> new ASC should be recognized.
> 
> My question is, how *exactly* did this patch change the reported sense
> key and ASC? That is, what did they use to be *before*? INVALID_OPCODE?

I found the bug in edk2. It's a missing error check. I'll send a patch
and CC you guys.

Thanks
Laszlo

Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Laszlo Ersek]

On 08/18/17 02:16, Laszlo Ersek wrote:
> On 08/17/17 22:57, Laszlo Ersek wrote:
>> On 08/04/17 12:49, Paolo Bonzini wrote:
>>> On 04/08/2017 10:36, Hannes Reinecke wrote:
>>>> The LUN0 emulation is just that, an emulation for a non-existing
>>>> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
>>>> coming from any other LUN.
>>>> And we should be aborting unhandled commands with INVALID OPCODE,
>>>> not LUN NOT SUPPORTED.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>>> ---
>>>>  hw/scsi/scsi-bus.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>>>> index 8419c75..79a222f 100644
>>>> --- a/hw/scsi/scsi-bus.c
>>>> +++ b/hw/scsi/scsi-bus.c
>>>> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>>  {
>>>>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>>>>
>>>> +    if (req->lun != 0) {
>>>> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>>> +        scsi_req_complete(req, CHECK_CONDITION);
>>>> +        return 0;
>>>> +    }
>>>>      switch (buf[0]) {
>>>>      case REPORT_LUNS:
>>>>          if (!scsi_target_emulate_report_luns(r)) {
>>>> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>>      case TEST_UNIT_READY:
>>>>          break;
>>>>      default:
>>>> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>>> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>>>>          scsi_req_complete(req, CHECK_CONDITION);
>>>>          return 0;
>>>>      illegal_request:
>>>>
>>>
>>> I am queuing this one since it's an independent bugfix.
>>
>> This patch (ded6ddc5a7b9, "scsi: clarify sense codes for LUN0
>> emulation", 2017-08-04) seems to confuse the media detection in
>> edk2's "MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c".
>>
>> Namely, when it enumerates the {targets}x{LUNs} matrix on the
>> virtio-scsi HBA, it now reports the following message, for each
>> (target,LUN) pair to which no actual SCSI device (like disk or
>> CD-ROM) is assigned on the command line:
>>
>> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
>>
>> Unfortunately, this is not all that happens -- the ScsiDiskDxe driver
>> even installs a BlockIo protocol instance on the handle (again, there
>> is no media, and no actual SCSI device), on which further protocols
>> are stacked, such as BlockIo2:
>>
>> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
>> InstallProtocolInterface: [EfiBlockIoProtocol] 13A59A3A8
>> InstallProtocolInterface: [EfiBlockIo2Protocol] 13A59A3D8
>> InstallProtocolInterface: [EfiDiskInfoProtocol] 13A59A4D0
>>
>> In turn, in BDS, UEFI boot options are auto-generated for these
>> devices, which is not nice, given that this procedure in BDS is very
>> pflash-intensive, and pflash access is remarkably slow on aarch64
>> KVM.
>>
>> For example, if I use one virtio-scsi HBA, and put a CD-ROM on target
>> 0, LUN 0, and a disk on target 1, LUN 0, then edk2 will create
>> protocol interfaces, and matching boot options, for
>>
>>   2 targets * 7 LUNs/target = 14 LUNs
>>
>> of which only 2 make sense.
>>
>>
>> If I revert the patch (on top of v2.10.0-rc3), then everything works
>> as before -- BlockIo protocol instances are produced only for actual
>> devices (with media).
>>
>> I guess the path forward is to fix the ScsiDiskDxe driver in edk2;
>> the new ASC should be recognized.
>>
>> My question is, how *exactly* did this patch change the reported
>> sense key and ASC? That is, what did they use to be *before*?
>> INVALID_OPCODE?
>
> I found the bug in edk2. It's a missing error check. I'll send a patch
> and CC you guys.

Actually, I think both QEMU and edk2 have bugs in this area.

The problem surfaces when the DiscoverScsiDevice() function in edk2's
"MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c" file sends an INQUIRY
command to a nonexistent LUN (for probing purposes).

(1) About QEMU:

(1.1) Without the above patch, QEMU sends the following response:

> DiscoverScsiDevice:1361: Lun=2 HostAdapterStatus=0 TargetStatus=0
>                          SenseDataLength=0 InquiryDataLength=36
> Sense {
> Sense }
> Inquiry {
> Inquiry 000000 7F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000020 00 00 00 00
> Inquiry }

This response *conforms* to the SPC-4, as follows:

> 6.4.1 INQUIRY command introduction
>
> [...]
>
> In response to an INQUIRY command received by an incorrect logical
> unit, the SCSI target device shall return the INQUIRY data with the
> peripheral qualifier set to the value defined in 6.4.2. The INQUIRY
> command shall return CHECK CONDITION status only when the device
> server is unable to return the requested INQUIRY data.
>
> [...]
>
> 6.4.2 Standard INQUIRY data
>
> [...]
>
> The PERIPHERAL QUALIFIER field and PERIPHERAL DEVICE TYPE field
> identify the peripheral device connected to the logical unit. If the
> SCSI target device is not capable of supporting a peripheral device
> connected to this logical unit, the device server shall set these
> fields to 7Fh (i.e., PERIPHERAL QUALIFIER field set to 011b and
> PERIPHERAL DEVICE TYPE field set to 1Fh).

(1.2) With the patch, QEMU sends the following response (ignore
InquiryData here, it's just an artifact of my debug patch):

> DiscoverScsiDevice:1361: Lun=2 HostAdapterStatus=0 TargetStatus=2
>                          SenseDataLength=18 InquiryDataLength=96
> Sense {
> Sense 000000 70 00 05 00 00 00 00 0A 00 00 00 00 25 00 00 00
> Sense 000010 00 00
> Sense }
> Inquiry {
> Inquiry 000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry 000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Inquiry }

Here "TargetStatus=2" means CHECK CONDITION, and the sense data
corresponds to "sense_code_LUN_NOT_SUPPORTED", returned by the first
hunk of the patch.

According to the SPC-4, this is less preferable, if not outright
invalid. The spec says (repeating it from above),

> The INQUIRY command shall return CHECK CONDITION status only when the
> device server is unable to return the requested INQUIRY data

with the "requested INQUIRY data" being, in this case,

> PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE TYPE
> field set to 1Fh

So in this regard, the patch implements an INQUIRY response that we
should minimally call "less preferred".

(2) About edk2:

The INQUIRY request site in question totally ignores TargetStatus and
SenseData. (This is arguably a bug; the SPC-4 *does* spell out a case
when the device server is allowed to respond with CHECK CONDITION.)

ScsiBusDxe happily produces ScsiIo protocol interfaces for nonexistent
LUNs in both cases, blindly saving the PERIPHERAL DEVICE TYPE value in
the ScsiIo protocol instance. The behavior differs only at a higher
level:

(2.1) Without the QEMU patch, the device type saved in the ScsiIo
protocol is 0x1f. This device type means "Unknown or no device type",
and so none of the SCSI device drivers in edk2 support it! In other
words, the ScsiIo protocol instances all exist (with an invalid device
type), but are ignored by other drivers.

(2.2) With the QEMU patch, the device type saved in the ScsiIo protocol
is zero, simply because the CHECK CONDITION response does not overwrite
the pre-zeroed InquiryData array in edk2. Type 0 happens to mean "Direct
access block device" (that is, disk), and that *is* bound by
ScsiDiskDxe. Hence the bogus BlockIo protocol instances, which show up
even in BDS.

I think I'll try to send an edk2 patch, but I suggest that the QEMU
patch too be reconsidered or revised.

Thanks
Laszlo

Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Hannes Reinecke]

On 08/18/2017 02:57 AM, Laszlo Ersek wrote:
> On 08/18/17 02:16, Laszlo Ersek wrote:
>> On 08/17/17 22:57, Laszlo Ersek wrote:
>>> On 08/04/17 12:49, Paolo Bonzini wrote:
>>>> On 04/08/2017 10:36, Hannes Reinecke wrote:
>>>>> The LUN0 emulation is just that, an emulation for a non-existing
>>>>> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
>>>>> coming from any other LUN.
>>>>> And we should be aborting unhandled commands with INVALID OPCODE,
>>>>> not LUN NOT SUPPORTED.
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>>>> ---
>>>>>  hw/scsi/scsi-bus.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>>>>> index 8419c75..79a222f 100644
>>>>> --- a/hw/scsi/scsi-bus.c
>>>>> +++ b/hw/scsi/scsi-bus.c
>>>>> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>>>  {
>>>>>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>>>>>
>>>>> +    if (req->lun != 0) {
>>>>> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>>>> +        scsi_req_complete(req, CHECK_CONDITION);
>>>>> +        return 0;
>>>>> +    }
>>>>>      switch (buf[0]) {
>>>>>      case REPORT_LUNS:
>>>>>          if (!scsi_target_emulate_report_luns(r)) {
>>>>> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>>>      case TEST_UNIT_READY:
>>>>>          break;
>>>>>      default:
>>>>> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>>>> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>>>>>          scsi_req_complete(req, CHECK_CONDITION);
>>>>>          return 0;
>>>>>      illegal_request:
>>>>>
>>>>
>>>> I am queuing this one since it's an independent bugfix.
>>>
>>> This patch (ded6ddc5a7b9, "scsi: clarify sense codes for LUN0
>>> emulation", 2017-08-04) seems to confuse the media detection in
>>> edk2's "MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c".
>>>
>>> Namely, when it enumerates the {targets}x{LUNs} matrix on the
>>> virtio-scsi HBA, it now reports the following message, for each
>>> (target,LUN) pair to which no actual SCSI device (like disk or
>>> CD-ROM) is assigned on the command line:
>>>
>>> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
>>>
>>> Unfortunately, this is not all that happens -- the ScsiDiskDxe driver
>>> even installs a BlockIo protocol instance on the handle (again, there
>>> is no media, and no actual SCSI device), on which further protocols
>>> are stacked, such as BlockIo2:
>>>
>>> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
>>> InstallProtocolInterface: [EfiBlockIoProtocol] 13A59A3A8
>>> InstallProtocolInterface: [EfiBlockIo2Protocol] 13A59A3D8
>>> InstallProtocolInterface: [EfiDiskInfoProtocol] 13A59A4D0
>>>
>>> In turn, in BDS, UEFI boot options are auto-generated for these
>>> devices, which is not nice, given that this procedure in BDS is very
>>> pflash-intensive, and pflash access is remarkably slow on aarch64
>>> KVM.
>>>
>>> For example, if I use one virtio-scsi HBA, and put a CD-ROM on target
>>> 0, LUN 0, and a disk on target 1, LUN 0, then edk2 will create
>>> protocol interfaces, and matching boot options, for
>>>
>>>   2 targets * 7 LUNs/target = 14 LUNs
>>>
>>> of which only 2 make sense.
>>>
>>>
>>> If I revert the patch (on top of v2.10.0-rc3), then everything works
>>> as before -- BlockIo protocol instances are produced only for actual
>>> devices (with media).
>>>
>>> I guess the path forward is to fix the ScsiDiskDxe driver in edk2;
>>> the new ASC should be recognized.
>>>
>>> My question is, how *exactly* did this patch change the reported
>>> sense key and ASC? That is, what did they use to be *before*?
>>> INVALID_OPCODE?
>>
>> I found the bug in edk2. It's a missing error check. I'll send a patch
>> and CC you guys.
> 
> Actually, I think both QEMU and edk2 have bugs in this area.
> 
> The problem surfaces when the DiscoverScsiDevice() function in edk2's
> "MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c" file sends an INQUIRY
> command to a nonexistent LUN (for probing purposes).
> 
> (1) About QEMU:
> 
> (1.1) Without the above patch, QEMU sends the following response:
> 
>> DiscoverScsiDevice:1361: Lun=2 HostAdapterStatus=0 TargetStatus=0
>>                          SenseDataLength=0 InquiryDataLength=36
>> Sense {
>> Sense }
>> Inquiry {
>> Inquiry 000000 7F 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000020 00 00 00 00
>> Inquiry }
> 
> This response *conforms* to the SPC-4, as follows:
> 
>> 6.4.1 INQUIRY command introduction
>>
>> [...]
>>
>> In response to an INQUIRY command received by an incorrect logical
>> unit, the SCSI target device shall return the INQUIRY data with the
>> peripheral qualifier set to the value defined in 6.4.2. The INQUIRY
>> command shall return CHECK CONDITION status only when the device
>> server is unable to return the requested INQUIRY data.
>>
>> [...]
>>
>> 6.4.2 Standard INQUIRY data
>>
>> [...]
>>
>> The PERIPHERAL QUALIFIER field and PERIPHERAL DEVICE TYPE field
>> identify the peripheral device connected to the logical unit. If the
>> SCSI target device is not capable of supporting a peripheral device
>> connected to this logical unit, the device server shall set these
>> fields to 7Fh (i.e., PERIPHERAL QUALIFIER field set to 011b and
>> PERIPHERAL DEVICE TYPE field set to 1Fh).
> 
> (1.2) With the patch, QEMU sends the following response (ignore
> InquiryData here, it's just an artifact of my debug patch):
> 
>> DiscoverScsiDevice:1361: Lun=2 HostAdapterStatus=0 TargetStatus=2
>>                          SenseDataLength=18 InquiryDataLength=96
>> Sense {
>> Sense 000000 70 00 05 00 00 00 00 0A 00 00 00 00 25 00 00 00
>> Sense 000010 00 00
>> Sense }
>> Inquiry {
>> Inquiry 000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry 000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> Inquiry }
> 
> Here "TargetStatus=2" means CHECK CONDITION, and the sense data
> corresponds to "sense_code_LUN_NOT_SUPPORTED", returned by the first
> hunk of the patch.
> 
> According to the SPC-4, this is less preferable, if not outright
> invalid. The spec says (repeating it from above),
> 
>> The INQUIRY command shall return CHECK CONDITION status only when the
>> device server is unable to return the requested INQUIRY data
> 
> with the "requested INQUIRY data" being, in this case,
> 
>> PERIPHERAL QUALIFIER field set to 011b and PERIPHERAL DEVICE TYPE
>> field set to 1Fh
> 
> So in this regard, the patch implements an INQUIRY response that we
> should minimally call "less preferred".
> 
> (2) About edk2:
> 
> The INQUIRY request site in question totally ignores TargetStatus and
> SenseData. (This is arguably a bug; the SPC-4 *does* spell out a case
> when the device server is allowed to respond with CHECK CONDITION.)
> 
> ScsiBusDxe happily produces ScsiIo protocol interfaces for nonexistent
> LUNs in both cases, blindly saving the PERIPHERAL DEVICE TYPE value in
> the ScsiIo protocol instance. The behavior differs only at a higher
> level:
> 
> (2.1) Without the QEMU patch, the device type saved in the ScsiIo
> protocol is 0x1f. This device type means "Unknown or no device type",
> and so none of the SCSI device drivers in edk2 support it! In other
> words, the ScsiIo protocol instances all exist (with an invalid device
> type), but are ignored by other drivers.
> 
> (2.2) With the QEMU patch, the device type saved in the ScsiIo protocol
> is zero, simply because the CHECK CONDITION response does not overwrite
> the pre-zeroed InquiryData array in edk2. Type 0 happens to mean "Direct
> access block device" (that is, disk), and that *is* bound by
> ScsiDiskDxe. Hence the bogus BlockIo protocol instances, which show up
> even in BDS.
> 
> I think I'll try to send an edk2 patch, but I suggest that the QEMU
> patch too be reconsidered or revised.
> 
Hmm. Guess you are right.

Does this help?

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index e364410a23..e9c70101a7 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -517,7 +517,7 @@ static int32_t scsi_target_send_command(SCSIRequest
*req, uint8_t *buf)
 {
     SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);

-    if (req->lun != 0) {
+    if (req->lun != 0 || buf[0] != INQUIRY) {
         scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
         scsi_req_complete(req, CHECK_CONDITION);
         return 0;

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCHv2 4/4] scsi: Add 'enclosure' option for scsi devices

[Hannes Reinecke]

Make enclosure support optional with the 'enclosure' argument to
the scsi device.
Adding 'enclosure=on' as option to the SCSI device will present
an enclosure service device on LUN0, either as a stand-alone
LUN (in case LUN0 is not assigned) or by setting the EncServ bit
int the inquiry data if LUN0 is assigned to a block devices.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 hw/scsi/scsi-bus.c     | 11 ++++++++---
 hw/scsi/scsi-disk.c    |  4 +++-
 include/hw/scsi/scsi.h |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 79a222f..c11422b 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,6 +22,7 @@ static Property scsi_props[] = {
     DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
     DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
     DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
+    DEFINE_PROP_BOOL("enclosure", SCSIDevice, enclosure, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -494,11 +495,14 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
     if (r->req.lun != 0) {
         r->buf[0] = TYPE_NO_LUN;
     } else {
-        r->buf[0] = TYPE_ENCLOSURE;
+        r->buf[0] = r->req.dev->enclosure ?
+            TYPE_ENCLOSURE : TYPE_NOT_PRESENT | TYPE_INACTIVE;
         r->buf[2] = 5; /* Version */
         r->buf[3] = 2 | 0x10; /* HiSup, response data format */
         r->buf[4] = r->len - 5; /* Additional Length = (Len - 1) - 4 */
-        r->buf[6] = 0x40; /* Enclosure service */
+        if (r->req.dev->enclosure) {
+            r->buf[6] = 0x40; /* Enclosure service */
+        }
         r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ.  */
         memcpy(&r->buf[8], "QEMU    ", 8);
         memcpy(&r->buf[16], "QEMU TARGET     ", 16);
@@ -600,7 +604,8 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
         }
         break;
     case RECEIVE_DIAGNOSTIC:
-        if (!scsi_target_emulate_receive_diagnostic(r)) {
+        if (!r->req.dev->enclosure ||
+            !scsi_target_emulate_receive_diagnostic(r)) {
             goto illegal_request;
         }
         break;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5f1e5e8..153d97d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -792,7 +792,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
                the additional length is not adjusted */
         outbuf[4] = 36 - 5;
     }
-
+    if (s->qdev.lun == 0 && s->qdev.enclosure) {
+            outbuf[6] = 0x40; /* Enclosure service */
+    }
     /* Sync data transfer and TCQ.  */
     outbuf[7] = 0x10 | (req->bus->info->tcq ? 0x02 : 0);
     return buflen;
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 6b85786..243c185 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -100,6 +100,7 @@ struct SCSIDevice
     BlockConf conf;
     SCSISense unit_attention;
     bool sense_is_ua;
+    bool enclosure;
     uint8_t sense[SCSI_SENSE_BUF_SIZE];
     uint32_t sense_len;
     QTAILQ_HEAD(, SCSIRequest) requests;
-- 
1.8.5.6

Re: [Qemu-devel] [PATCHv2 4/4] scsi: Add 'enclosure' option for scsi devices

[Paolo Bonzini]

On 04/08/2017 10:36, Hannes Reinecke wrote:
> Make enclosure support optional with the 'enclosure' argument to
> the scsi device.
> Adding 'enclosure=on' as option to the SCSI device will present
> an enclosure service device on LUN0, either as a stand-alone
> LUN (in case LUN0 is not assigned) or by setting the EncServ bit
> int the inquiry data if LUN0 is assigned to a block devices.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  hw/scsi/scsi-bus.c     | 11 ++++++++---
>  hw/scsi/scsi-disk.c    |  4 +++-
>  include/hw/scsi/scsi.h |  1 +
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 79a222f..c11422b 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -22,6 +22,7 @@ static Property scsi_props[] = {
>      DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
>      DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
>      DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
> +    DEFINE_PROP_BOOL("enclosure", SCSIDevice, enclosure, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -494,11 +495,14 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>      if (r->req.lun != 0) {
>          r->buf[0] = TYPE_NO_LUN;
>      } else {
> -        r->buf[0] = TYPE_ENCLOSURE;
> +        r->buf[0] = r->req.dev->enclosure ?
> +            TYPE_ENCLOSURE : TYPE_NOT_PRESENT | TYPE_INACTIVE;
>          r->buf[2] = 5; /* Version */
>          r->buf[3] = 2 | 0x10; /* HiSup, response data format */
>          r->buf[4] = r->len - 5; /* Additional Length = (Len - 1) - 4 */
> -        r->buf[6] = 0x40; /* Enclosure service */
> +        if (r->req.dev->enclosure) {
> +            r->buf[6] = 0x40; /* Enclosure service */
> +        }
>          r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ.  */
>          memcpy(&r->buf[8], "QEMU    ", 8);
>          memcpy(&r->buf[16], "QEMU TARGET     ", 16);
> @@ -600,7 +604,8 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>          }
>          break;
>      case RECEIVE_DIAGNOSTIC:
> -        if (!scsi_target_emulate_receive_diagnostic(r)) {
> +        if (!r->req.dev->enclosure ||
> +            !scsi_target_emulate_receive_diagnostic(r)) {
>              goto illegal_request;
>          }
>          break;
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 5f1e5e8..153d97d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -792,7 +792,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>                 the additional length is not adjusted */
>          outbuf[4] = 36 - 5;
>      }
> -
> +    if (s->qdev.lun == 0 && s->qdev.enclosure) {
> +            outbuf[6] = 0x40; /* Enclosure service */
> +    }

Should this really be set on disks even if you do have a LUN0?

Why don't you instead add a very simple scsi-enclosure device that you
can specify as the LUN0?

Thanks,

Paolo

>      /* Sync data transfer and TCQ.  */
>      outbuf[7] = 0x10 | (req->bus->info->tcq ? 0x02 : 0);
>      return buflen;
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 6b85786..243c185 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -100,6 +100,7 @@ struct SCSIDevice
>      BlockConf conf;
>      SCSISense unit_attention;
>      bool sense_is_ua;
> +    bool enclosure;
>      uint8_t sense[SCSI_SENSE_BUF_SIZE];
>      uint32_t sense_len;
>      QTAILQ_HEAD(, SCSIRequest) requests;
> 

Re: [Qemu-devel] [PATCHv2 4/4] scsi: Add 'enclosure' option for scsi devices

[Hannes Reinecke]

On 08/04/2017 12:48 PM, Paolo Bonzini wrote:
> On 04/08/2017 10:36, Hannes Reinecke wrote:
>> Make enclosure support optional with the 'enclosure' argument to
>> the scsi device.
>> Adding 'enclosure=on' as option to the SCSI device will present
>> an enclosure service device on LUN0, either as a stand-alone
>> LUN (in case LUN0 is not assigned) or by setting the EncServ bit
>> int the inquiry data if LUN0 is assigned to a block devices.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  hw/scsi/scsi-bus.c     | 11 ++++++++---
>>  hw/scsi/scsi-disk.c    |  4 +++-
>>  include/hw/scsi/scsi.h |  1 +
>>  3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>> index 79a222f..c11422b 100644
>> --- a/hw/scsi/scsi-bus.c
>> +++ b/hw/scsi/scsi-bus.c
>> @@ -22,6 +22,7 @@ static Property scsi_props[] = {
>>      DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
>>      DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
>>      DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
>> +    DEFINE_PROP_BOOL("enclosure", SCSIDevice, enclosure, false),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> @@ -494,11 +495,14 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>>      if (r->req.lun != 0) {
>>          r->buf[0] = TYPE_NO_LUN;
>>      } else {
>> -        r->buf[0] = TYPE_ENCLOSURE;
>> +        r->buf[0] = r->req.dev->enclosure ?
>> +            TYPE_ENCLOSURE : TYPE_NOT_PRESENT | TYPE_INACTIVE;
>>          r->buf[2] = 5; /* Version */
>>          r->buf[3] = 2 | 0x10; /* HiSup, response data format */
>>          r->buf[4] = r->len - 5; /* Additional Length = (Len - 1) - 4 */
>> -        r->buf[6] = 0x40; /* Enclosure service */
>> +        if (r->req.dev->enclosure) {
>> +            r->buf[6] = 0x40; /* Enclosure service */
>> +        }
>>          r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ.  */
>>          memcpy(&r->buf[8], "QEMU    ", 8);
>>          memcpy(&r->buf[16], "QEMU TARGET     ", 16);
>> @@ -600,7 +604,8 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>          }
>>          break;
>>      case RECEIVE_DIAGNOSTIC:
>> -        if (!scsi_target_emulate_receive_diagnostic(r)) {
>> +        if (!r->req.dev->enclosure ||
>> +            !scsi_target_emulate_receive_diagnostic(r)) {
>>              goto illegal_request;
>>          }
>>          break;
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 5f1e5e8..153d97d 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -792,7 +792,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>>                 the additional length is not adjusted */
>>          outbuf[4] = 36 - 5;
>>      }
>> -
>> +    if (s->qdev.lun == 0 && s->qdev.enclosure) {
>> +            outbuf[6] = 0x40; /* Enclosure service */
>> +    }
> 
> Should this really be set on disks even if you do have a LUN0?
> 
Why not? You _can_ have embedded enclosure devices; that's what this bit
is for...

> Why don't you instead add a very simple scsi-enclosure device that you
> can specify as the LUN0?
> 
You don't have to.
Once you leave LUN0 free (eg by assigning your first disk LUN1) you'll
get this device by virtue of the current LUN0 emulation.

But yeah, maybe I can be doing a separate enclosure device.
Will be checking if and how I can make it to work.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)