[PATCH] SignedCapsulePkg/EdkiiSystemCapsuleLib: Fix logic error.

[PATCH] SignedCapsulePkg/EdkiiSystemCapsuleLib: Fix logic error.

[Jiewen Yao]

This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=367

Cc: Wang Cloud <winggundum82@163.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c b/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
index dfd8d10..62be8eb 100644
--- a/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
+++ b/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
@@ -643,7 +643,7 @@ CapsuleAuthenticateSystemFirmware (
       return EFI_SECURITY_VIOLATION;
     }
   } else {
-    if (CurrentImageFmpInfo->Version < ImageFmpInfo->LowestSupportedImageVersion) {
+    if (ImageFmpInfo->Version < CurrentImageFmpInfo->LowestSupportedImageVersion) {
       *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
       DEBUG((DEBUG_INFO, "LowestSupportedImageVersion check - fail\n"));
       return EFI_SECURITY_VIOLATION;
-- 
2.7.4.windows.1

Re: [PATCH] SignedCapsulePkg/EdkiiSystemCapsuleLib: Fix logic error.

[wang xiaofeng]

Hi Jiewen,
   The fix is OK  for me. Reviewed-by Wang,Cloud





At 2017-02-07 14:33:45, "Jiewen Yao" <jiewen.yao@intel.com> wrote:
>This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=367
>
>Cc: Wang Cloud <winggundum82@163.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
>---
> SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c b/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
>index dfd8d10..62be8eb 100644
>--- a/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
>+++ b/SignedCapsulePkg/Library/EdkiiSystemCapsuleLib/EdkiiSystemCapsuleLib.c
>@@ -643,7 +643,7 @@ CapsuleAuthenticateSystemFirmware (
>       return EFI_SECURITY_VIOLATION;
>     }
>   } else {
>-    if (CurrentImageFmpInfo->Version < ImageFmpInfo->LowestSupportedImageVersion) {
>+    if (ImageFmpInfo->Version < CurrentImageFmpInfo->LowestSupportedImageVersion) {
>       *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
>       DEBUG((DEBUG_INFO, "LowestSupportedImageVersion check - fail\n"));
>       return EFI_SECURITY_VIOLATION;
>-- 
>2.7.4.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
\x16&#65533;&

CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[wang xiaofeng]

Hi Jiewen, 
       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset��S3) in OS or application like CapsuleApp.
     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!


Done:
  for (Index = 0; Index < CapsuleNum; Index++) {
    if (CapsuleBuffer[Index] != NULL) {
      FreePool (CapsuleBuffer[Index]);
    }
  }


  CleanGatherList(BlockDescriptors, CapsuleNum);
  
   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case



\x16&#65533;&

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[Yao, Jiewen]

That is an interesting question.

A general rule for Capsule App is:

1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)

2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)

3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.

May I know where you think we have logic error?

Thank you
Yao Jiewen

From: wang xiaofeng [mailto:winggundum82@163.com]
Sent: Friday, February 10, 2017 1:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

Hi Jiewen,
       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset（S3) in OS or application like CapsuleApp.
     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!

Done:
  for (Index = 0; Index < CapsuleNum; Index++) {
    if (CapsuleBuffer[Index] != NULL) {
      FreePool (CapsuleBuffer[Index]);
    }
  }

  CleanGatherList(BlockDescriptors, CapsuleNum);

   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[wang xiaofeng]

Hi Jiewen,
     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.
But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?
The reasons are as the followings:
1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design
2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.


Anyhow , this is a new feature request and you can decide whether need to add this feature.










At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>That is an interesting question.
>
>A general rule for Capsule App is:
>
>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)
>
>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)
>
>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.
>
>May I know where you think we have logic error?
>
>Thank you
>Yao Jiewen
>
>From: wang xiaofeng [mailto:winggundum82@163.com]
>Sent: Friday, February 10, 2017 1:27 AM
>To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>Hi Jiewen,
>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset&#65533;&#65533;S3) in OS or application like CapsuleApp.
>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!
>
>Done:
>  for (Index = 0; Index < CapsuleNum; Index++) {
>    if (CapsuleBuffer[Index] != NULL) {
>      FreePool (CapsuleBuffer[Index]);
>    }
>  }
>
>  CleanGatherList(BlockDescriptors, CapsuleNum);
>
>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case
>
>
>
>
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[Yao, Jiewen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3997 bytes --]

