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* 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
