| Message ID | 1501835795-92331-4-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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:
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(-)