From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id B3D377803E4 for ; Tue, 16 Jan 2024 13:44:13 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ILMTxPRwHLCDfmEizw8hMgUzos0YgJb5zwOGpZLrtL8=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1705412652; v=1; b=DG/r+5nfAiYGhsiMbT0TlNu8YASNbPWZDpNEqQ9ATiJXtnMGI5YrZOT9vj+PWJdSjd69C1CB YVVuh7/g/AyNHqO+5Expbw0QV2RqorLqcKRKSV0ptrI2XqsNCZmTzuMnBFC9qy7lp+NaXpwLuwy e4xucTWCMIA4rdfQ8OaJAg0A= X-Received: by 127.0.0.2 with SMTP id bWgUYY7687511xn7gYV60Y7z; Tue, 16 Jan 2024 05:44:12 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.14056.1705412651515109882 for ; Tue, 16 Jan 2024 05:44:11 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-588-UB4EbltNNLqIjITKNj337g-1; Tue, 16 Jan 2024 08:44:07 -0500 X-MC-Unique: UB4EbltNNLqIjITKNj337g-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id F3A201C0432B for ; Tue, 16 Jan 2024 13:44:06 +0000 (UTC) X-Received: from [10.39.194.248] (unknown [10.39.194.248]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2D3C12166B32; Tue, 16 Jan 2024 13:44:06 +0000 (UTC) Message-ID: <4c9e81d6-59ca-ace0-0670-59a3e2ec507b@redhat.com> Date: Tue, 16 Jan 2024 14:44:05 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 6/6] OvmfPkg/VirtNorFlashDxe: move DoErase code block into new function To: devel@edk2.groups.io, kraxel@redhat.com Cc: oliver@redhat.com References: <20240115155948.136499-1-kraxel@redhat.com> <20240115155948.136499-7-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240115155948.136499-7-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: u3GScHS1lFxK69y7glulyG5Vx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="DG/r+5nf"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/15/24 16:59, Gerd Hoffmann wrote: > Move the DoErase code block into a separate function, call the function > instead of jumping around with goto. > > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 76 +++++++++++++++++--------- > 1 file changed, 51 insertions(+), 25 deletions(-) > > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlas= hDxe/VirtNorFlash.c > index d80e9f0a2f3a..203bd64f2bdf 100644 > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c > @@ -502,6 +502,37 @@ NorFlashRead ( > return EFI_SUCCESS; > } > > +STATIC EFI_STATUS (1) EFI_STATUS is not needed (and if it were needed, then we'd put it on a separate line) > +NorFlashWriteSingleBlockWithErase ( > + IN NOR_FLASH_INSTANCE *Instance, > + IN EFI_LBA Lba, > + IN UINTN Offset, > + IN OUT UINTN *NumBytes, > + IN UINT8 *Buffer > + ) > +{ > + EFI_STATUS Status; > + > + // Read NOR Flash data into shadow buffer > + Status =3D NorFlashReadBlocks (Instance, Lba, Instance->BlockSize, Ins= tance->ShadowBuffer); > + if (EFI_ERROR (Status)) { > + // Return one of the pre-approved error statuses > + return EFI_DEVICE_ERROR; > + } > + > + // Put the data at the appropriate location inside the buffer area > + CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *Nu= mBytes); > + > + // Write the modified buffer back to the NorFlash > + Status =3D NorFlashWriteBlocks (Instance, Lba, Instance->BlockSize, In= stance->ShadowBuffer); > + if (EFI_ERROR (Status)) { > + // Return one of the pre-approved error statuses > + return EFI_DEVICE_ERROR; > + } > + > + return EFI_SUCCESS; > +} Good; "git-show --color-moved=3Dzebra" is really helpful here. What changes across the code movement is the BufferSizeInBytes argument for the NorFlashReadBlocks / NorFlashWriteBlocks calls. Previously, we'd pass in the local variable BlockSize, with type UINTN. After, we pass in Instance->BlockSize, a UINT32. However, this is all fine, given that the BufferSizeInBytes param of both callees is UINTN, and even the local variable is assigned from Instance->BlockSize, pre-patch. OK. > + > /* > Write a full or portion of a block. It must not span block boundaries;= that is, > Offset + *NumBytes <=3D Instance->BlockSize. > @@ -606,7 +637,14 @@ NorFlashWriteSingleBlock ( > // that we want to set. In that case, we will need to erase the bloc= k first. > for (CurOffset =3D 0; CurOffset < *NumBytes; CurOffset++) { > if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) { > - goto DoErase; > + Status =3D NorFlashWriteSingleBlockWithErase ( > + Instance, > + Lba, > + Offset, > + NumBytes, > + Buffer > + ); > + goto Exit; > } > > OrigData[CurOffset] =3D Buffer[CurOffset]; > @@ -635,33 +673,21 @@ NorFlashWriteSingleBlock ( > goto Exit; > } > } > + } else { > + Status =3D NorFlashWriteSingleBlockWithErase ( > + Instance, > + Lba, > + Offset, > + NumBytes, > + Buffer > + ); > + } > > Exit: > - // Put device back into Read Array mode > - SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY= ); > + // Put device back into Read Array mode > + SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); > > - return Status; > - } > - > -DoErase: > - // Read NOR Flash data into shadow buffer > - Status =3D NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->Sha= dowBuffer); > - if (EFI_ERROR (Status)) { > - // Return one of the pre-approved error statuses > - return EFI_DEVICE_ERROR; > - } > - > - // Put the data at the appropriate location inside the buffer area > - CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *Nu= mBytes); > - > - // Write the modified buffer back to the NorFlash > - Status =3D NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->Sh= adowBuffer); > - if (EFI_ERROR (Status)) { > - // Return one of the pre-approved error statuses > - return EFI_DEVICE_ERROR; > - } > - > - return EFI_SUCCESS; > + return Status; > } > > EFI_STATUS (2) The extraction of the erase code path is fine, but how it is being put to use is not identical to the pre-patch state. The key observation is that, pre-patch, the Exit label is *semantically local* to the "word-based writes" branch. Of course this statement makes no sense at the C language level, because labels are local to functions, not to blocks within functions. Either way, the original logic is: - if the logical update is too big (i.e., we don't run the word-based optimization), just run DoErase. "Exit" is not reached, and we don't put device back into Read Array mode. - if the logical update is not too big, we verify the bit transitions. If those prevent the word-based optimization, we just run DoErase again. "Exit" is not reached, and we don't put the device back into Read Array mode. - Otherwise, we commit to the word-based optimization. We start out by GET_NOR_BLOCK_ADDRESS(), after which we need to finish with "Exit", i.e., with kicking the flash back to Read Array mode, regardless of success vs. error. The patch changes this, because now we reach "Exit" (and the Read Array mode setting) in *all three* cases. That seems wrong (or at least an unjustified change). (2.1) The simplest fix (to be squashed) that I can imagine is just to "return Status" right after each NorFlashWriteSingleBlockWithErase() call returns: >| diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| index 203bd64f2bdf..bd343562f4f0 100644 >| --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| @@ -638,19 +638,19 @@ NorFlashWriteSingleBlock ( >| for (CurOffset =3D 0; CurOffset < *NumBytes; CurOffset++) { >| if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) { >| Status =3D NorFlashWriteSingleBlockWithErase ( >| Instance, >| Lba, >| Offset, >| NumBytes, >| Buffer >| ); >| - goto Exit; >| + return Status; >| } >| >| OrigData[CurOffset] =3D Buffer[CurOffset]; >| } >| >| // >| // Write the updated buffer to NOR. >| // >| BlockAddress =3D GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress= , Lba, >| BlockSize); >| @@ -675,18 +675,19 @@ NorFlashWriteSingleBlock ( >| } >| } else { >| Status =3D NorFlashWriteSingleBlockWithErase ( >| Instance, >| Lba, >| Offset, >| NumBytes, >| Buffer >| ); >| + return Status; >| } >| >| Exit: >| // Put device back into Read Array mode >| SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY)= ; >| >| return Status; >| } >| (2.2) A more extensive reorganization would be the following (also to be squashed). The benefit of this approach is that it decreses the total nesting depth in NorFlashWriteSingleBlock(), using early exit points. The diff is formatted with "git diff -b", in order to make the un-indentation less confusing. >| diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFla= shDxe/VirtNorFlash.c >| index 203bd64f2bdf..ba043f851547 100644 >| --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c >| @@ -596,106 +596,107 @@ NorFlashWriteSingleBlock ( >| // ^ ^ ^ ^ >| // | | | | >| // | | | End, the next "word" boundary = beyond >| // | | | the (logical) update >| // | | | >| // | | (Offset & 0x7F) + NumBytes; i.e., the Offset in= side >| // | | (or just past) the *double-word* such that Offs= et is >| // | | the *exclusive* end of the (logical) update. >| // | | >| // | Offset & BOUNDARY_OF_32_WORDS; i.e., Offset within th= e "word"; >| // | this is where the (logical) update is supposed to sta= rt >| // | >| // Start =3D Offset & ~BOUNDARY_OF_32_WORDS; i.e., Offset truncate= d to "word" boundary >| >| Start =3D Offset & ~BOUNDARY_OF_32_WORDS; >| End =3D ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYT= ES); >| >| - if ((End - Start) <=3D (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) { >| + if ((End - Start) > (4 * P30_MAX_BUFFER_SIZE_IN_BYTES)) { >| + Status =3D NorFlashWriteSingleBlockWithErase ( >| + Instance, >| + Lba, >| + Offset, >| + NumBytes, >| + Buffer >| + ); >| + return Status; >| + } >| + >| // Check to see if we need to erase before programming the data into = NOR. >| // If the destination bits are only changing from 1s to 0s we can jus= t write. >| // After a block is erased all bits in the block is set to 1. >| // If any byte requires us to erase we just give up and rewrite all o= f it. >| >| // Read the old version of the data into the shadow buffer >| Status =3D NorFlashRead ( >| Instance, >| Lba, >| Start, >| End - Start, >| Instance->ShadowBuffer >| ); >| if (EFI_ERROR (Status)) { >| return EFI_DEVICE_ERROR; >| } >| >| // Make OrigData point to the start of the old version of the data in= side >| // the word aligned buffer >| OrigData =3D Instance->ShadowBuffer + (Offset & BOUNDARY_OF_32_WORDS)= ; >| >| // Update the buffer containing the old version of the data with the = new >| // contents, while checking whether the old version had any bits clea= red >| // that we want to set. In that case, we will need to erase the block= first. >| for (CurOffset =3D 0; CurOffset < *NumBytes; CurOffset++) { >| if (~(UINT32)OrigData[CurOffset] & (UINT32)Buffer[CurOffset]) { >| Status =3D NorFlashWriteSingleBlockWithErase ( >| Instance, >| Lba, >| Offset, >| NumBytes, >| Buffer >| ); >| - goto Exit; >| + return Status; >| } >| >| OrigData[CurOffset] =3D Buffer[CurOffset]; >| } >| >| // >| // Write the updated buffer to NOR. >| // >| BlockAddress =3D GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress, = Lba, BlockSize); >| >| // Unlock the block if we have to >| Status =3D NorFlashUnlockSingleBlockIfNecessary (Instance, BlockAddre= ss); >| if (EFI_ERROR (Status)) { >| goto Exit; >| } >| >| Count =3D (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES; >| for (Index =3D 0; Index < Count; Index++) { >| Status =3D NorFlashWriteBuffer ( >| Instance, >| BlockAddress + Start + Index * P30_MAX_BUFFER_SIZE_IN_BY= TES, >| P30_MAX_BUFFER_SIZE_IN_BYTES, >| Instance->ShadowBuffer + Index * P30_MAX_BUFFER_SIZE_IN_= BYTES >| ); >| if (EFI_ERROR (Status)) { >| goto Exit; >| } >| } >| - } else { >| - Status =3D NorFlashWriteSingleBlockWithErase ( >| - Instance, >| - Lba, >| - Offset, >| - NumBytes, >| - Buffer >| - ); >| - } >| >| Exit: >| // Put device back into Read Array mode >| SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY)= ; >| >| return Status; >| } I'd be OK with ether (2.1) or (2.2). Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113899): https://edk2.groups.io/g/devel/message/113899 Mute This Topic: https://groups.io/mt/103741665/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-