From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 2B3C2203525EE for ; Thu, 26 Oct 2017 08:02:48 -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 E622380C0E; Thu, 26 Oct 2017 15:06:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E622380C0E Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-110.rdu2.redhat.com [10.10.120.110]) by smtp.corp.redhat.com (Postfix) with ESMTP id A7DE3600CA; Thu, 26 Oct 2017 15:06:31 +0000 (UTC) To: "Yao, Jiewen" , "Zeng, Star" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , Ard Biesheuvel , "Kinney, Michael D" References: <1508984017-11740-1-git-send-email-jiewen.yao@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE2AB@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA0491C@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE3FE@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA04CE4@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE45B@shsmsx102.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA04F32@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE4D0@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9AE57E@shsmsx102.ccr.corp.intel.com> <657e3efc-c5f3-df60-4263-07f076f07a1c@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503AA06060@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 26 Oct 2017 17:06:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA06060@shsmsx102.ccr.corp.intel.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.26]); Thu, 26 Oct 2017 15:06:34 +0000 (UTC) Subject: Re: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK. 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: Thu, 26 Oct 2017 15:02:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/26/17 15:36, Yao, Jiewen wrote: > Hi Laszlo > I have discussed this with Mike Kinney offline and some Microsoft engineers. > > We believe the impact of BME disable is different with the impact of SEV. > > For SEV, if a DMA buffer is in transition when SEV bit change, the DMA will still be active, but the content is different. It will bring wrong data from device perspective. > > For BME, if a DMA buffer is in transition when BME is clear, the DMA will be stopped immediately. The device only sees the DMA transition is abort. But there is no wrong data transmitted. I agree with the above analysis. > Because of above reason, we think it is OK to let PCI bus driver to clear BME bit even there is active DMA transaction. The reason why I believe that the PciBusDxe driver should not clear the BME bit is different. It is unrelated to SEV. Imagine a PCI device that requires a special DMA transfer before it can be quiesced at ExitBootServices(). The vendor of this device will implement an EBS notification function like this: - check the private data structure to see if the device needs the special DMA transfer - initiate the special DMA transfer -- write some data to a preallocated and pre-programmed memory buffer, and then push the doorbell in MMIO or config space, - busy wait (poll) unil the transfer is complete, - clear BME (as required by the DWG / spec) - done Now, if PciBusDxe introduces its own EBS notification function, which iterates over all the PciIo instances, and clears the BME bit in each command register, then this notification function may, or may not, be queued before the other one that I described above. If the PciBusDxe function is queued "after", then everything is fine. If it is queued "before", then the driver's own notification function will break. (It may even hang, if the busy wait never completes.) UEFI drivers for PCI devices are currently not forbidden from doing a quick CommonBuffer DMA transfer in their EBS callbacks (as long as they don't allocate or release memory -- but the memory buffer and the corresponding CommonBuffer mapping are not hard to set up in advance, for example in DriverBindingStart()). This means that any automated IOMMU deactivation, and/or BME clearing in PciBusDxe, should occur only after the individual driver callbacks have returned. If PciBusDxe can guarantee this, then I have no objections :) Thanks! Laszlo > > Thank you > Yao Jiewen > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, October 26, 2017 9:07 PM >> To: Zeng, Star ; Yao, Jiewen ; >> edk2-devel@lists.01.org >> Cc: Ni, Ruiyu ; Ard Biesheuvel >> Subject: Re: [edk2] [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >> >> On 10/26/17 10:10, Zeng, Star wrote: >>> Is it security reason when IOMMU disabled and Bus Master not disabled? >> >> No, I don't think there is a security issue here. >> >> But, my previous assessment about VTdDxe was indeed wrong -- there may >> be a *correctness* issue. >> >> Namely, if the IOMMU is de-activated by VTdDxe before PCI drivers abort >> pending DMA, then live system RAM references in the devices may become >> bogus. This is not a security issue (because de-activating the IOMMU >> will grant the devices access to all of the system RAM anyway), instead >> it's a correctness problem: DMA read/write may now be directed to the >> wrong spots in RAM (if the IOMMU mappings were not 1:1 previously). >> >> So, I agree that PCI drivers should get a chance to abort pending DMA >> first, before the IOMMU driver removes the mappings. >> >>> Could our code have a central place to disable Bus Master? For example >> PciBusDxe? >> >> No, I don't think PciBusDxe is a good idea. Higher-level PCI drivers >> might want to do one-shot bus master DMA operations in their own EBS >> callbacks. If PciBusDxe's callback ran first, then these higher-level >> drivers would break. >> >> For the SEV IOMMU driver, we solved the problem in commit 7aee391fa3d0 >> ("OvmfPkg/IoMmuDxe: unmap all IOMMU mappings at ExitBootServices()", >> 2017-09-07). I think the same could be applied to VTdDxe. >> >> >> Another idea (suggested / supported by Ard) was to modify the edk2 >> ExitBootServices() implementation. In CoreExitBootServices() >> [MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c], we could signal a special >> edk2 IOMMU event group, right after signaling >> "gEfiEventExitBootServicesGuid": >> >> // >> // Notify other drivers that we are exiting boot services. >> // >> CoreNotifySignalList (&gEfiEventExitBootServicesGuid); >> >> [HERE] >> >> // >> // Report that ExitBootServices() has been called >> // >> REPORT_STATUS_CODE ( >> EFI_PROGRESS_CODE, >> (EFI_SOFTWARE_EFI_BOOT_SERVICE | >> EFI_SW_BS_PC_EXIT_BOOT_SERVICES) >> ); >> >> This would ensure that the IOMMU callback ran last. >> >> >> Yet another idea (from Jiewen I think?) was to catch the >> EFI_SW_BS_PC_EXIT_BOOT_SERVICES status code in the IOMMU driver. I >> didn't like the idea because (IMO) it put too many requirements on >> platforms. >> >> Thanks, >> Laszlo >> >> >>> >>> >>> Thanks, >>> Star >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>> Sent: Thursday, October 26, 2017 3:53 PM >>> To: Zeng, Star ; Yao, Jiewen ; >> edk2-devel@lists.01.org >>> Cc: Ni, Ruiyu >>> Subject: Re: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>> >>> On 10/26/17 08:54, Zeng, Star wrote: >>>> Ok, please add more description into the commit log, for example, "PCI device >> should disable BME at NOTIFY", etc. >>> >>> Last time we discussed this question, the consensus was that edk2 should not >> present any requirement for PCI drivers that is not required by the UEFI spec. >> UEFI drivers for PCI devices come from third parties as well, and those drivers will >> only care about the UEFI spec (as they should), not about edk2. >>> >>> In fact, I think this additional requirement is not necessary: >>> >>> * In the earlier discussion (for the SEV IoMmuDxe in OVMF), it was really >> necessary to delay the IoMmuDxe ExitBootServices() callback after all the PCI >> driver callbacks. The reason for this was that the IoMmuDxe >>> ExitBootServices() callback was going to *lock down* all RAM from devices, and >> pending DMA had to be aborted before this lock-down. >>> >>> * In comparison, the VTdDxe callback at EBS does the opposite: it "disable[s] >> the protection and allow[s] all DMA access", in Jiewen's words from up-thread. >> So, IMO, neither the PCI driver requirement, nor this patch, are necessary -- >> there is never an IOMMU state that conflicts with a correctly written PCI driver's >> pending DMA operation. >>> >>> Thanks >>> Laszlo >>> >>>> >>>> With that, Reviewed-by: Star Zeng >>>> >>>> >>>> Thanks, >>>> Star >>>> -----Original Message----- >>>> From: Yao, Jiewen >>>> Sent: Thursday, October 26, 2017 2:51 PM >>>> To: Zeng, Star ; edk2-devel@lists.01.org >>>> Cc: Laszlo Ersek (lersek@redhat.com) ; Ni, Ruiyu >>>> >>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>>> >>>> Yes, this PCI patch will be submitted soon. :) >>>> >>>> Thank you >>>> Yao Jiewen >>>> >>>>> -----Original Message----- >>>>> From: Zeng, Star >>>>> Sent: Thursday, October 26, 2017 2:18 PM >>>>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>>>> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, Star >>>>> >>>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>>>> >>>>> So there will be a guidance for this " PCI device disable BME at >>>>> NOTIFY " to be documented? >>>>> >>>>> Thanks, >>>>> Star >>>>> -----Original Message----- >>>>> From: Yao, Jiewen >>>>> Sent: Thursday, October 26, 2017 2:03 PM >>>>> To: Zeng, Star ; edk2-devel@lists.01.org >>>>> Cc: Laszlo Ersek (lersek@redhat.com) >>>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>>>> >>>>> Right. In the future, we will let PCI device disable BME at NOTIFY. >>>>> >>>>> So we let IOMMU use CALLBACK, to make sure BME is disabled before >>>>> IOMMU is disabled. >>>>> >>>>> Thank you >>>>> Yao Jiewen >>>>> >>>>>> -----Original Message----- >>>>>> From: Zeng, Star >>>>>> Sent: Thursday, October 26, 2017 1:55 PM >>>>>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>>>>> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, Star >>>>>> >>>>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >>>>> CALLBACK. >>>>>> >>>>>> I am confused. >>>>>> >>>>>> Is this patch to make the device driver's EBS event notification to >>>>>> be run before IntelVTdDxe's EBS event notification? >>>>>> >>>>>> If yes, this patch seemingly can only make sure the behavior when >>>>>> the device driver's EBS event notification is at NOTIFY, but not CALLBACK. >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Star >>>>>> -----Original Message----- >>>>>> From: Yao, Jiewen >>>>>> Sent: Thursday, October 26, 2017 1:16 PM >>>>>> To: Zeng, Star ; edk2-devel@lists.01.org >>>>>> Cc: Laszlo Ersek (lersek@redhat.com) >>>>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >>>>> CALLBACK. >>>>>> >>>>>> That is fine. >>>>>> >>>>>> Here, disabling IOMMU means to disable the protection and allow all >>>>>> DMA access. >>>>>> I do not think it will bring any functional impact. >>>>>> >>>>>> Thank you >>>>>> Yao Jiewen >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Zeng, Star >>>>>>> Sent: Thursday, October 26, 2017 12:58 PM >>>>>>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) ; Zeng, >>>>>>> Star >>>>>>> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL >>>>>>> to >>>>>> CALLBACK. >>>>>>> >>>>>>> Some device driver may also have exit boot service event at >>>>>>> CALLBACK, for example AtaPassThruExitBootServices() that was added >>>>>>> by >>>>> Laszlo. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Star >>>>>>> -----Original Message----- >>>>>>> From: Yao, Jiewen >>>>>>> Sent: Thursday, October 26, 2017 10:14 AM >>>>>>> To: edk2-devel@lists.01.org >>>>>>> Cc: Zeng, Star >>>>>>> Subject: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >> CALLBACK. >>>>>>> >>>>>>> Change ExitBootServices TPL to CALLBACK, so that a device can >>>>>>> disable BME before IOMMU grants access right. >>>>>>> >>>>>>> Cc: Star Zeng >>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>> Signed-off-by: Jiewen Yao >>>>>>> --- >>>>>>> IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git >>>>>>> a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>>>>> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>>>>> index f5de01f..4a4d82e 100644 >>>>>>> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>>>>> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c >>>>>>> @@ -483,7 +483,7 @@ InitializeDmaProtection ( >>>>>>> >>>>>>> Status = gBS->CreateEventEx ( >>>>>>> EVT_NOTIFY_SIGNAL, >>>>>>> - TPL_NOTIFY, >>>>>>> + TPL_CALLBACK, >>>>>>> OnExitBootServices, >>>>>>> NULL, >>>>>>> &gEfiEventExitBootServicesGuid, @@ -492,7 >>>>>>> +492,7 @@ InitializeDmaProtection ( >>>>>>> ASSERT_EFI_ERROR (Status); >>>>>>> >>>>>>> Status = EfiCreateEventLegacyBootEx ( >>>>>>> - TPL_NOTIFY, >>>>>>> + TPL_CALLBACK, >>>>>>> OnLegacyBoot, >>>>>>> NULL, >>>>>>> &LegacyBootEvent >>>>>>> -- >>>>>>> 2.7.4.windows.1 >>>> >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >