From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 16 Sep 2019 10:36:05 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E0579308620E; Mon, 16 Sep 2019 17:36:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-96.rdu2.redhat.com [10.10.124.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id 54A605DA5B; Mon, 16 Sep 2019 17:36:03 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Ard Biesheuvel , Julien Grall , Jordan Justen , xen-devel@lists.xenproject.org References: <20190913145100.303433-1-anthony.perard@citrix.com> <20190913145100.303433-10-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <26405443-8a65-5d03-dd35-1000ac3fbf0a@redhat.com> Date: Mon, 16 Sep 2019 19:36:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190913145100.303433-10-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Mon, 16 Sep 2019 17:36:05 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/13/19 16:50, Anthony PERARD wrote: > This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoid > using the Memory Allocation Services. > > This comes with a new interface named RegisterExitCallback so that PV > drivers can disconnect from the backend before XenBusDxe is teared > down. > > Instead of using Disconnect() to tear down the XenBus driver and the > children drivers, we are going to ask every driver using > XENBUS_PROTOCOL to disconnect from the hardware via the callback set > with RegisterExitCallback, then reset the xenstore shared ring and > the grant table. I think this approach -- a lower-level bus driver calling out to dependent device drivers -- is quite unusual. How about the following instead: - introduce two XenBusIo protocol member functions, AddReference() and RemoveReference(). RemoveReference() should take a BOOLEAN called "HandOffToOs". The device drivers should call AddReference() just before exiting DriverBindingStart() with success, and RemoveReference(FALSE) in DriverBindingStop(). - these protocol member functions would increment / decrement a reference counter in the underlying XenBus abstraction. Additionally, RemoveReference() would store the HandOffToOs parameter to a bus-level BOOLEAN too (regardless of previous value stored there -- a TRUE->FALSE transition would never happen anyway; see below). - both XenBusDxe and the Xen device drivers should register EBS callbacks, per controller handle (in BindingStart()), and unregister them (in BindingStop()) - the ordering between EBS notification functions (queued at the same TPL) is unspecified. In the device driver notification functions, the last action should be a call to XenBusIo->RemoveReference(TRUE) -- after the device-specific "forget me" actions have been done. - if RemoveReference() gets a TRUE parameter, then it should check the resultant (post-decrement) value of the refcount. If it has gone to zero, RemoveReference() should re-set the xenbus / xenstore connection. If the parameter is FALSE, it shouldn't do anything particular after decrementing the refcount. - in the XenBus EBS handler, if the refcount is positive at the time of the call, nothing should be done. Otherwise, if HandOffToOs is TRUE, nothing should be done, similarly. Otherwise, the xenbus/xenstore connection should be re-set. The idea is that normal Start/Stop should manage the refcount as expected. At ExitBootServices(), the XenBus level handler should only clear the connection to the hypervisor if no RemoveReference() call has done, or will do, it. (If the counter is positive, then a later RemoveReference() call will do it; if it's zero but HandOffToOs is TRUE, then it's been done already. If the counter is zero and the BOOLEAN is FALSE, then all devices have been disconnected normally with Stop() -- or none have been connected at all --, before ExitBootServices(), so the XenBus driver itself has to ask for being forgotten.) Admittedly, this is more complicated (due to the unspecified ordering between EBS notifications). I just feel it's more idiomatic to go through normal protocol member functions in EBS notification functions, rather than special callbacks. (Side comment: the reference counting could normally be replaced by gBS->OpenProtocolInformation(); however, that service itself allocates memory, so we can't use it in EBS notification functions.) Thanks Laszlo