Hi, Laszlo
1. I don't think the code in VirtioScsi.c is correct. It hardcodes the block
size as 512bytes. The driver should send READ_CAPACITY cmd to get block size,
then convert it to InTransferLength & OutTransferLength.
> //
> // Catch oversized requests eagerly. If this condition evaluates to false,
> // then the combined size of a bidirectional request will not exceed the
> // virtio-scsi device's transfer limit either.
> //
> if (ALIGN_VALUE (Packet->OutTransferLength, 512) / 512
> > Dev->MaxSectors / 2 ||
> ALIGN_VALUE (Packet->InTransferLength, 512) / 512
> > Dev->MaxSectors / 2) {
> Packet->InTransferLength = (Dev->MaxSectors / 2) * 512;
> Packet->OutTransferLength = (Dev->MaxSectors / 2) * 512;
> Packet->HostAdapterStatus =
>
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> Packet->SenseDataLength = 0;
> return EFI_BAD_BUFFER_SIZE;
> }
2. Although the UEFI spec doesn't say the relationship of
InTransferLength/OutTransferLength and CDB.TransferLength, but I think it's
implied that InTransferLength/OutTransferLength = CDB.TransferLength *
BlockSize. Otherwise how the lowest layer host controller driver constructs
transfer descriptor? How many bytes to read? Is
InTransferLength/OutTransferLength or the one of CDB.TransferLength * BlockSize?
3. If we don't think #2 is implied, why we can assume "if pass thru driver
returns EFI_BAD_BUFFER_SIZE and leave HostAdapterStatus at
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK, but *then* it must set either TargetStatus
to some error code (different from EFI_EXT_SCSI_STATUS_TARGET_GOOD), *or* it
must report some problem in the sense data."? IMHO, a pass thru driver could
only return EFI_BAD_BUFFER_SIZE and leave HostAdpterStatus/TargetStatus to ok
and no sense data.
Thanks
Feng
-----Original Message-----
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Monday, September 07, 2015 17:46
To: Tian, Feng
Cc: edk2-de...@ml01.01.org
Subject: Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when
shortening transfers
On 09/06/15 08:09, Tian, Feng wrote:
> 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.
Yes, the block count of 65535 is the same in this case. However, the "read too
large" issue (marked as (2) in my commit message) is not returned by the
virtio-scsi controller provided by QEMU. It is caught by the virtio-scsi driver
in OVMF.
The reason for that is that during virtio-scsi bringup, the driver in OvmfPkg
and the controller in QEMU negotiate the maximum transfer size up-front. See
the VirtioScsiInit() function:
> Status = VIRTIO_CFG_READ (Dev, MaxSectors, &Dev->MaxSectors);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> if (Dev->MaxSectors < 2) {
> //
> // We must be able to halve it for bidirectional transfers
> // (see EFI_BAD_BUFFER_SIZE in PopulateRequest()).
> //
> Status = EFI_UNSUPPORTED;
> goto Failed;
> }
(Note that "sectors" is meant in 512 byte sectors here.)
Then, when a request that is too large arrives from higher layers of the driver
stack, the virtio-scsi driver can reject that immediately, in the
PopulateRequest() function:
> //
> // Catch oversized requests eagerly. If this condition evaluates to false,
> // then the combined size of a bidirectional request will not exceed the
> // virtio-scsi device's transfer limit either.
> //
> if (ALIGN_VALUE (Packet->OutTransferLength, 512) / 512
> > Dev->MaxSectors / 2 ||
> ALIGN_VALUE (Packet->InTransferLength, 512) / 512
> > Dev->MaxSectors / 2) {
> Packet->InTransferLength = (Dev->MaxSectors / 2) * 512;
> Packet->OutTransferLength = (Dev->MaxSectors / 2) * 512;
> Packet->HostAdapterStatus =
>
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> Packet->SenseDataLength = 0;
> return EFI_BAD_BUFFER_SIZE;
> }
As you can see, there is no sense data in this case at all.
However, the lack of sense data conforms to the UEFI spec. If you check the
specification of the EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
function, it says:
> InTransferLength On Input, the size, in bytes, of InDataBuffer. On
> output, the number of bytes transferred between
> the SCSI controller and the SCSI device. If
> InTransferLength is larger than the SCSI
> controller can handle, no data will be
> transferred, InTransferLength will be updated to
> contain the number of bytes that the SCSI
> controller is able to transfer, and
> EFI_BAD_BUFFER_SIZE will be returned.
>
> [...]
>
> If the data buffer described by InDataBuffer and InTransferLength is
> too big to be transferred in a single command, then no data is
> transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes
> that can be transferred in a single command are returned in
> InTransferLength.
>
> [...]
>
> If EFI_SUCCESS, EFI_BAD_BUFFER_SIZE, EFI_DEVICE_ERROR, or EFI_TIMEOUT
> is returned, then the caller must examine the status fields in Packet
> in the following precedence order: HostAdapterStatus followed by
> TargetStatus followed by SenseDataLength, followed by SenseData.
Note the words "in the following precedence order".
> [...]
>
> EFI_BAD_BUFFER_SIZE The SCSI Request Packet was not executed. The
> number of bytes that could be transferred is
> returned in InTransferLength. For write and
> bi-directional commands, OutTransferLength
> bytes were transferred by OutDataBuffer. See
> HostAdapterStatus, TargetStatus, and in that
> order for additional status information.
Note the words "in that order".
This means that the caller of PassThru() first has to check the return status
of PassThru() first -- obviously --, and HostAdapterStatus second.
VirtioScsiDxe is compliant with the spec, because it sets HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, when it returns
EFI_BAD_BUFFER_SIZE, therefore the presence or absence of sense data makes no
difference. (This is not a SCSI device level error but a Host Bus Adapter-level
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.
Yep.
> 3) Combo/BlueRay DVDs we tested are all ok to send 65535 sectors each
> time.
Okay.
> 4) DVD-Only CDRoms we tested all have problem to send 65535 sectors
> each time and cause win8 32 bit installation failure.
In OVMF's / QEMU's case, the error conditions are different. It is not the SCSI
CD-ROM (device) that rejects the read that is too long, it is the host bus
adapter. (More precisely, it is the virtio-scsi driver on behalf of the HBA,
based on pre-negotiated details.)
However, the values returned by the virtio-scsi driver conform to the UEFI spec.
> That's why r15491 was introduced. The fix depends on real h/w
> response, that is sense data, to do action.
I claim that this dependency on the sense data is not correct; as I wrote
above, the HBA driver is not required to return any sense data if it returns
EFI_BAD_BUFFER_SIZE *and* sets HostAdapterStatus.
The ScsiDiskRead10() function (in
"MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c") does the following:
> ReturnStatus = ScsiRead10Command (
> ScsiDiskDevice->ScsiIo,
> Timeout,
> ScsiDiskDevice->SenseData,
> &SenseDataLength,
> &HostAdapterStatus,
> &TargetStatus,
> DataBuffer,
> DataLength,
> StartLba,
> SectorCount
> );
>
> if (ReturnStatus == EFI_NOT_READY) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
This branch doesn't catch EFI_BAD_BUFFER_SIZE.
> } else if ((ReturnStatus == EFI_INVALID_PARAMETER) || (ReturnStatus ==
> EFI_UNSUPPORTED)) {
> *NeedRetry = FALSE;
> return ReturnStatus;
Neither does this branch.
> }
>
> //
> // go ahead to check HostAdapterStatus and TargetStatus
> // (EFI_TIMEOUT, EFI_DEVICE_ERROR, EFI_WARN_BUFFER_TOO_SMALL)
> //
> Status = CheckHostAdapterStatus (HostAdapterStatus);
And here HostAdapterStatus is being checked without actual knowledge of
ReturnStatus (ie. EFI_BAD_BUFFER_SIZE).
In the case at hand, the
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN host adapter status is
mapped to EFI_NOT_READY by CheckHostAdapterStatus().
> if ((Status == EFI_TIMEOUT) || (Status == EFI_NOT_READY)) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
For which reason this branch is taken, and the ScsiDiskRead10() function
returns EFI_DEVICE_ERROR.
> 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)
Correct!
> 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,
I disagree, in this specific case. I do not contest that there is no
*generic* specification about combining the return status and the host status,
but it is specified that the caller of PassThru() must first consult
HostAdapterStatus if EFI_BAD_BUFFER_SIZE is the return status, regardless of
sense data.
> 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.
I don't understand the word "whether" here.
In any case, an EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() function is allowed
to return EFI_BAD_BUFFER_SIZE and leave HostAdapterStatus at
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK, but *then* it must set either TargetStatus
to some error code (different from EFI_EXT_SCSI_STATUS_TARGET_GOOD), *or* it
must report some problem in the sense data.
In any case, this doesn't apply to VirtioScsiDxe, beause it does set
HostAdapterStatus to a code that is different from
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK. And that fact takes priority over both
TargetStatus and the (lack of) sense data.
> If there is such case, the "Retry" variable may be FALSE rather TRUE
> and causes the new SectorCount adjustment logic bypass.
You are right insofar as ScsiDiskRead10() indeed returns even before it reaches
the "backoff" algorithm.
However, the NeedRetry variable *is* set to TRUE. (See above which branch is
taken.)
> So could we update the patch as attached?
I can see three problems with the patch. Please see my comments below:
> ScsiDisk.patch
>
>
> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 31
> ++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> index e7abe54..7a8379a 100644
> --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> @@ -1889,6 +1889,17 @@ ScsiDiskReadSectors (
> return EFI_DEVICE_ERROR;
> }
>
> + //
> + // Retry here with new SectorCount value as the callee
> + // may lower down ByteCount due to EFI_BAD_BUFFER_SIZE
> + //
> + if ((ByteCount % BlockSize) != 0) {
> + return EFI_DEVICE_ERROR;
> + }
Problem #1: this is an incorrect assumption. I don't see any requirement that
the Host Bus Adapter's maximum transfer size be a whole multiple of a specific
device's media's block size.
And, this check would actually fail in my use case. If you double-check remark
(2) in my patch, you can see:
> (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
InTransferLength is not *exactly* 16 MB on return. (In remark (4) of my patch,
I wrote "approx. 16MB".)
It is 15 megabytes, 1023 kilobytes, and 512 bytes. In other words, it is (16MB
- 512B).
This host bus adapter limit is not an integral multiple of the 2KB CD-ROM
blocksize.
And it doesn't have to be. So that's the first problem I see with your patch.
Back to it:
> +
> + if (SectorCount != ByteCount / BlockSize) {
> + SectorCount = ByteCount / BlockSize;
> + }
> }
>
> if ((Index == MaxRetry) && (Status != EFI_SUCCESS)) { @@ -2145,6
> +2156,11 @@ BackOff:
> return EFI_DEVICE_ERROR;
> }
>
> + if (ReturnStatus == EFI_BAD_BUFFER_SIZE) {
> + *NeedRetry = TRUE;
> + return ReturnStatus;
> + }
> +
Problem #2: this check occurs too late.
As I explained above, CheckHostAdapterStatus() maps
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN to EFI_NOT_READY, which
causes ScsiDiskRead10() to return EFI_DEVICE_ERROR before this hunk would be
reached.
Problem #3: ScsiDiskWriteSectors() is not updated in your patch. The same issue
is possible for writing: assume that a UEFI application (or a higher level
driver) tries to write 256 MB in a single operation, and that the underlying
disk is a SCSI disk, on a virtio-scsi controller.
VirtioScsiDxe will reject the write similarly with EFI_BAD_BUFFER_SIZE.
The ScsiWrite10Command() function in UefiScsiLib will correctly propagate
OutTransferLength to DataLength (ie. "ByteCount" in ScsiDiskWriteSectors()).
Hence ScsiDiskWriteSectors() ought to adjust the sector count too.
My patch doesn't have problems #1 and #3, so I'd like to stick with it.
However, I agree that my patch too should address problem #2:
EFI_BAD_BUFFER_SIZE should be checked early (before looking at
"HostAdapterStatus" at all.) Therefore I should *prepend* another patch to the
one I already posted, with contents like:
> diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> index 8070ccc..282e9c2 100644
> --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c
> @@ -2148,7 +2148,7 @@ BackOff:
> SectorCount
> );
>
> - if (ReturnStatus == EFI_NOT_READY) {
> + if (ReturnStatus == EFI_NOT_READY || ReturnStatus ==
> + EFI_BAD_BUFFER_SIZE) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
> } else if ((ReturnStatus == EFI_INVALID_PARAMETER) || (ReturnStatus
> == EFI_UNSUPPORTED)) { @@ -2272,7 +2272,7 @@ BackOff:
> StartLba,
> SectorCount
> );
> - if (ReturnStatus == EFI_NOT_READY) {
> + if (ReturnStatus == EFI_NOT_READY || ReturnStatus ==
> + EFI_BAD_BUFFER_SIZE) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
> } else if ((ReturnStatus == EFI_INVALID_PARAMETER) || (ReturnStatus
> == EFI_UNSUPPORTED)) { @@ -2395,7 +2395,7 @@ BackOff:
> StartLba,
> SectorCount
> );
> - if (ReturnStatus == EFI_NOT_READY) {
> + if (ReturnStatus == EFI_NOT_READY || ReturnStatus ==
> + EFI_BAD_BUFFER_SIZE) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
> } else if ((ReturnStatus == EFI_INVALID_PARAMETER) || (ReturnStatus
> == EFI_UNSUPPORTED)) { @@ -2519,7 +2519,7 @@ BackOff:
> StartLba,
> SectorCount
> );
> - if (ReturnStatus == EFI_NOT_READY) {
> + if (ReturnStatus == EFI_NOT_READY || ReturnStatus ==
> + EFI_BAD_BUFFER_SIZE) {
> *NeedRetry = TRUE;
> return EFI_DEVICE_ERROR;
> } else if ((ReturnStatus == EFI_INVALID_PARAMETER) || (ReturnStatus
> == EFI_UNSUPPORTED)) {
This patch (as first patch) would explicitly recognize EFI_BAD_BUFFER_SIZE, and
force a retry without depending on the details; and the second patch (ie. the
patch I already posted) would adjust the sector count (for both reads and
writes, and also rounding down as necessary).
What do you think?
Thanks!
Laszlo
> if ((TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) ||
> (EFI_ERROR (ReturnStatus))) {
> DEBUG ((EFI_D_ERROR, "ScsiDiskRead10: Check Condition happened!\n"));
> Status = DetectMediaParsingSenseKeys (ScsiDiskDevice,
> ScsiDiskDevice->SenseData, SenseDataLength / sizeof (EFI_SCSI_SENSE_DATA),
> &Action); @@ -2269,6 +2285,11 @@ BackOff:
> return EFI_DEVICE_ERROR;
> }
>
> + if (ReturnStatus == EFI_BAD_BUFFER_SIZE) {
> + *NeedRetry = TRUE;
> + return ReturnStatus;
> + }
> +
> if ((TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) ||
> (EFI_ERROR (ReturnStatus))) {
> DEBUG ((EFI_D_ERROR, "ScsiDiskWrite10: Check Condition happened!\n"));
> Status = DetectMediaParsingSenseKeys (ScsiDiskDevice,
> ScsiDiskDevice->SenseData, SenseDataLength / sizeof (EFI_SCSI_SENSE_DATA),
> &Action); @@ -2392,6 +2413,11 @@ BackOff:
> return EFI_DEVICE_ERROR;
> }
>
> + if (ReturnStatus == EFI_BAD_BUFFER_SIZE) {
> + *NeedRetry = TRUE;
> + return ReturnStatus;
> + }
> +
> if ((TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) ||
> (EFI_ERROR (ReturnStatus))) {
> DEBUG ((EFI_D_ERROR, "ScsiDiskRead16: Check Condition happened!\n"));
> Status = DetectMediaParsingSenseKeys (ScsiDiskDevice,
> ScsiDiskDevice->SenseData, SenseDataLength / sizeof (EFI_SCSI_SENSE_DATA),
> &Action); @@ -2516,6 +2542,11 @@ BackOff:
> return EFI_DEVICE_ERROR;
> }
>
> + if (ReturnStatus == EFI_BAD_BUFFER_SIZE) {
> + *NeedRetry = TRUE;
> + return ReturnStatus;
> + }
> +
> if ((TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) ||
> (EFI_ERROR (ReturnStatus))) {
> DEBUG ((EFI_D_ERROR, "ScsiDiskWrite16: Check Condition happened!\n"));
> Status = DetectMediaParsingSenseKeys (ScsiDiskDevice,
> ScsiDiskDevice->SenseData, SenseDataLength / sizeof
> (EFI_SCSI_SENSE_DATA), &Action);
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
