From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (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 0416A21E97801 for ; Tue, 5 Sep 2017 14:08:21 -0700 (PDT) Received: by mail-it0-x22b.google.com with SMTP id p6so5193602itb.1 for ; Tue, 05 Sep 2017 14:11:10 -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=IWoRHa1H3c7iNmPLzFJvKU/Dxc5NIXSNuEKMr1TIKnQ=; b=BP9FTEE3u2aIRpd84QUYjww6DJx8JXUoeJu/wH0BjQDI8enPJW3vI4awl3Rj8ZFBvR 1QId4AhPZTbKWFWcdf/Y9fAiFdjm7Ip2RDaYJZ7pxwhr0/lGovAT72rSt15//0tm9buq O1Q2dn3l4D5K82w+xMV7fGlFGBIBMaWx+vl6c= 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=IWoRHa1H3c7iNmPLzFJvKU/Dxc5NIXSNuEKMr1TIKnQ=; b=OuGiFQq1dNw2rAgws2YFQ2D9AoYMlEs0DR17uy3booCVomZI2yXqr1PoI0Vig/crPr X/aLNUo1ND7nHJ7YPfGrRmJfIuv51cFN4wBkCVCkhPz7G4ooVBmYr5/208UYd1QVBPbD MohsqEniMVC7Set7dNhFab32O/9jz/AknM+s3kTcUAcLTzTtSoAefouifH8i9lb+CzaP HPdyA0fGJLJ5ADMzhuEekmht2cZItaCikmPiIWa1TnHJO69AiiO5YdX54vz8C7W5S3sS fs7ETHBT8uIzet6BjdcwuPZPA90curRQN1B3c5kcBj+bjPGxCM6VT0ci7uqtldOaGwQq BIKA== X-Gm-Message-State: AHPjjUhm97VPekKrK0jFQUQYubmADF9xkhbJ1PE+PqU5TFr+2mRrgEbz o1vymbUA3ntBUdqedNFNVS5BWvr+pyPW X-Google-Smtp-Source: ADKCNb74eaO0m1rwBlbgO9MHANbZ6t5/dd4GRVg4L18GaXR/iy0Mv+DrTBYMbC8/nLkSwi2DyJrArfdnauZ+h216LcA= X-Received: by 10.36.184.4 with SMTP id m4mr561762ite.41.1504645869546; Tue, 05 Sep 2017 14:11:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Tue, 5 Sep 2017 14:11:08 -0700 (PDT) In-Reply-To: <5992734a-1ded-9d4b-36bd-99c13f30ca32@redhat.com> 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 22:11:08 +0100 Message-ID: To: Laszlo Ersek Cc: Brijesh Singh , "edk2-devel@lists.01.org" , Jordan Justen , Tom Lendacky , 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 21:08:21 -0000 Content-Type: text/plain; charset="UTF-8" On 5 September 2017 at 21:17, Laszlo Ersek wrote: [...] > ... Based on the discussion in the other thread ("[edk2] [PATCH 0/4] > MdeModulePkg: some PCI HC drivers: unmap common buffers at > ExitBootServices()"), I'm worried that we might need another > restructuring for the IOMMU driver, and for the ExitBootServices() > handlers for the virtio drivers (removing the Unmap() calls). > > If that's the case, then it wouldn't be good if you wasted work on > VirtioNetExitBoot() in v2 of this series. > > Also under patch v1 #4, I requested that you not use > VirtioOperationBusMasterRead for DMA that remains pending after the > protocol member function returns, because we cannot Unmap() > BusMasterRead in VirtioNetExitBoot() without freeing memory. Dependent > of the outcome of said other thread, this might not be good advice after > all. > > I'd be pretty disappointed if we had to go back to the drawing board at > this stage. In my opinion, the UEFI-2.7 spec doesn't offer any > facilities to us that would let us reliably and safely Unmap() bus > master operations (= re-encrypt memory) at ExitBootServices(), for such > PCI device drivers that we cannot modify. Whatever we do at this point > looks like a hack: > > > * Option #1: modify Virtio and other PCI drivers to use only > CommonBuffer operations for asynch DMA, and manually Unmap() those > operations in the ExitBootServices() handlers of the drivers. In > addition, guarantee that Unmap() for CommonBuffer will not release > memory. > > This is the approach I've been supporting. We could implement it for > OVMF, because the community controls most of the platform (QEMU, > KVM, OVMF), OVMF is 100% open source, and we can propose patches for > other (e.g. MdeModulePkg) PCI drivers in edk2, if necessary. > > Problem: PCI drivers are not required by the spec, or the Driver > Writer's Guide, to Unmap() on ExitBootServices() (and then to Unmap() > only CommonBuffer). Also, PciIo implementations are not required by > the spec to behave like this (= not free memory when Unmap()ing > CommonBuffer). We can satisfy those assumptions in OvmfPkg, but > perhaps not in MdeModulePkg drivers. > I think you will have much bigger problems with out of tree PCI drivers that never use Map/Unmap in the first place, because stuff just works on PCs if you omit them. This is actually the reason I am quite pleased with this SEV support: it means x86 drivers will start breaking in the same way as they would have on ARM systems with non-coherent DMA or non-1:1 mapped PCI, and vendors will have to clean up their code anyway, not just for ARM compatibility. The only requirement imposed on DMA capable devices is that they cease to perform DMA after ExitBootServices. Anything beyond that should not be the responsibility of the device driver, simply because that requirement did not exist before, and so we cannot go back in time and add it. > * 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 (which means it could be the second time it was called). 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. 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.