Re: [PATCH] MdeModulePkg: Add EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL

["Duran, Leo" <leo.duran@amd.com>]

Hi Mike,

Please see my replies below.

Leo.

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Wednesday, April 12, 2017 2:39 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Tian, Feng <feng.tian@intel.com>;
> Singh, Brijesh <brijesh.singh@amd.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH] MdeModulePkg: Add
> EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL
> 
> Hi Leo,
> 
> DXE Main is supposed to use Arch Protocols for platform specific behavior.
> 
> In this case, can the CPU Arch Protocol SetMemoryAttributes() function be
> used to customize behavior?

[Duran, Leo]  DxeMain supports a list of Arch protocols and also a list of optional protocols.
SetMemoryAttributes() is a service provided by GCD, via CoreConvertSpace(), just as Add/RemoveMemorySpace().
The behavior we need is a notification into OvmfPkg as GCD gets called to Add/RemoveMemorySpace(), not just if/when GCD gets called to SetMemoryAttributes().
BTW, SetMemoryAttributes() may get called on any range, without specifying type (whereas we're interested in MMIO ranges, which are specified in ADD operations)

> 
> In other modules that add/remove/modify MMIO, gDS-
> >SetMemorySpaceAttributes() Is called to set memory space attributed such
> as cachability.  Can the module that is adding/removing memory regions from
> GCD also call
> gDS-> SetMemorySpaceAttributes() so a CPU specific module can perform
> extra actions?
> 
> Thanks,
> 
> Mike
> 
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Leo Duran
> > Sent: Wednesday, April 12, 2017 11:57 AM
> > To: edk2-devel@ml01.01.org
> > Cc: Laszlo Ersek <lersek@redhat.com>; Tian, Feng
> > <feng.tian@intel.com>; Leo Duran <leo.duran@amd.com>; Brijesh Singh
> > <brijesh.singh@amd.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: [edk2] [PATCH] MdeModulePkg: Add
> > EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL
> >
> > GCD consumes the protocol to issue a Notify() on Add/Remove operations.
> >
> > The intended use-case is to allow OvmfPkg take actions on behalf of an
> > SEV-enabled guest.
> >
> > The new protocol is simply added to the list of optional protocols
> > handled by DxeMain, and as such leverages the existing DxeProtocolNotify
> framework.
> >
> > Cc: Feng Tian <feng.tian@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h                    | 10 +++-
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                  |  4 ++
> >  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c            |  7 +++
> >  MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c  |  9 ++-
> >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c                    |  8 +++
> >  .../Include/Protocol/GcdMemorySpaceNotify.h        | 65
> ++++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                      |  3 +
> >  7 files changed, 100 insertions(+), 6 deletions(-)  create mode
> > 100644 MdeModulePkg/Include/Protocol/GcdMemorySpaceNotify.h
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > b/MdeModulePkg/Core/Dxe/DxeMain.h index 1a0babb..8a037ff 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -3,6 +3,8 @@
> >    internal structure and functions used by DxeCore module.
> >
> >  Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> > +
> >  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 @@ -17,7 +19,6 @@ WITHOUT WARRANTIES OR
> REPRESENTATIONS OF
> > ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  #define _DXE_MAIN_H_
> >
> >
> > -
> >  #include <PiDxe.h>
> >
> >  #include <Protocol/LoadedImage.h>
> > @@ -53,6 +54,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND,
> > EITHER EXPRESS OR IMPLIED.
> >  #include <Protocol/TcgService.h>
> >  #include <Protocol/HiiPackageList.h>
> >  #include <Protocol/SmmBase2.h>
> > +#include <Protocol/GcdMemorySpaceNotify.h>
> > +
> >  #include <Guid/MemoryTypeInformation.h>  #include
> > <Guid/FirmwareFileSystem2.h>  #include <Guid/FirmwareFileSystem3.h>
> @@
> > -296,12 +299,13 @@ extern EFI_RUNTIME_ARCH_PROTOCOL
> gRuntimeTemplate;
> >
> >  extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
> > gLoadModuleAtFixAddressConfigurationTable;
> >  extern BOOLEAN
> > gLoadFixedAddressCodeMemoryReady;
> > +
> > +extern EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL
> *gGcdMemorySpaceNotify;
> > +
> >  //
> >  // Service Initialization Functions
> >  //
> >
> > -
> > -
> >  /**
> >    Called to initialize the pool.
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 30d5984..888a16f 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -4,6 +4,8 @@
> >  #  It provides an implementation of DXE Core that is compliant with DXE
> CIS.
> >  #
> >  #  Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +#  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
> >  #  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 @@ -162,6 +164,8 @@
> >    gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES
> >    gEfiBlockIoProtocolGuid                       ## SOMETIMES_CONSUMES
> >
> > +  gEfiGcdMemorySpaceNotifyProtocolGuid          ## CONSUMES
> > +
> >    # Arch Protocols
> >    gEfiBdsArchProtocolGuid                       ## CONSUMES
> >    gEfiCpuArchProtocolGuid                       ## CONSUMES
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > index 91e94a7..46b68da 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > @@ -2,6 +2,8 @@
> >    DXE Core Main Entry Point
> >
> >  Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> > +
> >  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
> > @@ -42,6 +44,11 @@ EFI_GUID                           *gDxeCoreFileName;
> >  EFI_LOADED_IMAGE_PROTOCOL          *gDxeCoreLoadedImage;
> >
> >  //
> > +// DXE Core global for GCD notification protocol //
> > +EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL
> *gGcdMemorySpaceNotify = NULL;
> > +
> > +//
> >  // DXE Core Module Variables
> >  //
> >  EFI_BOOT_SERVICES mBootServices = {
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > index ea7c610..2314e34 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > @@ -4,6 +4,8 @@
> >    events that represent the Architectural Protocols.
> >
> >  Copyright (c) 2006 - 2014, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> > +
> >  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 @@ -45,9 +47,10 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> > mArchProtocols[] = {  // Optional protocols that the DXE Core will use
> > if they are present  //  EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> > mOptionalProtocols[] = {
> > -  { &gEfiSecurity2ArchProtocolGuid,        (VOID **)&gSecurity2,     NULL,
> NULL,
> > FALSE },
> > -  { &gEfiSmmBase2ProtocolGuid,             (VOID **)&gSmmBase2,      NULL,
> NULL,
> > FALSE },
> > -  { NULL,                                  (VOID **)NULL,            NULL, NULL,
> > FALSE }
> > +  { &gEfiSecurity2ArchProtocolGuid,        (VOID **)&gSecurity2,
> > NULL, NULL, FALSE },
> > +  { &gEfiSmmBase2ProtocolGuid,             (VOID **)&gSmmBase2,
> > NULL, NULL, FALSE },
> > +  { &gEfiGcdMemorySpaceNotifyProtocolGuid, (VOID
> > + **)&gGcdMemorySpaceNotify,
> > NULL, NULL, FALSE },
> > +  { NULL,                                  (VOID **)NULL,
> > NULL, NULL, FALSE }
> >  };
> >
> >  //
> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index a06f8bb..223fcd8 100644
> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > @@ -4,6 +4,8 @@
> >    are accessible to the CPU that is executing the DXE core.
> >
> >  Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> > +
> >  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 @@ -896,6 +898,9 @@ CoreConvertSpace (
> >        } else {
> >          Entry->Capabilities  = Capabilities | EFI_MEMORY_RUNTIME;
> >        }
> > +      if (gGcdMemorySpaceNotify) {
> > +        gGcdMemorySpaceNotify->MemorySpaceAddNotify
> (GcdMemoryType,
> > + BaseAddress,
> > Length, Entry->Capabilities);
> > +      }
> >        break;
> >      case GCD_ADD_IO_OPERATION:
> >        Entry->GcdIoType = GcdIoType;
> > @@ -914,6 +919,9 @@ CoreConvertSpace (
> >      case GCD_REMOVE_MEMORY_OPERATION:
> >        Entry->GcdMemoryType = EfiGcdMemoryTypeNonExistent;
> >        Entry->Capabilities  = 0;
> > +      if (gGcdMemorySpaceNotify) {
> > +        gGcdMemorySpaceNotify->MemorySpaceRemoveNotify
> (BaseAddress, Length);
> > +      }
> >        break;
> >      case GCD_REMOVE_IO_OPERATION:
> >        Entry->GcdIoType = EfiGcdIoTypeNonExistent; diff --git
> > a/MdeModulePkg/Include/Protocol/GcdMemorySpaceNotify.h
> > b/MdeModulePkg/Include/Protocol/GcdMemorySpaceNotify.h
> > new file mode 100644
> > index 0000000..9174957
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/GcdMemorySpaceNotify.h
> > @@ -0,0 +1,65 @@
> > +/** @file
> > +  This file declares the GcdMemorySpaceNotify Protocol.
> > +
> > +  This Protocol is consumed by GCD to issue notications during
> > + ADD/REMOVE
> > operations.
> > +
> > +  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> > +
> > +  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 __EFI_GCD_MEMORY_SPACE_NOTIFY_H__ #define
> > +__EFI_GCD_MEMORY_SPACE_NOTIFY_H__
> > +
> > +#define EFI_GCD_MEMORY_SPACE_NOTIFY_GUID \
> > +  { \
> > +    0xc842db69, 0x610e, 0x401a, {0x90, 0xd0, 0x88, 0x41, 0xf1, 0xdc,
> > +0x53, 0x79
> > } \
> > +  }
> > +
> > +/**
> > +  Notify on: Add a segment of memory to GCD map.
> > +
> > +  @param  GcdMemoryType          Memory type of the segment.
> > +  @param  BaseAddress            Base address of the segment.
> > +  @param  Length                 Length of the segment.
> > +  @param  Capabilities           Alterable attributes of the segment.
> > +
> > +**/
> > +typedef
> > +VOID
> > +(EFIAPI *GCD_ADD_MEMORY_NOTIFY) (
> > +  IN EFI_GCD_MEMORY_TYPE   GcdMemoryType,
> > +  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
> > +  IN UINT64                Length,
> > +  IN UINT64                Capabilities
> > +);
> > +
> > +/**
> > +  Notify on: Remove a segment of memory to GCD map.
> > +
> > +  @param  BaseAddress            Base address of the segment.
> > +  @param  Length                 Length of the segment.
> > +
> > +**/
> > +typedef
> > +VOID
> > +(EFIAPI *GCD_REMOVE_MEMORY_NOTIFY) (
> > +  IN EFI_PHYSICAL_ADDRESS  BaseAddress,
> > +  IN UINT64                Length
> > +);
> > +
> > +typedef struct {
> > +  GCD_ADD_MEMORY_NOTIFY     MemorySpaceAddNotify;
> > +  GCD_REMOVE_MEMORY_NOTIFY  MemorySpaceRemoveNotify; }
> > +EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL;
> > +
> > +extern EFI_GUID gEfiGcdMemorySpaceNotifyProtocolGuid;
> > +
> > +#endif
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index ca09cbc..95f9311 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -546,6 +546,9 @@
> >    ## Include/Protocol/NonDiscoverableDevice.h
> >    gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e,
> > 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
> >
> > +  ## Include/Protocol/GcdMemorySpaceNotify.h
> > +  gEfiGcdMemorySpaceNotifyProtocolGuid  = { 0xc842db69, 0x610e,
> > + 0x401a, {0x90,
> > 0xd0, 0x88, 0x41, 0xf1, 0xdc, 0x53, 0x79 } }
> > +
> >  #
> >  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> >  #   0x80000001 | Invalid value provided.
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel