From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=IAEK2hu9; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by groups.io with SMTP; Mon, 16 Sep 2019 13:50:19 -0700 Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x8GKlHiU060937; Mon, 16 Sep 2019 13:50:13 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=72wVYMIybrHwiM17lWUy6sjI8QWq6a3xG3JD38mrUWU=; b=IAEK2hu9iUQnPoZg8Pw+ecWZ/l2v9nujTusmjQfUO5YZln/qwySjILOB8VxhMo0NJsD5 US2g9Rrz/b8mXOVwgIfR3s1Ud2Lqww7IcwyjsS0rNN7llG3NnR0l7peJw37CqClKIfJJ 9u5tyYR3Sf/VGANueCmRyfqWmyijsHglb/e2l8VoP7DkBmJQENLRbCIfDGH1kyyfZpG7 HHVZ1h3sN6gO3E53Mingesu/8sHa9dpAKqeZL8jdTU6joEsnC+l6wF9M9OHfDFal+p9z TCujcgnvnaxafCXdK9wijFcafO9/Y2ye3IAYL7TFRRk8WMYwEYl59vaDilobVrNtsliP Ow== Received: from mr2-mtap-s03.rno.apple.com (mr2-mtap-s03.rno.apple.com [17.179.226.135]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2v1gejgypm-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Mon, 16 Sep 2019 13:50:13 -0700 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by mr2-mtap-s03.rno.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PXX00CL7Z7OS1A0@mr2-mtap-s03.rno.apple.com>; Mon, 16 Sep 2019 13:50:12 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz12.apple.com by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PXX00G00YUVP500@nwk-mmpp-sz12.apple.com>; Mon, 16 Sep 2019 13:50:12 -0700 (PDT) X-Va-A: X-Va-T-CD: 07a9f7dd315dc6000695a0402a47d12d X-Va-E-CD: a5a5e046124b1576edd6ac48f9946ad5 X-Va-R-CD: 5689ef722b0dd46b2e850591228443f8 X-Va-CD: 0 X-Va-ID: 1f6ba6ce-189e-497b-8016-14208eed63ec X-V-A: X-V-T-CD: 07a9f7dd315dc6000695a0402a47d12d X-V-E-CD: a5a5e046124b1576edd6ac48f9946ad5 X-V-R-CD: 5689ef722b0dd46b2e850591228443f8 X-V-CD: 0 X-V-ID: 6ae4ebae-ee41-4366-84e4-bf2089b875a8 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-09-16_08:,, signatures=0 Received: from [17.235.22.77] (unknown [17.235.22.77]) by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PXX00LKNZ7MLOC0@nwk-mmpp-sz12.apple.com>; Mon, 16 Sep 2019 13:50:11 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services Date: Mon, 16 Sep 2019 13:50:04 -0700 In-reply-to: <5e73e526-0f76-5f91-aa7c-a4adaeee4608@redhat.com> Cc: devel@edk2.groups.io, Anthony Perard , Ard Biesheuvel , Julien Grall , Jordan Justen , xen-devel@lists.xenproject.org To: Laszlo Ersek References: <20190913145100.303433-1-anthony.perard@citrix.com> <20190913145100.303433-10-anthony.perard@citrix.com> <26405443-8a65-5d03-dd35-1000ac3fbf0a@redhat.com> <5e73e526-0f76-5f91-aa7c-a4adaeee4608@redhat.com> X-Mailer: Apple Mail (2.3445.104.11) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-09-16_08:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_7D7E9BD1-1AFE-4709-86D5-D49A2550EB02" --Apple-Mail=_7D7E9BD1-1AFE-4709-86D5-D49A2550EB02 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On Sep 16, 2019, at 12:31 PM, Laszlo Ersek wrote: >=20 > On 09/16/19 20:36, Andrew Fish wrote: >>=20 >>=20 >>> On Sep 16, 2019, at 10:36 AM, Laszlo Ersek wrote: >>>=20 >>> 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. >>>>=20 >>>> This comes with a new interface named RegisterExitCallback so that PV >>>> drivers can disconnect from the backend before XenBusDxe is teared >>>> down. >>>>=20 >>>> 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. >>>=20 >>> I think this approach -- a lower-level bus driver calling out to >>> dependent device drivers -- is quite unusual. >>>=20 >>=20 >> Laszlo, >>=20 >> I agree given the timer event activity is stopped prior to calling any = of the EXIT_BOOT_SERVICES events there is usually not a requirement to call= the drivers Stop() function. Generally Exit Boot Services events just turn= off DMA, or any other hardware that could touch memory that is being freed= back for OS usage. Since the timer activity, and thus all event activity i= s stopped there is not a lot of ways for the drivers to ever have any EFI c= ode run again.=20 >>=20 >> The only other exception I can think of is if the OS driver makes some = kind of assumption about the state of the hardware. >=20 > The >=20 > hardware that could touch memory that is being freed back for OS > usage >=20 > is the part that applies here. Most paravirtual devices work by sharing > at least some memory between the host-side device model (emulation) and > guest-side device driver. When the guest is transitioning from firmware > to OS (and those of course have different guest drivers for the > paravirtual device), the host must be made forget the memory shared with > the guest firmware driver (as the guest OS boot loader or the guest OS > itself might load anything at all into that RAM area after > ExitBootServices()). >=20 > So what I found unusual in this patch wasn't the necessity of EBS > notification functions, but how they would be ordered / coordinated > between each other. There is a bus-like device that shares its own > resources (RAM) with the host, and installs protocol interfaces. And > there are dependent endpoint-like devices that consume those protocol > interfaces, and share their own stuff (RAM) with the host OS. >=20 > All of that shared memory needs to be cleared from the host's > book-keeping when we leave firmware land; the tricky part is that the > bus-like device can't request the host for its bus-level shared memory > to be forgotten before all of the dependent endpoints ask for their > shared ranges to be forgotten. >=20 Laszlo, In "real hardware" the bus driver can usually clear the memory usage by by= turning off the DMA. I guess this would look like sending a reset to the v= irtual device. The buffers allocated by children of the "hardware" bus driv= er free all their buffers back to the OS as a side effect of the ExitBootSe= rvices, so it is typical for the children of the bus driver to not have an = ExitBootServices event.=20 I find it strange there is not a reset operation for the virtual bus devic= e, but it seems like worst case the bus driver could could track all the s= hared memory allocations and free the child allocations 1st?=20 In general the ExitBootServices routines for hardware can be very simple s= ince EFI is a cooperative event model and the events are shutdown as part o= f the ExitBootService processes. Most of the times I've seen complex code i= n ExitBootServices routines it was due to people thinking in terms of threa= ding vs. events, and forgetting that just returning from ExitBootServices f= rees most of the the resources allocated by the driver.=20 I'd also point out the order of the ExitBootServices events are not archit= ectural, but a given implementation may do the same thing every time. For e= xample the ExitBootServices events are probably just an insertion to a doub= ly linked list based on execution order of the drivers. But if a lot of the= drivers are Driver Model drivers then they all have the same Depex so the = order could depend on the order of those drivers in the FV. Some times the = root bridge driver is a DXE driver for something like PCI, but a USB Host B= ridge driver would be a Driver Model driver that Starts on a PCI IO Handle.= Thanks, Andrew Fish > Thanks! > Laszlo >=20 >=20 >>> How about the following instead: >>>=20 >>> - introduce two XenBusIo protocol member functions, AddReference() and >>> RemoveReference(). RemoveReference() should take a BOOLEAN called >>> "HandOffToOs". The device drivers should call AddReference() just befo= re >>> exiting DriverBindingStart() with success, and RemoveReference(FALSE) = in >>> DriverBindingStop(). >>>=20 >>> - 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->FALS= E >>> transition would never happen anyway; see below). >>>=20 >>> - both XenBusDxe and the Xen device drivers should register EBS >>> callbacks, per controller handle (in BindingStart()), and unregister >>> them (in BindingStop()) >>>=20 >>> - 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) -- aft= er >>> the device-specific "forget me" actions have been done. >>>=20 >>> - 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. >>>=20 >>> - in the XenBus EBS handler, if the refcount is positive at the time o= f >>> 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. >>>=20 >>> 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 ha= s >>> 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 TRU= E, >>> 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 t= he >>> XenBus driver itself has to ask for being forgotten.) >>>=20 >>> 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. >>>=20 >>> (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.) >>>=20 >>> Thanks >>> Laszlo >>>=20 >>>=20 --Apple-Mail=_7D7E9BD1-1AFE-4709-86D5-D49A2550EB02 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
On Sep 16= , 2019, at 12:31 PM, Laszlo Ersek <lersek@redhat.com> wrote:

On 09/16/19 20:36, Andrew Fish wrote:<= /span>


On Sep 16, 2019,= at 10:36 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 09/= 13/19 16:50, Anthony PERARD wrote:
This patch fix the EVT_SIGNAL_EXIT_BOOT_SERVICES handler to avoi= d
using the Memory Allocation Services.

This comes with a new interface named RegisterExitCallback so that = PV
drivers can disconnect from the backend before XenBusDxe i= s 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_PROTOC= OL to disconnect from the hardware via the callback set
with = RegisterExitCallback, then reset the xenstore shared ring and
the grant table.

I think this ap= proach -- a lower-level bus driver calling out to
dependent d= evice drivers -- is quite unusual.


Laszlo,

I agree given the tim= er event activity is stopped prior to calling any of the EXIT_BOOT_SERVICES= events there is usually not a requirement to call the drivers Stop() funct= ion. Generally Exit Boot Services events just turn off DMA, or any other ha= rdware that could touch memory that is being freed back for OS usage. Since= the timer activity, and thus all event activity is stopped there is not a = lot of ways for the drivers to ever have any EFI code run again. 

The = only other exception I can think of is if the OS driver makes some kind of = assumption about the state of the hardware.

The

  &= nbsp;hardware that could touch memory that is being freed back for OS
   usage

is the part that applies here. Most paravirtual devices work = by sharing
at least som= e memory between the host-side device model (emulation) and
guest-side device driver. When the gu= est is transitioning from firmware
to OS (and those of course have different guest drivers for t= he
paravirtual device),= the host must be made forget the memory shared with
the guest firmware driver (as the guest OS bo= ot loader or the guest OS
itself might load anything at all into that RAM area after
ExitBootServices()).

So what I found unusual in this patch wasn't the necessity of EBS<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">notification functions, but ho= w they would be ordered / coordinated
between each other. There is a bus-like device that shares i= ts own
resources (RAM) = with the host, and installs protocol interfaces. And
there are dependent endpoint-like devices tha= t consume those protocol
interfaces, and share their own stuff (RAM) with the host OS.

All of that shared memory needs to be cleared from the host's=
book-keeping when we leave fi= rmware land; the tricky part is that the
bus-like device can't request the host for its bus-level = shared memory
to be for= gotten before all of the dependent endpoints ask for their
shared ranges to be forgotten.=


Laszlo,

In "real hardware" the bus driver can usually clear t= he memory usage by by turning off the DMA. I guess this would look like sen= ding a reset to the virtual device. The buffers allocated by children of th= e "hardware" bus driver free all their buffers back to the OS as a side eff= ect of the ExitBootServices, so it is typical for the children of the bus d= river to not have an ExitBootServices event. 

I find it strange there is not a reset operation for the virtua= l bus device, but it seems like worst case the bus driver could  could= track all the shared memory allocations and free the child allocations 1st= ? 

In general the ExitBootServices= routines for hardware can be very simple since EFI is a cooperative event = model and the events are shutdown as part of the ExitBootService processes.= Most of the times I've seen complex code in ExitBootServices routines it w= as due to people thinking in terms of threading vs. events, and forgetting = that just returning from ExitBootServices frees most of the the resources a= llocated by the driver. 

I'd also = point out the order of the ExitBootServices events are not architectural, b= ut a given implementation may do the same thing every time. For example the= ExitBootServices events are probably just an insertion to a doubly linked = list based on execution order of the drivers. But if a lot of the drivers a= re Driver Model drivers then they all have the same Depex so the order coul= d depend on the order of those drivers in the FV. Some times the root bridg= e driver is a DXE driver for something like PCI, but a USB Host Bridge driv= er would be a Driver Model driver that Starts on a PCI IO Handle.  

Thanks,

Andrew Fish

Thanks!
Laszlo


How about the following instead= :

- introduce two XenBusIo protocol member fun= ctions, AddReference() and
RemoveReference(). RemoveReference= () should take a BOOLEAN called
"HandOffToOs". The device dri= vers should call AddReference() just before
exiting DriverBin= dingStart() with success, and RemoveReference(FALSE) in
Drive= rBindingStop().

- these protocol member functi= ons would increment / decrement a
reference counter in the un= derlying 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<= br class=3D"">callbacks, per controller handle (in BindingStart()), and unr= egister
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->RemoveRefere= nce(TRUE) -- after
the device-specific "forget me" actions ha= ve been done.

- if RemoveReference() gets a TR= UE parameter, then it should check the
resultant (post-decrem= ent) value of the refcount. If it has gone to
zero, RemoveRef= erence() should re-set the xenbus / xenstore connection.
If t= he parameter is FALSE, it shouldn't do anything particular after
decrementing the refcount.

- in the Xen= Bus EBS handler, if the refcount is positive at the time of
t= he call, nothing should be done. Otherwise, if HandOffToOs is TRUE,
nothing should be done, similarly. Otherwise, the xenbus/xenstoreconnection should be re-set.

The i= dea is that normal Start/Stop should manage the refcount as
e= xpected. 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 H= andOffToOs is TRUE,
then it's been done already. If the count= er is zero and the BOOLEAN is
FALSE, then all devices have be= en disconnected normally with Stop() --
or none have been con= nected at all --, before ExitBootServices(), so the
XenBus dr= iver itself has to ask for being forgotten.)

A= dmittedly, this is more complicated (due to the unspecified ordering
between EBS notifications). I just feel it's more idiomatic to gothrough normal protocol member functions in EBS notification f= unctions,
rather than special callbacks.

(Side comment: the reference counting could normally be replaced b= y
gBS->OpenProtocolInformation(); however, that service it= self allocates
memory, so we can't use it in EBS notification= functions.)

Thanks
Laszlo

=
--Apple-Mail=_7D7E9BD1-1AFE-4709-86D5-D49A2550EB02--