Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Simon Glass <sjg@chromium.org>]

Hi Ilias,

On Mon, 27 Sept 2021 at 02:52, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > > > - remove unnecessary const qualifier from smbios_string()
> > > > > - create non-const version of next_header()
> > > > >
> > > > >  include/efi_loader.h          |   2 +
> > > > >  include/efi_tcg2.h            |  15 ++++
> > > > >  include/smbios.h              |  17 +++-
> > > > >  lib/efi_loader/Kconfig        |   1 +
> > > > >  lib/efi_loader/efi_boottime.c |   2 +
> > > > >  lib/efi_loader/efi_smbios.c   |   2 -
> > > > >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> > > > >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> > > > >  8 files changed, 261 insertions(+), 14 deletions(-)
> > > >
> > > > Where are the tests for this new code, please?
> > >
> > > We've mentioned this in the past.  The sandbox TPM is very limited wrt
> > > tpm testing for the EFI TCG protocol.
> >
> > So let's add some more features? If it helps, think of the sandbox TPM
> > as test code, not an emulator. It is a very simple kind of emulator to
> > allow tests to work.
>
> The amount of features needed to test EFI TCG are not minimal.  Since I'll
> upstream the mmio tpm anyway,  we'll just test TCG there.  If someone wants
> to go ahead and make the sandbox TPM a TIS compliant device that covers the
> requirements of the EFI TCG,  I am fine using it.

Do you know how many features? There's 250 LOC in this patch.

>
> >
> > > I did send TPM MMIO patches a while back [1].  This would allow us to
> > > test everything under QEMU,  but you asked for *another* device to be
> > > part of the API I posted (apart from the MMIO).  I've found some time
> >
> > Yes that is because if you just add a new protocol you have not made
> > anything better, just added one more way of doing things.
>
> Our perspective of 'better' seems to be different.
>
> I added a TIS API for any driver to use.  I actually did 2 iterations of
> the driver.  The first one was replicating all the code and you said 'why
> are we replicating code',  which was done already in a bunch of drivers
> already...
> Then I added an API and a driver using it but you wanted to convert more
> *existing* drivers to the API before merging it. But the fact is that if
> anyone wants to add a new driver he has to code  ~900 lines instead of the
> ~150 needed with the API in place,  not to mention the duplication of bugs
> all over the place....

It would be like adding a new filesystem in U-Boot with its own new
framework for filesystems. It creates technical debt and we don't know
if anyone will actually use it.

https://xkcd.com/927/

I think your API is a great idea but we need some effort to migrate to
it, to avoid the problem above. After all, who else is going to do it?

Regards,
Simon