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
