Laszlo,

Minor comments included below.

Reviewed-by: Michael Kinney <michael.d.kin...@intel.com>

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Tuesday, November 3, 2015 1:01 PM
> To: edk2-de...@ml01.01.org
> Subject: [edk2] [PATCH v4 08/41] OvmfPkg: add DXE_DRIVER for providing 
> TSEG-as-SMRAM during boot-time DXE
> 
> The SMM core depends on EFI_SMM_ACCESS2_PROTOCOL. This small driver (which is 
> a thin wrapper around
> "OvmfPkg/SmmAccess/SmramInternal.c" that was added in the previous patch) 
> provides that protocol.
> 
> Notably, EFI_SMM_ACCESS2_PROTOCOL is for boot time only, therefore our 
> MODULE_TYPE is not DXE_RUNTIME_DRIVER.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc             |   4 +
>  OvmfPkg/OvmfPkgIa32X64.dsc          |   4 +
>  OvmfPkg/OvmfPkgX64.dsc              |   4 +
>  OvmfPkg/OvmfPkgIa32.fdf             |   4 +
>  OvmfPkg/OvmfPkgIa32X64.fdf          |   4 +
>  OvmfPkg/OvmfPkgX64.fdf              |   4 +
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf |  57 +++++++
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.c   | 156 ++++++++++++++++++++
>  8 files changed, 237 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 
> 0b729ca..d7bc38d 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -672,3 +672,7 @@ [Components]
>  !endif
> 
>    OvmfPkg/PlatformDxe/Platform.inf
> +
> +!if $(SMM_REQUIRE) == TRUE
> +  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index 
> 7f672d9..e17cbe5 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -679,3 +679,7 @@ [Components.X64]
>  !endif
> 
>    OvmfPkg/PlatformDxe/Platform.inf
> +
> +!if $(SMM_REQUIRE) == TRUE
> +  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 
> 986c019..a748fb3 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -677,3 +677,7 @@ [Components]
>  !endif
> 
>    OvmfPkg/PlatformDxe/Platform.inf
> +
> +!if $(SMM_REQUIRE) == TRUE
> +  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index 
> 650dab1..285720f 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -355,6 +355,10 @@ [FV.DXEFV]
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
> 
> +!if $(SMM_REQUIRE) == TRUE
> +INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> +
>  
> ################################################################################
> 
>  [FV.FVMAIN_COMPACT]
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index 
> 5830401..02e8752 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -355,6 +355,10 @@ [FV.DXEFV]
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
> 
> +!if $(SMM_REQUIRE) == TRUE
> +INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> +
>  
> ################################################################################
> 
>  [FV.FVMAIN_COMPACT]
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index 
> 9dd6171..f04c36b 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -355,6 +355,10 @@ [FV.DXEFV]
>  INF  OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
>  INF  OvmfPkg/PlatformDxe/Platform.inf
> 
> +!if $(SMM_REQUIRE) == TRUE
> +INF  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> +!endif
> +
>  
> ################################################################################
> 
>  [FV.FVMAIN_COMPACT]
> diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf 
> b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> new file mode 100644
> index 0000000..3ce48ae
> --- /dev/null
> +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> @@ -0,0 +1,57 @@
> +## @file
> +# A DXE_DRIVER providing SMRAM access by producing EFI_SMM_ACCESS2_PROTOCOL.
> +#
> +# Q35 TSEG is expected to have been verified and set up by the
> +SmmAccessPei # driver.
> +#
> +# Copyright (C) 2013, 2015, Red Hat, Inc.
> +#
> +# This program and the accompanying materials are licensed and made
> +available # under the terms and conditions of the BSD License which
> +accompanies this # distribution. The full text of the license may be
> +found at # http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SmmAccess2Dxe
> +  FILE_GUID                      = AC95AD3D-4366-44BF-9A62-E4B29D7A2206
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  PI_SPECIFICATION_VERSION       = 0x00010400
> +  ENTRY_POINT                    = SmmAccess2DxeEntryPoint
> +
> +#
> +# The following information is for reference only and not required by the 
> build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources]
> +  SmmAccess2Dxe.c
> +  SmramInternal.c

SmramInternal.h is missing from [Sources] section.

> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  PcdLib
> +  PciLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gEfiSmmAccess2ProtocolGuid   # PROTOCOL ALWAYS_PRODUCED

Comment should be:       ## PRODUCES

> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> +
> +[Depex]
> +  TRUE
> diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c 
> b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
> new file mode 100644
> index 0000000..d513039
> --- /dev/null
> +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c
> @@ -0,0 +1,156 @@
> +/** @file
> +
> +  A DXE_DRIVER providing SMRAM access by producing EFI_SMM_ACCESS2_PROTOCOL.
> +
> +  Q35 TSEG is expected to have been verified and set up by the
> + SmmAccessPei  driver.
> +
> +  Copyright (C) 2013, 2015, Red Hat, Inc.<BR>  Copyright (c) 2009 -
> + 2010, Intel Corporation. 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.
> +
> +**/
> +
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/SmmAccess2.h>
> +
> +#include "SmramInternal.h"
> +
> +/**
> +  Opens the SMRAM area to be accessible by a boot-service driver.
> +
> +  This function "opens" SMRAM so that it is visible while not inside of SMM.
> +  The function should return EFI_UNSUPPORTED if the hardware does not
> + support  hiding of SMRAM. The function should return EFI_DEVICE_ERROR
> + if the SMRAM  configuration is locked.
> +
> +  @param[in] This           The EFI_SMM_ACCESS2_PROTOCOL instance.
> +
> +  @retval EFI_SUCCESS       The operation was successful.
> +  @retval EFI_UNSUPPORTED   The system does not support opening and closing 
> of
> +                            SMRAM.
> +  @retval EFI_DEVICE_ERROR  SMRAM cannot be opened, perhaps because it is
> +                            locked.
> +**/
> +STATIC

Recommend removing STATIC from public functions.

> +EFI_STATUS
> +EFIAPI
> +SmmAccess2DxeOpen (
> +  IN EFI_SMM_ACCESS2_PROTOCOL  *This
> +  )
> +{
> +  return SmramAccessOpen (&This->LockState, &This->OpenState); }
> +
> +/**
> +  Inhibits access to the SMRAM.
> +
> +  This function "closes" SMRAM so that it is not visible while outside of 
> SMM.
> +  The function should return EFI_UNSUPPORTED if the hardware does not
> + support  hiding of SMRAM.
> +
> +  @param[in] This           The EFI_SMM_ACCESS2_PROTOCOL instance.
> +
> +  @retval EFI_SUCCESS       The operation was successful.
> +  @retval EFI_UNSUPPORTED   The system does not support opening and closing 
> of
> +                            SMRAM.
> +  @retval EFI_DEVICE_ERROR  SMRAM cannot be closed.
> +**/
> +STATIC

Recommend removing STATIC from public functions.

> +EFI_STATUS
> +EFIAPI
> +SmmAccess2DxeClose (
> +  IN EFI_SMM_ACCESS2_PROTOCOL  *This
> +  )
> +{
> +  return SmramAccessClose (&This->LockState, &This->OpenState); }
> +
> +/**
> +  Inhibits access to the SMRAM.
> +
> +  This function prohibits access to the SMRAM region.  This function is
> + usually  implemented such that it is a write-once operation.
> +
> +  @param[in] This          The EFI_SMM_ACCESS2_PROTOCOL instance.
> +
> +  @retval EFI_SUCCESS      The device was successfully locked.
> +  @retval EFI_UNSUPPORTED  The system does not support locking of SMRAM.
> +**/
> +STATIC

Recommend removing STATIC from public functions.

> +EFI_STATUS
> +EFIAPI
> +SmmAccess2DxeLock (
> +  IN EFI_SMM_ACCESS2_PROTOCOL  *This
> +  )
> +{
> +  return SmramAccessLock (&This->LockState, &This->OpenState); }
> +
> +/**
> +  Queries the memory controller for the possible regions that will
> +support
> +  SMRAM.
> +
> +  @param[in]     This           The EFI_SMM_ACCESS2_PROTOCOL instance.
> +  @param[in,out] SmramMapSize   A pointer to the size, in bytes, of the
> +                                SmramMemoryMap buffer.
> +  @param[in,out] SmramMap       A pointer to the buffer in which firmware
> +                                places the current memory map.
> +
> +  @retval EFI_SUCCESS           The chipset supported the given resource.
> +  @retval EFI_BUFFER_TOO_SMALL  The SmramMap parameter was too small.  The
> +                                current buffer size needed to hold the memory
> +                                map is returned in SmramMapSize.
> +**/
> +STATIC

Recommend removing STATIC from public functions.

> +EFI_STATUS
> +EFIAPI
> +SmmAccess2DxeGetCapabilities (
> +  IN CONST EFI_SMM_ACCESS2_PROTOCOL  *This,
> +  IN OUT UINTN                       *SmramMapSize,
> +  IN OUT EFI_SMRAM_DESCRIPTOR        *SmramMap
> +  )
> +{
> +  return SmramAccessGetCapabilities (This->LockState, This->OpenState,
> +           SmramMapSize, SmramMap);
> +}
> +
> +//
> +// LockState and OpenState will be filled in by the entry point.
> +//
> +STATIC EFI_SMM_ACCESS2_PROTOCOL mAccess2 = {
> +  &SmmAccess2DxeOpen,
> +  &SmmAccess2DxeClose,
> +  &SmmAccess2DxeLock,
> +  &SmmAccess2DxeGetCapabilities
> +};

Recommend removing STATIC from produced protocol structure.
Global variable are typically placed before function implementation in a file.

> +
> +//
> +// Entry point of this driver.
> +//
> +EFI_STATUS
> +EFIAPI
> +SmmAccess2DxeEntryPoint (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  //
> +  // This module should only be included if SMRAM support is required.
> +  //
> +  ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
> +
> +  GetStates (&mAccess2.LockState, &mAccess2.OpenState);
> +  return gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
> +                &gEfiSmmAccess2ProtocolGuid, &mAccess2,
> +                NULL);
> +}
> --
> 1.8.3.1
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to