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

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

[Jiewen Yao]

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

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

[Zeng, Star]

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

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

[Yao, Jiewen]

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

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

[Zeng, Star]

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

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

[Yao, Jiewen]

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

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

[Zeng, Star]

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

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

[Yao, Jiewen]

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

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

[Zeng, Star]

Ok, please add more description into the commit log, for example, "PCI device should disable BME at NOTIFY", etc.

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

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

[Yao, Jiewen]

Agree!

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, October 26, 2017 2:55 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH] IntelSiliconPkg/VTdDxe: Change EBS Event TPL to CALLBACK.
> 
> Ok, please add more description into the commit log, for example, "PCI device
> should disable BME at NOTIFY", etc.
> 
> 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

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

[Laszlo Ersek]

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
> 

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

[Zeng, Star]

Is it security reason when IOMMU disabled and Bus Master not disabled?

Could our code have a central place to disable Bus Master? For example PciBusDxe?


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
> 

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

[Laszlo Ersek]

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
> 

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

[Yao, Jiewen]

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.

Because of above reason, we think it is OK to let PCI bus driver to clear BME bit even there is active DMA transaction.

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

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

[Laszlo Ersek]

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

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

[Yao, Jiewen]

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

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

[Ni, Ruiyu]

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

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

[Yao, Jiewen]

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

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

[Ni, Ruiyu]

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

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

[Yao, Jiewen]

I think it does exist.
If I use IOMMU to prevent all DMA transaction, I have seen XHCI driver and AHCI driver reporting error.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, October 27, 2017 10:38 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.
> 
> 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
> > > > >>>
> > > > >

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

[Laszlo Ersek]

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
>

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

[Yao, Jiewen]

Sure. We will definitely go through normal edk2 process.

I mentioned it here, just because I am seeing lots of discussion around it.

There won't be any shortcut to check in a patch.

thank you!
Yao, Jiewen


> 在 2017年10月28日，上午12:41，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
>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel