Re: [edk2-devel] [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

[Vladimir Olovyannikov via groups.io]

Hi Laszlo, Macieji,

Thank you for spotting these.
Some comments below.
> -Original Message-
> From: Laszlo Ersek 
> Sent: Friday, September 4, 2020 8:21 AM
> To: Rabeda, Maciej ; Vladimir Olovyannikov
> ; devel@edk2.groups.io; Zhichao
> Gao ; Ray Ni 
> Cc: Samer El-Haj-Mahmoud ; Jiaxin Wu
> ; Siyuan Fu ; Liming Gao
> ; Nd 
> Subject: Re: [PATCH v10 1/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> question for Ray and Zhichao at the bottom
>
> On 09/04/20 15:10, Rabeda, Maciej wrote:
> > Hi Vladimir,
> >
> > One remark regarding these macros:
> > #define SEC_PER_MONTH   ((UINTN)
> > 2,592,000)
> > #define SEC_PER_YEAR((UINTN)
> > 31,536,000)
> >
> > They are not being used in your code and were most probably taken from
> > EmbeddedPkg/Include/Library/TimeBaseLib.h.
> > However - using them in a simple example:
> > UINTN a = SEC_PER_MONTH
> > DebugPrint (EFI_D_INFO, "%d\n", a);
> >
> > and looking at code listing, 'a' variable will be assigned a value of 0.
> > Commas should be removed from macros for them to work as designed.
> >
> > I leave the decision whether this should be fixed in ShellPkg and
> > EmbeddedPkg to the appropriate maintainers.
>
> I think these macros should be eliminated altogether from edk2. They
> date back to commit 0f4386e775c7
> ("ArmPlatformPkg/PL031RealTimeClockLib:
> Implement PL031 RTC drive", 2011-06-11) and I can imagine no
> circumstance under which they could possibly work.
>
> For example, SEC_PER_MONTH evaluates to 0 (having type "int"). The
> comma
> operator has the weakest binding of all operators, so not only is the
> value 0, but even the (UINTN) cast applies only to the "2" that stands
> to the left of the first comma.
>
> Bonus: the rightmost 000 is an octal constant.
Thank you for spotting these. Indeed, those were taken "as is" from
TimeBaseLib.h
Copy/paste...
There is also another issue with the TimebaseLib: inconsistency in return
values of the
EfiTimeToEpoch (returns UINT32, should return UINTN, as Zhichao pointed out
earlier in the previous HttpDynamicCommand patchset).
If this one is fixed, I can just use the TimeBaseLib.h header for constants.
>
> > For the rest of the code:
> > Reviewed-by: Maciej Rabeda 
>
> Thanks.
>
> Ray, Zhichao, what's your plan for approving / merging this patch? Are
> you OK to merge the next version (v11), with the macros removed?
>
> Asking because I'd like to test the patch one last time, just before
> it's merged.
>
> Thanks!
> Laszlo
>
Thank you,
Vladimir
> >
> > Thanks,
> > Maciej
> >
> > On 02-Sep-20 06:08, Vladimir Olovyannikov wrote:
> >> Introduce an http client utilizing EDK2 HTTP protocol, to
> >> allow fast image downloading from http/https servers.
> >> HTTP download speed is usually faster than tftp.
> >> The client is based on the same approach as tftp dynamic command, and
> >> uses the same UEFI Shell command line parameters. This makes it easy
> >> integrating http into existing UEFI Shell scripts.
> >> Note that to enable HTTP download, feature Pcd
> >> gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must
> >> be set to TRUE.
> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
> >>
> >> Signed-off-by: Vladimir Olovyannikov
> 
> >> Cc: Samer El-Haj-Mahmoud 
> >> Cc: Laszlo Ersek 
> >> Cc: Zhichao Gao 
> >> Cc: Maciej Rabeda 
> >> Cc: Jiaxin Wu 
> >> Cc: Siyuan Fu 
> >> Cc: Ray Ni 
> >> Cc: Liming Gao 
> >> Cc: Nd 
> >> ---
> >>   ShellPkg/ShellPkg.dec |1 +
> >>   ShellPkg/ShellPkg.dsc |5 +
> >>   .../HttpDynamicCommand/HttpApp.inf|   58 +
> >>   .../HttpDynamicCommand/HttpDynamicCommand.inf |   63 +
> >>   .../DynamicCommand/HttpDynamicCommand/Http.h  |   90 +
> >>   ShellPkg/Include/Guid/ShellLibHiiGuid.h   |5 +
> >>   .../DynamicCommand/HttpDynamicCommand/Http.c  | 1831
> +
> >>   .../HttpDynamicCommand/HttpApp.c  |   61 +
> >>   .../HttpDynamicCommand/HttpDynamicCommand.c   |  137 ++
> >>   .../HttpDynamicCommand/Http.uni   |  117 ++
> >>   10 files changed, 2368 insertions(+)
> >>   create mode 100644
> >> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
> >>   create mode 100644
> >>
> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
> .inf
> >>   create mode 100644
> ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
> >>   create mode 100644
> ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
> >>   create mode 100644
> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
> >>   create mode 100644
> >>
> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand
> .c
> >>   create mode 100644
> ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni
> >>
> >> diff --git a/ShellPkg/ShellPkg.dec b/ShellPkg/ShellPkg.dec
> >> index d0843d338126..7b2d1230bd2c 100644
> >> --- a/ShellPkg/ShellPkg.dec
> >> +++ b/ShellPkg/ShellPkg.dec
> >> @@ -53,6 +53,7 @@ [Guids]
> >> gShellNetwork

Re: [edk2-devel] [PATCH] EmbeddedPkg/TimeBaseLib: remove the SEC_PER_MONTH, SEC_PER_YEAR macros

[Philippe Mathieu-Daudé]

On 9/4/20 6:42 PM, Leif Lindholm wrote:
> Ming bounced, adding xiewenyi2.
> 
> On Fri, Sep 04, 2020 at 17:16:44 +0100, Leif Lindholm wrote:
>> +Ming
>>
>> On Fri, Sep 04, 2020 at 17:45:41 +0200, Laszlo Ersek wrote:
>>> The SEC_PER_MONTH and SEC_PER_YEAR macros are wrong: they both evaluate to
>>> 0 (of type "int"). They are also unused (they could never be used for
>>> division, for example); so remove them. The macros were originally
>>> introduced in commit 0f4386e775c7 ("ArmPlatformPkg/PL031RealTimeClockLib:
>>> Implement PL031 RTC drive", 2011-06-11).
>>>
>>> Cc: Ard Biesheuvel 
>>> Cc: Leif Lindholm 
>>> Cc: Maciej Rabeda 
>>> Cc: Philippe Mathieu-Daudé 
>>> Reported-by: Maciej Rabeda 
>>> Signed-off-by: Laszlo Ersek 
>>
>> Hmm, these are used in a couple of Hisilicon libraries in
>> edk2-platforms. Cleary, they are not now functioning as expected (but
>> they never invoke these macros for division).
>>
>> We should fix this, but possibly by correcting the macros instead of
>> deleting them?

I'm not sure how to fix, not all months have 30 days...

Reviewed-by: Philippe Mathieu-Daude 

>>
>> /
>> Leif (goes back to now hopefuly lit barbecue)
>>
>>> ---
>>>  EmbeddedPkg/Include/Library/TimeBaseLib.h | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/EmbeddedPkg/Include/Library/TimeBaseLib.h 
>>> b/EmbeddedPkg/Include/Library/TimeBaseLib.h
>>> index ee2f191d985b..3c2d3660c66c 100644
>>> --- a/EmbeddedPkg/Include/Library/TimeBaseLib.h
>>> +++ b/EmbeddedPkg/Include/Library/TimeBaseLib.h
>>> @@ -51,8 +51,6 @@
>>>  #define SEC_PER_MIN ((UINTN)60)
>>>  #define SEC_PER_HOUR((UINTN)  3600)
>>>  #define SEC_PER_DAY ((UINTN) 86400)
>>> -#define SEC_PER_MONTH   ((UINTN)  
>>> 2,592,000)
>>> -#define SEC_PER_YEAR((UINTN) 
>>> 31,536,000)
>>>  
>>>  BOOLEAN
>>>  EFIAPI
>>> -- 
>>> 2.19.1.3.g30247aa5d201
>>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65061): https://edk2.groups.io/g/devel/message/65061
Mute This Topic: https://groups.io/mt/76632579/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] EmbeddedPkg/TimeBaseLib: remove the SEC_PER_MONTH, SEC_PER_YEAR macros

[Leif Lindholm]

Ming bounced, adding xiewenyi2.

On Fri, Sep 04, 2020 at 17:16:44 +0100, Leif Lindholm wrote:
> +Ming
> 
> On Fri, Sep 04, 2020 at 17:45:41 +0200, Laszlo Ersek wrote:
> > The SEC_PER_MONTH and SEC_PER_YEAR macros are wrong: they both evaluate to
> > 0 (of type "int"). They are also unused (they could never be used for
> > division, for example); so remove them. The macros were originally
> > introduced in commit 0f4386e775c7 ("ArmPlatformPkg/PL031RealTimeClockLib:
> > Implement PL031 RTC drive", 2011-06-11).
> > 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Maciej Rabeda 
> > Cc: Philippe Mathieu-Daudé 
> > Reported-by: Maciej Rabeda 
> > Signed-off-by: Laszlo Ersek 
> 
> Hmm, these are used in a couple of Hisilicon libraries in
> edk2-platforms. Cleary, they are not now functioning as expected (but
> they never invoke these macros for division).
> 
> We should fix this, but possibly by correcting the macros instead of
> deleting them?
> 
> /
> Leif (goes back to now hopefuly lit barbecue)
> 
> > ---
> >  EmbeddedPkg/Include/Library/TimeBaseLib.h | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/EmbeddedPkg/Include/Library/TimeBaseLib.h 
> > b/EmbeddedPkg/Include/Library/TimeBaseLib.h
> > index ee2f191d985b..3c2d3660c66c 100644
> > --- a/EmbeddedPkg/Include/Library/TimeBaseLib.h
> > +++ b/EmbeddedPkg/Include/Library/TimeBaseLib.h
> > @@ -51,8 +51,6 @@
> >  #define SEC_PER_MIN ((UINTN)60)
> >  #define SEC_PER_HOUR((UINTN)  3600)
> >  #define SEC_PER_DAY ((UINTN) 86400)
> > -#define SEC_PER_MONTH   ((UINTN)  
> > 2,592,000)
> > -#define SEC_PER_YEAR((UINTN) 
> > 31,536,000)
> >  
> >  BOOLEAN
> >  EFIAPI
> > -- 
> > 2.19.1.3.g30247aa5d201
> > 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65060): https://edk2.groups.io/g/devel/message/65060
Mute This Topic: https://groups.io/mt/76632579/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] EmbeddedPkg/TimeBaseLib: remove the SEC_PER_MONTH, SEC_PER_YEAR macros

