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

[Daniel Kiper]

Adding Vladimir...

On Wed, Nov 30, 2022 at 02:47:41PM -0500, 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.

Cool! Thanks!

> >    - 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
> }

I think it would be better if you fix Makefile to not install non-existing
modules than use trick mentioned above. And it seems to me we should have
similar case somewhere to steal idea from. Or maybe Vladimir will have an
idea how to easily/nicely fix this issue. Vladimir?

Daniel