From nobody Tue Feb 10 01:50:26 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) client-ip=66.175.222.108; envelope-from=bounce+27952+95094+1787277+3901457@groups.io; helo=mail02.groups.io; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce+27952+95094+1787277+3901457@groups.io ARC-Seal: i=1; a=rsa-sha256; t=1665571599; cv=none; d=zohomail.com; s=zohoarc; b=csVc+IjGKxCa8Ga5BZLyY/qCo3XeE6UI6kQKAXthjXbTo046j596vHMaifax9hCF4b+/qaz+DJoy5pNE6odDHEZbLt1d4LA7t2PE9jnqK5Mxrx5SGl3IvvmLKkKmJvm93oHIIln2KDDpPYawGPqic+C2LqUG/AvERBErvwSqa8Y= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1665571599; h=Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Subscribe:List-Id:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:References:Sender:Subject:To; bh=wu/qSEkmJyXrssBlVCqJen6t5f/HOeXeaa8jiR+Cc84=; b=ghBPYyaoBTUrP9Ypslwc6VocfsMEPxZYqeXv6lIHGmGwnvO50V+NxBwvgjfSJd0vpaZkpY6YheOguA8sOctU9lvr/6+s6YvQhBUrpvaBVVTT+/C581+nPq96geLd9Ujt4d4x2QcB6R28W02VpOGbsaLOrmK054oxaZYuYs8ywpk= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce+27952+95094+1787277+3901457@groups.io Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by mx.zohomail.com with SMTPS id 1665571599632681.4078815975864; Wed, 12 Oct 2022 03:46:39 -0700 (PDT) Return-Path: X-Received: by 127.0.0.2 with SMTP id aScPYY1788612x1g9k7ZT7v5; Wed, 12 Oct 2022 03:46:39 -0700 X-Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mx.groups.io with SMTP id smtpd.web10.18675.1665571598677851748 for ; Wed, 12 Oct 2022 03:46:38 -0700 X-Received: by mail-pj1-f51.google.com with SMTP id o17-20020a17090aac1100b0020d98b0c0f4so251202pjq.4 for ; Wed, 12 Oct 2022 03:46:38 -0700 (PDT) X-Gm-Message-State: 9w8uKAoFfMIspaWWTVdgwKtLx1787277AA= X-Google-Smtp-Source: AMsMyM6UF1IYsbThREHtE5oQhglQgiq1rtzgumA1vZFggoeBXn7PG07PO3V83Aa20z1DBl75W/8K+Q== X-Received: by 2002:a17:902:d34a:b0:17f:9be0:cdf9 with SMTP id l10-20020a170902d34a00b0017f9be0cdf9mr28747308plk.132.1665571597905; Wed, 12 Oct 2022 03:46:37 -0700 (PDT) X-Received: from localhost.localdomain ([49.206.13.138]) by smtp.gmail.com with ESMTPSA id z4-20020aa79e44000000b0054223a0185asm10812221pfq.161.2022.10.12.03.46.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Oct 2022 03:46:37 -0700 (PDT) From: "Sunil V L" To: devel@edk2.groups.io Cc: Ard Biesheuvel , Leif Lindholm , Jiewen Yao , Jordan Justen , Gerd Hoffmann Subject: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V2 30/33] OvmfPkg/NorFlashDxe: Avoid switching between modes in a tight loop Date: Wed, 12 Oct 2022 16:14:53 +0530 Message-Id: <20221012104456.1393376-31-sunilvl@ventanamicro.com> In-Reply-To: <20221012104456.1393376-1-sunilvl@ventanamicro.com> References: <20221012104456.1393376-1-sunilvl@ventanamicro.com> MIME-Version: 1.0 Precedence: Bulk List-Unsubscribe: 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,sunilvl@ventanamicro.com Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=groups.io; q=dns/txt; s=20140610; t=1665571599; bh=2KBoyOtdXmMWYeTtlaIXViiKOsiYtYiPLGjwBpoEnnU=; h=Cc:Date:From:Reply-To:Subject:To; b=xYUfsi97MX4AaeUgHiiaMzQ16TfJWmED27jmIHmnihecWYmSx18B+qd4i9r0PvKa1rF H2WltNP7JwvIUD4mBmMVtZqM61CsD1bhvywkrHGUcBf1Wmn2EMFEqij+TckS31ntuzAh5 0KRNna+y61jLtnjuurAPBFIwWnaTMKbQ1qI= X-ZohoMail-DKIM: pass (identity @groups.io) X-ZM-MESSAGEID: 1665571600942100009 Content-Type: text/plain; charset="utf-8" Currently, when dealing with small updates that can be written out directly (i.e., if they only involve clearing bits and not setting bits, as the latter requires a block level erase), we iterate over the data one word at a time, read the old value, compare it, write the new value, and repeat, unless we encountered a value that we cannot write (0->1 transition), in which case we fall back to a block level operation. This is inefficient for two reasons: - reading and writing a word at a time involves switching between array and programming mode for every word of data, which is disproportionately costly when running under KVM; - we end up writing some data twice, as we may not notice that a block erase is needed until after some data has been written to flash. So replace this sequence with a single read of up to twice the buffered write maximum size, followed by one or two buffered writes if the data can be written directly. Otherwise, fall back to the existing block level sequence, but without writing out part of the data twice. Cc: Ard Biesheuvel Cc: Leif Lindholm Cc: Jiewen Yao Cc: Jordan Justen Cc: Gerd Hoffmann Signed-off-by: Ard Biesheuvel --- OvmfPkg/Drivers/NorFlashDxe/NorFlash.c | 211 +++++++++---------------- 1 file changed, 75 insertions(+), 136 deletions(-) diff --git a/OvmfPkg/Drivers/NorFlashDxe/NorFlash.c b/OvmfPkg/Drivers/NorFl= ashDxe/NorFlash.c index cdc6b5da8bfb..649e0789db3c 100644 --- a/OvmfPkg/Drivers/NorFlashDxe/NorFlash.c +++ b/OvmfPkg/Drivers/NorFlashDxe/NorFlash.c @@ -582,20 +582,11 @@ NorFlashWriteSingleBlock ( IN UINT8 *Buffer ) { - EFI_STATUS TempStatus; - UINT32 Tmp; - UINT32 TmpBuf; - UINT32 WordToWrite; - UINT32 Mask; - BOOLEAN DoErase; - UINTN BytesToWrite; + EFI_STATUS Status; UINTN CurOffset; - UINTN WordAddr; UINTN BlockSize; UINTN BlockAddress; - UINTN PrevBlockAddress; - - PrevBlockAddress =3D 0; + UINT8 *OrigData; =20 DEBUG ((DEBUG_BLKIO, "NorFlashWriteSingleBlock(Parameters: Lba=3D%ld, Of= fset=3D0x%x, *NumBytes=3D0x%x, Buffer @ 0x%08x)\n", Lba, Offset, *NumBytes,= Buffer)); =20 @@ -606,6 +597,12 @@ NorFlashWriteSingleBlock ( return EFI_ACCESS_DENIED; } =20 + // Check we did get some memory. Buffer is BlockSize. + if (Instance->ShadowBuffer =3D=3D NULL) { + DEBUG ((DEBUG_ERROR, "FvbWrite: ERROR - Buffer not ready\n")); + return EFI_DEVICE_ERROR; + } + // Cache the block size to avoid de-referencing pointers all the time BlockSize =3D Instance->Media.BlockSize; =20 @@ -625,149 +622,91 @@ NorFlashWriteSingleBlock ( return EFI_BAD_BUFFER_SIZE; } =20 - // Pick 128bytes as a good start for word operations as opposed to erasi= ng the - // block and writing the data regardless if an erase is really needed. - // It looks like most individual NV variable writes are smaller than 128= bytes. - if (*NumBytes <=3D 128) { + // Pick P30_MAX_BUFFER_SIZE_IN_BYTES (=3D=3D 128 bytes) as a good start = for word + // operations as opposed to erasing the block and writing the data regar= dless + // if an erase is really needed. It looks like most individual NV varia= ble + // writes are smaller than 128 bytes. + // To avoid pathological cases were a 2 byte write is disregarded becaus= e it + // occurs right at a 128 byte buffered write alignment boundary, permit = up to + // twice the max buffer size, and perform two writes if needed. + if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <=3D (2 * P30_MAX_BUFF= ER_SIZE_IN_BYTES)) { // Check to see if we need to erase before programming the data into N= OR. // If the destination bits are only changing from 1s to 0s we can just= 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 of= it. - DoErase =3D FALSE; - BytesToWrite =3D *NumBytes; - CurOffset =3D Offset; =20 - while (BytesToWrite > 0) { - // Read full word from NOR, splice as required. A word is the smalle= st - // unit we can write. - TempStatus =3D NorFlashRead (Instance, Lba, CurOffset & ~(0x3), size= of (Tmp), &Tmp); - if (EFI_ERROR (TempStatus)) { - return EFI_DEVICE_ERROR; - } - - // Physical address of word in NOR to write. - WordAddr =3D (CurOffset & ~(0x3)) + GET_NOR_BLOCK_ADDRESS ( - Instance->RegionBaseAddress, - Lba, - BlockSize - ); - // The word of data that is to be written. - TmpBuf =3D *((UINT32 *)(Buffer + (*NumBytes - BytesToWrite))); - - // First do word aligned chunks. - if ((CurOffset & 0x3) =3D=3D 0) { - if (BytesToWrite >=3D 4) { - // Is the destination still in 'erased' state? - if (~Tmp !=3D 0) { - // Check to see if we are only changing bits to zero. - if ((Tmp ^ TmpBuf) & TmpBuf) { - DoErase =3D TRUE; - break; - } - } - - // Write this word to NOR - WordToWrite =3D TmpBuf; - CurOffset +=3D sizeof (TmpBuf); - BytesToWrite -=3D sizeof (TmpBuf); - } else { - // BytesToWrite < 4. Do small writes and left-overs - Mask =3D ~((~0) << (BytesToWrite * 8)); - // Mask out the bytes we want. - TmpBuf &=3D Mask; - // Is the destination still in 'erased' state? - if ((Tmp & Mask) !=3D Mask) { - // Check to see if we are only changing bits to zero. - if ((Tmp ^ TmpBuf) & TmpBuf) { - DoErase =3D TRUE; - break; - } - } - - // Merge old and new data. Write merged word to NOR - WordToWrite =3D (Tmp & ~Mask) | TmpBuf; - CurOffset +=3D BytesToWrite; - BytesToWrite =3D 0; - } - } else { - // Do multiple words, but starting unaligned. - if (BytesToWrite > (4 - (CurOffset & 0x3))) { - Mask =3D ((~0) << ((CurOffset & 0x3) * 8)); - // Mask out the bytes we want. - TmpBuf &=3D Mask; - // Is the destination still in 'erased' state? - if ((Tmp & Mask) !=3D Mask) { - // Check to see if we are only changing bits to zero. - if ((Tmp ^ TmpBuf) & TmpBuf) { - DoErase =3D TRUE; - break; - } - } + // Read the old version of the data into the shadow buffer + Status =3D NorFlashRead ( + Instance, + Lba, + Offset & ~BOUNDARY_OF_32_WORDS, + (*NumBytes | BOUNDARY_OF_32_WORDS) + 1, + Instance->ShadowBuffer + ); + if (EFI_ERROR (Status)) { + return EFI_DEVICE_ERROR; + } =20 - // Merge old and new data. Write merged word to NOR - WordToWrite =3D (Tmp & ~Mask) | TmpBuf; - BytesToWrite -=3D (4 - (CurOffset & 0x3)); - CurOffset +=3D (4 - (CurOffset & 0x3)); - } else { - // Unaligned and fits in one word. - Mask =3D (~((~0) << (BytesToWrite * 8))) << ((CurOffset & 0x3) *= 8); - // Mask out the bytes we want. - TmpBuf =3D (TmpBuf << ((CurOffset & 0x3) * 8)) & Mask; - // Is the destination still in 'erased' state? - if ((Tmp & Mask) !=3D Mask) { - // Check to see if we are only changing bits to zero. - if ((Tmp ^ TmpBuf) & TmpBuf) { - DoErase =3D TRUE; - break; - } - } + // Make OrigData point to the start of the old version of the data ins= ide + // the word aligned buffer + OrigData =3D Instance->ShadowBuffer + (Offset & BOUNDARY_OF_32_WORDS); =20 - // Merge old and new data. Write merged word to NOR - WordToWrite =3D (Tmp & ~Mask) | TmpBuf; - CurOffset +=3D BytesToWrite; - BytesToWrite =3D 0; - } + // Update the buffer containing the old version of the data with the n= ew + // contents, while checking whether the old version had any bits clear= ed + // that we want to set. In that case, we will need to erase the block = first. + for (CurOffset =3D 0; CurOffset < *NumBytes; CurOffset++) { + if (~OrigData[CurOffset] & Buffer[CurOffset]) { + goto DoErase; } =20 - // - // Write the word to NOR. - // + OrigData[CurOffset] =3D Buffer[CurOffset]; + } =20 - BlockAddress =3D GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress,= Lba, BlockSize); - if (BlockAddress !=3D PrevBlockAddress) { - TempStatus =3D NorFlashUnlockSingleBlockIfNecessary (Instance, Blo= ckAddress); - if (EFI_ERROR (TempStatus)) { - return EFI_DEVICE_ERROR; - } + // + // Write the updated buffer to NOR. + // + BlockAddress =3D GET_NOR_BLOCK_ADDRESS (Instance->RegionBaseAddress, L= ba, BlockSize); =20 - PrevBlockAddress =3D BlockAddress; - } + // Unlock the block if we have to + Status =3D NorFlashUnlockSingleBlockIfNecessary (Instance, BlockAddres= s); + if (EFI_ERROR (Status)) { + goto Exit; + } =20 - TempStatus =3D NorFlashWriteSingleWord (Instance, WordAddr, WordToWr= ite); - // Put device back into Read Array mode - SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY= ); + Status =3D NorFlashWriteBuffer ( + Instance, + BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS), + P30_MAX_BUFFER_SIZE_IN_BYTES, + Instance->ShadowBuffer + ); + if (EFI_ERROR (Status)) { + goto Exit; + } =20 - if (EFI_ERROR (TempStatus)) { - return EFI_DEVICE_ERROR; + if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) > P30_MAX_BUFFER_SIZ= E_IN_BYTES) { + BlockAddress +=3D P30_MAX_BUFFER_SIZE_IN_BYTES; + Status =3D NorFlashWriteBuffer ( + Instance, + BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS), + P30_MAX_BUFFER_SIZE_IN_BYTES, + Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BY= TES + ); + if (EFI_ERROR (Status)) { + goto Exit; } } =20 - // Exit if we got here and could write all the data. Otherwise do the - // Erase-Write cycle. - if (!DoErase) { - return EFI_SUCCESS; - } - } +Exit: + // Put device back into Read Array mode + SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); =20 - // Check we did get some memory. Buffer is BlockSize. - if (Instance->ShadowBuffer =3D=3D NULL) { - DEBUG ((DEBUG_ERROR, "FvbWrite: ERROR - Buffer not ready\n")); - return EFI_DEVICE_ERROR; + return Status; } =20 +DoErase: // Read NOR Flash data into shadow buffer - TempStatus =3D NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->S= hadowBuffer); - if (EFI_ERROR (TempStatus)) { + Status =3D NorFlashReadBlocks (Instance, Lba, BlockSize, Instance->Shado= wBuffer); + if (EFI_ERROR (Status)) { // Return one of the pre-approved error statuses return EFI_DEVICE_ERROR; } @@ -776,8 +715,8 @@ NorFlashWriteSingleBlock ( CopyMem ((VOID *)((UINTN)Instance->ShadowBuffer + Offset), Buffer, *NumB= ytes); =20 // Write the modified buffer back to the NorFlash - TempStatus =3D NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->= ShadowBuffer); - if (EFI_ERROR (TempStatus)) { + Status =3D NorFlashWriteBlocks (Instance, Lba, BlockSize, Instance->Shad= owBuffer); + if (EFI_ERROR (Status)) { // Return one of the pre-approved error statuses return EFI_DEVICE_ERROR; } --=20 2.25.1 -=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 (#95094): https://edk2.groups.io/g/devel/message/95094 Mute This Topic: https://groups.io/mt/94278116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-