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

[Glenn Washburn]

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.

Glenn

> 
> >>
> >>     stefan
> >>
> >>>
> >>> If something is not clear please drop me a line.
> >>>
> >>> Last but not least, sorry for huge delay. I hope I will be able to
> >>> review much faster next version of this patch set.
> >>>
> >>> Daniel
> >>>
> >>> [1]
> >>> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Coding-style
> > 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel