public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: Andrew Fish <afish@apple.com>, devel@edk2.groups.io
Cc: Anthony Perard <anthony.perard@citrix.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Julien Grall <julien.grall@arm.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [edk2-devel] [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services
Date: Mon, 16 Sep 2019 21:31:06 +0200	[thread overview]
Message-ID: <5e73e526-0f76-5f91-aa7c-a4adaeee4608@redhat.com> (raw)
In-Reply-To: <FDC048FC-B33F-49A4-9FC3-DEF91AAC6334@apple.com>

On 09/16/19 20:36, Andrew Fish wrote:
> 
> 
>> 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 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.
>>
> 
> Laszlo,
> 
> 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 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

    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 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()).

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.

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.

Thanks!
Laszlo


>> 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
>>
>> 
>>
> 


  reply	other threads:[~2019-09-16 19:31 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-13 14:50 [PATCH 00/11] OvmfPkg/XenBusDxe: Fix ExitBootServices handler to avoid allocation Anthony PERARD
2019-09-13 14:50 ` [PATCH 01/11] OvmfPkg/XenBusDxe: Fix missing \n in DEBUG messages Anthony PERARD
2019-09-16 14:24   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 02/11] OvmfPkg/XenBusDxe: Have XenStoreFindWatch take a pointer Anthony PERARD
2019-09-16 14:38   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 03/11] OvmfPkg/XenBusDxe: Rework watch events reception Anthony PERARD
2019-09-16 14:39   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 04/11] OvmfPkg/XenBusDxe: Avoid Allocate in XenStoreVSPrint Anthony PERARD
2019-09-16 14:45   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 05/11] OvmfPkg/XenBusDxe: Construct paths without allocation Anthony PERARD
2019-09-16 15:39   ` [edk2-devel] " Laszlo Ersek
2019-09-16 15:43     ` Laszlo Ersek
2019-09-13 14:50 ` [PATCH 06/11] OvmfPkg/XenBusDxe: Rework XenStoreProcessMessage to avoid allocating memory Anthony PERARD
2019-09-16 15:41   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 07/11] OvmfPkg/XenBusDxe: Use on stack buffer in internal functions Anthony PERARD
2019-09-16 16:11   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory Anthony PERARD
2019-09-16 16:16   ` [edk2-devel] " Laszlo Ersek
2019-09-13 14:50 ` [PATCH 09/11] OvmfPkg/XenBusDxe: Fix NotifyExitBoot to avoid Memory Allocation Services Anthony PERARD
2019-09-16 17:36   ` [edk2-devel] " Laszlo Ersek
2019-09-16 18:36     ` Andrew Fish
2019-09-16 19:31       ` Laszlo Ersek [this message]
2019-09-16 20:50         ` Andrew Fish
2019-09-13 14:50 ` [PATCH 10/11] OvmfPkg/XenPvBlkDxe: Use XenBusIo->RegisterExitCallback Anthony PERARD
2019-09-13 14:51 ` [PATCH 11/11] OvmfPkg/XenBusDxe: Fix XenStoreWaitForEvent use during EBS Anthony PERARD

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5e73e526-0f76-5f91-aa7c-a4adaeee4608@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox