From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.21865.1594923714499352047 for ; Thu, 16 Jul 2020 11:21:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dlQoU/AX; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594923713; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=L3hk08lrbJzaQtdwCvmeclI8ub1vzWSczv20eHinZ1E=; b=dlQoU/AX+bQzkHE8SmN2ZCFOlgIBuTw6XUTu93HrStGfbx5E0JSja4y7/lWVz55Yck86nm WNkotfRWbJNphAiy60nFjngi9V4rTZ+bfiZe41INTiQQWjzSvb8Yxn5tylWoK9WYTX5x/d ZvrTcYWeSLPzLZlD1Lxu1h1PhKTuKiU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-399-TLI2OpdWOJCdpH3O8tiOVg-1; Thu, 16 Jul 2020 14:21:50 -0400 X-MC-Unique: TLI2OpdWOJCdpH3O8tiOVg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 120DC8014D4; Thu, 16 Jul 2020 18:21:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-123.ams2.redhat.com [10.36.112.123]) by smtp.corp.redhat.com (Postfix) with ESMTP id D4B1872AC8; Thu, 16 Jul 2020 18:21:47 +0000 (UTC) Subject: Re: [PATCH v2 11/12] OvmfPkg/LsiScsiDxe: Calculate the transferred bytes To: Gary Lin , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel References: <20200716074607.18048-1-glin@suse.com> <20200716074607.18048-12-glin@suse.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 16 Jul 2020 20:21:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200716074607.18048-12-glin@suse.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Gary Lin > --- > 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 >