[edk2-devel] [edk2-platforms: PATCH] IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.

[Isaac Oram]

It is a legacy that exists in current and past implementations.  There is a complex arbitrary relationship between the runtime updateable regions in existing platform designs.
There is something like:
- Variable store (large)
- Error log (small)
- Fault tolerant working area (>= size of prior 2 regions)
- Fault tolerant metadata (small).
And there are assumptions about ordering and packing built into board flash layouts.

I don't think that we should introduce "other variable" as a concept, because variable solutions don't support two regions, so it isn't a UEFI variable region.  OtherUpdatable might be ok, but still seems confusing to me.
I think that we should add the support for the *ErrorLog* region so that the open FvbServices can be used by current implementations.  Then we should eliminate the "ErrorLog" use completely.  My thought is that this makes the connection to legacy clear.  And also motivates us to eliminate all the vestigial references to the ErrorLog in edk2 and edk2-platforms.

New updateable regions should not be hard-coded into this area and should have a cleaner solution, as Michael suggests.

I understand if we don't want to support legacy or workarounds, but I think that currently adoption and use of the open content is higher priority.  Which is why we are requesting this workaround to match "proprietary" FVB services behavior.

Regards,
Isaac

-----Original Message-----
From: Michael Kubacki <mikuback at linux.microsoft.com> 
Sent: Thursday, February 9, 2023 7:40 AM
To: devel at edk2.groups.io; Chiu, Chasel <chasel.chiu at intel.com>
Cc: S, Ashraf Ali <ashraf.ali.s at intel.com>; Oram, Isaac W <isaac.w.oram at intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Ni, Ray <ray.ni at intel.com>; Kubacki, Michael <michael.kubacki at microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms: PATCH] IntelSiliconPkg/SpiFvbServiceSmm: Support Other NVS variable region.

Is there a reason this other content can't go into it's own FV?

On 2/9/2023 12:14 AM, Chiu, Chasel wrote:
> Platform may implement Other NVS variable region following Regular 
> variable region and in this case SpiFvbService should include both 
> region size when calculating the total NVS region size.
> 
> One usage model is EventLog NVS region and there could be others.
> 
> Cc: Ashraf Ali S <ashraf.ali.s at intel.com>
> Cc: Isaac Oram <isaac.w.oram at intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Michael Kubacki <michael.kubacki at microsoft.com>
> Signed-off-by: Chasel Chiu <chasel.chiu at intel.com>
> ---
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c | 7 +++++++
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf  | 7 ++++---
>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec                               | 8 ++++++++
>   3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> index 942abf95a6..bcde98131d 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.c
> @@ -568,6 +568,13 @@ GetVariableFvInfo (
>       return;
> 
>     }
> 
>   
> 
> +  //
> 
> +  // GetVariableFlashNvStorageInfo () only reports regular variable 
> + region information,
> 
> +  // if platform implemented a separate Other variable region 
> + following the regular variable region,
> 
> +  // the size should be included as overall NVS variable region size.
> 
> +  //
> 
> +  NvStoreLength += PcdGet32 (PcdFlashNvStorageOtherVariableSize);
> 
> +
> 
>     Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress, &Length64);
> 
>     if (!EFI_ERROR (Status)) {
> 
>       // Stay within the current UINT32 size assumptions in the variable stack.
> 
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> index 73049eceb2..f40067418a 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceSmm.inf
> @@ -43,9 +43,10 @@
>     IntelSiliconPkg/IntelSiliconPkg.dec
> 
>   
> 
>   [Pcd]
> 
> -  gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
> 
> -  gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
> 
> -  gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType       ## SOMETIMES_CONSUMES
> 
> +  gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase            ## CONSUMES
> 
> +  gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize            ## CONSUMES
> 
> +  gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType          ## SOMETIMES_CONSUMES
> 
> +  gIntelSiliconPkgTokenSpaceGuid.PcdFlashNvStorageOtherVariableSize 
> + ## CONSUMES
> 
>   
> 
>   [Sources]
> 
>     FvbInfo.c
> 
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec 
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> index 63dae756ad..7034ab93b0 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> @@ -194,3 +194,11 @@
>     #  Other value: reserved for future use.<BR>
> 
>     # @Prompt Flash Variable Store type.
> 
>     
> gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType|0x00|UINT8|0x
> 0000000E
> 
> +
> 
> +  ## Declares Separate NVS Variable Region Size.<BR><BR>
> 
> +  #  Platform may implement a Regular variable region and an Other 
> + variable region, which will require this PCD
> 
> +  #  to tell SpiFvbService to include both regions.<BR>
> 
> +  #  0: No separate Other variable region.<BR>
> 
> +  #  non-zero: The size of a separate Other variable region following 
> + the Regular variable region.<BR>
> 
> +  # @Prompt Separate NVS Variable Region Size.
> 
> +  
> + gIntelSiliconPkgTokenSpaceGuid.PcdFlashNvStorageOtherVariableSize|0x
> + 00000000|UINT32|0x0000000F
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99907): https://edk2.groups.io/g/devel/message/99907
Mute This Topic: https://groups.io/mt/96847771/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-