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

["Gary Lin" <glin@suse.com>]

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
> > 
>