From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: citrix.com, ip: 216.71.155.144, mailfrom: anthony.perard@citrix.com) Received: from esa4.hc3370-68.iphmx.com (esa4.hc3370-68.iphmx.com [216.71.155.144]) by groups.io with SMTP; Fri, 26 Jul 2019 09:06:51 -0700 Authentication-Results: esa4.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=anthony.perard@citrix.com; spf=Pass smtp.mailfrom=anthony.perard@citrix.com; spf=None smtp.helo=postmaster@mail.citrix.com Received-SPF: None (esa4.hc3370-68.iphmx.com: no sender authenticity information available from domain of anthony.perard@citrix.com) identity=pra; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible Received-SPF: Pass (esa4.hc3370-68.iphmx.com: domain of anthony.perard@citrix.com designates 162.221.158.21 as permitted sender) identity=mailfrom; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="anthony.perard@citrix.com"; x-conformance=sidf_compatible; x-record-type="v=spf1"; x-record-text="v=spf1 ip4:209.167.231.154 ip4:178.63.86.133 ip4:195.66.111.40/30 ip4:85.115.9.32/28 ip4:199.102.83.4 ip4:192.28.146.160 ip4:192.28.146.107 ip4:216.52.6.88 ip4:216.52.6.188 ip4:162.221.158.21 ip4:162.221.156.83 ~all" Received-SPF: None (esa4.hc3370-68.iphmx.com: no sender authenticity information available from domain of postmaster@mail.citrix.com) identity=helo; client-ip=162.221.158.21; receiver=esa4.hc3370-68.iphmx.com; envelope-from="anthony.perard@citrix.com"; x-sender="postmaster@mail.citrix.com"; x-conformance=sidf_compatible IronPort-SDR: E11NKDcj59WL82tqAkFtUu84+Jv7Ph/kX3w+ZkF9kUkdegCmiWAD6DFRxKxBtbZw8NF+gwIJYd gZN6daIBaBDz8Qj8G7EAClDlTG58+47cVgazhkSJa48C/+5gZ7sXfci3kFa6/MEZ6E8IjAIFUe 31HqBWkibtiBrS38Y9Oh8FZJ8bylbGtuILFAH6vVBgpWaq2oJbknxBdnUB2zt/q1Xzi4XHrtYf Dn7/x9Lb2xsrWNvltsPxMG+Rdly/5Naw0WiBUhUgefFBPBuWAT64Y8ZZN1xJ42twAQFk9gjHxM r8c= X-SBRS: 2.7 X-MesageID: 3620380 X-Ironport-Server: esa4.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.64,311,1559534400"; d="scan'208";a="3620380" Date: Fri, 26 Jul 2019 17:06:42 +0100 From: "Anthony PERARD" To: Laszlo Ersek CC: , , Ard Biesheuvel , Jordan Justen , Julien Grall Subject: Re: [PATCH v3 33/35] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables Message-ID: <20190726160642.GG1242@perard.uk.xensource.com> References: <20190704144233.27968-1-anthony.perard@citrix.com> <20190704144233.27968-34-anthony.perard@citrix.com> <4badd535-c23d-c64d-7bb3-fb42bbbf538a@redhat.com> MIME-Version: 1.0 In-Reply-To: <4badd535-c23d-c64d-7bb3-fb42bbbf538a@redhat.com> User-Agent: Mutt/1.12.1 (2019-06-15) Return-Path: anthony.perard@citrix.com Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline On Wed, Jul 10, 2019 at 04:05:02PM +0200, Laszlo Ersek wrote: > On 07/04/19 16:42, Anthony PERARD wrote: > > + > > +STATIC > > +VOID > > +EFIAPI > > +XenIoPvhDxeNotifyExitBoot ( > > + IN EFI_EVENT Event, > > + IN VOID *Context > > + ) > > +{ > > + XEN_IO_PVH_STATE *State; > > + EFI_STATUS Status; > > + > > + State = Context; > > + > > + gBS->CloseEvent(&State->ExitBootEvent); > > + Status = XenIoMmioUninstall(State->XenIoHandle); > > + if (Status == EFI_SUCCESS) { > > + // > > + // Only free the reserved space for grant table if no driver is using it. > > + // > > + FreePages (State->Allocation, XEN_GRANT_FRAMES); > > + } > > + FreePool (State); > > +} > > In my v2 review at > > http://mid.mail-archive.com/2c386393-b886-a7e7-e8b9-c5e339c94727@redhat.com > > I asked, in point (6), whether we needed the memory allocation to be > "reserved". I guess this change (in v3) is supposed to address that. > (And, indeed, version 3 of this patch correctly addresses all other > points from my v2 review.) > > However: an ExitBootServices notification function *must not* perform > actions that could change the UEFI memory map. Thus, you can not release > memory there -- you can't call CloseEvent(), > UninstallMultipleProtocolInterfaces(), FreePool(), FreePages(), and so on. > > In my earlier review, I wrote, > > > [...] then we could allocate EfiBootServicesData type memory here -- > > and perhaps add an ExitBootServices() callback to disconnect from Xen > > [...] > > By "disconnect", I meant actions that do *not* include memory allocation > / release, but only make the hypervisor *forget* its guest RAM > references that it currently holds. > > ExitBootServices() handlers (notification functions) are different from > normal "teardown" functions. A "teardown" function generally mirrors a > "construct" function, where you remove all references, and release all > memory allocations, in precise reverse order of construction. > > ExitBootServices() handlers perform a *subsequence* of that (usually > preserving the same relative order between the steps): you only erase > external references to RAM -- that is, RAM references that are held by > external entities, such as PCI devices (in-flight DMA), the hypervisor, > and so on. The referred-to RAM, originally allocated as "boot services > data", will be released later, whole-sale, by the OS. > > In that sense, ExitBootServices() callbacks implement a kind of > ownership transfer. You no longer own the UEFI memory map, you just have > to kick out such external references into it that the OS would not know > about. > > To summarize: > > - If this memory *needs* to be reserved past ExitBootServices(), then > stick with AllocateReservedPages(). Otherwise, use AllocatePages(); then > the pages will be freed automatically by the OS, soon after it is started. > > - In the latter case, if necessary, you can make the hypervisor forget > about the area, as part of ExitBootServices(), in a notification > function. But that can only happen without allocating or releasing > memory in the notification function. > > > ... I guess you could make a XENMEM_remove_from_physmap hypercall, for > example. But, it's *entirely* possible that said hypercall doesn't > belong in *this* driver. After all, this is not the driver that calls > XENMEM_add_to_physmap either! > > ... In fact, I think XenBusDxe has a bug. Right now, you have the > following call chain, when the OS calls ExitBootServices(): > > NotifyExitBoot() > gBS->DisconnectController() > XenBusDxeDriverBindingStop() > XenGrantTableDeinit() > XenHypercallMemoryOp (XENMEM_remove_from_physmap) > > Unfortunately, this call tree releases memory left and right, and so it > is not valid on the stack of gBS->ExitBootServices(). > > Instead, NotifyExitBoot() should collect just the subsequence of > XenBusDxeDriverBindingStop() -- interpreting that function as a flatened > sequence of operations -- that removes external references to RAM. > Making hypercalls (using parameters constructed on the stack) is fine. > Using dynamically *pre*allocated memory (for constructing hypercall > arguments) is also fine. Dynamically allocating or releasing memory *on > the spot* is not fine. > > (2) So, for now, I'd recommend simply removing > XenIoPvhDxeNotifyExitBoot(), in this patch. Ok, I'll remove it. > You might want to fix XenBusDxe's NotifyExitBoot(), separately. I'll put that on my TODO list. > > + return EFI_UNSUPPORTED; > > + } > > + > > + State = AllocatePool (sizeof (*State)); > > (4) What do we gain by allocating "State" dynamically? IMO, it could be > a local variable (= "automatic storage duration"). Without the exit boot notification, State isn't needed anymore, so I'll remove it. > > + if (State == NULL) { > > + Status = EFI_OUT_OF_RESOURCES; > > + goto Error; > > + } > > + > > + Allocation = AllocateReservedPages (XEN_GRANT_FRAMES); > > (5) So, again, please evaluate if this could simply be AllocatePages(). I would prefer to let know the operating system that those pages are potentially used, unless I'm sure OVMF has let know Xen that those pages don't contain the grant table anymore. I'll probably try to make some changes in OVMF to allow to give back those pages, but for now I think keeping those pages as reserved will be good enough. Thanks, -- Anthony PERARD