Hi, Laszlo Thanks for root cause this issue.
We ever met the same issue when installing windows 8 32bit through cdrom and we found: 1) Why 32bit UEFI win8 OS installation fails with some DVD-Only devices is because OS would invoke a single read operation to read 65535 sector one time. The CDRoms couldn't handle this request and the sense data shows here is HARDWARE_ERROR. 2) Why 64bit UEFI win8 OS installation could succeed with Combo/Blue-ray/DVD-only devices is because the maximum transfer sector number in a single read operation is less than or equal to 512 sector. 3) Combo/BlueRay DVDs we tested are all ok to send 65535 sectors each time. 4) DVD-Only CDRoms we tested all have problem to send 65535 sectors each time and cause win8 32 bit installation failure. That's why r15491 was introduced. The fix depends on real h/w response, that is sense data, to do action. In your findings, it looks like the ScsiDisk driver forgets to handle EFI_BAD_BUFFER_SIZE and EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN status (point (2) in the your commit log) and it causes SectorCount and ByteCount mismatch and brings this BSOD. As the UEFI spec doesn't clearly speak how to combine the return status and host status, whether is it possible for 3rd scsi pass thru driver to only return EFI_BAD_BUFFER_SIZE and not set the host status like VirtioScsi driver. If there is such case, the "Retry" variable may be FALSE rather TRUE and causes the new SectorCount adjustment logic bypass. So could we update the patch as attached? Thanks Feng -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Saturday, September 05, 2015 02:22 To: edk2-de...@ml01.01.org Cc: Tian, Feng Subject: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers The specification of the EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() function documents the EFI_BAD_BUFFER_SIZE return status, and the EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN host adapter status. These allow an EFI_EXT_SCSI_PASS_THRU_PROTOCOL implementation to request higher layers in the stack (in this instance, UefiScsiLib and ScsiDiskDxe) to break up the transfer into smaller pieces. These conditions percolate up the stack correctly: the retry loops in ScsiDiskDxe's ScsiDiskReadSectors() and ScsiDiskWriteSectors() functions correctly and transparently update the transfer size (ByteCount), accommodating any shortening requested by lower levels of the stack. After the loop -- if the request ultimately succeeds -- SectorCount is even recalculated from the final ByteCount, to see how many sectors the outer loop should advance. However, the inner (ie. retry) loops both have the same error: when the underlying protocols request the transfer to be shortened, the decrease in transfer size (ie. ByteCount) should immediately be reflected in SectorCount. Otherwise the sector count encoded in the CDB will exceed the transfer size, which is a permanent error. This issue has been witnessed while booting en_windows_8.1_pro_n_vl_with_update_x86_dvd_6051127.iso on the 32-bit build of OVMF, from a virtio-scsi CD-ROM: (1) "cdboot.efi" correctly requested (from far atop) a long read: Timeout=940000000 CdbLength=10 DataDir=Read InTransferLength=134215680 OutTransferLength=0 SenseDataLength=108 Cdb: 28 00 00 00 25 DD 00 FF FF 00 ^ ^^^^^^^^^^^ ^^^^^ | | | | | number of 2KB sectors to read, | | corresponding to 2048 * 65535 = 134215680 bytes | | (see InTransferLength above) | | | LBA to read from | READ (10) (2) In turn, the EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() function provided by "OvmfPkg/VirtioScsiDxe/VirtioScsi.c" asked for the request to be shortened: InTransferLength=16776704 OutTransferLength=16776704 SenseDataLength=0 HostAdapterStatus=EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN TargetStatus=0 Status=EFI_BAD_BUFFER_SIZE (3) Then ScsiDiskReadSectors() in "MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c" retried the request with correctly shortened transfer length, but incorrectly unchanged sector count: Timeout=940000000 CdbLength=10 DataDir=Read InTransferLength=16776704 <--- updated as requested OutTransferLength=0 SenseDataLength=108 Cdb: 28 00 00 00 25 DD 00 FF FF 00 ^ ^^^^^^^^^^^ ^^^^^ | | | | | not changed! | | | LBA to read from | READ (10) (4) Since 65535 sectors of 2KB each wouldn't fit in a buffer of approx. 16MB, QEMU's virtio-scsi controller unconditionally rejected this request with VIRTIO_SCSI_S_OVERRUN, which VirtioScsiDxe then mapped to: InTransferLength=16776704 OutTransferLength=0 SenseDataLength=0 HostAdapterStatus=EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN TargetStatus=0 Status=EFI_DEVICE_ERROR (5) After two more tries of the same, ScsiDiskDxe passed up the error, which ultimately caused "cdboot.efi" to BSOD. Many thanks to Larry Cleeton from Microsoft for helping debug "cdboot.efi". Cc: Larry Cleeton <larry.clee...@microsoft.com> Cc: Feng Tian <feng.t...@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- Notes: For ease of review, this patch has been formatted with the "--function-context" git option. Although including extra context can lead to unnecessary conflicts during patch application, I expect zero churn in this file in the meantime, so that aspect of "--function-context" should not be an issue. (In any case I intend to commit the patch myself, if Feng approves the patch.) MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 41 ++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c index e7abe54..8070ccc 100644 --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c @@ -1779,144 +1779,164 @@ EFI_STATUS ScsiDiskReadSectors ( IN SCSI_DISK_DEV *ScsiDiskDevice, OUT VOID *Buffer, IN EFI_LBA Lba, IN UINTN NumberOfBlocks ) { UINTN BlocksRemaining; UINT8 *PtrBuffer; UINT32 BlockSize; UINT32 ByteCount; UINT32 MaxBlock; UINT32 SectorCount; + UINT32 NextSectorCount; UINT64 Timeout; EFI_STATUS Status; UINT8 Index; UINT8 MaxRetry; BOOLEAN NeedRetry; Status = EFI_SUCCESS; BlocksRemaining = NumberOfBlocks; BlockSize = ScsiDiskDevice->BlkIo.Media->BlockSize; // // limit the data bytes that can be transferred by one Read(10) or Read(16) Command // if (!ScsiDiskDevice->Cdb16Byte) { MaxBlock = 0xFFFF; } else { MaxBlock = 0xFFFFFFFF; } PtrBuffer = Buffer; while (BlocksRemaining > 0) { if (BlocksRemaining <= MaxBlock) { if (!ScsiDiskDevice->Cdb16Byte) { SectorCount = (UINT16) BlocksRemaining; } else { SectorCount = (UINT32) BlocksRemaining; } } else { SectorCount = MaxBlock; } ByteCount = SectorCount * BlockSize; // // |------------------------|-----------------|------------------|-----------------| // | ATA Transfer Mode | Transfer Rate | SCSI Interface | Transfer Rate | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 0 | 3.3Mbytes/sec | SCSI-1 | 5Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 1 | 5.2Mbytes/sec | Fast SCSI | 10Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 2 | 8.3Mbytes/sec | Fast-Wide SCSI | 20Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 3 | 11.1Mbytes/sec | Ultra SCSI | 20Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 4 | 16.6Mbytes/sec | Ultra Wide SCSI | 40Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 0 | 2.1Mbytes/sec | Ultra2 SCSI | 40Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 1 | 4.2Mbytes/sec | Ultra2 Wide SCSI | 80Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 2 | 8.4Mbytes/sec | Ultra3 SCSI | 160Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Multi-word DMA Mode 0 | 4.2Mbytes/sec | Ultra-320 SCSI | 320Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Multi-word DMA Mode 1 | 13.3Mbytes/sec | Ultra-640 SCSI | 640Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // // As ScsiDisk and ScsiBus driver are used to manage SCSI or ATAPI devices, we have to use // the lowest transfer rate to calculate the possible maximum timeout value for each operation. // From the above table, we could know 2.1Mbytes per second is lowest one. // The timout value is rounded up to nearest integar and here an additional 30s is added // to follow ATA spec in which it mentioned that the device may take up to 30s to respond // commands in the Standby/Idle mode. // Timeout = EFI_TIMER_PERIOD_SECONDS (ByteCount / 2100000 + 31); MaxRetry = 2; for (Index = 0; Index < MaxRetry; Index++) { if (!ScsiDiskDevice->Cdb16Byte) { Status = ScsiDiskRead10 ( ScsiDiskDevice, &NeedRetry, Timeout, PtrBuffer, &ByteCount, (UINT32) Lba, SectorCount ); } else { Status = ScsiDiskRead16 ( ScsiDiskDevice, &NeedRetry, Timeout, PtrBuffer, &ByteCount, Lba, SectorCount ); } if (!EFI_ERROR (Status)) { break; } if (!NeedRetry) { return EFI_DEVICE_ERROR; } + // + // We need to retry. However, if ScsiDiskRead10() or ScsiDiskRead16() has + // lowered ByteCount on output, we must make sure that we lower + // SectorCount accordingly. SectorCount will be encoded in the CDB, and + // it is invalid to request more sectors in the CDB than the entire + // transfer (ie. ByteCount) can carry. + // + // In addition, ByteCount is only expected to go down, or stay unchaged. + // Therefore we don't need to update Timeout: the original timeout should + // accommodate shorter transfers too. + // + NextSectorCount = ByteCount / BlockSize; + if (NextSectorCount < SectorCount) { + SectorCount = NextSectorCount; + // + // Account for any rounding down. + // + ByteCount = SectorCount * BlockSize; + } } if ((Index == MaxRetry) && (Status != EFI_SUCCESS)) { return EFI_DEVICE_ERROR; } // // actual transferred sectors // SectorCount = ByteCount / BlockSize; Lba += SectorCount; PtrBuffer = PtrBuffer + SectorCount * BlockSize; BlocksRemaining -= SectorCount; } return EFI_SUCCESS; } /** Write sector to SCSI Disk. @param ScsiDiskDevice The pointer of SCSI_DISK_DEV @param Buffer The buffer of data to be written into SCSI Disk @param Lba Logic block address @param NumberOfBlocks The number of blocks to read @retval EFI_DEVICE_ERROR Indicates a device error. @retval EFI_SUCCESS Operation is successful. **/ @@ -1924,143 +1944,164 @@ EFI_STATUS ScsiDiskWriteSectors ( IN SCSI_DISK_DEV *ScsiDiskDevice, IN VOID *Buffer, IN EFI_LBA Lba, IN UINTN NumberOfBlocks ) { UINTN BlocksRemaining; UINT8 *PtrBuffer; UINT32 BlockSize; UINT32 ByteCount; UINT32 MaxBlock; UINT32 SectorCount; + UINT32 NextSectorCount; UINT64 Timeout; EFI_STATUS Status; UINT8 Index; UINT8 MaxRetry; BOOLEAN NeedRetry; Status = EFI_SUCCESS; BlocksRemaining = NumberOfBlocks; BlockSize = ScsiDiskDevice->BlkIo.Media->BlockSize; // // limit the data bytes that can be transferred by one Read(10) or Read(16) Command // if (!ScsiDiskDevice->Cdb16Byte) { MaxBlock = 0xFFFF; } else { MaxBlock = 0xFFFFFFFF; } PtrBuffer = Buffer; while (BlocksRemaining > 0) { if (BlocksRemaining <= MaxBlock) { if (!ScsiDiskDevice->Cdb16Byte) { SectorCount = (UINT16) BlocksRemaining; } else { SectorCount = (UINT32) BlocksRemaining; } } else { SectorCount = MaxBlock; } ByteCount = SectorCount * BlockSize; // // |------------------------|-----------------|------------------|-----------------| // | ATA Transfer Mode | Transfer Rate | SCSI Interface | Transfer Rate | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 0 | 3.3Mbytes/sec | SCSI-1 | 5Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 1 | 5.2Mbytes/sec | Fast SCSI | 10Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 2 | 8.3Mbytes/sec | Fast-Wide SCSI | 20Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 3 | 11.1Mbytes/sec | Ultra SCSI | 20Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | PIO Mode 4 | 16.6Mbytes/sec | Ultra Wide SCSI | 40Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 0 | 2.1Mbytes/sec | Ultra2 SCSI | 40Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 1 | 4.2Mbytes/sec | Ultra2 Wide SCSI | 80Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Single-word DMA Mode 2 | 8.4Mbytes/sec | Ultra3 SCSI | 160Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Multi-word DMA Mode 0 | 4.2Mbytes/sec | Ultra-320 SCSI | 320Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // | Multi-word DMA Mode 1 | 13.3Mbytes/sec | Ultra-640 SCSI | 640Mbytes/sec | // |------------------------|-----------------|------------------|-----------------| // // As ScsiDisk and ScsiBus driver are used to manage SCSI or ATAPI devices, we have to use // the lowest transfer rate to calculate the possible maximum timeout value for each operation. // From the above table, we could know 2.1Mbytes per second is lowest one. // The timout value is rounded up to nearest integar and here an additional 30s is added // to follow ATA spec in which it mentioned that the device may take up to 30s to respond // commands in the Standby/Idle mode. // Timeout = EFI_TIMER_PERIOD_SECONDS (ByteCount / 2100000 + 31); MaxRetry = 2; for (Index = 0; Index < MaxRetry; Index++) { if (!ScsiDiskDevice->Cdb16Byte) { Status = ScsiDiskWrite10 ( ScsiDiskDevice, &NeedRetry, Timeout, PtrBuffer, &ByteCount, (UINT32) Lba, SectorCount ); } else { Status = ScsiDiskWrite16 ( ScsiDiskDevice, &NeedRetry, Timeout, PtrBuffer, &ByteCount, Lba, SectorCount ); } if (!EFI_ERROR (Status)) { break; } if (!NeedRetry) { return EFI_DEVICE_ERROR; } + + // + // We need to retry. However, if ScsiDiskWrite10() or ScsiDiskWrite16() + // has lowered ByteCount on output, we must make sure that we lower + // SectorCount accordingly. SectorCount will be encoded in the CDB, and + // it is invalid to request more sectors in the CDB than the entire + // transfer (ie. ByteCount) can carry. + // + // In addition, ByteCount is only expected to go down, or stay unchaged. + // Therefore we don't need to update Timeout: the original timeout should + // accommodate shorter transfers too. + // + NextSectorCount = ByteCount / BlockSize; + if (NextSectorCount < SectorCount) { + SectorCount = NextSectorCount; + // + // Account for any rounding down. + // + ByteCount = SectorCount * BlockSize; + } } if ((Index == MaxRetry) && (Status != EFI_SUCCESS)) { return EFI_DEVICE_ERROR; } // // actual transferred sectors // SectorCount = ByteCount / BlockSize; Lba += SectorCount; PtrBuffer = PtrBuffer + SectorCount * BlockSize; BlocksRemaining -= SectorCount; } return EFI_SUCCESS; } /** Submit Read(10) command. @param ScsiDiskDevice The pointer of ScsiDiskDevice @param NeedRetry The pointer of flag indicates if needs retry if error happens @param Timeout The time to complete the command @param DataBuffer The buffer to fill with the read out data @param DataLength The length of buffer @param StartLba The start logic block address @param SectorCount The number of blocks to read @return EFI_STATUS is returned by calling ScsiRead10Command(). **/ -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
