Re: [PATCH 0/6] Dynamic allocation of memory regions and IBM vTPM v2

[Daniel Kiper]

On Thu, Dec 01, 2022 at 08:43:56AM -0500, Stefan Berger wrote:
> On 12/1/22 00:19, Glenn Washburn wrote:
> > On Wed, 30 Nov 2022 17:42:40 -0500
> > Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > On 11/30/22 16:24, Stefan Berger wrote:
> > > > On 11/30/22 14:47, Stefan Berger wrote:
> > > > > On 11/24/22 12:56, Daniel Kiper wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Adding Sudhakar and Glenn...
> > > > > >
> > > > > > On Thu, Aug 11, 2022 at 02:40:58PM -0300, Diego Domingos wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > This is an addition to the series sent from Daniel Axtens
> > > > > > > (https://lists.gnu.org/archive/html/grub-devel/2022-04/msg00064.html).
> > > > > > >
> > > > > > > Patch 'ieee1275: request memory with
> > > > > > > ibm,client-architecture-support' implements vectors 1-4 of
> > > > > > > client-architecture-support negotiation However, during some
> > > > > > > tests, we found this can be a problem if:
> > > > > > >
> > > > > > > - we have more than 64 CPUs
> > > > > > > - Hardware Management Console (HMC) is configured to minimum of
> > > > > > > CPUs >64 (for example, min of 200 CPUs)
> > > > > > > - Grub needs to request memory.
> > > > > > >
> > > > > > > If vector 5 is not implemented, Power Hypervisor will consider
> > > > > > > the default value for vector 5 and 64 will bet set as the
> > > > > > > maximum number of CPUs supported by the OS, causing the machine
> > > > > > > to fail to init. Today we support 256 CPUs (max) on Power, so we
> > > > > > > need to implement vector 5 and set the MAX CPUs bits to this
> > > > > > > value.
> > > > > > >
> > > > > > > The patches 11-15 aren't merged to the grub tree yet, so I'm
> > > > > > > sending those patches again together with my patch to implement
> > > > > > > vector 5 on top of them.
> > > > > > >
> > > > > > > The patches 11-15 contains the following:
> > > > > > >
> > > > > > > Daniel Axtens (4):
> > > > > > >     ieee1275: request memory with ibm,client-architecture-support
> > > > > > >     ieee1275: drop len -= 1 quirk in heap_init
> > > > > > >     ieee1275: support runtime memory claiming
> > > > > > >     [RFC] Add memtool module with memory allocation stress-test
> > > > > > >
> > > > > > > Stefan Berger (1):
> > > > > > >     ibmvtpm: Add support for trusted boot using a vTPM 2.0
> > > > > >
> > > > > > I went through all patches and cannot see major problems with
> > > > > > them. Though there are a lot of minor things which have to be
> > > > > > fixed. Sadly due to number of them I cannot simply ignore that.
> > > > > >
> > > > > > Here is the list of the issues:
> > > > > >     - functions calls/sizeof(): e.g. "grub_printf()" should be
> > > > > > replaced with "grub_printf ()", add space before "(", in the
> > > > > > code; though I am OK with the former in comments and commit
> > > > > > messages,
> > > > > >     - casts: e.g. "*(grub_uint32_t *)data" should be replaced with
> > > > > > "*(grub_uint32_t *) data", add space between ")" and "data",
> > > > > >     - s/__attribute__((packed))/GRUB_PACKED/
> > > > > >     - if you use grub_err_t type please test for GRUB_ERR_NONE
> > > > > > instead of !err or err; please do not use plain numbers, e.g. 0
> > > > > > to substitute GRUB_ERR_NONE,
> > > > > >     - if you test pointers for NULL please test using NULL
> > > > > > constant instead of e.g. !ptr
> > > > > >     - if you use a value often please define constant for it; good
> > > > > > candidate for such change is at least 0x30000000 in the patch #3;
> > > > > > if constant definition is an overkill please comment what given
> > > > > > numbers/strings mean or at least where they come from,
> > > > > >     - please do not use "//" for comments,
> > > > > >     - I am OK with lines a bit longer than 80; so, please do not
> > > > > > wrap lines too early,
> > > > >
> > > > > This is a bit vague but I think I addressed them now.
> > > > >
> > > > > >     - year in the copyright should be 2022.
> > > > > >
> > > > > > The GRUB coding style is described here [1] and you can find good
> > > > > > example of coding style in the grub-core/kern/efi/sb.c file.
> > > > > >
> > > > > > Please take into account latest comments from Daniel A. and Glenn
> > > > > > too.
> > > > >
> > > > > I don't know how to support the memtool without --enable-mm-debug
> > > > > at the same time since the module seems to be missing then but the
> > > > > build system still expects it on 'make install'. Unless there's an
> > > > > existing example of how to do it I would not post with this patch.
> > > > >
> > > > > I can get it to create an empty module with this trick here but
> > > > > don't know whether this helps the cause.
> > > > >
> > > > > GRUB_MOD_FINI (memtools)
> > > > > {
> > > > > #ifdef MM_DEBUG
> > > > >     grub_unregister_command (cmd_lsmem);
> > > > >     grub_unregister_command (cmd_lsfreemem);
> > > > >     grub_unregister_command (cmd_sba);
> > > > > #else
> > > > >     (void) grub_unregister_command;
> > > > > #endif
> > > > > }
> > > > >
> > > > >
> > > > In 1/6 we have this here. Is this sufficiently gating the usage of
> > > > the code or do we need to use '#if defined(__powerpc__)' to only
> > > > compile code newly added powerpc-specific code  used due to this
> > > > flag being set?
> > > >
> > > > +
> > > > +      if (grub_strncmp (tmp, "IBM,", 4) == 0)
> > > > +       grub_ieee1275_set_flag
> > > > (GRUB_IEEE1275_FLAG_CAN_TRY_CAS_FOR_MORE_MEMORY);
> > >
> > > And yet another question: Is __i386__ actually using
> > > grub-core/kern/ieee1275/init.c ? I don't see it compiling this file
> > > but there's a #ifdef __i386__ in this file.
> >
> > Yes, there is a i386-ieee1275 target. It builds and the tests run
> > successfully, iirc.
>
> How is this target enabled? Just configuring on an i386 host doesn't
> seem to do it and I don't see an obvious configure option to build for
> it, either.

./configure --target=i386 --with-platform=ieee1275 ...

Daniel