[edk2] [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of PcdSet64

[Star Zeng]

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.
For this case, PcdS3BootScriptTablePrivateDataPtr and 
PcdS3BootScriptTablePrivateSmmDataPtr are expected to be DynamicDefault,
so use PcdSet64S to instead of PcdSet64 and assert when set failure.

Cc: Jiewen Yao 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 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));
 
 //
-- 
1.9.5.msysgit.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of PcdSet64

[Yao, Jiewen]

Looks good.

-Original Message-
From: Zeng, Star 
Sent: Thursday, October 08, 2015 3:40 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen; Gao, Liming
Subject: [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of 
PcdSet64

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.
For this case, PcdS3BootScriptTablePrivateDataPtr and 
PcdS3BootScriptTablePrivateSmmDataPtr are expected to be DynamicDefault, so use 
PcdSet64S to instead of PcdSet64 and assert when set failure.

Cc: Jiewen Yao 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 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));
 
 //
--
1.9.5.msysgit.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of PcdSet64

[Andrew Fish]

> On Oct 8, 2015, at 3:11 PM, Laszlo Ersek  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 
>> Cc: Liming Gao 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng 
>> ---
>> 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

Re: [edk2] [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of PcdSet64

[Laszlo Ersek]

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.

Second, "MdeModulePkg/MdeModulePkg.dec" restricts these PCDs to
[PcdsDynamic, PcdsDynamicEx], so no platform DSC can make them
PcdsDynamicHii.

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 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  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

Re: [edk2] [PATCH] MdeModulePkg PiDxeS3BootScriptLib: Use PcdSet64S to instead of PcdSet64

[Zeng, Star]

On 2015/10/9 6:33, Andrew Fish wrote:



On Oct 8, 2015, at 3:11 PM, Laszlo Ersek  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.


Right, thanks for the explanation.

Star



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 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
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




___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel