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

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

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Friday, July 29, 2022 9:48 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; hao.a.wu@intel.com; ray.ni@intel.com;
> quic_llindhol@quicinc.com; ardb+tianocore@kernel.org
> Subject: Re: [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe:
> Allow partial FreeBuffer
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 28 Jul 2022 at 13:25, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> >
> > Adding Leif/Ard to CC incase they have any comments on this patch.
> >
> 
> This generally looks ok to me. I just wonder if it wouldn't be simpler to reuse
> the existing allocation descriptor if it is not being freed entirely. Given the
> [presumably] the most common case is to allocate and then free some pages
> at the end, lowering the page count on the existing descriptor would cover
> most cases, and we'd only need to allocate new ones if pages are being freed
> at the start or in the middle.

There is often freeing at the beginning as well as this is being used to create a 64K aligned section of memory in the case. So it over allocates and the free's some at the beginning and the end. I could probably make it detect and use that but figured this code would support all cases and required less case specific detection.

-Jeff

> 
> 
> > > -----Original Message-----
> > > From: Jeff Brasen
> > > Sent: Friday, June 17, 2022 9:39 AM
> > > To: devel@edk2.groups.io
> > > Cc: hao.a.wu@intel.com; ray.ni@intel.com
> > > Subject: RE: [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe:
> > > Allow partial FreeBuffer
> > >
> > > Any thoughts on this patch?
> > >
> > > > -----Original Message-----
> > > > From: Jeff Brasen <jbrasen@nvidia.com>
> > > > Sent: Monday, February 14, 2022 11:46 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: hao.a.wu@intel.com; ray.ni@intel.com; Jeff Brasen
> > > > <jbrasen@nvidia.com>
> > > > Subject: [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
> >