[Leif Lindholm]

+Ming

On Fri, Sep 04, 2020 at 17:45:41 +0200, Laszlo Ersek wrote:
> The SEC_PER_MONTH and SEC_PER_YEAR macros are wrong: they both evaluate to
> 0 (of type "int"). They are also unused (they could never be used for
> division, for example); so remove them. The macros were originally
> introduced in commit 0f4386e775c7 ("ArmPlatformPkg/PL031RealTimeClockLib:
> Implement PL031 RTC drive", 2011-06-11).
> 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Cc: Maciej Rabeda 
> Cc: Philippe Mathieu-Daudé 
> Reported-by: Maciej Rabeda 
> Signed-off-by: Laszlo Ersek 

Hmm, these are used in a couple of Hisilicon libraries in
edk2-platforms. Cleary, they are not now functioning as expected (but
they never invoke these macros for division).

We should fix this, but possibly by correcting the macros instead of
deleting them?

/
Leif (goes back to now hopefuly lit barbecue)

> ---
>  EmbeddedPkg/Include/Library/TimeBaseLib.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/EmbeddedPkg/Include/Library/TimeBaseLib.h 
> b/EmbeddedPkg/Include/Library/TimeBaseLib.h
> index ee2f191d985b..3c2d3660c66c 100644
> --- a/EmbeddedPkg/Include/Library/TimeBaseLib.h
> +++ b/EmbeddedPkg/Include/Library/TimeBaseLib.h
> @@ -51,8 +51,6 @@
>  #define SEC_PER_MIN ((UINTN)60)
>  #define SEC_PER_HOUR((UINTN)  3600)
>  #define SEC_PER_DAY ((UINTN) 86400)
> -#define SEC_PER_MONTH   ((UINTN)  2,592,000)
> -#define SEC_PER_YEAR((UINTN) 31,536,000)
>  
>  BOOLEAN
>  EFIAPI
> -- 
> 2.19.1.3.g30247aa5d201
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65059): https://edk2.groups.io/g/devel/message/65059
Mute This Topic: https://groups.io/mt/76632579/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] EmbeddedPkg/TimeBaseLib: remove the SEC_PER_MONTH, SEC_PER_YEAR macros

[Laszlo Ersek]

The SEC_PER_MONTH and SEC_PER_YEAR macros are wrong: they both evaluate to
0 (of type "int"). They are also unused (they could never be used for
division, for example); so remove them. The macros were originally
introduced in commit 0f4386e775c7 ("ArmPlatformPkg/PL031RealTimeClockLib:
Implement PL031 RTC drive", 2011-06-11).

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Cc: Maciej Rabeda 
Cc: Philippe Mathieu-Daudé 
Reported-by: Maciej Rabeda 
Signed-off-by: Laszlo Ersek 
---
 EmbeddedPkg/Include/Library/TimeBaseLib.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/EmbeddedPkg/Include/Library/TimeBaseLib.h 
b/EmbeddedPkg/Include/Library/TimeBaseLib.h
index ee2f191d985b..3c2d3660c66c 100644
--- a/EmbeddedPkg/Include/Library/TimeBaseLib.h
+++ b/EmbeddedPkg/Include/Library/TimeBaseLib.h
@@ -51,8 +51,6 @@
 #define SEC_PER_MIN ((UINTN)60)
 #define SEC_PER_HOUR((UINTN)  3600)
 #define SEC_PER_DAY ((UINTN) 86400)
-#define SEC_PER_MONTH   ((UINTN)  2,592,000)
-#define SEC_PER_YEAR((UINTN) 31,536,000)
 
 BOOLEAN
 EFIAPI
-- 
2.19.1.3.g30247aa5d201


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65058): https://edk2.groups.io/g/devel/message/65058
Mute This Topic: https://groups.io/mt/76632579/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

[Laszlo Ersek]

question for Ray and Zhichao at the bottom

On 09/04/20 15:10, Rabeda, Maciej wrote:
> Hi Vladimir,
> 
> One remark regarding these macros:
> #define SEC_PER_MONTH   ((UINTN) 2,592,000)
> #define SEC_PER_YEAR    ((UINTN)
> 31,536,000)
> 
> They are not being used in your code and were most probably taken from
> EmbeddedPkg/Include/Library/TimeBaseLib.h.
> However - using them in a simple example:
> UINTN a = SEC_PER_MONTH
> DebugPrint (EFI_D_INFO, "%d\n", a);
> 
> and looking at code listing, 'a' variable will be assigned a value of 0.
> Commas should be removed from macros for them to work as designed.
> 
> I leave the decision whether this should be fixed in ShellPkg and
> EmbeddedPkg to the appropriate maintainers.

I think these macros should be eliminated altogether from edk2. They
date back to commit 0f4386e775c7 ("ArmPlatformPkg/PL031RealTimeClockLib:
Implement PL031 RTC drive", 2011-06-11) and I can imagine no
circumstance under which they could possibly work.

For example, SEC_PER_MONTH evaluates to 0 (having type "int"). The comma
operator has the weakest binding of all operators, so not only is the
value 0, but even the (UINTN) cast applies only to the "2" that stands
to the left of the first comma.

Bonus: the rightmost 000 is an octal constant.

> For the rest of the code:
> Reviewed-by: Maciej Rabeda 

Thanks.

Ray, Zhichao, what's your plan for approving / merging this patch? Are
you OK to merge the next version (v11), with the macros removed?

Asking because I'd like to test the patch one last time, just before
it's merged.

Thanks!
Laszlo

> 
> Thanks,
> Maciej
> 
> On 02-Sep-20 06:08, Vladimir Olovyannikov wrote:
>> Introduce an http client utilizing EDK2 HTTP protocol, to
>> allow fast image downloading from http/https servers.
>> HTTP download speed is usually faster than tftp.
>> The client is based on the same approach as tftp dynamic command, and
>> uses the same UEFI Shell command line parameters. This makes it easy
>> integrating http into existing UEFI Shell scripts.
>> Note that to enable HTTP download, feature Pcd
>> gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must
>> be set to TRUE.
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860
>>
>> Signed-off-by: Vladimir Olovyannikov 
>> Cc: Samer El-Haj-Mahmoud 
>> Cc: Laszlo Ersek 
>> Cc: Zhichao Gao 
>> Cc: Maciej Rabeda 
>> Cc: Jiaxin Wu 
>> Cc: Siyuan Fu 
>> Cc: Ray Ni 
>> Cc: Liming Gao 
>> Cc: Nd 
>> ---
>>   ShellPkg/ShellPkg.dec |    1 +
>>   ShellPkg/ShellPkg.dsc |    5 +
>>   .../HttpDynamicCommand/HttpApp.inf    |   58 +
>>   .../HttpDynamicCommand/HttpDynamicCommand.inf |   63 +
>>   .../DynamicCommand/HttpDynamicCommand/Http.h  |   90 +
>>   ShellPkg/Include/Guid/ShellLibHiiGuid.h   |    5 +
>>   .../DynamicCommand/HttpDynamicCommand/Http.c  | 1831 +
>>   .../HttpDynamicCommand/HttpApp.c  |   61 +
>>   .../HttpDynamicCommand/HttpDynamicCommand.c   |  137 ++
>>   .../HttpDynamicCommand/Http.uni   |  117 ++
>>   10 files changed, 2368 insertions(+)
>>   create mode 100644
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
>>   create mode 100644
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
>>   create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
>>   create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
>>   create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
>>   create mode 100644
>> ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c
>>   create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni
>>
>> diff --git a/ShellPkg/ShellPkg.dec b/ShellPkg/ShellPkg.dec
>> index d0843d338126..7b2d1230bd2c 100644
>> --- a/ShellPkg/ShellPkg.dec
>> +++ b/ShellPkg/ShellPkg.dec
>> @@ -53,6 +53,7 @@ [Guids]
>>     gShellNetwork1HiiGuid   = {0xf3d301bb, 0xf4a5, 0x45a8,
>> {0xb0, 0xb7, 0xfa, 0x99, 0x9c, 0x62, 0x37, 0xae}}
>>     gShellNetwork2HiiGuid   = {0x174b2b5, 0xf505, 0x4b12,
>> {0xaa, 0x60, 0x59, 0xdf, 0xf8, 0xd6, 0xea, 0x37}}
>>     gShellTftpHiiGuid   = {0x738a9314, 0x82c1, 0x4592,
>> {0x8f, 0xf7, 0xc1, 0xbd, 0xf1, 0xb2, 0x0e, 0xd4}}
>> +  gShellHttpHiiGuid   = {0x390f84b3, 0x221c, 0x4d9e,
>> {0xb5, 0x06, 0x6d, 0xb9, 0x42, 0x3e, 0x0a, 0x7e}}
>>     gShellBcfgHiiGuid   = {0x5f5f605d, 0x1583, 0x4a2d,
>> {0xa6, 0xb2, 0xeb, 0x12, 0xda, 0xb4, 0xa2, 0xb6}}
>>     gShellAcpiViewHiiGuid   = {0xda8ccdf4, 0xed8f, 0x4ffc,
>> {0xb5, 0xef, 0x2e, 0xf5, 0x5e, 0x24, 0x93, 0x2a}}
>>     # FILE_GUID as defined in ShellPkg/Application/Shell/Shell.inf
>> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
>> index 86e9f1e0040d..c42bc9464a0f 100644
>> --- a/ShellPkg/ShellPkg.dsc
>> +++ b/ShellPkg/ShellPkg.dsc
>> @@ -139,6 +139,11 @@ [Components]
>>     gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoIniti

[edk2-devel] [Patch] BaseTools: Sort the Pcd set when generating the VPD binary

[Bob Feng]

If VPD PcdNvStoreDefaultValueBuffer is used, all DynamicHii and
DynamicExHii PCD value will be generated into that VPD.

In order to generate the same VPD binary file in every build,
sort the Pcd set when generating VPD.

Signed-off-by: Bob Feng 
Cc: Liming Gao 
Cc: Yuwei Chen 
---
 BaseTools/Source/Python/AutoGen/PlatformAutoGen.py | 2 +-
 BaseTools/Source/Python/Workspace/DscBuildData.py  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/PlatformAutoGen.py 
b/BaseTools/Source/Python/AutoGen/PlatformAutoGen.py
index af66c48c7d..26ab8e7f36 100644
--- a/BaseTools/Source/Python/AutoGen/PlatformAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/PlatformAutoGen.py
@@ -251,11 +251,11 @@ class PlatformAutoGen(AutoGen):
 
 VariableInfo = VariableMgr(self.DscBuildDataObj._GetDefaultStores(), 
self.DscBuildDataObj.SkuIds)
 VariableInfo.SetVpdRegionMaxSize(VpdRegionSize)
 VariableInfo.SetVpdRegionOffset(VpdRegionBase)
 Index = 0
-for Pcd in DynamicPcdSet:
+for Pcd in sorted(DynamicPcdSet):
 pcdname = ".".join((Pcd.TokenSpaceGuidCName, Pcd.TokenCName))
 for SkuName in Pcd.SkuInfoList:
 Sku = Pcd.SkuInfoList[SkuName]
 SkuId = Sku.SkuId
 if SkuId is None or SkuId == '':
diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index 1afbd3eefc..4a128c8a77 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -2582,11 +2582,11 @@ class DscBuildData(PlatformBuildClassObject):
 IncludeFiles.add(IncludeFile)
 CApp = CApp + '#include <%s>\n' % (IncludeFile)
 CApp = CApp + '\n'
 for Pcd in StructuredPcds.values():
 CApp = CApp + self.GenerateArrayAssignment(Pcd)
-for PcdName in StructuredPcds:
+for PcdName in sorted(StructuredPcds.keys()):
 Pcd = StructuredPcds[PcdName]
 CApp = CApp + self.GenerateSizeFunction(Pcd)
 CApp = CApp + self.GenerateDefaultValueAssignFunction(Pcd)
 CApp = CApp + self.GenerateFdfValue(Pcd)
 CApp = CApp + self.GenerateCommandLineValue(Pcd)
-- 
2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65056): https://edk2.groups.io/g/devel/message/65056
Mute This Topic: https://groups.io/mt/76630786/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v10 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

