Re: [PATCH v2 3/6] ieee1275: support runtime memory claiming

[Daniel Kiper]

Adding Zhang...

On Tue, Dec 13, 2022 at 02:10:01PM -0500, Stefan Berger wrote:
> On 12/13/22 11:14, Daniel Kiper wrote:
> > On Thu, Dec 01, 2022 at 04:11:58PM -0500, Stefan Berger wrote:
> > > From: Daniel Axtens <dja@axtens.net>
> > >
> > > On powerpc-ieee1275, we are running out of memory trying to verify
> > > anything. This is because:
> > >
> > >   - we have to load an entire file into memory to verify it. This is
> > >     difficult to change with appended signatures.
> > >   - We only have 32MB of heap.
> > >   - Distro kernels are now often around 30MB.
> > >
>
> > > +
> > > +  if (flags & GRUB_MM_ADD_REGION_CONSECUTIVE)
> > > +    {
> > > +      /* first try rounding up hard for the sake of speed */
> > > +      total = grub_max (ALIGN_UP(size, 1024 * 1024) + 1024 * 1024, 32 * 
> > > 1024 * 1024);
> >
> > s/ALIGN_UP(/ALIGN_UP (/
> >
> > > +      total = grub_min (free_memory - RUNTIME_MIN_SPACE, total);
> > > +
> > > +      grub_dprintf ("ieee1275", "looking for %x bytes of memory (%x 
> > > requested)\n", total, size);
> > > +
> > > +      grub_machine_mmap_iterate (region_claim, &total);
> > > +      grub_dprintf ("ieee1275", "get memory from fw %s\n", total == 0 ? 
> > > "succeeded" : "failed");
> > > +
> > > +      /* fallback: requested size + padding for a grub_mm_header_t and 
> > > region */
> >
> > This...
>
> With 'this' you mean the comment above or which exact part?

Yes, exactly...

> > > +      if (total != 0)
> > > +        {
> > > +          total = size + 48;
> >
> > ... and this should be dropped. I think this patch set, [1], [2], solves
>
> You mean adding 48 to it can be dropped?

Yep... Sorry for being imprecise...

> > this kinds of problems in general. Please take a look...
>
> I looked at them and I don't know how they are related.

If I do not miss anything your patch set does not take into account size
needed for *grub_mm_region_t and *grub_mm_header structs. They are
needed for mm mgmt. So, grub_mm_add_region_fn() can be called twice to
fulfill request or memory addition may fail. The "mm: Preallocate some
space when adding new regions" patch extends the region size further to
not call firmware too often to fulfill small allocations. Due to that
I think your series should be merged into master together with Zhang one.
I hope he will repost updated mm series soon.

> > > +          total = grub_min (free_memory - RUNTIME_MIN_SPACE, total);
> > > +
> > > +          grub_dprintf ("ieee1275", "fallback for %x bytes of memory (%x 
> > > requested)\n", total, size);
> > > +
> > > +          grub_machine_mmap_iterate (region_claim, &total);
> > > +          grub_dprintf ("ieee1275", "fallback from fw %s\n", total == 0 
> > > ? "succeeded" : "failed");
> > > +        }
> > > +    }
> > > +  else
> > > +    {
> > > +      /* provide padding for a grub_mm_header_t and region */
> > > +      total = size + 48;
> >
> > Ditto... And then probably total can be dropped too...
>
> Don't we need a parameter to grub_machine_mmap_iterate (region_claim, &total) 
> ?

I think you can use size variable instead of total then.

Daniel