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.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Gary Lin <glin@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
+ // transferred across the SCSI bus during data phases, i.e. it will not
+ // bytes sent in command, status, message in and out phases.
+ //
+ Status = In32 (Dev, LSI_REG_CSBC, &CsbcBase);
+ if (EFI_ERROR (Status)) {
+ return 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.
+ //
+ // Note: The number of transferred bytes is bounded by
+ // "sizeof Dev->Dma->Data", so it's safe to subtract CsbcBase
+ // from Csbc.
+ //
+ Status = In32 (Dev, LSI_REG_CSBC, &Csbc);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ if (Packet->InTransferLength > 0) {
+ Packet->InTransferLength = Csbc - CsbcBase;
+ } else if (Packet->OutTransferLength > 0) {
+ Packet->OutTransferLength = Csbc - CsbcBase;
+ }
+
//
// Check if everything is good.
// SCSI Message Code 0x00: COMMAND COMPLETE
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62665): https://edk2.groups.io/g/devel/message/62665
Mute This Topic: https://groups.io/mt/75537216/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Gary Lin <glin@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/
> + // transferred across the SCSI bus during data phases, i.e. it will not
(2) typo (I think): s/will not/will not count/
> + // 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.
> + }
> +
> //
> // 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".
> + // 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.
(6) Furthermore, please dedicate a new variable to the difference:
UINT32 Transferred;
Transferred = Csbc - CsbcBase;
> + Status = In32 (Dev, LSI_REG_CSBC, &Csbc);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
(7) Again I think we should jump to the Error label.
> + 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,
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 (#62710): https://edk2.groups.io/g/devel/message/62710
Mute This Topic: https://groups.io/mt/75537216/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Signed-off-by: Gary Lin <glin@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.