Re: [edk2-devel] [PATCH v4 1/6] OvmfPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms

["Marvin Häuser" <mhaeuser@posteo.de>]

Good day Stefan,

Do you think you could split the first patch into a 1:1 initial import 
and have a modifications commit separately?

One of my big issues with EDK II is duplicated code. I look at it, I 
don't understand why it is duplicated. I look at the differences, I 
don't understand why they are there. I dig deeper into the matter (e.g. 
git blame), and I realise there is no reason but someone was taking a 
copy+paste route, and future contributors did not know or care about the 
respective other piece of code. This series standalone is basically that 
out-of-the-box, with no future changes needed. I'll suggest once more to 
enforce a code duplication ban, both within edk2, and between edk2 and 
edk2-platforms.

It would of course be great if you submitted a follow-up series to drop 
the old library from edk2-platforms. If you plan not to, and all 
maintainers agree to merge this without such a series being submitted, 
please CC me so I know when this is merged and can propose such a patch 
set myself.

Best regards,
Marvin

On 13/08/2021 21:02, Stefan Berger wrote:
>
> On 8/13/21 2:47 PM, Sean Brogan wrote:
>> Thanks for the link as i missed that message.
>>
>> To me this just points out more problems with how OVMF is being 
>> managed in the edk2 project and the uselessness of edk2 platforms as 
>> anything more than just a dumping ground repo to hold sample code.  
>> But that is a problem larger than this patchset.
>>
>> I guess if you are going doing option 2 can we rename the library 
>> interface you are defining in OvmfPkg so it doesn't conflict with the 
>> existing one in edk2-platforms/minplatform. That would mean change:
>
>
> I have now created v5 here with the latest code appearing in 
> SecurityPkg again: 
> https://github.com/stefanberger/edk2/commits/stefanberger/ovmf_disable_platform_hierarchy.v5
>
> I can probably post that pretty quickly but I'll be out for a while. 
> If it's urgent, someone else can pick it it up from there. I tested it 
> on QEMU for x86 and aarch64 and test-compiled on various platforms 
> that I touched (some didn't compile for me before the changes).
>
> What I wasn't sure about is whether edk2-platforms is a 'holding area' 
> for code to be imported ideally 1:1 into edk2. So I ended up making 
> those changes already in v1 to cut out a dependency. If what I have in 
> v5 (or also v4) is sufficient for general consumption, then let's put 
> it into SecurityPkg.
>
>
>    Stefan
>
>
>>
>> * name in OvmfPkg.dec file
>> * header file in OvmfPkg/Include/Library
>> * all references in DSC file for mapping an instance
>> * all references in your INFs for dependency
>>
>> Thanks
>> Sean
>>
>>
>>
>>
>>
>>
>> On 8/12/2021 3:19 PM, Stefan Berger wrote:
>>>
>>> On 8/12/21 4:59 PM, Sean Brogan wrote:
>>>> This seems like a bad place for a general purpose lib that many 
>>>> other platforms may take a dependency on.
>>>>
>>>> In v1 this was SecurityPkg.  OvmfPkg is a platform package and 
>>>> therefore not a good place to define broad interfaces.
>>>>
>>>> What caused this to move here?
>>>
>>>
>>> Option 2 from this message: 
>>> https://listman.redhat.com/archives/edk2-devel-archive/2021-August/msg00398.html 
>>>
>>>
>>>    Stefan
>>>
>>>
>>>>
>>>> Thanks
>>>> Sean
>>>>
>>>>
>
>
> 
>
>