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

[Zhang Boyang]

Hi,

On 2022/12/14 22:37, Daniel Kiper wrote:
> 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.

I also think 48 can be removed, if 48 means 
sizeof(grub_mm_region)+sizeof(grub_mm_header). With my patchset, the 
"size" passed to grub_mm_add_region_fn() will include these overhead and 
there is no need to add them in arch specific code.

Best Regards,
Zhang Boyang

> > > > +          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