[PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment


Sean Rhodes
 

Hi

Would any one be able to review please?

Thank you

On Fri, 1 Apr 2022, 09:03 Sean Rhodes via groups.io, <sean=starlabs.systems@groups.io> wrote:
WorkSpaceAddress and SpareAreaAddress point into MMIO, which isn't
always aligned. Remove the check for block alignment to avoid
false assertions.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Sean Rhodes <sean@...>
Change-Id: Ia1c1f44b6a0e7f32cac0d7806e74d729e5d83a6d
---
 .../Universal/FaultTolerantWriteDxe/FtwMisc.c    | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
index 661e148767..3b9ff1c828 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
@@ -1121,12 +1121,10 @@ FindFvbForFtw (
           FtwDevice->NumberOfWorkSpaceBlock = FTW_BLOCKS (FtwDevice->FtwWorkSpaceBase + FtwDevice->FtwWorkSpaceSize, FtwDevice->WorkBlockSize);
           if (FtwDevice->FtwWorkSpaceSize >= FtwDevice->WorkBlockSize) {
             //
-            // Check the alignment of work space address and length, they should be block size aligned when work space size is larger than one block size.
+            // Check the alignment of work space length, it should be block size aligned when work space size is larger than one block size.
             //
-            if (((FtwDevice->WorkSpaceAddress & (FtwDevice->WorkBlockSize - 1)) != 0) ||
-                ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize - 1)) != 0))
-            {
-              DEBUG ((DEBUG_ERROR, "Ftw: Work space address or length is not block size aligned when work space size is larger than one block size\n"));
+            if ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize - 1)) != 0) {
+              DEBUG ((EFI_D_ERROR, "Ftw: Work space length is not block size aligned when work space size is larger than one block size\n"));
               FreePool (HandleBuffer);
               ASSERT (FALSE);
               return EFI_ABORTED;
@@ -1171,12 +1169,10 @@ FindFvbForFtw (
           }

           //
-          // Check the alignment of spare area address and length, they should be block size aligned
+          // Check the alignment of spare area length, it should be block size aligned
           //
-          if (((FtwDevice->SpareAreaAddress & (FtwDevice->SpareBlockSize - 1)) != 0) ||
-              ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - 1)) != 0))
-          {
-            DEBUG ((DEBUG_ERROR, "Ftw: Spare area address or length is not block size aligned\n"));
+          if ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - 1)) != 0) {
+            DEBUG ((EFI_D_ERROR, "Ftw: Spare area address or length is not block size aligned\n"));
             FreePool (HandleBuffer);
             //
             // Report Status Code EFI_SW_EC_ABORTED.
--
2.32.0



------------
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88320): https://edk2.groups.io/g/devel/message/88320
Mute This Topic: https://groups.io/mt/90173290/6718866
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [sean@...]
------------



Wu, Hao A
 

Sorry for a question.

 

I referred the code in InitFtwDevice():

  FtwDevice->WorkSpaceAddress = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdFlashNvStorageFtwWorkingBase64);

  if (FtwDevice->WorkSpaceAddress == 0) {

    FtwDevice->WorkSpaceAddress = (EFI_PHYSICAL_ADDRESS)PcdGet32 (PcdFlashNvStorageFtwWorkingBase);

  }

 

and the PCD definition in MdeModulePkg.dec:

  ## Base address of the FTW working block range in flash device.

  # If PcdFlashNvStorageFtwWorkingSize is larger than one block size, this value should be block size aligned.

  # @Prompt Base address of flash FTW working block range.

  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0|UINT32|0x30000010

 

  ## 64-bit Base address of the FTW working block range in flash device.

  # If PcdFlashNvStorageFtwWorkingSize is larger than one block size, this value should be block size aligned.

  # @Prompt 64-bit Base address of flash FTW working block range.

  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0x0|UINT64|0x80000010

 

The description of both PCDs mentioned a block size alignment requirement.

Does the change in this patch conflict with the above PCD description?

 

(SpareAreaAddress is having a similar case.)

 

Best Regards,

Hao Wu

 

