Re: [edk2] [PATCH edk2-platforms 19/27] Silicon/NXP: Add i.MX6 ACPI tables

2019-01-08 Thread Chris Co via edk2-devel
Hi Ard,

> -Original Message-
> From: Ard Biesheuvel 
> Sent: Monday, December 17, 2018 3:14 AM
> To: Chris Co 
> Cc: edk2-devel@lists.01.org; Leif Lindholm ; Michael
> D Kinney 
> Subject: Re: [PATCH edk2-platforms 19/27] Silicon/NXP: Add i.MX6 ACPI tables
> 
> On Fri, 21 Sep 2018 at 10:26, Chris Co 
> wrote:
> >
> > +// PL310 L2 Cache Controller Resource Group
> > +#define CSRT_PL310_MINIMUM_VERSION   1
> > +#define CSRT_PL310_VERSION_2 2
> > +#define CSRT_PL310_RAW_PROTOCOL  0
> > +#define CSRT_PL310_SMC_PROTOCOL  1
> > +
> > +// We use PSCI_CPU_ON to turn on the L2 cache, with special value
> > +// 0x0010 for the Core ID. PSCI sees this core ID and knows
> > +// this is an L2 cache operation, then looks at R2 for the
> > +// operation to perform.
> > +#define PSCI_FID_CPU_ON0x8403
> > +#define L2CACHE_SMC_R1 0x0010
> > +#define L2CACHE_OP_ENABLE  1
> > +#define L2CACHE_OP_DISABLE 2
> > +#define L2CACHE_OP_ENABLE_WRITEBACK3
> > +#define L2CACHE_OP_DISABLE_WRITEBACK   4
> > +#define L2CACHE_OP_ENABLE_WFLZ 5
> > +
> 
> Who defined this protocol? Where is the opposite side implemented?
> 
> Overloading architected PSCI calls to manage platform specific pieces
> is really a no-go.
> 

Since my initial patch set submission, we have updated this code to use the 
standardized protocol instead of overloading PSCI_CPU_ON. It will be in the V2 
patch set.

// PL310 L2 Cache Controller Resource Group
#define CSRT_PL310_MINIMUM_VERSION   1
#define CSRT_PL310_VERSION_2 2
#define CSRT_PL310_RAW_PROTOCOL  0
#define CSRT_PL310_SMC_PROTOCOL  1

#define SMC_CALL_VAL(owner, funcid) \
(0x8000 | (((owner) & 0x3F) << 24) | ((funcid) & 0x))

#define SMC_OWNER_SIP   2
#define IMX_SMC_PL310_ENABLESMC_CALL_VAL(SMC_OWNER_SIP, 1)
#define IMX_SMC_PL310_DISABLE   SMC_CALL_VAL(SMC_OWNER_SIP, 2)
#define IMX_SMC_PL310_ENABLE_WRITEBACK  SMC_CALL_VAL(SMC_OWNER_SIP, 3)
#define IMX_SMC_PL310_DISABLE_WRITEBACK SMC_CALL_VAL(SMC_OWNER_SIP, 4)
#define IMX_SMC_PL310_ENABLE_WFLZ   SMC_CALL_VAL(SMC_OWNER_SIP, 5)

> > +// Debug Port 2 table
> > +EFI_ACPI_5_0_DEBUG_PORT_2_TABLE Dbg2 = {
> 
> STATIC
> 
> > +  {
> > +// Header
> > +{
> > +  EFI_ACPI_5_0_DEBUG_PORT_2_TABLE_SIGNATURE, // Signature
> "DBG2"
> > +  sizeof (EFI_ACPI_5_0_DEBUG_PORT_2_TABLE),  // Length
> > +  EFI_ACPI_DEBUG_PORT_2_TABLE_REVISION,  // Revision
> > +  EFI_ACPI_5_0_UNDEFINED,// Checksum - updated 
> > at
> runtime
> > +  EFI_ACPI_OEM_ID,   // OEM ID[6]
> > +  EFI_ACPI_OEM_TABLE_ID, // OEM Table ID
> > +  EFI_ACPI_OEM_REVISION, // OEM Revision
> > +  EFI_ACPI_CREATOR_ID,   // Creator ID
> > +  EFI_ACPI_CREATOR_REVISION  // Creator Revision
> > +},
> > +sizeof (EFI_ACPI_5_0_DEBUG_PORT_2_TABLE_HEADER), //
> OffsetDbgDeviceinfo
> > +1,   // NumberDbgDeviceInfo
> > +  },
> > +  {
> > +// Uart
> > +{
> > +  // DeviceInfo
> > +  EFI_ACPI_RESERVED_BYTE, // Revision
> > +  sizeof (DEBUG_DEVICE_INFO_UART),// Length
> > +  1,  // 
> > NumberofGenericAddressRegisters
> > +  UART_NAME_SPACE_STRING_LENGTH,  //
> NameSpaceStringLength
> > +  OFFSET_OF (DEBUG_DEVICE_INFO_UART, NameSpaceString),//
> NameSpaceStringOffset
> > +  0,  // 
> > OemDataLength
> > +  EFI_ACPI_RESERVED_WORD, // 
> > OemDataOffset
> > +  DBG2_TYPE_SERIAL,   // PortType
> > +  DBG_PORT_SUBTYPE_IMX6,  // 
> > PortSubtype 000Ch
> 
> Is this subtype defined in a published version of the SPCR/DBG2 specs?
> 

Just checked and the IMX6 subtype is not present in the latest DBG2 spec. I'll 
work on getting it added. Does the subtype need to be in the published DBG2 
spec before the patch can be accepted upstream?

Note: The definition is present in our WDK headers but I'm guessing that is not 
sufficient...

//
// ACPI debug device port types.  The bottom 15 bits of these values should
// match the BCDE_DEBUGGER_TYPE values that are defined in the header
// minkernel\published\base\bcdtypes.w
//

#define DEBUG_DEVICE_PORT_SERIAL 0x8000
#define DEBUG_DEVICE_PORT_1394 0x8001
#define DEBUG_DEVICE_PORT_USB 0x8002
#define DEBUG_DEVICE_PORT_NET 0x8003
#define DEBUG_DEVICE_PORT_LOCAL 0x8004

#define DEBUG_DEVICE_SERIAL_LEGACY_16550 0x0
#define DEBUG_DEVICE_SERIAL_GEN_16550 0x1
#define DEBUG_DEVICE_SERIAL_SPI_MAX311XE 0x2

Re: [edk2] [PATCH edk2-platforms 00/27] Import Hummingboard Edge platform for Windows IoT Core

2018-12-19 Thread Chris Co via edk2-devel
Hi Leif,

No problem - it's been a crazy year all around so I fully understand. Thank you 
for making time to review and provide feedback on this patch set.

I am already out of office for the rest of the year so I will make the changes 
recommended by you and Ard when I get back. And I also have some changes marked 
to simplify portions of the code and make it more readable/sane.

Have a great holiday season!

Chris

From: Leif Lindholm 
Sent: Saturday, December 15, 2018 5:32 AM
To: Chris Co
Cc: edk2-devel@lists.01.org; Ard Biesheuvel; Michael D Kinney
Subject: Re: [PATCH edk2-platforms 00/27] Import Hummingboard Edge platform for 
Windows IoT Core

Hi Chris,

Sorry for dragging this out, but it's been a crazy year.
I think I've now provided feedback for all but the SdMmc bits.

For the next revision, please break out SdMmc support (including and put it last
in the series). I still don't like the idea of adding a third driver
stack to the project, and am likely to have ideas of major
restructuring, but if we can isolate that from the rest, reviewing the
remainder should be reasonably quick next time around.

My last day "in the office" before Christmas is 21 December, and then
I'll be out until 7 January. So basically, if you can get a new set
out early nexy week, I may have a chance to look at it, and if not -
take your time :)

Regards,

Leif

On Fri, Sep 21, 2018 at 08:25:52AM +, Chris Co wrote:
> REF: 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fchristopherco%2Fedk2-platforms%2Ftree%2Fwiniot_hmb_v1data=02%7C01%7CChristopher.Co%40microsoft.com%7Cfe389e235d9f4b5a3db608d66291b91f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636804775359296793sdata=ArbTsmK70lP0z2I5DtAMUtjMaoGvxmjo421G0pFHw20%3Dreserved=0
>
> v0:
> * 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fpipermail%2Fedk2-devel%2F2018-July%2F027213.htmldata=02%7C01%7CChristopher.Co%40microsoft.com%7Cfe389e235d9f4b5a3db608d66291b91f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636804775359296793sdata=YPaztVWBhe3mimyel5pBFmUQn16PDobNrZDG2Pw9Z4M%3Dreserved=0
> * 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fpipermail%2Fedk2-devel%2F2018-July%2F027266.htmldata=02%7C01%7CChristopher.Co%40microsoft.com%7Cfe389e235d9f4b5a3db608d66291b91f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636804775359296793sdata=aNuEw0cnaQTgI9vG1aoaeDKCKeZX0nPZPBxrk5IMZLM%3Dreserved=0
> * 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fpipermail%2Fedk2-devel%2F2018-July%2F027333.htmldata=02%7C01%7CChristopher.Co%40microsoft.com%7Cfe389e235d9f4b5a3db608d66291b91f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636804775359296793sdata=KBKCyLHP2AmOifipqCPt2Q6cxByt%2Fw18sb1v63Eh5%2FE%3Dreserved=0
> * 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fpipermail%2Fedk2-devel%2F2018-July%2F027409.htmldata=02%7C01%7CChristopher.Co%40microsoft.com%7Cfe389e235d9f4b5a3db608d66291b91f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636804775359296793sdata=chPGxhVlyJmKSOLTn9UNWRdfeya2IdqCIuno2DQlRpk%3Dreserved=0
>
> The patch set supports the bring up of Windows 10 IoT Core on
> Solidrun's Hummingboard Edge board running NXP's i.MX 6Quad SoC.
>
> This patch set is a preliminary submission, with the goal to get further 
> review
> feedback from maintainers since the v0 version had too many initial issues to
> conduct a full review.
>
> Changes in this patch set from v0:
> * Merged the 4 previous patch sets into one.
> * All code should now follow the edk2 coding style.
> * SMBIOS driver refactored to use PCDs. Fixed UUID generation to use MAC 
> address.
> * Updated ACPI HIDs to proper NXP IDs where applicable.
> * Removed unnecessary (and spec-violating) _DSD methods from our ACPI tables.
> * General code cleanup and refactoring.
> * Add Silicon package support for i.MX 6Solo/DualLite, 6SoloX, 
> 6DualPlus/QuadPlus
> families in iMX6Pkg.
>
> Known issues remaining from previous review:
> * Silicon/NXP/iMXPlatformPkg: SdhcDxe fixed initialization needs to be moved 
> to
> a PlatformDxe init and use NonDiscoverableDeviceRegistrationLib.
> * Platform/Microsoft: Left in SdMmcDxe code. Alternatives are still under
> evaluation.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
>
> Christopher Co (27):
>   Platform/Microsoft: Add OpteeClientPkg dec
>   Platform/Microsoft: Add SdMmc Dxe Driver
>   Platform/Microsoft: Add MsPkg
>   Silicon/NXP: Add iMXPlatformPkg dec
>   Silicon/NXP: Add UART library support for i.MX platforms
>   Silicon/NXP: Add I2C library support for i.MX platforms
>   Silicon/NXP: Add i.MX display library support
>   Silicon/NXP: Add Virtual RTC support for i.MX platform
>   Silicon/NXP: Add headers for SoC-specific i.MX packages to use
>   Silicon/NXP: Add 

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

2018-12-03 Thread Chris Co via edk2-devel
Hi Leif,

> -Original Message-
> From: Leif Lindholm 
> Sent: Thursday, November 8, 2018 10:14 AM
> To: Chris Co 
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney 
> Subject: Re: [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and
> EPIT timer headers
> 
> On Fri, Sep 21, 2018 at 08:26:05AM +, Chris Co wrote:
> > This adds the definitions for the NXP i.MX6 General Purpose Timer and
> > the Enhanced Periodic Interrupt Timer modules.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > ---
> >  Silicon/NXP/iMX6Pkg/Include/common_epit.h | 118 +
> > Silicon/NXP/iMX6Pkg/Include/common_gpt.h  | 271
> 
> >  2 files changed, 389 insertions(+)
> >
> > 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)

> > @@ -0,0 +1,118 @@
> > +/** @file
> > +*
> > +*  Provides definitions for the EPIT (Enhanced Periodic Interrupt
> > +Timer)
> > +*  module that are common to Freescale SoCs.
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*  Copyright (c) 2004-2010, Freescale Semiconductor, Inc. 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
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C5da90dbbf4b5452a4c8e08d645a605cc%7C72f988
> bf86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772976703090578sdata=
> g3OOywqu7
> > +VDuq39F3PVs8SaXZVbA7h77F7riMUXb0lE%3Dreserved=0
> > +*
> > +*  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 __COMMON_EPIT_H
> > +#define __COMMON_EPIT_H
> 
> COMMON is a bit too common. Can we at least stick an IMX on the front?
> 
> This is really also a problem for all of the structs and #defines below as 
> well.
> 
> > +
> > +typedef struct {
> > +  UINT32 CR;
> > +  UINT32 SR;
> > +  UINT32 LR;
> > +  UINT32 CMPR;
> > +  UINT32 CNT;
> > +} CSP_EPIT_REG, *PCSP_EPIT_REG;
> 
> I'm really not a fan of typedef pointers. Please drop the *PSCP one.
> 
> > +
> > +#define EPIT_CR_OFFSET  0x
> > +#define EPIT_SR_OFFSET  0x0004
> > +#define EPIT_LR_OFFSET  0x0008
> > +#define EPIT_CMPR_OFFSET0x000C
> > +#define EPIT_CNR_OFFSET 0x0010
> > +
> > +#define EPIT_CR_EN_LSH  0
> 
> What is LSH?
> 
> > +#define EPIT_CR_ENMOD_LSH   1
> > +#define EPIT_CR_OCIEN_LSH   2
> > +#define EPIT_CR_RLD_LSH 3
> > +#define EPIT_CR_PRESCALAR_LSH   4
> > +#define EPIT_CR_SWR_LSH 16
> > +#define EPIT_CR_IOVW_LSH17
> > +#define EPIT_CR_DBGEN_LSH   18
> > +#define EPIT_CR_WAITEN_LSH  19
> > +#define EPIT_CR_DOZEN_LSH   20
> > +#define EPIT_CR_STOPEN_LSH  21
> > +#define EPIT_CR_OM_LSH  22
> > +#define EPIT_CR_CLKSRC_LSH  24
> > +
> > +#define EPIT_SR_OCIF_LSH0
> > +#define EPIT_LR_LOAD_LSH0
> > +#define EPIT_CMPR_COMPARE_LSH   0
> > +#define EPIT_CNT_COUNT_LSH  0
> > +
> > +#define EPIT_CR_EN_WID  1
> 
> What is WID?
> 

LSH is the number of bits to left shift to get to the specified bitfield in the 
register. WID is the length of the specified bitfield.
I don't like this register access pattern at all. Will define structs of the 
registers instead...

Thanks,
Chris

> > +#define EPIT_CR_ENMOD_WID   1
> > +#define EPIT_CR_OCIEN_WID   2
> > +#define EPIT_CR_RLD_WID 1
> > +#define EPIT_CR_PRESCALAR_WID   12
> > +#define EPIT_CR_SWR_WID 1
> > +#define EPIT_CR_IOVW_WID1
> > +#define EPIT_CR_DBGEN_WID   1
> > +#define EPIT_CR_WAITEN_WID  1
> > +#define EPIT_CR_DOZEN_WID   1
> > +#define EPIT_CR_STOPEN_WID  1
> > +#define EPIT_CR_OM_WID  2
> > +#define EPIT_CR_CLKSRC_WID  2
> > +
> > +#define EPIT_SR_OCIF_WID1
> > +#define EPIT_LR_LOAD_WID32
> > +#define EPIT_CMPR_COMPARE_WID   32
> > +#define EPIT_CNT_COUNT_WID  32
> > +
> > +// CR
> > +#define EPIT_CR_EN_DISABLE  0
> > +#define EPIT_CR_EN_ENABLE   1
> > +
> > +#define EPIT_CR_ENMOD_RESUME0
> > +#define EPIT_CR_ENMOD_LOAD  1
> > +
> > +#define EPIT_CR_OCIEN_DISABLE   0
> > +#define EPIT_CR_OCIEN_ENABLE1
> > +
> > +#define 

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

2018-12-03 Thread Chris Co via edk2-devel



> -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.

Thanks,
Chris

> > 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


Re: [edk2] [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library

2018-12-03 Thread Chris Co via edk2-devel
Hi Leif,

> -Original Message-
> From: Leif Lindholm 
> Sent: Thursday, November 8, 2018 10:00 AM
> To: Chris Co 
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney 
> Subject: Re: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX
> library
> 
> On Fri, Sep 21, 2018 at 08:26:03AM +, Chris Co wrote:
> > This adds support for initializing and manipulating the I/O Pads on
> > NXP i.MX6 SoCs.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > ---
> >  Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c  | 151
> 
> >  Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf |  41
> > ++
> >  2 files changed, 192 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > new file mode 100644
> > index ..7c0c7b54a2fe
> > --- /dev/null
> > +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > @@ -0,0 +1,151 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +*  This program and the accompanying materials
> > +*  are licensed and made available under the terms and conditions of
> > +the BSD License
> > +*  which accompanies this distribution.  The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b
> f86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670sdata=
> vkaVmLhCg
> > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3Dreserved=0
> > +*
> > +*  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 
> > +
> > +// Muxing functions
> > +VOID
> > +ImxPadConfig (
> > +  IN  IMX_PAD Pad,
> > +  IN  IMX_PADCFG  PadConfig
> 
> I'll get back to reviewing patch 11 at some point, but that one is hard 
> going. So
> I'll point out here what I'll mention then:
> Please just use UINT64. Typedefs are useful to reduce pointless repetition for
> structs, but here it just obscures what is programatically going on.
> 

I'm thinking to rework the PADCFG to actually use structs properly instead of 
custom-packing with macros. It seems the previous dev was very macro happy and 
I am the opposite...
The remaining feedback makes sense. Will address for v2.

Thanks,
Chris

> > +  )
> > +{
> > +  // Configure Mux Control
> > +  MmioWrite32 (
> > +IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad),
> > +_IMX_PADCFG_MUX_CTL (PadConfig));
> 
> It would be worth adding macros or simple accessor functions for these - 
> there's
> a lot of text in this file that has no semantic value and needs to be manually
> filtered when reading.
> 
> I.e. IomuxWrite32 (Pad, ...), IomuxRead32 (Pad), ...
> 
> Other comment really belonging to 11:
> Please drop leading _ from macros. Such macros are intended for internal use
> by toolchains.
> 
> > +
> > +  // Configure Select Input Control
> > +  if (_IMX_PADCFG_SEL_INP (PadConfig) != 0) {
> > +DEBUG ((DEBUG_INFO, "Setting INPUT_SELECT %x value %x\n",
> > +_IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)),
> > +_IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig;
> > +
> > +MmioWrite32 (
> > +  _IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)),
> > +  _IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig)));  }
> > +
> > +  // Configure Pad Control
> > +  MmioWrite32 (
> > +IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad),
> > +_IMX_PADCFG_PAD_CTL (PadConfig)); }
> > +
> > +VOID
> > +ImxPadDumpConfig (
> > +  IN  CHAR8 *SignalFriendlyName,
> > +  IN  IMX_PAD Pad
> > +  )
> > +{
> > +  IMX_IOMUXC_MUX_CTL  MuxCtl;
> > +  IMX_IOMUXC_PAD_CTL  PadCtl;
> > +
> > +  MuxCtl.AsUint32 = MmioRead32 (
> > +  IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad));
> > +
> > +  DEBUG ((
> > +   DEBUG_INIT,
> > +   "- %a MUX_CTL(0x%p)=0x%08x: MUX_MODE:%d SION:%d | ",
> > +   SignalFriendlyName,
> > +   IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad),
> > +   MuxCtl.AsUint32,
> > +   MuxCtl.Fields.MUX_MODE,
> > +   MuxCtl.Fields.SION));
> > +
> > +  PadCtl.AsUint32 = MmioRead32 (
> > +  IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad));
> > +
> > +  DEBUG ((
> > +   DEBUG_INIT,
> > +   "PAD_CTL(0x%p)=0x%08x: SRE:%d DSE:%d SPEED:%d ODE:%d PKE:%d
> PUE:%d PUS:%d HYS:%d\n",
> > +   IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad),
> > +   PadCtl.AsUint32,
> > +   

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

2018-11-30 Thread Chris Co via edk2-devel
Hi Leif,

> -Original Message-
> From: Leif Lindholm 
> Sent: Thursday, November 1, 2018 11:20 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 Fri, Sep 21, 2018 at 08:26:00AM +, Chris Co wrote:
> > This adds common headers for other NXP i.MX SoC packages.
> > More specifically, this adds i.MX-generic GPIO, IoMux, and Platform
> > definitions.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > ---
> >  Silicon/NXP/iMXPlatformPkg/Include/Platform.h | 67 ++
> > Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h  | 92
> >   Silicon/NXP/iMXPlatformPkg/Include/iMXIoMux.h |
> > 24 +
> >  3 files changed, 183 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > new file mode 100644
> > index ..8a1e828f68ea
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > @@ -0,0 +1,67 @@
> > +/** @file
> > +*
> > +*  i.MX Platform specific defines for constructing ACPI tables
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +*  This program and the accompanying materials
> > +*  are licensed and made available under the terms and conditions of
> > +the BSD License
> > +*  which accompanies this distribution.  The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%3Dreserved=0
> > +*
> > +*  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 _PLATFORM_IMX_H_
> > +#define _PLATFORM_IMX_H_
> > +
> > +#include 
> > +
> > +#define EFI_ACPI_OEM_ID   {'M','C','R','S','F','T'} // OEMID 6 
> > bytes
> > +#define EFI_ACPI_VENDOR_IDSIGNATURE_32('N','X','P','I')
> > +#define EFI_ACPI_CSRT_REVISION0x0005
> > +#define EFI_ACPI_5_0_CSRT_REVISION0x
> > +
> > +// Resource Descriptor Types
> > +#define EFI_ACPI_CSRT_RD_TYPE_INTERRUPT 1 #define
> > +EFI_ACPI_CSRT_RD_TYPE_TIMER 2 #define EFI_ACPI_CSRT_RD_TYPE_DMA 3
> > +#define EFI_ACPI_CSRT_RD_TYPE_CACHE 4
> > +
> > +// Resource Descriptor Subtypes
> > +#define EFI_ACPI_CSRT_RD_SUBTYPE_INTERRUPT_LINES 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_INTERRUPT_CONTROLLER 1 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_TIMER 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_DMA_CHANNEL 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_DMA_CONTROLLER 1 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_CACHE 0
> 
> 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 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)
> > +
> > +#endif // !_PLATFORM_IMX_H_
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > new file mode 100644
> > index ..dce01f789058
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > @@ -0,0 +1,92 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +*  This program and the accompanying materials

Re: [edk2] [PATCH edk2-platforms 07/27] Silicon/NXP: Add i.MX display library support

2018-11-28 Thread Chris Co via edk2-devel
Hi Leif,

> -Original Message-
> From: Leif Lindholm 
> Sent: Thursday, November 1, 2018 11:05 AM
> To: Chris Co 
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney 
> Subject: Re: [PATCH edk2-platforms 07/27] Silicon/NXP: Add i.MX display
> library support
> 
> On Fri, Sep 21, 2018 at 08:25:58AM +, Chris Co wrote:
> > This adds support for processing EDID data on NXP i.MX platforms.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > ---
> >  Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h| 114
> +++
> >  Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.c   | 152
> 
> >  Silicon/NXP/iMXPlatformPkg/Library/iMXDisplayLib/iMXDisplayLib.inf |
> > 31 
> >  3 files changed, 297 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > new file mode 100644
> > index ..70ef8d0af97f
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXDisplay.h
> > @@ -0,0 +1,114 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +*
> > +*  This program and the accompanying materials
> > +*  are licensed and made available under the terms and conditions of
> > +the BSD License
> > +*  which accompanies this distribution.  The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C06e5c1f1adef43f79f9008d6402494f7%7C72f988bf86
> f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766923203571004sdata=6GJ
> qphnDZ
> > +gT%2FGOiEeSSnPeTRGiRn%2B9CB%2Fi4A0ilq3MA%3Dreserved=0
> > +*
> > +*  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 __IMX_DISPLAY_H__
> > +#define __IMX_DISPLAY_H__
> > +
> > +#define EDID_MIN_SIZE   128
> > +#define EDID_I2C_ADDRESS0x50
> 
> Are all of these #defines and functions called only from within iMX platform
> code?
> 
These defines and functions are called by iMX platform and silicon (iMX6Pkg) 
code. Should I prefix these defines with "IMX_" and function names with "Imx"?

I made changes to address the rest of the feedback.

Chris

> > +
> > +// The first DTD is the preferred timing, refer to 3.1 VESA EDID spec.
> > +#define EDID_DTD_1_OFFSET   0x36
> > +#define EDID_DTD_2_OFFSET   0x48
> > +#define EDID_DTD_3_OFFSET   0x5A
> > +#define EDID_DTD_4_OFFSET   0x6C
> > +
> > +typedef enum {
> > +  PIXEL_FORMAT_ARGB32,
> > +  PIXEL_FORMAT_BGRA32,
> > +} PIXEL_FORMAT;
> > +
> > +typedef struct _DISPLAY_TIMING {
> > +  UINT32 PixelClock;
> > +  UINT32 HActive;
> > +  UINT32 HBlank;
> > +  UINT32 VActive;
> > +  UINT32 VBlank;
> > +  UINT32 HSync;
> > +  UINT32 VSync;
> > +  UINT32 HSyncOffset;
> > +  UINT32 VSyncOffset;
> > +  UINT32 HImageSize;
> > +  UINT32 VImageSize;
> > +  UINT32 HBorder;
> > +  UINT32 VBorder;
> > +  UINT32 EdidFlags;
> > +  UINT32 Flags;
> > +  UINT32 PixelRepetition;
> > +  UINT32 Bpp;
> > +  PIXEL_FORMAT PixelFormat;
> > +} DISPLAY_TIMING, *PDISPLAY_TIMING, DTD;
> > +
> > +typedef struct _DETAILED_TIMING_DESCRIPTOR {
> > +  UINT8 PixelClock[2];
> > +  UINT8 HActive;
> > +  UINT8 HBlank;
> > +  UINT8 HActiveBlank;
> > +  UINT8 VActive;
> > +  UINT8 VBlank;
> > +  UINT8 VActiveBlank;
> > +  UINT8 HSyncOffset;
> > +  UINT8 HSyncWidth;
> > +  UINT8 VSyncOffsetWidth;
> > +  UINT8 HVOffsetWidth;
> > +  UINT8 HImageSize;
> > +  UINT8 VImageSize;
> > +  UINT8 HVImageSize;
> > +  UINT8 HBorder;
> > +  UINT8 VBorder;
> > +  UINT8 EdidFlags;
> > +} DETAILED_TIMING_DESCRIPTOR, *PDETAILED_TIMING_DESCRIPTOR;
> > +
> > +/**
> > +  Convert detailed timing descriptor to display timing format
> > +
> > +  @param[in]DTDPtrPointer to detailed timing descriptor.
> > +  @param[out]   DisplayTimingPtr  Pointer to display timing structure.
> > +
> > +  @retval   EFI_SUCCESS   Detailed timing descriptor data was converted.
> > +
> > +**/
> > +EFI_STATUS
> > +ConvertDTDToDisplayTiming (
> > +  IN DETAILED_TIMING_DESCRIPTOR   *DTDPtr,
> > +  OUT DISPLAY_TIMING  *DisplayTimingPtr
> > +  );
> > +
> > +/**
> > +  Debug dump of Display Timing structure
> > +
> > +  @param[in]DisplayTimingNamePtr  Name of display timing structure.
> > +  @param[in]DisplayTimingPtr  Pointer to display timing structure.
> > +**/
> > +VOID
> > +PrintDisplayTiming (
> > +  IN CHAR8*DisplayTimingNamePtr,
> > +  IN DISPLAY_TIMING   *DisplayTimingPtr
> > +  );
> > +
> > +/**
> > +  Check if EDID is valid
> > +
> > +  @param[in]EdidDataPtr  Pointer to EDID data.
> > +
> > +  @retval   EFI_SUCCESS   

Re: [edk2] [PATCH edk2-platforms 01/27] Platform/Microsoft: Add OpteeClientPkg dec

2018-11-05 Thread Chris Co via edk2-devel
Hi Sumit,

> -Original Message-
> From: Sumit Garg 
> 
> Hi Chris,
> 
> On Sat, 3 Nov 2018 at 05:25, Chris Co  wrote:
> >
> > Hi Sumit,
> >
> > > -Original Message-
> > > From: Sumit Garg 
> > >
> > > + OP-TEE ML.
> > >
> > > On Fri, 2 Nov 2018 at 06:11, Chris Co 
> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > Our full OpteeClientPkg has:
> > > > - Our OpteeClientAPI implementation. I was monitoring the merge
> > > > progress
> > > on OpteeLib and will look into moving over now that it is available.
> > > > - The fTPM and AuthVar TA binaries. In our current design, the TA
> > > > binaries
> > > are loaded at runtime. We could host the binaries themselves
> > > elsewhere on the filesystem, but we do not want these binaries as
> > > early/pseudo TAs. Is there a plan for OpteeLib to support loading full 
> > > TAs?
> > >
> > > Early TAs [1] are basically full TAs only, running in Secure EL0 mode.
> > > So instead of loading TA from normal world file-system, they are
> > > linked into a special data section in the OP-TEE core blob.
> > >
> > > Also I don't think loading TAs dynamically especially during boot
> > > makes much sense due to following reasons:
> > > 1. Increased boot time.
> > > 2. Fixed TAs like in your case which could be linked as early TAs as well.
> > >
> >
> > We prefer to load TAs dynamically for a more flexible servicing story. My
> understanding is that Early TAs are coupled with the OP-TEE binary itself, so
> to update an Early TA, a new OP-TEE binary would need to be created and
> pushed. We want to avoid rolling a new OP-TEE and only update the TA
> binary in this scenario.
> >
> 
> Are you referring to run-time updates on the device in the field? If this is 
> the
> case then how do you think to update TAs, is it via some custom capsule
> update method?
> 

Yes, run-time TA updates. Currently, our fTPM and Authvar TAs get packaged 
inside our UEFI binary. So an update to a TA means a UEFI update via firmware 
capsule.
The discussion of these TA binaries living on the filesystem were ideas we were 
discussing internally but are not fully baked or committed to.

> I do consider these TAs used during boot as essential secure services provided
> by the secure firmware (OP-TEE in this case). So these TAs should be part of
> firmware itself and updates for them should come through firmware capsule
> updates only.
> 

I agree in principle and I think I see where the misalignment is, mostly coming 
from my end.
The security guarantees (termed TCPS) we want to provide on the current 
hardware we support (NXP i.MX6), mean OP-TEE becomes prohibitively difficult to 
update. This is due to a hardware resource limitation (not enough fuse space). 
If this limitation were not present, we could freely update OP-TEE and package 
these TAs as EarlyTAs.

Info on TCPS (whitepaper at bottom of post) - 
https://www.microsoft.com/en-us/microsoft-365/blog/2018/04/24/trusted-cyber-physical-systems-looks-to-protect-your-critical-infrastructure-from-modern-threats-in-the-world-of-iot/

I'm not sure how you want to handle this from an OpteeLib vs custom platform 
package perspective.

> > > And you mentioned filesystem, are you referring to root filesystem?
> > >
> >
> > We have not implemented this yet, but we were thinking to have the TA
> binaries present in the EFI partition.
> >
> 
> AFAIK, EFI partition is shared among Linux and UEFI. This provides Linux
> access to secure firmware TAs that could be a security concern (denial of
> service could be one of them).
> 

Note - we are booting Windows, though your point here is still valid. The TAs 
living in the filesystem is not what is implemented today. It was an idea we 
were discussing internally.

> > > > - We have two client drivers: a firmware TPM TA driver and an
> > > authenticated variable TA driver. These talk through the
> > > tee-supplicant to their respective TAs.
> > > >
> > >
> > > Here from tee-supplicant apart from loading TAs, what other services
> > > are you expecting? If you are looking for secure storage via RPMB,
> > > that could be an enhancement to OpteeLib adding corresponding RPC
> handling here [2].
> > >
> >
> > For RPC handling, we are looking for the following callback support:
> > - OPTEE_SMC_RPC_FUNC_ALLOC
> > - OPTEE_SMC_RPC_FUNC_FREE
> > - OPTEE_SMC_RPC_FUNC_CMD
> > - OPTEE_MSG_RPC_CMD_LOAD_TA
> 
> Please see above comments for this.
> 
> > - OPTEE_MSG_RPC_CMD_RPMB
> > - OPTEE_MSG_RPC_CMD_GET_TIME
> 
> Can you share the usage of OPTEE_MSG_RPC_CMD_GET_TIME? AFAIK, this is
> used to get REE time from OP-TEE.
> 

I dug further and found that this was being used in our fTPM TA for debug logs. 
It has since been deprecated so we do not need this RPC command.

> > - OPTEE_MSG_RPC_CMD_SHM_ALLOC
> > - OPTEE_MSG_RPC_CMD_SHM_FREE
> > - OPTEE_MSG_RPC_CMD_WAIT_QUEUE
> 
> I don't think we need OPTEE_MSG_RPC_CMD_WAIT_QUEUE implementation
> in UEFI as its a single threaded 

Re: [edk2] [PATCH edk2-platforms 01/27] Platform/Microsoft: Add OpteeClientPkg dec

2018-11-02 Thread Chris Co via edk2-devel
Hi Sumit,

> -Original Message-
> From: Sumit Garg 
> Sent: Thursday, November 1, 2018 10:24 PM
> To: Chris Co 
> Cc: Leif Lindholm ; edk2-devel@lists.01.org; Ard
> Biesheuvel ; Michael D Kinney
> ; tee-...@lists.linaro.org
> Subject: Re: [PATCH edk2-platforms 01/27] Platform/Microsoft: Add
> OpteeClientPkg dec
> 
> + OP-TEE ML.
> 
> On Fri, 2 Nov 2018 at 06:11, Chris Co  wrote:
> >
> > Hi Sumit,
> >
> > Our full OpteeClientPkg has:
> > - Our OpteeClientAPI implementation. I was monitoring the merge progress
> on OpteeLib and will look into moving over now that it is available.
> > - The fTPM and AuthVar TA binaries. In our current design, the TA binaries
> are loaded at runtime. We could host the binaries themselves elsewhere on
> the filesystem, but we do not want these binaries as early/pseudo TAs. Is
> there a plan for OpteeLib to support loading full TAs?
> 
> Early TAs [1] are basically full TAs only, running in Secure EL0 mode.
> So instead of loading TA from normal world file-system, they are linked into a
> special data section in the OP-TEE core blob.
> 
> Also I don't think loading TAs dynamically especially during boot makes much
> sense due to following reasons:
> 1. Increased boot time.
> 2. Fixed TAs like in your case which could be linked as early TAs as well.
> 

We prefer to load TAs dynamically for a more flexible servicing story. My 
understanding is that Early TAs are coupled with the OP-TEE binary itself, so 
to update an Early TA, a new OP-TEE binary would need to be created and pushed. 
We want to avoid rolling a new OP-TEE and only update the TA binary in this 
scenario.

> And you mentioned filesystem, are you referring to root filesystem?
> 

We have not implemented this yet, but we were thinking to have the TA binaries 
present in the EFI partition.

> > - We have two client drivers: a firmware TPM TA driver and an
> authenticated variable TA driver. These talk through the tee-supplicant to
> their respective TAs.
> >
> 
> Here from tee-supplicant apart from loading TAs, what other services are you
> expecting? If you are looking for secure storage via RPMB, that could be an
> enhancement to OpteeLib adding corresponding RPC handling here [2].
> 

For RPC handling, we are looking for the following callback support:
- OPTEE_SMC_RPC_FUNC_ALLOC
- OPTEE_SMC_RPC_FUNC_FREE
- OPTEE_SMC_RPC_FUNC_CMD
- OPTEE_MSG_RPC_CMD_LOAD_TA
- OPTEE_MSG_RPC_CMD_RPMB
- OPTEE_MSG_RPC_CMD_GET_TIME
- OPTEE_MSG_RPC_CMD_SHM_ALLOC
- OPTEE_MSG_RPC_CMD_SHM_FREE
- OPTEE_MSG_RPC_CMD_WAIT_QUEUE

Thanks,
Chris

> [1]
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2FOP-
> TEE%2Foptee_os%2Fblob%2Fmaster%2Fdocumentation%2Foptee_design.md
> %23early-trusted-
> applicationsdata=02%7C01%7CChristopher.Co%40microsoft.com%7C4a
> 7d8c01e4804365f4eb08d640837a15%7C72f988bf86f141af91ab2d7cd011db47%
> 7C1%7C0%7C636767330779998429sdata=yaDWw5Z6yuux1o89kxzbknVp
> b%2B1OHUagbB%2FOGS4dAcU%3Dreserved=0
> [2]
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FArmPkg%2FLibrary%2FOpteeL
> ib%2FOptee.c%23L147data=02%7C01%7CChristopher.Co%40microsoft.c
> om%7C4a7d8c01e4804365f4eb08d640837a15%7C72f988bf86f141af91ab2d7cd
> 011db47%7C1%7C0%7C636767330779998429sdata=Lsplb1L7Ugd2C6cXG
> 8gBo40Ei8UQPtIA7fNEDL1t%2Fbg%3Dreserved=0
> 
> Regards,
> Sumit
> 
> > Chris
> >
> > > -Original Message-
> > > From: Sumit Garg 
> > > Sent: Thursday, November 1, 2018 3:55 AM
> > > To: Chris Co ; Leif Lindholm
> > > 
> > > Cc: edk2-devel@lists.01.org; Ard Biesheuvel
> > > ; Michael D Kinney
> > > 
> > > Subject: Re: [PATCH edk2-platforms 01/27] Platform/Microsoft: Add
> > > OpteeClientPkg dec
> > >
> > > Hi Christopher,
> > >
> > > Optee Client library has recently been merged to edk2 source code.
> > > It tries to provide a generic interface [1] to OP-TEE based trusted
> > > applications (pseudo/early).
> > >
> > > AFAIK, you don't need any platform specific hook in client interface
> > > to work with upstream OP-TEE. So instead you should use Optee library.
> > >
> > > [1]
> > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > > hub.c
> > >
> om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FArmPkg%2FInclude%2FLibrary
> > >
> %2FOpteeLib.hdata=02%7C01%7CChristopher.Co%40microsoft.com%7C
> > >
> c19b84ef7f8f4213424108d63fe88f66%7C72f988bf86f141af91ab2d7cd011db47
> > >
> %7C1%7C0%7C63675404786500sdata=m24akbKtoyCERVN77meoSU
> > > H6E%2Bpf8W2P5MF7nvU5y7I%3Dreserved=0
> > >
> > > Regards,
> > > Sumit
> > >
> > > On Thu, 1 Nov 2018 at 02:13, Leif Lindholm 
> wrote:
> > > >
> > > > +Sumit (just to loop you two together). Is there anything
> > > > +Microsoft
> > > > platform specific about what will go in here?
> > > >
> > > > /
> > > > Leif
> > > >
> > > > On Fri, Sep 21, 2018 at 08:25:53AM +, Chris Co wrote:
> > > > > On Windows IoT Core devices with ARM TrustZone 

Re: [edk2] [PATCH edk2-platforms 05/27] Silicon/NXP: Add UART library support for i.MX platforms

2018-11-01 Thread Chris Co via edk2-devel
Hi Leif,

> -Original Message-
> From: Leif Lindholm 
> Sent: Thursday, November 1, 2018 2:00 AM
> To: Chris Co 
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney 
> Subject: Re: [PATCH edk2-platforms 05/27] Silicon/NXP: Add UART library
> support for i.MX platforms
> 
> On Fri, Sep 21, 2018 at 08:25:56AM +, Chris Co wrote:
> > This adds support for interact with the UART controller on NXP i.MX
> > platforms.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Michael D Kinney 
> > ---
> > +
> > +**/
> > +RETURN_STATUS
> > +EFIAPI
> > +SerialPortInitialize (
> > +  VOID
> > +  )
> > +{
> > +  MX6UART_REGISTERS   *UartBase;
> > +  UINT32  Ucr1;
> 
> ... once we create variables to hold data from those registers, those 
> variables
> should really be FullyCompliantCamelCase.
> But really, something like this could just be "Data" or "Value" or something
> like that. Or if you want a bit more, "ControlRegister".
> 
> Not going to comment on each function, but the pattern repeats throughout
> the file.
> 
Noted. I'll change variable names throughout all of the files.

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


Re: [edk2] [PATCH edk2-platforms 01/27] Platform/Microsoft: Add OpteeClientPkg dec

2018-11-01 Thread Chris Co via edk2-devel
Hi Sumit,

Our full OpteeClientPkg has:
- Our OpteeClientAPI implementation. I was monitoring the merge progress on 
OpteeLib and will look into moving over now that it is available.
- The fTPM and AuthVar TA binaries. In our current design, the TA binaries are 
loaded at runtime. We could host the binaries themselves elsewhere on the 
filesystem, but we do not want these binaries as early/pseudo TAs. Is there a 
plan for OpteeLib to support loading full TAs?
- We have two client drivers: a firmware TPM TA driver and an authenticated 
variable TA driver. These talk through the tee-supplicant to their respective 
TAs.

Chris

> -Original Message-
> From: Sumit Garg 
> Sent: Thursday, November 1, 2018 3:55 AM
> To: Chris Co ; Leif Lindholm
> 
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney 
> Subject: Re: [PATCH edk2-platforms 01/27] Platform/Microsoft: Add
> OpteeClientPkg dec
> 
> Hi Christopher,
> 
> Optee Client library has recently been merged to edk2 source code. It tries to
> provide a generic interface [1] to OP-TEE based trusted applications
> (pseudo/early).
> 
> AFAIK, you don't need any platform specific hook in client interface to work
> with upstream OP-TEE. So instead you should use Optee library.
> 
> [1]
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FArmPkg%2FInclude%2FLibrary
> %2FOpteeLib.hdata=02%7C01%7CChristopher.Co%40microsoft.com%7C
> c19b84ef7f8f4213424108d63fe88f66%7C72f988bf86f141af91ab2d7cd011db47
> %7C1%7C0%7C63675404786500sdata=m24akbKtoyCERVN77meoSU
> H6E%2Bpf8W2P5MF7nvU5y7I%3Dreserved=0
> 
> Regards,
> Sumit
> 
> On Thu, 1 Nov 2018 at 02:13, Leif Lindholm  wrote:
> >
> > +Sumit (just to loop you two together). Is there anything Microsoft
> > platform specific about what will go in here?
> >
> > /
> > Leif
> >
> > On Fri, Sep 21, 2018 at 08:25:53AM +, Chris Co wrote:
> > > On Windows IoT Core devices with ARM TrustZone capabilities,
> > > EDK2 runs in normal world and we use OP-TEE to execute secure world
> > > operations. The overall package will contain client-side support to
> > > invoke EDK2 services implemented as OP-TEE trusted applications that
> > > run in secure world.
> > >
> > > This commit adds the initial dec file to add some PCD settings
> > > needed by other packages.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Christopher Co 
> > > Cc: Ard Biesheuvel 
> > > Cc: Leif Lindholm 
> > > Cc: Michael D Kinney 
> > > ---
> > >  Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec | 49
> > > 
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > b/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > new file mode 100644
> > > index ..4752eab39ce3
> > > --- /dev/null
> > > +++ b/Platform/Microsoft/OpteeClientPkg/OpteeClientPkg.dec
> > > @@ -0,0 +1,49 @@
> > > +## @file
> > > +#
> > > +#  OP-TEE client package
> > > +#
> > > +#  OP-TEE client package contains the client-side interface to invoke OP-
> TEE TAs.
> > > +#  Certain EDKII services are implemented in Trusted Applications
> > > +running in #  the secure world OP-TEE OS.
> > > +#
> > > +#  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > > +#
> > > +#  This program and the accompanying materials #  are licensed and
> > > +made available under the terms and conditions of the BSD License #
> > > +which accompanies this distribution.  The full text of the license
> > > +may be found at #
> > > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
> > > +nsource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristo
> > >
> +pher.Co%40microsoft.com%7Cc19b84ef7f8f4213424108d63fe88f66%7C72f988
> > >
> +bf86f141af91ab2d7cd011db47%7C1%7C0%7C63675404786500sda
> ta=1
> > > +MxFvlsMPhk19grEexBXo5VqRd0jZaCSRjxZCi87A2w%3Dreserved=0
> > > +#
> > > +#  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]
> > > +  DEC_SPECIFICATION  = 0x0001001A
> > > +  PACKAGE_NAME   = OpteeClientPkg
> > > +  PACKAGE_GUID   = 77416fcb-10ec-4693-bdc0-1bdd74ec9595
> > > +  PACKAGE_VERSION= 0.01
> > > +
> > > +[Includes]
> > > +
> > > +[LibraryClasses]
> > > +
> > > +[Guids]
> > > +  gOpteeClientPkgTokenSpaceGuid   = { 0x04ad34ca, 0xdd25, 0x4156, {
> 0x90, 0xf5, 0x16, 0xf9, 0x40, 0xd0, 0x49, 0xe3 }}
> > > +
> > > +[PcdsFixedAtBuild]
> > > +
> > >
> +gOpteeClientPkgTokenSpaceGuid.PcdTpm2AcpiBufferBase|0|UINT64|0x
> > > +0005
> > > +
> > >
> +gOpteeClientPkgTokenSpaceGuid.PcdTpm2AcpiBufferSize|0|UINT32|0x
> > > +0006
> > > +
> > > +  ## The base address of the Trust Zone OpTEE OS private memory
> > > + region  # This memory is manager 

Re: [edk2] Another Test Message - please ignore

2018-10-29 Thread Chris Co via edk2-devel
Testing Reply-All

Chris

> -Original Message-
> From: edk2-devel  On Behalf Of Kinney,
> Michael D
> Sent: Monday, October 29, 2018 3:44 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Another Test Message - please ignore
> 
> Start new test message thread.
> 
> Mike
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.o
> rg%2Fmailman%2Flistinfo%2Fedk2-
> develdata=02%7C01%7Cchristopher.co%40microsoft.com%7C3b7c858a6
> e34427ebfd308d63df007f3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0
> %7C636764498470253846sdata=D96MYFbrcqLcaZr72ukFJ6XYenR4z50rD
> hGNopmHgNc%3Dreserved=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Test message. Please ignore.

2018-10-29 Thread Chris Co via edk2-devel
Hi Mike,

This is a Reply-All.

Chris

> -Original Message-
> From: Kinney, Michael D 
> Sent: Monday, October 29, 2018 1:11 PM
> To: EDK II Development ; Gretzinger, Adam R
> ; Jeremiah Cox ;
> Kinney, Michael D 
> Cc: Chris Co ; Chad Mace
> ; Sean Brogan ;
> Cetola, Stephano 
> Subject: RE: Test message. Please ignore.
> 
> Hi Chris,
> 
> I am trying a different configuration setting to see if the posters reply-to
> address can be preserved.
> 
> Please try both a Reply and a Reply-All to this test message.
> 
> Thanks,
> 
> Mike
> 
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Monday, October 29, 2018 11:32 AM
> > To: EDK II Development ; Gretzinger, Adam R
> > ; Jeremiah Cox ;
> > Kinney, Michael D 
> > Cc: Chris Co ; Chad Mace
> > ; Sean Brogan ;
> > Cetola, Stephano 
> > Subject: RE: Test message. Please ignore.
> >
> > Chris,
> >
> > Thanks.  That looks like what I expected.
> >
> > Mike
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel- boun...@lists.01.org] On Behalf
> > > Of Chris Co via edk2- devel
> > > Sent: Monday, October 29, 2018 11:22 AM
> > > To: Kinney, Michael D ; Gretzinger, Adam
> > > R ;
> > edk2-
> > > de...@lists.01.org; Jeremiah Cox
> > 
> > > Cc: Chris Co ; Chad Mace
> > > ; Sean Brogan ;
> > > Cetola, Stephano 
> > > Subject: Re: [edk2] Test message. Please ignore.
> > >
> > > Hi Mike,
> > >
> > > Here is a Reply All to the test message.
> > >
> > > Chris
> > >
> > > > -Original Message-
> > > > From: Kinney, Michael D 
> > > > Sent: Monday, October 29, 2018 10:31 AM
> > > > To: Chris Co ;
> > > Gretzinger, Adam R
> > > > ; edk2-
> > > de...@lists.01.org; Kinney, Michael D
> > > > ; Jeremiah Cox
> > > 
> > > > Cc: Sean Brogan ; Cetola,
> > > Stephano
> > > > ; Chad Mace
> > > 
> > > > Subject: RE: Test message. Please ignore.
> > > >
> > > > Hi Chris,
> > > >
> > > > This is a response to this test message.  The Reply-
> > To
> > > setting has been
> > > > updated to make sure edk2-devel is always present.
> > > >
> > > > Please do a Reply All to this test message.
> > > >
> > > > Thanks,
> > > >
> > > > Mike
> > > >
> > > > From: Chris Co [mailto:christopher...@microsoft.com]
> > > > Sent: Friday, October 26, 2018 6:09 PM
> > > > To: Kinney, Michael D ;
> > > Gretzinger, Adam R
> > > > ; edk2-
> > de...@lists.01.org
> > > > Cc: Sean Brogan ;
> > Jeremiah
> > > Cox
> > > > ; Cetola, Stephano
> > > ;
> > > > Chad Mace 
> > > > Subject: Test message. Please ignore.
> > > >
> > > > Test message. Checking for DMARC bounces...
> > > >
> > > > Chris
> > > ___
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> develdata=02%7C01%7CChris
> > >
> topher.Co%40microsoft.com%7C868cbd744373450b6b3b08d63dda97d8%7C72
> f98
> > >
> 8bf86f141af91ab2d7cd011db47%7C1%7C0%7C636764406403589714sda
> ta=G
> > > AoQ6V1QtnZJ1DDVKRFy2kvjtPJfWyEM6Pd4KGgzzf4%3Dreserved=0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Test message. Please ignore.

2018-10-29 Thread Chris Co via edk2-devel
Hi Mike,

Here is a Reply All to the test message.

Chris

> -Original Message-
> From: Kinney, Michael D 
> Sent: Monday, October 29, 2018 10:31 AM
> To: Chris Co ; Gretzinger, Adam R
> ; edk2-devel@lists.01.org; Kinney, Michael D
> ; Jeremiah Cox 
> Cc: Sean Brogan ; Cetola, Stephano
> ; Chad Mace 
> Subject: RE: Test message. Please ignore.
> 
> Hi Chris,
> 
> This is a response to this test message.  The Reply-To setting has been
> updated to make sure edk2-devel is always present.
> 
> Please do a Reply All to this test message.
> 
> Thanks,
> 
> Mike
> 
> From: Chris Co [mailto:christopher...@microsoft.com]
> Sent: Friday, October 26, 2018 6:09 PM
> To: Kinney, Michael D ; Gretzinger, Adam R
> ; edk2-devel@lists.01.org
> Cc: Sean Brogan ; Jeremiah Cox
> ; Cetola, Stephano ;
> Chad Mace 
> Subject: Test message. Please ignore.
> 
> Test message. Checking for DMARC bounces...
> 
> Chris
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] Test message. Please ignore.

2018-10-26 Thread Chris Co via edk2-devel
Test message. Checking for DMARC bounces...

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