Re: [PATCH v6 00/10] ppc64: Restrict memory allocations for kernel and initrd

From: Daniel Kiper <dkiper@net-space.pl>
To: Stefan Berger <stefanb@linux.ibm.com>
Cc: The development of GNU GRUB <grub-devel@gnu.org>,
	hbathini@linux.ibm.com, pavrampu@in.ibm.com, mpe@ellerman.id.au,
	cpscherr@us.ibm.com, mahesh@linux.ibm.com,
	sourabhjain@linux.ibm.com, avnish.chouhan@ibm.com
Subject: Re: [PATCH v6 00/10] ppc64: Restrict memory allocations for kernel and initrd
Date: Fri, 24 Nov 2023 22:31:00 +0100	[thread overview]
Message-ID: <20231124213100.kgalwoybiagst6zq@tomti.i.net-space.pl> (raw)
In-Reply-To: <4afcfa7c-02b3-4970-9dfa-e8e4bae8fc37@linux.ibm.com>

On Fri, Nov 24, 2023 at 02:10:05PM -0500, Stefan Berger wrote:
> On 11/23/23 11:38, Daniel Kiper wrote:
> > On Mon, Nov 20, 2023 at 09:49:57AM -0500, Stefan Berger wrote:
> > > This series of patches converts the PowerPC ieee1275 memory allocator
> > > for kernel and initrd to use the (PowerPC) regions_claim memory allocator
> > > that takes into account memory regions that are not allowed to be used,
> > > such as the gap between 640MB and 768MB as well as memory regions beyond
> > > an address in case an fadump is present. Otherwise those two could be
> > > loaded into restricted memory regions and either cause a crash or
> > > corruption of the fadump.
> > >
> > > I adjusted the kernel and initrd load in loader/powerpc/ieee1275 to use the
> > > new memory allocator only on PowerVM and PowerKVM since this code is shared
> > > with other platforms, such as old PowerMACs.
> >
> > Sadly your patch set breaks at least i386/ieee1275 build...
> >
> >    ./configure --target=i386 --with-platform=ieee1275 ...
> >    make
> >
> >    kern/ieee1275/cmain.c: In function ‘grub_ieee1275_find_options’:
> >    kern/ieee1275/cmain.c:199:31: error: ‘GRUB_IEEE1275_FLAG_POWER_KVM’ undeclared (first use in this function)
> >           grub_ieee1275_set_flag (GRUB_IEEE1275_FLAG_POWER_KVM);
> >                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > ... or...
> >
> >    kern/ieee1275/init.c:322:1: error: no previous prototype for ‘grub_regions_claim’ [-Werror=missing-prototypes]
> >      322 | grub_regions_claim (grub_uint64_t addr, grub_uint64_t len,
> >          | ^~~~~~~~~~~~~~~~~~
> >    kern/ieee1275/init.c: In function ‘grub_regions_claim’:
> >    kern/ieee1275/init.c:510:10: error: invalid use of undefined type ‘struct regions_claim_request’
> >      510 |   if (rcr->align)
> >          |          ^~
> >    In file included from ../include/grub/dl.h:29,
> >                     from kern/ieee1275/init.c:23:
> >    kern/ieee1275/init.c:512:53: error: invalid use of undefined type ‘struct regions_claim_request’
> >      512 |       grub_uint64_t align_addr = ALIGN_UP (addr, rcr->align);
> >          |                                                     ^~
> >    ../include/grub/misc.h:31:37: note: in definition of macro ‘ALIGN_UP’
> >       31 |         (((addr) + (typeof (addr)) (align) - 1) & ~((typeof (addr)) (align) - 1))
> >          |                                     ^~~~~
> >    ...
> >
> > Please fix these issues...
>
> This required two fixes:
> - move include/grub/powerpc/ieee1275/alloc.h to
> include/grub/ieee1275/alloc.h
> - move the 2 new #defines for POWER_VM and KVM out of #ifdef __powerpc__

I think we need some more #ifdef __powerpc__ instead, e.g. around
grub_ieee1275_set_flag(GRUB_IEEE1275_FLAG_POWER_KVM).

>  Can I keep your Reviewed-by tags?

If changes are not that big and you provide git range-diff then yes.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel