mbox series

[edk2,0/6] implement standalone MM versions of the variable runtime drivers

Message ID 20190103182825.32231-1-ard.biesheuvel@linaro.org
Headers show
Series implement standalone MM versions of the variable runtime drivers | expand

Message

Ard Biesheuvel Jan. 3, 2019, 6:28 p.m. UTC
This series proposed an alternative approach to the series sent out by
Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the
special PCD, as well as some other if() conditionals.

The primary difference is that this series defines and implements
MmServicesTableLib in such a way that the traditional SMM drivers
can use it as well. This is appropriate, considering that the PI
spec has rebranded traditional SMM as one implementation of the generic
MM framework.

Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib
library class, but for all SMM flavours, not only for standalone MM.

Patch #2 implements MmServicesTableLib for traditional SMM implementations.

Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM
driver that invoke boot services are separated from the core SMM pieces.

Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

Patches #5 and #6 do the same, respectively, for the variable runtime driver.

This approach minimizes the delta, and thus the maintenance burden, between
the traditional SMM and standalone MM drivers, while not resorting to runtime
checks or other conditionals in the code to implement logic that should be
decided at build time.

Note that this series only covers part of the work contributed by Jagadeesh.
This series focuses on the MdePkg and MdeModulePkg changes that affect shared
code.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>
Cc: Achin Gupta <Achin.Gupta@arm.com>
Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>

Ard Biesheuvel (5):
  MdePkg: implement MmServicesTableLib based on traditional SMM
  MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses
  MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version
  MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses
  MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

Jagadeesh Ujja (1):
  MdePkg/Include: add MmServicesTableLib header file

 MdeModulePkg/MdeModulePkg.dsc                 |   1 +
 .../FaultTolerantWrite.h                      |  22 ++-
 .../FaultTolerantWriteDxe.c                   |  31 ++++
 .../FaultTolerantWriteSmm.c                   |  54 +++----
 .../FaultTolerantWriteSmm.inf                 |   5 +-
 .../FaultTolerantWriteSmmCommon.h             |  31 ++++
 .../FaultTolerantWriteSmmDxe.c                |   1 +
 .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++
 .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++
 .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++
 .../UpdateWorkingBlock.c                      |  10 +-
 .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--
 .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++
 .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----
 .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-
 .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++
 .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++
 .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++
 MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++
 .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++
 .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++
 .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++
 MdePkg/MdePkg.dec                             |   4 +
 MdePkg/MdePkg.dsc                             |   1 +
 24 files changed, 916 insertions(+), 103 deletions(-)
 create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c
 create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf
 create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
 create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c
 create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h
 create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
 create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
 create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel Jan. 3, 2019, 7:13 p.m. UTC | #1
On Thu, 3 Jan 2019 at 19:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> This series proposed an alternative approach to the series sent out by

> Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> special PCD, as well as some other if() conditionals.

>


That would be

[0] https://lists.01.org/pipermail/edk2-devel/2019-January/034542.html

Also, I seem to have included a BaseTools/ patch in error. Apologies
for the sloppiness.


> The primary difference is that this series defines and implements

> MmServicesTableLib in such a way that the traditional SMM drivers

> can use it as well. This is appropriate, considering that the PI

> spec has rebranded traditional SMM as one implementation of the generic

> MM framework.

>

> Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> library class, but for all SMM flavours, not only for standalone MM.

>

> Patch #2 implements MmServicesTableLib for traditional SMM implementations.

>

> Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> driver that invoke boot services are separated from the core SMM pieces.

>

> Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

>

> Patches #5 and #6 do the same, respectively, for the variable runtime driver.

>

> This approach minimizes the delta, and thus the maintenance burden, between

> the traditional SMM and standalone MM drivers, while not resorting to runtime

> checks or other conditionals in the code to implement logic that should be

> decided at build time.

>

> Note that this series only covers part of the work contributed by Jagadeesh.

> This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> code.

>

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Hao Wu <hao.a.wu@intel.com>

> Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> Cc: Achin Gupta <Achin.Gupta@arm.com>

> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> Cc: Sami Mujawar <Sami.Mujawar@arm.com>

>

> Ard Biesheuvel (5):

>   MdePkg: implement MmServicesTableLib based on traditional SMM

>   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

>   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

>   MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

>   MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

>

> Jagadeesh Ujja (1):

>   MdePkg/Include: add MmServicesTableLib header file

>

>  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

>  .../FaultTolerantWrite.h                      |  22 ++-

>  .../FaultTolerantWriteDxe.c                   |  31 ++++

>  .../FaultTolerantWriteSmm.c                   |  54 +++----

>  .../FaultTolerantWriteSmm.inf                 |   5 +-

>  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

>  .../FaultTolerantWriteSmmDxe.c                |   1 +

>  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

>  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

>  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

>  .../UpdateWorkingBlock.c                      |  10 +-

>  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

>  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

>  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

>  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

>  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

>  .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

>  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

>  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

>  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

>  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

>  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

>  MdePkg/MdePkg.dec                             |   4 +

>  MdePkg/MdePkg.dsc                             |   1 +

>  24 files changed, 916 insertions(+), 103 deletions(-)

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

>  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

>

> --

> 2.17.1

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Jan. 7, 2019, 12:44 p.m. UTC | #2
Ard:
  I agree this design is good. But, I need some time to evaluate its impact on our X86 platform. Could you wait for several days?

Thanks
Liming
> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Friday, January 4, 2019 2:28 AM

> To: edk2-devel@lists.01.org

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney,

> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> Abraham <thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> Subject: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> 

> This series proposed an alternative approach to the series sent out by

> Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> special PCD, as well as some other if() conditionals.

> 

> The primary difference is that this series defines and implements

> MmServicesTableLib in such a way that the traditional SMM drivers

> can use it as well. This is appropriate, considering that the PI

> spec has rebranded traditional SMM as one implementation of the generic

> MM framework.

> 

> Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> library class, but for all SMM flavours, not only for standalone MM.

> 

> Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> 

> Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> driver that invoke boot services are separated from the core SMM pieces.

> 

> Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> 

> Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> 

> This approach minimizes the delta, and thus the maintenance burden, between

> the traditional SMM and standalone MM drivers, while not resorting to runtime

> checks or other conditionals in the code to implement logic that should be

> decided at build time.

> 

> Note that this series only covers part of the work contributed by Jagadeesh.

> This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> code.

> 

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Hao Wu <hao.a.wu@intel.com>

> Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> Cc: Achin Gupta <Achin.Gupta@arm.com>

> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> 

> Ard Biesheuvel (5):

>   MdePkg: implement MmServicesTableLib based on traditional SMM

>   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

>   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

>   MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

>   MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> 

> Jagadeesh Ujja (1):

>   MdePkg/Include: add MmServicesTableLib header file

> 

>  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

>  .../FaultTolerantWrite.h                      |  22 ++-

>  .../FaultTolerantWriteDxe.c                   |  31 ++++

>  .../FaultTolerantWriteSmm.c                   |  54 +++----

>  .../FaultTolerantWriteSmm.inf                 |   5 +-

>  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

>  .../FaultTolerantWriteSmmDxe.c                |   1 +

>  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

>  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

>  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

>  .../UpdateWorkingBlock.c                      |  10 +-

>  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

>  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

>  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

>  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

>  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

>  .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

>  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

>  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

>  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

>  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

>  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

>  MdePkg/MdePkg.dec                             |   4 +

>  MdePkg/MdePkg.dsc                             |   1 +

>  24 files changed, 916 insertions(+), 103 deletions(-)

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

>  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

>  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

>  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> 

> --

> 2.17.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 7, 2019, 1:05 p.m. UTC | #3
On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:
>

> Ard:

>   I agree this design is good. But, I need some time to evaluate its impact on our X86 platform. Could you wait for several days?

>


Of course.

Thanks,

> > -----Original Message-----

> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > Sent: Friday, January 4, 2019 2:28 AM

> > To: edk2-devel@lists.01.org

> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney,

> > Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> > Abraham <thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> > Subject: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> >

> > This series proposed an alternative approach to the series sent out by

> > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> > special PCD, as well as some other if() conditionals.

> >

> > The primary difference is that this series defines and implements

> > MmServicesTableLib in such a way that the traditional SMM drivers

> > can use it as well. This is appropriate, considering that the PI

> > spec has rebranded traditional SMM as one implementation of the generic

> > MM framework.

> >

> > Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> > library class, but for all SMM flavours, not only for standalone MM.

> >

> > Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> >

> > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> > driver that invoke boot services are separated from the core SMM pieces.

> >

> > Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> >

> > Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> >

> > This approach minimizes the delta, and thus the maintenance burden, between

> > the traditional SMM and standalone MM drivers, while not resorting to runtime

> > checks or other conditionals in the code to implement logic that should be

> > decided at build time.

> >

> > Note that this series only covers part of the work contributed by Jagadeesh.

> > This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> > code.

> >

> > Cc: Laszlo Ersek <lersek@redhat.com>

> > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > Cc: Liming Gao <liming.gao@intel.com>

> > Cc: Jian J Wang <jian.j.wang@intel.com>

> > Cc: Hao Wu <hao.a.wu@intel.com>

> > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> > Cc: Achin Gupta <Achin.Gupta@arm.com>

> > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> >

> > Ard Biesheuvel (5):

> >   MdePkg: implement MmServicesTableLib based on traditional SMM

> >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

> >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

> >   MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

> >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> >

> > Jagadeesh Ujja (1):

> >   MdePkg/Include: add MmServicesTableLib header file

> >

> >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

> >  .../FaultTolerantWrite.h                      |  22 ++-

> >  .../FaultTolerantWriteDxe.c                   |  31 ++++

> >  .../FaultTolerantWriteSmm.c                   |  54 +++----

> >  .../FaultTolerantWriteSmm.inf                 |   5 +-

> >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

> >  .../FaultTolerantWriteSmmDxe.c                |   1 +

> >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

> >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

> >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

> >  .../UpdateWorkingBlock.c                      |  10 +-

> >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

> >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

> >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

> >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

> >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

> >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

> >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

> >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

> >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

> >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

> >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

> >  MdePkg/MdePkg.dec                             |   4 +

> >  MdePkg/MdePkg.dsc                             |   1 +

> >  24 files changed, 916 insertions(+), 103 deletions(-)

> >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

> >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

> >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

> >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

> >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

> >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

> >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

> >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

> >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> >

> > --

> > 2.17.1

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Jan. 7, 2019, 7:08 p.m. UTC | #4
On 01/07/19 14:05, Ard Biesheuvel wrote:
> On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

>>

>> Ard:

>>   I agree this design is good. But, I need some time to evaluate its impact on our X86 platform. Could you wait for several days?

>>

> 

> Of course.


I think it would be prudent of me to at least regression-test this
series. Adding it to my queue... It's likely that I won't be too quick. :/

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Jan. 9, 2019, 9:44 a.m. UTC | #5
On 01/03/19 19:28, Ard Biesheuvel wrote:
> This series proposed an alternative approach to the series sent out by

> Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> special PCD, as well as some other if() conditionals.

>

> The primary difference is that this series defines and implements

> MmServicesTableLib in such a way that the traditional SMM drivers can

> use it as well. This is appropriate, considering that the PI spec has

> rebranded traditional SMM as one implementation of the generic MM

> framework.

>

> Patch #1 is based on Jagadeesh's patch, and introduces the

> MmServicesTableLib library class, but for all SMM flavours, not only

> for standalone MM.

>

> Patch #2 implements MmServicesTableLib for traditional SMM

> implementations.

>

> Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> driver that invoke boot services are separated from the core SMM

> pieces.

>

> Patch #4 implements FaultTolerantWriteSmm for the standalone MM

> environment.

>

> Patches #5 and #6 do the same, respectively, for the variable runtime

> driver.

>

> This approach minimizes the delta, and thus the maintenance burden,

> between the traditional SMM and standalone MM drivers, while not

> resorting to runtime checks or other conditionals in the code to

> implement logic that should be decided at build time.

>

> Note that this series only covers part of the work contributed by

> Jagadeesh. This series focuses on the MdePkg and MdeModulePkg changes

> that affect shared code.

>

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Hao Wu <hao.a.wu@intel.com>

> Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> Cc: Achin Gupta <Achin.Gupta@arm.com>

> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> Cc: Sami Mujawar <Sami.Mujawar@arm.com>


I tried building this, on top of commit a53a888de8f5:

build \
  -a IA32 \
  -p OvmfPkg/OvmfPkgIa32.dsc \
  -t GCC48 \
  -b NOOPT \
  -D HTTP_BOOT_ENABLE \
  -D NETWORK_IP6_ENABLE \
  -D SECURE_BOOT_ENABLE \
  -D SMM_REQUIRE \
  -D TLS_ENABLE \
  --cmd-len=65536 \
  --hash \
  --genfds-multi-thread

but it failed with:

> OvmfPkg/OvmfPkgIa32.dsc(...): error 4000: Instance of library class [MmServicesTableLib] is not found

>         in [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf] [IA32]

>         consumed by module [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf]


You did mention earlier that adding new lib class resolutions to several
x86 DSC files would be necessary, so this is not unexpected. Can you
please insert such a patch for OvmfPkg between patches #2 and #3?

I've looked at the current OVMF DSC files, and SmmServicesTableLib is
resolved for two module types,

> [LibraryClasses.common.DXE_SMM_DRIVER]

>   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

>

> [LibraryClasses.common.SMM_CORE]

>   SmmServicesTableLib|MdeModulePkg/Library/PiSmmCoreSmmServicesTableLib/PiSmmCoreSmmServicesTableLib.inf


I assume it should be enough, for this series, to update the
DXE_SMM_DRIVER resolution only, and to leave SMM_CORE alone.

(Because, my understanding is that the current, x86 specific

  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf

module, of type SMM_CORE, will not be refactored; instead, it is
entirely supplanted -- in the affected platforms -- by the

  StandaloneMmPkg/Core/StandaloneMmCore.inf

module, which is of type MM_CORE_STANDALONE.)

But, it's still not clear to me (without trying) whether I should
resolve MmServicesTableLib  for DXE_SMM_DRIVER in addition to
SmmServicesTableLib, or in its place. I'd prefer not experimenting with
this myself; I'd just like to apply the series, and build & test it. :)

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 9, 2019, 10:28 a.m. UTC | #6
On Wed, 9 Jan 2019 at 10:44, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 01/03/19 19:28, Ard Biesheuvel wrote:

> > This series proposed an alternative approach to the series sent out by

> > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> > special PCD, as well as some other if() conditionals.

> >

> > The primary difference is that this series defines and implements

> > MmServicesTableLib in such a way that the traditional SMM drivers can

> > use it as well. This is appropriate, considering that the PI spec has

> > rebranded traditional SMM as one implementation of the generic MM

> > framework.

> >

> > Patch #1 is based on Jagadeesh's patch, and introduces the

> > MmServicesTableLib library class, but for all SMM flavours, not only

> > for standalone MM.

> >

> > Patch #2 implements MmServicesTableLib for traditional SMM

> > implementations.

> >

> > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> > driver that invoke boot services are separated from the core SMM

> > pieces.

> >

> > Patch #4 implements FaultTolerantWriteSmm for the standalone MM

> > environment.

> >

> > Patches #5 and #6 do the same, respectively, for the variable runtime

> > driver.

> >

> > This approach minimizes the delta, and thus the maintenance burden,

> > between the traditional SMM and standalone MM drivers, while not

> > resorting to runtime checks or other conditionals in the code to

> > implement logic that should be decided at build time.

> >

> > Note that this series only covers part of the work contributed by

> > Jagadeesh. This series focuses on the MdePkg and MdeModulePkg changes

> > that affect shared code.

> >

> > Cc: Laszlo Ersek <lersek@redhat.com>

> > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > Cc: Liming Gao <liming.gao@intel.com>

> > Cc: Jian J Wang <jian.j.wang@intel.com>

> > Cc: Hao Wu <hao.a.wu@intel.com>

> > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> > Cc: Achin Gupta <Achin.Gupta@arm.com>

> > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

>

> I tried building this, on top of commit a53a888de8f5:

>

> build \

>   -a IA32 \

>   -p OvmfPkg/OvmfPkgIa32.dsc \

>   -t GCC48 \

>   -b NOOPT \

>   -D HTTP_BOOT_ENABLE \

>   -D NETWORK_IP6_ENABLE \

>   -D SECURE_BOOT_ENABLE \

>   -D SMM_REQUIRE \

>   -D TLS_ENABLE \

>   --cmd-len=65536 \

>   --hash \

>   --genfds-multi-thread

>

> but it failed with:

>

> > OvmfPkg/OvmfPkgIa32.dsc(...): error 4000: Instance of library class [MmServicesTableLib] is not found

> >         in [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf] [IA32]

> >         consumed by module [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf]

>

> You did mention earlier that adding new lib class resolutions to several

> x86 DSC files would be necessary, so this is not unexpected. Can you

> please insert such a patch for OvmfPkg between patches #2 and #3?

>


Ah yes, of course.

> I've looked at the current OVMF DSC files, and SmmServicesTableLib is

> resolved for two module types,

>

> > [LibraryClasses.common.DXE_SMM_DRIVER]

> >   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> >

> > [LibraryClasses.common.SMM_CORE]

> >   SmmServicesTableLib|MdeModulePkg/Library/PiSmmCoreSmmServicesTableLib/PiSmmCoreSmmServicesTableLib.inf

>

> I assume it should be enough, for this series, to update the

> DXE_SMM_DRIVER resolution only, and to leave SMM_CORE alone.

>

> (Because, my understanding is that the current, x86 specific

>

>   MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf

>

> module, of type SMM_CORE, will not be refactored; instead, it is

> entirely supplanted -- in the affected platforms -- by the

>

>   StandaloneMmPkg/Core/StandaloneMmCore.inf

>

> module, which is of type MM_CORE_STANDALONE.)

>

> But, it's still not clear to me (without trying) whether I should

> resolve MmServicesTableLib  for DXE_SMM_DRIVER in addition to

> SmmServicesTableLib, or in its place. I'd prefer not experimenting with

> this myself; I'd just like to apply the series, and build & test it. :)

>


At the moment, you will need both resolutions for DXE_SMM_DRIVERS
globally. Based on the outcome of the review of this series, this is
something we will need to clean up going forward, but currently, even
the drivers that are updated to use MmServicesTableLib pull in
libraries that depend on SmmServicesTableLib.

This should be a rather straight-forward search/replace operation
[famous last words], and I can commit to dedicating some of my time to
getting this fixed throughout, at least to the point where modules
that consume MmServicesTableLib no longer have to supply a resolution
for SmmServicesTableLib as well.

So, I will include a patch in the next revision of the series. In the
mean time, the hunk below should suffice to complete your regression
testing.

--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -391,6 +391,7 @@
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
+  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
 !ifdef $(DEBUG_ON_SERIAL_PORT)
   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
 !else
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Jan. 9, 2019, 1:56 p.m. UTC | #7
Ard:
  Now, the impact is to update platform DSC to include MmServicesTableLib library instance. This change is acceptable for me. I suggest your create one BZ for this patch set.
  Besides, I can't apply for these patches in my machine. Could you share git branch to me? Then, I can further verify its functionality on SMM mode. 
  
Thanks
Liming
> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, January 7, 2019 9:06 PM

> To: Gao, Liming <liming.gao@intel.com>

> Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

> <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

> Sami Mujawar <Sami.Mujawar@arm.com>

> Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> 

> On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

> >

> > Ard:

> >   I agree this design is good. But, I need some time to evaluate its impact on our X86 platform. Could you wait for several days?

> >

> 

> Of course.

> 

> Thanks,

> 

> > > -----Original Message-----

> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > > Sent: Friday, January 4, 2019 2:28 AM

> > > To: edk2-devel@lists.01.org

> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;

> Kinney,

> > > Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> > > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> > > Abraham <thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> > > Subject: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> > >

> > > This series proposed an alternative approach to the series sent out by

> > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> > > special PCD, as well as some other if() conditionals.

> > >

> > > The primary difference is that this series defines and implements

> > > MmServicesTableLib in such a way that the traditional SMM drivers

> > > can use it as well. This is appropriate, considering that the PI

> > > spec has rebranded traditional SMM as one implementation of the generic

> > > MM framework.

> > >

> > > Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> > > library class, but for all SMM flavours, not only for standalone MM.

> > >

> > > Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> > >

> > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> > > driver that invoke boot services are separated from the core SMM pieces.

> > >

> > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> > >

> > > Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> > >

> > > This approach minimizes the delta, and thus the maintenance burden, between

> > > the traditional SMM and standalone MM drivers, while not resorting to runtime

> > > checks or other conditionals in the code to implement logic that should be

> > > decided at build time.

> > >

> > > Note that this series only covers part of the work contributed by Jagadeesh.

> > > This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> > > code.

> > >

> > > Cc: Laszlo Ersek <lersek@redhat.com>

> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > Cc: Liming Gao <liming.gao@intel.com>

> > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > Cc: Hao Wu <hao.a.wu@intel.com>

> > > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> > > Cc: Achin Gupta <Achin.Gupta@arm.com>

> > > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> > > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> > >

> > > Ard Biesheuvel (5):

> > >   MdePkg: implement MmServicesTableLib based on traditional SMM

> > >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

> > >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

> > >   MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

> > >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> > >

> > > Jagadeesh Ujja (1):

> > >   MdePkg/Include: add MmServicesTableLib header file

> > >

> > >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

> > >  .../FaultTolerantWrite.h                      |  22 ++-

> > >  .../FaultTolerantWriteDxe.c                   |  31 ++++

> > >  .../FaultTolerantWriteSmm.c                   |  54 +++----

> > >  .../FaultTolerantWriteSmm.inf                 |   5 +-

> > >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

> > >  .../FaultTolerantWriteSmmDxe.c                |   1 +

> > >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

> > >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

> > >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

> > >  .../UpdateWorkingBlock.c                      |  10 +-

> > >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

> > >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

> > >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

> > >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

> > >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

> > >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

> > >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

> > >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

> > >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

> > >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

> > >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

> > >  MdePkg/MdePkg.dec                             |   4 +

> > >  MdePkg/MdePkg.dsc                             |   1 +

> > >  24 files changed, 916 insertions(+), 103 deletions(-)

> > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

> > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

> > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

> > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

> > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

> > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

> > >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

> > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

> > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> > >

> > > --

> > > 2.17.1

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Jan. 9, 2019, 3:04 p.m. UTC | #8
On 01/09/19 11:28, Ard Biesheuvel wrote:
> On Wed, 9 Jan 2019 at 10:44, Laszlo Ersek <lersek@redhat.com> wrote:


>> But, it's still not clear to me (without trying) whether I should

>> resolve MmServicesTableLib  for DXE_SMM_DRIVER in addition to

>> SmmServicesTableLib, or in its place. I'd prefer not experimenting with

>> this myself; I'd just like to apply the series, and build & test it. :)

>>

> 

> At the moment, you will need both resolutions for DXE_SMM_DRIVERS

> globally. Based on the outcome of the review of this series, this is

> something we will need to clean up going forward, but currently, even

> the drivers that are updated to use MmServicesTableLib pull in

> libraries that depend on SmmServicesTableLib.

> 

> This should be a rather straight-forward search/replace operation

> [famous last words], and I can commit to dedicating some of my time to

> getting this fixed throughout, at least to the point where modules

> that consume MmServicesTableLib no longer have to supply a resolution

> for SmmServicesTableLib as well.

> 

> So, I will include a patch in the next revision of the series.


Great, thank you. This is exactly the info I needed.

> In the mean time, the hunk below should suffice to complete your regression

> testing.

> 

> --- a/OvmfPkg/OvmfPkgX64.dsc

> +++ b/OvmfPkg/OvmfPkgX64.dsc

> @@ -391,6 +391,7 @@

>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

>    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

>    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  !ifdef $(DEBUG_ON_SERIAL_PORT)

>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

>  !else

> 


I'll replicate this to the other two DSC files [*], and then retry the
tests.

[*] SMM in OVMF has a non-intuitive restriction, in relation to X64 PEI.
SMM "just works" in the IA32 and IA32X64 builds, however, in the X64
build, one has to disable S3 support on the QEMU command line, or else
we hang the boot intentionally. See commit 5133d1f1d297 ("OvmfPkg:
replace README fine print about X64 SMM S3 with PlatformPei check",
2015-11-30).

For this reason, the IA32X64 build is considered the most-featureful, if
-D SMM_REQUIRE is desired.


For those that insist on the X64 build nevertheless, OvmfPkg/README
documents the QEMU option that disables S3, on the Q35 machine type,
which is required for SMM anyway:

  -global ICH9-LPC.disable_s3=1

When using libvirt, the matching knob is:

https://libvirt.org/formatdomain.html#elementsPowerManagement

<pm>
  <suspend-to-mem enabled='no'/>
</pm>


Personally, I focus my SMM testing on IA32 and IA32X64; I almost never
test SMM in the X64 build. This is because S3 has historically proved
very effective at triggering multiprocessing bugs in core SMM code, and
in general UefiCpuPkg infrastructure. Thus, my SMM regression tests:

https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt

always include S3 suspend/resume, and that precludes the X64 build of OVMF.

... Sorry about the wall of text, I just wanted to explain why precisely
the short hunk you pasted is unhelpful in this case. Obviously, it does
answer my question, I can copy the class resolution to the other two DSC
files, and in the final OvmfPkg patch, we should update all three DSC files.

I'll be back with test results later.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 9, 2019, 3:29 p.m. UTC | #9
On Wed, 9 Jan 2019 at 14:56, Gao, Liming <liming.gao@intel.com> wrote:
>

> Ard:

>   Now, the impact is to update platform DSC to include MmServicesTableLib library instance. This change is acceptable for me. I suggest your create one BZ for this patch set.


https://bugzilla.tianocore.org/show_bug.cgi?id=1442

>   Besides, I can't apply for these patches in my machine. Could you share git branch to me? Then, I can further verify its functionality on SMM mode.

>


https://github.com/ardbiesheuvel/edk2/tree/variable-ftw-standalone-mm-conversion

Note that I included the changes to add the MmServicesTableLib
resolution to consumers of the FTW and variable drivers.

Thanks,
Ard.



> Thanks

> Liming

> > -----Original Message-----

> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > Sent: Monday, January 7, 2019 9:06 PM

> > To: Gao, Liming <liming.gao@intel.com>

> > Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

> > <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

> > <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

> > Sami Mujawar <Sami.Mujawar@arm.com>

> > Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> >

> > On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

> > >

> > > Ard:

> > >   I agree this design is good. But, I need some time to evaluate its impact on our X86 platform. Could you wait for several days?

> > >

> >

> > Of course.

> >

> > Thanks,

> >

> > > > -----Original Message-----

> > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > > > Sent: Friday, January 4, 2019 2:28 AM

> > > > To: edk2-devel@lists.01.org

> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;

> > Kinney,

> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> > > > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> > > > Abraham <thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> > > > Subject: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> > > >

> > > > This series proposed an alternative approach to the series sent out by

> > > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> > > > special PCD, as well as some other if() conditionals.

> > > >

> > > > The primary difference is that this series defines and implements

> > > > MmServicesTableLib in such a way that the traditional SMM drivers

> > > > can use it as well. This is appropriate, considering that the PI

> > > > spec has rebranded traditional SMM as one implementation of the generic

> > > > MM framework.

> > > >

> > > > Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> > > > library class, but for all SMM flavours, not only for standalone MM.

> > > >

> > > > Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> > > >

> > > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> > > > driver that invoke boot services are separated from the core SMM pieces.

> > > >

> > > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> > > >

> > > > Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> > > >

> > > > This approach minimizes the delta, and thus the maintenance burden, between

> > > > the traditional SMM and standalone MM drivers, while not resorting to runtime

> > > > checks or other conditionals in the code to implement logic that should be

> > > > decided at build time.

> > > >

> > > > Note that this series only covers part of the work contributed by Jagadeesh.

> > > > This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> > > > code.

> > > >

> > > > Cc: Laszlo Ersek <lersek@redhat.com>

> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > Cc: Hao Wu <hao.a.wu@intel.com>

> > > > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> > > > Cc: Achin Gupta <Achin.Gupta@arm.com>

> > > > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> > > > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> > > >

> > > > Ard Biesheuvel (5):

> > > >   MdePkg: implement MmServicesTableLib based on traditional SMM

> > > >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

> > > >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

> > > >   MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

> > > >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> > > >

> > > > Jagadeesh Ujja (1):

> > > >   MdePkg/Include: add MmServicesTableLib header file

> > > >

> > > >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

> > > >  .../FaultTolerantWrite.h                      |  22 ++-

> > > >  .../FaultTolerantWriteDxe.c                   |  31 ++++

> > > >  .../FaultTolerantWriteSmm.c                   |  54 +++----

> > > >  .../FaultTolerantWriteSmm.inf                 |   5 +-

> > > >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

> > > >  .../FaultTolerantWriteSmmDxe.c                |   1 +

> > > >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

> > > >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

> > > >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

> > > >  .../UpdateWorkingBlock.c                      |  10 +-

> > > >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

> > > >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

> > > >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

> > > >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

> > > >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

> > > >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

> > > >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

> > > >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

> > > >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

> > > >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

> > > >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

> > > >  MdePkg/MdePkg.dec                             |   4 +

> > > >  MdePkg/MdePkg.dsc                             |   1 +

> > > >  24 files changed, 916 insertions(+), 103 deletions(-)

> > > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

> > > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

> > > >  create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

> > > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

> > > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

> > > >  create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

> > > >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

> > > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

> > > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> > > >  create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> > > >

> > > > --

> > > > 2.17.1

> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Jan. 9, 2019, 9:46 p.m. UTC | #10
On 01/09/19 16:04, Laszlo Ersek wrote:
> On 01/09/19 11:28, Ard Biesheuvel wrote:

>> In the mean time, the hunk below should suffice to complete your

>> regression testing.


I used:

> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc

> index 3f3533e5c163..908450eda174 100644

> --- a/OvmfPkg/OvmfPkgIa32.dsc

> +++ b/OvmfPkg/OvmfPkgIa32.dsc

> @@ -386,6 +386,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

>    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

>    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  !ifdef $(DEBUG_ON_SERIAL_PORT)

>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

>  !else

> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc

> index 6c08b2728d63..14166e946a93 100644

> --- a/OvmfPkg/OvmfPkgIa32X64.dsc

> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc

> @@ -391,6 +391,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

>    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

>    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  !ifdef $(DEBUG_ON_SERIAL_PORT)

>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

>  !else

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc

> index 4072c839d73f..0cd5f76cccd9 100644

> --- a/OvmfPkg/OvmfPkgX64.dsc

> +++ b/OvmfPkg/OvmfPkgX64.dsc

> @@ -391,6 +391,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

>    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

>    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>  !ifdef $(DEBUG_ON_SERIAL_PORT)

>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

>  !else


On 01/09/19 16:04, Laszlo Ersek wrote:
> I'll be back with test results later.


Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 9, 2019, 9:56 p.m. UTC | #11
On Wed, 9 Jan 2019 at 22:46, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 01/09/19 16:04, Laszlo Ersek wrote:

> > On 01/09/19 11:28, Ard Biesheuvel wrote:

> >> In the mean time, the hunk below should suffice to complete your

> >> regression testing.

>

> I used:

>

> > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc

> > index 3f3533e5c163..908450eda174 100644

> > --- a/OvmfPkg/OvmfPkgIa32.dsc

> > +++ b/OvmfPkg/OvmfPkgIa32.dsc

> > @@ -386,6 +386,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

> >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

> >    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

> >    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> > +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> >  !ifdef $(DEBUG_ON_SERIAL_PORT)

> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

> >  !else

> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc

> > index 6c08b2728d63..14166e946a93 100644

> > --- a/OvmfPkg/OvmfPkgIa32X64.dsc

> > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc

> > @@ -391,6 +391,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

> >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

> >    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

> >    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> > +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> >  !ifdef $(DEBUG_ON_SERIAL_PORT)

> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

> >  !else

> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc

> > index 4072c839d73f..0cd5f76cccd9 100644

> > --- a/OvmfPkg/OvmfPkgX64.dsc

> > +++ b/OvmfPkg/OvmfPkgX64.dsc

> > @@ -391,6 +391,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]

> >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

> >    SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf

> >    SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf

> > +  MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> >  !ifdef $(DEBUG_ON_SERIAL_PORT)

> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

> >  !else

>

> On 01/09/19 16:04, Laszlo Ersek wrote:

> > I'll be back with test results later.

>

> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

>


Wonderful! Thanks a lot Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star Jan. 10, 2019, 8:24 a.m. UTC | #12
We'd better have a bugzilla to track this change.
And since it will require platform change in platform dsc to add the new 
library mapping, we need add notes in 
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Notes.


Thanks,
Star

On 2019/1/4 2:28, Ard Biesheuvel wrote:
> This series proposed an alternative approach to the series sent out by

> Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> special PCD, as well as some other if() conditionals.

> 

> The primary difference is that this series defines and implements

> MmServicesTableLib in such a way that the traditional SMM drivers

> can use it as well. This is appropriate, considering that the PI

> spec has rebranded traditional SMM as one implementation of the generic

> MM framework.

> 

> Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> library class, but for all SMM flavours, not only for standalone MM.

> 

> Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> 

> Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> driver that invoke boot services are separated from the core SMM pieces.

> 

> Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> 

> Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> 

> This approach minimizes the delta, and thus the maintenance burden, between

> the traditional SMM and standalone MM drivers, while not resorting to runtime

> checks or other conditionals in the code to implement logic that should be

> decided at build time.

> 

> Note that this series only covers part of the work contributed by Jagadeesh.

> This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> code.

> 

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Hao Wu <hao.a.wu@intel.com>

> Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> Cc: Achin Gupta <Achin.Gupta@arm.com>

> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> 

> Ard Biesheuvel (5):

>    MdePkg: implement MmServicesTableLib based on traditional SMM

>    MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

>    MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

>    MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

>    MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> 

> Jagadeesh Ujja (1):

>    MdePkg/Include: add MmServicesTableLib header file

> 

>   MdeModulePkg/MdeModulePkg.dsc                 |   1 +

>   .../FaultTolerantWrite.h                      |  22 ++-

>   .../FaultTolerantWriteDxe.c                   |  31 ++++

>   .../FaultTolerantWriteSmm.c                   |  54 +++----

>   .../FaultTolerantWriteSmm.inf                 |   5 +-

>   .../FaultTolerantWriteSmmCommon.h             |  31 ++++

>   .../FaultTolerantWriteSmmDxe.c                |   1 +

>   .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

>   .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

>   .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

>   .../UpdateWorkingBlock.c                      |  10 +-

>   .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

>   .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

>   .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

>   .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

>   .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

>   .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

>   .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

>   MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

>   .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

>   .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

>   .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

>   MdePkg/MdePkg.dec                             |   4 +

>   MdePkg/MdePkg.dsc                             |   1 +

>   24 files changed, 916 insertions(+), 103 deletions(-)

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

>   create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star Jan. 13, 2019, 3:42 p.m. UTC | #13
Ard,

FYI:
There is minor change overlap to VariableDxe/Smm.c between this patch 
series and the patch series at 
https://lists.01.org/pipermail/edk2-devel/2019-January/034921.html 
([PATCH 04/12]) I just sent.

After one patch series is pushed, the other patch series will need a 
simple rebase.


Thanks,
Star

On 2019/1/4 2:28, Ard Biesheuvel wrote:
> This series proposed an alternative approach to the series sent out by

> Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> special PCD, as well as some other if() conditionals.

> 

> The primary difference is that this series defines and implements

> MmServicesTableLib in such a way that the traditional SMM drivers

> can use it as well. This is appropriate, considering that the PI

> spec has rebranded traditional SMM as one implementation of the generic

> MM framework.

> 

> Patch #1 is based on Jagadeesh's patch, and introduces the MmServicesTableLib

> library class, but for all SMM flavours, not only for standalone MM.

> 

> Patch #2 implements MmServicesTableLib for traditional SMM implementations.

> 

> Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> driver that invoke boot services are separated from the core SMM pieces.

> 

> Patch #4 implements FaultTolerantWriteSmm for the standalone MM environment.

> 

> Patches #5 and #6 do the same, respectively, for the variable runtime driver.

> 

> This approach minimizes the delta, and thus the maintenance burden, between

> the traditional SMM and standalone MM drivers, while not resorting to runtime

> checks or other conditionals in the code to implement logic that should be

> decided at build time.

> 

> Note that this series only covers part of the work contributed by Jagadeesh.

> This series focuses on the MdePkg and MdeModulePkg changes that affect shared

> code.

> 

> Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Hao Wu <hao.a.wu@intel.com>

> Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> Cc: Achin Gupta <Achin.Gupta@arm.com>

> Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> 

> Ard Biesheuvel (5):

>    MdePkg: implement MmServicesTableLib based on traditional SMM

>    MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses

>    MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version

>    MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses

>    MdeModulePkg/VariableRuntimeDxe: implement standalone MM version

> 

> Jagadeesh Ujja (1):

>    MdePkg/Include: add MmServicesTableLib header file

> 

>   MdeModulePkg/MdeModulePkg.dsc                 |   1 +

>   .../FaultTolerantWrite.h                      |  22 ++-

>   .../FaultTolerantWriteDxe.c                   |  31 ++++

>   .../FaultTolerantWriteSmm.c                   |  54 +++----

>   .../FaultTolerantWriteSmm.inf                 |   5 +-

>   .../FaultTolerantWriteSmmCommon.h             |  31 ++++

>   .../FaultTolerantWriteSmmDxe.c                |   1 +

>   .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

>   .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

>   .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

>   .../UpdateWorkingBlock.c                      |  10 +-

>   .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

>   .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

>   .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

>   .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

>   .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

>   .../RuntimeDxe/VariableStandaloneMm.inf       | 135 ++++++++++++++++++

>   .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

>   MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

>   .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

>   .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

>   .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

>   MdePkg/MdePkg.dec                             |   4 +

>   MdePkg/MdePkg.dsc                             |   1 +

>   24 files changed, 916 insertions(+), 103 deletions(-)

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.c

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf

>   create mode 100644 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditionalMm.c

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

>   create mode 100644 MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

>   create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>   create mode 100644 MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Jan. 14, 2019, 2:55 a.m. UTC | #14
Ard:
  I don't find the function issue in this patch. I have no other comments for the change in MdePkg. Reviewed-by: Liming Gao <liming.gao@intel.com>. For this patch set, if you push the change, please push the patches in MdePkg first, and tell me the revision. I will update our internal platform DSC to include new MmServicesTableLib library instance. After I am done, I will let you know. Then, you can continue to push the change in MdeModulePkg. Is it OK?

  I see you will continue to look add MmStandaloneEntryPointLib and MmServiceLib for MmStandalone driver. You can create another BZ for it. I will review them once you are done. 

Thanks
Liming
>-----Original Message-----

>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>Sent: Wednesday, January 09, 2019 11:30 PM

>To: Gao, Liming <liming.gao@intel.com>

>Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm

><leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>;

>Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;

>Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta

><Achin.Gupta@arm.com>; Thomas Panakamattam Abraham

><thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

>Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable

>runtime drivers

>

>On Wed, 9 Jan 2019 at 14:56, Gao, Liming <liming.gao@intel.com> wrote:

>>

>> Ard:

>>   Now, the impact is to update platform DSC to include MmServicesTableLib

>library instance. This change is acceptable for me. I suggest your create one BZ

>for this patch set.

>

>https://bugzilla.tianocore.org/show_bug.cgi?id=1442

>

>>   Besides, I can't apply for these patches in my machine. Could you share git

>branch to me? Then, I can further verify its functionality on SMM mode.

>>

>

>https://github.com/ardbiesheuvel/edk2/tree/variable-ftw-standalone-mm-

>conversion

>

>Note that I included the changes to add the MmServicesTableLib

>resolution to consumers of the FTW and variable drivers.

>

>Thanks,

>Ard.

>

>

>

>> Thanks

>> Liming

>> > -----Original Message-----

>> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>> > Sent: Monday, January 7, 2019 9:06 PM

>> > To: Gao, Liming <liming.gao@intel.com>

>> > Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif

>Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

>> > <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,

>Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

>> > <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>;

>Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

>> > Sami Mujawar <Sami.Mujawar@arm.com>

>> > Subject: Re: [PATCH 0/6] implement standalone MM versions of the

>variable runtime drivers

>> >

>> > On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

>> > >

>> > > Ard:

>> > >   I agree this design is good. But, I need some time to evaluate its impact

>on our X86 platform. Could you wait for several days?

>> > >

>> >

>> > Of course.

>> >

>> > Thanks,

>> >

>> > > > -----Original Message-----

>> > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>> > > > Sent: Friday, January 4, 2019 2:28 AM

>> > > > To: edk2-devel@lists.01.org

>> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek

><lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;

>> > Kinney,

>> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

><liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

>> > > > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>;

>Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

>> > > > Abraham <thomas.abraham@arm.com>; Sami Mujawar

><Sami.Mujawar@arm.com>

>> > > > Subject: [PATCH 0/6] implement standalone MM versions of the

>variable runtime drivers

>> > > >

>> > > > This series proposed an alternative approach to the series sent out by

>> > > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

>> > > > special PCD, as well as some other if() conditionals.

>> > > >

>> > > > The primary difference is that this series defines and implements

>> > > > MmServicesTableLib in such a way that the traditional SMM drivers

>> > > > can use it as well. This is appropriate, considering that the PI

>> > > > spec has rebranded traditional SMM as one implementation of the

>generic

>> > > > MM framework.

>> > > >

>> > > > Patch #1 is based on Jagadeesh's patch, and introduces the

>MmServicesTableLib

>> > > > library class, but for all SMM flavours, not only for standalone MM.

>> > > >

>> > > > Patch #2 implements MmServicesTableLib for traditional SMM

>implementations.

>> > > >

>> > > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

>> > > > driver that invoke boot services are separated from the core SMM

>pieces.

>> > > >

>> > > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM

>environment.

>> > > >

>> > > > Patches #5 and #6 do the same, respectively, for the variable runtime

>driver.

>> > > >

>> > > > This approach minimizes the delta, and thus the maintenance burden,

>between

>> > > > the traditional SMM and standalone MM drivers, while not resorting to

>runtime

>> > > > checks or other conditionals in the code to implement logic that should

>be

>> > > > decided at build time.

>> > > >

>> > > > Note that this series only covers part of the work contributed by

>Jagadeesh.

>> > > > This series focuses on the MdePkg and MdeModulePkg changes that

>affect shared

>> > > > code.

>> > > >

>> > > > Cc: Laszlo Ersek <lersek@redhat.com>

>> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

>> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

>> > > > Cc: Liming Gao <liming.gao@intel.com>

>> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

>> > > > Cc: Hao Wu <hao.a.wu@intel.com>

>> > > > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

>> > > > Cc: Achin Gupta <Achin.Gupta@arm.com>

>> > > > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

>> > > > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

>> > > >

>> > > > Ard Biesheuvel (5):

>> > > >   MdePkg: implement MmServicesTableLib based on traditional SMM

>> > > >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service

>accesses

>> > > >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM

>version

>> > > >   MdeModulePkg/VariableRuntimeDxe: factor out boot service

>accesses

>> > > >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM

>version

>> > > >

>> > > > Jagadeesh Ujja (1):

>> > > >   MdePkg/Include: add MmServicesTableLib header file

>> > > >

>> > > >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

>> > > >  .../FaultTolerantWrite.h                      |  22 ++-

>> > > >  .../FaultTolerantWriteDxe.c                   |  31 ++++

>> > > >  .../FaultTolerantWriteSmm.c                   |  54 +++----

>> > > >  .../FaultTolerantWriteSmm.inf                 |   5 +-

>> > > >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

>> > > >  .../FaultTolerantWriteSmmDxe.c                |   1 +

>> > > >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

>> > > >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

>> > > >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

>> > > >  .../UpdateWorkingBlock.c                      |  10 +-

>> > > >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

>> > > >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

>> > > >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

>> > > >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

>> > > >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

>> > > >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135

>++++++++++++++++++

>> > > >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

>> > > >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

>> > > >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

>> > > >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

>> > > >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

>> > > >  MdePkg/MdePkg.dec                             |   4 +

>> > > >  MdePkg/MdePkg.dsc                             |   1 +

>> > > >  24 files changed, 916 insertions(+), 103 deletions(-)

>> > > >  create mode 100644

>MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

>oneMm.c

>> > > >  create mode 100644

>MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

>oneMm.inf

>> > > >  create mode 100644

>MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditio

>nalMm.c

>> > > >  create mode 100644

>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

>> > > >  create mode 100644

>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

>> > > >  create mode 100644

>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

>> > > >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

>> > > >  create mode 100644

>MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

>> > > >  create mode 100644

>MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

>> > > >  create mode 100644

>MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

>> > > >

>> > > > --

>> > > > 2.17.1

>> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 14, 2019, 8:26 a.m. UTC | #15
On Mon, 14 Jan 2019 at 03:55, Gao, Liming <liming.gao@intel.com> wrote:
>

> Ard:

>   I don't find the function issue in this patch. I have no other comments for the change in MdePkg. Reviewed-by: Liming Gao <liming.gao@intel.com>. For this patch set, if you push the change, please push the patches in MdePkg first, and tell me the revision. I will update our internal platform DSC to include new MmServicesTableLib library instance. After I am done, I will let you know. Then, you can continue to push the change in MdeModulePkg. Is it OK?

>


Yes, that is fine. I will need to respin the remaining patches anyway.

I have pushed the following patches

b94aecb4ec94 MdePkg/Include: add MmServicesTableLib header file
17f5fd9291e0 MdePkg: implement MmServicesTableLib based on traditional SMM

with Star's and Jian's feedback addressed, and your R-b's added.


>   I see you will continue to look add MmStandaloneEntryPointLib and MmServiceLib for MmStandalone driver. You can create another BZ for it. I will review them once you are done.

>


OK.

> >-----Original Message-----

> >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> >Sent: Wednesday, January 09, 2019 11:30 PM

> >To: Gao, Liming <liming.gao@intel.com>

> >Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm

> ><leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>;

> >Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;

> >Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta

> ><Achin.Gupta@arm.com>; Thomas Panakamattam Abraham

> ><thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> >Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable

> >runtime drivers

> >

> >On Wed, 9 Jan 2019 at 14:56, Gao, Liming <liming.gao@intel.com> wrote:

> >>

> >> Ard:

> >>   Now, the impact is to update platform DSC to include MmServicesTableLib

> >library instance. This change is acceptable for me. I suggest your create one BZ

> >for this patch set.

> >

> >https://bugzilla.tianocore.org/show_bug.cgi?id=1442

> >

> >>   Besides, I can't apply for these patches in my machine. Could you share git

> >branch to me? Then, I can further verify its functionality on SMM mode.

> >>

> >

> >https://github.com/ardbiesheuvel/edk2/tree/variable-ftw-standalone-mm-

> >conversion

> >

> >Note that I included the changes to add the MmServicesTableLib

> >resolution to consumers of the FTW and variable drivers.

> >

> >Thanks,

> >Ard.

> >

> >

> >

> >> Thanks

> >> Liming

> >> > -----Original Message-----

> >> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> >> > Sent: Monday, January 7, 2019 9:06 PM

> >> > To: Gao, Liming <liming.gao@intel.com>

> >> > Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif

> >Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

> >> > <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,

> >Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

> >> > <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>;

> >Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

> >> > Sami Mujawar <Sami.Mujawar@arm.com>

> >> > Subject: Re: [PATCH 0/6] implement standalone MM versions of the

> >variable runtime drivers

> >> >

> >> > On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

> >> > >

> >> > > Ard:

> >> > >   I agree this design is good. But, I need some time to evaluate its impact

> >on our X86 platform. Could you wait for several days?

> >> > >

> >> >

> >> > Of course.

> >> >

> >> > Thanks,

> >> >

> >> > > > -----Original Message-----

> >> > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> >> > > > Sent: Friday, January 4, 2019 2:28 AM

> >> > > > To: edk2-devel@lists.01.org

> >> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek

> ><lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;

> >> > Kinney,

> >> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> ><liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> >> > > > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>;

> >Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> >> > > > Abraham <thomas.abraham@arm.com>; Sami Mujawar

> ><Sami.Mujawar@arm.com>

> >> > > > Subject: [PATCH 0/6] implement standalone MM versions of the

> >variable runtime drivers

> >> > > >

> >> > > > This series proposed an alternative approach to the series sent out by

> >> > > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> >> > > > special PCD, as well as some other if() conditionals.

> >> > > >

> >> > > > The primary difference is that this series defines and implements

> >> > > > MmServicesTableLib in such a way that the traditional SMM drivers

> >> > > > can use it as well. This is appropriate, considering that the PI

> >> > > > spec has rebranded traditional SMM as one implementation of the

> >generic

> >> > > > MM framework.

> >> > > >

> >> > > > Patch #1 is based on Jagadeesh's patch, and introduces the

> >MmServicesTableLib

> >> > > > library class, but for all SMM flavours, not only for standalone MM.

> >> > > >

> >> > > > Patch #2 implements MmServicesTableLib for traditional SMM

> >implementations.

> >> > > >

> >> > > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> >> > > > driver that invoke boot services are separated from the core SMM

> >pieces.

> >> > > >

> >> > > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM

> >environment.

> >> > > >

> >> > > > Patches #5 and #6 do the same, respectively, for the variable runtime

> >driver.

> >> > > >

> >> > > > This approach minimizes the delta, and thus the maintenance burden,

> >between

> >> > > > the traditional SMM and standalone MM drivers, while not resorting to

> >runtime

> >> > > > checks or other conditionals in the code to implement logic that should

> >be

> >> > > > decided at build time.

> >> > > >

> >> > > > Note that this series only covers part of the work contributed by

> >Jagadeesh.

> >> > > > This series focuses on the MdePkg and MdeModulePkg changes that

> >affect shared

> >> > > > code.

> >> > > >

> >> > > > Cc: Laszlo Ersek <lersek@redhat.com>

> >> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> >> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> >> > > > Cc: Liming Gao <liming.gao@intel.com>

> >> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> >> > > > Cc: Hao Wu <hao.a.wu@intel.com>

> >> > > > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> >> > > > Cc: Achin Gupta <Achin.Gupta@arm.com>

> >> > > > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> >> > > > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> >> > > >

> >> > > > Ard Biesheuvel (5):

> >> > > >   MdePkg: implement MmServicesTableLib based on traditional SMM

> >> > > >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service

> >accesses

> >> > > >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM

> >version

> >> > > >   MdeModulePkg/VariableRuntimeDxe: factor out boot service

> >accesses

> >> > > >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM

> >version

> >> > > >

> >> > > > Jagadeesh Ujja (1):

> >> > > >   MdePkg/Include: add MmServicesTableLib header file

> >> > > >

> >> > > >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

> >> > > >  .../FaultTolerantWrite.h                      |  22 ++-

> >> > > >  .../FaultTolerantWriteDxe.c                   |  31 ++++

> >> > > >  .../FaultTolerantWriteSmm.c                   |  54 +++----

> >> > > >  .../FaultTolerantWriteSmm.inf                 |   5 +-

> >> > > >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

> >> > > >  .../FaultTolerantWriteSmmDxe.c                |   1 +

> >> > > >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

> >> > > >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

> >> > > >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

> >> > > >  .../UpdateWorkingBlock.c                      |  10 +-

> >> > > >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

> >> > > >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

> >> > > >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

> >> > > >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

> >> > > >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

> >> > > >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135

> >++++++++++++++++++

> >> > > >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

> >> > > >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

> >> > > >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

> >> > > >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

> >> > > >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

> >> > > >  MdePkg/MdePkg.dec                             |   4 +

> >> > > >  MdePkg/MdePkg.dsc                             |   1 +

> >> > > >  24 files changed, 916 insertions(+), 103 deletions(-)

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

> >oneMm.c

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

> >oneMm.inf

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditio

> >nalMm.c

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

> >> > > >  create mode 100644

> >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

> >> > > >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

> >> > > >  create mode 100644

> >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

> >> > > >  create mode 100644

> >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> >> > > >  create mode 100644

> >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> >> > > >

> >> > > > --

> >> > > > 2.17.1

> >> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming Jan. 14, 2019, 3:33 p.m. UTC | #16
Ard:
  Got it. I will update our internal platform dsc to include new MmServicesTableLib library.

  Besides, will you send the patch to update platform DSC files in edk2-platforms? If yes, please update DSCs in https://github.com/tianocore/edk2-platforms/tree/devel-MinPlatform. They both depend on edk2 master. 

Thanks
Liming
> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, January 14, 2019 4:27 PM

> To: Gao, Liming <liming.gao@intel.com>

> Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

> <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

> Sami Mujawar <Sami.Mujawar@arm.com>

> Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers

> 

> On Mon, 14 Jan 2019 at 03:55, Gao, Liming <liming.gao@intel.com> wrote:

> >

> > Ard:

> >   I don't find the function issue in this patch. I have no other comments for the change in MdePkg. Reviewed-by: Liming Gao

> <liming.gao@intel.com>. For this patch set, if you push the change, please push the patches in MdePkg first, and tell me the revision. I

> will update our internal platform DSC to include new MmServicesTableLib library instance. After I am done, I will let you know. Then,

> you can continue to push the change in MdeModulePkg. Is it OK?

> >

> 

> Yes, that is fine. I will need to respin the remaining patches anyway.

> 

> I have pushed the following patches

> 

> b94aecb4ec94 MdePkg/Include: add MmServicesTableLib header file

> 17f5fd9291e0 MdePkg: implement MmServicesTableLib based on traditional SMM

> 

> with Star's and Jian's feedback addressed, and your R-b's added.

> 

> 

> >   I see you will continue to look add MmStandaloneEntryPointLib and MmServiceLib for MmStandalone driver. You can create

> another BZ for it. I will review them once you are done.

> >

> 

> OK.

> 

> > >-----Original Message-----

> > >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > >Sent: Wednesday, January 09, 2019 11:30 PM

> > >To: Gao, Liming <liming.gao@intel.com>

> > >Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm

> > ><leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>;

> > >Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;

> > >Jagadeesh Ujja <jagadeesh.ujja@arm.com>; Achin Gupta

> > ><Achin.Gupta@arm.com>; Thomas Panakamattam Abraham

> > ><thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>

> > >Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable

> > >runtime drivers

> > >

> > >On Wed, 9 Jan 2019 at 14:56, Gao, Liming <liming.gao@intel.com> wrote:

> > >>

> > >> Ard:

> > >>   Now, the impact is to update platform DSC to include MmServicesTableLib

> > >library instance. This change is acceptable for me. I suggest your create one BZ

> > >for this patch set.

> > >

> > >https://bugzilla.tianocore.org/show_bug.cgi?id=1442

> > >

> > >>   Besides, I can't apply for these patches in my machine. Could you share git

> > >branch to me? Then, I can further verify its functionality on SMM mode.

> > >>

> > >

> > >https://github.com/ardbiesheuvel/edk2/tree/variable-ftw-standalone-mm-

> > >conversion

> > >

> > >Note that I included the changes to add the MmServicesTableLib

