Re: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine operation

["Yao, Jiewen" <jiewen.yao@intel.com>]

It is good to have lock for linked list management.

However, I do not think we should update TPM for ExitBootServices/LegacyBoot.

I purposely use TPL_CALLBACK to make sure VTd is tear down later, so that other driver can stop DMA before that.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, January 4, 2018 11:32 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine
> operation
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c    | 19 ++++++-------
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.c        |  8 +++---
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.h        |  2 ++
>  .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c          | 32
> +++++++++-------------
>  4 files changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> index e8685666e79a..57e086a64dbc 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> @@ -1,7 +1,7 @@
>  /** @file
>    BmDma related function
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2018, 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
> @@ -12,15 +12,7 @@
> 
>  **/
> 
> -#include <PiDxe.h>
> -
> -#include <Protocol/IoMmu.h>
> -
> -#include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/BaseMemoryLib.h>
> -#include <Library/MemoryAllocationLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> +#include "DmaProtection.h"
> 
>  // TBD: May make it a policy
>  #define DMA_MEMORY_TOP          MAX_UINTN
> @@ -76,6 +68,7 @@ IoMmuMap (
>    MAP_INFO                                          *MapInfo;
>    EFI_PHYSICAL_ADDRESS
> DmaMemoryTop;
>    BOOLEAN                                           NeedRemap;
> +  EFI_TPL                                           OriginalTpl;
> 
>    if (NumberOfBytes == NULL || DeviceAddress == NULL ||
>        Mapping == NULL) {
> @@ -198,7 +191,9 @@ IoMmuMap (
>      MapInfo->DeviceAddress = MapInfo->HostAddress;
>    }
> 
> +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
>    InsertTailList (&gMaps, &MapInfo->Link);
> +  gBS->RestoreTPL (OriginalTpl);
> 
>    //
>    // The DeviceAddress is the address of the maped buffer below 4GB
> @@ -233,6 +228,7 @@ IoMmuUnmap (
>  {
>    MAP_INFO                 *MapInfo;
>    LIST_ENTRY               *Link;
> +  EFI_TPL                  OriginalTpl;
> 
>    DEBUG ((DEBUG_VERBOSE, "IoMmuUnmap: 0x%08x\n", Mapping));
> 
> @@ -241,6 +237,7 @@ IoMmuUnmap (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
>    MapInfo = NULL;
>    for (Link = GetFirstNode (&gMaps)
>         ; !IsNull (&gMaps, Link)
> @@ -255,10 +252,12 @@ IoMmuUnmap (
>    // Mapping is not a valid value returned by Map()
>    //
>    if (MapInfo != Mapping) {
> +    gBS->RestoreTPL (OriginalTpl);
>      DEBUG ((DEBUG_ERROR, "IoMmuUnmap: %r\n",
> EFI_INVALID_PARAMETER));
>      return EFI_INVALID_PARAMETER;
>    }
>    RemoveEntryList (&MapInfo->Link);
> +  gBS->RestoreTPL (OriginalTpl);
> 
>    if (MapInfo->DeviceAddress != MapInfo->HostAddress) {
>      //
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> index 648f64c20b77..013823cc161f 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> @@ -478,7 +478,7 @@ InitializeDmaProtection (
> 
>    Status = gBS->CreateEventEx (
>                    EVT_NOTIFY_SIGNAL,
> -                  TPL_CALLBACK,
> +                  VTD_TPL_LEVEL,
>                    AcpiNotificationFunc,
>                    NULL,
>                    &gEfiAcpi10TableGuid,
> @@ -488,7 +488,7 @@ InitializeDmaProtection (
> 
>    Status = gBS->CreateEventEx (
>                    EVT_NOTIFY_SIGNAL,
> -                  TPL_CALLBACK,
> +                  VTD_TPL_LEVEL,
>                    AcpiNotificationFunc,
>                    NULL,
>                    &gEfiAcpi20TableGuid,
> @@ -505,7 +505,7 @@ InitializeDmaProtection (
> 
>    Status = gBS->CreateEventEx (
>                    EVT_NOTIFY_SIGNAL,
> -                  TPL_CALLBACK,
> +                  VTD_TPL_LEVEL,
>                    OnExitBootServices,
>                    NULL,
>                    &gEfiEventExitBootServicesGuid,
> @@ -514,7 +514,7 @@ InitializeDmaProtection (
>    ASSERT_EFI_ERROR (Status);
> 
>    Status = EfiCreateEventLegacyBootEx (
> -             TPL_CALLBACK,
> +             VTD_TPL_LEVEL,
>               OnLegacyBoot,
>               NULL,
>               &LegacyBootEvent
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
> index 519a5ab00450..bc14ff9a6631 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
> @@ -48,6 +48,8 @@
>  #define ALIGN_VALUE_UP(Value, Alignment)  (((Value) + (Alignment) - 1) &
> (~((Alignment) - 1)))
>  #define ALIGN_VALUE_LOW(Value, Alignment) ((Value) & (~((Alignment) - 1)))
> 
> +#define VTD_TPL_LEVEL TPL_NOTIFY
> +
>  //
>  // This is the initial max PCI DATA number.
>  // The number may be enlarged later.
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
> index 89d9bea3fc0f..570b47cf7364 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
> @@ -12,16 +12,6 @@
> 
>  **/
> 
> -#include <PiDxe.h>
> -
> -#include <Protocol/IoMmu.h>
> -#include <Protocol/PciIo.h>
> -
> -#include <Library/IoLib.h>
> -#include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> -
>  #include "DmaProtection.h"
> 
>  /**
> @@ -306,18 +296,22 @@ IoMmuSetAttribute (
>    EFI_STATUS            Status;
>    EFI_PHYSICAL_ADDRESS  DeviceAddress;
>    UINTN                 NumberOfPages;
> +  EFI_TPL               OriginalTpl;
> +
> +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
> 
>    Status = GetDeviceInfoFromMapping (Mapping, &DeviceAddress,
> &NumberOfPages);
> -  if (EFI_ERROR(Status)) {
> -    return Status;
> +  if (!EFI_ERROR(Status)) {
> +    Status = VTdSetAttribute (
> +               This,
> +               DeviceHandle,
> +               DeviceAddress,
> +               EFI_PAGES_TO_SIZE(NumberOfPages),
> +               IoMmuAccess
> +               );
>    }
> -  Status = VTdSetAttribute (
> -             This,
> -             DeviceHandle,
> -             DeviceAddress,
> -             EFI_PAGES_TO_SIZE(NumberOfPages),
> -             IoMmuAccess
> -             );
> +
> +  gBS->RestoreTPL (OriginalTpl);
> 
>    return Status;
>  }
> --
> 2.7.0.windows.1