From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f181.google.com (mail-yb1-f181.google.com [209.85.219.181]) by mx.groups.io with SMTP id smtpd.web09.8657.1652800592316650964 for ; Tue, 17 May 2022 08:16:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@starlabs-systems.20210112.gappssmtp.com header.s=20210112 header.b=4TSuOzMi; spf=pass (domain: starlabs.systems, ip: 209.85.219.181, mailfrom: sean@starlabs.systems) Received: by mail-yb1-f181.google.com with SMTP id v71so7903829ybi.4 for ; Tue, 17 May 2022 08:16:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=starlabs-systems.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=shOkhIwR+tOs1fxFO+bEPWOQZmCwP2EO9/GU+vMq+xo=; b=4TSuOzMiYHfUdEWDgxAhPrS8zGuMBi48h2D2xpBkrxKvdnsB2G4GPv58V3nuwpQ5XJ 8xnfB6Avk1PFFYYQUBV1AE4K4dU+7mDCOSeF59GJ90m/NWpMuqQoJTwgz2ldZ+eHbAEF 2Nk3ivtk5KATLlxboXYOVq7bC2+Zplz5OMUaHrbMdHQ0bBjT+6ADGkGjcvCckWraU58P z86xN7h/WB1/s/GZnOJwV8HJETTVzRBkXs9IWQ1eBTbeavZ423NErMNMva3qzzQJ97VQ ZY6jqVnZPKhYGENbd/iC6XJH1VSbK1Fq7Mc4Lo5D2bSpT7XDRG4m95FOr9s+kyxvX086 L+SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=shOkhIwR+tOs1fxFO+bEPWOQZmCwP2EO9/GU+vMq+xo=; b=1ob2umWHh+CO7GL6g7P+RfqSzN6cscc9MMJG92hKgXxaBiVs25gKYVPeagrHtCluFU VJI/X2mzaVOEtlDHkuYeFADPMZP2tNaIF4BqSsCmlFYPqiY0lYNngnzlMUewcL2iApil UCszukASFmc9lr2KNS+TzyD8gp7AzEpO4ty4bXmTofdiA5uiGuDeF3J+DuKRnCwiSmBy /yizkxIseLHrV+vJdxz+k8Ru89RmAqL8NxcyJvoEScrqk9WzWxbgazY0lIcuvHdTb6ec NQI4rbHbwI3Qtax+bEVdFFMwxMk711tzhnMM1mgeUJKmwuXCexLElGdmSUTIPq19l03u kTVw== X-Gm-Message-State: AOAM533Bnjebyv7MBWqkTUf9bXTDe6qYxnSSxdsp9t5Ilpoxh4iP1ZEG demp0hhXNezbD8FBM5ENch98IsOdxQBEITGnrTfU X-Google-Smtp-Source: ABdhPJxDfZ2EwMKdWG4O9nxw+oZyUimyxB05v2AnQtKP/NsqewNi3XAznFalyngf0SG2VZSfHde8NkoYbm5+xqqO7f0= X-Received: by 2002:a25:ccd4:0:b0:64d:ae5f:e2a3 with SMTP id l203-20020a25ccd4000000b0064dae5fe2a3mr10719554ybf.125.1652800591267; Tue, 17 May 2022 08:16:31 -0700 (PDT) MIME-Version: 1.0 References: <2106.1652684379544394027@groups.io> In-Reply-To: From: "Sean Rhodes" Date: Tue, 17 May 2022 16:16:20 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe: Don't check for address alignment To: "Zeng, Star" Cc: "Tan, Lean Sheng" , "devel@edk2.groups.io" , "Wu, Hao A" , "Gao, Liming" Content-Type: multipart/alternative; boundary="00000000000020a06305df36a087" --00000000000020a06305df36a087 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 >=3D (FvbBaseAddress + BlockSize * (LbaIndex= - 1))) Things become more obvious if we remove the unnecessary parentheses: FtwDevice->WorkSpaceAddress >=3D FvbBaseAddress + BlockSize * (LbaIndex -= 1) It's the same as: FtwDevice->WorkSpaceAddress - FvbBaseAddress >=3D BlockSize * (LbaIndex -= 1) And _if_ aligned, the same as: (FtwDevice->WorkSpaceAddress - FvbBaseAddress) / BlockSize >=3D 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 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 oth= er > 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 > *Sent:* Tuesday, May 17, 2022 7:58 PM > *To:* devel@edk2.groups.io; Wu, Hao A > *Cc:* Zeng, Star ; Gao, Liming < > gaoliming@byosoft.com.cn>; Rhodes, 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=C3=9Fe 19-21, 44787 Bochum, Germany > > Email: sheng.tan@9elements.com > > Phone: *+49 234 68 94 188 <+492346894188>* > > Mobile: *+49 176 76 113842 <+4917676113842>* > > > > Registered office: Bochum > > Commercial register: Amtsgericht Bochum, HRB 17519 > > Management: Sebastian German, Eray Bazaar > > > Data protection information according to Art. 13 GDPR > > > > > > > On Mon, 16 May 2022 at 11:03, Wu, Hao A 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 *On Behalf Of *Sean > Rhodes > *Sent:* Monday, May 16, 2022 3:54 PM > *To:* Wu, Hao A > *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 wrote: > > Sorry for not being clear on what I mean. > > Is it possible to change the platform PCD values and keep these block siz= e > alignment requirements. > > > > Best Regards, > > Hao Wu > > > > *From:* devel@edk2.groups.io *On Behalf Of *Sean > Rhodes > *Sent:* Monday, May 16, 2022 3:00 PM > *To:* Wu; Wu, Hao A ; 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 > >=20 > > --00000000000020a06305df36a087 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Star

I think the point is shown in a co= mment from coreboot:

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

=C2=A0 (FtwDevice->WorkSpaceAddress >=3D (FvbBaseAddr= ess + BlockSize * (LbaIndex - 1)))
Things become more obvious if we remo= ve the unnecessary parentheses:

=C2=A0 FtwDevice->WorkSpaceAddres= s >=3D FvbBaseAddress + BlockSize * (LbaIndex - 1)
It's the same = as:

=C2=A0 FtwDevice->WorkSpaceAddress - FvbBaseAddress >=3D B= lockSize * (LbaIndex - 1)
And _if_ aligned, the same as:

=C2=A0 (= FtwDevice->WorkSpaceAddress - FvbBaseAddress) / BlockSize >=3D LbaInd= ex - 1
Now it's easy to see: neither `FtwDevice->WorkSpaceAddress= ` nor `FvbBaseAddress`
have to be aligned, but their relative distance h= as to be."

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

Many thanks

On T= ue, 17 May 2022 at 16:05, Zeng, Star <star.zeng@intel.com> wrote:

When length is larger than block size and block size= aligned, if the address is not block size aligned, that means the range wi= ll mix with other range, but erase operation will be done per block, that w= ill 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.

=C2=A0

=C2=A0

Thanks,

Star

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

=C2=A0

Hi Star & Liming,

Any update on this?

Much appreciated.


Best Regards,

Lean Sheng Tan


9elements GmbH, Kortumstra=C3=9Fe 19-21, 44787 Boc= hum, Germany

Email:=C2=A0sheng.tan@9= elements.com

Phone:=C2=A0= +49 234 68 94 188

Mobile:=C2=A0+49 176 76 113842

=C2=A0

Registered= office: Bochum

Commercial= register: Amtsgericht Bochum, HRB 17519

Management= : Sebastian German, Eray Bazaar

=C2=A0

=C2=A0

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

Sorry Star and Liming,

=C2=A0

For the below patch (removing the alignment check fo= r WorkSpace & SpareArea):

https://edk2.groups.io/g/devel/message/89742<= /u>

=C2=A0

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

=C2=A0

Best Regards,

Hao Wu

=C2=A0

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@intel.com>
Cc: devel@= edk2.groups.io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe= : Don't check for address alignment

=C2=A0

The bug discovered was with coreboot, and the PCD values are de= rived 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

=C2=A0

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

=C2=A0

Thanks!

=C2=A0

=C2=A0

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

Sorry for not being clear on what I mean.<= /u>

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

=C2=A0

Best Regards,

Hao Wu

=C2=A0

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@intel.com>; devel@edk2.groups= .io
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/FaultTolerantWriteDxe= : Don't check for address alignment

=C2=A0

Hi Hao

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

--00000000000020a06305df36a087--