[Maciej Rabeda]

Hi Vladimir,

One remark regarding these macros:
#define SEC_PER_MONTH   ((UINTN) 2,592,000)
#define SEC_PER_YEAR    ((UINTN) 31,536,000)

They are not being used in your code and were most probably taken from 
EmbeddedPkg/Include/Library/TimeBaseLib.h.

However - using them in a simple example:
UINTN a = SEC_PER_MONTH
DebugPrint (EFI_D_INFO, "%d\n", a);

and looking at code listing, 'a' variable will be assigned a value of 0. 
Commas should be removed from macros for them to work as designed.


I leave the decision whether this should be fixed in ShellPkg and 
EmbeddedPkg to the appropriate maintainers.


For the rest of the code:
Reviewed-by: Maciej Rabeda 

Thanks,
Maciej

On 02-Sep-20 06:08, Vladimir Olovyannikov wrote:

Introduce an http client utilizing EDK2 HTTP protocol, to
allow fast image downloading from http/https servers.
HTTP download speed is usually faster than tftp.
The client is based on the same approach as tftp dynamic command, and
uses the same UEFI Shell command line parameters. This makes it easy
integrating http into existing UEFI Shell scripts.
Note that to enable HTTP download, feature Pcd
gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must
be set to TRUE.
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860

Signed-off-by: Vladimir Olovyannikov 
Cc: Samer El-Haj-Mahmoud 
Cc: Laszlo Ersek 
Cc: Zhichao Gao 
Cc: Maciej Rabeda 
Cc: Jiaxin Wu 
Cc: Siyuan Fu 
Cc: Ray Ni 
Cc: Liming Gao 
Cc: Nd 
---
  ShellPkg/ShellPkg.dec |1 +
  ShellPkg/ShellPkg.dsc |5 +
  .../HttpDynamicCommand/HttpApp.inf|   58 +
  .../HttpDynamicCommand/HttpDynamicCommand.inf |   63 +
  .../DynamicCommand/HttpDynamicCommand/Http.h  |   90 +
  ShellPkg/Include/Guid/ShellLibHiiGuid.h   |5 +
  .../DynamicCommand/HttpDynamicCommand/Http.c  | 1831 +
  .../HttpDynamicCommand/HttpApp.c  |   61 +
  .../HttpDynamicCommand/HttpDynamicCommand.c   |  137 ++
  .../HttpDynamicCommand/Http.uni   |  117 ++
  10 files changed, 2368 insertions(+)
  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
  create mode 100644 
ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
  create mode 100644 
ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c
  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni

diff --git a/ShellPkg/ShellPkg.dec b/ShellPkg/ShellPkg.dec
index d0843d338126..7b2d1230bd2c 100644
--- a/ShellPkg/ShellPkg.dec
+++ b/ShellPkg/ShellPkg.dec
@@ -53,6 +53,7 @@ [Guids]
gShellNetwork1HiiGuid   = {0xf3d301bb, 0xf4a5, 0x45a8, {0xb0, 0xb7, 
0xfa, 0x99, 0x9c, 0x62, 0x37, 0xae}}
gShellNetwork2HiiGuid   = {0x174b2b5, 0xf505, 0x4b12, {0xaa, 0x60, 
0x59, 0xdf, 0xf8, 0xd6, 0xea, 0x37}}
gShellTftpHiiGuid   = {0x738a9314, 0x82c1, 0x4592, {0x8f, 0xf7, 
0xc1, 0xbd, 0xf1, 0xb2, 0x0e, 0xd4}}
+  gShellHttpHiiGuid   = {0x390f84b3, 0x221c, 0x4d9e, {0xb5, 0x06, 
0x6d, 0xb9, 0x42, 0x3e, 0x0a, 0x7e}}
gShellBcfgHiiGuid   = {0x5f5f605d, 0x1583, 0x4a2d, {0xa6, 0xb2, 
0xeb, 0x12, 0xda, 0xb4, 0xa2, 0xb6}}
gShellAcpiViewHiiGuid   = {0xda8ccdf4, 0xed8f, 0x4ffc, {0xb5, 0xef, 
0x2e, 0xf5, 0x5e, 0x24, 0x93, 0x2a}}
# FILE_GUID as defined in ShellPkg/Application/Shell/Shell.inf
diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
index 86e9f1e0040d..c42bc9464a0f 100644
--- a/ShellPkg/ShellPkg.dsc
+++ b/ShellPkg/ShellPkg.dsc
@@ -139,6 +139,11 @@ [Components]
gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
}
ShellPkg/DynamicCommand/TftpDynamicCommand/TftpApp.inf
+  ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf {
+
+  gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
+  }
+  ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
ShellPkg/DynamicCommand/DpDynamicCommand/DpDynamicCommand.inf {
  
gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
diff --git a/ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf 
b/ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
new file mode 100644
index ..d08d47fb37d5
--- /dev/null
+++ b/ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
@@ -0,0 +1,58 @@
+##  @file
+# Provides Shell 'http' standalone application.
+#
+# Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved. 
+# Copyright (c) 2015, ARM Ltd. All rights reserved.
+# Copyright (c) 2020, Broadcom. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010006
+  BASE_NAME  = http
+  FILE_GUID

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 13:48, Yao, Jiewen wrote:
> Jeff
> I think the CR3 should be managed by the CPU driver. And it should NOT be 
> changed by other module during post phase without a clear contract.
> 
> Since CPU driver managed the value (or DebugAgent managing it under 
> contract), it should *always* be valid. I don’t see the value to check the 
> value managed by the module itself, and return EFI_UNSUPPORTED.
> 
> If CPU driver sets CR3 above 4G and sync it to AP, then it is a bug.
> We should fix the CPU driver or MP Library to *make it always work*.
> 
> There are multiple options. For example, the CPU driver can always set a 
> dedicate CR3/GDT/IDT for AP (below 4G). Then no matter BSP state is changed, 
> we can always wake up AP correctly with a pre-defined state. Syncing BSP 
> state to AP just an implementation choice, it is not absolute necessary.
> 
> In a system boot, the ability to wake up AP is very important. Especially 
> when the BIOS transfers the state to OS, the BSP need wake up AP to put AP in 
> a good state, such as protected mode with paging disabled. Without doing 
> that, we may get random system crash during boot.
> 
> If we really really want to do some check to ensure AP state is good, I would 
> rather to prepare a dedicated known good state for AP and always use this 
> known good state for AP wakeup.
> Giving up and returning UNSUPPORTED is definitely NOT my preference.

Should we audit the various AllocatePageTableMemory() function
implementations in edk2?

Or is there more stuff we need to look at?

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65054): https://edk2.groups.io/g/devel/message/65054
Mute This Topic: https://groups.io/mt/76625679/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Yao, Jiewen]

