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

1 files changed, 41 insertions, 0 deletions

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)) {