Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

2018-04-12 Thread Bi, Dandan 
Thanks Laszlo for the detailed description.
I will communicate will ECC owner to see whether ECC tool can be enhanced for 
these issues firstly. 
Thank you all.

Regards,
Dandan
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Friday, April 13, 2018 1:51 AM
To: Kinney, Michael D <michael.d.kin...@intel.com>; Bi, Dandan 
<dandan...@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.d...@intel.com>
Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for 
"gPatchxxx"

On 04/12/18 18:47, Kinney, Michael D wrote:
> Laszlo,
> 
> I think I would rather see the ECC tool fixed.

I didn't dare suggest that, but I agree it's a superior solution. When I tried 
ECC last time, I was surprised how powerful it was, so if it recognized even 
this case, that would certainly fit its quality :)

Thanks!
Laszlo

> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Thursday, April 12, 2018 2:34 AM
>> To: Bi, Dandan <dandan...@intel.com>; edk2- de...@lists.01.org
>> Cc: Dong, Eric <eric.d...@intel.com>
>> Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm:
>> Add "extern" keyword for "gPatchxxx"
>>
>> Hello Dandan,
>>
>> On 04/12/18 10:50, Dandan Bi wrote:
>>> Background description:
>>> In SmmProfileInternal.h, ECC check tool report an
>> issue at line 103.
>>> Detailed ECC Error info:Variable definition appears
>> in header file.
>>> Include files should contain only public or only
>> private data and
>>> cannot contain code or define data variables
>>>
>>> ECC report similar issues in PiSmmCpuDxeSmm.h.
>>>
>>> Then we review all the new introduced "gPatchxxx",
>> since they have
>>> been defined in the nasm file, we can add "extern"
>> keyword for them
>>> in the C source or header files.
>>>
>>> Cc: Eric Dong <eric.d...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>> Signed-off-by: Dandan Bi <dandan...@intel.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 8
>> 
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2
>> +-
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 6
>> +++---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c  | 4
>> ++--
>>>  4 files changed, 10 insertions(+), 10 deletions(-)
>>
>> This is a bug (a false positive) in the ECC tool. The following
>> declaration:
>>
>>> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
>>
>> does not declare an *object* (a variable). Instead, it declares a
>> *function* (and not a pointer to a function!), because (from
>> "MdePkg/Include/Library/BaseLib.h"):
>>
>>> ///
>>> /// Type definition for representing labels in NASM
>> source code that allow for
>>> /// the patching of immediate operands of IA32 and
>> X64 instructions.
>>> ///
>>> /// While the type is technically defined as a
>> function type (note: not a
>>> /// pointer-to-function type), such labels in NASM
>> source code never stand for
>>> /// actual functions, and identifiers declared with
>> this function type should
>>> /// never be called. This is also why the EFIAPI
>> calling convention specifier
>>> /// is missing from the typedef, and why the typedef
>> does not follow the usual
>>> /// edk2 coding style for function (or pointer-to-
>> function) typedefs. The VOID
>>> /// return type and the VOID argument list are merely
>> artifacts.
>>> ///
>>> typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID);
>>
>> That is, when you see
>>
>>> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
>>
>> That is identical to the following function
>> declaration:
>>
>>> VOID gPatchSmmCr0 (VOID);
>>
>> Now, the ISO C99 standard says:
>>
>>> 6.2.2 Linkages of identifiers
>>>
>>> [...]
>>>
>>>   5 If the declaration of an identifier for a
>> function has no
>>> storage-class specifier, its linkage is
>> determined exactly as if
>>> it were declared with the storage-class specifier
>> /extern/. [...]
>>
>> Thus, the report from ECC is a false positive.
>>
>> I don't mind the patch (the changes don't make any difference at the 
>> C-language level, see the spec above); however, the commit message 
>> should be 100% clear that the patch works around a limitation with 
>> the ECC tool.
>>
>> Can you please submit v2 with an updated commit message?
>>
>> Thanks!
>> Laszlo
>> ___
>> 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] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

2018-04-12 Thread Laszlo Ersek 
On 04/12/18 18:47, Kinney, Michael D wrote:
> Laszlo,
> 
> I think I would rather see the ECC tool fixed.