I see your point.


1)       That is possible. To make the solution complete, I am thinking to add ¨CNR (no-reset) flag.

2)       If you just want to update multiple capsules at one time, current APP already support this feature.
You may use:
CapsuleApp Bios.Cap Me.Cap Bmc.Cap

Thank you
Yao Jiewen

From: wang xiaofeng [mailto:winggundum82@163.com]
Sent: Sunday, February 12, 2017 6:10 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

Hi Jiewen,
     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.
But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?
The reasons are as the followings:
1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design
2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.

Anyhow , this is a new feature request and you can decide whether need to add this feature.






At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:

>That is an interesting question.

>

>A general rule for Capsule App is:

>

>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)

>

>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)

>

>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.

>

>May I know where you think we have logic error?

>

>Thank you

>Yao Jiewen

>

>From: wang xiaofeng [mailto:winggundum82@163.com]

>Sent: Friday, February 10, 2017 1:27 AM

>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>

>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

>

>Hi Jiewen,

>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset£¨S3) in OS or application like CapsuleApp.

>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!

>

>Done:

>  for (Index = 0; Index < CapsuleNum; Index++) {

>    if (CapsuleBuffer[Index] != NULL) {

>      FreePool (CapsuleBuffer[Index]);

>    }

>  }

>

>  CleanGatherList(BlockDescriptors, CapsuleNum);

>

>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case

>

>

>

>

>

>_______________________________________________

>edk2-devel mailing list

>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>

>https://lists.01.org/mailman/listinfo/edk2-devel

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[wang xiaofeng]

Hi Jiewen,
   Thanks ! I believe �CNR flag will be helpful to me.








At 2017-02-13 11:52:50, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>I see your point.
>
>
>1)       That is possible. To make the solution complete, I am thinking to add &#65533;CNR (no-reset) flag.
>
>2)       If you just want to update multiple capsules at one time, current APP already support this feature.
>You may use:
>CapsuleApp Bios.Cap Me.Cap Bmc.Cap
>
>Thank you
>Yao Jiewen
>
>From: wang xiaofeng [mailto:winggundum82@163.com]
>Sent: Sunday, February 12, 2017 6:10 PM
>To: Yao, Jiewen <jiewen.yao@intel.com>
>Cc: edk2-devel@lists.01.org
>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>Hi Jiewen,
>     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.
>But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?
>The reasons are as the followings:
>1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design
>2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.
>
>Anyhow , this is a new feature request and you can decide whether need to add this feature.
>
>
>
>
>
>
>At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
>>That is an interesting question.
>
>>
>
>>A general rule for Capsule App is:
>
>>
>
>>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)
>
>>
>
>>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)
>
>>
>
>>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.
>
>>
>
>>May I know where you think we have logic error?
>
>>
>
>>Thank you
>
>>Yao Jiewen
>
>>
>
>>From: wang xiaofeng [mailto:winggundum82@163.com]
>
>>Sent: Friday, February 10, 2017 1:27 AM
>
>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>
>>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>>
>
>>Hi Jiewen,
>
>>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset&#65533;&#65533;S3) in OS or application like CapsuleApp.
>
>>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!
>
>>
>
>>Done:
>
>>  for (Index = 0; Index < CapsuleNum; Index++) {
>
>>    if (CapsuleBuffer[Index] != NULL) {
>
>>      FreePool (CapsuleBuffer[Index]);
>
>>    }
>
>>  }
>
>>
>
>>  CleanGatherList(BlockDescriptors, CapsuleNum);
>
>>
>
>>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case
>
>>
>
>>
>
>>
>
>>
>
>>
>
>>_______________________________________________
>
>>edk2-devel mailing list
>
>>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>
>>https://lists.01.org/mailman/listinfo/edk2-devel
>
>
>

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[Yao, Jiewen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 5205 bytes --]

