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

[Daniel Kiper]

On Wed, Nov 30, 2022 at 04:24:59PM -0500, 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);

Good point! I think we should use "#if defined(__powerpc__)" if it is
powerpc-specific code.

Daniel