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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Thanks, that's useful background. @chasel, you should probably put this 
info in the commit message so it is captured in source history.

Given the default value is zero, it seems reasonable. I was also 
initially confused by the name of the PCD.

Another idea would be something like "PcdFlashNvStorageAdditionalSize".

Please do at least update the commit message to include additional context.

Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 2/9/2023 11:04 AM, Isaac Oram wrote:
> 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@linux.microsoft.com>
> Sent: Thursday, February 9, 2023 7:40 AM
> To: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Kubacki, Michael <michael.kubacki@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@intel.com>
>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>> Signed-off-by: Chasel Chiu <chasel.chiu@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
>>
> 
> 
> 
> 
>