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.web12.960.1662652491797710384 for ; Thu, 08 Sep 2022 08:54:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=b1ocPO+o; 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 A9769B8218D for ; Thu, 8 Sep 2022 15:54:49 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5F4B5C433C1 for ; Thu, 8 Sep 2022 15:54:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662652488; bh=yUCIxxDr/ebd0FhXypSPS1GjLAEdk6f0EIOZGURB+EE=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=b1ocPO+o0n/O1IGT5Qtw8i9QvfTcYX3PfjHWY4Uhm86rj5pGH17r0CqNUvA83KuCv HTn5shXnwfLXdeYcNP/hv8102JZdMu1qyxcZP3RxLj6u8KVKFX0Th9Fr4M3idyskhK am3oJ4t+VUN9syjbTyNxXN2FIblIckZ1vWCHXvnW6s3PEv6Ri2ub9bgcl790vVBOGu aWOvS2mUEBe0ldLWhzCXm1Jov7eErsROfuigiMig4Lmoh6w0qHtSdlUQaLPSisiQQ9 FHrn5NwYJT0J06bpKe+IppqVge56f6xCtFTX2MsMWdidJ/TkmdrpRXPkMWzEgr8eVB vHA+BNJZwruPw== Received: by mail-lj1-f177.google.com with SMTP id bx38so20400085ljb.10 for ; Thu, 08 Sep 2022 08:54:48 -0700 (PDT) X-Gm-Message-State: ACgBeo0LnXrw3bEYGeQMW32IVwZ/iGKiO7sYjLCAz9hrC4hB8V4UljLr ARSrHtEnBK2zZgm6GEkIIs4jNXpA7MWUqfNAQ88= X-Google-Smtp-Source: AA6agR57jST7Jx+N4bnhiW3GeGegCOC4X2WeWs/Gig632wRftpfSLS+tYqMONDQ9GA4NchFng7c0N3ljSZ8UUjUQG5w= X-Received: by 2002:a2e:9115:0:b0:26a:c086:5138 with SMTP id m21-20020a2e9115000000b0026ac0865138mr2496093ljg.189.1662652486451; Thu, 08 Sep 2022 08:54:46 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 8 Sep 2022 17:54:35 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [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" Content-Transfer-Encoding: quoted-printable On Thu, 8 Sept 2022 at 17:39, Jeff Brasen wrote: > > > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Monday, August 15, 2022 8:42 AM > > To: devel@edk2.groups.io; Jeff Brasen > > Cc: hao.a.wu@intel.com; ray.ni@intel.com; quic_llindhol@quicinc.com; > > ardb+tianocore@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 > > wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Ard Biesheuvel > > > > Sent: Tuesday, August 2, 2022 10:51 AM > > > > 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 > > > > 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 wrote= : > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Ard Biesheuvel > > > > > > Sent: Friday, July 29, 2022 9:48 AM > > > > > > 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 > > > > > > Subject: Re: [PATCH v2] MdeModulePkg/NonDiscoverablePciDeviceDx= e: > > > > > > Allow partial FreeBuffer > > > > > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > > > > On Thu, 28 Jul 2022 at 13:25, Jeff Brasen = wrote: > > > > > > > > > > > > > > > > > > > > > Adding Leif/Ard to CC incase they have any comments on this p= atch. > > > > > > > > > > > > > > > > > > > This generally looks ok to me. I just wonder if it wouldn't be > > > > > > simpler to reuse the existing allocation descriptor if it is no= t > > > > > > 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 ar= e > > > > > > 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 reques= t 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 w= e could > > the existing allocation tracker but figured it would be better to corre= ct the > > function just in case someone called it to free in the middle. > > > > > > > What I was wondering is whether such allocations are themselves multipl= es of > > 64k. This is perhaps orthogonal to the issue this patch addresses, as w= e'' still > > need to deal with partial free calls regardless. But I was curious whet= her XHCI in > > particular, and perhaps more generally, we could streamline this by ali= gning 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_PA= GE_SIZE allocates number of requested pages plus pages for the alignment an= d then frees pages at the beginning and end of the allocation. I am not su= re we really could change this without adding an alignment field to the Pci= Io protocol. > > Is there anything else you would like to change on this patch? > No. Thanks for the clarification. Reviewed-by: Ard Biesheuvel