Would you please help to file a bugzillar for tracking purpose?

Thank you
Yao Jiewen

From: wang xiaofeng [mailto:winggundum82@163.com]
Sent: Sunday, February 12, 2017 8:46 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

Hi Jiewen,
   Thanks ! I believe ¨CNR flag will be helpful to me.





At 2017-02-13 11:52:50, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:

>I see your point.

>

>

>1)       That is possible. To make the solution complete, I am thinking to add ¨CNR (no-reset) flag.

>

>2)       If you just want to update multiple capsules at one time, current APP already support this feature.

>You may use:

>CapsuleApp Bios.Cap Me.Cap Bmc.Cap

>

>Thank you

>Yao Jiewen

>

>From: wang xiaofeng [mailto:winggundum82@163.com]

>Sent: Sunday, February 12, 2017 6:10 PM

>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>

>Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>

>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

>

>Hi Jiewen,

>     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.

>But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?

>The reasons are as the followings:

>1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design

>2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.

>

>Anyhow , this is a new feature request and you can decide whether need to add this feature.

>

>

>

>

>

>

>At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> wrote:

>

>>That is an interesting question.

>

>>

>

>>A general rule for Capsule App is:

>

>>

>

>>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)

>

>>

>

>>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)

>

>>

>

>>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.

>

>>

>

>>May I know where you think we have logic error?

>

>>

>

>>Thank you

>

>>Yao Jiewen

>

>>

>

>>From: wang xiaofeng [mailto:winggundum82@163.com]

>

>>Sent: Friday, February 10, 2017 1:27 AM

>

>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>

>

>>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

>

>>

>

>>Hi Jiewen,

>

>>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset£¨S3) in OS or application like CapsuleApp.

>

>>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!

>

>>

>

>>Done:

>

>>  for (Index = 0; Index < CapsuleNum; Index++) {

>

>>    if (CapsuleBuffer[Index] != NULL) {

>

>>      FreePool (CapsuleBuffer[Index]);

>

>>    }

>

>>  }

>

>>

>

>>  CleanGatherList(BlockDescriptors, CapsuleNum);

>

>>

>

>>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case

>

>>

>

>>

>

>>

>

>>

>

>>

>

>>_______________________________________________

>

>>edk2-devel mailing list

>

>>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>

>

>>https://lists.01.org/mailman/listinfo/edk2-devel

>

>

>

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[wang xiaofeng]

Hi JIewen,
   I am OOO today, I will create a bugzillar next monday.







At 2017-02-17 09:23:08, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>Would you please help to file a bugzillar for tracking purpose?
>
>Thank you
>Yao Jiewen
>
>From: wang xiaofeng [mailto:winggundum82@163.com]
>Sent: Sunday, February 12, 2017 8:46 PM
>To: Yao, Jiewen <jiewen.yao@intel.com>
>Cc: edk2-devel@lists.01.org
>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>Hi Jiewen,
>   Thanks ! I believe &#65533;CNR flag will be helpful to me.
>
>
>
>
>
>At 2017-02-13 11:52:50, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
>>I see your point.
>
>>
>
>>
>
>>1)       That is possible. To make the solution complete, I am thinking to add &#65533;CNR (no-reset) flag.
>
>>
>
>>2)       If you just want to update multiple capsules at one time, current APP already support this feature.
>
>>You may use:
>
>>CapsuleApp Bios.Cap Me.Cap Bmc.Cap
>
>>
>
>>Thank you
>
>>Yao Jiewen
>
>>
>
>>From: wang xiaofeng [mailto:winggundum82@163.com]
>
>>Sent: Sunday, February 12, 2017 6:10 PM
>
>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>
>>Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>
>>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>>
>
>>Hi Jiewen,
>
>>     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.
>
>>But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?
>
>>The reasons are as the followings:
>
>>1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design
>
>>2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.
>
>>
>
>>Anyhow , this is a new feature request and you can decide whether need to add this feature.
>
>>
>
>>
>
>>
>
>>
>
>>
>
>>
>
>>At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> wrote:
>
>>
>
>>>That is an interesting question.
>
>>
>
>>>
>
>>
>
>>>A general rule for Capsule App is:
>
>>
>
>>>
>
>>
>
>>>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)
>
>>
>
>>>
>
>>
>
>>>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)
>
>>
>
>>>
>
>>
>
>>>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.
>
>>
>
>>>
>
>>
>
>>>May I know where you think we have logic error?
>
>>
>
>>>
>
>>
>
>>>Thank you
>
>>
>
>>>Yao Jiewen
>
>>
>
>>>
>
>>
>
>>>From: wang xiaofeng [mailto:winggundum82@163.com]
>
>>
>
>>>Sent: Friday, February 10, 2017 1:27 AM
>
>>
>
>>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>
>>
>
>>>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>>
>
>>>
>
>>
>
>>>Hi Jiewen,
>
>>
>
>>>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset&#65533;&#65533;S3) in OS or application like CapsuleApp.
>
>>
>
>>>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!
>
>>
>
>>>
>
>>
>
>>>Done:
>
>>
>
>>>  for (Index = 0; Index < CapsuleNum; Index++) {
>
>>
>
>>>    if (CapsuleBuffer[Index] != NULL) {
>
>>
>
>>>      FreePool (CapsuleBuffer[Index]);
>
>>
>
>>>    }
>
>>
>
>>>  }
>
>>
>
>>>
>
>>
>
>>>  CleanGatherList(BlockDescriptors, CapsuleNum);
>
>>
>
>>>
>
>>
>
>>>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>_______________________________________________
>
>>
>
>>>edk2-devel mailing list
>
>>
>
>>>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>
>>
>
>>>https://lists.01.org/mailman/listinfo/edk2-devel
>
>>
>
>>
>
>>
>
>
>

Re: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule

[wang xiaofeng]

HI Jiewen,


Bug 388 - is  submitted for this issue








At 2017-02-17 09:23:08, "Yao, Jiewen" <jiewen.yao@intel.com> wrote:
>Would you please help to file a bugzillar for tracking purpose?
>
>Thank you
>Yao Jiewen
>
>From: wang xiaofeng [mailto:winggundum82@163.com]
>Sent: Sunday, February 12, 2017 8:46 PM
>To: Yao, Jiewen <jiewen.yao@intel.com>
>Cc: edk2-devel@lists.01.org
>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>Hi Jiewen,
>   Thanks ! I believe &#65533;CNR flag will be helpful to me.
>
>
>
>
>
>At 2017-02-13 11:52:50, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
>>I see your point.
>
>>
>
>>
>
>>1)       That is possible. To make the solution complete, I am thinking to add &#65533;CNR (no-reset) flag.
>
>>
>
>>2)       If you just want to update multiple capsules at one time, current APP already support this feature.
>
>>You may use:
>
>>CapsuleApp Bios.Cap Me.Cap Bmc.Cap
>
>>
>
>>Thank you
>
>>Yao Jiewen
>
>>
>
>>From: wang xiaofeng [mailto:winggundum82@163.com]
>
>>Sent: Sunday, February 12, 2017 6:10 PM
>
>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>
>>Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>
>>Subject: Re:Re: [edk2] CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>>
>
>>Hi Jiewen,
>
>>     Sorry, I forgot that I modified Capsuleapp before, Trunk Caspuleapp behavior is like you said" If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will trigger reset". The logic doesn;t have errors.
>
>>But the behavior is a little different from my understanding . Could you think if we can change the behavior to " If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, capsule app will NOT trigger reset,let user trigger the reset later"?
>
>>The reasons are as the followings:
>
>>1) user cannot distinguish whether the reset is triggered by bios code or application code , there is no difference for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule and CAPSULE_FLAGS_PERSIST_ACROSS_RESET + CAPSULE_FLAGS_INITIATE_RESET capsule in current design
>
>>2) user may have to update bios capsule , ME capsule ,BMC capsule together,otherwise system cannot work in next reboot. So after bios capsule update done , a reset is not expected. User want to update it later after all capsule are updated.
>
>>
>
>>Anyhow , this is a new feature request and you can decide whether need to add this feature.
>
>>
>
>>
>
>>
>
>>
>
>>
>
>>
>
>>At 2017-02-10 19:24:55, "Yao, Jiewen" <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>> wrote:
>
>>
>
>>>That is an interesting question.
>
>>
>
>>>
>
>>
>
>>>A general rule for Capsule App is:
>
>>
>
>>>
>
>>
>
>>>1)       If a capsule has CAPSULE_FLAGS_INITIATE_RESET, the CapsuleService will issue reset. No need to handler in CapsuleApp. (The free memory logic will not run)
>
>>
>
>>>
>
>>
>
>>>2)       If a capsule has CAPSULE_FLAGS_PERSIST_ACROSS_RESET, the CapsuleService will not issue reset, because CapsuleService will let caller decide when to reset. So here CapsuleApp does reset. (The free memory logic will not run)
>
>>
>
>>>
>
>>
>
>>>3)       If a capsule has no CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, the CapsuleService processes the image immediately. Then CapsuleApp can free the buffer finally, because it is already processed.
>
>>
>
>>>
>
>>
>
>>>May I know where you think we have logic error?
>
>>
>
>>>
>
>>
>
>>>Thank you
>
>>
>
>>>Yao Jiewen
>
>>
>
>>>
>
>>
>
>>>From: wang xiaofeng [mailto:winggundum82@163.com]
>
>>
>
>>>Sent: Friday, February 10, 2017 1:27 AM
>
>>
>
>>>To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>
>>
>
>>>Subject: CapsuleApp behavior for CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule
>
>>
>
>>>
>
>>
>
>>>Hi Jiewen,
>
>>
>
>>>       I think it might be a logic error for CapsuleApp  with CAPSULE_FLAGS_PERSIST_ACROSS_RESET only capsule. If a capsule only have CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag, my understanding it will not trigger reset automatially. User will trigger reset&#65533;&#65533;S3) in OS or application like CapsuleApp.
>
>>
>
>>>     CapsuleApp will not trigger reset if CAPSULE_FLAGS_INITIATE_RESET is not set, the problem is it will free the capsule buffer also!
>
>>
>
>>>
>
>>
>
>>>Done:
>
>>
>
>>>  for (Index = 0; Index < CapsuleNum; Index++) {
>
>>
>
>>>    if (CapsuleBuffer[Index] != NULL) {
>
>>
>
>>>      FreePool (CapsuleBuffer[Index]);
>
>>
>
>>>    }
>
>>
>
>>>  }
>
>>
>
>>>
>
>>
>
>>>  CleanGatherList(BlockDescriptors, CapsuleNum);
>
>>
>
>>>
>
>>
>
>>>   I trited to remove the above free buffer logic and capsuleApp can work with CAPSULE_FLAGS_INITIATE_RESET only attribute. The capsule data should be kept in this case
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>
>
>>
>
>>>_______________________________________________
>
>>
>
>>>edk2-devel mailing list
>
>>
>
>>>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>
>>
>
>>>https://lists.01.org/mailman/listinfo/edk2-devel
>
>>
>
>>
>
>>
>
>
>

Tcg2Pei.efi assert after sync to latest trunk

[wang xiaofeng]

Hi All,
   I just updated to latest edk2 trunk this afternoon , and meet the following error:


Loading PEIM at 0x00008FB1000 EntryPoint=0x00008FB13CF Tcg2Pei.efi
PROGRESS CODE: V03020002 I0
WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
Hash Interface (2AE9D80F-3FB2-4095-B7B1-E93157B946B6) has been registered
ASSERT_EFI_ERROR (Status = Already started)
ASSERT [Tcg2Pei] e:\code\cl819sync\Build\SariPlatformPkg\DEBUG_VS2013x86\IA32\SecurityPkg\Tcg\Tcg2Pei\Tcg2Pei\DEBUG\AutoGen.c(466): !EFI_ERROR (Status)


While AutoGen.c(466) have the following code:


  Status = HashInstanceLibSha1Constructor ();
  ASSERT_EFI_ERROR (Status);
  Status = HashInstanceLibSha1Constructor ();
  ASSERT_EFI_ERROR (Status); <-asset here