Jeff
I think the CR3 should be managed by the CPU driver. And it should NOT be 
changed by other module during post phase without a clear contract.

Since CPU driver managed the value (or DebugAgent managing it under contract), 
it should *always* be valid. I don’t see the value to check the value managed 
by the module itself, and return EFI_UNSUPPORTED.

If CPU driver sets CR3 above 4G and sync it to AP, then it is a bug.
We should fix the CPU driver or MP Library to *make it always work*.

There are multiple options. For example, the CPU driver can always set a 
dedicate CR3/GDT/IDT for AP (below 4G). Then no matter BSP state is changed, we 
can always wake up AP correctly with a pre-defined state. Syncing BSP state to 
AP just an implementation choice, it is not absolute necessary.

In a system boot, the ability to wake up AP is very important. Especially when 
the BIOS transfers the state to OS, the BSP need wake up AP to put AP in a good 
state, such as protected mode with paging disabled. Without doing that, we may 
get random system crash during boot.

If we really really want to do some check to ensure AP state is good, I would 
rather to prepare a dedicated known good state for AP and always use this known 
good state for AP wakeup.
Giving up and returning UNSUPPORTED is definitely NOT my preference.


Thank you
Yao Jiewen







From: devel@edk2.groups.io  On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 7:17 PM
To: vanjeff_...@hotmail.com; Yao, Jiewen ; 
devel@edk2.groups.io; ler...@redhat.com; Dong, Eric ; Ni, 
Ray 
Cc: Lou, Yun 
Subject: Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check 
for CR3/GDT/IDT.

Jiewen,

Maybe my word "valid during post phase" confused you. My point is that those 
values maybe changed during post phase.

So, if their spaces above 4GB are not legal, i agree not to do anything. If 
legal, then we need to something.

Jeff



在 Fan Jeff mailto:vanjeff_...@hotmail.com>>，2020年9月4日 
下午4:44写道：
Jiewen,

I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT 
corrently firstly.  Othersize, everything does not make sense.

For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are 
any limitation on it, please corrent me) but the current CPU AP wakeup method 
does not allow CR3 above 4GB space, this maybe the CPU driver’s bug or 
implementation limitation.

The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell 
the caller.

Jeff

发件人: Yao, Jiewen
发送时间: 2020年9月4日 16:07
收件人: devel@edk2.groups.io; 
vanjeff_...@hotmail.com; Laszlo 
Ersek; Dong, Eric; Ni, 
Ray
抄送: Lou, Yun
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

When we say “validate”, we need get clear on what is the contract between 
caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with 
below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the 
validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the 
validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on 
why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that 
which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct 
and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state 
changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io 
mailto:devel@edk2.groups.io>> On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek mailto:ler...@redhat.com>>; 
devel@edk2.groups.io; Dong, Eric 
mailto:eric.d...@intel.com>>; Ni, Ray 
mailto:ray...@intel.com>>
Cc: Lou, Yun mailto:yun@intel.com>>
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check fo

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

Jiewen,

Maybe my word "valid during post phase" confused you. My point is that those values maybe changed during post phase.

So, if their spaces above 4GB are not legal, i agree not to do anything. If legal, then we need to something.

Jeff



在 Fan Jeff ，2020年9月4日 下午4:44写道：


Jiewen,
 
I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT corrently firstly.  Othersize, everything does not make sense.
 
For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are any limitation on it, please corrent me) but the current CPU AP wakeup method does not allow CR3 above 4GB space, this maybe the CPU driver’s
 bug or implementation limitation. 
 
The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell the caller.
 
Jeff

 

发件人: 
Yao, Jiewen
发送时间: 2020年9月4日 16:07
收件人: devel@edk2.groups.io;
vanjeff_...@hotmail.com; 
Laszlo Ersek; Dong, Eric; 
Ni, Ray
抄送: Lou, Yun
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

 
When we say “validate”, we need get clear on what is the contract between caller and callee, and what is the expectation of the validation.
 
For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

if the GDT table is valid.
if the IDT table is validif the exception handler is validif the timer handler still worksif the page table is validif the page table is 1:1 mappingif the page table still enforce the expected protection, such as RO, NX……
 
If an application creates a crazy state, CpuDxe may pass the validation with below 4G page table, but still fail to wake up APs.
 
If it is the app’s responsibility to ensure the system in good state, the validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the validation is insufficient.
 
I think we should wait. I am working with Eric to collect the requirement on why test case is designed in this way.
 
DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.
 
 
 
 


From: devel@edk2.groups.io 
On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek ; devel@edk2.groups.io; Dong, Eric ; Ni, Ray 
Cc: Lou, Yun 
Subject: 回复:
回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.


 
Laszlo,
 
Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is that we CANNOT
 assume original values are still valid during POST phase.
 
On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one function. Validating them are necessary.
 
For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.
 
Jeff
 

发件人: Laszlo Ersek
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff;
devel@edk2.groups.io; 
eric.d...@intel.com; Ni, Ray
抄送: Lou, Yun
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

 
On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
> 
> Introducing one new PCD to handle such rare test case is too heavy. I think We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Re: [edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Abner Chang]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, September 04, 2020 5:44 PM
> To: Chang, Abner (HPS SW/FW Technologist) ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Leif Lindholm ;
> Michael D Kinney ; Leif Lindholm
> 
> Subject: Re: [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64
> 
> Hi Abner,
> 
> On 09/04/20 10:51, Chang, Abner (HPS SW/FW Technologist) wrote:
> >
> >
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Friday, September 04, 2020 4:25 PM
> >> To: Chang, Abner (HPS SW/FW Technologist) ;
> >> devel@edk2.groups.io
> >> Cc: Andrew Fish ; Leif Lindholm ;
> >> Michael D Kinney ; Leif Lindholm
> >> 
> >> Subject: Re: [PATCH v2 1/1] Maintainers.txt: Update reviewers of
> >> */RiscV64
> >>
> >> On 09/04/20 09:19, Abner Chang wrote:
> >>> Add reviewers for all /RiscV64 folders.
> >>>
> >>> Signed-off-by: Abner Chang 
> >>> Cc: Andrew Fish 
> >>> Cc: Laszlo Ersek 
> >>> Cc: Leif Lindholm 
> >>> Cc: Michael D Kinney 
> >>> Cc: Leif Lindholm 
> >>> Acked-by: Laszlo Ersek 
> >>> ---
> >>>  Maintainers.txt | 5 +
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Maintainers.txt b/Maintainers.txt index
> >>> 57cd2fc662..6a22a14796 100644
> >>> --- a/Maintainers.txt
> >>> +++ b/Maintainers.txt
> >>> @@ -98,6 +98,11 @@ F: */Arm/
> >>>  M: Leif Lindholm 
> >>>  M: Ard Biesheuvel 
> >>>
> >>> +RISCV64
> >>> +F: */RiscV64/
> >>> +M: Abner Chang 
> >>> +R: Daniel Schaefer 
> >>> +
> >>>  EDK II Continuous Integration:
> >>>  --
> >>>  .azurepipelines/
> >>>
> >>
> >> What are the changes relative to v1?
> > R: for Abner changed to M: for Abner
> >>
> >> (Also, I believe Leif is away at the moment, and I think we should
> >> have his ACK on this patch, before we merge the patch.)
> > Yes, I had few conversations with Leif and Mike for this in this.
> 
> Wait, that reminds me... OK, I've looked up the v1 discussion now.
> 
> Is there any particular reason you didn't pick up Leif's Reviewed-by from the
> v1 thread? Leif gave his R-b conditional on the R->M change for your name:
Oops, I forget this. No particular reason to not picking up Leif's r-b.
> 
> https://edk2.groups.io/g/devel/message/64807
> 
> and you have implemented that change. So I think Leif's R-b should have
> been picked up.
> 
> Based on  -- do you have
> push access to edk2 at this time? Because if that's the case, then we should
> merge this patch now.
> 
> Hmm let me see.
> 
> https://github.com/orgs/tianocore/teams/edk-ii-maintainers/members
> 
> Yes, you are a member of this group.
> 
> https://github.com/orgs/tianocore/teams?query=@changab
> 
> So I'm going to apply Leif's R-b from the v1 thread now, and then merge this
> patch.
Sure, thanks. 
Abner

> 
> Thanks,
> Laszlo
> 
> > Sure we can wait for his Ack.
> > Thanks
> >>
> >> Thanks
> >> Laszlo
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65051): https://edk2.groups.io/g/devel/message/65051
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Laszlo Ersek]

On 09/04/20 09:19, Abner Chang wrote:
> Add reviewers for all /RiscV64 folders.
> 
> Signed-off-by: Abner Chang 
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> Cc: Leif Lindholm 
> Acked-by: Laszlo Ersek 
> ---
>  Maintainers.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 57cd2fc662..6a22a14796 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -98,6 +98,11 @@ F: */Arm/
>  M: Leif Lindholm 
>  M: Ard Biesheuvel 
>  
> +RISCV64
> +F: */RiscV64/
> +M: Abner Chang 
> +R: Daniel Schaefer 
> +
>  EDK II Continuous Integration:
>  --
>  .azurepipelines/
> 

Merged as commit 2ace920de1e9, via
.

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65050): https://edk2.groups.io/g/devel/message/65050
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Laszlo Ersek]

Hi Abner,

