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
