From: Laszlo Ersek Date: Thu, 10 Sep 2015 11:25:09 +0000 (+0000) Subject: MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers X-Git-Tag: edk2-stable201903~8881 X-Git-Url: https://git.proxmox.com/?p=mirror_edk2.git;a=commitdiff_plain;h=5abc2a70da4fa918186421cd8cae3915c92b9ff4 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: Feng Tian Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek Reviewed-by: Feng Tian git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18438 6f19259b-4bc3-4df7-8a09-765794883524 --- diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c index 6da1751a57..282e9c2832 100644 --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c @@ -1789,6 +1789,7 @@ ScsiDiskReadSectors ( UINT32 ByteCount; UINT32 MaxBlock; UINT32 SectorCount; + UINT32 NextSectorCount; UINT64 Timeout; EFI_STATUS Status; UINT8 Index; @@ -1889,6 +1890,25 @@ ScsiDiskReadSectors ( 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)) { @@ -1934,6 +1954,7 @@ ScsiDiskWriteSectors ( UINT32 ByteCount; UINT32 MaxBlock; UINT32 SectorCount; + UINT32 NextSectorCount; UINT64 Timeout; EFI_STATUS Status; UINT8 Index; @@ -2032,6 +2053,26 @@ ScsiDiskWriteSectors ( 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)) {