[PATCH] lmb: Correctly unmap memory after notifications

[Ilias Apalodimas]

Hi Heinrich

On Tue, 29 Oct 2024 at 10:00, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Heinrich
>
> On Mon, 28 Oct 2024 at 08:37, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 10/24/24 12:46, Ilias Apalodimas wrote:
> > > We never unmap the memory used to update the EFI memory map after
> > > notifications
> > >
> > > Fixes: commit 2f6191526a13 ("lmb: notify of any changes to the LMB memory map")
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > ---
> > >   lib/lmb.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > index 890e2cbfdf6b..38c6e1d5ba8d 100644
> > > --- a/lib/lmb.c
> > > +++ b/lib/lmb.c
> > > @@ -65,6 +65,7 @@ static int __maybe_unused lmb_map_update_notify(phys_addr_t addr,
> > >                       status & ~EFI_ERROR_MASK);
> > >               return -1;
> > >       }
> > > +     unmap_sysmem((void *)(uintptr_t)efi_addr);
> >
> > If PCI memory was ever mapped via pci_map_physmem(), I don't think that
> > we want to unmap it here.
>
> Ok, so looking at it again, not calling unmap_sysmem() here won't
> break anything, so I guess we can drop this patch.

Looking at it again, for special sandbox tags, we need to correctly
decrease the refcnt. So why do you think this isnt needed here?

Thanks
/Ilias
>
> >
> > Why do we map the memory in this notification function first hand?
>
> Because the memory was added as such in efi_allocate_pages(). So for
> sandbox we need to find the correct virtual memory that was added.
>
> > Can't
> > we use len = 0 when calling map_sysmem()?
>
> We are using len = 0. But I am not following up on how could this
> help. All you will get is a warning for sanbox if a pci device mapped
> more that 0.
>
> Thanks
> /Ilias
>
> >
> > I guess we have to critically review all map_sysmem() calls.
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > >       return 0;
> > >   }
> >