From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4BB6F20945C13 for ; Fri, 8 Sep 2017 02:45:54 -0700 (PDT) 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 F22994E4CA; Fri, 8 Sep 2017 09:48:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F22994E4CA Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-10.rdu2.redhat.com [10.10.120.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id 89BDD5D97B; Fri, 8 Sep 2017 09:48:43 +0000 (UTC) To: Ard Biesheuvel , Leif Lindholm , "afish@apple.com" , "Kinney, Michael D" Cc: edk2-devel-01 , Brijesh Singh , Eric Dong , Jiewen Yao , Jordan Justen , Star Zeng References: <20170907224116.895-1-lersek@redhat.com> From: Laszlo Ersek Message-ID: <49a0e877-0500-b1f5-e4ba-753df311f868@redhat.com> Date: Fri, 8 Sep 2017 11:48:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: 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.38]); Fri, 08 Sep 2017 09:48:46 +0000 (UTC) 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 09:45:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 operations >> 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 buffers >> 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 progress >> -- Brijesh, when you submit v2 of that, under this approach, there is no >> need to change VirtioNetExitBoot() relative to current upstream, and you >> 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 spec. 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, “waiting” or “signaled.” When an event is created, firmware puts it in the “waiting” state. When the event is signaled, firmware changes its state to “signaled” 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 “basic” 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-fold. Thanks Laszlo