grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Berger
Subject: Re: [PATCH 0/6] Dynamic allocation of memory regions and IBM vTPM v2
Date: Thu, 1 Dec 2022 08:43:56 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1



On 12/1/22 00:19, Glenn Washburn wrote:
On Wed, 30 Nov 2022 17:42:40 -0500
Stefan Berger <stefanb@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.

   Stefan




reply via email to

[Prev in Thread] Current Thread [Next in Thread]