From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (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 DE24721E977F5 for ; Tue, 5 Sep 2017 15:15:52 -0700 (PDT) Received: by mail-it0-x233.google.com with SMTP id k189so5572625itk.0 for ; Tue, 05 Sep 2017 15:18:42 -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; bh=4Z0gjeKk9T9Bz2nAvfIKD0XLtczGj3PJymG3hJcA1F0=; b=ZVFWCazksFHkv8/31uXEQ6SDBYbLzDNWZ3+S8pUDPJXO1XIyTk2mie/zam5Z1DIqu4 ZFursSEpBzRKR8vhCsLvk1WGMhQeSwP2MxQIfAtmgQn4yY+ufbJumFYDApJDCAYJnXGo wlSZ0eNQrIui8EUbnOHkzrzh8K8/QzlpU/mVc= 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; bh=4Z0gjeKk9T9Bz2nAvfIKD0XLtczGj3PJymG3hJcA1F0=; b=aypsGma5vDhyZSEnDnHSbbgjekOptYKpdLn5iiNE8fhvv0RxHTU6z1hvFnEvpZ1OxC d++CA7oNcdJdxtTYu7aawUIgGNqUWHm9wQaJcYWfV7ZW+CvMzTBCZwjY+VDE2E9hMZmW dRc4G7eEVx3LD92pOa5acN/YLIrW7eL7+mJu1CdWXJNhX/XzKk3UNL+ZJe2xRSDBxel2 P4VUNmHMgStEdhDEMfJQZzvGYFZPGRfixX4+2v4oTngjdiJa/bUbxFWpH2i4QboJhnqE 6103OcbTrwpWuLHfihp6MXM9s9QL4Rh7Ht4Sx5QFnOMOHeEg4lhtDDY7mQgiVOayyYs/ NWWA== X-Gm-Message-State: AHPjjUh3Tp+ABY1KryuebOQOooYCz6C1ONJdWinnq/mLHrrsV4tzcZDl 3rSpVP9PJB3vNeIRqkH0pRLTjw/Bqkxp X-Google-Smtp-Source: ADKCNb4rWAj4keRlLngJTflmD3gRUme+7lzRK8aQKjO7R5+JAuiJvJu2TZaMUT+HGaGGoX+I+lvcaT25Rg3KNexR70Y= X-Received: by 10.36.8.138 with SMTP id 132mr772482itc.162.1504649921537; Tue, 05 Sep 2017 15:18:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Tue, 5 Sep 2017 15:18:40 -0700 (PDT) In-Reply-To: References: <1504265045-19008-1-git-send-email-brijesh.singh@amd.com> <1504265045-19008-2-git-send-email-brijesh.singh@amd.com> <04fefb16-23d5-f6df-0657-9d4c5e96ac57@redhat.com> <4f286363-6f04-bee0-8b12-34b50a813e70@amd.com> <5992734a-1ded-9d4b-36bd-99c13f30ca32@redhat.com> From: Ard Biesheuvel Date: Tue, 5 Sep 2017 23:18:40 +0100 Message-ID: To: Laszlo Ersek Cc: Tom Lendacky , Jordan Justen , "edk2-devel@lists.01.org" , Jiewen Yao , Star Zeng Subject: Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap() 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: Tue, 05 Sep 2017 22:15:53 -0000 Content-Type: text/plain; charset="UTF-8" On 5 September 2017 at 22:59, Laszlo Ersek wrote: > On 09/05/17 23:11, Ard Biesheuvel wrote: >> On 5 September 2017 at 21:17, Laszlo Ersek wrote: [...] >>> * Option #2: add an ExitBootServices() handler to the IOMMU driver, and >>> clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers >>> are finalized in their ExitBootServices() handlers. Ignore mappings in >>> the drivers' ExitBootServices() handlers. >>> >>> Problem: the keyword is "after". Under this approach, we *must* clean >>> up the mappings in the IOMMU driver, but we *must not* do that before >>> the device drivers are finished. And the UEFI spec does not allow us >>> to express a dependency order between ExitBootServices() handlers. >>> >>> We could hack that around by deferring the actual cleanup to *another* >>> event handler, signaled from the IOMMU's "normal" ExitBootServices() >>> handler. >>> >>> Problem: the UEFI spec does not promise that signaling events from >>> ExitBootServices() handlers is safe. >>> >>> Problem: if some PCI driver does the same trick for whatever reason in >>> its ExitBootServices() handler, then we're back to square one. >>> >>> >> >> I think this is the only feasible option. Fortunately, we don't have >> to solve this at the spec level, but only have to deal with the >> implementation. >> >> What we need is a method in the IOMMU protocol that is invoked when >> the ExitBootServices implementation is about to return EFI_SUCCESS > > Yes, after "everything else" is done. > > (Of course, this is the age-old problem with UEFI, when components that > were originally meant to be independent now try to order themselves in > some fashion. For example, "let me just install, or patch, this ACPI > table or configuration table quickly at ReadyToBoot, *after* everyone > else is done, and I get a good look at system state". Now combine two > such DXE drivers into a firmware, and hilarity ensues as they fight for > the last word.) > > No facility exists to my knowledge (on the spec level) that would enable > such fine delaying or dependency ordering between EBS handlers. The only > ordering is between notification functions enqueued at TPL_NOTIFY vs. > TPL_CALLBACK, and there is a guarantee (I think?) that if you get > signalled demonstrably later (i.e., not via an event group), then you > get queued for later, within the same TPL. > > The problem (for this discussion) is that any random PCI driver can > register its EBS event notification function at either TPL_NOTIFY or at > TPL_CALLBACK, plus that all EBS events are signaled together as an event > group. Consequently, if the IOMMU driver registers its EBS notifier at > TPL_NOTIFY, it is guaranteed to run earlier than all notifiers with > TPL_CALLBACK (which is exactly what we *don't* want). If the IOMMU > registers its EBS handler at TPL_CALLBACK, then (due to being signaled > through a large event group) the notifier will still be invoked > somewhere in the middle of a bunch of other TPL_CALLBACK-level notifiers > -- that is, not necessarily in the last position. > > Hence my floating of a hack to re-queue the notification (to another > TPL_CALLBACK handler), so that we get to the "end of the low-prio > queue". But calling SignalEvent() from an EBS handler is not explicitly > permitted in the spec. (Below you even suggest that an EBS handler > should not call any boot service.) > > > Of course, if CoreExitBootServices() can be updated specifically for > this use case, I won't protest. > I think this is the only solution, and not unreasonable given that the IOMMU protocol is private to EDK2. >> >> (which means it could be the second time it was called). > > Side remark: the CoreExitBootServices() implementation does not notice > memory map changes incurred by the ExitBootServices() handlers, in my > interpretation. > > CoreExitBootServices() has a MapKey (= "memmap generation") check early > on, in CoreTerminateMemoryMap(). This check catches memmap changes > between the last GetMemoryMap(), and the call to ExitBootServices(). > > After this check succeeds, the notify functions are kicked, and on this > path, CoreExitBootServices() simply cannot return any other value than > EFI_SUCCESS. So whatever mess the individual callbacks make, it doesn't > notice or report. > CoreExitBootServices() disables the timer first, and so it will return with event dispatch disabled if it fails, ensuring that it is no longer possible for an event handler to be invoked between GetMemoryMap and ExitBootServices. > Perhaps this is better for your argument, actually. I'm not fully sure. > Not really :-) >> This severely >> limits what the method is actually capable of doing, and I think it >> should not be allowed to call any boot or DXE services at all, but it >> should still be sufficient for some linked list traversals and >> CopyMem() operations, and whatever else is needed to re-encrypt the >> memory. > > Yes, the necessary actions are sufficiently low-level for this. The > problem is the ordering. > >> >> And actually, this is not only useful for SEV, other IOMMU drivers >> will have to deal with the same issue: in most cases, you'll want to >> disable it before handing over to the OS, but this can never be done >> safely in a EBS event handler for precisely the reasons you pointed >> out. Most PCI devices probably deal with this gracefully, but tearing >> down mappings should simply be avoided if a device could still be >> accessing it. >> > > Fully agreed. Once Jiewen adds the policy option to lock down VT-d at > EBS (I hope I understood that plan right), dependent on OS support for > the IOMMU, the IOMMU faults can show up just the same at EBS, until the > rest of the PCI driver EBS handlers finish up. > Indeed. I think it is justified to treat the IOMMU protocol specially.