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

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

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