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