On 09/04/20 10:51, Chang, Abner (HPS SW/FW Technologist) wrote:
> 
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Friday, September 04, 2020 4:25 PM
>> To: Chang, Abner (HPS SW/FW Technologist) ;
>> devel@edk2.groups.io
>> Cc: Andrew Fish ; Leif Lindholm ;
>> Michael D Kinney ; Leif Lindholm
>> 
>> Subject: Re: [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64
>>
>> On 09/04/20 09:19, Abner Chang wrote:
>>> Add reviewers for all /RiscV64 folders.
>>>
>>> Signed-off-by: Abner Chang 
>>> Cc: Andrew Fish 
>>> Cc: Laszlo Ersek 
>>> Cc: Leif Lindholm 
>>> Cc: Michael D Kinney 
>>> Cc: Leif Lindholm 
>>> Acked-by: Laszlo Ersek 
>>> ---
>>>  Maintainers.txt | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Maintainers.txt b/Maintainers.txt index
>>> 57cd2fc662..6a22a14796 100644
>>> --- a/Maintainers.txt
>>> +++ b/Maintainers.txt
>>> @@ -98,6 +98,11 @@ F: */Arm/
>>>  M: Leif Lindholm 
>>>  M: Ard Biesheuvel 
>>>
>>> +RISCV64
>>> +F: */RiscV64/
>>> +M: Abner Chang 
>>> +R: Daniel Schaefer 
>>> +
>>>  EDK II Continuous Integration:
>>>  --
>>>  .azurepipelines/
>>>
>>
>> What are the changes relative to v1?
> R: for Abner changed to M: for Abner
>>
>> (Also, I believe Leif is away at the moment, and I think we should have his
>> ACK on this patch, before we merge the patch.)
> Yes, I had few conversations with Leif and Mike for this in this.

Wait, that reminds me... OK, I've looked up the v1 discussion now.

Is there any particular reason you didn't pick up Leif's Reviewed-by
from the v1 thread? Leif gave his R-b conditional on the R->M change for
your name:

https://edk2.groups.io/g/devel/message/64807

and you have implemented that change. So I think Leif's R-b should have
been picked up.

Based on  -- do you have
push access to edk2 at this time? Because if that's the case, then we
should merge this patch now.

Hmm let me see.

https://github.com/orgs/tianocore/teams/edk-ii-maintainers/members

Yes, you are a member of this group.

https://github.com/orgs/tianocore/teams?query=@changab

So I'm going to apply Leif's R-b from the v1 thread now, and then merge
this patch.

Thanks,
Laszlo

> Sure we can wait for his Ack.
> Thanks
>>
>> Thanks
>> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65049): https://edk2.groups.io/g/devel/message/65049
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Abner Chang]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, September 04, 2020 4:25 PM
> To: Chang, Abner (HPS SW/FW Technologist) ;
> devel@edk2.groups.io
> Cc: Andrew Fish ; Leif Lindholm ;
> Michael D Kinney ; Leif Lindholm
> 
> Subject: Re: [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64
> 
> On 09/04/20 09:19, Abner Chang wrote:
> > Add reviewers for all /RiscV64 folders.
> >
> > Signed-off-by: Abner Chang 
> > Cc: Andrew Fish 
> > Cc: Laszlo Ersek 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > Cc: Leif Lindholm 
> > Acked-by: Laszlo Ersek 
> > ---
> >  Maintainers.txt | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt index
> > 57cd2fc662..6a22a14796 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -98,6 +98,11 @@ F: */Arm/
> >  M: Leif Lindholm 
> >  M: Ard Biesheuvel 
> >
> > +RISCV64
> > +F: */RiscV64/
> > +M: Abner Chang 
> > +R: Daniel Schaefer 
> > +
> >  EDK II Continuous Integration:
> >  --
> >  .azurepipelines/
> >
> 
> What are the changes relative to v1?
R: for Abner changed to M: for Abner
> 
> (Also, I believe Leif is away at the moment, and I think we should have his
> ACK on this patch, before we merge the patch.)
Yes, I had few conversations with Leif and Mike for this in this.
Sure we can wait for his Ack.
Thanks
> 
> Thanks
> Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65048): https://edk2.groups.io/g/devel/message/65048
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

Jiewen,

I don’t think AP need to validate everyting. BSP should make sure CR3/GDT/IDT 
corrently firstly.  Othersize, everything does not make sense.

For CR3 case, if we assume CR3 above 4GB space is legal for BSP (If there are 
any limitation on it, please corrent me) but the current CPU AP wakeup method 
does not allow CR3 above 4GB space, this maybe the CPU driver’s bug or 
implementation limitation.

The key is that CR3 above 4GB is legal or not.
If we think this is bug, we need to enhance CPU driver to fix it.
If we think this is one limiation, we need to validate the CR3 value and tell 
the caller.

Jeff

发件人: Yao, Jiewen
发送时间: 2020年9月4日 16:07
收件人: devel@edk2.groups.io; 
vanjeff_...@hotmail.com; Laszlo 
Ersek; Dong, Eric; Ni, 
Ray
抄送: Lou, Yun
主题: RE: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

When we say “validate”, we need get clear on what is the contract between 
caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with 
below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the 
validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the 
validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on 
why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that 
which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct 
and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state 
changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io  On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek ; devel@edk2.groups.io; Dong, Eric 
; Ni, Ray 
Cc: Lou, Yun 
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using 
old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is 
that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one 
function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source 
level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff; 
devel@edk2.groups.io; 
eric.d...@intel.com; Ni, 
Ray
抄送: Lou, Yun
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think 
> We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same 

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 10:06, Yao, Jiewen wrote:
> When we say “validate”, we need get clear on what is the contract between 
> caller and callee, and what is the expectation of the validation.
> 
> For example, in this case, the validation is limited to 4G or not 4G. Why?
> This function does not verify following, why?
> 
>   1.  if the GDT table is valid.
>   2.  if the IDT table is valid
>   3.  if the exception handler is valid
>   4.  if the timer handler still works
>   5.  if the page table is valid
>   6.  if the page table is 1:1 mapping
>   7.  if the page table still enforce the expected protection, such as RO, NX
>   8.  ……

Yes, very good point; it has crossed my mind before too. The currently
proposed checks verify the CR3 (but not the end of the root page
directory). They also don't try to walk the whole forest of page tables
and check every entry against 4GB (or, as you say, for 1:1 mapping). The
check covers the GDT and the IDT, but not the GDT and IDT entries
(segment granularity? direction of growth?)

I'm OK with the proposed rudimentary checks because in my mind they are
supposed to catch only one idiosyncratic UEFI application.

> If an application creates a crazy state, CpuDxe may pass the validation with 
> below 4G page table, but still fail to wake up APs.

Yes, absolutely.

> 
> If it is the app’s responsibility to ensure the system in good state, the 
> validation is unnecessary.
> If it is the CpuDxe’s responsibility to ensure the system in good state, the 
> validation is insufficient.

Agreed on both counts.

I'm just under the impression that Eric has some internal use case that
doesn't let him fix the application -- or maybe there's no time left for
them for fixing the application.

> 
> I think we should wait. I am working with Eric to collect the requirement on 
> why test case is designed in this way.

Sounds good, thanks!

> 
> DebugAgentDxe is a good case. It is for debug purpose.
> I believe there must be contract between CPU driver and DebugAgentDxe that 
> which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
> It is DebugAgentDxe’s responsibility to ensure the new system state is 
> correct and compatible with the CPU driver.

Agreed 100%

> With the contract, I don’t think CPU driver need validate the system state 
> changed by DebugAgentDxe.
> If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.

Agreed again.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65046): https://edk2.groups.io/g/devel/message/65046
Mute This Topic: https://groups.io/mt/76625321/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Laszlo Ersek]

On 09/04/20 09:19, Abner Chang wrote:
> Add reviewers for all /RiscV64 folders.
> 
> Signed-off-by: Abner Chang 
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> Cc: Leif Lindholm 
> Acked-by: Laszlo Ersek 
> ---
>  Maintainers.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 57cd2fc662..6a22a14796 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -98,6 +98,11 @@ F: */Arm/
>  M: Leif Lindholm 
>  M: Ard Biesheuvel 
>  
> +RISCV64
> +F: */RiscV64/
> +M: Abner Chang 
> +R: Daniel Schaefer 
> +
>  EDK II Continuous Integration:
>  --
>  .azurepipelines/
> 

What are the changes relative to v1?

(Also, I believe Leif is away at the moment, and I think we should have
his ACK on this patch, before we merge the patch.)

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65045): https://edk2.groups.io/g/devel/message/65045
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 09:32, Fan Jeff wrote:
> Laszlo,
>
> Why sync the BSP's CR3/GDT/IDT values for AP when AP wakes up instead
> of using old cached BSP's CR3/GDT/IDT values when CPU driver
> initiallly dispatched is that we CANNOT assume original values are
> still valid during POST phase.
>
> On this senario, BSP's CR3/GDT/IDT are just like input parameters for
> one function. Validating them are necessary.
>
> For example, DebugAgentDxe driver may be loaded in UEFI shell to setup
> source level debugging enviroment of EDKII debugger tools.  It may
> setup new IDT space.

Then DebugAgentDxe should be changed to allocate the new IDT in the
32-bit address space.

I don't think it's acceptable that loading DebugAgentDxe from the UEFI
shell may or may not render the MP services protocol unusable. What if
the programmer wants to debug something related to MP services? "I've
loaded DebugAgentDxe, but now I cannot start the payload I want to
debug."

As far as I can see,
"SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgent/DxeDebugAgentLib.c"
uses the static variable "mIdtEntryTable" as IDT. I think it should be
possible to replace "mIdtEntryTable" with a dynamically allocated
object. The allocation should be restricted to 32-bit. It should be
possible to perform the allocation very early (perhaps in the library
constructor).

Again, I'm OK with adding ASSERT()s to FillExchangeInfoData(). If IDT /
GDT / CR3 are out of 32-bit address space, then that's a programming
error, or a platform misconfiguration. It's not something we should try
to dynamically recover from.

The MP services protocol implementation may not expect that the
CR3/GDT/IDT remain unchanged on the BSP during the DXE/BDS phases, but
it does expect them to remain under 4GB.

- A UEFI driver or app must not break this assumption at all, because a
  UEFI driver or app has no business messing with these artifacts.

- Whereas a DXE driver (such as DebugAgentDxe), exactly because it is
  part of the platform firmware, is expected to collaborate with CpuDxe
  / MpInitLib, and to satisfy the invariants.

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65044): https://edk2.groups.io/g/devel/message/65044
Mute This Topic: https://groups.io/mt/76625078/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Yao, Jiewen]

When we say “validate”, we need get clear on what is the contract between 
caller and callee, and what is the expectation of the validation.

For example, in this case, the validation is limited to 4G or not 4G. Why?
This function does not verify following, why?

  1.  if the GDT table is valid.
  2.  if the IDT table is valid
  3.  if the exception handler is valid
  4.  if the timer handler still works
  5.  if the page table is valid
  6.  if the page table is 1:1 mapping
  7.  if the page table still enforce the expected protection, such as RO, NX
  8.  ……

If an application creates a crazy state, CpuDxe may pass the validation with 
below 4G page table, but still fail to wake up APs.

If it is the app’s responsibility to ensure the system in good state, the 
validation is unnecessary.
If it is the CpuDxe’s responsibility to ensure the system in good state, the 
validation is insufficient.

I think we should wait. I am working with Eric to collect the requirement on 
why test case is designed in this way.

DebugAgentDxe is a good case. It is for debug purpose.
I believe there must be contract between CPU driver and DebugAgentDxe that 
which status DebugAgentDxe may modify and which state DebugAgentDxe may not.
It is DebugAgentDxe’s responsibility to ensure the new system state is correct 
and compatible with the CPU driver.
With the contract, I don’t think CPU driver need validate the system state 
changed by DebugAgentDxe.
If DebugAgentDxe put system in a wrong state, CPUDriver has no chance to run.




From: devel@edk2.groups.io  On Behalf Of Fan Jeff
Sent: Friday, September 4, 2020 3:32 PM
To: Laszlo Ersek ; devel@edk2.groups.io; Dong, Eric 
; Ni, Ray 
Cc: Lou, Yun 
Subject: 回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using 
old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is 
that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one 
function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source 
level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff; 
devel@edk2.groups.io; 
eric.d...@intel.com; Ni, 
Ray
抄送: Lou, Yun
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think 
> We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray; 
> devel@edk2.groups.io>;
>  ler...@redhat.com
> 抄送: Lou, Yun
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
> CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this 
> special shell application case. He wants to use default FALSE to always 
> ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray mailto:ray..

[edk2-devel] EDK II Stable Tag release edk2-stable202008 completed

[gaoliming]

Hi, all

 

The tag edk2-stable202008 has been created.
https://github.com/tianocore/edk2/releases/tag/edk2-stable202008

  git clone -b edk2-stable202008 https://github.com/tianocore/edk2.git

 

The tag edk2-stable202008 has been added into the main EDK II Wiki page.

  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II

 

The quiet period has now ended. Thank you for your cooperation and patience.
Normal commits can now be resumed.

 

Next edk2 stable tag (edk2-stable202011) planning has been added into wiki
page.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Plannin
g.

 

If you have ideas for features in the next stable tag, please enter a
Bugzilla for evaluation. Please let me know if there are existing open
Bugzilla entries that should be targeted at this next stable tag.

 

Thanks

Liming


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65042): https://edk2.groups.io/g/devel/message/65042
Mute This Topic: https://groups.io/mt/76625313/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/1] Maintainers.txt: Update reviewers of */RiscV64

[Abner Chang]

Add reviewers for all /RiscV64 folders.

Signed-off-by: Abner Chang 
Cc: Andrew Fish 
Cc: Laszlo Ersek 
Cc: Leif Lindholm 
Cc: Michael D Kinney 
Cc: Leif Lindholm 
Acked-by: Laszlo Ersek 
---
 Maintainers.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 57cd2fc662..6a22a14796 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -98,6 +98,11 @@ F: */Arm/
 M: Leif Lindholm 
 M: Ard Biesheuvel 
 
+RISCV64
+F: */RiscV64/
+M: Abner Chang 
+R: Daniel Schaefer 
+
 EDK II Continuous Integration:
 --
 .azurepipelines/
-- 
2.25.0


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65041): https://edk2.groups.io/g/devel/message/65041
Mute This Topic: https://groups.io/mt/76625278/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 3/3] IntelSiliconPkg/PlatformVTdInfoSamplePei: Install Null Root Entry Table

[Chaganty, Rangasai V]

Reviewed-by: Sai Chaganty 

-Original Message-
From: Sheng, W  
Sent: Sunday, August 30, 2020 11:38 PM
To: devel@edk2.groups.io
Cc: Ni, Ray ; Chaganty, Rangasai V 

Subject: [PATCH v3 3/3] IntelSiliconPkg/PlatformVTdInfoSamplePei: Install Null 
Root Entry Table

BIOS uses TE with a null root entry table to block VT-d engine access to block 
any DMA traffic in pre-memory phase.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2867

Change-Id: I6c086c1f26e60f781de79cc37677cc5717c5edec
Cc: Ray Ni 
Cc: Rangasai V Chaganty 
Signed-off-by: Sheng Wei 
---
 .../PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c  | 16 
 .../PlatformVTdInfoSamplePei.inf |  3 ++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
index 6f6c14f7..616a96ce 100644
--- 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei
+++ /PlatformVTdInfoSamplePei.c
@@ -9,6 +9,7 @@
 #include 
 
 #include 
+#include 
 
 #include 
 #include 
@@ -164,6 +165,15 @@ EFI_PEI_PPI_DESCRIPTOR mPlatformVTdNoIgdInfoSampleDesc = {
   &mPlatformVTdNoIgdSample
 };
 
