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