Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers
I agreed ScsiDisk misses the handling for EFI_BAD_BUFFER_SIZE. That's why I proposed a patch although it's not good:-) The uncertain part from my side at previous discussion is about the value of In/OutTransferLength when EFI_BAD_BUFFER_SIZE is returned. In UEFI spec, there are two sentences to explain EFI_BAD_BUFFER_SIZE: 1) 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. 2) 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. I used to only refer to #2 and thought it means the target's capability rather than host. So I thought for read/write cmd the In/OutTransferLength == SectorCount * BlockSize. But now it looks Paolo's explanation makes more sense. So I agree with Laszlo's 2nd patch. Reviewed-by: Feng Tian <feng.t...@intel.com> Thanks Feng -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Tuesday, September 08, 2015 18:23 To: Paolo Bonzini; Tian, Feng Cc: edk2-de...@ml01.01.org Subject: Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers On 09/08/15 10:07, Paolo Bonzini wrote: > > > On 08/09/2015 09:02, Tian, Feng wrote: >> 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. > > This is the controller's maximum transfer size, not the LUN. The > LUN's limits are available from VPD and depend on the block size. The > controller's limit apply to all LUNs and to all commands, even those > that transfer bytes rather than blocks. > > For historical reasons the controller's maximum transfer size is > provided in 512-byte units. In fact, Laszlo raised this point to the > virtio committee just to be sure that his code is correct, which it is. Thanks. >> 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? > > It's always InTransferLength/OutTransferLength. If it is < or > > CDB.TransferLength * BlockSize you get respectively an underrun or an > overrun. An underrun is fatal, an overrun is not. > > InTransferLength/OutTransferLength exist because the controller may > not know how to infer the transfer length for all CDB opcodes (some > express it in bytes, some express it in blocks, some are vendor > specific, some are just crazy and you have to inspect multiple CDB > fields to compute the transfer length). > > Of course ScsiDisk *does* know how to infer the transfer length for > the CDBs it builds. Therefore, it makes sense for ScsiDisk's own READ > and WRITE CDBs to always have InTransferLength/OutTransferLength = > CDB.TransferLength * BlockSize. However, the controller driver must > not rely on this. I'd like to add something here (which might be simply rephrasing what you said): I think Feng's question is about my former statement, which goes like: In case the host controller (ie. not the LUN) is unable to satisfy the request, due to its size (InTransferLength/OutTransferLength) being too large, then it is allowed to return any kind of shorter InTransferLength/OutTransferLength, and the returned max transfer size *need not* be an integral multiple of any kind of block size. Ad absurdum, it could be a prime number too. Feng's patch allowed the controller to return a lower transfer length, but it also expected said lower transfer length to be a multiple of "the" block size. But this is incorrect, because at the host controller level, the concept of "block size" doesn't exist at all! Some LUN hanging off the same host controller could have a block size of 2KB, another 512B, yet another 4KB. So, going beyond the statement "the controller driver must not rely on this", I state that the controller driver doesn't even *know* about any block size! The condition that the host controller's maximum transfer size is exceeded is detected *much earlier* t
Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers
On 09/09/15 09:11, Tian, Feng wrote: > I agreed ScsiDisk misses the handling for EFI_BAD_BUFFER_SIZE. That's why I > proposed a patch although it's not good:-) > > The uncertain part from my side at previous discussion is about the value of > In/OutTransferLength when EFI_BAD_BUFFER_SIZE is returned. > > In UEFI spec, there are two sentences to explain EFI_BAD_BUFFER_SIZE: > > 1) 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. > > 2) 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. > > I used to only refer to #2 and thought it means the target's capability > rather than host. So I thought for read/write cmd the In/OutTransferLength == > SectorCount * BlockSize. > > But now it looks Paolo's explanation makes more sense. So I agree with > Laszlo's 2nd patch. > > Reviewed-by: Feng Tian <feng.t...@intel.com> Thank you. I'll now repost the full series (two patches), because I'd like to make sure that you agree with the commit message I wrote for the separate EFI_BAD_BUFFER_SIZE handling patch, and also just to be sure that your Reviewed-by covers both patches. Therefore I will not add your R-b from above just yet to those two patches -- can you please respond separately with your R-b to the series I'm about to post? There won't be any changes in that series relative to what you've seen here. (Sorry about the churn, but I think this is safer than me going ahead and committing it now, and then realizing you meant something else, and having to revert / update the patches.) Thank you! Laszlo > Thanks > Feng > > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Tuesday, September 08, 2015 18:23 > To: Paolo Bonzini; Tian, Feng > Cc: edk2-de...@ml01.01.org > Subject: Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when > shortening transfers > > On 09/08/15 10:07, Paolo Bonzini wrote: >> >> >> On 08/09/2015 09:02, Tian, Feng wrote: >>> 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. >> >> This is the controller's maximum transfer size, not the LUN. The >> LUN's limits are available from VPD and depend on the block size. The >> controller's limit apply to all LUNs and to all commands, even those >> that transfer bytes rather than blocks. >> >> For historical reasons the controller's maximum transfer size is >> provided in 512-byte units. In fact, Laszlo raised this point to the >> virtio committee just to be sure that his code is correct, which it is. > > Thanks. > >>> 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? >> >> It's always InTransferLength/OutTransferLength. If it is < or > >> CDB.TransferLength * BlockSize you get respectively an underrun or an >> overrun. An underrun is fatal, an overrun is not. >> >> InTransferLength/OutTransferLength exist because the controller may >> not know how to infer the transfer length for all CDB opcodes (some >> express it in bytes, some express it in blocks, some are vendor >> specific, some are just crazy and you have to inspect multiple CDB >> fields to compute the transfer length). >> >> Of course ScsiDisk *does* know how to infer the transfer length for >> the CDBs it builds. Therefore, it makes sense for ScsiDisk's own READ >> and WRITE CDBs to always have InTransferLength/OutTransferLength = >> CDB.TransferLength * BlockSize. However, the controller driver must >> not rely on this. > > I'd like to add something here (which might be simply rephrasing what you > said): > > I think Feng's quest
Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers
On 08/09/2015 09:02, Tian, Feng wrote: > 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. This is the controller's maximum transfer size, not the LUN. The LUN's limits are available from VPD and depend on the block size. The controller's limit apply to all LUNs and to all commands, even those that transfer bytes rather than blocks. For historical reasons the controller's maximum transfer size is provided in 512-byte units. In fact, Laszlo raised this point to the virtio committee just to be sure that his code is correct, which it is. > 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? It's always InTransferLength/OutTransferLength. If it is < or > CDB.TransferLength * BlockSize you get respectively an underrun or an overrun. An underrun is fatal, an overrun is not. InTransferLength/OutTransferLength exist because the controller may not know how to infer the transfer length for all CDB opcodes (some express it in bytes, some express it in blocks, some are vendor specific, some are just crazy and you have to inspect multiple CDB fields to compute the transfer length). Of course ScsiDisk *does* know how to infer the transfer length for the CDBs it builds. Therefore, it makes sense for ScsiDisk's own READ and WRITE CDBs to always have InTransferLength/OutTransferLength = CDB.TransferLength * BlockSize. However, the controller driver must not rely on this. > 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. I agree that: - there's no need for the controller driver to set HostAdapterStatus to anything - even if it sets it to something, TargetStatus should not be set and there should be no sense data. Regarding the first point, this is not exactly an underrun, though it may remind of one. EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER might be just as good, and even EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK might be okay because the request has never reached the controller. The important bit is EFI_BAD_BUFFER_SIZE. Thus, Laszlo's incremental patch in http://article.gmane.org/gmane.comp.bios.edk2.devel/2007 is necessary if I understand it correctly. Regarding the second point, the TargetStatus and sense data *could* be set if the transfer length in the CDB exceeds the maximum transfer length in the VPD. In this case, the sense data will be filled and the target status will be EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION. This is the case where the LUN's maximum transfer length is exceeded, which is why it is based on the CDB rather than InTransferLength/OutTransferLength. However, in this case you cannot expect EFI_BAD_BUFFER_SIZE to be set, because it's not the controller driver's business to inspect target status and sense data (it's called "passthru driver" for a reason :)). Thus, the best you can do is to always back off to smaller sector sizes when you get an unexpected error, like you did in r15491. Paolo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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, >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:
> InTransferLengthOn 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)
