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 <[email protected]>; Laszlo Ersek <[email protected]>; >> Zeng, Star <[email protected]>; [email protected] >> Cc: Ard Biesheuvel <[email protected]>; Kinney, Michael D >> <[email protected]> >> 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 <[email protected]>; Zeng, Star <[email protected]>; >> [email protected] >> Cc: Ni, Ruiyu <[email protected]>; Ard Biesheuvel >> <[email protected]>; >> Kinney, Michael D <[email protected]> >> 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:[email protected]] >>> Sent: Thursday, October 26, 2017 11:07 PM >>> To: Yao, Jiewen <[email protected]>; Zeng, Star >>> <[email protected]>; [email protected] >>> Cc: Ni, Ruiyu <[email protected]>; Ard Biesheuvel >>> <[email protected]>; Kinney, Michael D >>> <[email protected]> >>> 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:[email protected]] >>>>> Sent: Thursday, October 26, 2017 9:07 PM >>>>> To: Zeng, Star <[email protected]>; Yao, Jiewen >>>>> <[email protected]>; [email protected] >>>>> Cc: Ni, Ruiyu <[email protected]>; Ard Biesheuvel >>> <[email protected]> >>>>> 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:[email protected]] >>>>>> Sent: Thursday, October 26, 2017 3:53 PM >>>>>> To: Zeng, Star <[email protected]>; Yao, Jiewen >>>>>> <[email protected]>; >>>>> [email protected] >>>>>> Cc: Ni, Ruiyu <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Star >>>>>>> -----Original Message----- >>>>>>> From: Yao, Jiewen >>>>>>> Sent: Thursday, October 26, 2017 2:51 PM >>>>>>> To: Zeng, Star <[email protected]>; [email protected] >>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]>; Ni, >>>>>>> Ruiyu <[email protected]> >>>>>>> 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 <[email protected]>; [email protected] >>>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]>; Zeng, >>>>>>>> Star <[email protected]> >>>>>>>> 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 <[email protected]>; [email protected] >>>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]> >>>>>>>> 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 <[email protected]>; [email protected] >>>>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]>; Zeng, >>>>>>>>> Star <[email protected]> >>>>>>>>> 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 <[email protected]>; [email protected] >>>>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]> >>>>>>>>> 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 <[email protected]>; >>>>>>>>>> [email protected] >>>>>>>>>> Cc: Laszlo Ersek ([email protected]) <[email protected]>; >>>>>>>>>> Zeng, Star <[email protected]> >>>>>>>>>> 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: [email protected] >>>>>>>>>> Cc: Zeng, Star <[email protected]> >>>>>>>>>> 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 <[email protected]> >>>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>>>>> Signed-off-by: Jiewen Yao <[email protected]> >>>>>>>>>> --- >>>>>>>>>> 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 >>>>>> [email protected] >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>> >>>> > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
