[PATCH] lmb: Correctly unmap memory after notifications
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@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);
return 0; }
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@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.
Why do we map the memory in this notification function first hand? Can't we use len = 0 when calling map_sysmem()?
I guess we have to critically review all map_sysmem() calls.
Best regards
Heinrich
return 0; }
Hi Heinrich
On Mon, 28 Oct 2024 at 08:37, Heinrich Schuchardt xypron.glpk@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@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.
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;}
Hi Heinrich
On Tue, 29 Oct 2024 at 10:00, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Heinrich
On Mon, 28 Oct 2024 at 08:37, Heinrich Schuchardt xypron.glpk@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@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;}
On 10/29/24 09:48, Ilias Apalodimas wrote:
Hi Heinrich
On Tue, 29 Oct 2024 at 10:00, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Heinrich
On Mon, 28 Oct 2024 at 08:37, Heinrich Schuchardt xypron.glpk@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@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@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;}
participants (3)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Tom Rini