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. Thank you Yao Jiewen > -----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
