From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D6585208F7AD7 for ; Fri, 8 Sep 2017 04:22:51 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id j141so5058861ioj.4 for ; Fri, 08 Sep 2017 04:25:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=x9e/0ljPpy46wTvYk+QJltD0YL7JIGdR0QT2uU+Q2aQ=; b=OeSCgNC11/lPkLYDlZ3D5H/alumkPAAOQf870Y66Ftx7tR4nhTmADDtThBVA+SboYz oFtg5OxqvUNJXMnVcCxDvyccgDU3d8TZiM9qda9sgpjfYBcBQrNjgot36sCPF+yhVQ+X 1I+Vgznf7hcUilZOLOBp20Qo/GdiE86TFDuG0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=x9e/0ljPpy46wTvYk+QJltD0YL7JIGdR0QT2uU+Q2aQ=; b=JwizZjfMgpjBOYanWPM8h7Sl64h/Dub1Uiq1h/qrYue8S0xq3M01E8Ro+Iu0T1l8vr lVz6XT5ptnYgxi0cmmq/1oFK08f33tmqdGuhamwVBC8n8EPO8xOWXMkkPOADpXO7LpWg bA3e52jpc1Uic4aDdXx/odGd7XQODeMsLvlFz6OtHIN/yetdO9od6xfh+8C+NXU1RUAy SvsghBGnR+MyP3vxM9eMKuJy2iFsxJsi1l5jZGHZKNjIAMK0uI7wlw1yJpazqOCTlWKM dGuLWNGhH85mW9MfoKwdcnT8Z+4tecknFkIhFF5BxC7ixokx/SRpmZ+qoqMijWZBYVGg K7Pg== X-Gm-Message-State: AHPjjUgJWAaFlTQQ8wVj/I2d6QAUp83wiHRoeXnmcanRrqkFGr1ij4Fu ITyIS3mXJgeXlWh7vJDU2ahjYfv7UJiY X-Google-Smtp-Source: AOwi7QBUm8mO8kFjr0V2SKXJIdMOcVE7vkOz2rhQy1xyLoOupudlO5vxa7Uz2SZK/T9gsdpSr9pcJvEywQb1ayeYmj0= X-Received: by 10.107.132.226 with SMTP id o95mr2658779ioi.79.1504869943370; Fri, 08 Sep 2017 04:25:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 8 Sep 2017 04:25:42 -0700 (PDT) In-Reply-To: <49a0e877-0500-b1f5-e4ba-753df311f868@redhat.com> References: <20170907224116.895-1-lersek@redhat.com> <49a0e877-0500-b1f5-e4ba-753df311f868@redhat.com> From: Ard Biesheuvel Date: Fri, 8 Sep 2017 12:25:42 +0100 Message-ID: To: Laszlo Ersek Cc: Leif Lindholm , "afish@apple.com" , "Kinney, Michael D" , edk2-devel-01 , Brijesh Singh , Eric Dong , Jiewen Yao , Jordan Justen , Star Zeng Subject: Re: [PATCH 00/10] MdeModulePkg, OvmfPkg: unmap DMA buffers at ExitBootServices X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Sep 2017 11:22:52 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 8 September 2017 at 10:48, Laszlo Ersek wrote: > On 09/08/17 10:53, Ard Biesheuvel wrote: >> (cc'ing the trinity) >> >> On 7 September 2017 at 23:41, Laszlo Ersek wrote: >>> Repo: https://github.com/lersek/edk2.git >>> Branch: iommu_exit_boot >>> >>> This series is the result of the discussion under >>> >>> [edk2] [PATCH 0/4] MdeModulePkg: some PCI HC drivers: unmap common >>> buffers at ExitBootServices() >>> https://lists.01.org/pipermail/edk2-devel/2017-September/014099.html >>> >>> At ExitBootServices(), PCI and VirtIo drivers should only care about >>> aborting pending DMA on the devices. Cleaning up PciIo mappings (which >>> ultimately boil down to IOMMU mappings) for those aborted DMA operation= s >>> should be the job of the IOMMU driver. >>> >>> Patches 01 through 03 clean up the AtaAtapiPassThru driver in >>> MdeModulePkg a little bit, because at present, (a) it unmaps the buffer= s >>> and disables BMDMA in the wrong order in its DriverBindingStop() >>> function, (b) it doesn't abort pending DMA at ExitBootServices(). >>> >>> This subset can be treated separately from the rest of the series, but = I >>> thought they belonged loosely together (given that AtaAtapiPassThru is >>> used on QEMU's Q35 machine type). >>> >>> Patches 04 through 07 remove VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() >>> calls from the VirtIo drivers' ExitBootServices() handlers. >>> >>> (The conversion of VirtioNetDxe to device addresses is still in progres= s >>> -- Brijesh, when you submit v2 of that, under this approach, there is n= o >>> need to change VirtioNetExitBoot() relative to current upstream, and yo= u >>> can use VirtioOperationBusMasterRead to map outgoing packets.) >>> >>> Patches 08 through 10 make OvmfPkg/IoMmuDxe track all mappings, and >>> unmap all mappings (Read, Write, CommonBuffer) that are in effect when >>> ExitBootServices() is called. It is ensured that PCI and VirtIo drivers >>> abort pending DMA first, and IoMmuDxe clean up the mappings last. >>> >> >> The patches look fine to me >> >> Reviewed-by: Ard Biesheuvel > > Thank you! > >> Given that we are now depending on events signalled in an event >> handler to be queued after all currently pending events, > > marking this (*) > >> I think we >> need to explicitly agree that this behavior that needs to be >> preserved, and documented somewhere, given that the UEFI spec does not >> offer this guarantee. > > The condition that you describe under (*) *is* guaranteed in the UEFI spe= c. > > The *only* bit that is edk2 specific in the last patch is that we invoke > gBS->SignalEvent() from the notification function of an event that > *specifically* has type EVT_SIGNAL_EXIT_BOOT_SERVICES. > > If the event, whose notification function we were calling > gBS->SignalEvent() from, was a plain EVT_NOTIFY_SIGNAL type event, then > *nothing* in the last patch would be edk2-specific. > > * It is guaranteed by the UEFI spec that signaling an event group queues > the notification functions for all not-yet-signaled events in that > group, before the first notification function is invoked (regardless of > the signaled events' TPLs). From "CreateEventEx()": "All events are > guaranteed to be signaled before the first notification action is taken." > > * The UEFI spec guarantees that, within the same TPL, if an event is > signaled that is not pending yet, the notify function will be queued > after notify functions already queued on the same TPL. See "CreateEvent()= ": > > Events exist in one of two states, =E2=80=9Cwaiting=E2=80=9D or =E2= =80=9Csignaled.=E2=80=9D When an > event is created, firmware puts it in the =E2=80=9Cwaiting=E2=80=9D s= tate. When the > event is signaled, firmware changes its state to =E2=80=9Csignaled=E2= =80=9D and, if > EVT_NOTIFY_SIGNAL is specified, places a call to its notification > function in a FIFO queue. There is a queue for each of the =E2=80=9Cb= asic=E2=80=9D > task priority levels defined in Section 7.1 (TPL_CALLBACK, and > TPL_NOTIFY). The functions in these queues are invoked in FIFO > order, starting with the highest priority level queue and proceeding > to the lowest priority queue that is unmasked by the current TPL. If > the current TPL is equal to or greater than the queued notification, > it will wait until the TPL is lowered via > EFI_BOOT_SERVICES.RestoreTPL(). > > * gBS->SignalEvent() is valid to call at TPL_CALLBACK and TPL_NOTIFY > levels (see "Table 23. TPL Restrictions"); in fact it is one of the few > services that can even be called at TPL_HIGH_LEVEL (which is reserved > for the platform firmware). > > The upshot is: > > (1) assume you have Event1, Event2, Event3, Event4 in event group > EventGroupFoobarGuid > > (2) assume all events are EVT_NOTIFY_SIGNAL type > > (3) assume Event1 and Event2 have TPL_NOTIFY, Event3 and Event4 are > TPL_CALLBACK > > (4) Assume Event5 is also of type EVT_NOTIFY_SIGNAL, not part of any > event group, and has TPL_CALLBACK task prio level > > (5) Assume an agent running at TPL_APPLICATION creates an event > temporarily, with type 0 (see the code example in CreateEventEx), and > signals EventGroupFoobarGuid through it. > > (6) All of Event1, Event2, Event3 and Event4 move into the signaled > state at once. NotifyFunction1 and NotifyFunction2 are queued in > unspecified order in the TPL_NOTIFY queue, and Event3 and Event4 are > queued in unspecified order in the TPL_CALLBACK queue. > > (7) Because our current TPL is TPL_APPLICATION, and signaled events of > type EVT_NOTIFY_SIGNAL with higher TPLs exist, the SignalEvent() service > will start dispatching them immediately. > > (8) Either NotifyFunction1 or NotifyFunction2 is called first, running > at TPL_NOTIFY. > > (9) Assume that NotifyFunction2 signals Event5. (it doesn't matter if > NotifyFunction2 is called before or after NotifyFunction1). > > NotifyFunction5 is not dispatched immediately (on the stack of > SignalEvent()) because the current TPL, TPL_NOTIFY, is higher than or > equal to, the TPL of Event5, which is TPL_CALLBACK. So NotifyFunction5 > is queued instead. > > Because NotifyFunction3 and NotifyFunction4 are already in the > TPL_CALLBACK queue (in unspecified relative order), from step (6), > NotifyFunction5 will be queued after both of them, in FIFO order. > > (10) If NotifyFunction1 has not been invoked yet, it is invoked now. It > runs at TPL_NOTIFY. The TPL_NOTIFY queue becomes empty. > > (11) NotifyFunction3 and NotifyFunction4 are invoked in unspecified > order. They both run at TPL_CALLBACK. > > (12) NotifyFunction5 is invoked. It runs at TPL_CALLBACK. The > TPL_CALLBACK queue becomes empty. > > (13) SignalEvent() returns to the agent mentioned in (5). > > > The one thing to remember, from the client programmer's POV, is that > *any* event of type EVT_NOTIFY_SIGNAL can only be dispatched from within > two service calls: > > - SignalEvent(), if the caller is running at a TPL strictly below the > TPL of the event being signaled (directly or via event group GUID), and > > - RestoreTPL(), if the restored TPL falls strictly below the TPL of the > event, and the event has already been signaled. > > IOW, SignalEvent() "calls or queues", and RestoreTPL "de-queues and > calls, or does nothing". > > > So, again, the only question that the UEFI spec does not clarify is > whether we can call gBS->SignalEvent() specifically from an EBS > notification function. > > The intent is likely just that, because: > > EVT_SIGNAL_EXIT_BOOT_SERVICES > [...] This event is of type EVT_NOTIFY_SIGNAL [...] > > and, again, if we call SignalEvent from the NotifyFunction of a plain > EVT_NOTIFY_SIGNAL event, then it's all covered by the spec already. > > My apologies about the wall of text. I probably over-explained it five-fo= ld. > OK