Re: [PATCH V2 1/4] OvmfPkg/IoMmuDxe: Reserve shared memory region for DMA operation

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 12/12/22 23:48, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4171
> 
> A typical QEMU fw_cfg read bytes with IOMMU for td guest is that:
> (QemuFwCfgReadBytes@QemuFwCfgLib.c is the example)
> 1) Allocate DMA Access buffer
> 2) Map actual data buffer
> 3) start the transfer and wait for the transfer to complete
> 4) Free DMA Access buffer
> 5) Un-map actual data buffer
> 
> In step 1/2, Private memories are allocated, converted to shared memories.
> In Step 4/5 the shared memories are converted to private memories and
> accepted again. The final step is to free the pages.
> 
> This is time-consuming and impacts td guest's boot perf (both direct boot
> and grub boot) badly.
> 
> In a typical grub boot, there are about 5000 calls of page allocation and
> private/share conversion. Most of page size is less than 32KB.
> 
> This patch allocates a memory region and initializes it into pieces of
> memory with different sizes. A piece of such memory consists of 2 parts:
> the first page is of private memory, and the other pages are shared
> memory. This is to meet the layout of common buffer.
> 
> When allocating bounce buffer in IoMmuMap(), IoMmuAllocateBounceBuffer()
> is called to allocate the buffer. Accordingly when freeing bounce buffer
> in IoMmuUnmapWorker(), IoMmuFreeBounceBuffer() is called to free the
> bounce buffer. CommonBuffer is allocated by IoMmuAllocateCommonBuffer
> and accordingly freed by IoMmuFreeCommonBuffer.
> 
> This feature is tested in Intel TDX pre-production platform. It saves up
> to hundreds of ms in a grub boot.

This patch causes crashes for SEV guests and breaks bisect-ability of the 
EDK2 tree. See below...

> 
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c   | 142 +++++-----
>   OvmfPkg/IoMmuDxe/IoMmuBuffer.c   | 459 +++++++++++++++++++++++++++++++
>   OvmfPkg/IoMmuDxe/IoMmuDxe.inf    |   1 +
>   OvmfPkg/IoMmuDxe/IoMmuInternal.h | 179 ++++++++++++
>   4 files changed, 710 insertions(+), 71 deletions(-)
>   create mode 100644 OvmfPkg/IoMmuDxe/IoMmuBuffer.c
>   create mode 100644 OvmfPkg/IoMmuDxe/IoMmuInternal.h
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 6b65897f032a..77e46bbf4a60 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c


> @@ -633,8 +614,9 @@ IoMmuAllocateBuffer (
>     CommonBufferHeader = (VOID *)(UINTN)PhysicalAddress;
>     PhysicalAddress   += EFI_PAGE_SIZE;
>   
> -  CommonBufferHeader->Signature   = COMMON_BUFFER_SIG;
> -  CommonBufferHeader->StashBuffer = StashBuffer;
> +  CommonBufferHeader->Signature         = COMMON_BUFFER_SIG;
> +  CommonBufferHeader->StashBuffer       = StashBuffer;
> +  CommonBufferHeader->ReservedMemBitmap = ReservedMemBitmap;
>   
>     *HostAddress = (VOID *)(UINTN)PhysicalAddress;
>   
> @@ -707,7 +689,7 @@ IoMmuFreeBuffer (
>     // Release the common buffer itself. Unmap() has re-encrypted it in-place, so
>     // no need to zero it.
>     //
> -  return gBS->FreePages ((UINTN)CommonBufferHeader, CommonBufferPages);
> +  return IoMmuFreeCommonBuffer (CommonBufferHeader, CommonBufferPages);
>   }
>   
>   /**
> @@ -878,6 +860,11 @@ IoMmuUnmapAllMappings (
>         TRUE      // MemoryMapLocked
>         );
>     }
> +
> +  //
> +  // Release the reserved shared memory as well.
> +  //
> +  IoMmuReleaseReservedSharedMem (TRUE);

This call is the reason for the crash. You'll need to check for whether 
there has been any shared memory reserved before attempting to free it (in 
the case of SEV that doesn't happen until patch #3 of the series). I think 
adding the check in IoMmuReleaseReservedSharedMem() itself might be best, 
since you can also experience this crash should the below allocation fail, 
too.

Thanks,
Tom

>   }
>   
>   /**
> @@ -936,6 +923,19 @@ InstallIoMmuProtocol (
>       goto CloseExitBootEvent;
>     }
>   
> +  //
> +  // Currently only Tdx guest support Reserved shared memory for DMA operation.
> +  //
> +  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +    mReservedSharedMemSupported = TRUE;
> +    Status                      = IoMmuInitReservedSharedMem ();
> +    if (EFI_ERROR (Status)) {
> +      mReservedSharedMemSupported = FALSE;
> +    } else {
> +      DEBUG ((DEBUG_INFO, "%a: Feature of reserved memory for DMA is supported.\n", __FUNCTION__));
> +    }
> +  }
> +
>     return EFI_SUCCESS;
>   
>   CloseExitBootEvent: