This series imports code from the edk2-platforms project related to
changing the password of the TPM2 platform hierarchy and uses it to
disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
aspects of the following bugs:

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

There's no doubt that my struggles with the build system and handling
of dependencies are visible in this series. Quite a few aspects of
getting things right are more or less guesswork and I am often not sure
what the correct way of doing things are. If 'you' wanted to fix
things up and repost it, please go ahead...

Stefan

Stefan Berger (7):
  SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
    edk2-platforms
  SecruityPkg/TPM: Disable dependency on MinPlatformPkg
  SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
  SecurityPkg/TPM: Disable a Pcd
  SecurityPkg/TPM: Add a NULL implementation of
    PeiDxeTpmPlatformHierarchyLib
  OVMF: Reference new classes in the build system for compilation
  OVMF: Disable the TPM2 platform hierarchy

 OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
 .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
 .../PlatformBootManagerLib.inf                |   1 +
 .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
 .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
 OvmfPkg/OvmfPkgX64.dsc                        |   3 +
 .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
 .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
 .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
 .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
 13 files changed, 432 insertions(+)
 create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.c
 create mode 100644 SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf

-- 
2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78817): https://edk2.groups.io/g/devel/message/78817
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Stefan
It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.

A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.

For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.
Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Sent: Friday, August 6, 2021 11:33 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> 
> This series imports code from the edk2-platforms project related to
> changing the password of the TPM2 platform hierarchy and uses it to
> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> aspects of the following bugs:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> 
> There's no doubt that my struggles with the build system and handling
> of dependencies are visible in this series. Quite a few aspects of
> getting things right are more or less guesswork and I am often not sure
> what the correct way of doing things are. If 'you' wanted to fix
> things up and repost it, please go ahead...
> 
> Stefan
> 
> Stefan Berger (7):
>   SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>     edk2-platforms
>   SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>   SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>   SecurityPkg/TPM: Disable a Pcd
>   SecurityPkg/TPM: Add a NULL implementation of
>     PeiDxeTpmPlatformHierarchyLib
>   OVMF: Reference new classes in the build system for compilation
>   OVMF: Disable the TPM2 platform hierarchy
> 
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>  .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>  .../PlatformBootManagerLib.inf                |   1 +
>  .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>  .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>  .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>  .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>  .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>  .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>  13 files changed, 432 insertions(+)
>  create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> chyLib.c
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> chyLib.inf
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> erarchyLib.c
>  create mode 100644
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> erarchyLib.inf
> 
> --
> 2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78843): https://edk2.groups.io/g/devel/message/78843
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hey Jiewen,
Hey Stefan,

On 07.08.21 08:00, Yao, Jiewen wrote:
> Hi Stefan
> It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.
>
> A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.
>
> For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.
> Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

Maybe there should be stricter policies about what code goes where, and 
duplication should be outright banned? My PE/COFF loader proposal merged 
at least 4 copies of the Authenticode hashing and 3 copies of a record 
construction algorithm together. There have been other code 
centralisations, such as certificate iteration. If code exists twice, it 
needs to me maintained twice, and in reality this does not happen.

Regarding what goes where, e.g. FatPkg is in edk2, but the new EXT4 
driver was recommended to be located in edk2-platforms. I know there are 
a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and 
as someone with no expertise around TPM or the edk2-platforms design, I 
would never even have thought to look in edk2-platforms for such code. 
And especially for library code edk2-platforms seems to be an 
unfortunate location, as edk2 packages can never depend on them.

> Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.

I think for this initial inspection this was actually a good thing. The 
separation and the patches after 1 signal to me that there have been no 
functional changes to the library at all. Probably the best idea is to 
just move it to SecurityPkg entirely, including the PCDs, and update 
MinPlatformPkg to consume it from there instead?

Best regards,
Marvin

>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Sent: Friday, August 6, 2021 11:33 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
>> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
>>
>> This series imports code from the edk2-platforms project related to
>> changing the password of the TPM2 platform hierarchy and uses it to
>> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
>> aspects of the following bugs:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
>>
>> There's no doubt that my struggles with the build system and handling
>> of dependencies are visible in this series. Quite a few aspects of
>> getting things right are more or less guesswork and I am often not sure
>> what the correct way of doing things are. If 'you' wanted to fix
>> things up and repost it, please go ahead...
>>
>> Stefan
>>
>> Stefan Berger (7):
>>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>>      edk2-platforms
>>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>>    SecurityPkg/TPM: Disable a Pcd
>>    SecurityPkg/TPM: Add a NULL implementation of
>>      PeiDxeTpmPlatformHierarchyLib
>>    OVMF: Reference new classes in the build system for compilation
>>    OVMF: Disable the TPM2 platform hierarchy
>>
>>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>>   .../PlatformBootManagerLib.inf                |   1 +
>>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>>   13 files changed, 432 insertions(+)
>>   create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.inf
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.inf
>>
>> --
>> 2.31.1
>
>
> 
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78844): https://edk2.groups.io/g/devel/message/78844
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Marvin
There are many factors impacting where to put the feature code - EDKII or EDKII-platform/Feature.
I can list some of them:
If it is a basic feature v.s. an advanced feature.
If it is a mature production feature v.s. immature feature.
If it is a generic core feature v.s. a platform specific feature.
......


The position can be changed. For example, we can put a feature to EDKII-platform at first. Later, if everyone wants this feature in a production, we can move it to EDKII.

For Ext4, I think it had better be in EDKII-platform at first, because it seems to be an advanced feature. I am not sure how many production need it. And I am not sure the maturity level.

For TPM Hierarchy, we put it to EDKII-platform at first, because we think the platform hierarchy management is platform specific behavior. Each platform may choose their own way to do that. But if all people think the example in EDKII-platform can be used in their production. I think it is OK to move to SecurityPkg.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin
> Häuser
> Sent: Saturday, August 7, 2021 6:22 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Stefan Berger
> <stefanb@linux.vnet.ibm.com>
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com
> Subject: Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform
> hierarchy
> 
> Hey Jiewen,
> Hey Stefan,
> 
> On 07.08.21 08:00, Yao, Jiewen wrote:
> > Hi Stefan
> > It seems this patch series is not a production fix. It is more like a prototype, my
> personally feeling.
> >
> > A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the
> code. Please remove the unnecessary code directly without // or /**/ in C, and #
> in INF.
> >
> > For patch 1, if you want to move the code to SecurityPkg, that is fine. Please
> move the whole driver their and you should not remove and code by comment.
> Please fix the issue to make it pass build, instead of commenting the code like
> work-around.
> > Otherwise, you may copy the module to OvmfPkg. Then you can modify it as
> you need.
> 
> Maybe there should be stricter policies about what code goes where, and
> duplication should be outright banned? My PE/COFF loader proposal merged
> at least 4 copies of the Authenticode hashing and 3 copies of a record
> construction algorithm together. There have been other code
> centralisations, such as certificate iteration. If code exists twice, it
> needs to me maintained twice, and in reality this does not happen.
> 
> Regarding what goes where, e.g. FatPkg is in edk2, but the new EXT4
> driver was recommended to be located in edk2-platforms. I know there are
> a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and
> as someone with no expertise around TPM or the edk2-platforms design, I
> would never even have thought to look in edk2-platforms for such code.
> And especially for library code edk2-platforms seems to be an
> unfortunate location, as edk2 packages can never depend on them.
> 
> > Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1,
> then add fix in 2, 3, 4.
> 
> I think for this initial inspection this was actually a good thing. The
> separation and the patches after 1 signal to me that there have been no
> functional changes to the library at all. Probably the best idea is to
> just move it to SecurityPkg entirely, including the PCDs, and update
> MinPlatformPkg to consume it from there instead?
> 
> Best regards,
> Marvin
> 
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Sent: Friday, August 6, 2021 11:33 PM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> >> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> >>
> >> This series imports code from the edk2-platforms project related to
> >> changing the password of the TPM2 platform hierarchy and uses it to
> >> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> >> aspects of the following bugs:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> >>
> >> There's no doubt that my struggles with the build system and handling
> >> of dependencies are visible in this series. Quite a few aspects of
> >> getting things right are more or less guesswork and I am often not sure
> >> what the correct way of doing things are. If 'you' wanted to fix
> >> things up and repost it, please go ahead...
> >>
> >> Stefan
> >>
> >> Stefan Berger (7):
> >>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
> >>      edk2-platforms
> >>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
> >>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
> >>    SecurityPkg/TPM: Disable a Pcd
> >>    SecurityPkg/TPM: Add a NULL implementation of
> >>      PeiDxeTpmPlatformHierarchyLib
> >>    OVMF: Reference new classes in the build system for compilation
> >>    OVMF: Disable the TPM2 platform hierarchy
> >>
> >>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
> >>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
> >>   .../PlatformBootManagerLib.inf                |   1 +
> >>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
> >>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
> >>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
> >>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
> >>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
> >>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
> >>   13 files changed, 432 insertions(+)
> >>   create mode 100644
> SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.inf
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.inf
> >>
> >> --
> >> 2.31.1
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78864): https://edk2.groups.io/g/devel/message/78864
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 8/7/21 2:00 AM, Yao, Jiewen wrote:
> Hi Stefan
> It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.]

Yes, it's an RFC patch and with the struggles as pointed out below. I 
don't know how this project would go about importing code from 
'edk2-platforms' for example, what deviations from the original code in 
edk2-platforms you are willing to accept and what not, such as removing 
dependencies from the original code and commenting out code that doesn't 
work anymore due the removal. What I imported looked like it had a 
dependency on 'MinPlatformPkg/MinPlatformPkg.dec'. Do we need to import 
this package first or rather not?


>
> A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.
>
> For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.


What is the 'whole driver'? Can you be a bit more specific what all 
files are comprising the 'whole driver.'


> Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.

What is the 'module' versus the 'whole driver?'

And in the end, how would 'you' go about this in this case?


>
> Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Sent: Friday, August 6, 2021 11:33 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
>> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
>>
>> This series imports code from the edk2-platforms project related to
>> changing the password of the TPM2 platform hierarchy and uses it to
>> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
>> aspects of the following bugs:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
>>
>> There's no doubt that my struggles with the build system and handling
>> of dependencies are visible in this series. Quite a few aspects of
>> getting things right are more or less guesswork and I am often not sure
>> what the correct way of doing things are. If 'you' wanted to fix
>> things up and repost it, please go ahead...
>>
>> Stefan
>>
>> Stefan Berger (7):
>>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>>      edk2-platforms
>>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>>    SecurityPkg/TPM: Disable a Pcd
>>    SecurityPkg/TPM: Add a NULL implementation of
>>      PeiDxeTpmPlatformHierarchyLib
>>    OVMF: Reference new classes in the build system for compilation
>>    OVMF: Disable the TPM2 platform hierarchy
>>
>>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
>>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
>>   .../PlatformBootManagerLib.inf                |   1 +
>>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
>>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
>>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
>>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
>>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
>>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
>>   13 files changed, 432 insertions(+)
>>   create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.inf
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.c
>>   create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.inf
>>
>> --
>> 2.31.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78845): https://edk2.groups.io/g/devel/message/78845
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thanks for the clarification.
Comments below:


> -----Original Message-----
> From: Stefan Berger <stefanb@linux.ibm.com>
> Sent: Saturday, August 7, 2021 11:04 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Stefan Berger
> <stefanb@linux.vnet.ibm.com>; devel@edk2.groups.io
> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> dick_wilkins@phoenix.com
> Subject: Re: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> 
> 
> On 8/7/21 2:00 AM, Yao, Jiewen wrote:
> > Hi Stefan
> > It seems this patch series is not a production fix. It is more like a prototype, my
> personally feeling.]
> 
> Yes, it's an RFC patch and with the struggles as pointed out below. I
> don't know how this project would go about importing code from
> 'edk2-platforms' for example, what deviations from the original code in
> edk2-platforms you are willing to accept and what not, such as removing
> dependencies from the original code and commenting out code that doesn't
> work anymore due the removal. What I imported looked like it had a
> dependency on 'MinPlatformPkg/MinPlatformPkg.dec'. Do we need to import
> this package first or rather not?

[Jiewen]
If you want to move a feature from EDKII-platform to EDKII, you must remove the dependency on EDKII-platform.
And there are lots of ways on how to remove dependency.

For example, you can define the PCD in EDKII directly. As such, you don’t need depend upon MinPlatformPkg.



> >
> > A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the
> code. Please remove the unnecessary code directly without // or /**/ in C, and #
> in INF.
> >
> > For patch 1, if you want to move the code to SecurityPkg, that is fine. Please
> move the whole driver their and you should not remove and code by comment.
> Please fix the issue to make it pass build, instead of commenting the code like
> work-around.
> 
> 
> What is the 'whole driver'? Can you be a bit more specific what all
> files are comprising the 'whole driver.'
> 
> 
> > Otherwise, you may copy the module to OvmfPkg. Then you can modify it as
> you need.
> 
> What is the 'module' versus the 'whole driver?'

[Jiewen] Sorry, I don’t mean to confuse you.
Usually, Driver means a UEFI/PI driver. Module means a driver or a library.
But a code may be built as a driver or a library, some people treat them as same thing. 
So they are same thing in this context.

> 
> And in the end, how would 'you' go about this in this case?

[Jiewen] I have two options in my mind.
Option 1: We move the Tpm2PlatformHierachy support from EDKII-platform to EDKII/SecurityPkg.
The move action can take 2 steps:
A) To 'create' the same Tpm2PlatformHierachy modules in SecurityPkg.
B) To 'remove' the Tpm2PlatformHierachy modules in EDKII-platform.

The modules refer to Tcg2PlatformPei/ Tcg2PlatformDxe / PeiDxeTpmPlatformHierarchyLib in
https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/MinPlatformPkg/Tcg

'Create' means you don’t need use 'move/modify' approach to introduce a broken patch with MinPlatformPkg then fix it in the second patch. You just need create one workable patch.

However, since this impacts the EDKII core component, we need more time to review.


Option 2: We create the Tpm2PlatformHiearchy support in OvmfPkg directly.
This only takes 1 step:
A) To 'create' the Tpm2PlatformHierachy modules in OvmfPkg.
You cannot touch Tpm2PlatformHierachy modules in EDKII-platform.

This only impacts OvmfPkg. So you can make necessary change for virtual platform without considering the requirement in physical platform.

Thank you
Yao Jiewen

> 
> 
> >
> > Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1,
> then add fix in 2, 3, 4.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Sent: Friday, August 6, 2021 11:33 PM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
> >> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
> >>
> >> This series imports code from the edk2-platforms project related to
> >> changing the password of the TPM2 platform hierarchy and uses it to
> >> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
> >> aspects of the following bugs:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
> >>
> >> There's no doubt that my struggles with the build system and handling
> >> of dependencies are visible in this series. Quite a few aspects of
> >> getting things right are more or less guesswork and I am often not sure
> >> what the correct way of doing things are. If 'you' wanted to fix
> >> things up and repost it, please go ahead...
> >>
> >> Stefan
> >>
> >> Stefan Berger (7):
> >>    SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
> >>      edk2-platforms
> >>    SecruityPkg/TPM: Disable dependency on MinPlatformPkg
> >>    SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
> >>    SecurityPkg/TPM: Disable a Pcd
> >>    SecurityPkg/TPM: Add a NULL implementation of
> >>      PeiDxeTpmPlatformHierarchyLib
> >>    OVMF: Reference new classes in the build system for compilation
> >>    OVMF: Disable the TPM2 platform hierarchy
> >>
> >>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |   3 +
> >>   .../PlatformBootManagerLib/BdsPlatform.c      |   6 +
> >>   .../PlatformBootManagerLib.inf                |   1 +
> >>   .../PlatformBootManagerLibBhyve/BdsPlatform.c |   6 +
> >>   .../PlatformBootManagerLibGrub/BdsPlatform.c  |   6 +
> >>   OvmfPkg/OvmfPkgIa32.dsc                       |   3 +
> >>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
> >>   OvmfPkg/OvmfPkgX64.dsc                        |   3 +
> >>   .../Include/Library/TpmPlatformHierarchyLib.h |  27 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           | 266 ++++++++++++++++++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  46 +++
> >>   .../PeiDxeTpmPlatformHierarchyLib.c           |  23 ++
> >>   .../PeiDxeTpmPlatformHierarchyLib.inf         |  39 +++
> >>   13 files changed, 432 insertions(+)
> >>   create mode 100644
> SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
> >> chyLib.inf
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.c
> >>   create mode 100644
> >>
> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
> >> erarchyLib.inf
> >>
> >> --
> >> 2.31.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78865): https://edk2.groups.io/g/devel/message/78865
Mute This Topic: https://groups.io/mt/84712022/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-