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 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* than talking to any LUN. > > Therefore, if the host controller driver returns a value saying "hey this is > my max transfer size", it is the responsibility of the LUN- and > media-specific higher level driver (instance) to round that down to a whole > multiple of the block size that exists on *that* level (and that level only). > > Feng said: > >>> 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 think so, yes. As long as the passthru driver complies with the priority > order, ultimately it can set *just* the return status (to > EFI_BAD_BUFFER_SIZE), and leave everything else zeroed (ie. "success" > codes, and no sense data). In my previous statement I wanted to describe the > order in which a passthrough driver was allowed to provide error details, but > I missed that said priority order can be satisfied also by returning just > EFI_BAD_BUFFER_SIZE, and not explaining anything else. > > Back to Paolo's comments: > >> I agree that: >> >> - there's no need for the controller driver to set HostAdapterStatus >> to anything > > Agreed. There is no *need*, but it is allowed by the specification of > EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru(), and VirtioScsiDxe conforms to > that. > >> >> - even if it sets it to something, TargetStatus should not be set and >> there should be no sense data. > > Agreed, and VirtioScsiDxe behaves exactly like that. In the case at hand, > TargetStatus is set to EFI_EXT_SCSI_STATUS_TARGET_GOOD (value 0), and > SenseDataLength is set to zero. > >> 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. > > Yes. My original patch worked in isolation (ie. without the incremental > patch) probably only because VirtioScsiDxe had been setting HostAdapterStatus > to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, which is just one > of the correct possibilities, as you say. That HostAdapterStatus happened to > trigger a path in ScsiDiskDxe that was ultimately good. But, if VirtioScsiDxe > had set HostAdapterStatus to something else (eg. > ADAPTER_OTHER or ADAPTER_OK, as you say), then maybe ScsiDiskDxe would have > done the wrong thing even with my first patch in place, because it never > looked for EFI_BAD_BUFFER_SIZE explicitly. Other SCSI drivers could set > HostAdapterStatus like this, on EFI_BAD_BUFFER_SIZE (and it would be valid), > so that's why the incremental patch is needed for ScsiDiskDxe. > >> 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 :)). > > VirtioScsiDxe conforms to this; the only place PassThru() returns > EFI_BAD_BUFFER_SIZE is when the host controller's transfer limit is exceeded. > >> Thus, the best you can do is to always back off to smaller sector >> sizes > > (To smaller sector *counts*, ITYM. > > Independently, UefiScsiLib uses the "SectorSize" parameter name confusingly > in a few places; it should say "SectorCount".) > >> when you get an unexpected error, like you did in r15491. >> >> Paolo >> > > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
