Re: [PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4862 bytes --]

On Sun, Jun 23, 2024 at 03:52:12PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 21 Jun 2024 at 16:12, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 01:38:07PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Fri, 21 Jun 2024 at 13:19, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Jun 21, 2024 at 11:55:42AM -0600, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Fri, 21 Jun 2024 at 10:05, Tom Rini <trini@konsulko.com> wrote:
> > > > [snip]
> > > > > > Yes, I very much do not like guessing about 3 numbers instead of
> > > > > > guessing about 1 number and using the standard mechanism we already
> > > > > > have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
> > > > > > enforce size limits on U-Boot itself.
> > > > >
> > > > > If it were that easy I would have sent a patch :-)
> > > > >
> > > > > Here is the map for this board:
> > > > >
> > > > > ImagePos    Offset      Size  Name
> > > > > 00000000  00000000  00800000  rom
> > > > > ff800000   ff800000  00001000  intel-descriptor
> > > > > ff801000   ff801000  001ff000  intel-me
> > > > > ffef0000   ffef0000  000999f0  u-boot-with-ucode-ptr
> > > > > fff899f0   fff899f0  00005554  u-boot-dtb-with-ucode
> > > > > fff8ef50   fff8ef50  00000000  u-boot-ucode
> > > > > fff8ef50   fff8ef50  00000571  fdtmap
> > > > > fff90000   fff90000  00010000  intel-vga
> > > > > fffa0000   fffa0000  0002fc94  intel-mrc
> > > > > fffcfc94   fffcfc94  00000000  private-files
> > > > > fffff800   fffff800  00000070  x86-start16
> > > > > fffffff0   fffffff0  00000005  x86-reset16
> > > > > fffffff8   fffffff8  00000008  image-header
> > > > >
> > > > > What limit should I set on what?
> > > >
> > > > Is this a trick question?
> > > > $ printf %d\\n $(( 0xfff90000 - 0xffef0000))
> > > > 655360
> > > >
> > > > Of course since we're less than that today, you can reduce it by
> > > > whatever other magic numbers I'm not seeing but are part of your assumed
> > > > sizes.
> > >
> > > That limit is on u-boot-nodtb.bin. Even with a size (for that file) of
> > > 634816 it doesn't fit. I need to calculate a size based on the size of
> > > the dtb and the microcode...which of course can change.
> >
> > Yes, and you're able to assume some size for them, which is what you
> > put in the dts file?
> 
> I just put in a limit for the blobs, whose sizes are known.
> 
> >
> > > > > - the U-Boot is the thing you are wanting to limit
> > > > > - the dtb has microcode added
> > > > > - the ucode is empty in this case
> > > > > - the fdtmap is variable in size
> > > > >
> > > > > So this all seems a bit backwards. The actual limit is that
> > > > > (u-boot-with-ucode-ptr + u-boot-dtb-with-ucode + u-boot-ucode +
> > > > > fdtmap) fits in the space available. Note that some boards don't have
> > > > > intel-vga or intel-mrc.
> > > > >
> > > > > With the other patch I sent I can have a sensible limit for all x86 boards.
> > >
> > > Did you miss the comments above?
> >
> > No, I saw them. They're similar constraints to other systems.
> > >
> > > >
> > > > And you can set the same sensible limit with the existing mechanism with
> > > > the bonus of it not making x86 different from the rest?
> > >
> > > I understand that it is possible to set a limit for u-boot-nodtb.bin
> > > but that is not accurate nor sufficient in the presence of blobs. The
> > > solution belongs in Binman.
> >
> > Your series puts reasonable estimates on the size of the blobs and then
> > will give a failure such as:
> > binman: Node '/binman/rom/intel-vga': Offset 0xfff90000 (4294508544)
> > overlaps with previous entry '/binman/rom/fdtmap' ending at 0xfff902e1
> > (4294509281)
> > Which is not as nice as (I just threw in a limit):
> > u-boot-nodtb.bin exceeds file size limit:
> >   limit:  0x927c0 bytes
> >   actual: 0x9a810 bytes
> >   excess: 0x8050 bytes
> > make[1]: *** [/home/trini/work/u-boot/u-boot/Makefile:1359: u-boot-nodtb.bin] Error 1
> >
> > And tells us how much we need to get back size wise. Aside from when
> > using the actual blobs (and in which case a real error will be shown
> > when trying to use them), it's always about making an estimate on the
> > part of the system that we control.
> 
> Thanks for digging into this.
> 
> I wasn't aware that the limit was just an estimate. Since it produces
> a build error it seems to be enforced as a hard requirement. But I can
> set a limit and we'll see how things go.
> 
> I still like the binman block-size thing though. The error message
> binman provides actually tells you what to do (reduce the space of the
> things that are overflowing into intel-vga). So I'd like to add that
> too.

I think this is a reasonable compromise, thank you.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]