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