I didn't dare suggest that, but I agree it's a superior solution. When I
tried ECC last time, I was surprised how powerful it was, so if it
recognized even this case, that would certainly fit its quality :)

Thanks!
Laszlo

> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Thursday, April 12, 2018 2:34 AM
>> To: Bi, Dandan <dandan...@intel.com>; edk2-
>> de...@lists.01.org
>> Cc: Dong, Eric <eric.d...@intel.com>
>> Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm:
>> Add "extern" keyword for "gPatchxxx"
>>
>> Hello Dandan,
>>
>> On 04/12/18 10:50, Dandan Bi wrote:
>>> Background description:
>>> In SmmProfileInternal.h, ECC check tool report an
>> issue at line 103.
>>> Detailed ECC Error info:Variable definition appears
>> in header file.
>>> Include files should contain only public or only
>> private data and
>>> cannot contain code or define data variables
>>>
>>> ECC report similar issues in PiSmmCpuDxeSmm.h.
>>>
>>> Then we review all the new introduced "gPatchxxx",
>> since they have
>>> been defined in the nasm file, we can add "extern"
>> keyword for them
>>> in the C source or header files.
>>>
>>> Cc: Eric Dong <eric.d...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>> Signed-off-by: Dandan Bi <dandan...@intel.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 8
>> 
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2
>> +-
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 6
>> +++---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c  | 4
>> ++--
>>>  4 files changed, 10 insertions(+), 10 deletions(-)
>>
>> This is a bug (a false positive) in the ECC tool. The
>> following
>> declaration:
>>
>>> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
>>
>> does not declare an *object* (a variable). Instead, it
>> declares a
>> *function* (and not a pointer to a function!), because
>> (from
>> "MdePkg/Include/Library/BaseLib.h"):
>>
>>> ///
>>> /// Type definition for representing labels in NASM
>> source code that allow for
>>> /// the patching of immediate operands of IA32 and
>> X64 instructions.
>>> ///
>>> /// While the type is technically defined as a
>> function type (note: not a
>>> /// pointer-to-function type), such labels in NASM
>> source code never stand for
>>> /// actual functions, and identifiers declared with
>> this function type should
>>> /// never be called. This is also why the EFIAPI
>> calling convention specifier
>>> /// is missing from the typedef, and why the typedef
>> does not follow the usual
>>> /// edk2 coding style for function (or pointer-to-
>> function) typedefs. The VOID
>>> /// return type and the VOID argument list are merely
>> artifacts.
>>> ///
>>> typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID);
>>
>> That is, when you see
>>
>>> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
>>
>> That is identical to the following function
>> declaration:
>>
>>> VOID gPatchSmmCr0 (VOID);
>>
>> Now, the ISO C99 standard says:
>>
>>> 6.2.2 Linkages of identifiers
>>>
>>> [...]
>>>
>>>   5 If the declaration of an identifier for a
>> function has no
>>> storage-class specifier, its linkage is
>> determined exactly as if
>>> it were declared with the storage-class specifier
>> /extern/. [...]
>>
>> Thus, the report from ECC is a false positive.
>>
>> I don't mind the patch (the changes don't make any
>> difference at the
>> C-language level, see the spec above); however, the
>> commit message
>> should be 100% clear that the patch works around a
>> limitation with the
>> ECC tool.
>>
>> Can you please submit v2 with an updated commit
>> message?
>>
>> Thanks!
>> Laszlo
>> ___
>> 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] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

2018-04-12 Thread Kinney, Michael D 
Laszlo,

