RE: [PATCH] EFI: adjust cfg file buffer freeing

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Jan Beulich wrote:
> On 22.04.2026 13:59, Andrew Cooper wrote:
> > On 22/04/2026 12:51 pm, Jan Beulich wrote:
> >> The boot services FreePages() needs passing the size. Since we allocated
> >> one more byte to put a trailing nul there, we also need to bump the size
> >> passed there. Make a small helper function to centralize this.
> >>
> >> Note that there's no permanent memory leak because of the oversight: The
> >> allocation is done using EfiLoaderData, and all memory of that type is
> >> later reclaimed anyway.
> >
> > This depends on -mapbs at a minimum.

This memory is allocated allocated using EfiLoaderData and 'map_bs' is for
EfiBootServicesCode and Data, so at least this case does not apply to it:

        case EfiBootServicesCode:
        case EfiBootServicesData:
            if ( map_bs )
            {
                type = E820_RESERVED;
                break;
            }

> But that's affecting only EfiBootServices{Code,Data}, isn't it?

As allocated using EfiLoaderData, after ExitBootServices, it will show up
as EfiLoaderData, and will be processed when iterating over the memory
areas during boot.

On x86, Xen uses efi_arch_process_memory_map() for this, which handles it
like EfiConventionalMemory, where, if it has desc->Attribute & EFI_MEMORY_WB,
(which it should be as it is regular RAM) it is set to type E820_RAM by Xen.

Unless merged with adjacent areas it would show in the E820 map as RAM.
AFAICS, it would be reclaimed by Xen/x86 this way. On ARM, EfiLoaderData
is added using meminfo_add_bank(), so it would be added as a memory bank.

> >> Fixes: df75f77092c1 ("EFI: avoid OOB config file reads")
> >> Reported-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> This is an alternative proposal to
> >> https://lists.xen.org/archives/html/xen-devel/2026-04/msg01044.html.
> >
> > One thing this patch does not do is fix the boundary passed to
> > efi_arch_flush_dcache_area().
> 
> Deliberately so, and I commented to this effect in reply to Bernhard's
> patch. I do think ...
> 
> > I find it hard to believe that cache maintenance is actually needed, but
> > given it is present in the code currently it needs to stay correct.
> >
> > Or, if it's not needed, it should be dropped in a separate patch.
> 
> ... this is the way to go, but it'll need input from Arm folks (as
> requested there).

If in doubt, you may consult the gitweb search (see below) for the function
and take the safe route of not removing it.

Flushing the dcache may also be needed on CPUs with a split D/I-cache and
when using special code or HW on the SoC or board(e.g. a DMA engine) or
something else external to the dcache to access the loaded data.

Ordering efi_arch_flush_dcache_area() before re-dirtying the dcache looks
counter-intuitive which is why I updated the order to match the logical flow
of flushing the dcache after the last change.

Per the git history (see below), it was needed for copy_from_paddr() on ARM.

You might skip fixing up the cache flushing order. It was just a "while at it"
to be safe.

As we don't use copy_from_paddr() or similar to read or copy the config. 
However,
if copy_from_paddr() would be used for the config, at this point, the dcache
flush would be a factor to be done right (according to the git history).

Moving the two calls to FreePages() for cfg is a good cleanup for me, so:

Reviewed-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>

Bernhard

PS: Gitweb finds this commit for adding the function:

https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=0d6a3c755374f04f6dd25373da28291a8f35bede

efi: introduce efi_arch_flush_dcache_area

Objects loaded by FileHandle->Read need to be flushed from dcache,
otherwise copy_from_paddr will read stale data when copying the kernel,
causing a failure to boot.

Introduce efi_arch_flush_dcache_area and call it from read_file.

This commit introduces no functional changes on x86.

Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>