On Thu, 8 Sept 2022 at 17:39, Jeff Brasen <jbra...@nvidia.com> wrote: > > > > > -----Original Message----- > > From: Ard Biesheuvel <a...@kernel.org> > > Sent: Monday, August 15, 2022 8:42 AM > > To: devel@edk2.groups.io; Jeff Brasen <jbra...@nvidia.com> > > Cc: hao.a...@intel.com; ray...@intel.com; quic_llind...@quicinc.com; > > ardb+tianoc...@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....@groups.io> wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Ard Biesheuvel <a...@kernel.org> > > > > Sent: Tuesday, August 2, 2022 10:51 AM > > > > To: Jeff Brasen <jbra...@nvidia.com> > > > > Cc: devel@edk2.groups.io; hao.a...@intel.com; ray...@intel.com; > > > > quic_llind...@quicinc.com; ardb+tianoc...@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 <jbra...@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Ard Biesheuvel <a...@kernel.org> > > > > > > Sent: Friday, July 29, 2022 9:48 AM > > > > > > To: Jeff Brasen <jbra...@nvidia.com> > > > > > > Cc: devel@edk2.groups.io; hao.a...@intel.com; ray...@intel.com; > > > > > > quic_llind...@quicinc.com; ardb+tianoc...@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 <jbra...@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 <a...@kernel.org> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#93529): https://edk2.groups.io/g/devel/message/93529 Mute This Topic: https://groups.io/mt/89143704/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
