On Thu, Jul 16, 2020 at 08:21:46PM +0200, Laszlo Ersek wrote:
> On 07/16/20 09:46, Gary Lin wrote:
> > Calculate the transferred bytes during data phases based on the
> > Cumulative SCSI Byte Count (CSBC) and update
> > InTransferLength/OutTransferLength of the request packet.
> >
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> > Signed-off-by: Gary Lin <g...@suse.com>
> > ---
> > OvmfPkg/Include/IndustryStandard/LsiScsi.h | 1 +
> > OvmfPkg/LsiScsiDxe/LsiScsi.c | 50 ++++++++++++++++++++++
> > 2 files changed, 51 insertions(+)
> >
> > diff --git a/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > b/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > index 9964bd40385c..01d75323cdbc 100644
> > --- a/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > +++ b/OvmfPkg/Include/IndustryStandard/LsiScsi.h
> > @@ -25,6 +25,7 @@
> > #define LSI_REG_DSP 0x2C
> > #define LSI_REG_SIST0 0x42
> > #define LSI_REG_SIST1 0x43
> > +#define LSI_REG_CSBC 0xDC
> >
> > //
> > // The status bits for DMA Status (DSTAT)
> > diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > index d5fe1d4ab3b8..10483ed02bd7 100644
> > --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> > @@ -79,6 +79,24 @@ In8 (
> > );
> > }
> >
> > +STATIC
> > +EFI_STATUS
> > +In32 (
> > + IN LSI_SCSI_DEV *Dev,
> > + IN UINT32 Addr,
> > + OUT UINT32 *Data
> > + )
> > +{
> > + return Dev->PciIo->Io.Read (
> > + Dev->PciIo,
> > + EfiPciIoWidthUint32,
> > + PCI_BAR_IDX0,
> > + Addr,
> > + 1,
> > + Data
> > + );
> > +}
> > +
> > STATIC
> > EFI_STATUS
> > LsiScsiReset (
> > @@ -219,6 +237,8 @@ LsiScsiProcessRequest (
> > UINT8 DStat;
> > UINT8 SIst0;
> > UINT8 SIst1;
> > + UINT32 Csbc;
> > + UINT32 CsbcBase;
> >
> > Script = Dev->Dma->Script;
> > Cdb = Dev->Dma->Cdb;
> > @@ -232,6 +252,18 @@ LsiScsiProcessRequest (
> > SetMem (Cdb, sizeof Dev->Dma->Cdb, 0x00);
> > CopyMem (Cdb, Packet->Cdb, Packet->CdbLength);
> >
> > + //
> > + // Fetch the first Cumulative SCSI Byte Count (CSBC).
> > + //
> > + // CSBC is a cumulative counter of the actual number of bytes that has
> > been
>
> (1) typo: s/has been/have been/
>
Will fix in v3.
> > + // transferred across the SCSI bus during data phases, i.e. it will not
>
> (2) typo (I think): s/will not/will not count/
>
ditto
> > + // bytes sent in command, status, message in and out phases.
> > + //
> > + Status = In32 (Dev, LSI_REG_CSBC, &CsbcBase);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
>
> (3) IMO, we should not return directly, but jump to the Error label.
>
Right, the request packet has to be modified on the error status.
> > + }
> > +
> > //
> > // Clean up the DMA buffer for the script.
> > //
> > @@ -407,6 +439,24 @@ LsiScsiProcessRequest (
> > gBS->Stall (Dev->StallPerPollUsec);
> > }
> >
> > + //
> > + // Fetch CSBC again to calculate the transferred bytes and update
> > + // InTransferLength/OutTransferLength.
> > + //
>
> This calculation matters for EFI_SUCCESS. But at this point we cannot
> yet guarantee that we'll return EFI_SUCCESS.
>
> (4) So I suggest postponing the CSBC re-fetch until after we're past the
> last "goto Error" statement -- that is, just before "Copy Data to
> InDataBuffer if necessary".
>
I thought InTransferLength/OutTransferLength may matter for the error
path. After checking UEFI spec again, it only matters for EFI_SUCCESS
and EFI_BAD_BUFFER_SIZE. The latter is handled in LsiScsiCheckRequest().
Will move the code down.
> > + // Note: The number of transferred bytes is bounded by
> > + // "sizeof Dev->Dma->Data", so it's safe to subtract CsbcBase
> > + // from Csbc.
> > + //
>
> It's safe to perform the subtraction, but I think we should extend the
> comment.
>
> This register seems to be a 4-byte counter. It's not super difficult to
> transfer more than 4GB even in UEFI, and so the counter might wrap around.
>
> Luckily, when it wraps around, the subtraction will do exactly the right
> thing. (And for that, it is indeed relevant that the max requestable
> transfer size is smaller than (MAX_UINT32+1).)
>
> Assume
>
> Csbc < CsbcBase
>
> This means (a) the counter has wrapped, and (b) mathematically, the
> difference
>
> Csbc - Csbcbase
>
> is negative.
>
> Given that we use UINT32 variables here, the resultant value (in C) will
> be (per C standard):
>
> (MAX_UINT32 + 1) + (Csbc - Csbcbase) [1]
>
> And that's perfect! Because, what does it mean that "CSBC wraps"? It
> means that the mathematical meaning of the new CSBC value is:
>
> ((MAX_UINT32 + 1) + Csbc)
>
> (It's just a different way to say that bit#32 is set in the mathematical
> value.)
>
> Consequently, for getting the right difference, we'd have to calculate:
>
> ((MAX_UINT32 + 1) + Csbc) - Csbcbase [2]
>
> But that's exactly what the C language *already* gives us, in [1].
>
>
> (5) So please append the following sentence to the comment:
>
> If the CSBC register wraps around, the correct difference is ensured
> by the standard C modular arithmetic.
>
Fair enough. It's always good to have more comments :)
> (6) Furthermore, please dedicate a new variable to the difference:
>
> UINT32 Transferred;
>
> Transferred = Csbc - CsbcBase;
>
Ok, it's more readable. Will add it in v3.
> > + Status = In32 (Dev, LSI_REG_CSBC, &Csbc);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
>
> (7) Again I think we should jump to the Error label.
>
Will fix it in v3.
> > + if (Packet->InTransferLength > 0) {
> > + Packet->InTransferLength = Csbc - CsbcBase;
> > + } else if (Packet->OutTransferLength > 0) {
> > + Packet->OutTransferLength = Csbc - CsbcBase;
> > + }
> > +
>
> (8) I'd recommend a sanity check -- it's unlikely that the device will
> blatantly lie, but if it ever happens, we should not let it out.
>
> We may only ever reduce the transfer length. Thus:
>
> if (Packet->InTransferLength > 0) {
> if (Transferred <= Packet->InTransferLength) {
> Packet->InTransferLength = Transferred;
> } else {
> goto Error;
> }
> } else if (Packet->OutTransferLength > 0) {
> if (Transferred <= Packet->OutTransferLength) {
> Packet->OutTransferLength = Transferred;
> } else {
> goto Error;
> }
> }
>
Thanks! This handles error cases well. Will do it in v3.
Gary Lin
> Thanks,
> Laszlo
>
> > //
> > // Check if everything is good.
> > // SCSI Message Code 0x00: COMMAND COMPLETE
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62719): https://edk2.groups.io/g/devel/message/62719
Mute This Topic: https://groups.io/mt/75537216/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
