Re: [edk2] [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Gao, Liming]

Yes. The change is necessary. 

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, December 5, 2018 3:42 PM
> To: Gao, Liming 
> Cc: Laszlo Ersek ; edk2-devel@lists.01.org; Zhu, Yonghong 
> ; Feng, Bob C
> ; Carsey, Jaben 
> Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default 
> device path max size
> 
> On Wed, 5 Dec 2018 at 01:04, Gao, Liming  wrote:
> >
> > Laszlo:
> >   I agree with you. MAX_UINT32 is more comfortable.
> >
> 
> Liming,
> 
> No definitions for MAX_UINT32 exist currently in BaseTools, so I will
> have to add the following:
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h
> b/BaseTools/Source/C/Common/CommonLib.h
> index b1c6c00a3478..1c40180329c4 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
>  #define MAX_LONG_FILE_PATH 500
> 
>  #define MAX_UINT64 ((UINT64)0xULL)
> +#define MAX_UINT32 ((UINT32)0xULL)
>  #define MAX_UINT16  ((UINT16)0x)
>  #define MAX_UINT8   ((UINT8)0xFF)
>  #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))
> 
> Does your Reviewed-by cover that as well?
> 
> 
> 
> 
> > >-Original Message-
> > >From: Laszlo Ersek [mailto:ler...@redhat.com]
> > >Sent: Monday, December 03, 2018 9:06 PM
> > >To: Ard Biesheuvel ; edk2-devel@lists.01.org
> > >Cc: Zhu, Yonghong ; Gao, Liming
> > >; Feng, Bob C ; Carsey, Jaben
> > >
> > >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
> > >default device path max size
> > >
> > >On 11/30/18 23:45, Ard Biesheuvel wrote:
> > >> Replace the default size limit of IsDevicePathValid() with a value
> > >> that does not depend on the native word size of the build host.
> > >>
> > >> 64 KB seems sufficient as the upper bound of a device path handled
> > >> by UEFI.
> > >>
> > >> Contributed-under: TianoCore Contribution Agreement 1.1
> > >> Signed-off-by: Ard Biesheuvel 
> > >> Reviewed-by: Jaben Carsey 
> > >> ---
> > >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> index d4ec2742b7c8..ba7f83e53070 100644
> > >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> @@ -62,7 +62,7 @@ IsDevicePathValid (
> > >>ASSERT (DevicePath != NULL);
> > >>
> > >>if (MaxSize == 0) {
> > >> -MaxSize = MAX_UINTN;
> > >> +MaxSize = MAX_UINT16;
> > >>   }
> > >>
> > >>//
> > >> @@ -78,7 +78,7 @@ IsDevicePathValid (
> > >>return FALSE;
> > >>  }
> > >>
> > >> -if (NodeLength > MAX_UINTN - Size) {
> > >> +if (NodeLength > MAX_UINT16 - Size) {
> > >>return FALSE;
> > >>  }
> > >>  Size += NodeLength;
> > >>
> > >
> > >I'm somewhat undecided about this patch.
> > >
> > >(1) IsDevicePathValid() also exists in:
> > >
> > >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
> > >
> > >Both have:
> > >
> > >  if (MaxSize == 0) {
> > >MaxSize = MAX_UINTN;
> > >  }
> > >
> > >Relative to those, this change departs quite strongly.
> > >
> > >
> > >(2) In addition, a single device path node may extend up to 64KB. That
> > >would be pathologic, yes, but the option is there.
> > >
> > >
> > >... Of course, we are discussing theoretical limits. Still I'd feel more
> > >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
> > >cost us anything (in development effort), it would be a no-op on 32-bit
> > >build hosts, it would be a theoretical-only change on 64-bit build
> > >hosts, and it would leave us with a larger "safety margin".
> > >
> > >I won't insist, but I thought I should raise this. (Sorry if this has
> > >been discussed under v1 already.) If you agree, no need to repost (from
> > >my side anyway) just for this.
> > >
> > >With or without the update:
> > >
> > >Reviewed-by: Laszlo Ersek 
> > >
> > >Thanks
> > >Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Ard Biesheuvel]

On Wed, 5 Dec 2018 at 01:04, Gao, Liming  wrote:
>
> Laszlo:
>   I agree with you. MAX_UINT32 is more comfortable.
>

Liming,

No definitions for MAX_UINT32 exist currently in BaseTools, so I will
have to add the following:

diff --git a/BaseTools/Source/C/Common/CommonLib.h
b/BaseTools/Source/C/Common/CommonLib.h
index b1c6c00a3478..1c40180329c4 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
EITHER EXPRESS OR IMPLIED.
 #define MAX_LONG_FILE_PATH 500

 #define MAX_UINT64 ((UINT64)0xULL)
+#define MAX_UINT32 ((UINT32)0xULL)
 #define MAX_UINT16  ((UINT16)0x)
 #define MAX_UINT8   ((UINT8)0xFF)
 #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))

Does your Reviewed-by cover that as well?




> >-Original Message-
> >From: Laszlo Ersek [mailto:ler...@redhat.com]
> >Sent: Monday, December 03, 2018 9:06 PM
> >To: Ard Biesheuvel ; edk2-devel@lists.01.org
> >Cc: Zhu, Yonghong ; Gao, Liming
> >; Feng, Bob C ; Carsey, Jaben
> >
> >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
> >default device path max size
> >
> >On 11/30/18 23:45, Ard Biesheuvel wrote:
> >> Replace the default size limit of IsDevicePathValid() with a value
> >> that does not depend on the native word size of the build host.
> >>
> >> 64 KB seems sufficient as the upper bound of a device path handled
> >> by UEFI.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel 
> >> Reviewed-by: Jaben Carsey 
> >> ---
> >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> index d4ec2742b7c8..ba7f83e53070 100644
> >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> @@ -62,7 +62,7 @@ IsDevicePathValid (
> >>ASSERT (DevicePath != NULL);
> >>
> >>if (MaxSize == 0) {
> >> -MaxSize = MAX_UINTN;
> >> +MaxSize = MAX_UINT16;
> >>   }
> >>
> >>//
> >> @@ -78,7 +78,7 @@ IsDevicePathValid (
> >>return FALSE;
> >>  }
> >>
> >> -if (NodeLength > MAX_UINTN - Size) {
> >> +if (NodeLength > MAX_UINT16 - Size) {
> >>return FALSE;
> >>  }
> >>  Size += NodeLength;
> >>
> >
> >I'm somewhat undecided about this patch.
> >
> >(1) IsDevicePathValid() also exists in:
> >
> >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
> >
> >Both have:
> >
> >  if (MaxSize == 0) {
> >MaxSize = MAX_UINTN;
> >  }
> >
> >Relative to those, this change departs quite strongly.
> >
> >
> >(2) In addition, a single device path node may extend up to 64KB. That
> >would be pathologic, yes, but the option is there.
> >
> >
> >... Of course, we are discussing theoretical limits. Still I'd feel more
> >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
> >cost us anything (in development effort), it would be a no-op on 32-bit
> >build hosts, it would be a theoretical-only change on 64-bit build
> >hosts, and it would leave us with a larger "safety margin".
> >
> >I won't insist, but I thought I should raise this. (Sorry if this has
> >been discussed under v1 already.) If you agree, no need to repost (from
> >my side anyway) just for this.
> >
> >With or without the update:
> >
> >Reviewed-by: Laszlo Ersek 
> >
> >Thanks
> >Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v1 edk2-platforms 1/8] Platform/ARM/SgiPkg: Restructure virtio device registration

[Vijayenthiran Subramaniam]

Hi Ard,

The virtio block device and virtio network device are available in software
model only. As of now, it exposes only one instance of each device.

On Tue, Dec 4, 2018 at 8:16 PM Ard Biesheuvel 
wrote:

> On Tue, 4 Dec 2018 at 10:12, Vijayenthiran Subramaniam
>  wrote:
> >
> > From: Daniil Egranov 
> >
> > SGI platforms support multiple virtio devices. So the existing code, that
> > supports registration of only the virtio disk, is restructured to
> > accommodate the registration of additional virtio devices.
> >
> > In addition to this, PCDs to represent the virtio controller base and
> > address space size are introduced.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Daniil Egranov 
> > ---
> >  Platform/ARM/SgiPkg/SgiPlatform.dec
>   |  8 ++-
> >  Platform/ARM/SgiPkg/SgiPlatform.dsc
>   |  7 +-
> >  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
>   |  8 ++-
> >  Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
>  | 21 ++
> >  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
>   | 14 +---
> >  Platform/ARM/SgiPkg/Drivers/PlatformDxe/{VirtioBlockIo.c =>
> VirtioDevices.c} | 67 
> >  6 files changed, 81 insertions(+), 44 deletions(-)
> >
>
> Can these platforms only ever expose a single block device and a
> single network device?
>
> > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec
> b/Platform/ARM/SgiPkg/SgiPlatform.dec
> > index f6e0ba1e927a..ed29a4d5d91f 100644
> > --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> > @@ -37,12 +37,16 @@ [Guids.common]
> >gSgiClarkHeliosAcpiTablesFileGuid = { 0x2af40815, 0xa84e, 0x4de9, {
> 0x8c, 0x38, 0x91, 0x40, 0xb3, 0x54, 0x40, 0x73 } }
> >
> >  [PcdsFeatureFlag.common]
> > -  # Set this PCD to TRUE to enable virtio support.
> > -  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x0001
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|FALSE|BOOLEAN|0x0001
> >
> >  [PcdsFixedAtBuild]
> >gArmSgiTokenSpaceGuid.PcdDramBlock2Base|0|UINT64|0x0002
> >gArmSgiTokenSpaceGuid.PcdDramBlock2Size|0|UINT64|0x0003
> >
> > +  # Virtio Block device
> > +
> gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x|UINT32|0x0004
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x|UINT32|0x0005
> > +
> gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|0x|UINT32|0x0006
> > +
> >  [Ppis]
> >gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8,
> 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> > index b3f76d2d9720..ada72be72f8a 100644
> > --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> > @@ -98,7 +98,7 @@ [LibraryClasses.common.UEFI_DRIVER,
> LibraryClasses.common.UEFI_APPLICATION, Libr
> >
> 
> >
> >  [PcdsFeatureFlag.common]
> > -  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|TRUE
> >
> >  [PcdsFixedAtBuild.common]
> >gArmTokenSpaceGuid.PcdVFPEnabled|1
> > @@ -180,6 +180,11 @@ [PcdsFixedAtBuild.common]
> ># Ethernet
> >gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
> >
> > +  # Virtio Disk
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x1c13
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x1
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|202
> > +
> >
> 
> >  #
> >  # Components Section - list of all EDK II Modules needed by this
> Platform
> > diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > index d903ed8d3375..f920f6ecafb8 100644
> > --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > @@ -20,7 +20,7 @@ [Defines]
> >
> >  [Sources.common]
> >PlatformDxe.c
> > -  VirtioBlockIo.c
> > +  VirtioDevices.c
> >
> >  [Packages]
> >EmbeddedPkg/EmbeddedPkg.dec
> > @@ -41,7 +41,11 @@ [Guids]
> >gSgiClarkHeliosAcpiTablesFileGuid
> >
> >  [FeaturePcd]
> > -  gArmSgiTokenSpaceGuid.PcdVirtioSupported
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported
> > +
> > +[FixedPcd]
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
> > +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
> >
> >  [Depex]
> >TRUE
> > diff --git a/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
> b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
> > new file mode 100644
> > index ..80d3e3ae4f91
> > --- /dev/null
> > +++ b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
> > @@ -0,0 +1,21 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> > +*
> > +*  This program and the 

[edk2] [edk2-test][v3 Patch 3/3] uefi-sct/SctPkg:Add VerifySignature() Conf Test

[Eric Jin]

Enable the BBTestVerifySignatureConformanceTest()
with checkpoints under 7 cases below, it should
not be success.
a)Signature is NULL/SignatureSize is 0/InHash is NULL
/InHashSize is 0/AllowedDb is NULL
b)Modify the Data in P7TestSignature to make it invalid
c)Modify the Data in InHash to make it invalid
d)Input the invalid Hashsize
e)The SignedData buffer was correctly formatted but
signer was in RevokedDb
f)The SignedData buffer was correctly formatted but
signer was not in AllowedDb
g)Matching content hash found in the RevokedDb

Cc: Supreeth Venkatesh 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Jin 
---
 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c   |   5 +-
 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h   |  16 ++
 .../BlackBoxTest/Pkcs7BBTestConformance.c  | 301 +
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.c |   9 +
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.h |   8 +
 5 files changed, 338 insertions(+), 1 deletion(-)

diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
index 4d433c3..8f6546a 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
@@ -36,7 +36,10 @@ EFI_GUID gPkcs7BBTestConformanceAssertionGuid007 = 
EFI_TEST_PKCS7BBTESTCONFORMAN
 EFI_GUID gPkcs7BBTestConformanceAssertionGuid008 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_008_GUID;
 EFI_GUID gPkcs7BBTestConformanceAssertionGuid009 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_009_GUID;
 EFI_GUID gPkcs7BBTestConformanceAssertionGuid010 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_010_GUID;
-
+EFI_GUID gPkcs7BBTestConformanceAssertionGuid011 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_011_GUID;
+EFI_GUID gPkcs7BBTestConformanceAssertionGuid012 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_012_GUID;
+EFI_GUID gPkcs7BBTestConformanceAssertionGuid013 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_013_GUID;
+EFI_GUID gPkcs7BBTestConformanceAssertionGuid014 = 
EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_014_GUID;
 
 EFI_GUID gPkcs7BBTestFunctionAssertionGuid001 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_001_GUID;
 EFI_GUID gPkcs7BBTestFunctionAssertionGuid002 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_002_GUID;
diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
index 94d2568..1827207 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
@@ -65,6 +65,22 @@ extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid009;
 { 0xb136e016, 0x4f80, 0x44bd, {0xba, 0xb0, 0x1c, 0x34, 0x8a, 0x2d, 0xa1, 0x8a 
}}
 extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid010;
 
+#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_011_GUID \
+{ 0xa58f3626, 0xf16e, 0x4cd5, { 0xba, 0x12, 0x7a, 0x9d, 0xd6, 0x9a, 0x7a, 0x71 
}}
+extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid011;
+
+#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_012_GUID \
+{ 0xbe4a0bf2, 0x2d46, 0x4d96, { 0xa6, 0x11, 0x21, 0x99, 0xa5, 0x5f, 0xa4, 0xee 
}}
+extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid012;
+
+#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_013_GUID \
+{ 0xc0536a27, 0x304e, 0x497a, { 0xa5, 0xe3, 0x94, 0x11, 0x38, 0x53, 0x6e, 0x40 
}}
+extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid013;
+
+#define EFI_TEST_PKCS7BBTESTCONFORMANCE_ASSERTION_014_GUID \
+{ 0x8c5aa0e8, 0x17ff, 0x49cd, { 0x8f, 0xec, 0x37, 0xc3, 0xfd, 0x5f, 0x77, 0x0 
}}
+extern EFI_GUID gPkcs7BBTestConformanceAssertionGuid014;
+
 
 #define EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_001_GUID \
 { 0x5c0eec50, 0xa6ea, 0x413c, {0x8a, 0x46, 0x4a, 0xd1, 0x4a, 0x77, 0x76, 0xf1 
}}
diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestConformance.c
 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestConformance.c
index 1dc9921..ce7d5bb 100644
--- 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestConformance.c
+++ 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestConformance.c
@@ -502,3 +502,304 @@ BBTestVerifyBufferConformanceTest (
 
   return EFI_SUCCESS;
 }
+
+EFI_STATUS
+BBTestVerifySignatureConformanceTest (
+  IN EFI_BB_TEST_PROTOCOL*This,
+  IN VOID*ClientInterface,
+  IN EFI_TEST_LEVEL  TestLevel,
+  IN EFI_HANDLE  SupportHandle
+  )
+{
+  EFI_STANDARD_TEST_LIBRARY_PROTOCOL*StandardLib;
+  EFI_STATUSStatus;
+  EFI_PKCS7_VERIFY_PROTOCOL *Pkcs7Verify;
+  EFI_TEST_ASSERTIONAssertionType;
+
+  Pkcs7Verify = (EFI_PKCS7_VERIFY_PROTOCOL*)ClientInterface;
+  

[edk2] [edk2-test][v3 Patch 2/3] uefi-sct/SctPkg:Add VerifySignature() Func Test

[Eric Jin]

Enable the BBTestVerifySignatureFunctionTest()
with 2 checkpoints below, it should be success.
The Certificate/Hash/Digest/signedData are updated
correspondingly.
a)Signed hash was verified against caller-provided
hash of content, the signer's certificate was not
found in RevokedDb, and was found in AllowedDb.
b)Signer is found in both AllowedDb and RevokedDb,
the signing was allowed by reference to TimeStampDb,
and no hash matching content hash was found in RevokedDb.

Cc: Supreeth Venkatesh 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Jin 
---
 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c   |2 +
 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h   |8 +
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c | 1466 +---
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestFunction.c |   86 ++
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.c |   57 +-
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.h |   25 +-
 6 files changed, 1099 insertions(+), 545 deletions(-)

diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
index 142f6d4..4d433c3 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c
@@ -42,3 +42,5 @@ EFI_GUID gPkcs7BBTestFunctionAssertionGuid001 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASS
 EFI_GUID gPkcs7BBTestFunctionAssertionGuid002 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_002_GUID;
 EFI_GUID gPkcs7BBTestFunctionAssertionGuid003 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_003_GUID;
 EFI_GUID gPkcs7BBTestFunctionAssertionGuid004 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_004_GUID;
+EFI_GUID gPkcs7BBTestFunctionAssertionGuid005 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_005_GUID;
+EFI_GUID gPkcs7BBTestFunctionAssertionGuid006 = 
EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_006_GUID;
diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
index ce980c9..94d2568 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h
@@ -81,3 +81,11 @@ extern EFI_GUID gPkcs7BBTestFunctionAssertionGuid003;
 #define EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_004_GUID \
 { 0x912e23ef, 0x299c, 0x41ab, {0xa0, 0xf5, 0xfc, 0xbc, 0xf6, 0xfd, 0xd3, 0x32 
}}
 extern EFI_GUID gPkcs7BBTestFunctionAssertionGuid004;
