Sure. We will definitely go through normal edk2 process. I mentioned it here, just because I am seeing lots of discussion around it.
There won't be any shortcut to check in a patch. thank you! Yao, Jiewen > 在 2017年10月28日，上午12:41，Laszlo Ersek <ler...@redhat.com> 写道： > >> On 10/27/17 03:47, Yao, Jiewen wrote: >> I think the error might be PCI device specific. >> >> BTW: We already have bugzillar on that >> https://bugzilla.tianocore.org/show_bug.cgi?id=739 >> >> It has been validated by Microsoft. We can validate more device cards >> to see if there is any issue. > > Can this work please be posted to edk2-devel for the usual review? > > > Also, can we make this feature dependent on a Feature PCD? (Maybe even > better: a dynamic BOOLEAN PCD.) > > (Perhaps the code is already written like that; I can't easily tell from > > https://github.com/Microsoft/MS_UEFI/tree/share/disablebmeonexit2 > > which is linked under > > https://bugzilla.tianocore.org/show_bug.cgi?id=739#c0 > > The github summary for the branch is: > > This branch is 22508 commits ahead, 3 commits behind about > > which confuses me; I would expect a feature branch to be forked from a > recent commit on edk2 master.) > > > Regarding the PCD, I'm OK if the default value in the .dec file is TRUE; > I'm just concerned that I might not be able to test the change with 100% > coverage. QEMU supports PCI and PCI Express hierarchies flexibly [*], > there are several kinds of bridges and Express ports. And, a good number > of virtio device types (usable as PCI/PCIe endpoints) exist as well, > supported by OVMF. Add to that the PCI/PCIe expander bridges (they > basically provide multiple root bridges on a single host bridge), and > then SEV (for which DMA is not transparent; it requires actual mapping > -- decryption and re-encryption). > > So, I'd like to have an "escape switch". > > > [*] The PCI / PCIe support is in fact so flexible in QEMU that we have > to limit ourselves -- and users too -- via guidelines. > > https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/pcie.txt;hb=HEAD > > https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/pci_expander_bridge.txt;hb=HEAD > > https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/pcie_pci_bridge.txt;hb=HEAD > > Thank you, > Laszlo > >>> -----Original Message----- >>> From: Ni, Ruiyu >>> Sent: Friday, October 27, 2017 8:54 AM >>> To: Yao, Jiewen <jiewen....@intel.com>; Laszlo Ersek <ler...@redhat.com>; >>> Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>; Kinney, Michael D >>> <michael.d.kin...@intel.com> >>> Subject: RE: [edk2] [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >>> CALLBACK. >>> >>> Jiewen, >>> If the BME bit is cleared in Command register, but a device driver >>> uses DMA to transfer data, what kind of error will be seen by SW? >>> >>> -----Original Message----- >>> From: Yao, Jiewen >>> Sent: Friday, October 27, 2017 8:34 AM >>> To: Laszlo Ersek <ler...@redhat.com>; Zeng, Star <star.z...@intel.com>; >>> edk2-devel@lists.01.org >>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ard Biesheuvel >>> <ard.biesheu...@linaro.org>; >>> Kinney, Michael D <michael.d.kin...@intel.com> >>> Subject: RE: [edk2] [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to >>> CALLBACK. >>> >>> Good Info. I think a correct implementation should not use busy wait. >>> >>> It should add error handling to check if there is hardware error during >>> that. >>> >>>> - busy wait (poll) unil the transfer is complete, >>> >>> The process of busy wait should be something like below: >>> while(TRUE) { >>> if (error) { >>> break; >>> } >>> GetData >>> if (complete) { >>> Break >>> } >>> } >>> >>> BME clear will trigger error break. >>> >>> Thank you >>> Yao Jiewen >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>>> Sent: Thursday, October 26, 2017 11:07 PM >>>> To: Yao, Jiewen <jiewen....@intel.com>; Zeng, Star >>>> <star.z...@intel.com>; edk2-devel@lists.01.org >>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ard Biesheuvel >>>> <ard.biesheu...@linaro.org>; Kinney, Michael D >>>> <michael.d.kin...@intel.com> >>>> Subject: Re: [edk2] [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event >>>> TPL to CALLBACK. >>>> >>>>> 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:ler...@redhat.com] >>>>>> Sent: Thursday, October 26, 2017 9:07 PM >>>>>> To: Zeng, Star <star.z...@intel.com>; Yao, Jiewen >>>>>> <jiewen....@intel.com>; edk2-devel@lists.01.org >>>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Ard Biesheuvel >>>> <ard.biesheu...@linaro.org> >>>>>> 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:ler...@redhat.com] >>>>>>> Sent: Thursday, October 26, 2017 3:53 PM >>>>>>> To: Zeng, Star <star.z...@intel.com>; Yao, Jiewen >>>>>>> <jiewen....@intel.com>; >>>>>> edk2-devel@lists.01.org >>>>>>> Cc: Ni, Ruiyu <ruiyu...@intel.com> >>>>>>> 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 <star.z...@intel.com> >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Star >>>>>>>> -----Original Message----- >>>>>>>> From: Yao, Jiewen >>>>>>>> Sent: Thursday, October 26, 2017 2:51 PM >>>>>>>> To: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org >>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@redhat.com>; Ni, >>>>>>>> Ruiyu <ruiyu...@intel.com> >>>>>>>> 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 <jiewen....@intel.com>; edk2-devel@lists.01.org >>>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@redhat.com>; Zeng, >>>>>>>>> Star <star.z...@intel.com> >>>>>>>>> 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 <star.z...@intel.com>; edk2-devel@lists.01.org >>>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@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 <jiewen....@intel.com>; edk2-devel@lists.01.org >>>>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@redhat.com>; Zeng, >>>>>>>>>> Star <star.z...@intel.com> >>>>>>>>>> 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 <star.z...@intel.com>; edk2-devel@lists.01.org >>>>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@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 <jiewen....@intel.com>; >>>>>>>>>>> edk2-devel@lists.01.org >>>>>>>>>>> Cc: Laszlo Ersek (ler...@redhat.com) <ler...@redhat.com>; >>>>>>>>>>> Zeng, Star <star.z...@intel.com> >>>>>>>>>>> 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 <star.z...@intel.com> >>>>>>>>>>> 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 <star.z...@intel.com> >>>>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>>>>>> Signed-off-by: Jiewen Yao <jiewen....@intel.com> >>>>>>>>>>> --- >>>>>>>>>>> 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 >>>>>>> >>>>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
