I also doubt such device driver exists. Thanks/Ray
> -----Original Message----- > From: Yao, Jiewen > Sent: Friday, October 27, 2017 9:47 AM > To: Ni, Ruiyu <ruiyu...@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. > > 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 <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
