From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from de-smtp-delivery-102.mimecast.com (de-smtp-delivery-102.mimecast.com [62.140.7.102]) by mx.groups.io with SMTP id smtpd.web12.4207.1594955737483229315 for ; Thu, 16 Jul 2020 20:15:38 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=mimecast20200619 header.b=Cs/ElR9B; spf=pass (domain: suse.com, ip: 62.140.7.102, mailfrom: glin@suse.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=mimecast20200619; t=1594955736; 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: in-reply-to:in-reply-to:references:references; bh=Jem+yhhPvcM/JU7LVs7jkq94JKpWvPFeXO6UFzUgBpo=; b=Cs/ElR9BRIy6zRROd3xGKKV1SNPLYuu7pImgdXudKx5ty0dEV6ixjx48om/36tS0WdGgU+ UKVe/3KTbKavsxjkhibMrA57i4oJn7hznii6r5Ks/ZIkXOnajocc2lSioX6D5B9eUSutVq fiPLW4JNOjNS3L0E2zz5XvYyD/FIatA= Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05lp2170.outbound.protection.outlook.com [104.47.17.170]) (Using TLS) by relay.mimecast.com with ESMTP id de-mta-10-UXwkUIb3N86amLiZiHPySQ-1; Fri, 17 Jul 2020 05:15:34 +0200 X-MC-Unique: UXwkUIb3N86amLiZiHPySQ-1 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=oMRFtNhU0XLNVz/n5FWfKmH+f0axU3X1gNEJ9q7fNsOQTWMalK9B4Lwn5MyLa2idR3/OY54xHCT8dI5PdfipPiRXdd/4qyq9nAHsAoD3XtWyhhlvTsc67VUyfKSILGYo+DEPOtkfloWoFSlm0aEhn63dL0Ft5IlkUZLbIrKZ81HETuJ6L7MRKkLlccwjQ4c5e3nTA9XsAjMDxj7+b6D+EYLSk1ig+tcDoVf7eDrMXjEXXUWZlETWvxMT4cnafyepYgUfj3CZ2IUsh/Tluk62ad4nKwMgxsms8Z/ys8Y4ypQzKTfY+vxrBr2Lk4+kz4O743JfvdYoF7WXSOjV1IuvDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Jem+yhhPvcM/JU7LVs7jkq94JKpWvPFeXO6UFzUgBpo=; b=IkQCxqJvoTsAoWaD8Xk0aN4yYEp7BImrgstt6ULBEj/ace8JJJUhNDiynSK+swJ1GrG+G7OgVEndUgl4rhIzlQWFar2W3tm4nKscz0fMjwuQThjdxITLCQ5Mbtbp2GKzQN7gJZqe59lVfiLgD/IYUX0ejM3WyScwe7ajfxjzh4OMQewFf74Hf9GoG8y6xfOXnVGJDyBdIk5AaTh9vnJ1rFzXiWY9YLRfr7izG6WMzHqDaYhsX8RUtFgpS/P776nssL5uJ19/LZbH5gTYmyoAtA8+a5ciN2sjTI4FXS7aRKwEm1TTPXfsNTM5DKU6y69xNd4lIXzJiLDHzmLar38gVw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none Authentication-Results: redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=suse.com; Received: from AM0PR0402MB3809.eurprd04.prod.outlook.com (2603:10a6:208:10::30) by AM0PR04MB4562.eurprd04.prod.outlook.com (2603:10a6:208:72::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.23; Fri, 17 Jul 2020 03:15:33 +0000 Received: from AM0PR0402MB3809.eurprd04.prod.outlook.com ([fe80::a14c:d441:c8a9:77ba]) by AM0PR0402MB3809.eurprd04.prod.outlook.com ([fe80::a14c:d441:c8a9:77ba%6]) with mapi id 15.20.3174.026; Fri, 17 Jul 2020 03:15:32 +0000 Date: Fri, 17 Jul 2020 11:15:24 +0800 From: "Gary Lin" To: Laszlo Ersek Cc: devel@edk2.groups.io, Jordan Justen , Ard Biesheuvel Subject: Re: [PATCH v2 11/12] OvmfPkg/LsiScsiDxe: Calculate the transferred bytes Message-ID: <20200717031524.GO6058@GaryWorkstation> References: <20200716074607.18048-1-glin@suse.com> <20200716074607.18048-12-glin@suse.com> In-Reply-To: X-ClientProxiedBy: AM0PR06CA0094.eurprd06.prod.outlook.com (2603:10a6:208:fa::35) To AM0PR0402MB3809.eurprd04.prod.outlook.com (2603:10a6:208:10::30) Return-Path: glin@suse.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from GaryWorkstation (60.251.47.115) by AM0PR06CA0094.eurprd06.prod.outlook.com (2603:10a6:208:fa::35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3195.18 via Frontend Transport; Fri, 17 Jul 2020 03:15:31 +0000 X-Originating-IP: [60.251.47.115] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 0a26ee94-6932-45f5-d36a-08d829ffaa61 X-MS-TrafficTypeDiagnostic: AM0PR04MB4562: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:4502; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: vbUynjK67Xvk59b0mQNeKsBeuPirSZuDfP+pnTA7DzJwf3pxmlml7jLV3hw6zGZsiNdow2sLTmAQJuBHhjwPx9UGavNG9RVBUGHKa9WdyUF8LmL+YQoIRJg6EbuLbsZO5xAPrALwbXldwai+5Pn0Bfif51+d3ez3thPfje8hcqNyBwX1FSUIKphNKg4SJQIW8Rb+Id2bW4dXrIuREedX0/GgLSQte40+JQ2I1bqXK2aJs2AQH05+pEG+zEBAiEKuWtDQ9minOL8gLpV1KDn3kFcfhb6naONjRRb2AXq0MBl96V8AdxPAy+CphnZGjfZZOEyXjCH9wrEIsE1puJ0G+Q== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AM0PR0402MB3809.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(39860400002)(366004)(396003)(376002)(136003)(346002)(33656002)(6916009)(478600001)(8936002)(54906003)(5660300002)(8676002)(55016002)(4326008)(33716001)(316002)(2906002)(16526019)(186003)(9686003)(26005)(53546011)(55236004)(1076003)(66946007)(66476007)(956004)(83380400001)(66556008)(86362001)(52116002)(6496006)(6666004);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: LvlME8QsPOdvo5B8qCGkPOy0mizAXK/cJfBVatZ0kMheJ/fj5BAYKSgslhKuWDq7vvB0cB7cpaNGKemf6RHJKJr/Ior9kl4Rtd0Qh9c7UqGblHO/LsPvxzDIgI35v7SZ7yO9cO7/QKHGpQVYMRxPfjWXc6X4XkM7/RbWNoCvvGi87NUwVKo/TI84Z02lKtkHHsuhsCuBv+QLjD0W/QHZBuj4oMs3dyBHeyX9UQo/IdSLaAFIEu4k7KusTodFseoKx8objUPyGbNhOwAEhCNbg18dysxz9MoOuHmm+f/GxXzFOxg4WOY513BMDOS4eaHsF7BGHZFovMPfu4p4ksu0QEuPlPMtoSzY1Ng+HKzrNEVHi/dkCiWWK3KpbuykuPtkjOiOyhuD0XqYHDeTgwPt22UN0VEPKCYoAd76pJ5gktoCJliqPAblkuCf2JheIR7Rp05785iYcTeB4J3TRHxlaUPcr65SaY1YfeXejwPFluk6At8nyTTiH+ypHDI7cSAC X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: 0a26ee94-6932-45f5-d36a-08d829ffaa61 X-MS-Exchange-CrossTenant-AuthSource: AM0PR0402MB3809.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2020 03:15:32.7001 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: FDiNxru1W/6af7vNbloLXXqpTxMYHWwn9EEbpmR9ul8bcCodROmVQK7e5dlEqk6z X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB4562 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > 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/ > 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 > > >