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