Re: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK.

[Laszlo Ersek <lersek@redhat.com>]

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 <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
>> <michael.d.kinney@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 <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>;
>> edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>> Kinney, Michael D <michael.d.kinney@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:lersek@redhat.com]
>>> Sent: Thursday, October 26, 2017 11:07 PM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
>>> <star.zeng@intel.com>; edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Kinney, Michael D
>>> <michael.d.kinney@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:lersek@redhat.com]
>>>>> Sent: Thursday, October 26, 2017 9:07 PM
>>>>> To: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
>>>>> <jiewen.yao@intel.com>; edk2-devel@lists.01.org
>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Ard Biesheuvel
>>> <ard.biesheuvel@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:lersek@redhat.com]
>>>>>> Sent: Thursday, October 26, 2017 3:53 PM
>>>>>> To: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
>>>>>> <jiewen.yao@intel.com>;
>>>>> edk2-devel@lists.01.org
>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@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.zeng@intel.com>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Star
>>>>>>> -----Original Message-----
>>>>>>> From: Yao, Jiewen
>>>>>>> Sent: Thursday, October 26, 2017 2:51 PM
>>>>>>> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; Ni,
>>>>>>> Ruiyu <ruiyu.ni@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.yao@intel.com>; edk2-devel@lists.01.org
>>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; Zeng,
>>>>>>>> Star <star.zeng@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.zeng@intel.com>; edk2-devel@lists.01.org
>>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@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.yao@intel.com>; edk2-devel@lists.01.org
>>>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; Zeng,
>>>>>>>>> Star <star.zeng@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.zeng@intel.com>; edk2-devel@lists.01.org
>>>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@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.yao@intel.com>;
>>>>>>>>>> edk2-devel@lists.01.org
>>>>>>>>>> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>;
>>>>>>>>>> Zeng, Star <star.zeng@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.zeng@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.zeng@intel.com>
>>>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>>>>>> Signed-off-by: Jiewen Yao <jiewen.yao@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
>