Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add MM memory allocation library.

2018-05-04 Thread Supreeth Venkatesh 
Jiewen,

It checks for HOB by  GUID gMmCoreDataHobGuid first,
if it's not present, then it checks for HOB by GUID 
gEfiMmPeiMmramMemoryReserveGuid. (which is the case for Arm and intel won't be 
affected by this)
So it is as generic as possible and hence I think there is no need for ARM 
specific and intel specific funtions.
Please review it and let me know if you see issues.

Thanks,
Supreeth

-Original Message-
From: Yao, Jiewen <jiewen@intel.com>
Sent: Thursday, April 26, 2018 8:06 AM
To: Achin Gupta <achin.gu...@arm.com>; Supreeth Venkatesh 
<supreeth.venkat...@arm.com>
Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org; 
leif.lindh...@linaro.org; Gao, Liming <liming@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com>; nd <n...@arm.com>
Subject: RE: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add 
MM memory allocation library.

Same comment as previous 2.

Maybe to separate ARM specific function from generic function.

Thank you



> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Achin
> Gupta
> Sent: Wednesday, April 25, 2018 7:34 AM
> To: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org;
> leif.lindh...@linaro.org; Yao, Jiewen <jiewen@intel.com>; Gao, Liming
> <liming@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> n...@arm.com
> Subject: Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib:
> Add MM memory allocation library.
>
> Hi Supreeth,
>
> On Fri, Apr 06, 2018 at 03:42:14PM +0100, Supreeth Venkatesh wrote:
> > This patch implements management mode memory allocation services.The
> > implementation borrows the MM Core Memory Allocation services as the
> > primitive for memory allocation instead of using MM System Table
> > services.
>
> The commit message did not really register with me. Once the MMRAM ranges
> have
> been conveyed to the MMST memory allocation services (through
> MemoryAllocationLibConstructor()), all functions in MemoryAllocationLib.c use
> the MMST services. The message seems to indicate otherwise.
>
> On Arm, the gEfiMmPeiMmramMemoryReserveGuid HOB is used to convey the
> MMRAM
> ranges. It seems x86 uses gMmCoreDataHobGuid HOB. So it worth getting this
> reviewed by Jiewen.
>
> The copyright years in the files need to be updated.
>
> With that in mind..
>
> Acked-by: Achin Gupta <achin.gu...@arm.com>
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Achin Gupta <achin.gu...@arm.com>
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> > ---
> >  StandaloneMmPkg/Include/Guid/MmCoreData.h  | 132 +++
> >  StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h  |  62 ++
> >  .../MemoryAllocationLib/MemoryAllocationLib.c  | 907
> +
> >  .../MemoryAllocationLib/MemoryAllocationLib.inf|  49 ++
> >  .../MemoryAllocationLib/MemoryAllocationServices.h |  38 +
> >  5 files changed, 1188 insertions(+)
> >  create mode 100644 StandaloneMmPkg/Include/Guid/MmCoreData.h
> >  create mode 100644
> StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.c
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.inf
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationServices.h
> >
> > diff --git a/StandaloneMmPkg/Include/Guid/MmCoreData.h
> b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> > new file mode 100644
> > index 00..c0ac772014
> > --- /dev/null
> > +++ b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> > @@ -0,0 +1,132 @@
> > +/** @file
> > +  MM Core data.
> > +
> > +Copyright (c) 2015, Intel Corporation. All rights reserved.
> > +This program and the accompanying materials are licensed and made
> available under
> > +the terms and conditions of the BSD License that accompanies this
> distribution.
> > +The full text of the license may be found at
> > +http://opensource.org/licenses/bsd-license.php.
> > +
> > +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 __MM_CORE_DATA_H__
> > +#define __MM_CORE_DATA_H__
> > +
> > +#define MM_CORE_DATA_HOB_GUID \
> > +  { 0xa160bf99, 0x2aa4, 0x4d7d, { 0x99, 0x93, 0x89, 0x9c, 0xb1, 0x2d, 0xf3,
> 0x76 }}

Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add MM memory allocation library.

2018-05-04 Thread Supreeth Venkatesh 
My response inline.

-Original Message-
From: Achin Gupta
Sent: Wednesday, April 25, 2018 9:34 AM
To: Supreeth Venkatesh 
Cc: edk2-devel@lists.01.org; michael.d.kin...@intel.com; liming@intel.com; 
jiewen@intel.com; leif.lindh...@linaro.org; ard.biesheu...@linaro.org; nd 

Subject: Re: [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add MM 
memory allocation library.

Hi Supreeth,

On Fri, Apr 06, 2018 at 03:42:14PM +0100, Supreeth Venkatesh wrote:
> This patch implements management mode memory allocation services.The
> implementation borrows the MM Core Memory Allocation services as the
> primitive for memory allocation instead of using MM System Table
> services.

The commit message did not really register with me. Once the MMRAM ranges have 
been conveyed to the MMST memory allocation services (through 
MemoryAllocationLibConstructor()), all functions in MemoryAllocationLib.c use 
the MMST services. The message seems to indicate otherwise.

On Arm, the gEfiMmPeiMmramMemoryReserveGuid HOB is used to convey the MMRAM 
ranges. It seems x86 uses gMmCoreDataHobGuid HOB. So it worth getting this 
reviewed by Jiewen.

The copyright years in the files need to be updated.
[Supreeth] Ok. Done.

With that in mind..

Acked-by: Achin Gupta 

>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  StandaloneMmPkg/Include/Guid/MmCoreData.h  | 132 +++
>  StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h  |  62 ++
>  .../MemoryAllocationLib/MemoryAllocationLib.c  | 907 
> +
>  .../MemoryAllocationLib/MemoryAllocationLib.inf|  49 ++
>  .../MemoryAllocationLib/MemoryAllocationServices.h |  38 +
>  5 files changed, 1188 insertions(+)
>  create mode 100644 StandaloneMmPkg/Include/Guid/MmCoreData.h
>  create mode 100644 StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h
>  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.c
>  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.inf
>  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationServices.h
>
> diff --git a/StandaloneMmPkg/Include/Guid/MmCoreData.h
> b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> new file mode 100644
> index 00..c0ac772014
> --- /dev/null
> +++ b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> @@ -0,0 +1,132 @@
> +/** @file
> +  MM Core data.
> +
> +Copyright (c) 2015, Intel Corporation. All rights reserved. This
> +program and the accompanying materials are licensed and made
> +available under the terms and conditions of the BSD License that accompanies 
> this distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +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 __MM_CORE_DATA_H__
> +#define __MM_CORE_DATA_H__
> +
> +#define MM_CORE_DATA_HOB_GUID \
> +  { 0xa160bf99, 0x2aa4, 0x4d7d, { 0x99, 0x93, 0x89, 0x9c, 0xb1, 0x2d,
> +0xf3, 0x76 }}
> +
> +extern EFI_GUID gMmCoreDataHobGuid;
> +
> +typedef struct {
> +  //
> +  // Address pointer to MM_CORE_PRIVATE_DATA
> +  //
> +  EFI_PHYSICAL_ADDRESS   Address;
> +} MM_CORE_DATA_HOB_DATA;
> +
> +
> +///
> +/// Define values for the communications buffer used when
> +gEfiEventDxeDispatchGuid is /// event signaled.  This event is
> +signaled by the DXE Core each time the DXE Core /// dispatcher has
> +completed its work.  When this event is signaled, the MM Core /// if
> +notified, so the MM Core can dispatch MM drivers.  If
> +COMM_BUFFER_MM_DISPATCH_ERROR /// is returned in the communication
> +buffer, then an error occurred dispatching MM /// Drivers.  If
> +COMM_BUFFER_MM_DISPATCH_SUCCESS is returned, then the MM Core ///
> +dispatched all the drivers it could.  If
> +COMM_BUFFER_MM_DISPATCH_RESTART is /// returned, then the MM Core just 
> dispatched the MM Driver that registered /// the MM Entry Point enabling the 
> use of MM Mode.  In this case, the MM Core /// should be notified again to 
> dispatch more MM Drivers using MM Mode.
> +///
> +#define COMM_BUFFER_MM_DISPATCH_ERROR0x00
> +#define COMM_BUFFER_MM_DISPATCH_SUCCESS  0x01 #define
> +COMM_BUFFER_MM_DISPATCH_RESTART  0x02
> +
> +///
> +/// Signature for the private structure shared between the MM IPL and
> +the MM Core /// #define MM_CORE_PRIVATE_DATA_SIGNATURE  SIGNATURE_32
> +('m', 'm', 'i', 'c')
> +
> +///
> +/// Private structure that is used to share information between the
> +MM IPL and /// the MM Core.  This structure is allocated from memory of type 
> EfiRuntimeServicesData.
> +/// Since runtime memory types are converted to available memory when
> +a legacy boot /// is performed, the MM Core must not

Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add MM memory allocation library.

2018-04-26 Thread Yao, Jiewen 
Same comment as previous 2.

Maybe to separate ARM specific function from generic function.

Thank you



> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Achin
> Gupta
> Sent: Wednesday, April 25, 2018 7:34 AM
> To: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> Cc: ard.biesheu...@linaro.org; edk2-devel@lists.01.org;
> leif.lindh...@linaro.org; Yao, Jiewen <jiewen@intel.com>; Gao, Liming
> <liming@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> n...@arm.com
> Subject: Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib:
> Add MM memory allocation library.
> 
> Hi Supreeth,
> 
> On Fri, Apr 06, 2018 at 03:42:14PM +0100, Supreeth Venkatesh wrote:
> > This patch implements management mode memory allocation services.The
> > implementation borrows the MM Core Memory Allocation services as the
> > primitive for memory allocation instead of using MM System Table
> > services.
> 
> The commit message did not really register with me. Once the MMRAM ranges
> have
> been conveyed to the MMST memory allocation services (through
> MemoryAllocationLibConstructor()), all functions in MemoryAllocationLib.c use
> the MMST services. The message seems to indicate otherwise.
> 
> On Arm, the gEfiMmPeiMmramMemoryReserveGuid HOB is used to convey the
> MMRAM
> ranges. It seems x86 uses gMmCoreDataHobGuid HOB. So it worth getting this
> reviewed by Jiewen.
> 
> The copyright years in the files need to be updated.
> 
> With that in mind..
> 
> Acked-by: Achin Gupta <achin.gu...@arm.com>
> 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Achin Gupta <achin.gu...@arm.com>
> > Signed-off-by: Supreeth Venkatesh <supreeth.venkat...@arm.com>
> > ---
> >  StandaloneMmPkg/Include/Guid/MmCoreData.h  | 132 +++
> >  StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h  |  62 ++
> >  .../MemoryAllocationLib/MemoryAllocationLib.c  | 907
> +
> >  .../MemoryAllocationLib/MemoryAllocationLib.inf|  49 ++
> >  .../MemoryAllocationLib/MemoryAllocationServices.h |  38 +
> >  5 files changed, 1188 insertions(+)
> >  create mode 100644 StandaloneMmPkg/Include/Guid/MmCoreData.h
> >  create mode 100644
> StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.c
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.inf
> >  create mode 100644
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationServices.h
> >
> > diff --git a/StandaloneMmPkg/Include/Guid/MmCoreData.h
> b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> > new file mode 100644
> > index 00..c0ac772014
> > --- /dev/null
> > +++ b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> > @@ -0,0 +1,132 @@
> > +/** @file
> > +  MM Core data.
> > +
> > +Copyright (c) 2015, Intel Corporation. All rights reserved.
> > +This program and the accompanying materials are licensed and made
> available under
> > +the terms and conditions of the BSD License that accompanies this
> distribution.
> > +The full text of the license may be found at
> > +http://opensource.org/licenses/bsd-license.php.
> > +
> > +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 __MM_CORE_DATA_H__
> > +#define __MM_CORE_DATA_H__
> > +
> > +#define MM_CORE_DATA_HOB_GUID \
> > +  { 0xa160bf99, 0x2aa4, 0x4d7d, { 0x99, 0x93, 0x89, 0x9c, 0xb1, 0x2d, 0xf3,
> 0x76 }}
> > +
> > +extern EFI_GUID gMmCoreDataHobGuid;
> > +
> > +typedef struct {
> > +  //
> > +  // Address pointer to MM_CORE_PRIVATE_DATA
> > +  //
> > +  EFI_PHYSICAL_ADDRESS   Address;
> > +} MM_CORE_DATA_HOB_DATA;
> > +
> > +
> > +///
> > +/// Define values for the communications buffer used when
> gEfiEventDxeDispatchGuid is
> > +/// event signaled.  This event is signaled by the DXE Core each time the 
> > DXE
> Core
> > +/// dispatcher has completed its work.  When this event is signaled, the MM
> Core
> > +/// if notified, so the MM Core can dispatch MM drivers.  If
> COMM_BUFFER_MM_DISPATCH_ERROR
> > +/// is returned in the communication buffer, then an error occurred
> dispatching MM
> > +/// Drivers.  If COMM_BUFFER_MM_DISPATCH_SUCCESS is returned, then
> the MM Core
&g

Re: [edk2] [PATCH v1 09/18] StandaloneMmPkg/MemoryAllocationLib: Add MM memory allocation library.

2018-04-25 Thread Achin Gupta 
Hi Supreeth,

On Fri, Apr 06, 2018 at 03:42:14PM +0100, Supreeth Venkatesh wrote:
> This patch implements management mode memory allocation services.The
> implementation borrows the MM Core Memory Allocation services as the
> primitive for memory allocation instead of using MM System Table
> services.

The commit message did not really register with me. Once the MMRAM ranges have
been conveyed to the MMST memory allocation services (through
MemoryAllocationLibConstructor()), all functions in MemoryAllocationLib.c use
the MMST services. The message seems to indicate otherwise.

On Arm, the gEfiMmPeiMmramMemoryReserveGuid HOB is used to convey the MMRAM
ranges. It seems x86 uses gMmCoreDataHobGuid HOB. So it worth getting this
reviewed by Jiewen.

The copyright years in the files need to be updated.

With that in mind..

Acked-by: Achin Gupta 

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  StandaloneMmPkg/Include/Guid/MmCoreData.h  | 132 +++
>  StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h  |  62 ++
>  .../MemoryAllocationLib/MemoryAllocationLib.c  | 907 
> +
>  .../MemoryAllocationLib/MemoryAllocationLib.inf|  49 ++
>  .../MemoryAllocationLib/MemoryAllocationServices.h |  38 +
>  5 files changed, 1188 insertions(+)
>  create mode 100644 StandaloneMmPkg/Include/Guid/MmCoreData.h
>  create mode 100644 StandaloneMmPkg/Include/Guid/MmramMemoryReserve.h
>  create mode 100644 
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.c
>  create mode 100644 
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationLib.inf
>  create mode 100644 
> StandaloneMmPkg/Library/MemoryAllocationLib/MemoryAllocationServices.h
> 
> diff --git a/StandaloneMmPkg/Include/Guid/MmCoreData.h 
> b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> new file mode 100644
> index 00..c0ac772014
> --- /dev/null
> +++ b/StandaloneMmPkg/Include/Guid/MmCoreData.h
> @@ -0,0 +1,132 @@
> +/** @file
> +  MM Core data.
> +
> +Copyright (c) 2015, Intel Corporation. All rights reserved.
> +This program and the accompanying materials are licensed and made available 
> under
> +the terms and conditions of the BSD License that accompanies this 
> distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +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 __MM_CORE_DATA_H__
> +#define __MM_CORE_DATA_H__
> +
> +#define MM_CORE_DATA_HOB_GUID \
> +  { 0xa160bf99, 0x2aa4, 0x4d7d, { 0x99, 0x93, 0x89, 0x9c, 0xb1, 0x2d, 0xf3, 
> 0x76 }}
> +
> +extern EFI_GUID gMmCoreDataHobGuid;
> +
> +typedef struct {
> +  //
> +  // Address pointer to MM_CORE_PRIVATE_DATA
> +  //
> +  EFI_PHYSICAL_ADDRESS   Address;
> +} MM_CORE_DATA_HOB_DATA;
> +
> +
> +///
> +/// Define values for the communications buffer used when 
> gEfiEventDxeDispatchGuid is
> +/// event signaled.  This event is signaled by the DXE Core each time the 
> DXE Core
> +/// dispatcher has completed its work.  When this event is signaled, the MM 
> Core
> +/// if notified, so the MM Core can dispatch MM drivers.  If 
> COMM_BUFFER_MM_DISPATCH_ERROR
> +/// is returned in the communication buffer, then an error occurred 
> dispatching MM
> +/// Drivers.  If COMM_BUFFER_MM_DISPATCH_SUCCESS is returned, then the MM 
> Core
> +/// dispatched all the drivers it could.  If COMM_BUFFER_MM_DISPATCH_RESTART 
> is
> +/// returned, then the MM Core just dispatched the MM Driver that registered
> +/// the MM Entry Point enabling the use of MM Mode.  In this case, the MM 
> Core
> +/// should be notified again to dispatch more MM Drivers using MM Mode.
> +///
> +#define COMM_BUFFER_MM_DISPATCH_ERROR0x00
> +#define COMM_BUFFER_MM_DISPATCH_SUCCESS  0x01
> +#define COMM_BUFFER_MM_DISPATCH_RESTART  0x02
> +
> +///
> +/// Signature for the private structure shared between the MM IPL and the MM 
> Core
> +///
> +#define MM_CORE_PRIVATE_DATA_SIGNATURE  SIGNATURE_32 ('m', 'm', 'i', 'c')
> +
> +///
> +/// Private structure that is used to share information between the MM IPL 
> and
> +/// the MM Core.  This structure is allocated from memory of type 
> EfiRuntimeServicesData.
> +/// Since runtime memory types are converted to available memory when a 
> legacy boot
> +/// is performed, the MM Core must not access any fields of this structure 
> if a legacy
> +/// boot is performed.  As a result, the MM IPL must create an event 
> notification
> +/// for the Legacy Boot event and notify the MM Core that a legacy boot is 
> being
> +/// performed.  The MM Core can then use this information to filter accesses 
> to
> +/// thos structure.
> +///
> +typedef struct {
> +  UINT64  Signature;
> +
> +  ///
> +