[PATCH] lmb: Correctly unmap memory after notifications

[Heinrich Schuchardt]

On 10/29/24 09:48, Ilias Apalodimas wrote:
> 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


Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

>>
>>>
>>> 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;
>>>>    }
>>>