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 EFB9921E977F6 for ; Tue, 5 Sep 2017 13:14:24 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C65627570B; Tue, 5 Sep 2017 20:17:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C65627570B Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4B3D661345; Tue, 5 Sep 2017 20:17:11 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel , Jiewen Yao , Star Zeng 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> From: Laszlo Ersek Message-ID: <5992734a-1ded-9d4b-36bd-99c13f30ca32@redhat.com> Date: Tue, 5 Sep 2017 22:17:10 +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: <4f286363-6f04-bee0-8b12-34b50a813e70@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 05 Sep 2017 20:17:14 +0000 (UTC) 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 20:14:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/05/17 20:57, Brijesh Singh wrote: > Hi Laszlo, > > Thanks for quick reviews. I will go through each feedback address them > in v2. > > > > On 09/05/2017 06:47 AM, Laszlo Ersek wrote: > > [...] > >> >> Please also modify the commit message similarly: "map the VRING system >> physical address[es] to device address[es]". >> >> >> Would it be OK with you to submit only these two patches next? > > > I saw that you looked at other patches, just wondering, do you still > want me > to submit two patches and advance in small steps ? I'm not so sure anymore... I haven't looked at patch #3 yet. I guess I could review it tomorrow. ... 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. * 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. * Option #3: ignore pending mappings (= decrypted memory areas) at ExitBootServices(), leave them all to the OS. Problem: how will the OS find them? Should we introduce another UEFI configuration table? Config tables are generally allocated in BootServicesData type memory, so the boot loader has to save them. Who will write the grub2 code? Who will write the Linux kernel code to parse the table, and to re-encrypt the memory (inherited from the firmware) in-place? * Option #4: ignore pending mappings (= decrypted memory areas) at ExitBootServices(). Ignore them in the OS too. Let's hope that "it's just a few pages of stale data, it won't compromise guest confidentiality when the hypervisor is breached". Doesn't sound like a good sales pitch for SEV. Our approach thus far has been option #1 for the OvmfPkg VirtIo drivers, and option #4 for all PCI drivers outside of OvmfPkg. If you are fine with this, I am as well; we can continue this approach. (We should be conscious of it though.) My other series (see above) was inteded to extend option #1 to some MdeModulePkg drivers (the main ones that we pull into OVMF, USB 1-2-3 and AHCI), and to start a discussion. If you believe that extending option #1 to MdeModulePkg would be better for SEV, we should await the outcome of that discussion. (Please do participate.) Thanks, Laszlo >> I wouldn't like to look at the rest of the patches just now. I expect >> quite a few tweaks -- especially because I would *really* like to keep >> "TechNotes.txt" up to date! --, and I'd like to keep my focus, and >> advance in small steps. I must re-read TechNotes.txt myself, in parallel >> to the progress that we make with this series. >> >> Once we consider these patches complete, we can test them and commit >> them in isolation. Further versions of the series won't have to repeat >> these patches. >> >> I'll strive to be very responsive, so that you don't have to wait long >> after every small step. >> >