+// BIOS uses TE with a null root entry table to block VT-d engine access to 
block any DMA traffic in pre-memory phase.
+EDKII_VTD_NULL_ROOT_ENTRY_TABLE_PPI mNullRootEntryTable = 0xFED2;
+
+EFI_PEI_PPI_DESCRIPTOR mPlatformNullRootEntryTableDesc = {
+  (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
+  &gEdkiiVTdNullRootEntryTableGuid,
+  &mNullRootEntryTable
+};
+
 /**
   Initialize VTd register.
   Initialize the VTd hardware unit which has INCLUDE_PCI_ALL set @@ -344,6 
+354,12 @@ PlatformVTdInfoSampleInitialize (
   if (!EFI_ERROR(Status)) {
 SiliconInitialized = TRUE;
   }
+
+  Status = PeiServicesInstallPpi (&mPlatformNullRootEntryTableDesc);
+  if (EFI_ERROR (Status)) {
+ASSERT_EFI_ERROR (Status);
+  }
+
   DEBUG ((DEBUG_INFO, "SiliconInitialized - %x\n", SiliconInitialized));
   if (!SiliconInitialized) {
 Status = PeiServicesNotifyPpi (&mSiliconInitializedNotifyList); diff --git 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
index dacfdf5e..b35853b6 100644
--- 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei
+++ /PlatformVTdInfoSamplePei.inf
@@ -38,7 +38,8 @@
   IoLib
 
 [Ppis]
-  gEdkiiVTdInfoPpiGuid ## PRODUCES
+  gEdkiiVTdInfoPpiGuid  ## PRODUCES
+  gEdkiiVTdNullRootEntryTableGuid   ## PRODUCES
 
 [Depex]
   gEfiPeiMasterBootModePpiGuid
--
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65040): https://edk2.groups.io/g/devel/message/65040
Mute This Topic: https://groups.io/mt/76529335/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 2/3] IntelSiliconPkg/IntelVTdPmrPei: Fix PMR enabling setting confilct

[Chaganty, Rangasai V]

Reviewed-by: Sai Chaganty 

-Original Message-
From: Sheng, W  
Sent: Sunday, August 30, 2020 11:38 PM
To: devel@edk2.groups.io
Cc: Ni, Ray ; Chaganty, Rangasai V 

Subject: [PATCH v3 2/3] IntelSiliconPkg/IntelVTdPmrPei: Fix PMR enabling 
setting confilct

PMR enabling set by pre-boot DMA protection is cleared by RC when boot guard is 
enabled. Pre-boot DMA protection should only reset VT-d BAR when it is 0 and 
reset PMR region when it is not programmed to protect all memory address.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2867

Change-Id: Ic5370f474a43a94903871782ace5cce186b4ddc0
Cc: Ray Ni 
Cc: Rangasai V Chaganty 
Signed-off-by: Sheng Wei 
---
 .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c| 14 +++
 .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h| 15 +++
 .../Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf  |  1 +
 .../Feature/VTd/IntelVTdPmrPei/VtdReg.c| 47 ++
 4 files changed, 77 insertions(+)

diff --git 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
index ea944aa4..31a14f28 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdP
+++ mrPei.c
@@ -745,7 +745,21 @@ VTdInfoNotify (
 // Protect all system memory
 //
 InitVTdInfo ();
+
+Hob = GetFirstGuidHob (&mVTdInfoGuid);
+VTdInfo = GET_GUID_HOB_DATA(Hob);
+
+//
+// NOTE: We need check if PMR is enabled or not.
+//
+EnabledEngineMask = GetDmaProtectionEnabledEngineMask (VTdInfo, 
VTdInfo->EngineMask);
+if (EnabledEngineMask != 0) {
+  Status = PreMemoryEnableVTdTranslationProtection (VTdInfo, 
EnabledEngineMask);
+}
 InitVTdPmrForAll ();
+if (((EnabledEngineMask != 0) && (!EFI_ERROR (Status {
+  DisableVTdTranslationProtection (VTdInfo, EnabledEngineMask);
+}
 
 //
 // Install PPI.
diff --git 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h
index 58e6afad..ffed2c5b 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdP
+++ mrPei.h
@@ -97,6 +97,21 @@ GetHighMemoryAlignment (
   IN UINT64EngineMask
   );
 
+/**
+  Enable VTd translation table protection in pre-memory phase.
+
+  @param VTdInfoThe VTd engine context information.
+  @param EngineMask The mask of the VTd engine to be accessed.
+
+  @retval EFI_SUCCESS   DMAR translation protection is enabled.
+  @retval EFI_UNSUPPORTED   Null Root Entry Table is not supported.
+**/
+EFI_STATUS
+PreMemoryEnableVTdTranslationProtection (
+  IN VTD_INFO  *VTdInfo,
+  IN UINT64EngineMask
+  );
+
 /**
   Enable VTd translation table protection.
 
diff --git 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
index 3eb2b510..1e613ddd 100644
--- 
a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdP
+++ mrPei.inf
@@ -48,6 +48,7 @@
   gEdkiiVTdInfoPpiGuid## CONSUMES
   gEfiPeiMemoryDiscoveredPpiGuid  ## CONSUMES
   gEfiEndOfPeiSignalPpiGuid   ## CONSUMES
+  gEdkiiVTdNullRootEntryTableGuid ## PRODUCES
 
 [Pcd]
   gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask   ## CONSUMES
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/VtdReg.c 
b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/VtdReg.c
index c9669426..2e252fe5 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/VtdReg.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/VtdReg.c
@@ -13,8 +13,10 @@
 #include 
 #include   #include 

+#include 
 #include 
 #include 
+#include 
 
 #include "IntelVTdPmrPei.h"
 
@@ -246,6 +248,51 @@ DisableDmar (
   return EFI_SUCCESS;
 }
 
+/**
+  Enable VTd translation table protection in pre-memory phase.
+
+  @param VTdInfoThe VTd engine context information.
+  @param EngineMask The mask of the VTd engine to be accessed.
+
+  @retval EFI_SUCCESS   DMAR translation protection is enabled.
+  @retval EFI_UNSUPPORTED   Null Root Entry Table is not supported.
+**/
+EFI_STATUS
+PreMemoryEnableVTdTranslationProtection (
+  IN VTD_INFO  *VTdInfo,
+  IN UINT64EngineMask
+  )
+{
+  EFI_STATUSStatus;
+  UINTN Index;
+  EDKII_VTD_NULL_ROOT_ENTRY_TABLE_PPI   *RootEntryTable;
+
+  DEBUG ((DEBUG_INFO, "PreMemoryEnableVTdTranslationProtection - 
+ 0x%lx\n", EngineMask));
+
+  Status = PeiServicesLocatePpi (
+ &gEdkiiVTdNullRootEntryTableGuid,

Re: [edk2-devel] [PATCH v3 1/3] IntelSiliconPkg/VtdInfo: Add Null Root Entry Table PPI

[Chaganty, Rangasai V]

Reviewed-by: Sai Chaganty 

-Original Message-
From: Sheng, W  
Sent: Sunday, August 30, 2020 11:38 PM
To: devel@edk2.groups.io
Cc: Ni, Ray ; Chaganty, Rangasai V 

Subject: [PATCH v3 1/3] IntelSiliconPkg/VtdInfo: Add Null Root Entry Table PPI

Null root entry table address is a fixed silicon reserved address, which is 
used to block the DMA transfer.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2867

Change-Id: I3aa2b2e7a11e0327857c6ed9bc92cd209d3ade9d
Cc: Ray Ni 
Cc: Rangasai V Chaganty 
Signed-off-by: Sheng Wei 
---
 .../Include/Ppi/VtdNullRootEntryTable.h| 28 ++
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec  |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Include/Ppi/VtdNullRootEntryTable.h

diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Ppi/VtdNullRootEntryTable.h 
b/Silicon/Intel/IntelSiliconPkg/Include/Ppi/VtdNullRootEntryTable.h
new file mode 100644
index ..d79b5fd9
--- /dev/null
+++ b/Silicon/Intel/IntelSiliconPkg/Include/Ppi/VtdNullRootEntryTable.h
@@ -0,0 +1,28 @@
+/** @file
+  The definition for VTD Null Root Entry Table PPI.
+
+  This is a lightweight VTd null root entry table report in PEI phase.
+
+  Copyright (c) 2020, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __VTD_NULL_ROOT_ENTRY_TABLE_PPI_H__
+#define __VTD_NULL_ROOT_ENTRY_TABLE_PPI_H__
+
+#define EDKII_VTD_NULL_ROOT_ENTRY_TABLE_PPI_GUID \
+{ \
+  0x3de0593f, 0x6e3e, 0x4542, { 0xa1, 0xcb, 0xcb, 0xb2, 0xdb, 0xeb, 0xd8, 
0xff } \
+}
+
+//
+// Null root entry table address is a fixed silicon reserved address,
+//   which is used to block the DMA transfer.
+//
+typedef UINT64  EDKII_VTD_NULL_ROOT_ENTRY_TABLE_PPI;
+
+extern EFI_GUID gEdkiiVTdNullRootEntryTableGuid;
+
+#endif
+
diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec 
b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
index e4a7fec3..284820af 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -68,6 +68,7 @@
 
 [Ppis]
   gEdkiiVTdInfoPpiGuid = { 0x8a59fcb3, 0xf191, 0x400c, { 0x97, 0x67, 0x67, 
0xaf, 0x2b, 0x25, 0x68, 0x4a } }
+  gEdkiiVTdNullRootEntryTableGuid = { 0x3de0593f, 0x6e3e, 0x4542, { 
+ 0xa1, 0xcb, 0xcb, 0xb2, 0xdb, 0xeb, 0xd8, 0xff } }
 
 [Protocols]
   gEdkiiPlatformVTdPolicyProtocolGuid = { 0x3d17e448, 0x466, 0x4e20, { 0x99, 
0x9f, 0xb2, 0xe1, 0x34, 0x88, 0xee, 0x22 }}
--
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#65038): https://edk2.groups.io/g/devel/message/65038
Mute This Topic: https://groups.io/mt/76529333/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

回复: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT.

[Fan Jeff]

Laszlo,

Why sync the BSP’s CR3/GDT/IDT values for AP when AP wakes up instead of using 
old cached BSP’s CR3/GDT/IDT values when CPU driver initiallly dispatched is 
that we CANNOT assume original values are still valid during POST phase.

On this senario, BSP’s CR3/GDT/IDT are just like input parameters for one 
function. Validating them are necessary.

For example, DebugAgentDxe driver may be loaded in UEFI shell to setup source 
level debugging enviroment of EDKII debugger tools.  It may setup new IDT space.

Jeff

发件人: Laszlo Ersek
发送时间: 2020年9月4日 14:58
收件人: Fan Jeff; 
devel@edk2.groups.io; 
eric.d...@intel.com; Ni, 
Ray
抄送: Lou, Yun
主题: Re: 回复: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
CR3/GDT/IDT.

On 09/04/20 04:18, Fan Jeff wrote:
> Laszlo & Eric,
>
> Introducing one new PCD to handle such rare test case is too heavy. I think 
> We could do validating CR3/GDT/IDT space < 4GB address always in MpInitLib.

I disagree.

What the UEFI application does (interfere with GDT / IDT / CR3
placement) is invalid. It changes system properties under the feet of
platform DXE drivers. UEFI applications are supposed to be written
against public protocols and services in the UEFI spec (and maybe in the
PI spec).

If this application is a test application that purposely massages
low-level system properties, that's fine; but then, if we change core
edk2 components to be somewhat compatible with this application, then we
should make sure that platforms that do not care about this specific use
case do not suffer a performance hit or a code complexity hit.

What I could accept, under your proposal, is the following: add three
ASSERT()s to FillExchangeInfoData(), where we fetch the IDTR / GDTR /
CR3 anyway. This would be fine because it only expresses existing
assumptions / requirements.

However, my understanding is that this would not solve Eric's problem.
The system would hang -- in DEBUG / NOOPT builds -- or crash -- in a
RELEASE build -- just the same as before.

Now, *if* FillExchangeInfoData() is currently *wrong* to have these
32-bit assumptions, because edk2 modules themselves can break those
assumptions (without the custom UEFI application), then we have a more
serious problem. But for that problem, just "checking and rejecting" is
not a sufficient solution, regardless of how and where we check and reject.

Thanks
Laszlo




>
> Jeff
>
> 发件人: Dong, Eric
> 发送时间: 2020年9月4日 10:01
> 收件人: Ni, Ray; 
> devel@edk2.groups.io; 
> ler...@redhat.com
> 抄送: Lou, Yun
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
> CR3/GDT/IDT.
>
> I guess Laszlo think this check is not always needed, just used for this 
> special shell application case. He wants to use default FALSE to always 
> ignore this check and make code logic clear.
>
> Thanks,
> Eric
>
> From: Ni, Ray 
> Sent: Friday, September 4, 2020 9:34 AM
> To: devel@edk2.groups.io; ler...@redhat.com; Dong, Eric 
> Cc: Lou, Yun 
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
> CR3/GDT/IDT.
>
> Why do we need a new PCD to control such check? Under what circumstance the 
> PCD is false?
> We may need to move such check out of MpLib.c. Because when bps runs at 32bit 
> mode, AP doesn’t need to switch to long mode, such check is not needed and 
> also always passes.
>
> We should not assume PEI runs at 32 bit mode.
>
>
> 发件人: devel@edk2.groups.io 
> mailto:devel@edk2.groups.io>> 代表 Laszlo Ersek 
> mailto:ler...@redhat.com>>
> 发送时间: Friday, September 4, 2020 3:55:47 AM
> 收件人: Dong, Eric mailto:eric.d...@intel.com>>; 
> devel@edk2.groups.io 
> mailto:devel@edk2.groups.io>>
> 抄送: Ni, Ray mailto:ray...@intel.com>>
> 主题: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Add check for 
> CR3/GDT/IDT.
>
> On 09/03/20 21:00, Laszlo Ersek wrote:
>
>> (10) More importantly, ValidCR3GdtIdtCheck() should not be called in the
>> Worker functions for StartupAllAPs, StartupThisAP, SwitchBSP, and
>> EnableDisableAP, in "UefiCpuPkg/Library/MpInitLib/MpLib.c".
>>
>> Instead, the calls should be made in the DXE instance of the library
>> ("UefiCpuPkg/Library/MpInitLib/DxeMpLib.c"), at the very top of the
>> functions:
>>
>> - MpInitLibStartupAllAPs
>> - MpInitLibStartupThisAP
>> - MpInitLibSwitchBSP
>> - MpInitLibEnableDisableAP
>>
>> Here's why:
>>
>> (a) The symptom does not affect the PEI phase -- by the time the UEFI
>> application is executed, the PEI phase has ended; there's no need to
>> modify the PEI instance of the library.
>>
>> (b) The currently proposed failure exits are too late. For example, in
>> the SwitchBSPWorker() function, by the time we exit, we have called

Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Laszlo Ersek]

On 09/04/20 03:51, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
> 
> AP needs to run from real mode to 32bit mode to LONG mode. Page table
> (pointed by Cr3) and GDT are necessary to set up to correct value when
> CPU execution mode is switched to LONG mode.
> AP uses the same location page table (Cr3) and GDT as what BSP uses.
> But when the page table or GDT is above 4GB, it's impossible for CPU
> to use because GDTR.base and Cr3 are 32bits before switching to LONG
> mode.
> 
> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
> 
> The check is avoided -- assumed successful -- if the new
> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
> is that the 32-bit requirement is always ensured by edk2 itself; the
> requirement is only possibly invalidated by a particular UEFI shell
> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
> that don't intend to be compatible with such UEFI applications need not
> set the PCD to TRUE.
> 
> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
> rejected at once. Reporting an error immediately is more graceful than
> hanging when the APs attempt to switch to long mode.
> 
> Signed-off-by: Eric Dong 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> ---
> V3:
> only add check for the DxeMplib, no need for PeiMpLib.
> 
> V2:
> Change the check point. Just in the different caller to make the logic
> clear. V1 patch add check just before the use of the code. It make the
> logic complicated.
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c   | 97 +++
>  UefiCpuPkg/UefiCpuPkg.dec |  4 +
>  UefiCpuPkg/UefiCpuPkg.uni |  2 +
>  4 files changed, 104 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf 
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 1771575c69..7792df516e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -74,5 +74,6 @@
>gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## 
> CONSUMES
>gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled  ## 
> CONSUMES
>gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase   ## 
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck   ## 
> CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard  ## 
> CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase   ## 
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72dde..332b4447bb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,66 @@ VOID *mReservedApLoopFunc = NULL;
>  UINTNmReservedTopOfApStack;
>  volatile UINT32  mNumberToFinish = 0;
>  
> +/**
> +  Check whether Cr3/GDT/IDT value valid for the APs.
> +
> +  @retval  TRUE  Pass the check.
> +  @retval  FALSE Fail the check.
> +
> +**/
> +BOOLEAN
> +ValidCr3GdtIdtCheck (
> +  VOID
> +  )
> +{
> +  IA32_DESCRIPTOR   Gdtr;
> +  IA32_DESCRIPTOR   Idtr;
> +
> +  if (!PcdGetBool (PcdEnableCpuApCr3GdtIdtCheck)) {
> +return TRUE;
> +  }
> +
> +  //
> +  // AP needs to run from real mode to 32bit mode to LONG mode. Page table
> +  // (pointed by Cr3) and GDT are necessary to set up to correct value when
> +  // CPU execution mode is switched to LONG mode. IDT also necessary if the
> +  // exception happened.
> +  // AP uses the same location page table (Cr3) and GDT/IDT as what BSP uses.
> +  // But when the page table or GDT is above 4GB, it's impossible for CPU
> +  // to use because GDTR.base and Cr3 are 32bits before switching to LONG
> +  // mode.
> +  // Here add check for the Cr3, GDT.Base and range, IDT.Base and range are
> +  // not above 32 bits limitation.
> +  //
> +  if (AsmReadCr3 () >= BASE_4GB) {
> +return FALSE;
> +  }
> +
> +  AsmReadGdtr (&Gdtr);
> +  //
> +  // Here code needs to check both Gdtr.Base and Gdtr.Base + Gdtr.Limit
> +  // below BASE_4GB, but Gdtr.Base + Gdtr.Limit below BASE_4GB also means
> +  // Gdtr.Base below BASE_4GB. so here just add Gdtr.Base + Gdtr.Limit
> +  // check.
> +  //
> +  if (Gdtr.Base + Gdtr.Limit >= BASE_4GB) {
> +return FALSE;
> +  }
> +
> +  AsmReadIdtr (&Idtr);
> +  //
> +  // Here code needs to check both Idtr.Base and Idtr.Base + Idtr.Limit
> +  // below BASE_4GB, but Idtr.Base + Idtr.Limit below BASE_4GB also means
> +  // Idtr.Base below BASE_4GB. so here just add Idtr.Base + Idtr.Limit
> +  // check.
> +  //
> +  if (Idtr.Base + Idtr.Limit >= BASE_4GB) {
> +return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
>  /**
>Enable Debug Agent to support so

Re: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for Cr3/GDT/IDT.

[Laszlo Ersek]

Hello Jiewen,

On 09/04/20 05:48, Yao, Jiewen wrote:
> I provided the feedback in 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2954.
>
> I suggest we justify the requirement at first.

in comment , you
write,

> I don't treat it as a valid test case, to move page table, GDT, IDT in
> UEFI environment.
>
> The CPU context should be managed by CPU driver, and only CPU driver.
> Other driver or application should only change the system state via
> UEFI/PI spec defined API. Otherwise, the system may run into an
> unknown state.
>
> IMHO, I dont think it is a valid case.

and I couldn't agree more with you -- I'm also convinced that what the
particular UEFI application does is invalid.

However, it seems like Eric really needs this change. I don't know his
business case (maybe he's not at liberty to share it), but when he
implied it was really important, I trusted him. So I figured we can
accommodate this use case (even if we don't exactly know all the
details, and even if we know for a fact that what the UEFI application
does is *invalid*, per spec) -- but then the impact, on both
performance, and on code complexity, should be minimal, for platforms
that do not care. (And those platforms have the *right* not to care.)

So I'm going to review this patch now *assuming* that the edk2 community
is OK to accept the use case in the first place. I'm not *encouraging*
the use case or whatever -- the use case is clearly wrong. But Eric
seems to need it: maybe some of the circumstances are out of his
control, and he can't just say "don't run this application", or "change
the application". Perhaps it's a temporary fix in edk2 (Eric mentioned
release time pressure, IIRC). I can fully appreciate that, and I'm OK to
tolerate temporary fixes -- it's just that temporary fixes sometimes
become permanent (increasing technical debt), and for that reason, I'd
prefer this hack to be as clean and non-intrusive as possible.

Thanks,
Laszlo


>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Dong, Eric
>> Sent: Friday, September 4, 2020 9:52 AM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray ; Laszlo Ersek 
>> Subject: [edk2-devel] [PATCH v3] UefiCpuPkg/MpInitLib: Add check for
>> Cr3/GDT/IDT.
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2954
>>
>> AP needs to run from real mode to 32bit mode to LONG mode. Page table
>> (pointed by Cr3) and GDT are necessary to set up to correct value when
>> CPU execution mode is switched to LONG mode.
>> AP uses the same location page table (Cr3) and GDT as what BSP uses.
>> But when the page table or GDT is above 4GB, it's impossible for CPU
>> to use because GDTR.base and Cr3 are 32bits before switching to LONG
>> mode.
>>
>> This patch adds check for the Cr3, GDT and IDT to not above 4G limitation.
>>
>> The check is avoided -- assumed successful -- if the new
>> PcdEnableCpuApCr3GdtIdtCheck is FALSE (which is the default). The reason
>> is that the 32-bit requirement is always ensured by edk2 itself; the
>> requirement is only possibly invalidated by a particular UEFI shell
>> application that manually moves the GDT/IDT/CR3 above 4GB. Platforms
>> that don't intend to be compatible with such UEFI applications need not
>> set the PCD to TRUE.
>>
>> If the PCD is TRUE and the check fails, then the StartupAllAPs(),
>> StartupThisAP(), SwitchBSP() and EnableDisableAP() MP service APIs are
>> rejected at once. Reporting an error immediately is more graceful than
>> hanging when the APs attempt to switch to long mode.
>>
>> Signed-off-by: Eric Dong 
>> Cc: Ray Ni 
>> Cc: Laszlo Ersek 
>> ---
>>
>> V3:
>>
>> only add check for the DxeMplib, no need for PeiMpLib.
>>
>>
>>
>> V2:
>>
>> Change the check point. Just in the different caller to make the logic
>>
>> clear. V1 patch add check just before the use of the code. It make the
>>
>> logic complicated.
>>
>>
>>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c   | 97 +++
>>  UefiCpuPkg/UefiCpuPkg.dec |  4 +
>>  UefiCpuPkg/UefiCpuPkg.uni |  2 +
>>  4 files changed, 104 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> index 1771575c69..7792df516e 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> @@ -74,5 +74,6 @@
>>gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
>> ## CONSUMES
>>
>>gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled  ## 
>> CONSUMES
>>
>>gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase   ##
>> SOMETIMES_CONSUMES
>>
>> +  gUefiCpuPkgTokenSpaceGuid.PcdEnableCpuApCr3GdtIdtCheck   ##
>> CONSUMES
>>
>>gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard  ##
>> CONSUMES
>>
>>gEfiMdeModulePkgTok