> On Oct 8, 2015, at 3:11 PM, Laszlo Ersek <ler...@redhat.com> wrote:
>
> On 10/08/15 09:39, Star Zeng wrote:
>> PcdSet## has no error status returned, then the caller has no idea about
>> whether the set operation is successful or not.
>> PcdSet##S were added to return error status and PcdSet## APIs were put in
>> ifndef DISABLE_NEW_DEPRECATED_INTERFACES condition.
>> To adopt PcdSet##S and further code development with
>> DISABLE_NEW_DEPRECATED_INTERFACES defined, we need to Replace PcdSet## usage
>> with PcdSet##S.
>>
>> Normally, DynamicDefault PCD set is expected to be success, but DynamicHii
>> PCD set failure is a legal case.
>
> I almost had a question here ("why?"), but looking at
> "MdeModulePkg/Universal/PCD/Dxe/Pcd.inf", I know why -- because
> PcdsDynamicHii is stored in the non-volatile varstore.
>
>> For this case, PcdS3BootScriptTablePrivateDataPtr and
>> PcdS3BootScriptTablePrivateSmmDataPtr are expected to be DynamicDefault,
>> so use PcdSet64S to instead of PcdSet64 and assert when set failure.
>
> Okay, so this I don't understand.
>
> First, I don't think that any platform would like to store S3
> bootscript-related stuff in nv variables.
>
Historically I think there is an ACPI NVS memory region that the variable
pointed to (going from memory so I may be a little off).
The newer code uses the LockBox that is usually stored in SMM.
> Second, "MdeModulePkg/MdeModulePkg.dec" restricts these PCDs to
> [PcdsDynamic, PcdsDynamicEx], so no platform DSC can make them
> PcdsDynamicHii.
>
This is just confusing terminology/syntax….
From the DEC and INF point of view:
PcdDynamic means you use the PCD PPI/Protocol with the build generated Token
PcdDynamicEx means you use the PCD PPI/Protocol with the fixed Token and
EFI_GUID from the .DEC.
The DSC file also has a knob that controls how dynamic PCDs are stored. So
PcdsDynamicHii is a PcdDynamic form, but the driver that implements the
PPI/Protocol stores the data in an alternate location.
Thanks,
Andrew Fish
> Therefore, I guess the patch is correct, but why is it necessary? Is it
> just to be compatible with DISABLE_NEW_DEPRECATED_INTERFACES?
>
> Ah, I see. The commit message has three parts / arguments actually:
>
> (1) general description of status checking vs. non-checking interfaces
> (2) argument why we should move to checked interfaces
> (3) argument why the check will always succeed.
>
> Thanks
> Laszlo
>
>
>>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Liming Gao <liming....@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.z...@intel.com>
>> ---
>> MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> index 24c6798..e7d2a24 100644
>> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> @@ -454,7 +454,8 @@ S3BootScriptLibInitialize (
>> }
>> S3TablePtr = (VOID *) (UINTN) Buffer;
>>
>> - PcdSet64 (PcdS3BootScriptTablePrivateDataPtr, (UINT64)
>> (UINTN)S3TablePtr);
>> + Status = PcdSet64S (PcdS3BootScriptTablePrivateDataPtr, (UINT64)
>> (UINTN)S3TablePtr);
>> + ASSERT_EFI_ERROR (Status);
>> ZeroMem (S3TablePtr, sizeof(SCRIPT_TABLE_PRIVATE_DATA));
>> //
>> // Create event to notify the library system enter the SmmLocked phase.
>> @@ -506,7 +507,8 @@ S3BootScriptLibInitialize (
>> return RETURN_OUT_OF_RESOURCES;
>> }
>>
>> - PcdSet64 (PcdS3BootScriptTablePrivateSmmDataPtr, (UINT64)
>> (UINTN)S3TableSmmPtr);
>> + Status = PcdSet64S (PcdS3BootScriptTablePrivateSmmDataPtr, (UINT64)
>> (UINTN)S3TableSmmPtr);
>> + ASSERT_EFI_ERROR (Status);
>> ZeroMem (S3TableSmmPtr, sizeof(SCRIPT_TABLE_PRIVATE_DATA));
>>
>> //
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