gEfiTpmDeviceInstanceTpm20DtpmGuid = { 0x286bf25a, 0xc2c3, 0x408c, { 0xb3, 0xb4, 0x25, 0xe6, 0x75, 0x8b, 0x73, 0x17 } }


  So anyone meet the same assert before ?  Thanks in advance !

Re: Tcg2Pei.efi assert after sync to latest trunk

[Gao, Liming]

This is BaseTools regression issue. It is fixed today. Please try it on tomorrow. 

Thanks
Liming
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of wang xiaofeng
Sent: Thursday, February 23, 2017 7:21 PM
To: edk2-devel@lists.01.org
Subject: [edk2] Tcg2Pei.efi assert after sync to latest trunk

Hi All,
   I just updated to latest edk2 trunk this afternoon , and meet the following error:


Loading PEIM at 0x00008FB1000 EntryPoint=0x00008FB13CF Tcg2Pei.efi
PROGRESS CODE: V03020002 I0
WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
Hash Interface (2AE9D80F-3FB2-4095-B7B1-E93157B946B6) has been registered
ASSERT_EFI_ERROR (Status = Already started)
ASSERT [Tcg2Pei] e:\code\cl819sync\Build\SariPlatformPkg\DEBUG_VS2013x86\IA32\SecurityPkg\Tcg\Tcg2Pei\Tcg2Pei\DEBUG\AutoGen.c(466): !EFI_ERROR (Status)


While AutoGen.c(466) have the following code:


  Status = HashInstanceLibSha1Constructor ();
  ASSERT_EFI_ERROR (Status);
  Status = HashInstanceLibSha1Constructor ();
  ASSERT_EFI_ERROR (Status); <-asset here


gEfiTpmDeviceInstanceTpm20DtpmGuid = { 0x286bf25a, 0xc2c3, 0x408c, { 0xb3, 0xb4, 0x25, 0xe6, 0x75, 0x8b, 0x73, 0x17 } }


  So anyone meet the same assert before ?  Thanks in advance !

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

Re: Tcg2Pei.efi assert after sync to latest trunk

[wang xiaofeng]

Hi Liming,
   Thanks! The issue is fixed after I sync basetool fix.








At 2017-02-23 21:01:25, "Gao, Liming" <liming.gao@intel.com> wrote:
>This is BaseTools regression issue. It is fixed today. Please try it on tomorrow. 
>
>Thanks
>Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of wang xiaofeng
>Sent: Thursday, February 23, 2017 7:21 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] Tcg2Pei.efi assert after sync to latest trunk
>
>Hi All,
>   I just updated to latest edk2 trunk this afternoon , and meet the following error:
>
>
>Loading PEIM at 0x00008FB1000 EntryPoint=0x00008FB13CF Tcg2Pei.efi
>PROGRESS CODE: V03020002 I0
>WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
>WARNING: Tpm2RegisterTpm2DeviceLib - does not support 286BF25A-C2C3-408C-B3B4-25E6758B7317 registration
>Hash Interface (2AE9D80F-3FB2-4095-B7B1-E93157B946B6) has been registered
>ASSERT_EFI_ERROR (Status = Already started)
>ASSERT [Tcg2Pei] e:\code\cl819sync\Build\SariPlatformPkg\DEBUG_VS2013x86\IA32\SecurityPkg\Tcg\Tcg2Pei\Tcg2Pei\DEBUG\AutoGen.c(466): !EFI_ERROR (Status)
>
>
>While AutoGen.c(466) have the following code:
>
>
>  Status = HashInstanceLibSha1Constructor ();
>  ASSERT_EFI_ERROR (Status);
>  Status = HashInstanceLibSha1Constructor ();
>  ASSERT_EFI_ERROR (Status); <-asset here
>
>
>gEfiTpmDeviceInstanceTpm20DtpmGuid = { 0x286bf25a, 0xc2c3, 0x408c, { 0xb3, 0xb4, 0x25, 0xe6, 0x75, 0x8b, 0x73, 0x17 } }
>
>
>  So anyone meet the same assert before ?  Thanks in advance !
>
>_______________________________________________
>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