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

[Daniel Kiper]

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,
  - 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.

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