| Message ID | 20241024104625.1340355-1-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | e26d2cab4285d2953879054a4ff6955fb756a6d4 |
| Headers | show |
| Series | lmb: Correctly unmap memory after notifications | expand |
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; >>>> } >>>
On Thu, 24 Oct 2024 13:46:25 +0300, Ilias Apalodimas wrote: > We never unmap the memory used to update the EFI memory map after > notifications > > Applied to u-boot/master, thanks!
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; }
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(+)