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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.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
> > >>>
> > >