[PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer

[PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer

[Jeff Brasen]

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/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index c1c5c6267c..77809cfedf 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.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

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

[Ni, Ray]

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/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index c1c5c6267c..77809cfedf 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc
+++ 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

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

[Jeff Brasen]

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
> 
> 
> 
> 
> 

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

[Jeff Brasen]

[-- Attachment #1: Type: text/plain, Size: 48 bytes --]

Any comments on this patch?

Thanks,

Jeff

[-- Attachment #2: Type: text/html, Size: 200 bytes --]

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

[Jeff Brasen]

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

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

[Jeff Brasen]

Adding Leif/Ard to CC incase they have any comments on this patch.

Thanks
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

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

[Ard Biesheuvel]

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.


> > -----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
>

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

[Jeff Brasen]

> -----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
> >

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

[Ard Biesheuvel]

On Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
>
>
> > -----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.
>

Ah interesting. Would it help if the allocate routine aligned
allocations to their size?

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

[Jeff Brasen]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> >
> >
> > > -----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.
> >
> 
> Ah interesting. Would it help if the allocate routine aligned allocations to their
> size?

The PciIo->AllocateBuffer function doesn't support passing the request in so we would need to know that info beforehand. The current calling in the XHCI driver does a free at the beginning and then the end of the buffer so we could the existing allocation tracker but figured it would be better to correct the function just in case someone called it to free in the middle.

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

[Ard Biesheuvel]

On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
<jbrasen=nvidia.com@groups.io> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > >
> > >
> > >
> > > > -----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.
> > >
> >
> > Ah interesting. Would it help if the allocate routine aligned allocations to their
> > size?
>
> The PciIo->AllocateBuffer function doesn't support passing the request in so we would need to know that info beforehand. The current calling in the XHCI driver does a free at the beginning and then the end of the buffer so we could the existing allocation tracker but figured it would be better to correct the function just in case someone called it to free in the middle.
>

What I was wondering is whether such allocations are themselves
multiples of 64k. This is perhaps orthogonal to the issue this patch
addresses, as we'' still need to deal with partial free calls
regardless. But I was curious whether XHCI in particular, and perhaps
more generally, we could streamline this by aligning all allocations
to a log2 upper bound of their sizes.

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

[Jeff Brasen]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, August 15, 2022 8:42 AM
> To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org
> Subject: Re: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> External email: Use caution opening links or attachments
> 
> 
> On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> <jbrasen=nvidia.com@groups.io> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----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.
> > > >
> > >
> > > Ah interesting. Would it help if the allocate routine aligned
> > > allocations to their size?
> >
> > The PciIo->AllocateBuffer function doesn't support passing the request in so
> we would need to know that info beforehand. The current calling in the XHCI
> driver does a free at the beginning and then the end of the buffer so we could
> the existing allocation tracker but figured it would be better to correct the
> function just in case someone called it to free in the middle.
> >
> 
> What I was wondering is whether such allocations are themselves multiples of
> 64k. This is perhaps orthogonal to the issue this patch addresses, as we'' still
> need to deal with partial free calls regardless. But I was curious whether XHCI in
> particular, and perhaps more generally, we could streamline this by aligning all
> allocations to a log2 upper bound of their sizes.

Xhci code (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c#L604) in allocation requested is greater the EFI_PAGE_SIZE allocates number of requested pages plus pages for the alignment and then frees pages at the beginning and end of the allocation.  I am not sure we really could change this without adding an alignment field to the PciIo protocol.

Is there anything else you would like to change on this patch?

Thanks,
Jeff

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

[Ard Biesheuvel]

On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Monday, August 15, 2022 8:42 AM
> > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> > ardb+tianocore@kernel.org
> > Subject: Re: [edk2-devel] [PATCH v2]
> > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> > <jbrasen=nvidia.com@groups.io> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----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.
> > > > >
> > > >
> > > > Ah interesting. Would it help if the allocate routine aligned
> > > > allocations to their size?
> > >
> > > The PciIo->AllocateBuffer function doesn't support passing the request in so
> > we would need to know that info beforehand. The current calling in the XHCI
> > driver does a free at the beginning and then the end of the buffer so we could
> > the existing allocation tracker but figured it would be better to correct the
> > function just in case someone called it to free in the middle.
> > >
> >
> > What I was wondering is whether such allocations are themselves multiples of
> > 64k. This is perhaps orthogonal to the issue this patch addresses, as we'' still
> > need to deal with partial free calls regardless. But I was curious whether XHCI in
> > particular, and perhaps more generally, we could streamline this by aligning all
> > allocations to a log2 upper bound of their sizes.
>
> Xhci code (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c#L604) in allocation requested is greater the EFI_PAGE_SIZE allocates number of requested pages plus pages for the alignment and then frees pages at the beginning and end of the allocation.  I am not sure we really could change this without adding an alignment field to the PciIo protocol.
>
> Is there anything else you would like to change on this patch?
>

No. Thanks for the clarification.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

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

[Jeff Brasen]

Anything else needed to get this merged?

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, September 8, 2022 9:55 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: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Monday, August 15, 2022 8:42 AM
> > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > > Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> > > ardb+tianocore@kernel.org
> > > Subject: Re: [edk2-devel] [PATCH v2]
> > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> > > <jbrasen=nvidia.com@groups.io> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----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.
> > > > > >
> > > > >
> > > > > Ah interesting. Would it help if the allocate routine aligned
> > > > > allocations to their size?
> > > >
> > > > The PciIo->AllocateBuffer function doesn't support passing the
> > > > request in so
> > > we would need to know that info beforehand. The current calling in
> > > the XHCI driver does a free at the beginning and then the end of the
> > > buffer so we could the existing allocation tracker but figured it
> > > would be better to correct the function just in case someone called it to free
> in the middle.
> > > >
> > >
> > > What I was wondering is whether such allocations are themselves
> > > multiples of 64k. This is perhaps orthogonal to the issue this patch
> > > addresses, as we'' still need to deal with partial free calls
> > > regardless. But I was curious whether XHCI in particular, and
> > > perhaps more generally, we could streamline this by aligning all allocations
> to a log2 upper bound of their sizes.
> >
> > Xhci code
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Xhci
> Dxe/UsbHcMem.c#L604) in allocation requested is greater the EFI_PAGE_SIZE
> allocates number of requested pages plus pages for the alignment and then
> frees pages at the beginning and end of the allocation.  I am not sure we really
> could change this without adding an alignment field to the PciIo protocol.
> >
> > Is there anything else you would like to change on this patch?
> >
> 
> No. Thanks for the clarification.
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

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

[Ard Biesheuvel]

On Wed, 21 Sept 2022 at 18:27, Jeff Brasen via groups.io
<jbrasen=nvidia.com@groups.io> wrote:
>
> Anything else needed to get this merged?
>

That is up to the MdeModulePkg maintainers.

> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, September 8, 2022 9:55 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: [edk2-devel] [PATCH v2]
> > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > Sent: Monday, August 15, 2022 8:42 AM
> > > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > > > Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> > > > ardb+tianocore@kernel.org
> > > > Subject: Re: [edk2-devel] [PATCH v2]
> > > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> > > > <jbrasen=nvidia.com@groups.io> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----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.
> > > > > > >
> > > > > >
> > > > > > Ah interesting. Would it help if the allocate routine aligned
> > > > > > allocations to their size?
> > > > >
> > > > > The PciIo->AllocateBuffer function doesn't support passing the
> > > > > request in so
> > > > we would need to know that info beforehand. The current calling in
> > > > the XHCI driver does a free at the beginning and then the end of the
> > > > buffer so we could the existing allocation tracker but figured it
> > > > would be better to correct the function just in case someone called it to free
> > in the middle.
> > > > >
> > > >
> > > > What I was wondering is whether such allocations are themselves
> > > > multiples of 64k. This is perhaps orthogonal to the issue this patch
> > > > addresses, as we'' still need to deal with partial free calls
> > > > regardless. But I was curious whether XHCI in particular, and
> > > > perhaps more generally, we could streamline this by aligning all allocations
> > to a log2 upper bound of their sizes.
> > >
> > > Xhci code
> > (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Xhci
> > Dxe/UsbHcMem.c#L604) in allocation requested is greater the EFI_PAGE_SIZE
> > allocates number of requested pages plus pages for the alignment and then
> > frees pages at the beginning and end of the allocation.  I am not sure we really
> > could change this without adding an alignment field to the PciIo protocol.
> > >
> > > Is there anything else you would like to change on this patch?
> > >
> >
> > No. Thanks for the clarification.
> >
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
>
> 
>
>

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

[Jeff Brasen]

MdeModulePkg maintainers, Any comments on this?

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, September 21, 2022 10:32 AM
> To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org
> Subject: Re: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 21 Sept 2022 at 18:27, Jeff Brasen via groups.io
> <jbrasen=nvidia.com@groups.io> wrote:
> >
> > Anything else needed to get this merged?
> >
> 
> That is up to the MdeModulePkg maintainers.
> 
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Thursday, September 8, 2022 9:55 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: [edk2-devel] [PATCH v2]
> > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > Sent: Monday, August 15, 2022 8:42 AM
> > > > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > > > > Cc: hao.a.wu@intel.com; ray.ni@intel.com;
> > > > > quic_llindhol@quicinc.com;
> > > > > ardb+tianocore@kernel.org
> > > > > Subject: Re: [edk2-devel] [PATCH v2]
> > > > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial
> > > > > FreeBuffer
> > > > >
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> > > > > <jbrasen=nvidia.com@groups.io> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com>
> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----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.
> > > > > > > >
> > > > > > >
> > > > > > > Ah interesting. Would it help if the allocate routine
> > > > > > > aligned allocations to their size?
> > > > > >
> > > > > > The PciIo->AllocateBuffer function doesn't support passing the
> > > > > > request in so
> > > > > we would need to know that info beforehand. The current calling
> > > > > in the XHCI driver does a free at the beginning and then the end
> > > > > of the buffer so we could the existing allocation tracker but
> > > > > figured it would be better to correct the function just in case
> > > > > someone called it to free
> > > in the middle.
> > > > > >
> > > > >
> > > > > What I was wondering is whether such allocations are themselves
> > > > > multiples of 64k. This is perhaps orthogonal to the issue this
> > > > > patch addresses, as we'' still need to deal with partial free
> > > > > calls regardless. But I was curious whether XHCI in particular,
> > > > > and perhaps more generally, we could streamline this by aligning
> > > > > all allocations
> > > to a log2 upper bound of their sizes.
> > > >
> > > > Xhci code
> > >
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/
> > > Xhci
> > > Dxe/UsbHcMem.c#L604) in allocation requested is greater the
> > > EFI_PAGE_SIZE allocates number of requested pages plus pages for the
> > > alignment and then frees pages at the beginning and end of the
> > > allocation.  I am not sure we really could change this without adding an
> alignment field to the PciIo protocol.
> > > >
> > > > Is there anything else you would like to change on this patch?
> > > >
> > >
> > > No. Thanks for the clarification.
> > >
> > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> >
> >
> > 
> >
> >

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

[gaoliming]

Jeff: 
  I have no comments for this change. Acked-by: Liming Gao <gaoliming@byosoft.com.cn>

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Jeff Brasen
> via groups.io
> 发送时间: 2022年9月30日 0:20
> 收件人: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> 抄送: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; jian.j.wang@intel.com; gaoliming
> <gaoliming@byosoft.com.cn>
> 主题: Re: [edk2-devel] [PATCH v2]
> MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> 
> MdeModulePkg maintainers, Any comments on this?
> 
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Wednesday, September 21, 2022 10:32 AM
> > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com;
> > ardb+tianocore@kernel.org
> > Subject: Re: [edk2-devel] [PATCH v2]
> > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> >
> > External email: Use caution opening links or attachments
> >
> >
> > On Wed, 21 Sept 2022 at 18:27, Jeff Brasen via groups.io
> > <jbrasen=nvidia.com@groups.io> wrote:
> > >
> > > Anything else needed to get this merged?
> > >
> >
> > That is up to the MdeModulePkg maintainers.
> >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > Sent: Thursday, September 8, 2022 9:55 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: [edk2-devel] [PATCH v2]
> > > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Sent: Monday, August 15, 2022 8:42 AM
> > > > > > To: devel@edk2.groups.io; Jeff Brasen <jbrasen@nvidia.com>
> > > > > > Cc: hao.a.wu@intel.com; ray.ni@intel.com;
> > > > > > quic_llindhol@quicinc.com;
> > > > > > ardb+tianocore@kernel.org
> > > > > > Subject: Re: [edk2-devel] [PATCH v2]
> > > > > > MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial
> > > > > > FreeBuffer
> > > > > >
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > On Fri, 5 Aug 2022 at 18:56, Jeff Brasen via groups.io
> > > > > > <jbrasen=nvidia.com@groups.io> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@nvidia.com>
> > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----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.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Ah interesting. Would it help if the allocate routine
> > > > > > > > aligned allocations to their size?
> > > > > > >
> > > > > > > The PciIo->AllocateBuffer function doesn't support passing the
> > > > > > > request in so
> > > > > > we would need to know that info beforehand. The current calling
> > > > > > in the XHCI driver does a free at the beginning and then the end
> > > > > > of the buffer so we could the existing allocation tracker but
> > > > > > figured it would be better to correct the function just in case
> > > > > > someone called it to free
> > > > in the middle.
> > > > > > >
> > > > > >
> > > > > > What I was wondering is whether such allocations are themselves
> > > > > > multiples of 64k. This is perhaps orthogonal to the issue this
> > > > > > patch addresses, as we'' still need to deal with partial free
> > > > > > calls regardless. But I was curious whether XHCI in particular,
> > > > > > and perhaps more generally, we could streamline this by aligning
> > > > > > all allocations
> > > > to a log2 upper bound of their sizes.
> > > > >
> > > > > Xhci code
> > > >
> > (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/
> > > > Xhci
> > > > Dxe/UsbHcMem.c#L604) in allocation requested is greater the
> > > > EFI_PAGE_SIZE allocates number of requested pages plus pages for the
> > > > alignment and then frees pages at the beginning and end of the
> > > > allocation.  I am not sure we really could change this without adding an
> > alignment field to the PciIo protocol.
> > > > >
> > > > > Is there anything else you would like to change on this patch?
> > > > >
> > > >
> > > > No. Thanks for the clarification.
> > > >
> > > > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > >
> > >
> > >
> > >
> 
> 
> 
>