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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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