Re: [PATCH v2 11/12] OvmfPkg/LsiScsiDxe: Calculate the transferred bytes

["Laszlo Ersek" <lersek@redhat.com>]

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
>