Re: [edk2] [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

2017-06-25 Thread Zeng, Star 
Before UEFI 2.6a and 2.7, the behavior is unpredictable, our *CODE* chose to 
return EFI_NOT_FOUND.

"Passing in a VariableName parameter that is neither a Null-terminated string 
nor a value that was returned on the previous call to
GetNextVariableName() may also produce unpredictable results."



Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Monday, June 26, 2017 1:47 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to 
follow UEFI 2.7

Can you add more comments here to describe the purpose is to change the return 
status from Not Found to Invalid Parameter, and the reason of choosing Invalid 
Parameter?

Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:41 PM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Zeng, Star 
> 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> GetNextVariableName to follow UEFI 2.7
> 
> It is to return EFI_INVALID_PARAMETER when the input VariableName and 
> VendorGuid are not a valid variable to search next variable.
> It is added from UEFI 2.7 spec.
> Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate 
> the ending of searching.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:37 PM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> GetNextVariableName to follow UEFI 2.7
> 
> I understand your point.
> But I do think it hurts readability.
> 
> BTW, what does the below change does?
>if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> +if (VariableName[0] != 0) {
> +  //
> +  // The input values of VariableName and VendorGuid are not a 
> + name
> and GUID of an existing variable.
> +  //
> +  Status = EFI_INVALID_PARAMETER;
> +}
>  return Status;
>}
> 
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 11:05 AM
> > To: Ni, Ruiyu ; edk2-devel@lists.01.org
> > Cc: Gao, Liming ; Zeng, Star 
> > 
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> > GetNextVariableName to follow UEFI 2.7
> >
> > Ray,
> >
> > The code is like low hanging fruit from my practice for me, and I 
> > don't think it hurts readability although it may not bring 
> > performance improvement, it depends on how many variables in 
> > variable region, how many times of calling GetNextVariableName, and 
> > how fast of
> GetNextVariableName.
> >
> > The code practice I did is on NT32 and my real platforms. Is there 
> > anyone can make sure he/she tested all the systems in the world for 
> > their
> code?
> >
> >
> > Anyway, I can update the patch if you insist.
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Saturday, June 24, 2017 10:08 AM
> > To: Zeng, Star ; edk2-devel@lists.01.org
> > Cc: Gao, Liming 
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> > GetNextVariableName to follow UEFI 2.7
> >
> > Star,
> > I don't recommend to add the additional check for performance 
> > consideration.
> > Because we have no idea what the input VariableName buffer is like.
> > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals 
> > to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > The NT32 case you met cannot represent the all possible cases.
> > You could use the possibility theory to decide what the most 
> > efficient way
> is.
> >
> > Additionally I think code readability is more important than efficiency.
> > In this case, we need the data about the performance improvement to 
> > decide whether this check is necessary.
> >
> >
> > Regards,
> > Ray
> >
> > >-Original Message-
> > >From: Zeng, Star
> > >Sent: Friday, June 23, 2017 5:33 PM
> > >To: Ni, Ruiyu ; edk2-devel@lists.01.org
> > >Cc: Gao, Liming ; Zeng, Star 
> > >
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Ray,
> > >
> > >It is to pass the check quickly and avoid scanning all the chars in 
> > >VariableName by StrnLenS for normal boot without invalid cases.
> > >I did experiments in the code of GetNextVariableName with below 
> > >debug code for normal boot on NT32 and my real platforms, all the 
> > >cases will go
> > into the branch "xxx 2".
> > >  if (((VariableName[MaxLen - 1] != 0))) {
> > >DEBUG ((DEBUG_INFO, "xxx 1\n"));  } else {
> > >DEBUG ((DEBUG_INFO, "xxx 2\n"));  }
> > >
> > >
> >

[edk2] [patch 2/2] MdeModulePkg/BdsDxe: Report Status Code when booting from BootOrder list

2017-06-25 Thread Dandan Bi 
Report Status Code to indicate BDS starts attempting booting
from the UEFI BootOrder list.

Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index b5e6ef6..ac5f908 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -3,11 +3,11 @@
   When this module was dispatched by DxeCore, gEfiBdsArchProtocolGuid will be 
installed
   which contains interface of BdsEntry.
   After DxeCore finish DXE phase, gEfiBdsArchProtocolGuid->BdsEntry will be 
invoked
   to enter BDS phase.
 
-Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP
 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -368,10 +368,15 @@ BootBootOptions (
   )
 {
   UINTN  Index;
 
   //
+  // Report Status Code to indicate BDS starts attempting booting from the 
UEFI BootOrder list.
+  //
+  REPORT_STATUS_CODE (EFI_PROGRESS_CODE, (EFI_SOFTWARE_DXE_BS_DRIVER | 
EFI_SW_DXE_BS_PC_ATTEMPT_BOOT_ORDER_EVENT));
+
+  //
   // Attempt boot each boot option
   //
   for (Index = 0; Index < BootOptionCount; Index++) {
 //
 // According to EFI Specification, if a load option is not marked
-- 
1.9.5.msysgit.1

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

[edk2] [patch 1/2] MdePkg/PiStatusCode: Add new Status Code for BDS when attempting BootOrder

2017-06-25 Thread Dandan Bi 
According to new PI spec, add new Status Code to indicate BDS starts
attempting booting from the UEFI BootOrder list.

Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdePkg/Include/Pi/PiStatusCode.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Pi/PiStatusCode.h b/MdePkg/Include/Pi/PiStatusCode.h
index 8a5e040..953585c 100644
--- a/MdePkg/Include/Pi/PiStatusCode.h
+++ b/MdePkg/Include/Pi/PiStatusCode.h
@@ -1,9 +1,9 @@
 /** @file
   StatusCode related definitions in PI.
 
-Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.
+Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
 This program and the accompanying materials are licensed and made available 
under 
 the terms and conditions of the BSD License that accompanies this 
distribution.  
 The full text of the license may be found at
 http://opensource.org/licenses/bsd-license.php.

 
@@ -788,10 +788,11 @@ typedef struct {
 #define EFI_SW_DXE_BS_PC_LEGACY_OPROM_INIT(EFI_SUBCLASS_SPECIFIC | 
0x)
 #define EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT  (EFI_SUBCLASS_SPECIFIC | 
0x0001)
 #define EFI_SW_DXE_BS_PC_LEGACY_BOOT_EVENT(EFI_SUBCLASS_SPECIFIC | 
0x0002)
 #define EFI_SW_DXE_BS_PC_EXIT_BOOT_SERVICES_EVENT (EFI_SUBCLASS_SPECIFIC | 
0x0003)
 #define EFI_SW_DXE_BS_PC_VIRTUAL_ADDRESS_CHANGE_EVENT (EFI_SUBCLASS_SPECIFIC | 
0x0004)
+#define EFI_SW_DXE_BS_PC_ATTEMPT_BOOT_ORDER_EVENT (EFI_SUBCLASS_SPECIFIC | 
0x0007)
 ///@}
 
 //
 // Software Class SMM Driver Subclass Progress Code definitions.
 //
-- 
1.9.5.msysgit.1

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

[edk2] [patch 0/2] Add new Status Code "EFI_SW_DXE_BS_PC_ATTEMPT_BOOT_ORDER_EVENT"

2017-06-25 Thread Dandan Bi 
According to new PI spec, add new Status Code for BDS when 
attempting booting form the UEFI BootOrder list.

Dandan Bi (2):
  MdePkg/PiStatusCode: Add new Status Code for BDS when attempting
BootOrder
  MdeModulePkg/BdsDxe: Report Status Code when booting from BootOrder
list

 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 7 ++-
 MdePkg/Include/Pi/PiStatusCode.h | 3 ++-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

2017-06-25 Thread Ni, Ruiyu 
Can you add more comments here to describe the purpose is to change the return 
status
from Not Found to Invalid Parameter, and the reason of choosing Invalid 
Parameter?

Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:41 PM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Zeng, Star 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
> 
> It is to return EFI_INVALID_PARAMETER when the input VariableName and
> VendorGuid are not a valid variable to search next variable.
> It is added from UEFI 2.7 spec.
> Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate
> the ending of searching.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:37 PM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
> 
> I understand your point.
> But I do think it hurts readability.
> 
> BTW, what does the below change does?
>if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> +if (VariableName[0] != 0) {
> +  //
> +  // The input values of VariableName and VendorGuid are not a name
> and GUID of an existing variable.
> +  //
> +  Status = EFI_INVALID_PARAMETER;
> +}
>  return Status;
>}
> 
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 11:05 AM
> > To: Ni, Ruiyu ; edk2-devel@lists.01.org
> > Cc: Gao, Liming ; Zeng, Star
> > 
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Ray,
> >
> > The code is like low hanging fruit from my practice for me, and I
> > don't think it hurts readability although it may not bring performance
> > improvement, it depends on how many variables in variable region, how
> > many times of calling GetNextVariableName, and how fast of
> GetNextVariableName.
> >
> > The code practice I did is on NT32 and my real platforms. Is there
> > anyone can make sure he/she tested all the systems in the world for their
> code?
> >
> >
> > Anyway, I can update the patch if you insist.
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Saturday, June 24, 2017 10:08 AM
> > To: Zeng, Star ; edk2-devel@lists.01.org
> > Cc: Gao, Liming 
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Star,
> > I don't recommend to add the additional check for performance
> > consideration.
> > Because we have no idea what the input VariableName buffer is like.
> > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals
> > to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > The NT32 case you met cannot represent the all possible cases.
> > You could use the possibility theory to decide what the most efficient way
> is.
> >
> > Additionally I think code readability is more important than efficiency.
> > In this case, we need the data about the performance improvement to
> > decide whether this check is necessary.
> >
> >
> > Regards,
> > Ray
> >
> > >-Original Message-
> > >From: Zeng, Star
> > >Sent: Friday, June 23, 2017 5:33 PM
> > >To: Ni, Ruiyu ; edk2-devel@lists.01.org
> > >Cc: Gao, Liming ; Zeng, Star
> > >
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Ray,
> > >
> > >It is to pass the check quickly and avoid scanning all the chars in
> > >VariableName by StrnLenS for normal boot without invalid cases.
> > >I did experiments in the code of GetNextVariableName with below debug
> > >code for normal boot on NT32 and my real platforms, all the cases
> > >will go
> > into the branch "xxx 2".
> > >  if (((VariableName[MaxLen - 1] != 0))) {
> > >DEBUG ((DEBUG_INFO, "xxx 1\n"));
> > >  } else {
> > >DEBUG ((DEBUG_INFO, "xxx 2\n"));
> > >  }
> > >
> > >
> > >Thanks,
> > >Star
> > >-Original Message-
> > >From: Ni, Ruiyu
> > >Sent: Friday, June 23, 2017 4:20 PM
> > >To: Zeng, Star ; edk2-devel@lists.01.org
> > >Cc: Gao, Liming 
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Star,
> > >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> > >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should
> > >be
> > enough.
> > >
> > >Thanks/Ray
> > >
> > >> -Original Message-
> > >> From: Zeng, Star
> > >> Sent: Friday,

Re: [edk2] [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

2017-06-25 Thread Zeng, Star 
It is to return EFI_INVALID_PARAMETER when the input VariableName and 
VendorGuid are not a valid variable to search next variable.
It is added from UEFI 2.7 spec.
Before the spec change, the code is to return EFI_NOT_FOUND at that case.
After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate the 
ending of searching.


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Monday, June 26, 2017 1:37 PM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to 
follow UEFI 2.7

I understand your point.
But I do think it hurts readability.

BTW, what does the below change does?
   if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+if (VariableName[0] != 0) {
+  //
+  // The input values of VariableName and VendorGuid are not a name and 
GUID of an existing variable.
+  //
+  Status = EFI_INVALID_PARAMETER;
+}
 return Status;
   }


Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Monday, June 26, 2017 11:05 AM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Zeng, Star 
> 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> GetNextVariableName to follow UEFI 2.7
> 
> Ray,
> 
> The code is like low hanging fruit from my practice for me, and I 
> don't think it hurts readability although it may not bring performance 
> improvement, it depends on how many variables in variable region, how 
> many times of calling GetNextVariableName, and how fast of 
> GetNextVariableName.
> 
> The code practice I did is on NT32 and my real platforms. Is there 
> anyone can make sure he/she tested all the systems in the world for their 
> code?
> 
> 
> Anyway, I can update the patch if you insist.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Saturday, June 24, 2017 10:08 AM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> GetNextVariableName to follow UEFI 2.7
> 
> Star,
> I don't recommend to add the additional check for performance 
> consideration.
> Because we have no idea what the input VariableName buffer is like.
> If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals 
> to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> The NT32 case you met cannot represent the all possible cases.
> You could use the possibility theory to decide what the most efficient way is.
> 
> Additionally I think code readability is more important than efficiency.
> In this case, we need the data about the performance improvement to 
> decide whether this check is necessary.
> 
> 
> Regards,
> Ray
> 
> >-Original Message-
> >From: Zeng, Star
> >Sent: Friday, June 23, 2017 5:33 PM
> >To: Ni, Ruiyu ; edk2-devel@lists.01.org
> >Cc: Gao, Liming ; Zeng, Star 
> >
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> >GetNextVariableName to follow UEFI 2.7
> >
> >Ray,
> >
> >It is to pass the check quickly and avoid scanning all the chars in 
> >VariableName by StrnLenS for normal boot without invalid cases.
> >I did experiments in the code of GetNextVariableName with below debug 
> >code for normal boot on NT32 and my real platforms, all the cases 
> >will go
> into the branch "xxx 2".
> >  if (((VariableName[MaxLen - 1] != 0))) {
> >DEBUG ((DEBUG_INFO, "xxx 1\n"));
> >  } else {
> >DEBUG ((DEBUG_INFO, "xxx 2\n"));
> >  }
> >
> >
> >Thanks,
> >Star
> >-Original Message-
> >From: Ni, Ruiyu
> >Sent: Friday, June 23, 2017 4:20 PM
> >To: Zeng, Star ; edk2-devel@lists.01.org
> >Cc: Gao, Liming 
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> >GetNextVariableName to follow UEFI 2.7
> >
> >Star,
> >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should 
> >be
> enough.
> >
> >Thanks/Ray
> >
> >> -Original Message-
> >> From: Zeng, Star
> >> Sent: Friday, June 23, 2017 4:08 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star ; Gao, Liming 
> >> ; Ni, Ruiyu 
> >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update 
> >> GetNextVariableName to follow UEFI 2.7
> >>
> >> "The size must be large enough to fit input string supplied in 
> >> VariableName buffer" is added in the description for VariableNameSize.
> >> And two cases of EFI_INVALID_PARAMETER are added.
> >> 1. The input values of VariableName and VendorGuid are not a name and
> >>GUID of an existing variable.
> >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> >>the input VariableName buffer.
> >>
> >> This patch is to

Re: [edk2] [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

2017-06-25 Thread Ni, Ruiyu 
I understand your point.
But I do think it hurts readability.

BTW, what does the below change does?
   if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+if (VariableName[0] != 0) {
+  //
+  // The input values of VariableName and VendorGuid are not a name and 
GUID of an existing variable.
+  //
+  Status = EFI_INVALID_PARAMETER;
+}
 return Status;
   }


Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Monday, June 26, 2017 11:05 AM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Gao, Liming ; Zeng, Star 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
> 
> Ray,
> 
> The code is like low hanging fruit from my practice for me, and I don't think 
> it
> hurts readability although it may not bring performance improvement, it
> depends on how many variables in variable region, how many times of calling
> GetNextVariableName, and how fast of GetNextVariableName.
> 
> The code practice I did is on NT32 and my real platforms. Is there anyone can
> make sure he/she tested all the systems in the world for their code?
> 
> 
> Anyway, I can update the patch if you insist.
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Saturday, June 24, 2017 10:08 AM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
> 
> Star,
> I don't recommend to add the additional check for performance
> consideration.
> Because we have no idea what the input VariableName buffer is like.
> If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4,
> "VariableName[MaxLen-1] != 0" check is redundant.
> The NT32 case you met cannot represent the all possible cases.
> You could use the possibility theory to decide what the most efficient way is.
> 
> Additionally I think code readability is more important than efficiency.
> In this case, we need the data about the performance improvement to
> decide whether this check is necessary.
> 
> 
> Regards,
> Ray
> 
> >-Original Message-
> >From: Zeng, Star
> >Sent: Friday, June 23, 2017 5:33 PM
> >To: Ni, Ruiyu ; edk2-devel@lists.01.org
> >Cc: Gao, Liming ; Zeng, Star
> >
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Ray,
> >
> >It is to pass the check quickly and avoid scanning all the chars in
> >VariableName by StrnLenS for normal boot without invalid cases.
> >I did experiments in the code of GetNextVariableName with below debug
> >code for normal boot on NT32 and my real platforms, all the cases will go
> into the branch "xxx 2".
> >  if (((VariableName[MaxLen - 1] != 0))) {
> >DEBUG ((DEBUG_INFO, "xxx 1\n"));
> >  } else {
> >DEBUG ((DEBUG_INFO, "xxx 2\n"));
> >  }
> >
> >
> >Thanks,
> >Star
> >-Original Message-
> >From: Ni, Ruiyu
> >Sent: Friday, June 23, 2017 4:20 PM
> >To: Zeng, Star ; edk2-devel@lists.01.org
> >Cc: Gao, Liming 
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Star,
> >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be
> enough.
> >
> >Thanks/Ray
> >
> >> -Original Message-
> >> From: Zeng, Star
> >> Sent: Friday, June 23, 2017 4:08 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star ; Gao, Liming
> >> ; Ni, Ruiyu 
> >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >> GetNextVariableName to follow UEFI 2.7
> >>
> >> "The size must be large enough to fit input string supplied in
> >> VariableName buffer" is added in the description for VariableNameSize.
> >> And two cases of EFI_INVALID_PARAMETER are added.
> >> 1. The input values of VariableName and VendorGuid are not a name and
> >>GUID of an existing variable.
> >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> >>the input VariableName buffer.
> >>
> >> This patch is to update code to follow them.
> >>
> >> Cc: Liming Gao 
> >> Cc: Ruiyu Ni 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Star Zeng 
> >> ---
> >>  DuetPkg/FSVariable/FSVariable.c | 21 -
> >>  1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> >> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98
> >> 100644
> >> --- a/DuetPkg/FSVariable/FSVariable.c
> >> +++ b/DuetPkg/FSVariable/FSVariable.c
> >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able
> >> to

[edk2] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] Security of Setup Variable

2017-06-25 Thread Guo, Mang 
System can still boot to shell and OS successfully after EFI variable 
deletion/corruption.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Guo Mang 
---
 Platform/BroxtonPlatformPkg/BuildBios.bat  |  11 +-
 .../PlatformPreMemPei/MultiPlatSupport.h   | 201 
 .../PlatformPreMemPei/PlatformInitPreMem.c | 205 +
 .../PlatformPreMemPei/PlatformInitPreMem.h |   4 +
 .../PlatformSetupDxe/PlatformSetupDxe.c|  38 
 .../PlatformSettings/PlatformSetupDxe/Vfr.vfr  |  10 +-
 6 files changed, 463 insertions(+), 6 deletions(-)
 create mode 100644 
Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPreMemPei/MultiPlatSupport.h

diff --git a/Platform/BroxtonPlatformPkg/BuildBios.bat 
b/Platform/BroxtonPlatformPkg/BuildBios.bat
index f656876..87b4467 100644
--- a/Platform/BroxtonPlatformPkg/BuildBios.bat
+++ b/Platform/BroxtonPlatformPkg/BuildBios.bat
@@ -387,10 +387,15 @@ if not exist "%AutoGenPath%" (
 findstr /L "_PCD_VALUE_" %AutoGenPath% > %STITCH_PATH%\FlashMap.h
 
 echo Running FCE...
+copy /b %BUILD_PATH%\FV\FvIBBM.fv + /b %BUILD_PATH%\FV\Soc.fd /b 
%BUILD_PATH%\FV\Temp.fd
 :: Extract Hii data from build and store a copy in HiiDefaultData.txt
-fce.exe read -i %BUILD_PATH%\FV\Soc.fd > %BUILD_PATH%\FV\HiiDefaultData.txt 
2>>EDK2.log
+:: UQI 0006 005C 0078 0030 0031 0030 0031 is for Platid question 
prompt(STR_IPU_ENABLED)
+:: First 0006 is the length of string; Next six byte values are mapped to 
STR_PLATID_PROMPT string value defined in 
%WORKSPACE_PLATFORM%\%PLATFORM_PACKAGE%\Setup\UqiList.uni.
+fce.exe read -i %BUILD_PATH%\FV\Temp.fd 0006 005C 0078 0030 0031 0030 0031 > 
%BUILD_PATH%\FV\HiiDefaultData.txt 2>>EDK2.log
 :: Generate the Setup variable and save changes to BxtXXX.fd
-fce.exe update -i %BUILD_PATH%\FV\Soc.fd -s %BUILD_PATH%\FV\HiiDefaultData.txt 
-o %BUILD_PATH%\FV\Bxt%Arch%.fd  1>>EDK2.log 2>&1
+:: B73FE497-B92E-416e-8326-45AD0D270091 is the GUID of IBBR FV
+fce.exe update -i %BUILD_PATH%\FV\Temp.fd -s 
%BUILD_PATH%\FV\HiiDefaultData.txt -o %BUILD_PATH%\FV\Bxt%Arch%.fd  -g 
B73FE497-B92E-416e-8326-45AD0D270091 -a 1>>EDK2.log 2>&1
+split -f %BUILD_PATH%\FV\Bxt%Arch%.fd -s 0x35000 -o %BUILD_PATH%\FV\FvIBBM.fv
 
 if ErrorLevel 1 goto BldFail
 
@@ -401,7 +406,7 @@ if "%BUILD_TYPE%"=="R" set BUILD_TYPE=R
 
 echo Copy BIOS...
 set BIOS_Name=%BOARD_ID%_%Arch%_%BUILD_TYPE%_%VERSION_MAJOR%_%VERSION_MINOR%
-copy /y/b %BUILD_PATH%\FV\Bxt%Arch%.fd  %STITCH_PATH%\%BIOS_Name%.ROM >nul
+copy /y/b %BUILD_PATH%\FV\Soc.fd  %STITCH_PATH%\%BIOS_Name%.ROM >nul
 copy /y   %STITCH_PATH%\FlashMap.h%STITCH_PATH%\%BIOS_Name%.map >nul
 
 set Storage_Folder=%STITCH_PATH%\%BIOS_Name%
diff --git 
a/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPreMemPei/MultiPlatSupport.h
 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPreMemPei/MultiPlatSupport.h
new file mode 100644
index 000..7f21da2
--- /dev/null
+++ 
b/Platform/BroxtonPlatformPkg/Common/PlatformSettings/PlatformPreMemPei/MultiPlatSupport.h
@@ -0,0 +1,201 @@
+/**@file
+
+Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD 
License
+which accompanies this distribution.  The full text of the license may be 
found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __MULTI_PLATFORM_SUPPORT_H__
+#define __MULTI_PLATFORM_SUPPORT_H__
+
+extern EFI_GUID gDefaultDataOptSizeFileGuid;
+
+///
+/// Alignment of variable name and data, according to the architecture:
+/// * For IA-32 and Intel(R) 64 architectures: 1.
+/// * For IA-64 architecture: 8.
+///
+#if defined (MDE_CPU_IPF)
+#define ALIGNMENT 8
+#else
+#define ALIGNMENT 1
+#endif
+
+//
+// GET_PAD_SIZE calculates the miminal pad bytes needed to make the current 
pad size satisfy the alignment requirement.
+//
+#if (ALIGNMENT == 1)
+#define GET_PAD_SIZE(a) (0)
+#else
+#define GET_PAD_SIZE(a) (((~a) + 1) & (ALIGNMENT - 1))
+#endif
+
+///
+/// Alignment of Variable Data Header in Variable Store region.
+///
+#define HEADER_ALIGNMENT  4
+#define HEADER_ALIGN(Header)  (((UINTN) (Header) + HEADER_ALIGNMENT - 1) & 
(~(HEADER_ALIGNMENT - 1)))
+
+///
+/// Status of Variable Store Region.
+///
+/*typedef enum {
+  EfiRaw,
+  EfiValid,
+  EfiInvalid,
+  EfiUnknown
+} VARIABLE_STORE_STATUS;*/
+
+#pragma pack(1)
+
+///
+/// Variable Store Header Format and State.
+///
+#define VARIABLE_STORE_FORMATTED  0x5a
+#define VARIABLE_STORE_HEALTHY0xfe
+
+///
+/// Variable Store region header.
+///
+/*typedef struct {
+  ///
+  /// Variable store region signature.
+  ///
+  EFI_GUID  Signature;
+  ///
+  /// Size of entire variable

Re: [edk2] [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

2017-06-25 Thread Zeng, Star 
Ray,

The code is like low hanging fruit from my practice for me, and I don't think 
it hurts readability although it may not bring performance improvement, it 
depends on how many variables in variable region, how many times of calling 
GetNextVariableName, and how fast of GetNextVariableName.

The code practice I did is on NT32 and my real platforms. Is there anyone can 
make sure he/she tested all the systems in the world for their code?


Anyway, I can update the patch if you insist.


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Saturday, June 24, 2017 10:08 AM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to 
follow UEFI 2.7

Star,
I don't recommend to add the additional check for performance consideration.
Because we have no idea what the input VariableName buffer is like.
If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4, 
"VariableName[MaxLen-1] != 0" check is redundant.
The NT32 case you met cannot represent the all possible cases.
You could use the possibility theory to decide what the most efficient way is.

Additionally I think code readability is more important than efficiency.
In this case, we need the data about the performance improvement to decide 
whether this check is necessary.


Regards,
Ray

>-Original Message-
>From: Zeng, Star
>Sent: Friday, June 23, 2017 5:33 PM
>To: Ni, Ruiyu ; edk2-devel@lists.01.org
>Cc: Gao, Liming ; Zeng, Star 
>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>GetNextVariableName to follow UEFI 2.7
>
>Ray,
>
>It is to pass the check quickly and avoid scanning all the chars in 
>VariableName by StrnLenS for normal boot without invalid cases.
>I did experiments in the code of GetNextVariableName with below debug 
>code for normal boot on NT32 and my real platforms, all the cases will go into 
>the branch "xxx 2".
>  if (((VariableName[MaxLen - 1] != 0))) {
>DEBUG ((DEBUG_INFO, "xxx 1\n"));
>  } else {
>DEBUG ((DEBUG_INFO, "xxx 2\n"));
>  }
>
>
>Thanks,
>Star
>-Original Message-
>From: Ni, Ruiyu
>Sent: Friday, June 23, 2017 4:20 PM
>To: Zeng, Star ; edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>GetNextVariableName to follow UEFI 2.7
>
>Star,
>What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
>I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be 
>enough.
>
>Thanks/Ray
>
>> -Original Message-
>> From: Zeng, Star
>> Sent: Friday, June 23, 2017 4:08 PM
>> To: edk2-devel@lists.01.org
>> Cc: Zeng, Star ; Gao, Liming 
>> ; Ni, Ruiyu 
>> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>> GetNextVariableName to follow UEFI 2.7
>>
>> "The size must be large enough to fit input string supplied in 
>> VariableName buffer" is added in the description for VariableNameSize.
>> And two cases of EFI_INVALID_PARAMETER are added.
>> 1. The input values of VariableName and VendorGuid are not a name and
>>GUID of an existing variable.
>> 2. Null-terminator is not found in the first VariableNameSize bytes of
>>the input VariableName buffer.
>>
>> This patch is to update code to follow them.
>>
>> Cc: Liming Gao 
>> Cc: Ruiyu Ni 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng 
>> ---
>>  DuetPkg/FSVariable/FSVariable.c | 21 -
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/DuetPkg/FSVariable/FSVariable.c 
>> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98 
>> 100644
>> --- a/DuetPkg/FSVariable/FSVariable.c
>> +++ b/DuetPkg/FSVariable/FSVariable.c
>> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able 
>> to protoect those.
>>  Duet trusts all meta data from disk. If variable code, variable 
>> metadata and variable  data is modified in inproper way, the behavior 
>> is undefined.
>>
>> -Copyright (c) 2006 - 2016, Intel Corporation. All rights 
>> reserved.
>> +Copyright (c) 2006 - 2017, Intel Corporation. All rights 
>> +reserved.
>>  This program and the accompanying materials  are licensed and made 
>> available under the terms and conditions of the BSD License  which 
>> accompanies this distribution.  The full text of the license may be 
>> found at @@ -1400,14 +1400,33 @@ Returns:
>>VARIABLE_POINTER_TRACK  Variable;
>>UINTN   VarNameSize;
>>EFI_STATUS  Status;
>> +  UINTN   MaxLen;
>>
>>if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
>> NULL) {
>>  return EFI_INVALID_PARAMETER;
>>}
>>
>> +  //
>> +  // Calculate the possible maximum

Re: [edk2] MESSAGING_DEVICE_PATH Intel NVMe Vendor defined

2017-06-25 Thread Wu, Hao A 
Hi David,

The UEFI spec defines the format of an NVMe device node, I think the
driver (maybe on the NVME option rom) that produces the device path for
the NVME device should get updated to follow the spec.

For those vendor defined paths, I think the DevicePathLib will only dump
the hex of device node content. There is no method to extract the
information from them since they are not documented in the UEFI spec.

Best Regards,
Hao Wu

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of David
> F.
> Sent: Saturday, June 24, 2017 1:56 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] MESSAGING_DEVICE_PATH Intel NVMe Vendor defined
> 
> Hello,
> 
> Testing a system with NVMe Intel SSD drive doesn't give a meaningful
> description in the shell or converting the device path to text in
> general using the EDK2 (you only get a "?").   It appears it's because
> the MESSAGING_DEVICE_PATH for this Intel NVMe is vendor defined.
> Where do you find the information for vendor defined paths?   Adding
> these to the EDK2 as they are created would be nice to get friendly
> descriptions.
> 
> Thanks.
> ___
> 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 V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

2017-06-25 Thread Zeng, Star 
Patch has been pushed at 45cfcd8dccf84b8abbc1d6f587fedb5d2037ec79. :)

Thanks,
Star
-Original Message-
From: Zeng, Star 
Sent: Monday, June 26, 2017 9:20 AM
To: Amit Kumar ; edk2-devel@lists.01.org
Cc: Tian, Feng ; Gao, Liming ; 
Kinney, Michael D ; Zeng, Star 
Subject: RE: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned 
by CoreOpenProtocol

Reviewed-by: Star Zeng 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Amit 
Kumar
Sent: Friday, June 23, 2017 6:10 PM
To: edk2-devel@lists.01.org
Cc: Tian, Feng ; Gao, Liming ; 
Kinney, Michael D ; Zeng, Star 
Subject: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by 
CoreOpenProtocol

Change since v3:
1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Inteface = 
NULL case. [Reported by:star.zeng at intel.com]

Change Since v2:
1) Modified to use EFI_ERROR to get status code

Change since v1:
1) Fixed typo protocal to protocol
2) Fixed coding style

Modified source code to update Interface as per spec.
1) In case of Protocol is un-supported, interface should be returned NULL.
2) In case of any error, interface should not be modified.
3) In case of Test Protocol, interface is optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Amit Kumar 
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b8914..fe58b6c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1006,12 +1006,8 @@ CoreOpenProtocol (
   //
   // Check for invalid Interface
   //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-if (Interface == NULL) {
-  return EFI_INVALID_PARAMETER;
-} else {
-  *Interface = NULL;
-}
+  if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) {
+return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1075,15 +1071,13 @@ CoreOpenProtocol (
   Prot = CoreGetProtocolInterface (UserHandle, Protocol);
   if (Prot == NULL) {
 Status = EFI_UNSUPPORTED;
+if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL){
+  //Return NULL Interface if Unsupported Protocol
+  *Interface = NULL;
+}
 goto Done;
   }
 
-  //
-  // This is the protocol interface entry for this protocol
-  //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-*Interface = Prot->Interface;
-  }
   Status = EFI_SUCCESS;
 
   ByDriver= FALSE;
@@ -1177,6 +1171,14 @@ CoreOpenProtocol (
   }
 
 Done:
+
+  //
+  // This is the protocol interface entry for this protocol.
+  // In case of any Error, Interface should not be updated as per spec.
+  //
+  if (!EFI_ERROR (Status) && (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+*Interface = Prot->Interface;
+  }
   //
   // Done. Release the database lock are return
   //
--
1.9.1

___
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] [Patch][edk2-platforms/devel-MinnowBoard3-UDK2017] BIOS version.

2017-06-25 Thread lushifex 
Change BIOS version.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: lushifex 
---
 Platform/BroxtonPlatformPkg/BiosId.env | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/BroxtonPlatformPkg/BiosId.env 
b/Platform/BroxtonPlatformPkg/BiosId.env
index 1e28313..f8dfb32 100644
--- a/Platform/BroxtonPlatformPkg/BiosId.env
+++ b/Platform/BroxtonPlatformPkg/BiosId.env
@@ -30,6 +30,6 @@
 BOARD_ID  = APLKRVP
 BOARD_REV = 3
 BUILD_TYPE= D
-VERSION_MAJOR = 0064
+VERSION_MAJOR = 0065
 VERSION_MINOR = 01
 BOARD_EXT = X64
-- 
2.7.0.windows.1


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

Re: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

2017-06-25 Thread Zeng, Star 
Reviewed-by: Star Zeng 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Amit 
Kumar
Sent: Friday, June 23, 2017 6:10 PM
To: edk2-devel@lists.01.org
Cc: Tian, Feng ; Gao, Liming ; 
Kinney, Michael D ; Zeng, Star 
Subject: [edk2] [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by 
CoreOpenProtocol

Change since v3:
1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL and Inteface = 
NULL case. [Reported by:star.zeng at intel.com]

Change Since v2:
1) Modified to use EFI_ERROR to get status code

Change since v1:
1) Fixed typo protocal to protocol
2) Fixed coding style

Modified source code to update Interface as per spec.
1) In case of Protocol is un-supported, interface should be returned NULL.
2) In case of any error, interface should not be modified.
3) In case of Test Protocol, interface is optional.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Amit Kumar 
---
 MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c 
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b8914..fe58b6c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -1006,12 +1006,8 @@ CoreOpenProtocol (
   //
   // Check for invalid Interface
   //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-if (Interface == NULL) {
-  return EFI_INVALID_PARAMETER;
-} else {
-  *Interface = NULL;
-}
+  if ((Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) && (Interface == NULL)) {
+return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1075,15 +1071,13 @@ CoreOpenProtocol (
   Prot = CoreGetProtocolInterface (UserHandle, Protocol);
   if (Prot == NULL) {
 Status = EFI_UNSUPPORTED;
+if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL){
+  //Return NULL Interface if Unsupported Protocol
+  *Interface = NULL;
+}
 goto Done;
   }
 
-  //
-  // This is the protocol interface entry for this protocol
-  //
-  if (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
-*Interface = Prot->Interface;
-  }
   Status = EFI_SUCCESS;
 
   ByDriver= FALSE;
@@ -1177,6 +1171,14 @@ CoreOpenProtocol (
   }
 
 Done:
+
+  //
+  // This is the protocol interface entry for this protocol.
+  // In case of any Error, Interface should not be updated as per spec.
+  //
+  if (!EFI_ERROR (Status) && (Attributes != EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) {
+*Interface = Prot->Interface;
+  }
   //
   // Done. Release the database lock are return
   //
--
1.9.1

___
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