+
+#define EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_005_GUID \
+{ 0x93740b06, 0xa186, 0x47ff, { 0xba, 0xc3, 0xdd, 0xa8, 0xcb, 0x7b, 0x18, 0x5e 
}}
+extern EFI_GUID gPkcs7BBTestFunctionAssertionGuid005;
+
+#define EFI_TEST_PKCS7BBTESTFUNCTION_ASSERTION_006_GUID \
+{ 0x37253616, 0xca42, 0x4082, { 0x90, 0xda, 0xdb, 0x69, 0x98, 0x22, 0xa0, 0xe6 
}}
+extern EFI_GUID gPkcs7BBTestFunctionAssertionGuid006;
diff --git 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c
 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c
index 0511e00..9b66938 100644
--- 
a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c
+++ 
b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c
@@ -25,541 +25,979 @@ Abstract:
 --*/
 
 //
+// Test Root Certificate ("TestRoot.cer")
+//
+GLOBAL_REMOVE_IF_UNREFERENCED UINT8 TestRootCert[781] = {
+  0x30, 0x82, 0x03, 0x09, 0x30, 0x82, 0x01, 0xF1, 0xA0, 0x03, 0x02, 0x01,
+  0x02, 0x02, 0x10, 0xDE, 0x9F, 0x42, 0x91, 0x68, 0x16, 0xEA, 0x97, 0x4D,
+  0xA1, 0x8A, 0x32, 0x25, 0xD6, 0xEE, 0x8D, 0x30, 0x0D, 0x06, 0x09, 0x2A,
+  0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x01, 0x0B, 0x05, 0x00, 0x30, 0x13,
+  0x31, 0x11, 0x30, 0x0F, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x08, 0x54,
+  0x65, 0x73, 0x74, 0x52, 0x6F, 0x6F, 0x74, 0x30, 0x1E, 0x17, 0x0D, 0x31,
+  0x38, 0x30, 0x31, 0x32, 0x35, 0x30, 0x32, 0x30, 0x35, 0x35, 0x30, 0x5A,
+  0x17, 0x0D, 0x33, 0x39, 0x31, 0x32, 0x33, 0x31, 0x32, 0x33, 0x35, 0x39,
+  0x35, 0x39, 0x5A, 0x30, 0x13, 0x31, 0x11, 0x30, 0x0F, 0x06, 0x03, 0x55,
+  0x04, 0x03, 0x13, 0x08, 0x54, 0x65, 0x73, 0x74, 0x52, 0x6F, 0x6F, 0x74,
+  0x30, 0x82, 0x01, 0x22, 0x30, 0x0D, 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86,
+  0xF7, 0x0D, 0x01, 0x01, 0x01, 0x05, 0x00, 0x03, 0x82, 0x01, 0x0F, 0x00,
+  0x30, 0x82, 0x01, 0x0A, 0x02, 0x82, 0x01, 0x01, 0x00, 0xA5, 0x97, 0x23,
+  0x48, 0xBE, 0xCA, 0xC8, 0xE0, 0x88, 0xC6, 0xA2, 0xAF, 0x78, 0x60, 0x94,
+  0x48, 0x3E, 0x82, 0xE7, 0xD5, 0x62, 0x01, 0x73, 0x00, 0xEA, 0x42, 0x7A,
+  0x32, 0x0A, 0xD7, 0x3F, 0x4D, 0x0B, 0x71, 0x6D, 0xD3, 0x50, 0x5E, 0x26,
+  0x20, 0xE8, 0xCC, 0xB6, 0x0A, 0xAF, 0xD9, 0x07, 0x22, 0x17, 0x45, 0xD8,
+  0x91, 0x75, 0x75, 0x52, 0xD8, 0x8C, 0xAB, 0x63, 0x0A, 0xF0, 0x23, 0x14,
+  0x34, 0x92, 0x3F, 0xE0, 0x05, 0x24, 0x28, 0xED, 0x74, 0x8E, 0x4D, 0x3E,
+  

[edk2] [edk2-test][v3 Patch 0/3] Add VerifySignature() Test in uefi-sct

[Eric Jin]

Patch#1 is to remove un code and correct code style
Patch#2 is to add VerifySignature() Func Test in PKCS7Verify Protocol
Patch#3 is to add VerifySignature() Conf Test in PKCS7Verify Protocol

Eric Jin (3):
  uefi-sct/SctPkg:Format code style in PKCS7Verify
  uefi-sct/SctPkg:Add VerifySignature() Func Test
  uefi-sct/SctPkg:Add VerifySignature() Conf Test

 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c   |   93 +-
 .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h   |  192 +--
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTest.inf   |  126 +-
 .../BlackBoxTest/Pkcs7BBTestConformance.c  | 1305 +---
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c | 1568 +---
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestFunction.c |  553 ---
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.c |  458 +++---
 .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.h |  248 ++--
 8 files changed, 2689 insertions(+), 1854 deletions(-)

-- 
2.9.0.windows.1

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

Re: [edk2] [PATCH edk2-platforms 1/2] Platform/ARM/Sgi: fix incorrect check of config-id value

[chandni cherukuri]

On Tue, Dec 4, 2018 at 8:26 PM Ard Biesheuvel  wrote:
>
> On Tue, 4 Dec 2018 at 12:36, Chandni Cherukuri
>  wrote:
> >
> > On SGI platform, the value of configuration ID can be zero.
> > So avoid returning an error from the function that creates
> > the system ID HOB in case the value of the configuration ID
> > is zero.
> >
> > While at it, improve some of the error messages as well.
> >
>
> So the platform ID is still guaranteed to be non-zero?
>
>

Platform ID is the part number of the platform and it is a
unique 12-bit value as specified by the SGI platform
specification. So it is guaranteed to be non-zero value.

Thanks
Chandni

> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chandni Cherukuri 
> > ---
> >  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c 
> > b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> > index 15ea571..065b23d 100644
> > --- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> > +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> > @@ -67,7 +67,7 @@ GetSgiSystemId (
> >
> >Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
> >if (Property == NULL) {
> > -DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
> > +DEBUG ((DEBUG_ERROR, "platform-id property not found\n"));
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > @@ -75,7 +75,7 @@ GetSgiSystemId (
> >
> >Property = fdt_getprop (HwCfgDtBlob, Offset, "config-id", NULL);
> >if (Property == NULL) {
> > -DEBUG ((DEBUG_ERROR, "Config Id is NULL\n"));
> > +DEBUG ((DEBUG_ERROR, "config-id property not found\n"));
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > @@ -121,7 +121,7 @@ SgiPlatformPeim (
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > -  if (HobData->PlatformId == 0 || HobData->ConfigId == 0) {
> > +  if (HobData->PlatformId == 0) {
> >  ASSERT (FALSE);
> >  return EFI_INVALID_PARAMETER;
> >}
> > --
> > 2.7.4
> >
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] Update edk2-stable201903 tag planning with Remove DuetPkg

[Liming Gao]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 EDK-II-Release-Planning.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/EDK-II-Release-Planning.md b/EDK-II-Release-Planning.md
index f70ca5a..23011fa 100644
--- a/EDK-II-Release-Planning.md
+++ b/EDK-II-Release-Planning.md
@@ -42,6 +42,7 @@
 * [Remove EdkShellBinPkg from 
edk2/master](https://bugzilla.tianocore.org/show_bug.cgi?id=1108)
 * [BaseTools: Support Array and C code style initialization in Structure 
PCD](https://bugzilla.tianocore.org/show_bug.cgi?id=1292)
 * [Merge EmuVariable and Real variable 
driver](https://bugzilla.tianocore.org/show_bug.cgi?id=1323)
+* [Remove DuetPkg](https://bugzilla.tianocore.org/show_bug.cgi?id=1322)
 * TBD Bugzilla List
 
 ---
-- 
2.13.0.windows.1

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

[edk2] [Patch] ModuleWriteGuide: Add notes to define library instance module type

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=834
If the library instance supports the cross module types PEIM, UEFI_DIRVER,
DXE_DRIVER. Its module type can be PEIM or UEFI_DRIVER or DXE_DRIVER.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Bi Dandan 
---
 3_module_development/31_what_is_an_edk_ii_module.md | 2 ++
 3_module_development/32_creating_a_module.md| 5 +
 2 files changed, 7 insertions(+)

diff --git a/3_module_development/31_what_is_an_edk_ii_module.md 
b/3_module_development/31_what_is_an_edk_ii_module.md
index ace8d53..9f0fe8d 100644
--- a/3_module_development/31_what_is_an_edk_ii_module.md
+++ b/3_module_development/31_what_is_an_edk_ii_module.md
@@ -78,6 +78,8 @@ EDK II defines many module types. The module type is used to:
   example, a PEIM/DXE_DRIVER type module can have "depex" section in .efi
   binary image; a UEFI_DRIVER can have .ui or .ver section in .efi binary 
image;
 
+* Indicate EntryPoint() or Constructor() API for different types of modules.
+
 * Indicate the suitable library instance for different types of modules. A
   library instance will point out what module types are supported in INF file.
 
diff --git a/3_module_development/32_creating_a_module.md 
b/3_module_development/32_creating_a_module.md
index 65a4ac7..325156f 100644
--- a/3_module_development/32_creating_a_module.md
+++ b/3_module_development/32_creating_a_module.md
@@ -197,6 +197,11 @@ instance:
   DebugLib
 ```
 
+Note: if the library supports the cross module types PEIM, UEFI_DIRVER, 
DXE_DRIVER.
+Its module type can be PEIM or UEFI_DRIVER or DXE_DRIVER. If it has the library
+constructor, its module type must be BASE. BASE type library constructor has 
no 
+the input parameter that can link to the cross driver types.
+
 ### 3.2.3 Adding a Package Dependency
 
 The [Packages] section of the INF file describes all packages dependencies of
-- 
2.13.0.windows.1

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

[edk2] [Patch] edk2 Dscspec: Add the syntax to initialize structure PCD with C style value

[Liming Gao]

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

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Bob Feng 
---
 3_edk_ii_dsc_file_format/33_platform_dsc_definition.md | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/3_edk_ii_dsc_file_format/33_platform_dsc_definition.md 
b/3_edk_ii_dsc_file_format/33_platform_dsc_definition.md
index 0ff9d9d..4c7fa99 100644
--- a/3_edk_ii_dsc_file_format/33_platform_dsc_definition.md
+++ b/3_edk_ii_dsc_file_format/33_platform_dsc_definition.md
@@ -239,10 +239,11 @@ The following are common definitions used by multiple 
section types.
 ::= {} {} {}
 ::= "{" {} {[] 
[ [] ]* } "}"
- ::= {} {} {}
+ ::= {} {} {} {}
::= {} {} {}
{} {}
   ::= "GUID("  ")"
+  ::= "CODE("  ")"
   ::= {  }
{} {}
::= "DEVICE_PATH("  ")"
@@ -345,6 +346,11 @@ All C data arrays used in PCD value fields must be byte 
arrays. The C format
 GUID style is a special case that is permitted in some fields that use the
 `` nomenclature.
 
+**_CData_**
+
+All C data used in PCD value CODE syntax can be C style value to initialize 
+C structure or Array in C source code.
+
 **_EOL_**
 
 The DOS End Of Line: "0x0D 0x0A" character must be used for all EDK II
-- 
2.13.0.windows.1

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

[edk2] [Patch] edk2 DecSpec: Support the syntax of the structure array for structure PCD

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=1292
1. Support the syntax of the structure array for structure PCD
2. Add the syntax to initialize structure PCD with C style value

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
Cc: Bob Feng 
---
 3_edk_ii_dec_file_format/310_pcd_sections.md | 2 +-
 .../32_package_declaration_dec_definitions.md| 9 -
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/3_edk_ii_dec_file_format/310_pcd_sections.md 
b/3_edk_ii_dec_file_format/310_pcd_sections.md
index 42beee9..2711788 100644
--- a/3_edk_ii_dec_file_format/310_pcd_sections.md
+++ b/3_edk_ii_dec_file_format/310_pcd_sections.md
@@ -125,7 +125,7 @@ PCDs listed in `PcdsFeatureFlag` sections must only be 
listed in
::= {} {}  "UINT32"
::=  
::= {} {}  "UINT64"
-   ::="{" 

+   ::= {} {} 
  "{" 

 "}" 
  ::= ""  
diff --git a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md 
b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
index a3120f3..bc794fe 100644
--- a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
+++ b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
@@ -87,6 +87,7 @@ DEC file (for example, `` statements are not 
permitted).
 ::= (a-zA-Z_)
   ::=  *
::=  # A valid C variable name.
+  ::= "[""]" # A valid C variable 
array name.
   ::= (0x21 - 0x7E)
   ::= [{0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
   {(0x5D - 0x7E)} {}]*
@@ -208,7 +209,7 @@ DEC file (for example, `` statements are not 
permitted).
::= {} {} {}
::= "{" {} {[]  
[ [] ]* } "}"
-::= {} {} {} 
+::= {} {} {} {}
   ::= {}{} {}
   {} {}
::= {} {} {}
@@ -216,6 +217,7 @@ DEC file (for example, `` statements are not 
permitted).
   ::= {} {} {}
   ::= {} {} {}
  ::= "GUID("  ")"
+ ::= "CODE("  ")"
  ::= {  }
   {} {}
   ::= "DEVICE_PATH("  ")"
@@ -326,6 +328,11 @@ All C data arrays used in PCD value fields must be byte 
arrays. The C format
 GUID style is a special case that is permitted in some fields that use the
 `` nomenclature.
 
+**_CData_**
+
+All C data used in PCD value CODE syntax can be C style value to initialize 
+C structure or Array in C source code.
+
 **_EOL_**
 
 The DOS End Of Line: "0x0D 0x0A" character sequence must be used for all EDK II
-- 
2.13.0.windows.1

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

Re: [edk2] [Patch] BaseTools: Optimize string concatenation

[Feng, Bob C]

Hi,

I have added the performance data in BZ 
https://bugzilla.tianocore.org/show_bug.cgi?id=1288 . Please have a review.

Thanks,
Bob


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Feng, 
Bob C
Sent: Sunday, November 11, 2018 8:41 AM
To: Leif Lindholm 
Cc: Carsey, Jaben ; edk2-devel@lists.01.org; Gao, 
Liming 
Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation

Hi Leif,

I use my desktop to do the benchmark.
CPU: Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
Memory: 16G
OS: Win10

I'll add the performance detailed data to BZ.

-Bob

-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
Sent: Friday, November 9, 2018 7:49 PM
To: Feng, Bob C 
Cc: edk2-devel@lists.01.org; Carsey, Jaben ; Gao, 
Liming 
Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation

Hi Bob,

On Fri, Nov 09, 2018 at 03:25:04AM +, Feng, Bob C wrote:
> Yes. I should show the data.
> 
> My unites scripts is as below. The parameter lines is a string list 
> which size is 43395. The test result is
> 
> ''.join(String list) time:  0.042262
> String += String time  :  3.822699
> 
> def TestPlus(lines):
> str_target = ""
> 
> for line in lines:
> str_target += line
> 
> return str_target
> 
> def TestJoin(lines):
> str_target = []
> 
> for line in lines:
> str_target.append(line)
> 
> return "".join(str_target)
> 
> def CompareStrCat():
> lines = GetStrings()
> print (len(lines))
> 
> begin = time.perf_counter()
> for _ in range(10):
> TestJoin(lines)
> end = time.perf_counter() - begin
> print ("''.join(String list) time: %f" % end)
> 
> begin = time.perf_counter()
> for _ in range(10):
> TestPlus(lines)
> end = time.perf_counter() - begin
> print ("String += String time: %f" % end)
> 
> For build OvmfX64, it's not very effective, it saves 2~3 second in 
> Parse/AutoGen phase, because OvmfX64 is relatively simple. It does not 
> enable much features such as Multiple SKU and structure PCD by default 
> and there is no big size Autogen.c/Autogen.h/Makefile generated 
> either. but for the complex platform, this patch will be much 
> effective. The unites above simulates a real case that there is a
> 43395 lines of Autogen.c generated.

I beg to differ. Shaving 2-3 seconds off the autogen phase is a substantial 
improvement.

However, on my wimpy 24-core Cortex-A53 system, the effect is not noticeable 
(fluctuates between 1:56-1:58 whether with or without this patch).

And even on my x86 workstation, I see no measurable difference (12-13s).
What is the hardware you are benchmarking on?

It does not appear to be detrimental to performance on any of my platforms, but 
I would like to see some more measurements, and I would like to see that logged 
with some more detail in bugzilla.

Regards,

Leif

> Since this patch mostly effect the Parser/AutoGen phase, I just use 
> "build genmake" to show the improvement data.
> The final result for clean build is:
> Current code:  17 seconds
> After patch:  15 seconds
> 
> Details:
> Current data:
> 
> d:\edk2 (master -> origin)
> λ build genmake -p OvmfPkg\OvmfPkgIa32X64.dsc -a IA32 -a X64 -t
> VS2015x86 Build environment: Windows-10-10.0.10240 Build start time: 
> 10:12:32, Nov.09 2018
> 
> WORKSPACE= d:\edk2
> ECP_SOURCE   = d:\edk2\edkcompatibilitypkg
> EDK_SOURCE   = d:\edk2\edkcompatibilitypkg
> EFI_SOURCE   = d:\edk2\edkcompatibilitypkg
> EDK_TOOLS_PATH   = d:\edk2\basetools
> EDK_TOOLS_BIN= d:\edk2\basetools\bin\win32
> CONF_PATH= d:\edk2\conf
> 
> Architecture(s)  = IA32 X64
> Build target = DEBUG
> Toolchain= VS2015x86
> 
> Active Platform  = d:\edk2\OvmfPkg\OvmfPkgIa32X64.dsc
> Flash Image Definition   = d:\edk2\OvmfPkg\OvmfPkgIa32X64.fdf
> 
> Processing meta-data ... done!
> Generating code . done!
> Generating makefile . done!
> Generating code .. done!
> Generating makefile .. done!
> 
> - Done -
> Build end time: 10:12:49, Nov.09 2018
> Build total time: 00:00:17
> 
> After applying this patch:
> 
> d:\edk2 (master -> origin)
> λ build genmake -p OvmfPkg\OvmfPkgIa32X64.dsc -a IA32 -a X64 -
> Build environment: Windows-10-10.0.10240  
> Build start time: 10:11:41, Nov.09 2018   
>   
> WORKSPACE= d:\edk2
> ECP_SOURCE   = d:\edk2\edkcompatibilitypkg
> EDK_SOURCE   = d:\edk2\edkcompatibilitypkg
> EFI_SOURCE   = d:\edk2\edkcompatibilitypkg
> EDK_TOOLS_PATH   = d:\edk2\basetools  
> EDK_TOOLS_BIN= d:\edk2\basetools\bin\win32
> CONF_PATH= d:\edk2\conf   
>  

Re: [edk2] [edk2-test][v2 Patch 0/3] Add VerifySignature() Test

[Jin, Eric]

Hi Laszlo,

Thank for the comments. Will send out new patch series later. 

Best Regards
Eric

-Original Message-
From: Laszlo Ersek  
Sent: Monday, December 3, 2018 9:50 PM
To: Jin, Eric 
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [edk2-test][v2 Patch 0/3] Add VerifySignature() Test

On 12/03/18 08:53, Eric Jin wrote:
> This is the cover letter.
> The series of patches are listed below.
> 
>   uefi-sct/SctPkg:Format code style in PKCS7Verify
>   uefi-sct/SctPkg:Add VerifySignature() Func Test
>   uefi-sct/SctPkg:Add VerifySignature() Conf Test
> 
> 
>  .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.c   |   93 +-
>  .../EFI/Protocol/PKCS7Verify/BlackBoxTest/Guid.h   |  192 +--
>  .../PKCS7Verify/BlackBoxTest/Pkcs7BBTest.inf   |  126 +-
>  .../BlackBoxTest/Pkcs7BBTestConformance.c  | 1305 +---
>  .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestData.c | 1568 
> +---
>  .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestFunction.c |  553 ---
>  .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.c |  458 +++---
>  .../PKCS7Verify/BlackBoxTest/Pkcs7BBTestMain.h |  248 ++--
>  8 files changed, 2689 insertions(+), 1854 deletions(-)
> 

A cover letter like this doesn't make any sense.

- Please enable shallow threading for git-send-email, so that the patch emails 
are (direct) children of the cover letter. One of the goals of the cover letter 
is to group the patches under a common parent message.

- The other purpose of cover letters is to summarize the goal of the patch 
series (high level problem statement, chosen solution, maybe mention one or two 
details if they are important).

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

Re: [edk2] Missing PI definitions?

[Andrew Fish via edk2-devel]

Liming,

Sorry I guess I was confusing this with EFI_RESOURCE_ATTRIBUTE_TESTED. 

I'll a little confused that it is implementation given it is passed into 
gDS->AddMemorySpace() the PI Spec defines the values of Capabilities to be 
defined in the UEFI Spec. as the GetMemoryMap() attributes. It is not clear 
that the implementation actually owns these bits? Almost feels like we should 
update the PI spec to include these #defines. 

I seem to remember we have been using these bits for a long time.

Thanks,

Andrew Fish

> On Dec 4, 2018, at 4:19 PM, Gao, Liming  wrote:
> 
> Andrew:
>  UEFI spec doesn't define them. They are the implement related definitions. 
> They are not required to be exposed to OS. We can add one header file in 
> MdeModulePkg to share them between DxeCore and MemoryTest driver. Besides, 
> ECP package will be retired. There is no change for it. 
> 
> #define EFI_MEMORY_PRESENT  0x0100ULL
> #define EFI_MEMORY_INITIALIZED  0x0200ULL
> #define EFI_MEMORY_TESTED   0x0400ULL
> 
> Thanks
> Liming
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org 
>> ] On Behalf Of
>> Andrew Fish

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

Re: [edk2] [PATCH v2] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Chiu, Chasel 
Sent: Wednesday, December 5, 2018 8:49 AM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Desimone, Nathaniel L 
; Zeng, Star ; Chiu, 
Chasel 
Subject: [PATCH v2] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

Cc: Jiewen Yao 
Cc: Desimone Nathaniel L 
Cc: Zeng Star 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 Maintainers.txt | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt index 9a36f0232b..63edf01d56 
100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -147,21 +147,27 @@ M: Liming Gao 
 
 IntelFsp2Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFsp2WrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspWrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelSiliconPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
--
2.13.3.windows.1

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

Re: [edk2] [PATCH v2] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Desimone, Nathaniel L]

Reviewed-by: Nate DeSimone 

-Original Message-
From: Chiu, Chasel 
Sent: Tuesday, December 4, 2018 4:49 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Desimone, Nathaniel L 
; Zeng, Star ; Chiu, 
Chasel 
Subject: [PATCH v2] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

Cc: Jiewen Yao 
Cc: Desimone Nathaniel L 
Cc: Zeng Star 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 Maintainers.txt | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt index 9a36f0232b..63edf01d56 
100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -147,21 +147,27 @@ M: Liming Gao 
 
 IntelFsp2Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFsp2WrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspWrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelSiliconPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
--
2.13.3.windows.1

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

[edk2] [PATCH v2] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Chasel, Chiu]

Cc: Jiewen Yao 
Cc: Desimone Nathaniel L 
Cc: Zeng Star 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 Maintainers.txt | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt
index 9a36f0232b..63edf01d56 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -147,21 +147,27 @@ M: Liming Gao 
 
 IntelFsp2Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFsp2WrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelFspWrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Nate DeSimone 
+R: Star Zeng 
 
 IntelSiliconPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
-- 
2.13.3.windows.1

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

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Desimone, Nathaniel L]

Hi Chasel,

My github username is "Nate DeSimone":

https://github.com/nate-desimone

Please add my name as Nate DeSimone instead of Desimone Nathaniel L.

Regards,
Nate

-Original Message-
From: Chiu, Chasel 
Sent: Tuesday, December 4, 2018 1:19 AM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Desimone, Nathaniel L 
; Zeng, Star ; Chiu, 
Chasel 
Subject: [PATCH] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

Cc: Jiewen Yao 
Cc: Desimone Nathaniel L 
Cc: Zeng Star 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 Maintainers.txt | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt index 9a36f0232b..1ed4166d63 
100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -147,21 +147,27 @@ M: Liming Gao 
 
 IntelFsp2Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFsp2WrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFspPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFspWrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelSiliconPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
--
2.13.3.windows.1

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

Re: [edk2] Missing PI definitions?

[Gao, Liming]

Andrew:
  UEFI spec doesn't define them. They are the implement related definitions. 
They are not required to be exposed to OS. We can add one header file in 
MdeModulePkg to share them between DxeCore and MemoryTest driver. Besides, ECP 
package will be retired. There is no change for it. 

#define EFI_MEMORY_PRESENT  0x0100ULL
#define EFI_MEMORY_INITIALIZED  0x0200ULL
#define EFI_MEMORY_TESTED   0x0400ULL

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Andrew Fish
>Sent: Wednesday, December 05, 2018 12:01 AM
>To: edk2-devel 
>Subject: [edk2] Missing PI definitions?
>
>Anyone remember why these defines are not in a common header in the
>MdePkg?
>
>/Volumes/Case/UDK2018(vUDK2018)>git grep MEMORY_PRESENT -- *.h
>EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:31:#define
>EFI_MEMORY_PRESENT  0x0100
>MdeModulePkg/Core/Dxe/DxeMain.h:101:#define EFI_MEMORY_PRESENT
>0x0100ULL
>MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMem
>oryTest.h:42:#define EFI_MEMORY_PRESENT  0x0100ULL
>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryT
>est.h:33:#define EFI_MEMORY_PRESENT  0x0100ULL
>/Volumes/Case/UDK2018(vUDK2018)>git grep EFI_MEMORY_INITIALIZED --
>*.h
>EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:32:#define
>EFI_MEMORY_INITIALIZED  0x0200
>MdeModulePkg/Core/Dxe/DxeMain.h:102:#define
>EFI_MEMORY_INITIALIZED  0x0200ULL
>MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMem
>oryTest.h:43:#define EFI_MEMORY_INITIALIZED  0x0200ULL
>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryT
>est.h:34:#define EFI_MEMORY_INITIALIZED  0x0200ULL
>/Volumes/Case/UDK2018(vUDK2018)>git grep EFI_MEMORY_TESTED -- *.h
>EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:33:#define
>EFI_MEMORY_TESTED   0x0400
>MdeModulePkg/Core/Dxe/DxeMain.h:103:#define EFI_MEMORY_TESTED
>0x0400ULL
>MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMem
>oryTest.h:44:#define EFI_MEMORY_TESTED   0x0400ULL
>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryT
>est.h:35:#define EFI_MEMORY_TESTED   0x0400ULL
>
>
>Thanks,
>
>Andrew Fish
>___
>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 v2 0/6] BaseTools: get rid of MAX_UINTN

[Gao, Liming]

Reviewed-by: Liming Gao  for this serials. 

On patch 4, I have the same comments to Laszlo. 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: Saturday, December 01, 2018 6:46 AM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Carsey, Jaben
>; Laszlo Ersek 
>Subject: [edk2] [PATCH v2 0/6] BaseTools: get rid of MAX_UINTN
>
>There should be no reason for the build tools to care about the native
>word size of a particular target, so relying on a definition of MAX_UINTN
>is definitely wrong, and most likely inaccurate on 32-bit build hosts.
>
>So refactor the code in CommonLib and DevicePath so we no longer rely
>on this definition.
>
>Changes since v1:
>- miss type change in #1 causing a build failure on MSVC
>- add acks from Jaben
>
>Cc: Laszlo Ersek 
>Cc: Yonghong Zhu 
>Cc: Liming Gao 
>Cc: Bob Feng 
>Cc: Jaben Carsey 
>
>Ard Biesheuvel (6):
>  BaseTools/CommonLib: avoid using 'native' word size in IP address
>handling
>  BaseTools/CommonLib: use explicit 64-bit type in Strtoi()
>  BaseTools/DevicePath: use explicit 64-bit number parsing routines
>  BaseTools/DevicePath: use MAX_UINT16 as default device path max size
>  BaseTools/CommonLib: get rid of 'native' type string parsing routines
>  BaseTools/CommonLib: drop definition of MAX_UINTN
>
> BaseTools/Source/C/Common/CommonLib.h |  25 ---
> BaseTools/Source/C/Common/CommonLib.c | 206 ++
> .../Source/C/DevicePath/DevicePathFromText.c  |   4 +-
> .../Source/C/DevicePath/DevicePathUtilities.c |   4 +-
> 4 files changed, 25 insertions(+), 214 deletions(-)
>
>--
>2.19.1
>
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Gao, Liming]

Laszlo:
  I agree with you. MAX_UINT32 is more comfortable. 

Thanks
Liming
>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Monday, December 03, 2018 9:06 PM
>To: Ard Biesheuvel ; edk2-devel@lists.01.org
>Cc: Zhu, Yonghong ; Gao, Liming
>; Feng, Bob C ; Carsey, Jaben
>
>Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
>default device path max size
>
>On 11/30/18 23:45, Ard Biesheuvel wrote:
>> Replace the default size limit of IsDevicePathValid() with a value
>> that does not depend on the native word size of the build host.
>>
>> 64 KB seems sufficient as the upper bound of a device path handled
>> by UEFI.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> Reviewed-by: Jaben Carsey 
>> ---
>>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> index d4ec2742b7c8..ba7f83e53070 100644
>> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> @@ -62,7 +62,7 @@ IsDevicePathValid (
>>ASSERT (DevicePath != NULL);
>>
>>if (MaxSize == 0) {
>> -MaxSize = MAX_UINTN;
>> +MaxSize = MAX_UINT16;
>>   }
>>
>>//
>> @@ -78,7 +78,7 @@ IsDevicePathValid (
>>return FALSE;
>>  }
>>
>> -if (NodeLength > MAX_UINTN - Size) {
>> +if (NodeLength > MAX_UINT16 - Size) {
>>return FALSE;
>>  }
>>  Size += NodeLength;
>>
>
>I'm somewhat undecided about this patch.
>
>(1) IsDevicePathValid() also exists in:
>
>- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
>- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
>
>Both have:
>
>  if (MaxSize == 0) {
>MaxSize = MAX_UINTN;
>  }
>
>Relative to those, this change departs quite strongly.
>
>
>(2) In addition, a single device path node may extend up to 64KB. That
>would be pathologic, yes, but the option is there.
>
>
>... Of course, we are discussing theoretical limits. Still I'd feel more
>comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
>cost us anything (in development effort), it would be a no-op on 32-bit
>build hosts, it would be a theoretical-only change on 64-bit build
>hosts, and it would leave us with a larger "safety margin".
>
>I won't insist, but I thought I should raise this. (Sorry if this has
>been discussed under v1 already.) If you agree, no need to repost (from
>my side anyway) just for this.
>
>With or without the update:
>
>Reviewed-by: Laszlo Ersek 
>
>Thanks
>Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-announce] Research Request

[Laszlo Ersek]

On 12/03/18 18:22, Jeremiah Cox wrote:
> Laszlo,
>
> Did you want to summarize your experience of our GitHub experiments?

That's right. I'll provide a summary below.

>  From your comments on the PRs, it sounded like the email
>  notifications did not provide the level of detail that you desire for
>  archival purposes.

That's correct.

> Stephano's email suggested that as long as we have an alternative
> mechanism to archive all metadata, that may still be acceptable.

Indeed, that's what I think as well.

>  I propose that https://github.com/josegonzalez/python-github-backup
>  may suffice.

I didn't miss it when you first recommended this utility, in:

  https://github.com/lersek/edk2/pull/2#issuecomment-443066812

I didn't respond explicitly because, when you made that suggestion, I
had already stated on the edk2-devel list that external tools that
aren't a core part of the service wouldn't cut it, for me anyway:

  76cb4d25-7eff-b19b-0dd5-2fcc3a1e7d82@redhat.com">http://mid.mail-archive.com/76cb4d25-7eff-b19b-0dd5-2fcc3a1e7d82@redhat.com

On 11/27/18 13:53, Laszlo Ersek wrote:
> GitHub has extremely good availability. I doubt that any hack we could
> come up with (and that we'd have to run ourselves, elsewhere), could
> muster the same service level. This means that sooner or later our
> mirroring hack would go down, while GitHub would stay up, and then
> we'd start losing updates to our "mirror".
>
> The offline & full coverage audit trail has to be generated by a core
> part of the service.

I don't know who "josegonzalez" is, whom he works for, what his
interests are, what kind of support we can get from him (for his
software), where and how we should run his software, what SLA we could
get from the organization that actually runs "python-github-backup" for
us, and so on.

To repeat, it suffices if we get *at least one* of
(a) comprehensive email notifications,
(b) comprehensive backup/archival functionality that is core to the
service itself.

At this point, GitHub seems to provide zero of these.

(I'll also repeat that I agree that GitHub provides a *lot* of important
and useful functionality in other areas. To me those areas are not
interchangeable.)

OK, so let me summarize my points, from:
- this thread,
- https://github.com/lersek/edk2/pull/1
- https://github.com/lersek/edk2/pull/2

On the plus side:

- It is possible to enable email notifications about one's own actions.

- It is possible to attach comments to specific lines of a patch.

- The "commits" button at the top gives a complete view, with subject
  line, commit message, code, and (optionally) review comments
  displayed.

- Rejecting a pull request does not make the HEAD of the proposed topic
  branch disappear; the commit reference from the PR keeps working.

- This remains true even if the originator (pull requester) repository
  is removed.

On the minus side:

- I couldn't attach comments to the commit message (in particular to
  specific lines of the commit message). As a stop-gap measure, I could
  make a general comment and refer to the commit message.

- When making a comment on a patch, it is unclear how "add single
  comment" differs from "start a review".

- Email notifications lack context. The notification does not name the
  commit (the subject line of the patch is not quoted, just the title of
  the PR), which is a problem if a series consists of multiple patches.
  In addition, trailing code context (that follows the review comment
  being sent out in email) is not cited in the email, only the preceding
  code context is. The commit message is also not quoted in the email.

- The email notifications contain "web bugs". My MUA warns that it
  blocks remote content while displaying these emails. The emails should
  be self-contained.

- Some questions remain unanswered about longevity of PR branches whose
  originating repos disappear:

  - How can a CLI user fetch the orphaned branch into a local clone of
his/hers? The GitHub WebUI does not provide a "remote URL" for this.

  - Do such branches survive "git gc" (garbage collection) that GitHub
surely runs periodically?

  - What happens if not only the originating repo is deleted, but the
pull requestor's user account too?

I don't insist that others agree with me that these are "minuses"; I'm
expressing my personal impressions. Furthermore, I have no idea at all
whether other web-based development tools perform better in these areas.

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

Re: [edk2] [edk2-announce] Research Request

[Philippe Mathieu-Daudé]

Hi Stephano,

On 14/11/18 19:34, stephano wrote:
> We are currently researching several different options to help make
> contributing to TianoCore easier for the community. A big part of this
> effort will be enabling pull requests and allowing for a more
> customizable code review process.
> 
> I am looking for members of the community willing to answer a few
> questions about these solutions to allow us to evaluate our options
> quickly. The options are:
> 
> System/Tool    Investigator
> Phabricator    Rebecca Cran (thank you again :) )
> Github    ???
> Gerrit    ???
> Gitlab    ???
> 
> I have a list of questions that I can send out to each investigator.
> Assuming you are familiar with the software/system, these questions
> should be answerable with a couple hours of research, writing, and
> screenshots / examples.

I'll run your checklist [*] with GitLab on Thursday.

[*] https://lists.01.org/pipermail/edk2-devel/2018-November/032462.html

> 
> Thanks in advance for your help!
> 
> -Stephano
> 
> ___
> 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] [edk2-announce] Research Request

[Laszlo Ersek]

Hi Rebecca,

On 12/03/18 22:39, Rebecca Cran wrote:
> On Monday, 3 December 2018 02:29:28 MST Laszlo Ersek wrote:
>> On 11/29/18 22:20, Rebecca Cran wrote:
>>> Would you be interested in going through this process with Phabricator,
>>> too?
>> Sure! Just tell me where to create an account.
> 
> Go to https://code.bluestop.org/auth/register/ to create a new account on the 
> system, or https://code.bluestop.org/auth/ to login using an existing GitHub 
> account.

This is just a quick note to confirm that I've now tagged this for
later. I hope to follow up later this week. (My apologies, I ran out of
time/steam today -- exploration like this requires a fresh mind.)

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

Re: [edk2] [platforms: PATCH 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver

[Ard Biesheuvel]

On Tue, 4 Dec 2018 at 18:19, Leif Lindholm  wrote:
>
> On Tue, Dec 04, 2018 at 05:40:14PM +0100, Ard Biesheuvel wrote:
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +MvGpioGetValue (
> > > > +  IN MARVELL_GPIO_PROTOCOL *This,
> > > > +  IN UINTN ControllerIndex,
> > > > +  IN UINTN GpioPin,
> > > > +  IN OUT BOOLEAN *Value
> > > > +  )
> > > > +{
> > > > +  UINTN BaseAddress;
> > > > +  EFI_STATUS Status;
> > > > +
> > > > +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> > > > +  if (EFI_ERROR (Status)) {
> > > > +DEBUG ((DEBUG_ERROR,
> > > > +  "%a: Fail to get value of pin #%d\n",
> > > > +  __FUNCTION__,
> > > > +  GpioPin));
> > > > +return Status;
> > > > +  }
> > > > +
> > > > +  BaseAddress = 
> > > > mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> > > > +
> > > > +  *Value = !!(MmioRead32 (BaseAddress + GPIO_DATA_IN_REG) & BIT 
> > > > (GpioPin));
> > >
> > > Please don't !!.
> > > If necessary, please shift.
> >
> > Or cast to (BOOLEAN)
>
> Would be ideal if BOOLEAN wasn't just a typedef for unsigned char :/
>

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

Re: [edk2] [platforms: PATCH 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver

[Leif Lindholm]

On Tue, Dec 04, 2018 at 05:40:14PM +0100, Ard Biesheuvel wrote:
> > > +STATIC
> > > +EFI_STATUS
> > > +MvGpioGetValue (
> > > +  IN MARVELL_GPIO_PROTOCOL *This,
> > > +  IN UINTN ControllerIndex,
> > > +  IN UINTN GpioPin,
> > > +  IN OUT BOOLEAN *Value
> > > +  )
> > > +{
> > > +  UINTN BaseAddress;
> > > +  EFI_STATUS Status;
> > > +
> > > +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> > > +  if (EFI_ERROR (Status)) {
> > > +DEBUG ((DEBUG_ERROR,
> > > +  "%a: Fail to get value of pin #%d\n",
> > > +  __FUNCTION__,
> > > +  GpioPin));
> > > +return Status;
> > > +  }
> > > +
> > > +  BaseAddress = 
> > > mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> > > +
> > > +  *Value = !!(MmioRead32 (BaseAddress + GPIO_DATA_IN_REG) & BIT 
> > > (GpioPin));
> >
> > Please don't !!.
> > If necessary, please shift.
> 
> Or cast to (BOOLEAN)

Would be ideal if BOOLEAN wasn't just a typedef for unsigned char :/

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

Re: [edk2] [platforms: PATCH 12/12] Marvell/Armada7k8k: Introduce NonDiscoverable device init routines

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:41AM +0200, Marcin Wojtas wrote:
> To abstract the initialization required for non-discoverable devices, which
> is often platform specific (i.e., enabling VBUS gpios for USB), introduce
> a NonDiscoverableInitLib for use by the NonDiscoverable code, for which
> each platform can supply its own version.
> 
> Add VBUS enabling routines for supported platforms (Armada70x0Db,
> Armada80x0Db, Armada80x0McBin).

Please expand VBUS on first use in commit message.

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Marvell/Marvell.dec  
>|   1 +
>  Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc   
>|   3 +
>  Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc   
>|   3 +
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
>|   3 +
>  
> Platform/Marvell/Armada70x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  |  47 
>  
> Platform/Marvell/Armada80x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  |  48 +
>  
> Platform/SolidRun/Armada80x0McBin/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  |  48 +
>  Silicon/Marvell/Drivers/NonDiscoverableDxe/NonDiscoverableDxe.inf
>|   1 +
>  Silicon/Marvell/Include/Library/NonDiscoverableInitLib.h 
>|  28 +
>  
> Platform/Marvell/Armada70x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.c 
>   |  99 +
>  
> Platform/Marvell/Armada80x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.c 
>   | 113 
>  
> Platform/SolidRun/Armada80x0McBin/NonDiscoverableInitLib/NonDiscoverableInitLib.c
>|  73 +
>  Silicon/Marvell/Drivers/NonDiscoverableDxe/NonDiscoverableDxe.c  
>|   7 +-
>  13 files changed, 471 insertions(+), 3 deletions(-)
>  create mode 100644 
> Platform/Marvell/Armada70x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  create mode 100644 
> Platform/SolidRun/Armada80x0McBin/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
>  create mode 100644 Silicon/Marvell/Include/Library/NonDiscoverableInitLib.h
>  create mode 100644 
> Platform/Marvell/Armada70x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.c
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.c
>  create mode 100644 
> Platform/SolidRun/Armada80x0McBin/NonDiscoverableInitLib/NonDiscoverableInitLib.c
> 
> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> index 7e1c37a..20a32ef 100644
> --- a/Silicon/Marvell/Marvell.dec
> +++ b/Silicon/Marvell/Marvell.dec
> @@ -63,6 +63,7 @@
>ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h
>ArmadaIcuLib|Include/Library/ArmadaIcuLib.h
>ArmadaSoCDescLib|Include/Library/ArmadaSoCDescLib.h
> +  NonDiscoverableInitLib|Include/Library/NonDiscoverableInitLib.h
>SampleAtResetLib|Include/Library/SampleAtResetLib.h
>  
>  [Protocols]
> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc 
> b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> index 31815e4..e8cd177 100644
> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> @@ -48,6 +48,9 @@
>  
>  !include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>  
> +[LibraryClasses.common]
> +  
> NonDiscoverableInitLib|Platform/Marvell/Armada70x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
> +
>  [Components.common]
>Silicon/Marvell/Armada7k8k/DeviceTree/Armada70x0Db.inf
>  
> diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
> b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> index 42f7bd3..8e8c2ba 100644
> --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> @@ -48,6 +48,9 @@
>  
>  !include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>  
> +[LibraryClasses.common]
> +  
> NonDiscoverableInitLib|Platform/Marvell/Armada80x0Db/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
> +
>  [Components.common]
>Silicon/Marvell/Armada7k8k/DeviceTree/Armada80x0Db.inf
>  
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc 
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> index 077224d..d080136 100644
> --- a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> @@ -49,6 +49,9 @@
>  
>  !include Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
>  
> +[LibraryClasses.common]
> +  
> NonDiscoverableInitLib|Platform/SolidRun/Armada80x0McBin/NonDiscoverableInitLib/NonDiscoverableInitLib.inf
> +
>  [Components.common]
>Silicon/Marvell/Armada7k8k/DeviceTree/Armada80x0McBin.inf
>  
> diff --git 

Re: [edk2] [PATCH 1/4] OvmfPkg: introduce ACPI Test Support data structure and GUID

[Laszlo Ersek]

On 12/03/18 18:09, Igor Mammedov wrote:
> On Sun, 25 Nov 2018 11:01:49 +0100
> Laszlo Ersek  wrote:
> 
>> QEMU's test suite includes a set of cases called "BIOS tables test". Among
>> other things, it locates the RSD PTR ACPI table in guest RAM, and then
>> (chasing pointers to other ACPI tables) performs various sanity checks on
>> the QEMU-generated and firmware-installed tables.
>>
>> Currently this set of test cases doesn't work with UEFI guests, because
>> the ACPI spec's definition for locating the RSD PTR in UEFI guests is a
>> lot harder to implement from the hypervisor side -- just with raw guest
>> memory access -- than it is when scraping the memory of BIOS guests.
>>
>> Introduce a signature GUID, and a small, MB-aligned structure, identified
>> by the GUID. The hypervisor should loop over all MB-aligned pages in guest
>> RAM until one matches the signature GUID at offset 0, at which point the
>> hypervisor can fetch the RSDP address field(s) from the structure.
>>
>> QEMU's test logic currently spins on a pre-determined guest address, until
>> that address assumes a magic value. The method described in this patch is
>> conceptually the same ("busy loop until match is found"), except there is
>> no hard-coded address. This plays a lot more nicely with UEFI guest
>> firmware (we'll be able to use the normal page allocation UEFI service).
>> Given the size of EFI_GUID (16 bytes -- 128 bits), mismatches should be
>> astronomically unlikely. In addition, given the typical guest RAM size for
>> such tests (128 MB), there are 128 locations to check in one iteration of
>> the "outer" loop, which shouldn't introduce an intolerable delay after the
>> guest stores the RSDP address(es), and then the GUID.
>>
>> The GUID that the hypervisor should search for is
>>
>>   AB87A6B1-2034-BDA0-71BD-375007757785
>>
>> Expressed as a byte array:
>>
>>   {
>> 0xb1, 0xa6, 0x87, 0xab,
>> 0x34, 0x20,
>> 0xa0, 0xbd,
>> 0x71, 0xbd, 0x37, 0x50, 0x07, 0x75, 0x77, 0x85
>>   }
>>
>> Note that in the patch, we define "gAcpiTestSupportGuid" with all bits
>> inverted. This is a simple method to prevent the UEFI binaries that
>> incorporate "gAcpiTestSupportGuid" from matching the actual GUID in guest
>> RAM.
>>
>> Cc: Anthony Perard 
>> Cc: Ard Biesheuvel 
>> Cc: Drew Jones 
>> Cc: Igor Mammedov 
>> Cc: Jordan Justen 
>> Cc: Julien Grall 
>> Cc: Philippe Mathieu-Daudé 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  OvmfPkg/OvmfPkg.dec|  1 +
>>  OvmfPkg/Include/Guid/AcpiTestSupport.h | 67 
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 7666297cf8f1..e8c7d9423f43 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -73,11 +73,12 @@ [LibraryClasses]
>>  [Guids]
>>gUefiOvmfPkgTokenSpaceGuid  = {0x93bb96af, 0xb9f2, 0x4eb8, {0x94, 
>> 0x62, 0xe0, 0xba, 0x74, 0x56, 0x42, 0x36}}
>>gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 
>> 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
>>gOvmfPlatformConfigGuid = {0x7235c51c, 0x0c80, 0x4cab, {0x87, 
>> 0xac, 0x3b, 0x08, 0x4a, 0x63, 0x04, 0xb1}}
>>gVirtioMmioTransportGuid= {0x837dca9e, 0xe874, 0x4d82, {0xb2, 
>> 0x9a, 0x23, 0xfe, 0x0e, 0x23, 0xd1, 0xe2}}
>>gQemuRamfbGuid  = {0x557423a1, 0x63ab, 0x406c, {0xbe, 
>> 0x7e, 0x91, 0xcd, 0xbc, 0x08, 0xc4, 0x57}}
>>gXenBusRootDeviceGuid   = {0xa732241f, 0x383d, 0x4d9c, {0x8a, 
>> 0xe1, 0x8e, 0x09, 0x83, 0x75, 0x89, 0xd7}}
>>gRootBridgesConnectedEventGroupGuid = {0x24a2d66f, 0xeedd, 0x4086, {0x90, 
>> 0x42, 0xf2, 0x6e, 0x47, 0x97, 0xee, 0x69}}
>> +  gAcpiTestSupportGuid= {0x5478594e, 0xdfcb, 0x425f, {0x8e, 
>> 0x42, 0xc8, 0xaf, 0xf8, 0x8a, 0x88, 0x7a}}
>>  
>>  [Protocols]
>>gVirtioDeviceProtocolGuid   = {0xfa920010, 0x6785, 0x4941, {0xb6, 
>> 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}}
>> diff --git a/OvmfPkg/Include/Guid/AcpiTestSupport.h 
>> b/OvmfPkg/Include/Guid/AcpiTestSupport.h
>> new file mode 100644
>> index ..8526f2bfb391
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Guid/AcpiTestSupport.h
>> @@ -0,0 +1,67 @@
>> +/** @file
>> +  Expose the address(es) of the ACPI RSD PTR table(s) in a MB-aligned 
>> structure
>> +  to the hypervisor.
>> +
>> +  The hypervisor locates the MB-aligned structure based on the signature 
>> GUID
>> +  that is at offset 0 in the structure. Once the RSD PTR address(es) are
>> +  retrieved, the hypervisor may perform various ACPI checks.
>> +
>> +  This feature is a development aid, for supporting ACPI table unit tests in
>> +  hypervisors. Do not enable in production builds.
>> +
>> +  Copyright (C) 2018, Red Hat, Inc.
>> +
>> +  This program and the accompanying materials are licensed and made 
>> available
>> +  under the terms and conditions of the BSD License that accompanies this

Re: [edk2] [platforms: PATCH 11/12] Marvell/Armada7k8k: Enable GPIO drivers compilation

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:40AM +0200, Marcin Wojtas wrote:
> Enable building new GPIO drivers before adding VBUS
> pins handling. Update relevant boards .dsc files with
> IO expander information.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 

Reviewed-by: Leif Lindholm 


> ---
>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 2 ++
>  Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc| 4 ++--
>  Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf.inc| 2 ++
>  Platform/Marvell/Armada80x0Db/Armada80x0Db.fdf.inc| 2 ++
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.fdf.inc | 2 ++
>  5 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc 
> b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index d4c67a2..62a46a6 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -456,6 +456,8 @@
>Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
>  
># Platform drivers
> +  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> +  Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
>Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>MdeModulePkg/Bus/I2c/I2cDxe/I2cDxe.inf
>Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.inf
> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc 
> b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> index a935f36..31815e4 100644
> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> @@ -89,8 +89,8 @@
>gMarvellTokenSpaceGuid.PcdChip1MppSel6|{ 0xE, 0xE, 0xE, 0x0, 0x0, 0x0, 
> 0x0, 0x0, 0x0, 0x0 }
>  
># I2C
> -  gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60 }
> -  gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0 }
> +  gMarvellTokenSpaceGuid.PcdI2cSlaveAddresses|{ 0x50, 0x57, 0x60, 0x21 }
> +  gMarvellTokenSpaceGuid.PcdI2cSlaveBuses|{ 0x0, 0x0, 0x0, 0x0 }
>gMarvellTokenSpaceGuid.PcdI2cControllersEnabled|{ 0x1, 0x1 }
>gMarvellTokenSpaceGuid.PcdEepromI2cAddresses|{ 0x50, 0x57 }
>gMarvellTokenSpaceGuid.PcdEepromI2cBuses|{ 0x0, 0x0 }
> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf.inc 
> b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf.inc
> index b7e7a65..7129606 100644
> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf.inc
> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.fdf.inc
> @@ -12,6 +12,8 @@
>  
>  # Per-board additional content of the DXE phase firmware volume
>  
> +  INF Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
> +
># DTB
>INF RuleOverride = DTB 
> Silicon/Marvell/Armada7k8k/DeviceTree/Armada70x0Db.inf
>  
> diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.fdf.inc 
> b/Platform/Marvell/Armada80x0Db/Armada80x0Db.fdf.inc
> index 81a81d0..f2fcc55 100644
> --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.fdf.inc
> +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.fdf.inc
> @@ -12,6 +12,8 @@
>  
>  # Per-board additional content of the DXE phase firmware volume
>  
> +  INF Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
> +
># DTB
>INF RuleOverride = DTB 
> Silicon/Marvell/Armada7k8k/DeviceTree/Armada80x0Db.inf
>  
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.fdf.inc 
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.fdf.inc
> index 326da2e..254fcee 100644
> --- a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.fdf.inc
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.fdf.inc
> @@ -12,6 +12,8 @@
>  
>  # Per-board additional content of the DXE phase firmware volume
>  
> +  INF Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> +
># DTB
>INF RuleOverride = DTB 
> Silicon/Marvell/Armada7k8k/DeviceTree/Armada80x0McBin.inf
>  
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 10/12] Marvell/Drivers: MvPca95xxDxe: Introduce I2C GPIO driver

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:39AM +0200, Marcin Wojtas wrote:
> Marvell Armada 7k/8k-based platforms may use Pca95xx to extend
> amount of the GPIO pins.
> 
> This patch introduces support for PCA95xxx I2C IO expander family,
> which is a producer of MARVELL_GPIO_PROTOCOL, by implementing
> necessary routines.
> 
> Driver is based on initial work done by Allen Yan .

Yeah, this is the patch I would also like to add the enums and enum
values previously mentioned.
 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf |  44 ++
>  Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.h   |  74 +++
>  Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.c   | 592 
> 
>  3 files changed, 710 insertions(+)
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.h
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.c
> 
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf 
> b/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
> new file mode 100644
> index 000..3066732
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.inf
> @@ -0,0 +1,44 @@
> +## @file
> +#
> +#  Copyright (c) 2017, Marvell International Ltd. All rights reserved.
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = MvPca95xxDxe
> +  FILE_GUID  = f0e405eb-8407-43b9-88e6-2f7d70715c72
> +  MODULE_TYPE= DXE_DRIVER
> +  VERSION_STRING = 1.0
> +  ENTRY_POINT= MvPca95xxEntryPoint
> +
> +[Sources]
> +  MvPca95xxDxe.c
> +  MvPca95xxDxe.h
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  MemoryAllocationLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gEfiDriverBindingProtocolGuid
> +  gEfiI2cIoProtocolGuid
> +  gMarvellBoardDescProtocolGuid
> +  gMarvellGpioProtocolGuid
> +
> +[Depex]
> +  TRUE
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.h 
> b/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.h
> new file mode 100644
> index 000..43daee0
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvPca95xxDxe/MvPca95xxDxe.h
> @@ -0,0 +1,74 @@
> +/**
> +*
> +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +#ifndef __MV_PCA953X_H__
> +#define __MV_PCA953X_H__
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define PCA95XX_GPIO_SIGNATURE   SIGNATURE_32 ('I', 'O', 'E', 'X')
> +
> +#ifndef BIT
> +#define BIT(nr)  (1 << (nr))
> +#endif

Eew, now we're adding it again, in yet another file.
Same comment as (patch - 2) (I think). Consider trying to add this
macro (and a BITPOS one) to Base.h.

The alternative is just inlining the shifts. It's basic C and doesn't
confuse things.

> +
> +#define PCA95XX_INPUT_REG0x0
> +#define PCA95XX_OUTPUT_REG   0x2
> +#define PCA95XX_DIRECTION_REG0x6
> +
> +#define PCA95XX_BANK_SIZE8
> +#define PCA95XX_OPERATION_COUNT  2
> +#define PCA95XX_OPERATION_LENGTH 1
> +
> +typedef enum {
> +  PCA9505_PIN_COUNT = 40,
> +  PCA9534_PIN_COUNT = 8,
> +  PCA9535_PIN_COUNT = 16,
> +  PCA9536_PIN_COUNT = 4,
> +  PCA9537_PIN_COUNT = 4,
> +  PCA9538_PIN_COUNT = 8,
> +  PCA9539_PIN_COUNT = 16,
> +  PCA9554_PIN_COUNT = 8,
> +  PCA9555_PIN_COUNT = 16,
> +  PCA9556_PIN_COUNT = 16,
> +  PCA9557_PIN_COUNT = 16,
> +} PCA95XX_PIN_COUNT;
> +
> +typedef enum {
> +  PCA95XX_READ,
> +  PCA95XX_WRITE,
> +} PCA95XX_OPERATION;

So, the only use I see of this is where it is used to in turn select
between I2C_FLAG_READ and I2C_FLAG_NORESTART. Can we instead use
these 

Re: [edk2] [platforms: PATCH 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver

[Ard Biesheuvel]

On Tue, 4 Dec 2018 at 17:37, Leif Lindholm  wrote:
>
> On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote:
> > From: jinghua 
> >
> > Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> > one in AP and two in each possible CP hardware blocks.
> >
> > This patch introduces support for them, which is a producer of
> > MARVELL_GPIO_PROTOCOL, which add necessary routines.
> > Hardware description of the controllers is placed in MvHwDescLib.c,
> > same as other devices.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas 
> > ---
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf |  43 +++
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h   |  52 
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c   | 298 
> > 
> >  3 files changed, 393 insertions(+)
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> >
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf 
> > b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > new file mode 100644
> > index 000..2d56433
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > @@ -0,0 +1,43 @@
> > +## @file
> > +#
> > +#  Copyright (c) 2017, Marvell International Ltd. All rights reserved.
> > +#
> > +#  This program and the accompanying materials are licensed and made 
> > available
> > +#  under the terms and conditions of the BSD License which accompanies this
> > +#  distribution. The full text of the license may be found at
> > +#  http://opensource.org/licenses/bsd-license.php
> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> > +#  IMPLIED.
> > +#
> > +
> > +[Defines]
> > +  INF_VERSION= 0x0001001A
> > +  BASE_NAME  = MvGpioDxe
> > +  FILE_GUID  = 706eb761-b3b5-4f41-8558-5fd9217c0079
> > +  MODULE_TYPE= DXE_DRIVER
> > +  VERSION_STRING = 1.0
> > +  ENTRY_POINT= MvGpioEntryPoint
> > +
> > +[Sources]
> > +  MvGpioDxe.c
> > +  MvGpioDxe.h
> > +
> > +[Packages]
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > +  ArmadaSoCDescLib
> > +  DebugLib
> > +  MemoryAllocationLib
> > +  UefiDriverEntryPoint
> > +  UefiLib
> > +
> > +[Protocols]
> > +  gMarvellBoardDescProtocolGuid
> > +  gMarvellGpioProtocolGuid
> > +
> > +[Depex]
> > +  TRUE
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h 
> > b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > new file mode 100644
> > index 000..48744dc
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > @@ -0,0 +1,52 @@
> > +/**
> > +*
> > +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> > +*
> > +*  This program and the accompanying materials are licensed and made 
> > available
> > +*  under the terms and conditions of the BSD License which accompanies this
> > +*  distribution. The full text of the license may be found at
> > +*  http://opensource.org/licenses/bsd-license.php
> > +*
> > +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> > IMPLIED.
> > +*
> > +**/
> > +#ifndef __MV_GPIO_H__
> > +#define __MV_GPIO_H__
> > +
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#define GPIO_SIGNATURE   SIGNATURE_32 ('G', 'P', 'I', 'O')
>
> Needs more MV.
>
> > +
> > +#ifndef BIT
> > +#define BIT(nr)  (1 << (nr))
> > +#endif
>
> OK, you are using this with non-constants.
> But please drop the #ifndef and submit a patch to add a BIT macro to
> MdePkg/Include/Base.h. (And drop this if you get it accepted :)
>
> > +
> > +// Marvell GPIO Controller Registers
> > +#define GPIO_DATA_OUT_REG(0x0)
> > +#define GPIO_OUT_EN_REG  (0x4)
> > +#define GPIO_BLINK_EN_REG(0x8)
> > +#define GPIO_DATA_IN_POL_REG (0xc)
> > +#define GPIO_DATA_IN_REG (0x10)
>
> Needs more MV.
>
> > +
> > +typedef struct {
> > +  MARVELL_GPIO_PROTOCOL   GpioProtocol;
> > +  MV_BOARD_GPIO_DESC  *Desc;
> > +  UINTN   Signature;
> > +  EFI_HANDLE  Handle;
> > +} MV_GPIO;
> > +
> > +#endif // __MV_GPIO_H__
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c 
> > b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> > new file mode 100644
> > index 000..fc2d416
> > --- 

Re: [edk2] [platforms: PATCH 09/12] Marvell/Drivers: I2c: Use common header for macros

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:38AM +0200, Marcin Wojtas wrote:
> Hitherto I2c solution used same macros, defined in multiple
> places. Move them to a new common header.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 

Reviewed-by: Leif Lindholm 

> ---
>  Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h | 10 ---
>  Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h   | 17 ++-
>  Silicon/Marvell/Include/Protocol/MvI2c.h  | 31 
> 
>  Silicon/Marvell/Applications/EepromCmd/EepromCmd.c|  5 +---
>  Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c |  3 +-
>  Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c   |  4 +--
>  6 files changed, 37 insertions(+), 33 deletions(-)
>  create mode 100644 Silicon/Marvell/Include/Protocol/MvI2c.h
> 
> diff --git a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h 
> b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> index b1af645..c32ee48 100644
> --- a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> +++ b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> @@ -41,16 +41,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH 
> DAMAGE.
>  
>  #define MAX_BUFFER_LENGTH 64
>  
> -/*
> - * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> - * transmission without repeated start operation.
> - * FIXME: This flag is also defined in Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> - * and it's important to have both version synced. This solution is
> - * temporary and shared flag should be used by both files.
> - * Situation is analogous with I2C_GUID, which also should be common, but is
> - * for now defined same way in two header files.
> - */
> -#define I2C_FLAG_NORESTART 0x0002
>  #define I2C_GUID \
>{ \
>0xadc1901b, 0xb83c, 0x4831, { 0x8f, 0x59, 0x70, 0x89, 0x8f, 0x26, 0x57, 
> 0x1e } \
> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h 
> b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> index 3c9beaf..6850c34 100644
> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> @@ -32,8 +32,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  
> ***/
>  
> -#ifndef __MV_I2C_H__
> -#define __MV_I2C_H__
> +#ifndef __MV_I2C_DXE_H__
> +#define __MV_I2C_DXE_H__
>  
>  #include 
>  
> @@ -75,17 +75,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH 
> DAMAGE.
>  #define I2C_FAST   0x2
>  #define I2C_FASTEST0x3
>  
> -/*
> - * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> - * transmission without repeated start operation.
> - * FIXME: This flag is also defined in
> - * Platforms/Marvell/Include/Protocol/Eeprom.h and it's important to have 
> both
> - * version synced. This solution is temporary and shared flag should be used 
> by
> - * both files.
> - * Situation is analogous with I2C_GUID, which also should be common, but is
> - * for now defined same way in two header files.
> - */
> -#define I2C_FLAG_NORESTART 0x0002
>  #define I2C_GUID \
>{ \
>0xadc1901b, 0xb83c, 0x4831, { 0x8f, 0x59, 0x70, 0x89, 0x8f, 0x26, 0x57, 
> 0x1e } \
> @@ -266,4 +255,4 @@ MvI2cEnableConf (
>IN EFI_STATUS  *I2cStatus OPTIONAL
>);
>  
> -#endif // __MV_I2C_H__
> +#endif // __MV_I2C_DXE_H__
> diff --git a/Silicon/Marvell/Include/Protocol/MvI2c.h 
> b/Silicon/Marvell/Include/Protocol/MvI2c.h
> new file mode 100644
> index 000..d8e644e
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Protocol/MvI2c.h
> @@ -0,0 +1,31 @@
> +/**
> +*
> +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +#ifndef __MV_I2C_H__
> +#define __MV_I2C_H__
> +
> +/*
> + * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> + * transmission without repeated start operation.
> + */
> +#define I2C_FLAG_NORESTART 0x0002
> +
> +/*
> + * Helper macros for maintaining multiple I2C buses
> + * and devices defined via EFI_I2C_DEVICE.
> + */
> +#define I2C_DEVICE_ADDRESS(Index)  ((Index) & MAX_UINT16)
> +#define I2C_DEVICE_BUS(Index)  ((Index) >> 16)
> +#define I2C_DEVICE_INDEX(Bus, Address) (((Address) & MAX_UINT16) | (Bus) << 
> 16)
> +
> +#endif
> diff --git a/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c 
> b/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c
> index f43e411..a390f23 100644
> --- 

Re: [edk2] [platforms: PATCH 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote:
> From: jinghua 
> 
> Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> one in AP and two in each possible CP hardware blocks.
> 
> This patch introduces support for them, which is a producer of
> MARVELL_GPIO_PROTOCOL, which add necessary routines.
> Hardware description of the controllers is placed in MvHwDescLib.c,
> same as other devices.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf |  43 +++
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h   |  52 
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c   | 298 
> 
>  3 files changed, 393 insertions(+)
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
>  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> 
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf 
> b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> new file mode 100644
> index 000..2d56433
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> @@ -0,0 +1,43 @@
> +## @file
> +#
> +#  Copyright (c) 2017, Marvell International Ltd. All rights reserved.
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = MvGpioDxe
> +  FILE_GUID  = 706eb761-b3b5-4f41-8558-5fd9217c0079
> +  MODULE_TYPE= DXE_DRIVER
> +  VERSION_STRING = 1.0
> +  ENTRY_POINT= MvGpioEntryPoint
> +
> +[Sources]
> +  MvGpioDxe.c
> +  MvGpioDxe.h
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmadaSoCDescLib
> +  DebugLib
> +  MemoryAllocationLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gMarvellBoardDescProtocolGuid
> +  gMarvellGpioProtocolGuid
> +
> +[Depex]
> +  TRUE
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h 
> b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> new file mode 100644
> index 000..48744dc
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> @@ -0,0 +1,52 @@
> +/**
> +*
> +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +#ifndef __MV_GPIO_H__
> +#define __MV_GPIO_H__
> +
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#define GPIO_SIGNATURE   SIGNATURE_32 ('G', 'P', 'I', 'O')

Needs more MV.

> +
> +#ifndef BIT
> +#define BIT(nr)  (1 << (nr))
> +#endif

OK, you are using this with non-constants.
But please drop the #ifndef and submit a patch to add a BIT macro to
MdePkg/Include/Base.h. (And drop this if you get it accepted :)

> +
> +// Marvell GPIO Controller Registers
> +#define GPIO_DATA_OUT_REG(0x0)
> +#define GPIO_OUT_EN_REG  (0x4)
> +#define GPIO_BLINK_EN_REG(0x8)
> +#define GPIO_DATA_IN_POL_REG (0xc)
> +#define GPIO_DATA_IN_REG (0x10)

Needs more MV.

> +
> +typedef struct {
> +  MARVELL_GPIO_PROTOCOL   GpioProtocol;
> +  MV_BOARD_GPIO_DESC  *Desc;
> +  UINTN   Signature;
> +  EFI_HANDLE  Handle;
> +} MV_GPIO;
> +
> +#endif // __MV_GPIO_H__
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c 
> b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> new file mode 100644
> index 000..fc2d416
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> @@ -0,0 +1,298 @@
> +/**
> +*
> +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> 

[edk2] Missing PI definitions?

[Andrew Fish]

Anyone remember why these defines are not in a common header in the MdePkg?

/Volumes/Case/UDK2018(vUDK2018)>git grep MEMORY_PRESENT -- *.h
EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:31:#define 
EFI_MEMORY_PRESENT  0x0100
MdeModulePkg/Core/Dxe/DxeMain.h:101:#define EFI_MEMORY_PRESENT  
0x0100ULL
MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMemoryTest.h:42:#define
 EFI_MEMORY_PRESENT  0x0100ULL
MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.h:33:#define 
EFI_MEMORY_PRESENT  0x0100ULL
/Volumes/Case/UDK2018(vUDK2018)>git grep EFI_MEMORY_INITIALIZED -- *.h
EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:32:#define 
EFI_MEMORY_INITIALIZED  0x0200
MdeModulePkg/Core/Dxe/DxeMain.h:102:#define EFI_MEMORY_INITIALIZED  
0x0200ULL
MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMemoryTest.h:43:#define
 EFI_MEMORY_INITIALIZED  0x0200ULL
MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.h:34:#define 
EFI_MEMORY_INITIALIZED  0x0200ULL
/Volumes/Case/UDK2018(vUDK2018)>git grep EFI_MEMORY_TESTED -- *.h
EdkCompatibilityPkg/Foundation/Include/TianoTypes.h:33:#define 
EFI_MEMORY_TESTED   0x0400
MdeModulePkg/Core/Dxe/DxeMain.h:103:#define EFI_MEMORY_TESTED   
0x0400ULL
MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/LightMemoryTest.h:44:#define
 EFI_MEMORY_TESTED   0x0400ULL
MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTest.h:35:#define 
EFI_MEMORY_TESTED   0x0400ULL


Thanks,

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

Re: [edk2] [platforms: PATCH 07/12] Marvell/Protocol: Introduce MARVELL_GPIO_PROTOCOL

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:36AM +0200, Marcin Wojtas wrote:
> From: jinghua 
> 
> This patch introduces protocol that can be used by multiple
> producers (e.g. platform driver or I2C I/O expander).
> It exposes generic api for controlling GPIO pins state.
> Drives are differentiated by MARVELL_GPIO_DRIVER_TYPE
> field of driver's MV_GPIO_DEVICE_PATH. In order to ease
> selection of the desired GPIO controller a helper
> function was added - MarvellGpioGetHandle().
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Marvell/Marvell.dec   |   1 +
>  Silicon/Marvell/Include/Protocol/MvGpio.h | 199 
>  2 files changed, 200 insertions(+)
>  create mode 100644 Silicon/Marvell/Include/Protocol/MvGpio.h
> 
> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> index 616624e..7e1c37a 100644
> --- a/Silicon/Marvell/Marvell.dec
> +++ b/Silicon/Marvell/Marvell.dec
> @@ -215,6 +215,7 @@
>  [Protocols]
>gMarvellBoardDescProtocolGuid= { 0xebed8738, 0xd4a6, 0x4001, { 
> 0xa9, 0xc9, 0x52, 0xb0, 0xcb, 0x7d, 0xdb, 0xf9 }}
>gMarvellEepromProtocolGuid   = { 0x71954bda, 0x60d3, 0x4ef8, { 
> 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }}
> +  gMarvellGpioProtocolGuid = { 0x8b01a5b7, 0xc570, 0x4e97, { 
> 0x80, 0x95, 0x4f, 0x74, 0x2a, 0x8d, 0x7d, 0x43 }}
>gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 
> 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }}
>gMarvellPhyProtocolGuid  = { 0x32f48a43, 0x37e3, 0x4acf, { 
> 0x93, 0xc4, 0x3e, 0x57, 0xa7, 0xb0, 0xfb, 0xdc }}
>gMarvellSpiMasterProtocolGuid= { 0x23de66a3, 0xf666, 0x4b3e, { 
> 0xaa, 0xa2, 0x68, 0x9b, 0x18, 0xae, 0x2e, 0x19 }}
> diff --git a/Silicon/Marvell/Include/Protocol/MvGpio.h 
> b/Silicon/Marvell/Include/Protocol/MvGpio.h
> new file mode 100644
> index 000..a335cab
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Protocol/MvGpio.h
> @@ -0,0 +1,199 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +#ifndef __MARVELL_GPIO_PROTOCOL_H__
> +#define __MARVELL_GPIO_PROTOCOL_H__
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +extern EFI_GUID gMarvellGpioProtocolGuid;
> +
> +typedef struct _MARVELL_GPIO_PROTOCOL MARVELL_GPIO_PROTOCOL;
> +
> +typedef enum {
> +  GPIO_MODE_INPUT = 0x0,
> +  GPIO_MODE_OUTPUT= 0x1
> +} MARVELL_GPIO_MODE;

GPIO_ is a bit too specific a prefix (even if only used in
Marvell-specific code, it makes it look not Marvell-specific).
MVGPIO?

> +
> +typedef enum {
> +  GPIO_DRIVER_TYPE_SOC_CONTROLLER = 0x0,
> +  GPIO_DRIVER_TYPE_PCA95XX= 0x1
> +} MARVELL_GPIO_DRIVER_TYPE;

Here is sort of what I was getting at before. If you ever end up
adding another type to this enum (say on a derivative board), that
means the hard (semantic) dependence on I2c for the expander will make
things really confusing. And may require substantial code refactoring
at that point.

> +
> +typedef enum {
> +  PCA9505_ID,

Are these referring to software-addressable hardware IDs, or just used
for software-internal references?

> +  PCA9534_ID,
> +  PCA9535_ID,
> +  PCA9536_ID,
> +  PCA9537_ID,
> +  PCA9538_ID,
> +  PCA9539_ID,
> +  PCA9554_ID,
> +  PCA9555_ID,
> +  PCA9556_ID,
> +  PCA9557_ID,
> +  PCA95XX_MAX_ID,
> +} MARVELL_IO_EXPANDER_TYPE_PCA95XX;

MARVELL_PCA95XX_VARIANT? If you're checking for which version of it
you're dealing with, you already know what it is.

Either way, it would be easier to understand its use if it was
introduced with the patch that adds code that makes use of it.

> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *MV_GPIO_DIRECTION_OUTPUT) (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  IN  BOOLEAN Value
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *MV_GPIO_DIRECTION_INPUT) (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *MV_GPIO_GET_FUNCTION) (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  OUT MARVELL_GPIO_MODE *Mode
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *MV_GPIO_GET_VALUE) (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  OUT BOOLEAN *Value
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI 

Re: [edk2] [platforms: PATCH 06/12] Marvell/Drivers: MvBoardDesc: Extend protocol with GPIO support

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:35AM +0200, Marcin Wojtas wrote:
> Introduce new callback that can provide information
> about GPIO SoC controllers, as well as on-board
> I2C IO expanders. According ArmadaSoCDescLib
> ArmadaBoardDescLib routines are used for
> obtaining required data.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf |  1 +
>  Silicon/Marvell/Include/Protocol/BoardDesc.h |  8 
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c   | 48 
> 
>  3 files changed, 57 insertions(+)
> 
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf 
> b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> index 41f72d6..0b93948 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> @@ -47,6 +47,7 @@
>Silicon/Marvell/Marvell.dec
>  
>  [LibraryClasses]
> +  ArmadaBoardDescLib
>ArmadaSoCDescLib
>DebugLib
>MemoryAllocationLib
> diff --git a/Silicon/Marvell/Include/Protocol/BoardDesc.h 
> b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> index 1d57a16..e06d689 100644
> --- a/Silicon/Marvell/Include/Protocol/BoardDesc.h
> +++ b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> @@ -50,6 +50,13 @@ EFI_STATUS
>  
>  typedef
>  EFI_STATUS
> +(EFIAPI *MV_BOARD_DESC_GPIO_GET) (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_GPIO_DESC  **GpioDesc
> +  );
> +
> +typedef
> +EFI_STATUS
>  (EFIAPI *MV_BOARD_DESC_I2C_GET) (
>IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>IN OUT MV_BOARD_I2C_DESC   **I2cDesc
> @@ -106,6 +113,7 @@ VOID
>  struct _MARVELL_BOARD_DESC_PROTOCOL {
>MV_BOARD_DESC_AHCI_GET BoardDescAhciGet;
>MV_BOARD_DESC_COMPHY_GET   BoardDescComPhyGet;
> +  MV_BOARD_DESC_GPIO_GET BoardDescGpioGet;
>MV_BOARD_DESC_I2C_GET  BoardDescI2cGet;
>MV_BOARD_DESC_MDIO_GET BoardDescMdioGet;
>MV_BOARD_DESC_PP2_GET  BoardDescPp2Get;
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c 
> b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> index 39dc06c..948a6a0 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> @@ -100,6 +100,53 @@ MvBoardDescComPhyGet (
>  
>  STATIC
>  EFI_STATUS
> +MvBoardDescGpioGet (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_GPIO_DESC  **GpioDesc
> +  )
> +{
> +  MV_I2C_IO_EXPANDER_DESC *I2cIoExpanderDesc;
> +  UINTN SoCGpioCount, I2cIoExpanderCount;
> +  STATIC MV_BOARD_GPIO_DESC *BoardDesc;
> +  MV_SOC_GPIO_DESC *SoCDesc;
> +  EFI_STATUS Status;
> +
> +  if (BoardDesc != NULL) {
> +*GpioDesc = BoardDesc;
> +return EFI_SUCCESS;
> +  }

For a completely opposite comment to the previous patch:
I would much prefer for BoardDesc to be a globally visible variable
with a g prefix and proper namespace.

Even though it itself is not globally visible, the above maneuver
makes it effectively so, but not semantically.
It's not like there's a million call sites, so I would prefer if the
check was there rather than in this function.

> +
> +  /* Get SoC data about all available GPIO controllers */
> +  Status = ArmadaSoCDescGpioGet (, );
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  /* Get per-board information about all available I2C IO expanders */
> +  Status = ArmadaBoardDescGpioGet (, );

Same I2C->GPIO comments apply here.

> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  /* Allocate and fill board description */
> +  BoardDesc = AllocateZeroPool (sizeof (MV_BOARD_GPIO_DESC));
> +  if (BoardDesc == NULL) {
> +DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  BoardDesc->SoC = SoCDesc;
> +  BoardDesc->GpioDevCount = SoCGpioCount;
> +  BoardDesc->I2cIoExpanderDesc = I2cIoExpanderDesc;
> +  BoardDesc->I2cIoExpanderCount = I2cIoExpanderCount;
> +
> +  *GpioDesc = BoardDesc;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
>  MvBoardDescI2cGet (
>IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>IN OUT MV_BOARD_I2C_DESC   **I2cDesc
> @@ -556,6 +603,7 @@ MvBoardDescInitProtocol (
>  {
>BoardDescProtocol->BoardDescAhciGet = MvBoardDescAhciGet;
>BoardDescProtocol->BoardDescComPhyGet = MvBoardDescComPhyGet;
> +  BoardDescProtocol->BoardDescGpioGet = MvBoardDescGpioGet;
>BoardDescProtocol->BoardDescI2cGet = MvBoardDescI2cGet;

Even more so because of the two lines above :)

/
Leif

>BoardDescProtocol->BoardDescMdioGet = MvBoardDescMdioGet;
>BoardDescProtocol->BoardDescPp2Get = MvBoardDescPp2Get;
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 05/12] Marvell/Armada80x0Db: Introduce board description library

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:34AM +0200, Marcin Wojtas wrote:
> This patch implements ArmadaBoarDescLib library for
> Armada80x0Db and introduces ArmadaBoardDescGpioGet
> routine with per-board I2C IO expander description.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc   
>|  3 ++
>  
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  | 34 +
>  
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>| 50 
>  3 files changed, 87 insertions(+)
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  create mode 100644 
> Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> 
> diff --git a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc 
> b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> index 92e2dc8..42f7bd3 100644
> --- a/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> +++ b/Platform/Marvell/Armada80x0Db/Armada80x0Db.dsc
> @@ -54,6 +54,9 @@
>  [Components.AARCH64]
>Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0Db.inf
>  
> +[LibraryClasses.common]
> +  
> ArmadaBoardDescLib|Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> +
>  
> 
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> diff --git 
> a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
>  
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> new file mode 100644
> index 000..2d39d96
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = Armada80x0DbBoardDescLib
> +  FILE_GUID  = fee9e874-1481-4b4f-9882-966bd0d1310f
> +  MODULE_TYPE= BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = ArmadaBoardDescLib
> +
> +[Sources]
> +  Armada80x0DbBoardDescLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> diff --git 
> a/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
>  
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> new file mode 100644
> index 000..16561bc
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada80x0Db/Armada80x0DbBoardDescLib/Armada80x0DbBoardDescLib.c
> @@ -0,0 +1,50 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +STATIC MV_I2C_IO_EXPANDER_DESC mI2cIoExpanderDescTemplate[] = {

No objection to the above (apart from the I2c bit, see previous
patches), but: this is a STATIC array - it doesn't need to worry about
namespace.

And it isn't really a template - it is the actual description.
So ... you could just call it mGpioExpanders or something.

Same I2c -> GPIO comment as for preceding patches.

> +  {
> +PCA9555_ID,
> +0x21,
> +0x0,
> +  },
> +  {
> +PCA9555_ID,
> +0x25,
> +0x0,
> +  },
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaBoardDescGpioGet (
> +  IN OUT MV_I2C_IO_EXPANDER_DESC **I2cIoExpanderDesc,
> +  IN OUT UINTN*I2cIoExpanderCount
> +  )
> +{
> +  *I2cIoExpanderCount = ARRAY_SIZE (mI2cIoExpanderDescTemplate);
> +  *I2cIoExpanderDesc = mI2cIoExpanderDescTemplate;
> +
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
> 

Re: [edk2] [platforms: PATCH 04/12] Marvell/Armada70x0Db: Introduce board description library

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:33AM +0200, Marcin Wojtas wrote:
> This patch implements ArmadaBoarDescLib library for
> Armada70x0Db and introduces ArmadaBoardDescGpioGet
> routine with per-board I2C IO expander description.

Same commit message comment as 3/12.

This one actually has I2c, but still - please rename all of the types
and variables. (Feel free to add a comment saying the expander is on
I2c, but that is not the relevant bit code-wise.)

/
Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc   
>|  3 ++
>  
> Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
>  | 34 
>  
> Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.c
>| 43 
>  3 files changed, 80 insertions(+)
>  create mode 100644 
> Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
>  create mode 100644 
> Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.c
> 
> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc 
> b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> index e0bf447..a935f36 100644
> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> @@ -54,6 +54,9 @@
>  [Components.AARCH64]
>Silicon/Marvell/Armada7k8k/AcpiTables/Armada70x0Db.inf
>  
> +[LibraryClasses.common]
> +  
> ArmadaBoardDescLib|Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
> +
>  
> 
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> diff --git 
> a/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
>  
> b/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
> new file mode 100644
> index 000..b26f55b
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = Armada70x0DbBoardDescLib
> +  FILE_GUID  = 3164c8d9-19d4-4ad6-8196-cea094b1ddf1
> +  MODULE_TYPE= BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = ArmadaBoardDescLib
> +
> +[Sources]
> +  Armada70x0DbBoardDescLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> diff --git 
> a/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.c
>  
> b/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.c
> new file mode 100644
> index 000..51e6687
> --- /dev/null
> +++ 
> b/Platform/Marvell/Armada70x0Db/Armada70x0DbBoardDescLib/Armada70x0DbBoardDescLib.c
> @@ -0,0 +1,43 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +STATIC MV_I2C_IO_EXPANDER_DESC mI2cIoExpanderDescTemplate = {
> +  PCA9555_ID,
> +  0x21,
> +  0x0,
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaBoardDescGpioGet (
> +  IN OUT MV_I2C_IO_EXPANDER_DESC **I2cIoExpanderDesc,
> +  IN OUT UINTN*I2cIoExpanderCount
> +  )
> +{
> +  *I2cIoExpanderCount = 1;
> +  *I2cIoExpanderDesc = 
> +
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 03/12] SolidRun/Armada80x0McBin: Introduce board description library

[Leif Lindholm]

On Sat, Oct 20, 2018 at 03:57:32AM +0200, Marcin Wojtas wrote:
> This patch implements ArmadaBoarDescLib library for
> Armada80x0McBin comunity board and introduces ArmadaBoardDescGpioGet
> routine with per-board I2C IO expander description.

I would argue it "adds an implementation" rather than "introduces"
(2/12 did that). Also, since there _is_ no expansion here, the
referring to it as I2C gets ever weirder.

No issues beyond such nitpicking: if you address the commit message
and purge all mentions of I2C from this patch:
Reviewed-by: Leif Lindholm 

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas 
> ---
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
>  |  3 ++
>  
> Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
>  | 34 ++
>  
> Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
>| 36 
>  3 files changed, 73 insertions(+)
>  create mode 100644 
> Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
>  create mode 100644 
> Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
> 
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc 
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> index 52e2b9b..077224d 100644
> --- a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> @@ -55,6 +55,9 @@
>  [Components.AARCH64]
>Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin.inf
>  
> +[LibraryClasses.common]
> +  
> ArmadaBoardDescLib|Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> +
>  
> 
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> diff --git 
> a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
>  
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> new file mode 100644
> index 000..63a4f66
> --- /dev/null
> +++ 
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates
> +#
> +#  This program and the accompanying materials are licensed and made 
> available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION= 0x0001001A
> +  BASE_NAME  = ArmadaMcBinBoardDescLib
> +  FILE_GUID  = 8208558f-5f33-46e2-b5c5-43354384389e
> +  MODULE_TYPE= BASE
> +  VERSION_STRING = 1.0
> +  LIBRARY_CLASS  = ArmadaBoardDescLib
> +
> +[Sources]
> +  Armada80x0McBinBoardDescLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> diff --git 
> a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
>  
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
> new file mode 100644
> index 000..979db11
> --- /dev/null
> +++ 
> b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
> @@ -0,0 +1,36 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaBoardDescGpioGet (
> +  IN OUT MV_I2C_IO_EXPANDER_DESC **I2cIoExpanderDesc,
> +  IN OUT UINTN*I2cIoExpanderCount
> +  )
> +{
> +  /* No I2C IO expanders on board */
> +  *I2cIoExpanderDesc = NULL;
> +  *I2cIoExpanderCount = 0;
> +
> +  return EFI_SUCCESS;
> +}
> -- 
> 2.7.4
> 
___
edk2-devel mailing list

Re: [edk2] [platforms: PATCH 02/12] Marvell/Library: ArmadaBoardDescLib: Add GPIO information

[Leif Lindholm]

On Wed, Nov 14, 2018 at 01:12:22AM +, Leif Lindholm wrote:
> On Sat, Oct 20, 2018 at 03:57:31AM +0200, Marcin Wojtas wrote:
> > This patch extends library with GPIO devices per-board
> > description. Both embedded SoC controllers and
> > I2C IO expanders are supported. Add a helper routine
> > for obtaining information about the latter.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas 
> > ---
> >  Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h | 23 
> > 
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h 
> > b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > index ee8e06e..109164c 100644
> > --- a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > +++ b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > @@ -25,6 +25,29 @@ typedef struct {
> >  } MV_BOARD_COMPHY_DESC;
> >  
> >  //
> > +// GPIO devices per-board description
> > +//
> > +typedef struct {
> > +  UINTN ChipId;
> > +  UINTN I2cAddress;
> > +  UINTN I2cBus;
> > +} MV_I2C_IO_EXPANDER_DESC;
> > +
> > +typedef struct {
> > +  MV_SOC_GPIO_DESC*SoC;
> > +  UINTNGpioDevCount;
> > +  MV_I2C_IO_EXPANDER_DESC *I2cIoExpanderDesc;
> > +  UINTNI2cIoExpanderCount;
> > +} MV_BOARD_GPIO_DESC;
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +ArmadaBoardDescGpioGet (
> > +  IN OUT MV_I2C_IO_EXPANDER_DESC **I2cIoExpanderDesc,
> > +  IN OUT UINTN*I2cIoExpanderCount
> 
> Please expand all DESC/Desc to descriptor/description/whatever as
> appropriate.
> With that, Reviewed-by: Leif Lindholm 

Actually, I spotted something in 3-4/12 on finally getting back to this
set. The function is called ArmadaBoardDescGpioGet, but the IN/OUT
parameters talk abour I2c.

This seems like either a lack of abstraction (which given this is a
generic library class seems suboptimal).

Could that too be addressed in v2? (I'll mention it here instead of in
the users.)

Regards,

Leif

> > +  );
> > +
> > +//
> >  // I2C devices per-board description
> >  //
> >  typedef struct {
> > -- 
> > 2.7.4
> > 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 1/2] Platform/ARM/Sgi: fix incorrect check of config-id value

[Ard Biesheuvel]

On Tue, 4 Dec 2018 at 12:36, Chandni Cherukuri
 wrote:
>
> On SGI platform, the value of configuration ID can be zero.
> So avoid returning an error from the function that creates
> the system ID HOB in case the value of the configuration ID
> is zero.
>
> While at it, improve some of the error messages as well.
>

So the platform ID is still guaranteed to be non-zero?


> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c 
> b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> index 15ea571..065b23d 100644
> --- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> @@ -67,7 +67,7 @@ GetSgiSystemId (
>
>Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
>if (Property == NULL) {
> -DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
> +DEBUG ((DEBUG_ERROR, "platform-id property not found\n"));
>  return EFI_INVALID_PARAMETER;
>}
>
> @@ -75,7 +75,7 @@ GetSgiSystemId (
>
>Property = fdt_getprop (HwCfgDtBlob, Offset, "config-id", NULL);
>if (Property == NULL) {
> -DEBUG ((DEBUG_ERROR, "Config Id is NULL\n"));
> +DEBUG ((DEBUG_ERROR, "config-id property not found\n"));
>  return EFI_INVALID_PARAMETER;
>}
>
> @@ -121,7 +121,7 @@ SgiPlatformPeim (
>  return EFI_INVALID_PARAMETER;
>}
>
> -  if (HobData->PlatformId == 0 || HobData->ConfigId == 0) {
> +  if (HobData->PlatformId == 0) {
>  ASSERT (FALSE);
>  return EFI_INVALID_PARAMETER;
>}
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EmbeddedPkg : Corrected flow for setting Buswidth for eMMC

[Ard Biesheuvel]

Please explain

- what was wrong and why
- how this change fixes it

and put it in the commit log.

Thanks,


On Tue, 4 Dec 2018 at 05:42, Meenakshi Aggarwal
 wrote:
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal 
> ---
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c 
> b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index f661a0c..9118eb2 100755
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -309,11 +309,14 @@ InitializeEmmcDevice (
>}
>Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, BusMode);
>if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD 
> bus width, Status:%r\n", Status));
> +continue;
> +  } else {
> +DEBUG ((DEBUG_INFO, "InitializeEmmcDevice(): Set EXTCSD bus width %d 
> successfully\n", BusMode));
> +break;
>}
> -  return Status;
>  }
>}
> +
>return Status;
>  }
>
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v1 edk2-platforms 1/8] Platform/ARM/SgiPkg: Restructure virtio device registration

[Ard Biesheuvel]

On Tue, 4 Dec 2018 at 10:12, Vijayenthiran Subramaniam
 wrote:
>
> From: Daniil Egranov 
>
> SGI platforms support multiple virtio devices. So the existing code, that
> supports registration of only the virtio disk, is restructured to
> accommodate the registration of additional virtio devices.
>
> In addition to this, PCDs to represent the virtio controller base and
> address space size are introduced.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Daniil Egranov 
> ---
>  Platform/ARM/SgiPkg/SgiPlatform.dec  
> |  8 ++-
>  Platform/ARM/SgiPkg/SgiPlatform.dsc  
> |  7 +-
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf  
> |  8 ++-
>  Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h   
> | 21 ++
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> | 14 +---
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/{VirtioBlockIo.c => VirtioDevices.c} 
> | 67 
>  6 files changed, 81 insertions(+), 44 deletions(-)
>

Can these platforms only ever expose a single block device and a
single network device?

> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
> b/Platform/ARM/SgiPkg/SgiPlatform.dec
> index f6e0ba1e927a..ed29a4d5d91f 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> @@ -37,12 +37,16 @@ [Guids.common]
>gSgiClarkHeliosAcpiTablesFileGuid = { 0x2af40815, 0xa84e, 0x4de9, { 0x8c, 
> 0x38, 0x91, 0x40, 0xb3, 0x54, 0x40, 0x73 } }
>
>  [PcdsFeatureFlag.common]
> -  # Set this PCD to TRUE to enable virtio support.
> -  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x0001
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|FALSE|BOOLEAN|0x0001
>
>  [PcdsFixedAtBuild]
>gArmSgiTokenSpaceGuid.PcdDramBlock2Base|0|UINT64|0x0002
>gArmSgiTokenSpaceGuid.PcdDramBlock2Size|0|UINT64|0x0003
>
> +  # Virtio Block device
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x|UINT32|0x0004
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x|UINT32|0x0005
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|0x|UINT32|0x0006
> +
>  [Ppis]
>gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 
> 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
> b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index b3f76d2d9720..ada72be72f8a 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -98,7 +98,7 @@ [LibraryClasses.common.UEFI_DRIVER, 
> LibraryClasses.common.UEFI_APPLICATION, Libr
>  
> 
>
>  [PcdsFeatureFlag.common]
> -  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|TRUE
>
>  [PcdsFixedAtBuild.common]
>gArmTokenSpaceGuid.PcdVFPEnabled|1
> @@ -180,6 +180,11 @@ [PcdsFixedAtBuild.common]
># Ethernet
>gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
>
> +  # Virtio Disk
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x1c13
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x1
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|202
> +
>  
> 
>  #
>  # Components Section - list of all EDK II Modules needed by this Platform
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index d903ed8d3375..f920f6ecafb8 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -20,7 +20,7 @@ [Defines]
>
>  [Sources.common]
>PlatformDxe.c
> -  VirtioBlockIo.c
> +  VirtioDevices.c
>
>  [Packages]
>EmbeddedPkg/EmbeddedPkg.dec
> @@ -41,7 +41,11 @@ [Guids]
>gSgiClarkHeliosAcpiTablesFileGuid
>
>  [FeaturePcd]
> -  gArmSgiTokenSpaceGuid.PcdVirtioSupported
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported
> +
> +[FixedPcd]
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
> +  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
>
>  [Depex]
>TRUE
> diff --git a/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h 
> b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
> new file mode 100644
> index ..80d3e3ae4f91
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
> @@ -0,0 +1,21 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE 

Re: [edk2] Creating my own flashing app

[Richardson, Brian]

Guy:

I would follow Andrew's advice and investigate the use of Secure Capsule to 
implement SPI Flash Update. This is becoming the standard, and has a number of 
advantages (signed, OS independent, platform independent, compatible with 
fwupd.org & Windows Update workflow).

General information is on the TianoCore wiki:
https://github.com/tianocore/tianocore.github.io/wiki/Capsule-Based-Firmware-Update-and-Firmware-Recovery
 

There is a capsule-based update app in EDK II:
https://github.com/tianocore/edk2/tree/master/MdeModulePkg/Application/CapsuleApp

Thanks ... br
---
Brian Richardson -- Director, Firmware Ecosystem Development
brian.richard...@intel.com -- @intel_brian (Twitter & WeChat)
https://software.intel.com/en-us/meet-the-developers/evangelists/team/brian-richardson
 

-Original Message-
From: edk2-devel  On Behalf Of Andrew Fish
Sent: Tuesday, December 4, 2018 1:35 AM
To: Guy Raviv 
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] Creating my own flashing app

On a secure platform you likely need to update using a secure capsule. 
https://github.com/tianocore/tianocore.github.io/wiki/Capsule-Based-Firmware-Update-and-Firmware-Recovery
 
The capsule is the standard method, and then all the FLASH update code is part 
of the ROM.

Generally since an EFI platform has NVRAM services in the NOR FLASH there is an 
SPI driver to write to FLASH.

So if your platform does not secure FLASH you can use the services from the ROM.

Sent from my iPhone

> On Dec 3, 2018, at 8:45 PM, Guy Raviv  wrote:
> 
>   a whole SPI BIOS image.
> if i was not clear please tell me what i'm missing.
> 
> Thanks!
> Guy
> 
>   Virus-free. www.avg.com
> 
>> On Tue, Dec 4, 2018 at 6:42 AM Andrew Fish  wrote:
>> Guy,
>> 
>> What are you trying to FLASH?
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>> > On Dec 3, 2018, at 7:28 PM, Guy Raviv  wrote:
>> > 
>> > Hi,
>> > 
>> > I want to create my own flashing utility.
>> > Is there any EDKII App/Utilities that can help me?
>> > 
>> > Thanks,
>> > Guy
>> > 
>> > 
>> > Virus-free.
>> > www.avg.com
>> > 
>> > <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
>> > ___
>> > edk2-devel mailing list
>> > edk2-devel@lists.01.org
>> > https://lists.01.org/mailman/listinfo/edk2-devel
>> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and EPIT timer headers

[Leif Lindholm]

On Tue, Dec 04, 2018 at 02:06:04AM +, Chris Co wrote:
> > > diff --git a/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> > > b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> > > new file mode 100644
> > > index ..485d6ccbc51e
> > > --- /dev/null
> > > +++ b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> > 
> > Rename to CamelCase?
> 
> Can do. Would you prefer the iMX prefix to be in CamelCase too? (i.e. 
> Imx6Epit.h)

I'm honestly not too fussed.

My preference (which contradicts the coding style, mind you) is to
keep names looking like they do in normal reading. So I would
aesthetically prefer the former, whereas the coding style says the
latter. You choose :)

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

Re: [edk2] [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use

[Leif Lindholm]

On Tue, Dec 04, 2018 at 10:33:23AM +0100, Ard Biesheuvel wrote:
> > +#pragma pack(push, 1)
>  
>  I don't see this #pragma making any difference to the structs below,
>  can it be dropped?
> >>> 
> >>> The pragma pack is defensive. Without it, we rely on the compiler
> >>> packing structures by default and this may not happen on 64 bit
> >>> compiles.
> >> 
> >> I understand that is what the pragma does. My comment was because all of 
> >> the
> >> variables in the below structs are naturally aligned.
> >> The reason I dislike its use when effectively a no-op, is that it makes it 
> >> look like it
> >> it isn't a no-op.
> >> 
> >> If it covers a larger set of structs, some of which require the packed 
> >> attribute I'm
> >> OK with that. But I'm not a fan of adding it "just in case" without 
> >> contemplating
> >> the statement's (lack of) effect.
> >> 
> >> Regards,
> >> 
> >> Leif
> >> 
> > 
> > Makes sense. I am checking to make sure this pragma wasn't
> > included due to some observed compiler behavior on our end, since
> > this header is also used on our ARM64 work.
> > Will remove it once confirmed it is safe.
> 
> I’d prefer to keep the pragma. It doesn’t only remove padding, it
> also informs the compiler that the whole struct may appear at any
> unaligned offset.
> On 32 bit ARM, this means the compiler will
> refrain from using load/store double or load/store multiple to
> access the contents when dereferencing a pointer to such a struct,
> which would result in a crash otherwise.

OK, if it is a real concern that the structs themselves may appear
unaligned in memory, please ignore this feedback.

/
Leif


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

[edk2] [PATCH edk2-platforms 2/2] Platform/ARM/Sgi: Use NT_FW_CONFIG instead of HW_CONFIG

[Chandni Cherukuri]

The hardware configuration in HW_CONFIG dts is not being
passed onto the operating system but used and terminated
at edk2 boot stage (BL33). So, as per the recommendations
of the trusted-firmware design, the hardware description
specified in NT_FW_CONFIG dts should be used instead of
HW_CONFIG dts.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec   |  2 +-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |  2 +-
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  4 +--
 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  6 ++---
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 10 +++
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 28 
++--
 Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  6 ++---
 7 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index f6e0ba1..9166052 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -45,4 +45,4 @@
   gArmSgiTokenSpaceGuid.PcdDramBlock2Size|0|UINT64|0x0003
 
 [Ppis]
-  gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 
0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
+  gNtFwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 
0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
index 93377fa..260be58 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
@@ -71,4 +71,4 @@
 
 [Ppis]
   gArmMpCoreInfoPpiGuid
-  gHwConfigDtInfoPpiGuid
+  gNtFwConfigDtInfoPpiGuid
diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf 
b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
index a7c718b..a9173cc 100644
--- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
@@ -34,7 +34,7 @@
   gArmSgiPlatformIdDescriptorGuid
 
 [Ppis]
-  gHwConfigDtInfoPpiGuid
+  gNtFwConfigDtInfoPpiGuid
 
 [Depex]
-  gHwConfigDtInfoPpiGuid
+  gNtFwConfigDtInfoPpiGuid
diff --git a/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h 
b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
index 934eef2..b830326 100644
--- a/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
+++ b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
@@ -15,9 +15,9 @@
 #ifndef  SGI_PLATFORMID_PPI_
 #define  SGI_PLATFORMID_PPI_
 
-// HwConfig DT structure
+// NT_FW_CONFIG DT structure
 typedef struct {
-  UINT64  HwConfigDtAddr;
-} SGI_HW_CONFIG_INFO_PPI;
+  UINT64  NtFwConfigDtAddr;
+} SGI_NT_FW_CONFIG_INFO_PPI;
 
 #endif
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
index 13bb423..d264292 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
@@ -17,8 +17,8 @@
 #include 
 #include 
 
-UINT64 HwConfigDtBlob;
-STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
+UINT64 NtFwConfigDtBlob;
+STATIC SGI_NT_FW_CONFIG_INFO_PPI mNtFwConfigDtInfoPpi;
 
 STATIC ARM_CORE_INFO mCoreInfoTable[] = {
   {
@@ -40,7 +40,7 @@ ArmPlatformInitialize (
   IN  UINTN MpId
   )
 {
-  mHwConfigDtInfoPpi.HwConfigDtAddr = HwConfigDtBlob;
+  mNtFwConfigDtInfoPpi.NtFwConfigDtAddr = NtFwConfigDtBlob;
   return RETURN_SUCCESS;
 }
 
@@ -67,8 +67,8 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
   },
   {
 EFI_PEI_PPI_DESCRIPTOR_PPI,
-,
-
+,
+
   }
 };
 
diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c 
b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
index 065b23d..d133921 100644
--- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
@@ -24,9 +24,9 @@
 
   This function returns the System ID of the platform
 
-  This functions locates the HwConfig PPI and gets the base address of HW 
CONFIG
-  DT from which the platform ID and config ID is obtained using FDT helper
-  functions
+  This functions locates the NtFwConfig PPI and gets the base address of
+  NT_FW_CONFIG DT from which the platform ID and config ID is obtained
+  using FDT helper functions
 
   @param[out]  Pointer to the SGI_PLATFORM_DESCRIPTOR HoB
   @return  returns EFI_SUCCESS on success and EFI_INVALID_PARAMETER on 
error
@@ -41,31 +41,31 @@ GetSgiSystemId (
 {
   CONST UINT32  *Property;
   INT32 Offset;
-  CONST VOID*HwCfgDtBlob;
-  SGI_HW_CONFIG_INFO_PPI*HwConfigInfoPpi;
+  CONST VOID  

[edk2] [PATCH edk2-platforms 1/2] Platform/ARM/Sgi: fix incorrect check of config-id value

[Chandni Cherukuri]

On SGI platform, the value of configuration ID can be zero.
So avoid returning an error from the function that creates
the system ID HOB in case the value of the configuration ID
is zero.

While at it, improve some of the error messages as well.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri 
---
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c 
b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
index 15ea571..065b23d 100644
--- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
+++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
@@ -67,7 +67,7 @@ GetSgiSystemId (
 
   Property = fdt_getprop (HwCfgDtBlob, Offset, "platform-id", NULL);
   if (Property == NULL) {
-DEBUG ((DEBUG_ERROR, "Platform Id is NULL\n"));
+DEBUG ((DEBUG_ERROR, "platform-id property not found\n"));
 return EFI_INVALID_PARAMETER;
   }
 
@@ -75,7 +75,7 @@ GetSgiSystemId (
 
   Property = fdt_getprop (HwCfgDtBlob, Offset, "config-id", NULL);
   if (Property == NULL) {
-DEBUG ((DEBUG_ERROR, "Config Id is NULL\n"));
+DEBUG ((DEBUG_ERROR, "config-id property not found\n"));
 return EFI_INVALID_PARAMETER;
   }
 
@@ -121,7 +121,7 @@ SgiPlatformPeim (
 return EFI_INVALID_PARAMETER;
   }
 
-  if (HobData->PlatformId == 0 || HobData->ConfigId == 0) {
+  if (HobData->PlatformId == 0) {
 ASSERT (FALSE);
 return EFI_INVALID_PARAMETER;
   }
-- 
2.7.4

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

[edk2] [PATCH edk2-platforms 0/2] Use NT_FW_CONFIG instead of HW_CONFIG

[Chandni Cherukuri]

The first patch removes an incorrect check on the configuration id
register value and does minor changes to the console error messages.

The second patch in this series adds usage of NT_FW_CONFIG for SGI
platforms. The hardware configuration (system-id) in HW_CONFIG dts
is not being passed onto the operating system. As per the
recommendations of the trusted-firmware design, since the hardware
description is being used only in edk2 boot stage (BL33), the
non-trusted firmware config device tree (NT_FW_CONFIG) can be used
instead of HW_CONFIG device tree to specify the hardware description.

Chandni Cherukuri (2): 
  Platform/ARM/Sgi: fix incorrect check of config-id value
  Platform/ARM/Sgi: Use NT_FW_CONFIG instead of HW_CONFIG

 Platform/ARM/SgiPkg/SgiPlatform.dec   |  2 +-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |  2 +-
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  4 +--
 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  6 ++--
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 10 +++---
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 34 
++--
 Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  6 ++--
 7 files changed, 32 insertions(+), 32 deletions(-)

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

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Chiu, Chasel
> Sent: Tuesday, December 4, 2018 5:19 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Desimone, Nathaniel L
> ; Zeng, Star ; Chiu,
> Chasel 
> Subject: [PATCH] Maintainers.txt: Change package maintainer of
> IntelFsp*Pkg
> 
> Cc: Jiewen Yao 
> Cc: Desimone Nathaniel L 
> Cc: Zeng Star 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu 
> ---
>  Maintainers.txt | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 9a36f0232b..1ed4166d63 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -147,21 +147,27 @@ M: Liming Gao 
> 
>  IntelFsp2Pkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
> -M: Jiewen Yao 
>  M: Chasel Chiu 
> +R: Desimone Nathaniel L 
> +R: Zeng Star 
> 
>  IntelFsp2WrapperPkg
>  W:
> https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPk
> g
> -M: Jiewen Yao 
>  M: Chasel Chiu 
> +R: Desimone Nathaniel L 
> +R: Zeng Star 
> 
>  IntelFspPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
> -M: Jiewen Yao 
> +M: Chasel Chiu 
> +R: Desimone Nathaniel L 
> +R: Zeng Star 
> 
>  IntelFspWrapperPkg
>  W:
> https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
> -M: Jiewen Yao 
> +M: Chasel Chiu 
> +R: Desimone Nathaniel L 
> +R: Zeng Star 
> 
>  IntelSiliconPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
> --
> 2.13.3.windows.1

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

[edk2] [PATCH] Maintainers.txt: Change package maintainer of IntelFsp*Pkg

[Chasel, Chiu]

Cc: Jiewen Yao 
Cc: Desimone Nathaniel L 
Cc: Zeng Star 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu 
---
 Maintainers.txt | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt
index 9a36f0232b..1ed4166d63 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -147,21 +147,27 @@ M: Liming Gao 
 
 IntelFsp2Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFsp2WrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
-M: Jiewen Yao 
 M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFspPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelFspWrapperPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspWrapperPkg
-M: Jiewen Yao 
+M: Chasel Chiu 
+R: Desimone Nathaniel L 
+R: Zeng Star 
 
 IntelSiliconPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
-- 
2.13.3.windows.1

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

Re: [edk2] [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use

[Ard Biesheuvel]

> On 4 Dec 2018, at 02:44, Chris Co  wrote:
> 
> 
> 
>> -Original Message-
>> From: Leif Lindholm 
>> Sent: Monday, December 3, 2018 1:43 AM
>> To: Chris Co 
>> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
>> Michael D Kinney 
>> Subject: Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-
>> specific i.MX packages to use
>> 
>> On Sat, Dec 01, 2018 at 12:22:17AM +, Chris Co wrote:
 If using EFI_ACPI prefix, these #defines really should be in edk2
 MdePkg. And CSRT itself is, so that might not be a bad idea.
 
> +
> +#pragma pack(push, 1)
 
 I don't see this #pragma making any difference to the structs below,
 can it be dropped?
>>> 
>>> The pragma pack is defensive. Without it, we rely on the compiler
>>> packing structures by default and this may not happen on 64 bit
>>> compiles.
>> 
>> I understand that is what the pragma does. My comment was because all of the
>> variables in the below structs are naturally aligned.
>> The reason I dislike its use when effectively a no-op, is that it makes it 
>> look like it
>> it isn't a no-op.
>> 
>> If it covers a larger set of structs, some of which require the packed 
>> attribute I'm
>> OK with that. But I'm not a fan of adding it "just in case" without 
>> contemplating
>> the statement's (lack of) effect.
>> 
>> Regards,
>> 
>> Leif
>> 
> 
> Makes sense. I am checking to make sure this pragma wasn't included due to 
> some observed compiler behavior on our end, since this header is also used on 
> our ARM64 work.
> Will remove it once confirmed it is safe.


I’d prefer to keep the pragma. It doesn’t only remove padding, it also informs 
the compiler that the whole struct may appear at any unaligned offset. On 32 
bit ARM, this means the compiler will refrain from using load/store double or 
load/store multiple to access the contents when dereferencing a pointer to such 
a struct, which would result in a crash otherwise.

> 
>>> I have addressed the remaining feedback and will resubmit with v2.
>>> 
>>> Thanks,
>>> Chris
>>> 
> +//---
> +
> +- // CSRT Resource Group header 24 bytes long
> +//---
> +
> +-
> +typedef struct {
> +  UINT32 Length;
> +  UINT32 VendorID;
> +  UINT32 SubVendorId;
> +  UINT16 DeviceId;
> +  UINT16 SubdeviceId;
> +  UINT16 Revision;
> +  UINT16 Reserved;
> +  UINT32 SharedInfoLength;
> +} EFI_ACPI_5_0_CSRT_RESOURCE_GROUP_HEADER;
> +
> +//---
> +
> +- // CSRT Resource Descriptor 12 bytes total
> +//---
> +
> +-
> +typedef struct {
> +  UINT32 Length;
> +  UINT16 ResourceType;
> +  UINT16 ResourceSubType;
> +  UINT32 UID;
> +} EFI_ACPI_5_0_CSRT_RESOURCE_DESCRIPTOR_HEADER;
> +#pragma pack (pop)
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v1 edk2-platforms 8/8] SgiClark.Helios: AcpiTables: Add entry for virtio network device

[Vijayenthiran Subramaniam]

SgiClark Helios include an instance of the virtio network device. Add
a representation for it in the ACPI tables.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vijayenthiran Subramaniam 
---
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf |  3 +++
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl  | 18 
++
 2 files changed, 21 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
index 3686e91bb7e7..8b45702b7cd3 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
@@ -58,5 +58,8 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
   gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
index 3dcf6f71eadb..7cfc419eb3a2 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
@@ -264,5 +264,23 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI",
 }
   })
 }
+
+// VIRTIO NET
+Device (VR01) {
+  Name (_HID, "LNRO0005")
+  Name (_UID, 1)
+  Name (_CCA, 1)// mark the device coherent
+
+  Name (_CRS, ResourceTemplate() {
+Memory32Fixed (
+  ReadWrite,
+  FixedPcdGet32 (PcdVirtioNetBaseAddress),
+  FixedPcdGet32 (PcdVirtioNetSize)
+)
+Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+  FixedPcdGet32 (PcdVirtioNetInterrupt)
+}
+  })
+}
   } // Scope(_SB)
 }
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 7/8] SgiClark.Helios: AcpiTables: Use PCDs for virtio disk

[Vijayenthiran Subramaniam]

Use PCDs instead of hardcoded values for virtio disk in DSDT.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vijayenthiran Subramaniam 
---
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf |  4 
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl  | 10 --
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
index 0ecce2db8a5d..3686e91bb7e7 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf
@@ -55,4 +55,8 @@ [FixedPcd]
   gArmTokenSpaceGuid.PcdPciBusMin
   gArmTokenSpaceGuid.PcdPciBusMax
 
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
index b8eb3b8e7332..3dcf6f71eadb 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl
@@ -254,8 +254,14 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI",
   Name (_CCA, 1)// mark the device coherent
 
   Name (_CRS, ResourceTemplate() {
-Memory32Fixed (ReadWrite, 0x1c13, 0x1)
-Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 202 }
+Memory32Fixed (
+  ReadWrite,
+  FixedPcdGet32 (PcdVirtioBlkBaseAddress),
+  FixedPcdGet32 (PcdVirtioBlkSize)
+)
+Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+  FixedPcdGet32 (PcdVirtioBlkInterrupt)
+}
   })
 }
   } // Scope(_SB)
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 6/8] SgiClark.Ares: AcpiTables: Add entry for virtio network device

[Vijayenthiran Subramaniam]

SgiClark Ares include an instance of the virtio network device. Add
a representation for it in the ACPI tables.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vijayenthiran Subramaniam 
---
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf |  3 +++
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl  | 17 
+
 2 files changed, 20 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
index 10a805e07fd1..d4bacdbc8c85 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
@@ -58,5 +58,8 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
   gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
index af4dc424a77c..69dc33c06b4d 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
@@ -118,5 +118,22 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI",
 }
   })
 }
+
+// VIRTIO NET
+Device (VR01) {
+  Name (_HID, "LNRO0005")
+  Name (_UID, 1)
+  Name (_CCA, 1)// mark the device coherent
+
+  Name (_CRS, ResourceTemplate() {
+Memory32Fixed (ReadWrite,
+  FixedPcdGet32 (PcdVirtioNetBaseAddress),
+  FixedPcdGet32 (PcdVirtioNetSize)
+)
+Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+  FixedPcdGet32 (PcdVirtioNetInterrupt)
+}
+  })
+}
   } // Scope(_SB)
 }
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 5/8] SgiClark.Ares: AcpiTables: Use PCDs for virtio disk

[Vijayenthiran Subramaniam]

Use PCDs instead of hardcoded values for virtio disk in DSDT.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Vijayenthiran Subramaniam 
---
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf |  4 
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl  | 12 +---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
index dcfe4929bb5a..10a805e07fd1 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf
@@ -55,4 +55,8 @@ [FixedPcd]
   gArmTokenSpaceGuid.PcdPciBusMin
   gArmTokenSpaceGuid.PcdPciBusMax
 
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
index c94a7c69e33a..af4dc424a77c 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl
@@ -107,9 +107,15 @@ DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI",
   Name (_UID, 0)
   Name (_CCA, 1)// mark the device coherent
 
-  Name (_CRS, ResourceTemplate () {
-Memory32Fixed (ReadWrite, 0x1c13, 0x1)
-Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 202 }
+  Name (_CRS, ResourceTemplate() {
+Memory32Fixed (
+  ReadWrite,
+  FixedPcdGet32 (PcdVirtioBlkBaseAddress),
+  FixedPcdGet32 (PcdVirtioBlkSize)
+)
+Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+  FixedPcdGet32 (PcdVirtioBlkInterrupt)
+}
   })
 }
   } // Scope(_SB)
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 4/8] Sgi575: AcpiTables: Add entry for virtio network device

[Vijayenthiran Subramaniam]

From: Daniil Egranov 

SGI575 include an instance of the virtio network device. So add
a representation for it in the ACPI tables.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov 
---
 Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf |  3 +++
 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl  | 18 ++
 2 files changed, 21 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
index e236b940a802..c666ea9d51c7 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
@@ -58,5 +58,8 @@ [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
   gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt
 
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
index 833f5b44b6a8..36bc8c3809a0 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
@@ -114,4 +114,22 @@ DefinitionBlock("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI", EFI_ACPI_ARM_OEM
   }
 })
   }
+
+  // VIRTIO NET
+  Device (VR01) {
+Name (_HID, "LNRO0005")
+Name (_UID, 1)
+Name (_CCA, 1)// mark the device coherent
+
+Name (_CRS, ResourceTemplate() {
+  Memory32Fixed (
+ReadWrite,
+FixedPcdGet32 (PcdVirtioNetBaseAddress),
+FixedPcdGet32 (PcdVirtioNetSize)
+  )
+  Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+FixedPcdGet32 (PcdVirtioNetInterrupt)
+  }
+})
+  }
 }
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 3/8] Sgi575: AcpiTables: Use PCDs for virtio disk

[Vijayenthiran Subramaniam]

From: Daniil Egranov 

Use PCDs instead of hardcoded values for virtio disk in DSDT.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov 
---
 Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf |  4 
 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl  | 10 --
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf 
b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
index 59ccb79b6475..e236b940a802 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf
@@ -55,4 +55,8 @@ [FixedPcd]
   gArmTokenSpaceGuid.PcdPciBusMin
   gArmTokenSpaceGuid.PcdPciBusMax
 
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt
+
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl 
b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
index 509cd7cd4262..833f5b44b6a8 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
+++ b/Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl
@@ -104,8 +104,14 @@ DefinitionBlock("DsdtTable.aml", "DSDT", 1, "ARMLTD", 
"ARMSGI", EFI_ACPI_ARM_OEM
 Name (_CCA, 1)// mark the device coherent
 
 Name (_CRS, ResourceTemplate() {
-  Memory32Fixed (ReadWrite, 0x1c13, 0x1)
-  Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 202 }
+  Memory32Fixed (
+ReadWrite,
+FixedPcdGet32 (PcdVirtioBlkBaseAddress),
+FixedPcdGet32 (PcdVirtioBlkSize)
+  )
+  Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) {
+FixedPcdGet32 (PcdVirtioBlkInterrupt)
+  }
 })
   }
 }
-- 
2.7.4

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

[edk2] [PATCH v1 edk2-platforms 2/8] Platform/ARM/SgiPkg: Add support for virtio net device

[Vijayenthiran Subramaniam]

From: Daniil Egranov 

Add support for virtio net device by adding PCDs to specify the data
required to setup the virtio net device and register it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec|  6 +++
 Platform/ARM/SgiPkg/SgiPlatform.dsc| 18 +--
 Platform/ARM/SgiPkg/SgiPlatform.fdf|  4 ++
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf|  3 ++
 Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h |  3 ++
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/VirtioDevices.c| 54 

 6 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index ed29a4d5d91f..39cc3f89fd57 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -38,6 +38,7 @@ [Guids.common]
 
 [PcdsFeatureFlag.common]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|FALSE|BOOLEAN|0x0001
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSupported|FALSE|BOOLEAN|0x0010
 
 [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdDramBlock2Base|0|UINT64|0x0002
@@ -48,5 +49,10 @@ [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x|UINT32|0x0005
   gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|0x|UINT32|0x0006
 
+  # Virtio Network device
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress|0x|UINT32|0x0007
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize|0x|UINT32|0x0008
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt|0x|UINT32|0x0009
+
 [Ppis]
   gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 
0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc
index ada72be72f8a..0c794c6b299d 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
@@ -45,6 +45,7 @@ [LibraryClasses.common]
   VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
   
VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
   FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
+  
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
 
 [LibraryClasses.common.SEC]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -99,6 +100,7 @@ [LibraryClasses.common.UEFI_DRIVER, 
LibraryClasses.common.UEFI_APPLICATION, Libr
 
 [PcdsFeatureFlag.common]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|TRUE
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSupported|TRUE
 
 [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdVFPEnabled|1
@@ -177,14 +179,19 @@ [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdCoreCount|4
   gArmPlatformTokenSpaceGuid.PcdClusterCount|2
 
-  # Ethernet
-  gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
-
   # Virtio Disk
   gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x1c13
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x1
   gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|202
 
+  # Ethernet / Virtio Network
+!ifdef EDK2_ENABLE_SMSC_91X
+  gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
+!endif
+  gArmSgiTokenSpaceGuid.PcdVirtioNetBaseAddress|0x1c15
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSize|0x1
+  gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt|204
+
 

 #
 # Components Section - list of all EDK II Modules needed by this Platform
@@ -288,8 +295,11 @@ [Components.common]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
   }
 
-  # SMSC LAN 91C111
+  # SMSC LAN 91C111 / Virtio Network
+!ifdef EDK2_ENABLE_SMSC_91X
   EmbeddedPkg/Drivers/Lan91xDxe/Lan91xDxe.inf
+!endif
+  OvmfPkg/VirtioNetDxe/VirtioNet.inf
 
   #
   # Required by PCI
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf 
b/Platform/ARM/SgiPkg/SgiPlatform.fdf
index b7af4a2a5925..ddf1fda5a16e 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
+++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
@@ -191,7 +191,11 @@ [FV.FvMain]
   INF NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
   INF NetworkPkg/TcpDxe/TcpDxe.inf
   INF NetworkPkg/IScsiDxe/IScsiDxe.inf
+
+!ifdef EDK2_ENABLE_SMSC_91X
   INF EmbeddedPkg/Drivers/Lan91xDxe/Lan91xDxe.inf
+!endif
+  INF OvmfPkg/VirtioNetDxe/VirtioNet.inf
 
 [FV.FVMAIN_COMPACT]
 FvAlignment= 16
diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
index f920f6ecafb8..3283ff045372 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
@@ -42,10 +42,13 @@ [Guids]
 
 [FeaturePcd]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported
+  gArmSgiTokenSpaceGuid.PcdVirtioNetSupported
 
 [FixedPcd]
   gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
   

[edk2] [PATCH v1 edk2-platforms 1/8] Platform/ARM/SgiPkg: Restructure virtio device registration

[Vijayenthiran Subramaniam]

From: Daniil Egranov 

SGI platforms support multiple virtio devices. So the existing code, that
supports registration of only the virtio disk, is restructured to
accommodate the registration of additional virtio devices.

In addition to this, PCDs to represent the virtio controller base and
address space size are introduced.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov 
---
 Platform/ARM/SgiPkg/SgiPlatform.dec  | 
 8 ++-
 Platform/ARM/SgiPkg/SgiPlatform.dsc  | 
 7 +-
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf  | 
 8 ++-
 Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h   | 
21 ++
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c| 
14 +---
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/{VirtioBlockIo.c => VirtioDevices.c} | 
67 
 6 files changed, 81 insertions(+), 44 deletions(-)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
b/Platform/ARM/SgiPkg/SgiPlatform.dec
index f6e0ba1e927a..ed29a4d5d91f 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dec
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
@@ -37,12 +37,16 @@ [Guids.common]
   gSgiClarkHeliosAcpiTablesFileGuid = { 0x2af40815, 0xa84e, 0x4de9, { 0x8c, 
0x38, 0x91, 0x40, 0xb3, 0x54, 0x40, 0x73 } }
 
 [PcdsFeatureFlag.common]
-  # Set this PCD to TRUE to enable virtio support.
-  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x0001
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|FALSE|BOOLEAN|0x0001
 
 [PcdsFixedAtBuild]
   gArmSgiTokenSpaceGuid.PcdDramBlock2Base|0|UINT64|0x0002
   gArmSgiTokenSpaceGuid.PcdDramBlock2Size|0|UINT64|0x0003
 
+  # Virtio Block device
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x|UINT32|0x0004
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x|UINT32|0x0005
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|0x|UINT32|0x0006
+
 [Ppis]
   gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 
0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc
index b3f76d2d9720..ada72be72f8a 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
@@ -98,7 +98,7 @@ [LibraryClasses.common.UEFI_DRIVER, 
LibraryClasses.common.UEFI_APPLICATION, Libr
 

 
 [PcdsFeatureFlag.common]
-  gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported|TRUE
 
 [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdVFPEnabled|1
@@ -180,6 +180,11 @@ [PcdsFixedAtBuild.common]
   # Ethernet
   gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
 
+  # Virtio Disk
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress|0x1c13
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize|0x1
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkInterrupt|202
+
 

 #
 # Components Section - list of all EDK II Modules needed by this Platform
diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
index d903ed8d3375..f920f6ecafb8 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
@@ -20,7 +20,7 @@ [Defines]
 
 [Sources.common]
   PlatformDxe.c
-  VirtioBlockIo.c
+  VirtioDevices.c
 
 [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
@@ -41,7 +41,11 @@ [Guids]
   gSgiClarkHeliosAcpiTablesFileGuid
 
 [FeaturePcd]
-  gArmSgiTokenSpaceGuid.PcdVirtioSupported
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSupported
+
+[FixedPcd]
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkBaseAddress
+  gArmSgiTokenSpaceGuid.PcdVirtioBlkSize
 
 [Depex]
   TRUE
diff --git a/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h 
b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
new file mode 100644
index ..80d3e3ae4f91
--- /dev/null
+++ b/Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
@@ -0,0 +1,21 @@
+/** @file
+*
+*  Copyright (c) 2018, ARM Limited. All rights reserved.
+*
+*  This program and the accompanying materials are licensed and made available
+*  under the terms and conditions of the BSD License which accompanies this
+*  distribution. The full text of the license may be found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+*
+**/
+
+#ifndef  __SGI_VIRTIO_DEVICES_FORMSET_H__
+#define  __SGI_VIRTIO_DEVICES_FORMSET_H__
+
+#define SGI_VIRTIO_BLOCK_GUID  \
+  { 0x5a96cdcd, 0x6116, 0x4929, { 0xb7, 0x01, 0x3a, 0xc2, 0xfb, 0x1c, 0xe2, 
0x28 } }
+
+#endif
diff --git 

[edk2] [PATCH v1 edk2-platforms 0/8] Platform/ARM/Sgi: Add support for virtio network device

[Vijayenthiran Subramaniam]

This patch series adds support for virtio network controller found in ARM SGI
plaform's fast models. The first patch in this series restructure the
virtio device registration code to allow registration of additional virtio
devices. The second patch adds support for the virtio network device. The rest
of the patches update the ACPI tables to add an entry for the virtio network
device and corresponding PCDs for virtio block and network device.

Daniil Egranov (4):
  Platform/ARM/SgiPkg: Restructure virtio device registration
  Platform/ARM/SgiPkg: Add support for virtio net device
  Sgi575: AcpiTables: Use PCDs for virtio disk
  Sgi575: AcpiTables: Add entry for virtio network device

Vijayenthiran Subramaniam (4):
  SgiClark.Ares: AcpiTables: Use PCDs for virtio disk
  SgiClark.Ares: AcpiTables: Add entry for virtio network device
  SgiClark.Helios: AcpiTables: Use PCDs for virtio disk
  SgiClark.Helios: AcpiTables: Add entry for virtio network device

 Platform/ARM/SgiPkg/SgiPlatform.dec  | 
 14 ++-
 Platform/ARM/SgiPkg/SgiPlatform.dsc  | 
 21 +++-
 Platform/ARM/SgiPkg/SgiPlatform.fdf  | 
  4 +
 Platform/ARM/SgiPkg/AcpiTables/Sgi575AcpiTables.inf  | 
  7 ++
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAresAcpiTables.inf| 
  7 ++
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHeliosAcpiTables.inf  | 
  7 ++
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf  | 
 11 +-
 Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h   | 
 24 
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c| 
 14 +--
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/{VirtioBlockIo.c => VirtioDevices.c} | 
117 +++-
 Platform/ARM/SgiPkg/AcpiTables/Sgi575/Dsdt.asl   | 
 28 -
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkAres/Dsdt.asl | 
 29 -
 Platform/ARM/SgiPkg/AcpiTables/SgiClarkHelios/Dsdt.asl   | 
 28 -
 13 files changed, 260 insertions(+), 51 deletions(-)
 create mode 100644 Platform/ARM/SgiPkg/Include/Guid/SgiVirtioDevicesFormSet.h
 rename Platform/ARM/SgiPkg/Drivers/PlatformDxe/{VirtioBlockIo.c => 
VirtioDevices.c} (25%)

-- 
2.7.4

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