From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web09.251.1659109686906515019 for ; Fri, 29 Jul 2022 08:48:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=b3PQbe+D; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 38AE8B82840 for ; Fri, 29 Jul 2022 15:48:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E1F95C433B5 for ; Fri, 29 Jul 2022 15:48:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1659109682; bh=BJOJVwmTK3MGsPsow3CO00b8cZfNbnwEa5l1ggDxkRE=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=b3PQbe+D+47SlhNtRnnQ7G9pPirl5JWMX/PBTEPKbtU7h05q8bcD9iq1s0knw/1lv CUGoOMKYDjJjNrc0Rj9w51PlrLJTdp3x9+35/n6deyV9Oq8J1Q/0KmeT9Qexcg7lQt Cfg9mkx+ByUgYFJBMzc7yeUtNdkswVjlkFP54qWSt5hCbdw7xclttU2X8+pq4V516/ yX+o9nuHxkPXZAVc5U5V7VMeDIofXCRvA9Fjyn1X5PYU6hQkL/SMLZdtnVzoKervYS Mnsx/JMHwLKzwNQmh85asPT0g3hW6rgEgRNJG+E4XCFxxBx1B/gD+uZRJFp+xtUMEf w16Z5PNwNCTIQ== Received: by mail-oa1-f52.google.com with SMTP id 586e51a60fabf-10e615a36b0so4812680fac.1 for ; Fri, 29 Jul 2022 08:48:02 -0700 (PDT) X-Gm-Message-State: AJIora/MUJUR93QoIGbwaThEBpICKe7Qd54eztmP48trq8Koucx7RxYi EO7rbWUNu4HyFZm3zoqVDayhID+r39qtoOLyzJM= X-Google-Smtp-Source: AGRyM1sNOaaJ7wklyHTwKl2KtU7yemHrOn59EQybXYU5FZ6cV5y1vbEeCenQABxhJobwF/BSRteNAnIPzvK7NSaXdSM= X-Received: by 2002:a05:6870:b403:b0:10e:7914:adb with SMTP id x3-20020a056870b40300b0010e79140adbmr2049246oap.126.1659109682067; Fri, 29 Jul 2022 08:48:02 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 29 Jul 2022 08:47:51 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDxe: Allow partial FreeBuffer To: Jeff Brasen Cc: "devel@edk2.groups.io" , "hao.a.wu@intel.com" , "ray.ni@intel.com" , "quic_llindhol@quicinc.com" , "ardb+tianocore@kernel.org" Content-Type: text/plain; charset="UTF-8" On Thu, 28 Jul 2022 at 13:25, Jeff Brasen 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 > > > 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 > > > > > > 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 > > > --- > > > .../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 >