From: Sean Rhodes <sean@...>
Sent: Monday, May 16, 2022 1:41 PM
To: devel@edk2.groups.io; Rhodes, Sean <sean@...>
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Gao, Liming <gaoliming@...>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi

 

Would any one be able to review please?

 

Thank you

 

On Fri, 1 Apr 2022, 09:03 Sean Rhodes via groups.io, <sean=starlabs.systems@groups.io> wrote:

WorkSpaceAddress and SpareAreaAddress point into MMIO, which isn't
always aligned. Remove the check for block alignment to avoid
false assertions.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Sean Rhodes <sean@...>
Change-Id: Ia1c1f44b6a0e7f32cac0d7806e74d729e5d83a6d
---
 .../Universal/FaultTolerantWriteDxe/FtwMisc.c    | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
index 661e148767..3b9ff1c828 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
@@ -1121,12 +1121,10 @@ FindFvbForFtw (
           FtwDevice->NumberOfWorkSpaceBlock = FTW_BLOCKS (FtwDevice->FtwWorkSpaceBase + FtwDevice->FtwWorkSpaceSize, FtwDevice->WorkBlockSize);
           if (FtwDevice->FtwWorkSpaceSize >= FtwDevice->WorkBlockSize) {
             //
-            // Check the alignment of work space address and length, they should be block size aligned when work space size is larger than one block size.
+            // Check the alignment of work space length, it should be block size aligned when work space size is larger than one block size.
             //
-            if (((FtwDevice->WorkSpaceAddress & (FtwDevice->WorkBlockSize - 1)) != 0) ||
-                ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize - 1)) != 0))
-            {
-              DEBUG ((DEBUG_ERROR, "Ftw: Work space address or length is not block size aligned when work space size is larger than one block size\n"));
+            if ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize - 1)) != 0) {
+              DEBUG ((EFI_D_ERROR, "Ftw: Work space length is not block size aligned when work space size is larger than one block size\n"));
               FreePool (HandleBuffer);
               ASSERT (FALSE);
               return EFI_ABORTED;
@@ -1171,12 +1169,10 @@ FindFvbForFtw (
           }

           //
-          // Check the alignment of spare area address and length, they should be block size aligned
+          // Check the alignment of spare area length, it should be block size aligned
           //
-          if (((FtwDevice->SpareAreaAddress & (FtwDevice->SpareBlockSize - 1)) != 0) ||
-              ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - 1)) != 0))
-          {
-            DEBUG ((DEBUG_ERROR, "Ftw: Spare area address or length is not block size aligned\n"));
+          if ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - 1)) != 0) {
+            DEBUG ((EFI_D_ERROR, "Ftw: Spare area address or length is not block size aligned\n"));
             FreePool (HandleBuffer);
             //
             // Report Status Code EFI_SW_EC_ABORTED.
--
2.32.0



------------
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88320): https://edk2.groups.io/g/devel/message/88320
Mute This Topic: https://groups.io/mt/90173290/6718866
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [sean@...]
------------


Sean Rhodes
 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Sean Rhodes
 

WorkSpaceAddress and SpareAreaAddress point into MMIO, which isn't
always aligned. Remove the check for block alignment to avoid
false assertions.

Signed-off-by: Sean Rhodes <sean@...>
Change-Id: Ia1c1f44b6a0e7f32cac0d7806e74d729e5d83a6d
---
MdeModulePkg/MdeModulePkg.dec | 2 --
MdeModulePkg/MdeModulePkg.uni | 4 ++--
.../Universal/FaultTolerantWriteDxe/FtwMisc.c | 20 ++++++++-----------
3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index cf79292ec8..b7e2f48028 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1649,7 +1649,6 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x0|UINT32|=
0x30000014=0D
=0D
## Base address of the FTW working block range in flash device.=0D
- # If PcdFlashNvStorageFtwWorkingSize is larger than one block size, this=
value should be block size aligned.=0D
# @Prompt Base address of flash FTW working block range.=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0x0|UINT3=
2|0x30000010=0D
=0D
@@ -1668,7 +1667,6 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0x0|UINT6=
4|0x80000013=0D
=0D
## 64-bit Base address of the FTW working block range in flash device.=0D
- # If PcdFlashNvStorageFtwWorkingSize is larger than one block size, this=
value should be block size aligned.=0D
# @Prompt 64-bit Base address of flash FTW working block range.=0D
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0x0|UIN=
T64|0x80000010=0D
=0D
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index b070f15ff2..9f916506f7 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -374,7 +374,7 @@
=0D
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
_PROMPT #language en-US "Base address of flash FTW working block range"=0D
=0D
-#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
_HELP #language en-US "Base address of the FTW working block range in flas=
h device. If PcdFlashNvStorageFtwWorkingSize is larger than one block size,=
this value should be block size aligned."=0D
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
_HELP #language en-US "Base address of the FTW working block range in flas=
h device."=0D
=0D
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingSize=
_PROMPT #language en-US "Size of flash FTW working block range"=0D
=0D
@@ -390,7 +390,7 @@
=0D
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
64_PROMPT #language en-US "64-bit Base address of flash FTW working block =
range"=0D
=0D
-#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
64_HELP #language en-US "64-bit Base address of the FTW working block rang=
e in flash device. If PcdFlashNvStorageFtwWorkingSize is larger than one bl=
ock size, this value should be block size aligned."=0D
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdFlashNvStorageFtwWorkingBase=
64_HELP #language en-US "64-bit Base address of the FTW working block rang=
e in flash device."=0D
=0D
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdEmuVariableNvModeEnable_PROM=
PT #language en-US "EMU variable NV mode enable"=0D
=0D
diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c b/MdeMo=
dulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
index 661e148767..2fce694f22 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c
@@ -1108,8 +1108,8 @@ FindFvbForFtw (
// To get the LBA of work space=0D
//=0D
for (LbaIndex =3D 1; LbaIndex <=3D NumberOfBlocks; LbaIndex +=3D 1) =
{=0D
- if ( (FtwDevice->WorkSpaceAddress >=3D (FvbBaseAddress + BlockSiz=
e * (LbaIndex - 1)))=0D
- && (FtwDevice->WorkSpaceAddress < (FvbBaseAddress + BlockSize *=
LbaIndex)))=0D
+ if ((FtwDevice->WorkSpaceAddress - FvbBaseAddress >=3D BlockSize *=
(LbaIndex - 1)) &&=0D
+ ((FtwDevice->WorkSpaceAddress - FvbBaseAddress) / BlockSize >=
=3D LbaIndex - 1))=0D
{=0D
FtwDevice->FtwWorkSpaceLba =3D LbaIndex - 1;=0D
//=0D
@@ -1121,12 +1121,10 @@ FindFvbForFtw (
FtwDevice->NumberOfWorkSpaceBlock =3D FTW_BLOCKS (FtwDevice->Ftw=
WorkSpaceBase + FtwDevice->FtwWorkSpaceSize, FtwDevice->WorkBlockSize);=0D
if (FtwDevice->FtwWorkSpaceSize >=3D FtwDevice->WorkBlockSize) {=
=0D
//=0D
- // Check the alignment of work space address and length, they =
should be block size aligned when work space size is larger than one block =
size.=0D
+ // Check the alignment of work space length, it should be bloc=
k size aligned when work space size is larger than one block size.=0D
//=0D
- if (((FtwDevice->WorkSpaceAddress & (FtwDevice->WorkBlockSize =
- 1)) !=3D 0) ||=0D
- ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize -=
1)) !=3D 0))=0D
- {=0D
- DEBUG ((DEBUG_ERROR, "Ftw: Work space address or length is n=
ot block size aligned when work space size is larger than one block size\n"=
));=0D
+ if ((FtwDevice->WorkSpaceLength & (FtwDevice->WorkBlockSize - =
1)) !=3D 0) {=0D
+ DEBUG ((EFI_D_ERROR, "Ftw: Work space length is not block si=
ze aligned when work space size is larger than one block size\n"));=0D
FreePool (HandleBuffer);=0D
ASSERT (FALSE);=0D
return EFI_ABORTED;=0D
@@ -1171,12 +1169,10 @@ FindFvbForFtw (
}=0D
=0D
//=0D
- // Check the alignment of spare area address and length, they sh=
ould be block size aligned=0D
+ // Check the alignment of spare area length, it should be block =
size aligned=0D
//=0D
- if (((FtwDevice->SpareAreaAddress & (FtwDevice->SpareBlockSize -=
1)) !=3D 0) ||=0D
- ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - =
1)) !=3D 0))=0D
- {=0D
- DEBUG ((DEBUG_ERROR, "Ftw: Spare area address or length is not=
block size aligned\n"));=0D
+ if ((FtwDevice->SpareAreaLength & (FtwDevice->SpareBlockSize - 1=
)) !=3D 0) {=0D
+ DEBUG ((EFI_D_ERROR, "Ftw: Spare area address or length is not=
block size aligned\n"));=0D
FreePool (HandleBuffer);=0D
//=0D
// Report Status Code EFI_SW_EC_ABORTED.=0D
--=20
2.34.1


Wu, Hao A
 

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Sean Rhodes
 

The bug discovered was with coreboot, and the PCD values are derived from the block size of its SMMStore (NvStorage) region. The discussion on the patch can be found here: https://review.coreboot.org/c/coreboot/+/62990

Hacking the PCDs could work,, but why would we want to keep an incorrect check?

Thanks!


On Mon, 16 May 2022 at 08:36, Wu, Hao A <hao.a.wu@...> wrote:

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Wu, Hao A
 

Sorry Star and Liming,

 

For the below patch (removing the alignment check for WorkSpace & SpareArea):

https://edk2.groups.io/g/devel/message/89742

 

Do you think it will impact the FTW service on flash device? Thanks in advance.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:54 PM
To: Wu, Hao A <hao.a.wu@...>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

The bug discovered was with coreboot, and the PCD values are derived from the block size of its SMMStore (NvStorage) region. The discussion on the patch can be found here: https://review.coreboot.org/c/coreboot/+/62990

 

Hacking the PCDs could work,, but why would we want to keep an incorrect check?

 

Thanks!

 

 

On Mon, 16 May 2022 at 08:36, Wu, Hao A <hao.a.wu@...> wrote:

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Sheng Lean Tan
 

Hi Star & Liming,
Any update on this?
Much appreciated.

Best Regards,
Lean Sheng Tan



9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany

Registered office: Bochum
Commercial register: Amtsgericht Bochum, HRB 17519
Management: Sebastian German, Eray Bazaar


On Mon, 16 May 2022 at 11:03, Wu, Hao A <hao.a.wu@...> wrote:

Sorry Star and Liming,

 

For the below patch (removing the alignment check for WorkSpace & SpareArea):

https://edk2.groups.io/g/devel/message/89742

 

Do you think it will impact the FTW service on flash device? Thanks in advance.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:54 PM
To: Wu, Hao A <hao.a.wu@...>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

The bug discovered was with coreboot, and the PCD values are derived from the block size of its SMMStore (NvStorage) region. The discussion on the patch can be found here: https://review.coreboot.org/c/coreboot/+/62990

 

Hacking the PCDs could work,, but why would we want to keep an incorrect check?

 

Thanks!

 

 

On Mon, 16 May 2022 at 08:36, Wu, Hao A <hao.a.wu@...> wrote:

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Zeng, Star
 

When length is larger than block size and block size aligned, if the address is not block size aligned, that means the range will mix with other range, but erase operation will be done per block, that will be risky and may break the fault tolerant mechanism.

I could not remember all the details. Personally, I do not think it is right way to remove the check.

 

 

Thanks,

Star

From: Lean Sheng Tan <sheng.tan@...>
Sent: Tuesday, May 17, 2022 7:58 PM
To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>
Cc: Zeng, Star <star.zeng@...>; Gao, Liming <gaoliming@...>; Rhodes, Sean <sean@...>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Star & Liming,

Any update on this?

Much appreciated.


Best Regards,

Lean Sheng Tan


9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany

 

Registered office: Bochum

Commercial register: Amtsgericht Bochum, HRB 17519

Management: Sebastian German, Eray Bazaar

 

 

On Mon, 16 May 2022 at 11:03, Wu, Hao A <hao.a.wu@...> wrote:

Sorry Star and Liming,

 

For the below patch (removing the alignment check for WorkSpace & SpareArea):

https://edk2.groups.io/g/devel/message/89742

 

Do you think it will impact the FTW service on flash device? Thanks in advance.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:54 PM
To: Wu, Hao A <hao.a.wu@...>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

The bug discovered was with coreboot, and the PCD values are derived from the block size of its SMMStore (NvStorage) region. The discussion on the patch can be found here: https://review.coreboot.org/c/coreboot/+/62990

 

Hacking the PCDs could work,, but why would we want to keep an incorrect check?

 

Thanks!

 

 

On Mon, 16 May 2022 at 08:36, Wu, Hao A <hao.a.wu@...> wrote:

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you


Sean Rhodes
 

Hi Star

I think the point is shown in a comment from coreboot:

"As mentioned above, only the offsets need to be
aligned, not the absolute bases. Please, have a look for instance at
`MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c:1111`:

  (FtwDevice->WorkSpaceAddress >= (FvbBaseAddress + BlockSize * (LbaIndex - 1)))
Things become more obvious if we remove the unnecessary parentheses:

  FtwDevice->WorkSpaceAddress >= FvbBaseAddress + BlockSize * (LbaIndex - 1)
It's the same as:

  FtwDevice->WorkSpaceAddress - FvbBaseAddress >= BlockSize * (LbaIndex - 1)
And _if_ aligned, the same as:

  (FtwDevice->WorkSpaceAddress - FvbBaseAddress) / BlockSize >= LbaIndex - 1
Now it's easy to see: neither `FtwDevice->WorkSpaceAddress` nor `FvbBaseAddress`
have to be aligned, but their relative distance has to be."

So if this solution isn't acceptable, could you suggest one that would be?

Many thanks

On Tue, 17 May 2022 at 16:05, Zeng, Star <star.zeng@...> wrote:

When length is larger than block size and block size aligned, if the address is not block size aligned, that means the range will mix with other range, but erase operation will be done per block, that will be risky and may break the fault tolerant mechanism.

I could not remember all the details. Personally, I do not think it is right way to remove the check.

 

 

Thanks,

Star

From: Lean Sheng Tan <sheng.tan@...>
Sent: Tuesday, May 17, 2022 7:58 PM
To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>
Cc: Zeng, Star <star.zeng@...>; Gao, Liming <gaoliming@...>; Rhodes, Sean <sean@...>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Star & Liming,

Any update on this?

Much appreciated.


Best Regards,

Lean Sheng Tan


9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany

 

Registered office: Bochum

Commercial register: Amtsgericht Bochum, HRB 17519

Management: Sebastian German, Eray Bazaar

 

 

On Mon, 16 May 2022 at 11:03, Wu, Hao A <hao.a.wu@...> wrote:

Sorry Star and Liming,

 

For the below patch (removing the alignment check for WorkSpace & SpareArea):

https://edk2.groups.io/g/devel/message/89742

 

Do you think it will impact the FTW service on flash device? Thanks in advance.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:54 PM
To: Wu, Hao A <hao.a.wu@...>
Cc: devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

The bug discovered was with coreboot, and the PCD values are derived from the block size of its SMMStore (NvStorage) region. The discussion on the patch can be found here: https://review.coreboot.org/c/coreboot/+/62990

 

Hacking the PCDs could work,, but why would we want to keep an incorrect check?

 

Thanks!

 

 

On Mon, 16 May 2022 at 08:36, Wu, Hao A <hao.a.wu@...> wrote:

Sorry for not being clear on what I mean.

Is it possible to change the platform PCD values and keep these block size alignment requirements.

 

Best Regards,

Hao Wu

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, May 16, 2022 3:00 PM
To: Wu; Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment

 

Hi Hao

Yes, it does conflict - I will update the patch to fix these comments :)

Thank you