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


Jeff Brasen
 

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Monday, August 15, 2022 8:42 AM
To: devel@edk2.groups.io; Jeff Brasen <jbrasen@...>
Cc: hao.a.wu@...; ray.ni@...; quic_llindhol@...;
ardb+tianocore@...
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@...> wrote:



-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Tuesday, August 2, 2022 10:51 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 Tue, 2 Aug 2022 at 17:32, Jeff Brasen <jbrasen@...> wrote:



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

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