Re: [PATCH] MdeModulePkg: Add EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Hi Laszlo and Leo,

If you look at the CpuSetMemoryAttributes() function in 
UefiCpuPkg/CpuDxe/CpuDxe.c, you will see it now calls
AssignMemoryPageAttributes() at the end.  This function 
is implemented in UefiCpuPkg/CpuDxe/CpuPageTable.c and it
updates page table attributes based on the request.

This looks like a very similar action to update PTEs.

What MMIO areas are we discussing here?  MMIO ranges
associated with PCI BARs allocated through PCI Bus Driver
and PCI Host Bridge driver?

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 12, 2017 1:33 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Leo Duran
> <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: Tian, Feng <feng.tian@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Add
> EFI_GCD_MEMORY_SPACE_NOTIFY_PROTOCOL
> 
> Mike,
> 
> On 04/12/17 21:39, Kinney, Michael D wrote:
> > 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?
> >
> > 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?
> 
> The goal is to clear the "C" bit in the PTEs (--> disable encryption in
> SEV guests) for all MMIO areas in the GCD memory space map, when they
> are added, and to re-set the "C" bit when the area is removed (or the
> area's GCD type is changed to something non-MMIO).
> 
> This behavior should cover all possible gDS->AddMemorySpace() calls (for
> the MMIO type), regardless of what driver calls gDS->AddMemorySpace().
> In other words, drivers that add MMIO space should remain unaware of SEV
> / the "C" bit in PTEs. Otherwise we'd have to enable SEV detection and
> "C" bit massaging in every affected driver individually (even future
> ones, possibly pulled in from under MdeModulePkg or UefiCpuPkg).
> Cacheability and similar attributes for memory ranges are quite
> universal on all architectures, and it is okay to expect all drivers to
> manage those attributes manually (whenever that's necessary), but (a)
> SEV / memory encryption is not that universal, and (b) clearing "C" for
> MMIO ranges affects all drivers alike, so it would be good to generalize
> it at a low level.
> 
> What would be the best general location or API to hook the "C" bit
> manipulation into MMIO GCD memory space addition / removal?
> 
> Thank you,
> Laszlo
> 
> 
> >
> > 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