I think I would rather see the ECC tool fixed.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, April 12, 2018 2:34 AM
> To: Bi, Dandan <dandan...@intel.com>; edk2-
> de...@lists.01.org
> Cc: Dong, Eric <eric.d...@intel.com>
> Subject: Re: [edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm:
> Add "extern" keyword for "gPatchxxx"
> 
> Hello Dandan,
> 
> On 04/12/18 10:50, Dandan Bi wrote:
> > Background description:
> > In SmmProfileInternal.h, ECC check tool report an
> issue at line 103.
> > Detailed ECC Error info:Variable definition appears
> in header file.
> > Include files should contain only public or only
> private data and
> > cannot contain code or define data variables
> >
> > ECC report similar issues in PiSmmCpuDxeSmm.h.
> >
> > Then we review all the new introduced "gPatchxxx",
> since they have
> > been defined in the nasm file, we can add "extern"
> keyword for them
> > in the C source or header files.
> >
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Dandan Bi <dandan...@intel.com>
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 8
> 
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2
> +-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 6
> +++---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c  | 4
> ++--
> >  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> This is a bug (a false positive) in the ECC tool. The
> following
> declaration:
> 
> > X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
> 
> does not declare an *object* (a variable). Instead, it
> declares a
> *function* (and not a pointer to a function!), because
> (from
> "MdePkg/Include/Library/BaseLib.h"):
> 
> > ///
> > /// Type definition for representing labels in NASM
> source code that allow for
> > /// the patching of immediate operands of IA32 and
> X64 instructions.
> > ///
> > /// While the type is technically defined as a
> function type (note: not a
> > /// pointer-to-function type), such labels in NASM
> source code never stand for
> > /// actual functions, and identifiers declared with
> this function type should
> > /// never be called. This is also why the EFIAPI
> calling convention specifier
> > /// is missing from the typedef, and why the typedef
> does not follow the usual
> > /// edk2 coding style for function (or pointer-to-
> function) typedefs. The VOID
> > /// return type and the VOID argument list are merely
> artifacts.
> > ///
> > typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID);
> 
> That is, when you see
> 
> > X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
> 
> That is identical to the following function
> declaration:
> 
> > VOID gPatchSmmCr0 (VOID);
> 
> Now, the ISO C99 standard says:
> 
> > 6.2.2 Linkages of identifiers
> >
> > [...]
> >
> >   5 If the declaration of an identifier for a
> function has no
> > storage-class specifier, its linkage is
> determined exactly as if
> > it were declared with the storage-class specifier
> /extern/. [...]
> 
> Thus, the report from ECC is a false positive.
> 
> I don't mind the patch (the changes don't make any
> difference at the
> C-language level, see the spec above); however, the
> commit message
> should be 100% clear that the patch works around a
> limitation with the
> ECC tool.
> 
> Can you please submit v2 with an updated commit
> message?
> 
> Thanks!
> Laszlo
> ___
> 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] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

2018-04-12 Thread Laszlo Ersek 
Hello Dandan,

On 04/12/18 10:50, Dandan Bi wrote:
> Background description:
> In SmmProfileInternal.h, ECC check tool report an issue at line 103.
> Detailed ECC Error info:Variable definition appears in header file.
> Include files should contain only public or only private data and
> cannot contain code or define data variables
>
> ECC report similar issues in PiSmmCpuDxeSmm.h.
>
> Then we review all the new introduced "gPatchxxx", since they have
> been defined in the nasm file, we can add "extern" keyword for them
> in the C source or header files.
>
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 8 
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 6 +++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c  | 4 ++--
>  4 files changed, 10 insertions(+), 10 deletions(-)

This is a bug (a false positive) in the ECC tool. The following
declaration:

> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;

does not declare an *object* (a variable). Instead, it declares a
*function* (and not a pointer to a function!), because (from
"MdePkg/Include/Library/BaseLib.h"):

> ///
> /// Type definition for representing labels in NASM source code that allow for
> /// the patching of immediate operands of IA32 and X64 instructions.
> ///
> /// While the type is technically defined as a function type (note: not a
> /// pointer-to-function type), such labels in NASM source code never stand for
> /// actual functions, and identifiers declared with this function type should
> /// never be called. This is also why the EFIAPI calling convention specifier
> /// is missing from the typedef, and why the typedef does not follow the usual
> /// edk2 coding style for function (or pointer-to-function) typedefs. The VOID
> /// return type and the VOID argument list are merely artifacts.
> ///
> typedef VOID (X86_ASSEMBLY_PATCH_LABEL) (VOID);

That is, when you see

> X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;

That is identical to the following function declaration:

> VOID gPatchSmmCr0 (VOID);

Now, the ISO C99 standard says:

> 6.2.2 Linkages of identifiers
>
> [...]
>
>   5 If the declaration of an identifier for a function has no
> storage-class specifier, its linkage is determined exactly as if
> it were declared with the storage-class specifier /extern/. [...]

Thus, the report from ECC is a false positive.

I don't mind the patch (the changes don't make any difference at the
C-language level, see the spec above); however, the commit message
should be 100% clear that the patch works around a limitation with the
ECC tool.

Can you please submit v2 with an updated commit message?

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

[edk2] [patch] UefiCpuPkg/PiSmmCpuDxeSmm: Add "extern" keyword for "gPatchxxx"

2018-04-12 Thread Dandan Bi 
Background description:
In SmmProfileInternal.h, ECC check tool report an issue at line 103.
Detailed ECC Error info:Variable definition appears in header file.
Include files should contain only public or only private data and
cannot contain code or define data variables

ECC report similar issues in PiSmmCpuDxeSmm.h.

Then we review all the new introduced "gPatchxxx", since they have
been defined in the nasm file, we can add "extern" keyword for them
in the C source or header files.

Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 8 
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 6 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c  | 4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1d016594e02..bcfbab51144 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -295,16 +295,16 @@ WriteSaveStateRegister (
   IN CONST VOID   *Buffer
   );
 
 extern CONST UINT8  gcSmmInitTemplate[];
 extern CONST UINT16 gcSmmInitSize;
-X86_ASSEMBLY_PATCH_LABELgPatchSmmCr0;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0;
 extern UINT32   mSmmCr0;
-X86_ASSEMBLY_PATCH_LABELgPatchSmmCr3;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr3;
 extern UINT32   mSmmCr4;
-X86_ASSEMBLY_PATCH_LABELgPatchSmmCr4;
-X86_ASSEMBLY_PATCH_LABELgPatchSmmInitStack;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr4;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmmInitStack;
 
 /**
   Semaphore operation for all processor relocate SMMBase.
 **/
 VOID
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
index 1613e9cd5cb..662814412b2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
@@ -98,11 +98,11 @@ typedef struct {
 } SMM_PROFILE_ENTRY;
 
 extern SMM_S3_RESUME_STATE   *mSmmS3ResumeState;
 extern UINTN gSmiExceptionHandlers[];
 extern BOOLEAN   mXdSupported;
-X86_ASSEMBLY_PATCH_LABEL gPatchXdSupported;
+extern X86_ASSEMBLY_PATCH_LABEL  gPatchXdSupported;
 extern UINTN *mPFEntryCount;
 extern UINT64(*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];
 extern UINT64*(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];
 
 //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
index 5c2eb9ab6a1..dddc2f12325 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c
@@ -103,13 +103,13 @@ typedef struct {
 } CPU_SMM_SAVE_STATE_IO_WIDTH;
 
 ///
 /// Variables from SMI Handler
 ///
-X86_ASSEMBLY_PATCH_LABEL gPatchSmbase;
-X86_ASSEMBLY_PATCH_LABEL gPatchSmiStack;
-X86_ASSEMBLY_PATCH_LABEL gPatchSmiCr3;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmbase;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmiStack;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmiCr3;
 extern volatile UINT8gcSmiHandlerTemplate[];
 extern CONST UINT16  gcSmiHandlerSize;
 
 //
 // Variables used by SMI Handler
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c
index 87f595ddb8c..dfee5416161 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c
@@ -13,12 +13,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 **/
 
 #include "PiSmmCpuDxeSmm.h"
 
-X86_ASSEMBLY_PATCH_LABEL gPatchSmmRelocationOriginalAddressPtr32;
-X86_ASSEMBLY_PATCH_LABEL gPatchRebasedFlagAddr32;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchSmmRelocationOriginalAddressPtr32;
+extern X86_ASSEMBLY_PATCH_LABEL gPatchRebasedFlagAddr32;
 
 UINTN mSmmRelocationOriginalAddress;
 volatile BOOLEAN  *mRebasedFlag;
 
 /**
-- 
2.14.3.windows.1

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