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


Jeff Brasen
 

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Friday, July 29, 2022 9:48 AM
To: Jeff Brasen <jbrasen@...>
Cc: devel@edk2.groups.io; hao.a.wu@...; ray.ni@...;
quic_llindhol@...; ardb+tianocore@...
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@...> 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@...; ray.ni@...
Subject: RE: [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe:
Allow partial FreeBuffer

Any thoughts on this patch?

-----Original Message-----
From: Jeff Brasen <jbrasen@...>
Sent: Monday, February 14, 2022 11:46 AM
To: devel@edk2.groups.io
Cc: hao.a.wu@...; ray.ni@...; Jeff Brasen
<jbrasen@...>
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@...>
---
.../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

Join devel@edk2.groups.io to automatically receive all group messages.