diff options
author | Laszlo Ersek <lersek@redhat.com> | 2015-09-10 11:25:09 +0000 |
---|---|---|
committer | lersek <lersek@Edk2> | 2015-09-10 11:25:09 +0000 |
commit | 5abc2a70da4fa918186421cd8cae3915c92b9ff4 (patch) | |
tree | 6b3f5c18a66758379f72217c2a906b6f449248b5 | |
parent | fc3c83e0b355bffc6452bb0a0cb81b381684c8c4 (diff) | |
download | edk2-5abc2a70da4fa918186421cd8cae3915c92b9ff4.zip edk2-5abc2a70da4fa918186421cd8cae3915c92b9ff4.tar.gz edk2-5abc2a70da4fa918186421cd8cae3915c92b9ff4.tar.bz2 |
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 <feng.tian@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Feng Tian <feng.tian@intel.com>
git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18438 6f19259b-4bc3-4df7-8a09-765794883524
-rw-r--r-- | MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 41 |
1 files changed, 41 insertions, 0 deletions
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c index 6da1751..282e9c2 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)) {
|