On Thu, Dec 01, 2022 at 08:43:56AM -0500, Stefan Berger wrote: > On 12/1/22 00:19, Glenn Washburn wrote: > > On Wed, 30 Nov 2022 17:42:40 -0500 > > Stefan Berger <stef...@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. > > How is this target enabled? Just configuring on an i386 host doesn't > seem to do it and I don't see an obvious configure option to build for > it, either.
./configure --target=i386 --with-platform=ieee1275 ... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
