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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
