[PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: iommu_exit_boot

This series is the result of the discussion under

  [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
                     buffers at ExitBootServices()
  https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html

At ExitBootServices(), PCI and VirtIo drivers should only care about
aborting pending DMA on the devices. Cleaning up PciIo mappings (which
ultimately boil down to IOMMU mappings) for those aborted DMA operations
should be the job of the IOMMU driver.

Patches 01 through 03 clean up the AtaAtapiPassThru driver in
MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
and disables BMDMA in the wrong order in its DriverBindingStop()
function, (b) it doesn't abort pending DMA at ExitBootServices().

This subset can be treated separately from the rest of the series, but I
thought they belonged loosely together (given that AtaAtapiPassThru is
used on QEMU's Q35 machine type).

Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
calls from the VirtIo drivers' ExitBootServices() handlers.

(The conversion of VirtioNetDxe to device addresses is still in progress
-- Brijesh, when you submit v2 of that, under this approach, there is no
need to change VirtioNetExitBoot() relative to current upstream, and you
can use VirtioOperationBusMasterRead to map outgoing packets.)

Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
unmap all mappings (Read, Write, CommonBuffer) that are in effect when
ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
abort pending DMA first, and IoMmuDxe clean up the mappings last.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (10):
  MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
  MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
    DMA
  MdeModulePkg/AtaAtapiPassThru: disable the device at
    ExitBootServices()
  OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
    ExitBootServices
  OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/IoMmuDxe: track all mappings
  OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
  OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246 +++++++++++++++++---
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
 OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
 OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
 7 files changed, 299 insertions(+), 101 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 01/10] MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes

[Laszlo Ersek]

Both AtaAtapiPassThruStart() and AtaAtapiPassThruStop() fetch the
supported attributes of the device, just so they can toggle the
IO+MMIO+BusMaster subset.

After we compute this bitmask in AtaAtapiPassThruStart(), we can cache it
for later, and save the fetch in AtaAtapiPassThruStop().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  1 +
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 32 ++++++++------------
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 4f327dc30b60..85e5a5553953 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -98,10 +98,11 @@ typedef struct {
 
   //
   // The attached device list
   //
   LIST_ENTRY                        DeviceList;
+  UINT64                            EnabledPciAttributes;
   UINT64                            OriginalPciAttributes;
 
   //
   // For AtaPassThru protocol, using the following bytes to record the previous call in
   // GetNextPort()/GetNextDevice().
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 795443ef74f6..b7fdb8dd4876 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -92,10 +92,11 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
   },
   {                   // DeviceList
     NULL,
     NULL
   },
+  0,                  // EnabledPciAttributes
   0,                  // OriginalAttributes
   0,                  // PreviousPort
   0,                  // PreviousPortMultiplier
   0,                  // PreviousTargetId
   0,                  // PreviousLun
@@ -668,11 +669,11 @@ AtaAtapiPassThruStart (
 {
   EFI_STATUS                        Status;
   EFI_IDE_CONTROLLER_INIT_PROTOCOL  *IdeControllerInit;
   ATA_ATAPI_PASS_THRU_INSTANCE      *Instance;
   EFI_PCI_IO_PROTOCOL               *PciIo;
-  UINT64                            Supports;
+  UINT64                            EnabledPciAttributes;
   UINT64                            OriginalPciAttributes;
 
   Status                = EFI_SUCCESS;
   IdeControllerInit     = NULL;
   Instance              = NULL;
@@ -720,18 +721,18 @@ AtaAtapiPassThruStart (
 
   Status = PciIo->Attributes (
                     PciIo,
                     EfiPciIoAttributeOperationSupported,
                     0,
-                    &Supports
+                    &EnabledPciAttributes
                     );
   if (!EFI_ERROR (Status)) {
-    Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+    EnabledPciAttributes &= (UINT64)EFI_PCI_DEVICE_ENABLE;
     Status = PciIo->Attributes (
                       PciIo,
                       EfiPciIoAttributeOperationEnable,
-                      Supports,
+                      EnabledPciAttributes,
                       NULL
                       );
   }
 
   if (EFI_ERROR (Status)) {
@@ -747,10 +748,11 @@ AtaAtapiPassThruStart (
   }
 
   Instance->ControllerHandle      = Controller;
   Instance->IdeControllerInit     = IdeControllerInit;
   Instance->PciIo                 = PciIo;
+  Instance->EnabledPciAttributes  = EnabledPciAttributes;
   Instance->OriginalPciAttributes = OriginalPciAttributes;
   Instance->AtaPassThru.Mode      = &Instance->AtaPassThruMode;
   Instance->ExtScsiPassThru.Mode  = &Instance->ExtScsiPassThruMode;
   InitializeListHead(&Instance->DeviceList);
   InitializeListHead(&Instance->NonBlockingTaskList);
@@ -857,11 +859,10 @@ AtaAtapiPassThruStop (
   EFI_STATUS                        Status;
   ATA_ATAPI_PASS_THRU_INSTANCE      *Instance;
   EFI_ATA_PASS_THRU_PROTOCOL        *AtaPassThru;
   EFI_PCI_IO_PROTOCOL               *PciIo;
   EFI_AHCI_REGISTERS                *AhciRegisters;
-  UINT64                            Supports;
 
   DEBUG ((EFI_D_INFO, "==AtaAtapiPassThru Stop== Controller = %x\n", Controller));
 
   Status = gBS->OpenProtocol (
                   Controller,
@@ -950,25 +951,16 @@ AtaAtapiPassThruStop (
   }
 
   //
   // Disable this ATA host controller.
   //
-  Status = PciIo->Attributes (
-                    PciIo,
-                    EfiPciIoAttributeOperationSupported,
-                    0,
-                    &Supports
-                    );
-  if (!EFI_ERROR (Status)) {
-    Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
-    PciIo->Attributes (
-             PciIo,
-             EfiPciIoAttributeOperationDisable,
-             Supports,
-             NULL
-             );
-  }
+  PciIo->Attributes (
+           PciIo,
+           EfiPciIoAttributeOperationDisable,
+           Instance->EnabledPciAttributes,
+           NULL
+           );
 
   //
   // Restore original PCI attributes
   //
   Status = PciIo->Attributes (
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 02/10] MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM DMA

[Laszlo Ersek]

In AtaAtapiPassThruStop(), if the device has been operating in AHCI mode,
we unmap the DMA buffers and then disable the device (including bus master
DMA). The order of these actions is wrong; we shouldn't unmap DMA buffers
until bus master DMA is turned off. Reverse the steps.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 24 ++++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index b7fdb8dd4876..a48b295d26aa 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -911,16 +911,26 @@ AtaAtapiPassThruStop (
   //
   // Free allocated resource
   //
   DestroyDeviceInfoList (Instance);
 
+  PciIo = Instance->PciIo;
+
+  //
+  // Disable this ATA host controller.
+  //
+  PciIo->Attributes (
+           PciIo,
+           EfiPciIoAttributeOperationDisable,
+           Instance->EnabledPciAttributes,
+           NULL
+           );
+
   //
   // If the current working mode is AHCI mode, then pre-allocated resource
   // for AHCI initialization should be released.
   //
-  PciIo = Instance->PciIo;
-
   if (Instance->Mode == EfiAtaAhciMode) {
     AhciRegisters = &Instance->AhciRegisters;
     PciIo->Unmap (
              PciIo,
              AhciRegisters->MapCommandTable
@@ -948,20 +958,10 @@ AtaAtapiPassThruStop (
              EFI_SIZE_TO_PAGES ((UINTN) AhciRegisters->MaxReceiveFisSize),
              AhciRegisters->AhciRFis
              );
   }
 
-  //
-  // Disable this ATA host controller.
-  //
-  PciIo->Attributes (
-           PciIo,
-           EfiPciIoAttributeOperationDisable,
-           Instance->EnabledPciAttributes,
-           NULL
-           );
-
   //
   // Restore original PCI attributes
   //
   Status = PciIo->Attributes (
                     PciIo,
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Laszlo Ersek]

The AtaAtapiPassThru driver maps three system memory regions for Bus
Master Common Buffer operation on the following call path, if the
controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:

  AtaAtapiPassThruStart()
    EnumerateAttachedDevice()
      AhciModeInitialization()
        AhciCreateTransferDescriptor()

The device is disabled (including Bus Master DMA) when the controller is
unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.

The former step should also be done when we exit the boot services, and
the OS gains ownership of system memory.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  6 ++
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 85e5a5553953..8d6eac706c0b 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -119,10 +119,16 @@ typedef struct {
   //
   // For Non-blocking.
   //
   EFI_EVENT                         TimerEvent;
   LIST_ENTRY                        NonBlockingTaskList;
+
+  //
+  // For disabling the device (especially Bus Master DMA) at
+  // ExitBootServices().
+  //
+  EFI_EVENT                         ExitBootEvent;
 } ATA_ATAPI_PASS_THRU_INSTANCE;
 
 //
 // Task for Non-blocking mode.
 //
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index a48b295d26aa..09064dda18b7 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -102,11 +102,12 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
   0,                  // PreviousLun
   NULL,               // Timer event
   {                   // NonBlocking TaskList
     NULL,
     NULL
-  }
+  },
+  NULL,               // ExitBootEvent
 };
 
 ATAPI_DEVICE_PATH    mAtapiDevicePathTemplate = {
   {
     MESSAGING_DEVICE_PATH,
@@ -476,10 +477,42 @@ InitializeAtaAtapiPassThru (
   ASSERT_EFI_ERROR (Status);
 
   return Status;
 }
 
+/**
+  Disable the device (especially Bus Master DMA) when exiting the boot
+  services.
+
+  @param[in] Event    Event for which this notification function is being
+                      called.
+  @param[in] Context  Pointer to the ATA_ATAPI_PASS_THRU_INSTANCE that
+                      represents the HBA.
+**/
+VOID
+EFIAPI
+AtaPassThruExitBootServices (
+  IN EFI_EVENT Event,
+  IN VOID      *Context
+  )
+{
+  ATA_ATAPI_PASS_THRU_INSTANCE *Instance;
+  EFI_PCI_IO_PROTOCOL          *PciIo;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
+
+  Instance = Context;
+  PciIo = Instance->PciIo;
+
+  PciIo->Attributes (
+           PciIo,
+           EfiPciIoAttributeOperationDisable,
+           Instance->EnabledPciAttributes,
+           NULL
+           );
+}
+
 /**
   Tests to see if this driver supports a given controller. If a child device is provided,
   it further tests to see if this driver supports creating a handle for the specified child device.
 
   This function checks to see if the driver specified by This supports the device specified by
@@ -755,10 +788,21 @@ AtaAtapiPassThruStart (
   Instance->AtaPassThru.Mode      = &Instance->AtaPassThruMode;
   Instance->ExtScsiPassThru.Mode  = &Instance->ExtScsiPassThruMode;
   InitializeListHead(&Instance->DeviceList);
   InitializeListHead(&Instance->NonBlockingTaskList);
 
+  Status = gBS->CreateEvent (
+                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
+                  TPL_CALLBACK,
+                  AtaPassThruExitBootServices,
+                  Instance,
+                  &Instance->ExitBootEvent
+                  );
+  if (EFI_ERROR (Status)) {
+    goto ErrorExit;
+  }
+
   Instance->TimerEvent = NULL;
 
   Status = gBS->CreateEvent (
                   EVT_TIMER | EVT_NOTIFY_SIGNAL,
                   TPL_NOTIFY,
@@ -808,10 +852,14 @@ ErrorExit:
 
   if ((Instance != NULL) && (Instance->TimerEvent != NULL)) {
     gBS->CloseEvent (Instance->TimerEvent);
   }
 
+  if ((Instance != NULL) && (Instance->ExitBootEvent != NULL)) {
+    gBS->CloseEvent (Instance->ExitBootEvent);
+  }
+
   //
   // Remove all inserted ATA devices.
   //
   DestroyDeviceInfoList(Instance);
 
@@ -906,10 +954,19 @@ AtaAtapiPassThruStop (
   if (Instance->TimerEvent != NULL) {
     gBS->CloseEvent (Instance->TimerEvent);
     Instance->TimerEvent = NULL;
   }
   DestroyAsynTaskList (Instance, FALSE);
+
+  //
+  // Close event signaled at gBS->ExitBootServices().
+  //
+  if (Instance->ExitBootEvent != NULL) {
+    gBS->CloseEvent (Instance->ExitBootEvent);
+    Instance->ExitBootEvent = NULL;
+  }
+
   //
   // Free allocated resource
   //
   DestroyDeviceInfoList (Instance);
 
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Laszlo Ersek]

Hi Star, Eric,

On 09/08/17 00:41, Laszlo Ersek wrote:
> The AtaAtapiPassThru driver maps three system memory regions for Bus
> Master Common Buffer operation on the following call path, if the
> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
> 
>   AtaAtapiPassThruStart()
>     EnumerateAttachedDevice()
>       AhciModeInitialization()
>         AhciCreateTransferDescriptor()
> 
> The device is disabled (including Bus Master DMA) when the controller is
> unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
> 
> The former step should also be done when we exit the boot services, and
> the OS gains ownership of system memory.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  6 ++
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
>  2 files changed, 64 insertions(+), 1 deletion(-)

this patch -- that is, commit 6fb8ddd36bde
("MdeModulePkg/AtaAtapiPassThru: disable the device at
ExitBootServices()", 2017-09-03) -- has caused a performance regression
in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.

Interestingly, the performance regression only affects the "traditional"
IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
affect the AHCI/SATA controller of the "q35" machine type. My measurements:

QEMU      machine  OVMF                       time from launch to
          type                                first install screen
--------  -------  ---------------------      --------------------
v2.8.1.1  pc       704b71d7e11f                              117 s
v2.8.1.1  pc       704b71d7e11f\6fb8ddd36bde                  44 s

v2.8.1.1  q35      704b71d7e11f                                9 s
v2.8.1.1  q35      704b71d7e11f\6fb8ddd36bde                   9 s

v2.10.1   pc       704b71d7e11f                              119 s
v2.10.1   pc       704b71d7e11f\6fb8ddd36bde                  46 s

v2.10.1   q35      704b71d7e11f                               10 s
v2.10.1   q35      704b71d7e11f\6fb8ddd36bde                   9 s

(Here "704b71d7e11f" means "OVMF built at current upstream master,
704b71d7e11f"; and "\6fb8ddd36bde" means that commit 6fb8ddd36bde was
reverted on top.)

This issue was reported in:

  https://bugs.launchpad.net/ubuntu/+source/edk2/+bug/1725560

Personally I think that commit 6fb8ddd36bde is correct. Under the
LaunchPad report, I wrote:

> To me this totally looks like a Windows bug; edk2 commit 6fb8ddd36bde
> does the right thing. The patch disables PCI bus master DMA for the
> IDE/AHCI controller in question at ExitBootServices(), i.e. when the
> OS gains control of the system from the firmware. At that point
> ownership of the RAM is transferred to the OS, and in-flight DMA has
> to be aborted (otherwise DMA pending from the firmware could overwrite
> RAM that is now owned by the OS). Whether a controller is passed --
> from firmware to the OS -- with DMA enabled vs. DMA disabled in the
> PCI command register, should have zero consequence on how the OS
> drives the controller subsequently, with its own native driver.

Furthermore, the increased load time of the Windows ISO is *not* only
due to the genuinely lower performance of IDE, compared to SATA; there
is at least one very long wait where Windows doesn't seem to be doing
anything at all, when the machine type is "pc" (i440fx) and this patch
is applied:

> BTW, I've also noticed that a large chunk of the delay, with i440fx,
> is not even spent loading data from the IDE CD-ROM. IDE emulation
> means host CPU load, but in this case, Windows just sits there with
> the empty purplish/bluish screen, and there is zero host CPU load --
> nothing happens. It's as if Windows was waiting for some timer to
> expire.

Is my understanding correct that you guys have never seen the same
performance regression on physical hardware?

Is that perhaps because, in practice, physical computers only use SATA
controllers these days, not traditional IDE?

I'm in a difficult situation, because (a) the patch is obviously right,
(b) the correctness of the patch does not help the QEMU / OVMF users
that suffer from the performance regression.

I'm thinking maybe we can add some "bug compatibility" code to
AtaPassThruExitBootServices(). A dynamic PCD would technically work, but
I totally understand that MdeModulePkg only accepts new PCDs if there is
absolutely no other way to solve an issue. So, what do you guys think?

Meanwhile I'll also ask my colleagues for help with debugging QEMU /
Windows, to see why exactly this change slows down Windows (on
traditional IDE controllers). It will take a while though, my team is
busy at the KVM Forum 2017.

Thanks!
Laszlo



> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> index 85e5a5553953..8d6eac706c0b 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> @@ -119,10 +119,16 @@ typedef struct {
>    //
>    // For Non-blocking.
>    //
>    EFI_EVENT                         TimerEvent;
>    LIST_ENTRY                        NonBlockingTaskList;
> +
> +  //
> +  // For disabling the device (especially Bus Master DMA) at
> +  // ExitBootServices().
> +  //
> +  EFI_EVENT                         ExitBootEvent;
>  } ATA_ATAPI_PASS_THRU_INSTANCE;
>  
>  //
>  // Task for Non-blocking mode.
>  //
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> index a48b295d26aa..09064dda18b7 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> @@ -102,11 +102,12 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
>    0,                  // PreviousLun
>    NULL,               // Timer event
>    {                   // NonBlocking TaskList
>      NULL,
>      NULL
> -  }
> +  },
> +  NULL,               // ExitBootEvent
>  };
>  
>  ATAPI_DEVICE_PATH    mAtapiDevicePathTemplate = {
>    {
>      MESSAGING_DEVICE_PATH,
> @@ -476,10 +477,42 @@ InitializeAtaAtapiPassThru (
>    ASSERT_EFI_ERROR (Status);
>  
>    return Status;
>  }
>  
> +/**
> +  Disable the device (especially Bus Master DMA) when exiting the boot
> +  services.
> +
> +  @param[in] Event    Event for which this notification function is being
> +                      called.
> +  @param[in] Context  Pointer to the ATA_ATAPI_PASS_THRU_INSTANCE that
> +                      represents the HBA.
> +**/
> +VOID
> +EFIAPI
> +AtaPassThruExitBootServices (
> +  IN EFI_EVENT Event,
> +  IN VOID      *Context
> +  )
> +{
> +  ATA_ATAPI_PASS_THRU_INSTANCE *Instance;
> +  EFI_PCI_IO_PROTOCOL          *PciIo;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
> +
> +  Instance = Context;
> +  PciIo = Instance->PciIo;
> +
> +  PciIo->Attributes (
> +           PciIo,
> +           EfiPciIoAttributeOperationDisable,
> +           Instance->EnabledPciAttributes,
> +           NULL
> +           );
> +}
> +
>  /**
>    Tests to see if this driver supports a given controller. If a child device is provided,
>    it further tests to see if this driver supports creating a handle for the specified child device.
>  
>    This function checks to see if the driver specified by This supports the device specified by
> @@ -755,10 +788,21 @@ AtaAtapiPassThruStart (
>    Instance->AtaPassThru.Mode      = &Instance->AtaPassThruMode;
>    Instance->ExtScsiPassThru.Mode  = &Instance->ExtScsiPassThruMode;
>    InitializeListHead(&Instance->DeviceList);
>    InitializeListHead(&Instance->NonBlockingTaskList);
>  
> +  Status = gBS->CreateEvent (
> +                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                  TPL_CALLBACK,
> +                  AtaPassThruExitBootServices,
> +                  Instance,
> +                  &Instance->ExitBootEvent
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    goto ErrorExit;
> +  }
> +
>    Instance->TimerEvent = NULL;
>  
>    Status = gBS->CreateEvent (
>                    EVT_TIMER | EVT_NOTIFY_SIGNAL,
>                    TPL_NOTIFY,
> @@ -808,10 +852,14 @@ ErrorExit:
>  
>    if ((Instance != NULL) && (Instance->TimerEvent != NULL)) {
>      gBS->CloseEvent (Instance->TimerEvent);
>    }
>  
> +  if ((Instance != NULL) && (Instance->ExitBootEvent != NULL)) {
> +    gBS->CloseEvent (Instance->ExitBootEvent);
> +  }
> +
>    //
>    // Remove all inserted ATA devices.
>    //
>    DestroyDeviceInfoList(Instance);
>  
> @@ -906,10 +954,19 @@ AtaAtapiPassThruStop (
>    if (Instance->TimerEvent != NULL) {
>      gBS->CloseEvent (Instance->TimerEvent);
>      Instance->TimerEvent = NULL;
>    }
>    DestroyAsynTaskList (Instance, FALSE);
> +
> +  //
> +  // Close event signaled at gBS->ExitBootServices().
> +  //
> +  if (Instance->ExitBootEvent != NULL) {
> +    gBS->CloseEvent (Instance->ExitBootEvent);
> +    Instance->ExitBootEvent = NULL;
> +  }
> +
>    //
>    // Free allocated resource
>    //
>    DestroyDeviceInfoList (Instance);
>  
> 

Re: [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Ard Biesheuvel]

On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Star, Eric,
>
> On 09/08/17 00:41, Laszlo Ersek wrote:
>> The AtaAtapiPassThru driver maps three system memory regions for Bus
>> Master Common Buffer operation on the following call path, if the
>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>
>>   AtaAtapiPassThruStart()
>>     EnumerateAttachedDevice()
>>       AhciModeInitialization()
>>         AhciCreateTransferDescriptor()
>>
>> The device is disabled (including Bus Master DMA) when the controller is
>> unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>
>> The former step should also be done when we exit the boot services, and
>> the OS gains ownership of system memory.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  6 ++
>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 +++++++++++++++++++-
>>  2 files changed, 64 insertions(+), 1 deletion(-)
>
> this patch -- that is, commit 6fb8ddd36bde
> ("MdeModulePkg/AtaAtapiPassThru: disable the device at
> ExitBootServices()", 2017-09-03) -- has caused a performance regression
> in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>
> Interestingly, the performance regression only affects the "traditional"
> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not
> affect the AHCI/SATA controller of the "q35" machine type.

Does it make any difference if you only disable memory decoding and
bus mastering, but leave I/O port decoding enabled?

Re: [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Zeng, Star]

Good point.

Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable?
How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf.

7.7
Examples from the EDK II that use this feature are the PCI device drivers for USB Host
Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
memory buffer to poll for operation requests. Access to this memory buffer by a USB
Host Controller may be required to boot an operation system, but this activity must be
terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot
Services Event for these types of drivers is to disable the PCI bus master* and place the
USB Host Controller into a halted state


Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Thursday, October 26, 2017 4:12 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [edk2] [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Star, Eric,
>
> On 09/08/17 00:41, Laszlo Ersek wrote:
>> The AtaAtapiPassThru driver maps three system memory regions for Bus 
>> Master Common Buffer operation on the following call path, if the 
>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>
>>   AtaAtapiPassThruStart()
>>     EnumerateAttachedDevice()
>>       AhciModeInitialization()
>>         AhciCreateTransferDescriptor()
>>
>> The device is disabled (including Bus Master DMA) when the controller 
>> is unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>
>> The former step should also be done when we exit the boot services, 
>> and the OS gains ownership of system memory.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  6 ++  
>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 
>> +++++++++++++++++++-
>>  2 files changed, 64 insertions(+), 1 deletion(-)
>
> this patch -- that is, commit 6fb8ddd36bde
> ("MdeModulePkg/AtaAtapiPassThru: disable the device at 
> ExitBootServices()", 2017-09-03) -- has caused a performance 
> regression in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>
> Interestingly, the performance regression only affects the "traditional"
> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not 
> affect the AHCI/SATA controller of the "q35" machine type.

Does it make any difference if you only disable memory decoding and bus mastering, but leave I/O port decoding enabled?

Re: [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Laszlo Ersek]

Ard, Star,

(CC Igor)

On 10/26/17 07:08, Zeng, Star wrote:
> Good point.
> 
> Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable?
> How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf.
> 
> 7.7
> Examples from the EDK II that use this feature are the PCI device drivers for USB Host
> Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
> memory buffer to poll for operation requests. Access to this memory buffer by a USB
> Host Controller may be required to boot an operation system, but this activity must be
> terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot
> Services Event for these types of drivers is to disable the PCI bus master* and place the
> USB Host Controller into a halted state

thank you for the ideas.

* Disabling only EFI_PCI_IO_ATTRIBUTE_BUS_MASTER at EBS mitigates the
  symptom.

* Disabling (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_IO)
  at EBS preserves the symptom.

* Disabling
  (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_MEMORY) at EBS
  also mitigates the symptom.

So it is as Ard suspected, disabling IO port decoding is what tickles
the bug in Windows.

(Now I'm vaguely recalling an earlier discussion from qemu-devel that
Windows has a bug in that, if any given PCI device is disabled at boot,
then Windows will not load drivers for it, or some such. I'm struggling
to recall the context; maybe it was related to ACPI generation in QEMU.
I'm CC'ing Igor; maybe he remembers better.)

I will post a patch, for disabling EFI_PCI_IO_ATTRIBUTE_BUS_MASTER only.
First, that's going to follow the driver writers' guide verbatim;
second, disabling BMDMA and MMIO, but not IO, would look weird in the
code. :/

Thank you both for the help!
Laszlo


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
> Sent: Thursday, October 26, 2017 4:12 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh <brijesh.singh@amd.com>
> Subject: Re: [edk2] [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()
> 
> On 25 October 2017 at 16:26, Laszlo Ersek <lersek@redhat.com> wrote:
>> Hi Star, Eric,
>>
>> On 09/08/17 00:41, Laszlo Ersek wrote:
>>> The AtaAtapiPassThru driver maps three system memory regions for Bus 
>>> Master Common Buffer operation on the following call path, if the 
>>> controller has PCI_CLASS_MASS_STORAGE_SATADPA class code:
>>>
>>>   AtaAtapiPassThruStart()
>>>     EnumerateAttachedDevice()
>>>       AhciModeInitialization()
>>>         AhciCreateTransferDescriptor()
>>>
>>> The device is disabled (including Bus Master DMA) when the controller 
>>> is unbound, in AtaAtapiPassThruStop(). Then the regions are unmapped.
>>>
>>> The former step should also be done when we exit the boot services, 
>>> and the OS gains ownership of system memory.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |  6 ++  
>>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 59 
>>> +++++++++++++++++++-
>>>  2 files changed, 64 insertions(+), 1 deletion(-)
>>
>> this patch -- that is, commit 6fb8ddd36bde
>> ("MdeModulePkg/AtaAtapiPassThru: disable the device at 
>> ExitBootServices()", 2017-09-03) -- has caused a performance 
>> regression in OVMF, in booting Windows installer ISOs from emulated IDE CD-ROMs.
>>
>> Interestingly, the performance regression only affects the "traditional"
>> IDE controller of the "pc" (i440fx) machine type of QEMU; it does not 
>> affect the AHCI/SATA controller of the "q35" machine type.
> 
> Does it make any difference if you only disable memory decoding and bus mastering, but leave I/O port decoding enabled?
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 03/10] MdeModulePkg/AtaAtapiPassThru: disable the device at ExitBootServices()

[Ard Biesheuvel]

On 26 October 2017 at 15:09, Laszlo Ersek <lersek@redhat.com> wrote:
> Ard, Star,
>
> (CC Igor)
>
> On 10/26/17 07:08, Zeng, Star wrote:
>> Good point.
>>
>> Could we find out what change causes the performance regression? Bus Master disable / Memory Space disable / IO Space disable?
>> How about to only disable Bus Master in the exit boot service event notification? It seems the key point suggested by UEFI Driver_Writer_Guide_V1.0.1_120308.pdf.
>>
>> 7.7
>> Examples from the EDK II that use this feature are the PCI device drivers for USB Host
>> Controllers. Some USB Host Controllers are PCI Bus Masters that continuously access a
>> memory buffer to poll for operation requests. Access to this memory buffer by a USB
>> Host Controller may be required to boot an operation system, but this activity must be
>> terminated when the OS calls ExitBootServices(). *The typical action in the Exit Boot
>> Services Event for these types of drivers is to disable the PCI bus master* and place the
>> USB Host Controller into a halted state
>
> thank you for the ideas.
>
> * Disabling only EFI_PCI_IO_ATTRIBUTE_BUS_MASTER at EBS mitigates the
>   symptom.
>
> * Disabling (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_IO)
>   at EBS preserves the symptom.
>
> * Disabling
>   (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER | EFI_PCI_IO_ATTRIBUTE_MEMORY) at EBS
>   also mitigates the symptom.
>

Excellent!

> So it is as Ard suspected, disabling IO port decoding is what tickles
> the bug in Windows.
>
> (Now I'm vaguely recalling an earlier discussion from qemu-devel that
> Windows has a bug in that, if any given PCI device is disabled at boot,
> then Windows will not load drivers for it, or some such. I'm struggling
> to recall the context; maybe it was related to ACPI generation in QEMU.
> I'm CC'ing Igor; maybe he remembers better.)
>
> I will post a patch, for disabling EFI_PCI_IO_ATTRIBUTE_BUS_MASTER only.
> First, that's going to follow the driver writers' guide verbatim;
> second, disabling BMDMA and MMIO, but not IO, would look weird in the
> code. :/
>
> Thank you both for the help!

Anytime.

[PATCH 04/10] OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()

[Laszlo Ersek]

In one of the following patches, we'll change OvmfPkg/IoMmuDxe so that it
unmaps all existent bus master operations (CommonBuffer, Read, Write) at
ExitBootServices(), strictly after the individual device drivers abort
pending DMA on the devices they manage, in their own ExitBootServices()
notification functions.

In preparation, remove the explicit
VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() call from VirtioBlkExitBoot(),
originally added in commit 19165130470f ("OvmfPkg/VirtioBlkDxe: map VRING
using VirtioRingMap()", 2017-08-27).

Add a DEBUG message so we can observe the ordering between
VirtioBlkExitBoot() and the upcoming cleanup of mappings in
OvmfPkg/IoMmuDxe.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 5a63986b3f39..55598843455d 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -1017,25 +1017,20 @@ VirtioBlkExitBoot (
   IN  VOID      *Context
   )
 {
   VBLK_DEV *Dev;
 
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
   //
   // Reset the device. This causes the hypervisor to forget about the virtio
   // ring.
   //
   // We allocated said ring in EfiBootServicesData type memory, and code
   // executing after ExitBootServices() is permitted to overwrite it.
   //
   Dev = Context;
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-
-  //
-  // Unmap the ring buffer so that hypervisor will not be able to get
-  // readable data after device is reset.
-  //
-  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
 }
 
 /**
 
   After we've pronounced support for a specific device in
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 05/10] OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at ExitBootServices

[Laszlo Ersek]

In one of the following patches, we'll change OvmfPkg/IoMmuDxe so that it
unmaps all existent bus master operations (CommonBuffer, Read, Write) at
ExitBootServices(), strictly after the individual device drivers abort
pending DMA on the devices they manage, in their own ExitBootServices()
notification functions.

In preparation, remove the explicit
VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() calls from VirtioGpuExitBoot(),
originally added in commit 9bc5026c19a5 ("OvmfPkg/VirtioGpuDxe: map VRING
for bus master common buffer operation", 2017-08-26) and commit
f10ae923665f ("OvmfPkg/VirtioGpuDxe: map backing store to bus master
device address", 2017-08-26).

Add a DEBUG message so we can observe the ordering between
VirtioGpuExitBoot() and the upcoming cleanup of mappings in
OvmfPkg/IoMmuDxe.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioGpuDxe/Commands.c | 23 +-------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/OvmfPkg/VirtioGpuDxe/Commands.c b/OvmfPkg/VirtioGpuDxe/Commands.c
index 6e70b1c33f65..6ce21976c918 100644
--- a/OvmfPkg/VirtioGpuDxe/Commands.c
+++ b/OvmfPkg/VirtioGpuDxe/Commands.c
@@ -351,34 +351,13 @@ VirtioGpuExitBoot (
   IN VOID      *Context
   )
 {
   VGPU_DEV *VgpuDev;
 
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
   VgpuDev = Context;
   VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, 0);
-
-  //
-  // If VirtioGpuDriverBindingStart() and VirtioGpuDriverBindingStop() have
-  // been called thus far in such a sequence that right now our (sole) child
-  // handle exists -- with the GOP on it standing for head (scanout) #0 --,
-  // then we have to unmap the current video mode's backing store.
-  //
-  if (VgpuDev->Child != NULL) {
-    //
-    // The current video mode is guaranteed to have a valid and mapped backing
-    // store, due to the first Gop.SetMode() call, made internally in
-    // InitVgpuGop().
-    //
-    ASSERT (VgpuDev->Child->BackingStore != NULL);
-
-    VgpuDev->VirtIo->UnmapSharedBuffer (
-                       VgpuDev->VirtIo,
-                       VgpuDev->Child->BackingStoreMap
-                       );
-  }
-
-  VgpuDev->VirtIo->UnmapSharedBuffer (VgpuDev->VirtIo, VgpuDev->RingMap);
 }
 
 /**
   Internal utility function that sends a request to the VirtIo GPU device
   model, awaits the answer from the host, and returns a status.
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 06/10] OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()

[Laszlo Ersek]

In one of the following patches, we'll change OvmfPkg/IoMmuDxe so that it
unmaps all existent bus master operations (CommonBuffer, Read, Write) at
ExitBootServices(), strictly after the individual device drivers abort
pending DMA on the devices they manage, in their own ExitBootServices()
notification functions.

In preparation, remove the explicit
VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() call from VirtioRngExitBoot(),
originally added in commit 0a568ccbcbd1 ("OvmfPkg/VirtioRngDxe: map host
address to device address", 2017-08-23).

Add a DEBUG message so we can observe the ordering between
VirtioRngExitBoot() and the upcoming cleanup of mappings in
OvmfPkg/IoMmuDxe.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioRngDxe/VirtioRng.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index 80380bcdf8bf..3c733ea4db66 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -433,25 +433,20 @@ VirtioRngExitBoot (
   IN  VOID      *Context
   )
 {
   VIRTIO_RNG_DEV *Dev;
 
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
   //
   // Reset the device. This causes the hypervisor to forget about the virtio
   // ring.
   //
   // We allocated said ring in EfiBootServicesData type memory, and code
   // executing after ExitBootServices() is permitted to overwrite it.
   //
   Dev = Context;
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-
-  //
-  // Unmap the ring buffer so that hypervisor will not be able to get readable
-  // data after device reset.
-  //
-  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
 }
 
 
 //
 // Probe, start and stop functions of this driver, called by the DXE core for
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 07/10] OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()

[Laszlo Ersek]

In one of the following patches, we'll change OvmfPkg/IoMmuDxe so that it
unmaps all existent bus master operations (CommonBuffer, Read, Write) at
ExitBootServices(), strictly after the individual device drivers abort
pending DMA on the devices they manage, in their own ExitBootServices()
notification functions.

In preparation, remove the explicit
VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() call from VirtioScsiExitBoot(),
originally added in commit fc2168feb248 ("OvmfPkg/VirtioScsiDxe: map VRING
using VirtioRingMap()", 2017-08-31).

Add a DEBUG message so we can observe the ordering between
VirtioScsiExitBoot() and the upcoming cleanup of mappings in
OvmfPkg/IoMmuDxe.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 7b8c3d22c8de..1a68f062106c 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -1221,25 +1221,20 @@ VirtioScsiExitBoot (
   IN  VOID      *Context
   )
 {
   VSCSI_DEV *Dev;
 
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
   //
   // Reset the device. This causes the hypervisor to forget about the virtio
   // ring.
   //
   // We allocated said ring in EfiBootServicesData type memory, and code
   // executing after ExitBootServices() is permitted to overwrite it.
   //
   Dev = Context;
   Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
-
-  //
-  // Unmap the ring buffer so that hypervisor will not be able to get
-  // readable data after device reset.
-  //
-  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
 }
 
 
 //
 // Probe, start and stop functions of this driver, called by the DXE core for
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 08/10] OvmfPkg/IoMmuDxe: track all mappings

[Laszlo Ersek]

The "mRecycledMapInfos" list implements an internal pool of unused
MAP_INFO structures between the IoMmuUnmap() and IoMmuMap() functions. The
original goal was to allow IoMmuUnmap() to tear down CommonBuffer mappings
without releasing any memory: IoMmuUnmap() would recycle the MAP_INFO
structure to the list, and IoMmuMap() would always check the list first,
before allocating a brand new MAP_INFO structure.

In one of the following patches, we'll change OvmfPkg/IoMmuDxe so that it
unmaps all existent bus master operations (CommonBuffer, Read, Write) at
ExitBootServices(), strictly after the individual device drivers abort
pending DMA on the devices they manage, in their own ExitBootServices()
notification functions.

For this, rename and repurpose the list to track all live mappings.

This means that IoMmuUnmap() will always release a MAP_INFO structure
(even when cleaning up a CommonBuffer operation). That's fine (for now),
because device drivers are no longer expected to call Unmap() in their
ExitBootServices() notification functions.

In theory, we could also move the allocation and freeing of the stash
buffer from IoMmuAllocateBuffer() and IoMmuFreeBuffer(), respectively, to
IoMmuMap() and IoMmuUnmap(). However, this would require allocating and
freeing a stash buffer in *both* IoMmuMap() and IoMmuUnmap(), as
IoMmuMap() performs in-place decryption for CommonBuffer operations, and
IoMmuUnmap() performs in-place encryption for the same.

By keeping the stash buffer allocation as-is, not only do we keep the code
almost fully undisturbed, but

- we also continue to guarantee that IoMmuUnmap() succeeds: allocating a
  stash buffer in IoMmuUnmap(), for in-place encryption after a
  CommonBuffer operation, could fail;

- we also keep IoMmuUnmap() largely reusable for ExitBootServices()
  callback context: allocating a stash buffer in IoMmuUnmap() would simply
  be forbidden in that context.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 49 +++++++-------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index bc57de5b572b..c86e73498555 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -31,18 +31,15 @@ typedef struct {
   EFI_PHYSICAL_ADDRESS                      CryptedAddress;
   EFI_PHYSICAL_ADDRESS                      PlainTextAddress;
 } MAP_INFO;
 
 //
-// List of MAP_INFO structures recycled by Unmap().
+// List of the MAP_INFO structures that have been set up by IoMmuMap() and not
+// yet torn down by IoMmuUnmap(). The list represents the full set of mappings
+// currently in effect.
 //
-// Recycled MAP_INFO structures are equally good for future recycling and
-// freeing.
-//
-STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
-                                        mRecycledMapInfos
-                                        );
+STATIC LIST_ENTRY mMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (mMapInfos);
 
 #define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
 
 //
 // ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
@@ -121,11 +118,10 @@ IoMmuMap (
   OUT    EFI_PHYSICAL_ADDRESS                       *DeviceAddress,
   OUT    VOID                                       **Mapping
   )
 {
   EFI_STATUS                                        Status;
-  LIST_ENTRY                                        *RecycledMapInfo;
   MAP_INFO                                          *MapInfo;
   EFI_ALLOCATE_TYPE                                 AllocateType;
   COMMON_BUFFER_HEADER                              *CommonBufferHeader;
   VOID                                              *DecryptionSource;
 
@@ -148,23 +144,14 @@ IoMmuMap (
 
   //
   // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
   // called later.
   //
-  RecycledMapInfo = GetFirstNode (&mRecycledMapInfos);
-  if (RecycledMapInfo == &mRecycledMapInfos) {
-    //
-    // No recycled MAP_INFO structure, allocate a new one.
-    //
-    MapInfo = AllocatePool (sizeof (MAP_INFO));
-    if (MapInfo == NULL) {
-      Status = EFI_OUT_OF_RESOURCES;
-      goto Failed;
-    }
-  } else {
-    MapInfo = CR (RecycledMapInfo, MAP_INFO, Link, MAP_INFO_SIG);
-    RemoveEntryList (RecycledMapInfo);
+  MapInfo = AllocatePool (sizeof (MAP_INFO));
+  if (MapInfo == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto Failed;
   }
 
   //
   // Initialize the MAP_INFO structure, except the PlainTextAddress field
   //
@@ -296,10 +283,14 @@ IoMmuMap (
       DecryptionSource,
       MapInfo->NumberOfBytes
       );
   }
 
+  //
+  // Track all MAP_INFO structures.
+  //
+  InsertHeadList (&mMapInfos, &MapInfo->Link);
   //
   // Populate output parameters.
   //
   *DeviceAddress = MapInfo->PlainTextAddress;
   *Mapping       = MapInfo;
@@ -428,28 +419,24 @@ IoMmuUnmap (
     CopyMem (
       (VOID *)(UINTN)MapInfo->CryptedAddress,
       CommonBufferHeader->StashBuffer,
       MapInfo->NumberOfBytes
       );
-
-    //
-    // Recycle the MAP_INFO structure.
-    //
-    InsertTailList (&mRecycledMapInfos, &MapInfo->Link);
   } else {
     ZeroMem (
       (VOID *)(UINTN)MapInfo->PlainTextAddress,
       EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
       );
     gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
-
-    //
-    // Free the MAP_INFO structure.
-    //
-    FreePool (MapInfo);
   }
 
+  //
+  // Forget and free the MAP_INFO structure.
+  //
+  RemoveEntryList (&MapInfo->Link);
+  FreePool (MapInfo);
+
   return EFI_SUCCESS;
 }
 
 /**
   Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 09/10] OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()

[Laszlo Ersek]

IoMmuUnmapWorker() is identical to IoMmuUnmap(), it just takes an
additional BOOLEAN parameter called "MemoryMapLocked".  If the memory map
is locked, IoMmuUnmapWorker() does its usual job, but it purposely leaks
memory rather than freeing it. This makes it callable from
ExitBootServices() context.

Turn IoMmuUnmap() into a thin wrapper around IoMmuUnmapWorker() that
passes constant FALSE for "MemoryMapLocked".

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 60 +++++++++++++++++---
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index c86e73498555..34e1c6ee4a74 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -316,32 +316,46 @@ Failed:
 }
 
 /**
   Completes the Map() operation and releases any corresponding resources.
 
+  This is an internal worker function that only extends the Map() API with
+  the MemoryMapLocked parameter.
+
   @param  This                  The protocol instance pointer.
   @param  Mapping               The mapping value returned from Map().
+  @param  MemoryMapLocked       The function is executing on the stack of
+                                gBS->ExitBootServices(); changes to the UEFI
+                                memory map are forbidden.
 
   @retval EFI_SUCCESS           The range was unmapped.
   @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
                                 Map().
   @retval EFI_DEVICE_ERROR      The data was not committed to the target system
                                 memory.
 **/
+STATIC
 EFI_STATUS
 EFIAPI
-IoMmuUnmap (
+IoMmuUnmapWorker (
   IN  EDKII_IOMMU_PROTOCOL                     *This,
-  IN  VOID                                     *Mapping
+  IN  VOID                                     *Mapping,
+  IN  BOOLEAN                                  MemoryMapLocked
   )
 {
   MAP_INFO                 *MapInfo;
   EFI_STATUS               Status;
   COMMON_BUFFER_HEADER     *CommonBufferHeader;
   VOID                     *EncryptionTarget;
 
-  DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p\n", __FUNCTION__, Mapping));
+  DEBUG ((
+    DEBUG_VERBOSE,
+    "%a: Mapping=0x%p MemoryMapLocked=%d\n",
+    __FUNCTION__,
+    Mapping,
+    MemoryMapLocked
+    ));
 
   if (Mapping == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -410,11 +424,12 @@ IoMmuUnmap (
   //
   // For BusMasterCommonBuffer[64] operations, copy the stashed data to the
   // original (now encrypted) location.
   //
   // For all other operations, fill the late bounce buffer (which existed as
-  // plaintext at some point) with zeros, and then release it.
+  // plaintext at some point) with zeros, and then release it (unless the UEFI
+  // memory map is locked).
   //
   if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer ||
       MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
     CopyMem (
       (VOID *)(UINTN)MapInfo->CryptedAddress,
@@ -424,22 +439,53 @@ IoMmuUnmap (
   } else {
     ZeroMem (
       (VOID *)(UINTN)MapInfo->PlainTextAddress,
       EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages)
       );
-    gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
+    if (!MemoryMapLocked) {
+      gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
+    }
   }
 
   //
-  // Forget and free the MAP_INFO structure.
+  // Forget the MAP_INFO structure, then free it (unless the UEFI memory map is
+  // locked).
   //
   RemoveEntryList (&MapInfo->Link);
-  FreePool (MapInfo);
+  if (!MemoryMapLocked) {
+    FreePool (MapInfo);
+  }
 
   return EFI_SUCCESS;
 }
 
+/**
+  Completes the Map() operation and releases any corresponding resources.
+
+  @param  This                  The protocol instance pointer.
+  @param  Mapping               The mapping value returned from Map().
+
+  @retval EFI_SUCCESS           The range was unmapped.
+  @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
+                                Map().
+  @retval EFI_DEVICE_ERROR      The data was not committed to the target system
+                                memory.
+**/
+EFI_STATUS
+EFIAPI
+IoMmuUnmap (
+  IN  EDKII_IOMMU_PROTOCOL                     *This,
+  IN  VOID                                     *Mapping
+  )
+{
+  return IoMmuUnmapWorker (
+           This,
+           Mapping,
+           FALSE    // MemoryMapLocked
+           );
+}
+
 /**
   Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
   OperationBusMasterCommonBuffer64 mapping.
 
   @param  This                  The protocol instance pointer.
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 10/10] OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()

[Laszlo Ersek]

Register an ExitBootServices() callback that tears down all IOMMU
mappings, without modifying the UEFI memory map.

The trick is that in the ExitBootServices() callback, we don't immediately
do the work; instead we signal another (private) event.

Normally the dispatch order of ExitBootServices() callbacks is unspecified
(within the same task priority level anyway). By queueing another
function, we delay the unmapping until after all PciIo and Virtio drivers
abort -- in their own ExitBootServices() callbacks -- the pending DMA
operations of their respective controllers.

Furthermore, the fact that IoMmuUnmapWorker() rewrites client-owned memory
when it unmaps a Write or CommonBuffer bus master operation, is safe even
in this context. The existence of any given "MapInfo" in "mMapInfos"
implies that the client buffer pointed-to by "MapInfo->CryptedAddress" was
live when ExitBootServices() was entered. And, after entering
ExitBootServices(), nothing must have changed the UEFI memory map, hence
the client buffer at "MapInfo->CryptedAddress" still exists.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 145 ++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 34e1c6ee4a74..6b7df8d8aa3d 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -746,10 +746,111 @@ EDKII_IOMMU_PROTOCOL  mAmdSev = {
   IoMmuUnmap,
   IoMmuAllocateBuffer,
   IoMmuFreeBuffer,
 };
 
+/**
+  Notification function that is queued when gBS->ExitBootServices() signals the
+  EFI_EVENT_GROUP_EXIT_BOOT_SERVICES event group. This function signals another
+  event, received as Context, and returns.
+
+  Signaling an event in this context is safe. The UEFI spec allows
+  gBS->SignalEvent() to return EFI_SUCCESS only; EFI_OUT_OF_RESOURCES is not
+  listed, hence memory is not allocated. The edk2 implementation also does not
+  release memory (and we only have to care about the edk2 implementation
+  because EDKII_IOMMU_PROTOCOL is edk2-specific anyway).
+
+  @param[in] Event          Event whose notification function is being invoked.
+                            Event is permitted to request the queueing of this
+                            function at TPL_CALLBACK or TPL_NOTIFY task
+                            priority level.
+
+  @param[in] EventToSignal  Identifies the EFI_EVENT to signal. EventToSignal
+                            is permitted to request the queueing of its
+                            notification function only at TPL_CALLBACK level.
+**/
+STATIC
+VOID
+EFIAPI
+AmdSevExitBoot (
+  IN EFI_EVENT Event,
+  IN VOID      *EventToSignal
+  )
+{
+  //
+  // (1) The NotifyFunctions of all the events in
+  //     EFI_EVENT_GROUP_EXIT_BOOT_SERVICES will have been queued before
+  //     AmdSevExitBoot() is entered.
+  //
+  // (2) AmdSevExitBoot() is executing minimally at TPL_CALLBACK.
+  //
+  // (3) AmdSevExitBoot() has been queued in unspecified order relative to the
+  //     NotifyFunctions of all the other events in
+  //     EFI_EVENT_GROUP_EXIT_BOOT_SERVICES whose NotifyTpl is the same as
+  //     Event's.
+  //
+  // Consequences:
+  //
+  // - If Event's NotifyTpl is TPL_CALLBACK, then some other NotifyFunctions
+  //   queued at TPL_CALLBACK may be invoked after AmdSevExitBoot() returns.
+  //
+  // - If Event's NotifyTpl is TPL_NOTIFY, then some other NotifyFunctions
+  //   queued at TPL_NOTIFY may be invoked after AmdSevExitBoot() returns; plus
+  //   *all* NotifyFunctions queued at TPL_CALLBACK will be invoked strictly
+  //   after all NotifyFunctions queued at TPL_NOTIFY, including
+  //   AmdSevExitBoot(), have been invoked.
+  //
+  // - By signaling EventToSignal here, whose NotifyTpl is TPL_CALLBACK, we
+  //   queue EventToSignal's NotifyFunction after the NotifyFunctions of *all*
+  //   events in EFI_EVENT_GROUP_EXIT_BOOT_SERVICES.
+  //
+  DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));
+  gBS->SignalEvent (EventToSignal);
+}
+
+/**
+  Notification function that is queued after the notification functions of all
+  events in the EFI_EVENT_GROUP_EXIT_BOOT_SERVICES event group. The same memory
+  map restrictions apply.
+
+  This function unmaps all currently existing IOMMU mappings.
+
+  @param[in] Event    Event whose notification function is being invoked. Event
+                      is permitted to request the queueing of this function
+                      only at TPL_CALLBACK task priority level.
+
+  @param[in] Context  Ignored.
+**/
+STATIC
+VOID
+EFIAPI
+AmdSevUnmapAllMappings (
+  IN EFI_EVENT Event,
+  IN VOID      *Context
+  )
+{
+  LIST_ENTRY *Node;
+  LIST_ENTRY *NextNode;
+  MAP_INFO   *MapInfo;
+
+  DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__));
+
+  //
+  // All drivers that had set up IOMMU mappings have halted their respective
+  // controllers by now; tear down the mappings.
+  //
+  for (Node = GetFirstNode (&mMapInfos); Node != &mMapInfos; Node = NextNode) {
+    NextNode = GetNextNode (&mMapInfos, Node);
+    MapInfo = CR (Node, MAP_INFO, Link, MAP_INFO_SIG);
+    IoMmuUnmapWorker (
+      &mAmdSev, // This
+      MapInfo,  // Mapping
+      TRUE      // MemoryMapLocked
+      );
+  }
+}
+
 /**
   Initialize Iommu Protocol.
 
 **/
 EFI_STATUS
@@ -757,15 +858,59 @@ EFIAPI
 AmdSevInstallIoMmuProtocol (
   VOID
   )
 {
   EFI_STATUS  Status;
+  EFI_EVENT   UnmapAllMappingsEvent;
+  EFI_EVENT   ExitBootEvent;
   EFI_HANDLE  Handle;
 
+  //
+  // Create the "late" event whose notification function will tear down all
+  // left-over IOMMU mappings.
+  //
+  Status = gBS->CreateEvent (
+                  EVT_NOTIFY_SIGNAL,      // Type
+                  TPL_CALLBACK,           // NotifyTpl
+                  AmdSevUnmapAllMappings, // NotifyFunction
+                  NULL,                   // NotifyContext
+                  &UnmapAllMappingsEvent  // Event
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // Create the event whose notification function will be queued by
+  // gBS->ExitBootServices() and will signal the event created above.
+  //
+  Status = gBS->CreateEvent (
+                  EVT_SIGNAL_EXIT_BOOT_SERVICES, // Type
+                  TPL_CALLBACK,                  // NotifyTpl
+                  AmdSevExitBoot,                // NotifyFunction
+                  UnmapAllMappingsEvent,         // NotifyContext
+                  &ExitBootEvent                 // Event
+                  );
+  if (EFI_ERROR (Status)) {
+    goto CloseUnmapAllMappingsEvent;
+  }
+
   Handle = NULL;
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &Handle,
                   &gEdkiiIoMmuProtocolGuid, &mAmdSev,
                   NULL
                   );
+  if (EFI_ERROR (Status)) {
+    goto CloseExitBootEvent;
+  }
+
+  return EFI_SUCCESS;
+
+CloseExitBootEvent:
+  gBS->CloseEvent (ExitBootEvent);
+
+CloseUnmapAllMappingsEvent:
+  gBS->CloseEvent (UnmapAllMappingsEvent);
+
   return Status;
 }
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Yao, Jiewen]

Patch 1~3 reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, September 8, 2017 6:41 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Brijesh Singh
> <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at
> ExitBootServices
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: iommu_exit_boot
> 
> This series is the result of the discussion under
> 
>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>                      buffers at ExitBootServices()
>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
> 
> At ExitBootServices(), PCI and VirtIo drivers should only care about
> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> ultimately boil down to IOMMU mappings) for those aborted DMA operations
> should be the job of the IOMMU driver.
> 
> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
> and disables BMDMA in the wrong order in its DriverBindingStop()
> function, (b) it doesn't abort pending DMA at ExitBootServices().
> 
> This subset can be treated separately from the rest of the series, but I
> thought they belonged loosely together (given that AtaAtapiPassThru is
> used on QEMU's Q35 machine type).
> 
> Patches 04 through 07 remove
> VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
> calls from the VirtIo drivers' ExitBootServices() handlers.
> 
> (The conversion of VirtioNetDxe to device addresses is still in progress
> -- Brijesh, when you submit v2 of that, under this approach, there is no
> need to change VirtioNetExitBoot() relative to current upstream, and you
> can use VirtioOperationBusMasterRead to map outgoing packets.)
> 
> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
> abort pending DMA first, and IoMmuDxe clean up the mappings last.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (10):
>   MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
>   MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
>     DMA
>   MdeModulePkg/AtaAtapiPassThru: disable the device at
>     ExitBootServices()
>   OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
>     ExitBootServices
>   OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/IoMmuDxe: track all mappings
>   OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
>   OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()
> 
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246
> +++++++++++++++++---
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
>  OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
>  OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
>  7 files changed, 299 insertions(+), 101 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Yao, Jiewen]

Patch 8~10: reviewed-by: Jiewen.yao@intel.com

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Friday, September 8, 2017 3:28 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

Patch 1~3 reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, September 8, 2017 6:41 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Brijesh Singh
> <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Yao, Jiewen
> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Zeng,
> Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at
> ExitBootServices
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: iommu_exit_boot
>
> This series is the result of the discussion under
>
>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>                      buffers at ExitBootServices()
>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
>
> At ExitBootServices(), PCI and VirtIo drivers should only care about
> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> ultimately boil down to IOMMU mappings) for those aborted DMA operations
> should be the job of the IOMMU driver.
>
> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
> and disables BMDMA in the wrong order in its DriverBindingStop()
> function, (b) it doesn't abort pending DMA at ExitBootServices().
>
> This subset can be treated separately from the rest of the series, but I
> thought they belonged loosely together (given that AtaAtapiPassThru is
> used on QEMU's Q35 machine type).
>
> Patches 04 through 07 remove
> VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
> calls from the VirtIo drivers' ExitBootServices() handlers.
>
> (The conversion of VirtioNetDxe to device addresses is still in progress
> -- Brijesh, when you submit v2 of that, under this approach, there is no
> need to change VirtioNetExitBoot() relative to current upstream, and you
> can use VirtioOperationBusMasterRead to map outgoing packets.)
>
> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
> abort pending DMA first, and IoMmuDxe clean up the mappings last.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
> Cc: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Jordan Justen <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (10):
>   MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
>   MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
>     DMA
>   MdeModulePkg/AtaAtapiPassThru: disable the device at
>     ExitBootServices()
>   OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
>     ExitBootServices
>   OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/IoMmuDxe: track all mappings
>   OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
>   OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()
>
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246
> +++++++++++++++++---
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
>  OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
>  OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
>  7 files changed, 299 insertions(+), 101 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com> to MdeModulePkg changes.

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, September 8, 2017 6:41 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

Repo:   https://github.com/lersek/edk2.git
Branch: iommu_exit_boot

This series is the result of the discussion under

  [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
                     buffers at ExitBootServices()
  https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html

At ExitBootServices(), PCI and VirtIo drivers should only care about aborting pending DMA on the devices. Cleaning up PciIo mappings (which ultimately boil down to IOMMU mappings) for those aborted DMA operations should be the job of the IOMMU driver.

Patches 01 through 03 clean up the AtaAtapiPassThru driver in MdeModulePkg a little bit, because at present, (a) it unmaps the buffers and disables BMDMA in the wrong order in its DriverBindingStop() function, (b) it doesn't abort pending DMA at ExitBootServices().

This subset can be treated separately from the rest of the series, but I thought they belonged loosely together (given that AtaAtapiPassThru is used on QEMU's Q35 machine type).

Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
calls from the VirtIo drivers' ExitBootServices() handlers.

(The conversion of VirtioNetDxe to device addresses is still in progress
-- Brijesh, when you submit v2 of that, under this approach, there is no need to change VirtioNetExitBoot() relative to current upstream, and you can use VirtioOperationBusMasterRead to map outgoing packets.)

Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and unmap all mappings (Read, Write, CommonBuffer) that are in effect when
ExitBootServices() is called. It is ensured that PCI and VirtIo drivers abort pending DMA first, and IoMmuDxe clean up the mappings last.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (10):
  MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
  MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
    DMA
  MdeModulePkg/AtaAtapiPassThru: disable the device at
    ExitBootServices()
  OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
    ExitBootServices
  OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
  OvmfPkg/IoMmuDxe: track all mappings
  OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
  OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246 +++++++++++++++++---
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
 OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
 OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
 7 files changed, 299 insertions(+), 101 deletions(-)

--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Ard Biesheuvel]

(cc'ing the trinity)

On 7 September 2017 at 23:41, Laszlo Ersek <lersek@redhat.com> wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: iommu_exit_boot
>
> This series is the result of the discussion under
>
>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>                      buffers at ExitBootServices()
>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
>
> At ExitBootServices(), PCI and VirtIo drivers should only care about
> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> ultimately boil down to IOMMU mappings) for those aborted DMA operations
> should be the job of the IOMMU driver.
>
> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
> and disables BMDMA in the wrong order in its DriverBindingStop()
> function, (b) it doesn't abort pending DMA at ExitBootServices().
>
> This subset can be treated separately from the rest of the series, but I
> thought they belonged loosely together (given that AtaAtapiPassThru is
> used on QEMU's Q35 machine type).
>
> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
> calls from the VirtIo drivers' ExitBootServices() handlers.
>
> (The conversion of VirtioNetDxe to device addresses is still in progress
> -- Brijesh, when you submit v2 of that, under this approach, there is no
> need to change VirtioNetExitBoot() relative to current upstream, and you
> can use VirtioOperationBusMasterRead to map outgoing packets.)
>
> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
> abort pending DMA first, and IoMmuDxe clean up the mappings last.
>

The patches look fine to me

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Given that we are now depending on events signalled in an event
handler to be queued after all currently pending events, I think we
need to explicitly agree that this behavior that needs to be
preserved, and documented somewhere, given that the UEFI spec does not
offer this guarantee.

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Laszlo Ersek]

On 09/08/17 10:53, Ard Biesheuvel wrote:
> (cc'ing the trinity)
> 
> On 7 September 2017 at 23:41, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: iommu_exit_boot
>>
>> This series is the result of the discussion under
>>
>>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>>                      buffers at ExitBootServices()
>>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
>>
>> At ExitBootServices(), PCI and VirtIo drivers should only care about
>> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
>> ultimately boil down to IOMMU mappings) for those aborted DMA operations
>> should be the job of the IOMMU driver.
>>
>> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
>> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
>> and disables BMDMA in the wrong order in its DriverBindingStop()
>> function, (b) it doesn't abort pending DMA at ExitBootServices().
>>
>> This subset can be treated separately from the rest of the series, but I
>> thought they belonged loosely together (given that AtaAtapiPassThru is
>> used on QEMU's Q35 machine type).
>>
>> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
>> calls from the VirtIo drivers' ExitBootServices() handlers.
>>
>> (The conversion of VirtioNetDxe to device addresses is still in progress
>> -- Brijesh, when you submit v2 of that, under this approach, there is no
>> need to change VirtioNetExitBoot() relative to current upstream, and you
>> can use VirtioOperationBusMasterRead to map outgoing packets.)
>>
>> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
>> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
>> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
>> abort pending DMA first, and IoMmuDxe clean up the mappings last.
>>
> 
> The patches look fine to me
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thank you!

> Given that we are now depending on events signalled in an event
> handler to be queued after all currently pending events,

marking this (*)

> I think we
> need to explicitly agree that this behavior that needs to be
> preserved, and documented somewhere, given that the UEFI spec does not
> offer this guarantee.

The condition that you describe under (*) *is* guaranteed in the UEFI spec.

The *only* bit that is edk2 specific in the last patch is that we invoke
gBS->SignalEvent() from the notification function of an event that
*specifically* has type EVT_SIGNAL_EXIT_BOOT_SERVICES.

If the event, whose notification function we were calling
gBS->SignalEvent() from, was a plain EVT_NOTIFY_SIGNAL type event, then
*nothing* in the last patch would be edk2-specific.

* It is guaranteed by the UEFI spec that signaling an event group queues
the notification functions for all not-yet-signaled events in that
group, before the first notification function is invoked (regardless of
the signaled events' TPLs). From "CreateEventEx()": "All events are
guaranteed to be signaled before the first notification action is taken."

* The UEFI spec guarantees that, within the same TPL, if an event is
signaled that is not pending yet, the notify function will be queued
after notify functions already queued on the same TPL. See "CreateEvent()":

    Events exist in one of two states, “waiting” or “signaled.” When an
    event is created, firmware puts it in the “waiting” state. When the
    event is signaled, firmware changes its state to “signaled” and, if
    EVT_NOTIFY_SIGNAL is specified, places a call to its notification
    function in a FIFO queue. There is a queue for each of the “basic”
    task priority levels defined in Section 7.1 (TPL_CALLBACK, and
    TPL_NOTIFY). The functions in these queues are invoked in FIFO
    order, starting with the highest priority level queue and proceeding
    to the lowest priority queue that is unmasked by the current TPL. If
    the current TPL is equal to or greater than the queued notification,
    it will wait until the TPL is lowered via
    EFI_BOOT_SERVICES.RestoreTPL().

* gBS->SignalEvent() is valid to call at TPL_CALLBACK and TPL_NOTIFY
levels (see "Table 23. TPL Restrictions"); in fact it is one of the few
services that can even be called at TPL_HIGH_LEVEL (which is reserved
for the platform firmware).

The upshot is:

(1) assume you have Event1, Event2, Event3, Event4 in event group
EventGroupFoobarGuid

(2) assume all events are EVT_NOTIFY_SIGNAL type

(3) assume Event1 and Event2 have TPL_NOTIFY, Event3 and Event4 are
TPL_CALLBACK

(4) Assume Event5 is also of type EVT_NOTIFY_SIGNAL, not part of any
event group, and has TPL_CALLBACK task prio level

(5) Assume an agent running at TPL_APPLICATION creates an event
temporarily, with type 0 (see the code example in CreateEventEx), and
signals EventGroupFoobarGuid through it.

(6) All of Event1, Event2, Event3 and Event4 move into the signaled
state at once. NotifyFunction1 and NotifyFunction2 are queued in
unspecified order in the TPL_NOTIFY queue, and Event3 and Event4 are
queued in unspecified order in the TPL_CALLBACK queue.

(7) Because our current TPL is TPL_APPLICATION, and signaled events of
type EVT_NOTIFY_SIGNAL with higher TPLs exist, the SignalEvent() service
will start dispatching them immediately.

(8) Either NotifyFunction1 or NotifyFunction2 is called first, running
at TPL_NOTIFY.

(9) Assume that NotifyFunction2 signals Event5. (it doesn't matter if
NotifyFunction2 is called before or after NotifyFunction1).

NotifyFunction5 is not dispatched immediately (on the stack of
SignalEvent()) because the current TPL, TPL_NOTIFY, is higher than or
equal to, the TPL of Event5, which is TPL_CALLBACK. So NotifyFunction5
is queued instead.

Because NotifyFunction3 and NotifyFunction4 are already in the
TPL_CALLBACK queue (in unspecified relative order), from step (6),
NotifyFunction5 will be queued after both of them, in FIFO order.

(10) If NotifyFunction1 has not been invoked yet, it is invoked now. It
runs at TPL_NOTIFY. The TPL_NOTIFY queue becomes empty.

(11) NotifyFunction3 and NotifyFunction4 are invoked in unspecified
order. They both run at TPL_CALLBACK.

(12) NotifyFunction5 is invoked. It runs at TPL_CALLBACK. The
TPL_CALLBACK queue becomes empty.

(13) SignalEvent() returns to the agent mentioned in (5).


The one thing to remember, from the client programmer's POV, is that
*any* event of type EVT_NOTIFY_SIGNAL can only be dispatched from within
two service calls:

- SignalEvent(), if the caller is running at a TPL strictly below the
TPL of the event being signaled (directly or via event group GUID), and

- RestoreTPL(), if the restored TPL falls strictly below the TPL of the
event, and the event has already been signaled.

IOW, SignalEvent() "calls or queues", and RestoreTPL "de-queues and
calls, or does nothing".


So, again, the only question that the UEFI spec does not clarify is
whether we can call gBS->SignalEvent() specifically from an EBS
notification function.

The intent is likely just that, because:

  EVT_SIGNAL_EXIT_BOOT_SERVICES
    [...] This event is of type EVT_NOTIFY_SIGNAL [...]

and, again, if we call SignalEvent from the NotifyFunction of a plain
EVT_NOTIFY_SIGNAL event, then it's all covered by the spec already.

My apologies about the wall of text. I probably over-explained it five-fold.

Thanks
Laszlo

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Ard Biesheuvel]

On 8 September 2017 at 10:48, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/08/17 10:53, Ard Biesheuvel wrote:
>> (cc'ing the trinity)
>>
>> On 7 September 2017 at 23:41, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: iommu_exit_boot
>>>
>>> This series is the result of the discussion under
>>>
>>>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>>>                      buffers at ExitBootServices()
>>>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
>>>
>>> At ExitBootServices(), PCI and VirtIo drivers should only care about
>>> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
>>> ultimately boil down to IOMMU mappings) for those aborted DMA operations
>>> should be the job of the IOMMU driver.
>>>
>>> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
>>> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
>>> and disables BMDMA in the wrong order in its DriverBindingStop()
>>> function, (b) it doesn't abort pending DMA at ExitBootServices().
>>>
>>> This subset can be treated separately from the rest of the series, but I
>>> thought they belonged loosely together (given that AtaAtapiPassThru is
>>> used on QEMU's Q35 machine type).
>>>
>>> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
>>> calls from the VirtIo drivers' ExitBootServices() handlers.
>>>
>>> (The conversion of VirtioNetDxe to device addresses is still in progress
>>> -- Brijesh, when you submit v2 of that, under this approach, there is no
>>> need to change VirtioNetExitBoot() relative to current upstream, and you
>>> can use VirtioOperationBusMasterRead to map outgoing packets.)
>>>
>>> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
>>> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
>>> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
>>> abort pending DMA first, and IoMmuDxe clean up the mappings last.
>>>
>>
>> The patches look fine to me
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Thank you!
>
>> Given that we are now depending on events signalled in an event
>> handler to be queued after all currently pending events,
>
> marking this (*)
>
>> I think we
>> need to explicitly agree that this behavior that needs to be
>> preserved, and documented somewhere, given that the UEFI spec does not
>> offer this guarantee.
>
> The condition that you describe under (*) *is* guaranteed in the UEFI spec.
>
> The *only* bit that is edk2 specific in the last patch is that we invoke
> gBS->SignalEvent() from the notification function of an event that
> *specifically* has type EVT_SIGNAL_EXIT_BOOT_SERVICES.
>
> If the event, whose notification function we were calling
> gBS->SignalEvent() from, was a plain EVT_NOTIFY_SIGNAL type event, then
> *nothing* in the last patch would be edk2-specific.
>
> * It is guaranteed by the UEFI spec that signaling an event group queues
> the notification functions for all not-yet-signaled events in that
> group, before the first notification function is invoked (regardless of
> the signaled events' TPLs). From "CreateEventEx()": "All events are
> guaranteed to be signaled before the first notification action is taken."
>
> * The UEFI spec guarantees that, within the same TPL, if an event is
> signaled that is not pending yet, the notify function will be queued
> after notify functions already queued on the same TPL. See "CreateEvent()":
>
>     Events exist in one of two states, “waiting” or “signaled.” When an
>     event is created, firmware puts it in the “waiting” state. When the
>     event is signaled, firmware changes its state to “signaled” and, if
>     EVT_NOTIFY_SIGNAL is specified, places a call to its notification
>     function in a FIFO queue. There is a queue for each of the “basic”
>     task priority levels defined in Section 7.1 (TPL_CALLBACK, and
>     TPL_NOTIFY). The functions in these queues are invoked in FIFO
>     order, starting with the highest priority level queue and proceeding
>     to the lowest priority queue that is unmasked by the current TPL. If
>     the current TPL is equal to or greater than the queued notification,
>     it will wait until the TPL is lowered via
>     EFI_BOOT_SERVICES.RestoreTPL().
>
> * gBS->SignalEvent() is valid to call at TPL_CALLBACK and TPL_NOTIFY
> levels (see "Table 23. TPL Restrictions"); in fact it is one of the few
> services that can even be called at TPL_HIGH_LEVEL (which is reserved
> for the platform firmware).
>
> The upshot is:
>
> (1) assume you have Event1, Event2, Event3, Event4 in event group
> EventGroupFoobarGuid
>
> (2) assume all events are EVT_NOTIFY_SIGNAL type
>
> (3) assume Event1 and Event2 have TPL_NOTIFY, Event3 and Event4 are
> TPL_CALLBACK
>
> (4) Assume Event5 is also of type EVT_NOTIFY_SIGNAL, not part of any
> event group, and has TPL_CALLBACK task prio level
>
> (5) Assume an agent running at TPL_APPLICATION creates an event
> temporarily, with type 0 (see the code example in CreateEventEx), and
> signals EventGroupFoobarGuid through it.
>
> (6) All of Event1, Event2, Event3 and Event4 move into the signaled
> state at once. NotifyFunction1 and NotifyFunction2 are queued in
> unspecified order in the TPL_NOTIFY queue, and Event3 and Event4 are
> queued in unspecified order in the TPL_CALLBACK queue.
>
> (7) Because our current TPL is TPL_APPLICATION, and signaled events of
> type EVT_NOTIFY_SIGNAL with higher TPLs exist, the SignalEvent() service
> will start dispatching them immediately.
>
> (8) Either NotifyFunction1 or NotifyFunction2 is called first, running
> at TPL_NOTIFY.
>
> (9) Assume that NotifyFunction2 signals Event5. (it doesn't matter if
> NotifyFunction2 is called before or after NotifyFunction1).
>
> NotifyFunction5 is not dispatched immediately (on the stack of
> SignalEvent()) because the current TPL, TPL_NOTIFY, is higher than or
> equal to, the TPL of Event5, which is TPL_CALLBACK. So NotifyFunction5
> is queued instead.
>
> Because NotifyFunction3 and NotifyFunction4 are already in the
> TPL_CALLBACK queue (in unspecified relative order), from step (6),
> NotifyFunction5 will be queued after both of them, in FIFO order.
>
> (10) If NotifyFunction1 has not been invoked yet, it is invoked now. It
> runs at TPL_NOTIFY. The TPL_NOTIFY queue becomes empty.
>
> (11) NotifyFunction3 and NotifyFunction4 are invoked in unspecified
> order. They both run at TPL_CALLBACK.
>
> (12) NotifyFunction5 is invoked. It runs at TPL_CALLBACK. The
> TPL_CALLBACK queue becomes empty.
>
> (13) SignalEvent() returns to the agent mentioned in (5).
>
>
> The one thing to remember, from the client programmer's POV, is that
> *any* event of type EVT_NOTIFY_SIGNAL can only be dispatched from within
> two service calls:
>
> - SignalEvent(), if the caller is running at a TPL strictly below the
> TPL of the event being signaled (directly or via event group GUID), and
>
> - RestoreTPL(), if the restored TPL falls strictly below the TPL of the
> event, and the event has already been signaled.
>
> IOW, SignalEvent() "calls or queues", and RestoreTPL "de-queues and
> calls, or does nothing".
>
>
> So, again, the only question that the UEFI spec does not clarify is
> whether we can call gBS->SignalEvent() specifically from an EBS
> notification function.
>
> The intent is likely just that, because:
>
>   EVT_SIGNAL_EXIT_BOOT_SERVICES
>     [...] This event is of type EVT_NOTIFY_SIGNAL [...]
>
> and, again, if we call SignalEvent from the NotifyFunction of a plain
> EVT_NOTIFY_SIGNAL event, then it's all covered by the spec already.
>
> My apologies about the wall of text. I probably over-explained it five-fold.
>

OK

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Brijesh Singh]

Patch 4 - 10

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Tested-by: Brijesh Singh <brijesh.singh@amd.com>

Thank you Laszlo! I will work to finish virtio-net next week.

-Brijesh


On 09/07/2017 05:41 PM, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: iommu_exit_boot
> 
> This series is the result of the discussion under
> 
>    [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>                       buffers at ExitBootServices()
>    https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
> 
> At ExitBootServices(), PCI and VirtIo drivers should only care about
> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> ultimately boil down to IOMMU mappings) for those aborted DMA operations
> should be the job of the IOMMU driver.
> 
> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
> and disables BMDMA in the wrong order in its DriverBindingStop()
> function, (b) it doesn't abort pending DMA at ExitBootServices().
> 
> This subset can be treated separately from the rest of the series, but I
> thought they belonged loosely together (given that AtaAtapiPassThru is
> used on QEMU's Q35 machine type).
> 
> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
> calls from the VirtIo drivers' ExitBootServices() handlers.
> 
> (The conversion of VirtioNetDxe to device addresses is still in progress
> -- Brijesh, when you submit v2 of that, under this approach, there is no
> need to change VirtioNetExitBoot() relative to current upstream, and you
> can use VirtioOperationBusMasterRead to map outgoing packets.)
> 
> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
> abort pending DMA first, and IoMmuDxe clean up the mappings last.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (10):
>    MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
>    MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
>      DMA
>    MdeModulePkg/AtaAtapiPassThru: disable the device at
>      ExitBootServices()
>    OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
>    OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
>      ExitBootServices
>    OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
>    OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
>    OvmfPkg/IoMmuDxe: track all mappings
>    OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
>    OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()
> 
>   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
>   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246 +++++++++++++++++---
>   OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
>   OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
>   OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
>   OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
>   7 files changed, 299 insertions(+), 101 deletions(-)
> 

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Laszlo Ersek]

On 09/08/17 17:50, Brijesh Singh wrote:
> Patch 4 - 10
> 
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
> Tested-by: Brijesh Singh <brijesh.singh@amd.com>
> 
> Thank you Laszlo! I will work to finish virtio-net next week.

Awesome!

Thanks!
Laszlo

> On 09/07/2017 05:41 PM, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: iommu_exit_boot
>>
>> This series is the result of the discussion under
>>
>>    [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>>                       buffers at ExitBootServices()
>>    https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
>>
>> At ExitBootServices(), PCI and VirtIo drivers should only care about
>> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
>> ultimately boil down to IOMMU mappings) for those aborted DMA operations
>> should be the job of the IOMMU driver.
>>
>> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
>> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
>> and disables BMDMA in the wrong order in its DriverBindingStop()
>> function, (b) it doesn't abort pending DMA at ExitBootServices().
>>
>> This subset can be treated separately from the rest of the series, but I
>> thought they belonged loosely together (given that AtaAtapiPassThru is
>> used on QEMU's Q35 machine type).
>>
>> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
>> calls from the VirtIo drivers' ExitBootServices() handlers.
>>
>> (The conversion of VirtioNetDxe to device addresses is still in progress
>> -- Brijesh, when you submit v2 of that, under this approach, there is no
>> need to change VirtioNetExitBoot() relative to current upstream, and you
>> can use VirtioOperationBusMasterRead to map outgoing packets.)
>>
>> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
>> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
>> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
>> abort pending DMA first, and IoMmuDxe clean up the mappings last.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (10):
>>    MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
>>    MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
>>      DMA
>>    MdeModulePkg/AtaAtapiPassThru: disable the device at
>>      ExitBootServices()
>>    OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
>>    OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
>>      ExitBootServices
>>    OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
>>    OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
>>    OvmfPkg/IoMmuDxe: track all mappings
>>    OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
>>    OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()
>>
>>   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
>>   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
>>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246
>> +++++++++++++++++---
>>   OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
>>   OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
>>   OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
>>   OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
>>   7 files changed, 299 insertions(+), 101 deletions(-)
>>

Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices

[Laszlo Ersek]

On 09/08/17 00:41, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: iommu_exit_boot
> 
> This series is the result of the discussion under
> 
>   [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common
>                      buffers at ExitBootServices()
>   https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html
> 
> At ExitBootServices(), PCI and VirtIo drivers should only care about
> aborting pending DMA on the devices. Cleaning up PciIo mappings (which
> ultimately boil down to IOMMU mappings) for those aborted DMA operations
> should be the job of the IOMMU driver.
> 
> Patches 01 through 03 clean up the AtaAtapiPassThru driver in
> MdeModulePkg a little bit, because at present, (a) it unmaps the buffers
> and disables BMDMA in the wrong order in its DriverBindingStop()
> function, (b) it doesn't abort pending DMA at ExitBootServices().
> 
> This subset can be treated separately from the rest of the series, but I
> thought they belonged loosely together (given that AtaAtapiPassThru is
> used on QEMU's Q35 machine type).
> 
> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer()
> calls from the VirtIo drivers' ExitBootServices() handlers.
> 
> (The conversion of VirtioNetDxe to device addresses is still in progress
> -- Brijesh, when you submit v2 of that, under this approach, there is no
> need to change VirtioNetExitBoot() relative to current upstream, and you
> can use VirtioOperationBusMasterRead to map outgoing packets.)
> 
> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and
> unmap all mappings (Read, Write, CommonBuffer) that are in effect when
> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers
> abort pending DMA first, and IoMmuDxe clean up the mappings last.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (10):
>   MdeModulePkg/AtaAtapiPassThru: cache EnabledPciAttributes
>   MdeModulePkg/AtaAtapiPassThru: unmap DMA buffers after disabling BM
>     DMA
>   MdeModulePkg/AtaAtapiPassThru: disable the device at
>     ExitBootServices()
>   OvmfPkg/VirtioBlkDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioGpuDxe: don't unmap VRING & BackingStore at
>     ExitBootServices
>   OvmfPkg/VirtioRngDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/VirtioScsiDxe: don't unmap VRING at ExitBootServices()
>   OvmfPkg/IoMmuDxe: track all mappings
>   OvmfPkg/IoMmuDxe: generalize IoMmuUnmap() to IoMmuUnmapWorker()
>   OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()
> 
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 103 +++++---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h |   7 +
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 246 +++++++++++++++++---
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c                         |   7 +-
>  OvmfPkg/VirtioGpuDxe/Commands.c                          |  23 +-
>  OvmfPkg/VirtioRngDxe/VirtioRng.c                         |   7 +-
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                       |   7 +-
>  7 files changed, 299 insertions(+), 101 deletions(-)
> 

Supreme kudos to everyone for the feedback!; series pushed as commit
range 3281ebb4ae7d..7aee391fa3d0.

Laszlo