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


Jeff Brasen
 

MdeModulePkg maintainers, Any comments on this?

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Wednesday, September 21, 2022 10:32 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 Wed, 21 Sept 2022 at 18:27, Jeff Brasen via groups.io
<jbrasen@...> wrote:

Anything else needed to get this merged?
That is up to the MdeModulePkg maintainers.

-----Original Message-----
From: Ard Biesheuvel <ardb@...>
Sent: Thursday, September 8, 2022 9:55 AM
To: Jeff Brasen <jbrasen@...>
Cc: devel@edk2.groups.io; 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 Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbrasen@...> wrote:



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



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