Re: [edk2-devel] [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer

["Jeff Brasen" <jbrasen@nvidia.com>]

Any thoughts on this patch, now that the stable tag is out?

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, February 15, 2022 11:52 PM
> To: Sindhu, Deepthi <deepthi.sindhu@intel.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Jeff Brasen <jbrasen@nvidia.com>;
> devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> External email: Use caution opening links or attachments
> 
> 
> Deepthi,
> Can you please help to review the changes and provide comments?
> 
> Thanks,
> Ray
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
> Brasen via groups.io
> Sent: Tuesday, February 15, 2022 2:46 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Jeff
> Brasen <jbrasen@nvidia.com>
> Subject: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> Add support for partial free of non cached buffers.
> If a request for less than the full size is requested new allocations for the
> remaining head and tail of the buffer are added to the list.
> Added verification that Buffer is EFI_PAGE_SIZE aligned.
> The XHCI driver does this if the page size for the controller is >4KB.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  .../NonDiscoverablePciDeviceIo.c              | 53 ++++++++++++++++++-
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> index c1c5c6267c..77809cfedf 100644
> ---
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.c
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceIo.c
> @@ -960,12 +960,23 @@ NonCoherentPciIoFreeBuffer (
>    LIST_ENTRY                                   *Entry;
>    EFI_STATUS                                   Status;
>    NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION  *Alloc;
> +  NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION  *AllocHead;
> + NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION  *AllocTail;
>    BOOLEAN                                      Found;
> +  UINTN                                        StartPages;
> +  UINTN                                        EndPages;
> +
> +  if (HostAddress != ALIGN_POINTER (HostAddress, EFI_PAGE_SIZE)) {
> +    ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> +    return EFI_INVALID_PARAMETER;
> +  }
> 
>    Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO (This);
> 
>    Found = FALSE;
>    Alloc = NULL;
> +  AllocHead = NULL;
> +  AllocTail = NULL;
> 
>    //
>    // Find the uncached allocation list entry associated @@ -976,9 +987,13 @@
> NonCoherentPciIoFreeBuffer (
>         Entry = Entry->ForwardLink)
>    {
>      Alloc = BASE_CR (Entry,
> NON_DISCOVERABLE_DEVICE_UNCACHED_ALLOCATION, List);
> -    if ((Alloc->HostAddress == HostAddress) && (Alloc->NumPages == Pages))
> {
> +    StartPages = 0;
> +    if (Alloc->HostAddress < HostAddress) {
> +      StartPages = (HostAddress - Alloc->HostAddress) / EFI_PAGE_SIZE;
> +    }
> +    if ((Alloc->HostAddress <= HostAddress) && (Alloc->NumPages >=
> + (Pages + StartPages))) {
>        //
> -      // We are freeing the exact allocation we were given
> +      // We are freeing at least part of what we were given
>        // before by AllocateBuffer()
>        //
>        Found = TRUE;
> @@ -991,7 +1006,41 @@ NonCoherentPciIoFreeBuffer (
>      return EFI_NOT_FOUND;
>    }
> 
> +  EndPages = Alloc->NumPages - (Pages + StartPages);
> +
> +  if (StartPages != 0) {
> +    AllocHead = AllocatePool (sizeof *AllocHead);
> +    if (AllocHead == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    AllocHead->HostAddress = Alloc->HostAddress;
> +    AllocHead->NumPages = StartPages;
> +    AllocHead->Attributes = Alloc->Attributes;  }
> +
> +  if (EndPages != 0) {
> +    AllocTail = AllocatePool (sizeof *AllocTail);
> +    if (AllocTail == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    AllocTail->HostAddress = Alloc->HostAddress + ((Pages + StartPages) *
> EFI_PAGE_SIZE);
> +    AllocTail->NumPages = EndPages;
> +    AllocTail->Attributes = Alloc->Attributes;  }
> +
>    RemoveEntryList (&Alloc->List);
> +  //
> +  // Record this new sub allocations in the linked list, so we  // can
> + restore the memory space attributes later  //  if (AllocHead != NULL)
> + {
> +    InsertHeadList (&Dev->UncachedAllocationList, &AllocHead->List);  }
> + if (AllocTail != NULL) {
> +    InsertHeadList (&Dev->UncachedAllocationList, &AllocTail->List);  }
> 
>    Status = gDS->SetMemorySpaceAttributes (
>                    (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress,
> --
> 2.17.1
> 
> 
> 
> 
>