From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1diUxg-0001PX-W7 for qemu-devel@nongnu.org; Thu, 17 Aug 2017 20:16:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1diUxd-0002W0-QT for qemu-devel@nongnu.org; Thu, 17 Aug 2017 20:16:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50788) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1diUxd-0002To-GV for qemu-devel@nongnu.org; Thu, 17 Aug 2017 20:16:25 -0400 From: Laszlo Ersek References: <1501835795-92331-1-git-send-email-hare@suse.de> <1501835795-92331-4-git-send-email-hare@suse.de> <347fe244-d544-0bfa-f7a6-0f87ea7a475b@redhat.com> Message-ID: Date: Fri, 18 Aug 2017 02:16:22 +0200 MIME-Version: 1.0 In-Reply-To: <347fe244-d544-0bfa-f7a6-0f87ea7a475b@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Hannes Reinecke Cc: Hannes Reinecke , qemu-devel@nongnu.org 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 >>> --- >>> 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