> > >resolution to consumers of the FTW and variable drivers.

> > >

> > >Thanks,

> > >Ard.

> > >

> > >

> > >

> > >> Thanks

> > >> Liming

> > >> > -----Original Message-----

> > >> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > >> > Sent: Monday, January 7, 2019 9:06 PM

> > >> > To: Gao, Liming <liming.gao@intel.com>

> > >> > Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Leif

> > >Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D

> > >> > <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,

> > >Hao A <hao.a.wu@intel.com>; Jagadeesh Ujja

> > >> > <jagadeesh.ujja@arm.com>; Achin Gupta <Achin.Gupta@arm.com>;

> > >Thomas Panakamattam Abraham <thomas.abraham@arm.com>;

> > >> > Sami Mujawar <Sami.Mujawar@arm.com>

> > >> > Subject: Re: [PATCH 0/6] implement standalone MM versions of the

> > >variable runtime drivers

> > >> >

> > >> > On Mon, 7 Jan 2019 at 13:44, Gao, Liming <liming.gao@intel.com> wrote:

> > >> > >

> > >> > > Ard:

> > >> > >   I agree this design is good. But, I need some time to evaluate its impact

> > >on our X86 platform. Could you wait for several days?

> > >> > >

> > >> >

> > >> > Of course.

> > >> >

> > >> > Thanks,

> > >> >

> > >> > > > -----Original Message-----

> > >> > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> > >> > > > Sent: Friday, January 4, 2019 2:28 AM

> > >> > > > To: edk2-devel@lists.01.org

> > >> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek

> > ><lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;

> > >> > Kinney,

> > >> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > ><liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A

> > >> > > > <hao.a.wu@intel.com>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>;

> > >Achin Gupta <Achin.Gupta@arm.com>; Thomas Panakamattam

> > >> > > > Abraham <thomas.abraham@arm.com>; Sami Mujawar

> > ><Sami.Mujawar@arm.com>

> > >> > > > Subject: [PATCH 0/6] implement standalone MM versions of the

> > >variable runtime drivers

> > >> > > >

> > >> > > > This series proposed an alternative approach to the series sent out by

> > >> > > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the

> > >> > > > special PCD, as well as some other if() conditionals.

> > >> > > >

> > >> > > > The primary difference is that this series defines and implements

> > >> > > > MmServicesTableLib in such a way that the traditional SMM drivers

> > >> > > > can use it as well. This is appropriate, considering that the PI

> > >> > > > spec has rebranded traditional SMM as one implementation of the

> > >generic

> > >> > > > MM framework.

> > >> > > >

> > >> > > > Patch #1 is based on Jagadeesh's patch, and introduces the

> > >MmServicesTableLib

> > >> > > > library class, but for all SMM flavours, not only for standalone MM.

> > >> > > >

> > >> > > > Patch #2 implements MmServicesTableLib for traditional SMM

> > >implementations.

> > >> > > >

> > >> > > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM

> > >> > > > driver that invoke boot services are separated from the core SMM

> > >pieces.

> > >> > > >

> > >> > > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM

> > >environment.

> > >> > > >

> > >> > > > Patches #5 and #6 do the same, respectively, for the variable runtime

> > >driver.

> > >> > > >

> > >> > > > This approach minimizes the delta, and thus the maintenance burden,

> > >between

> > >> > > > the traditional SMM and standalone MM drivers, while not resorting to

> > >runtime

> > >> > > > checks or other conditionals in the code to implement logic that should

> > >be

> > >> > > > decided at build time.

> > >> > > >

> > >> > > > Note that this series only covers part of the work contributed by

> > >Jagadeesh.

> > >> > > > This series focuses on the MdePkg and MdeModulePkg changes that

> > >affect shared

> > >> > > > code.

> > >> > > >

> > >> > > > Cc: Laszlo Ersek <lersek@redhat.com>

> > >> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > >> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > >> > > > Cc: Liming Gao <liming.gao@intel.com>

> > >> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > >> > > > Cc: Hao Wu <hao.a.wu@intel.com>

> > >> > > > Cc: Jagadeesh Ujja <jagadeesh.ujja@arm.com>

> > >> > > > Cc: Achin Gupta <Achin.Gupta@arm.com>

> > >> > > > Cc: Thomas Panakamattam Abraham <thomas.abraham@arm.com>

> > >> > > > Cc: Sami Mujawar <Sami.Mujawar@arm.com>

> > >> > > >

> > >> > > > Ard Biesheuvel (5):

> > >> > > >   MdePkg: implement MmServicesTableLib based on traditional SMM

> > >> > > >   MdeModulePkg/FaultTolerantWriteDxe: factor out boot service

> > >accesses

> > >> > > >   MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM

> > >version

> > >> > > >   MdeModulePkg/VariableRuntimeDxe: factor out boot service

> > >accesses

> > >> > > >   MdeModulePkg/VariableRuntimeDxe: implement standalone MM

> > >version

> > >> > > >

> > >> > > > Jagadeesh Ujja (1):

> > >> > > >   MdePkg/Include: add MmServicesTableLib header file

> > >> > > >

> > >> > > >  MdeModulePkg/MdeModulePkg.dsc                 |   1 +

> > >> > > >  .../FaultTolerantWrite.h                      |  22 ++-

> > >> > > >  .../FaultTolerantWriteDxe.c                   |  31 ++++

> > >> > > >  .../FaultTolerantWriteSmm.c                   |  54 +++----

> > >> > > >  .../FaultTolerantWriteSmm.inf                 |   5 +-

> > >> > > >  .../FaultTolerantWriteSmmCommon.h             |  31 ++++

> > >> > > >  .../FaultTolerantWriteSmmDxe.c                |   1 +

> > >> > > >  .../FaultTolerantWriteStandaloneMm.c          |  70 +++++++++

> > >> > > >  .../FaultTolerantWriteStandaloneMm.inf        |  90 ++++++++++++

> > >> > > >  .../FaultTolerantWriteTraditionalMm.c         |  94 ++++++++++++

> > >> > > >  .../UpdateWorkingBlock.c                      |  10 +-

> > >> > > >  .../Variable/RuntimeDxe/TcgMorLockSmm.c       |  18 +--

> > >> > > >  .../Universal/Variable/RuntimeDxe/Variable.h  |  50 +++++++

> > >> > > >  .../Variable/RuntimeDxe/VariableSmm.c         |  59 +++-----

> > >> > > >  .../Variable/RuntimeDxe/VariableSmm.inf       |   5 +-

> > >> > > >  .../RuntimeDxe/VariableStandaloneMm.c         |  69 +++++++++

> > >> > > >  .../RuntimeDxe/VariableStandaloneMm.inf       | 135

> > >++++++++++++++++++

> > >> > > >  .../RuntimeDxe/VariableTraditionalMm.c        | 114 +++++++++++++++

> > >> > > >  MdePkg/Include/Library/MmServicesTableLib.h   |  25 ++++

> > >> > > >  .../MmServicesTableLib/MmServicesTableLib.c   |  63 ++++++++

> > >> > > >  .../MmServicesTableLib/MmServicesTableLib.inf |  45 ++++++

> > >> > > >  .../MmServicesTableLib/MmServicesTableLib.uni |  22 +++

> > >> > > >  MdePkg/MdePkg.dec                             |   4 +

> > >> > > >  MdePkg/MdePkg.dsc                             |   1 +

> > >> > > >  24 files changed, 916 insertions(+), 103 deletions(-)

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

> > >oneMm.c

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandal

> > >oneMm.inf

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteTraditio

> > >nalMm.c

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.c

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf

> > >> > > >  create mode 100644

> > >MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c

> > >> > > >  create mode 100644 MdePkg/Include/Library/MmServicesTableLib.h

> > >> > > >  create mode 100644

> > >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c

> > >> > > >  create mode 100644

> > >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf

> > >> > > >  create mode 100644

> > >MdePkg/Library/MmServicesTableLib/MmServicesTableLib.uni

> > >> > > >

> > >> > > > --

> > >> > > > 2.17.1

> > >> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel