On Sat, Aug 24, 2019 at 7:02 PM Waldek Kozaczuk <jwkozac...@gmail.com> wrote:
> > > On Saturday, August 24, 2019 at 5:34:22 AM UTC-4, Nadav Har'El wrote: >> >> >> On Sat, Aug 24, 2019 at 7:14 AM Waldemar Kozaczuk <jwkoz...@gmail.com> >> wrote: >> >>> Most of the time the kernel code references memory using virtual >>> addresses. >>> However some allocated system structures like page tables use physical >>> addresses. >>> For that reason it is critical that physical addresses are never 0 which >>> for example >>> in case of page table would mean that given entry is empty. >>> >>> This patch enforces that free physical memory ranges registered >>> during initial memory setup do NOT start at address 0. >> >> >> Excellent catch. I'm really surprised we didn't either have this code >> before - or actually, I would have expected our memory enumeration code to >> know that physical memory 0 is not usable and not try to "free" it at all. >> There are often other unusable ranges in low memory (< 1MB), such as bios >> data areas and the video memory. Can you please think if we don't need to >> avoid freeing these areas as well (or do we already do those correctly)? >> > Well, we did have the code that possibly was supposed to catch this. But > it was in the wrong place - memory::free_initial_memory_range() that > receives virtual address after translation instead of > mmu::free_initial_memory_range() > called from arch-setup.cc. I am guessing that possibly originally > memory::free_initial_memory_range() used to receive the physical address > and do translation later. Then than check made sense. Also, note that > either free_initial_memory_range is used only during the boot process. > I agree. Note that my comment was we should make sure that not only page 0 is skipped, there may be other pages - such as VGA memory and who-knows-what-archaic-BIOS-memory-is-still-relevant-in-hypervisors (I don't remember the detais) we need to skip. Maybe the code which enumerates the memory blocks skip those areas, they just didn't skip 0? >> >> Is this not still needed, to avoid the zero virtual address? Though I >> guess that this will never happen (if we skip the 0 physical address, how >> will we get a 0 virtual address), perhaps better safe then sorry? >> > Well, I deleted this bit of code for personal reasons - these 4 lines is > what confused me when you asked me about handling 0-addresses when applying > below kernel patch - > https://groups.google.com/d/msg/osv-dev/Lfj3IbO7ges/k0VuA-w_BgAJ - and > gave me the impression that we prevent 0 address. > Yes, it's unfortunate that I asked exactly about the possibility of that bug, and then we both missed that it is in fact is a real bug. > Based on the fact, that virtual addresses converted by > *mmu::phys_to_virt()* - > https://github.com/cloudius-systems/osv/blob/fb7ef9a791c494889b060c3ae8e1cc2cb33d5c5a/core/mmu.cc#L95-L100 > - > are either ffff800000000000 plus p or translated to kernel code > addresses, I do not see the reason for this check - they will never be 0 > (or can they?). It may confuse someone else. Unless you insist. > No, as I said, I don't think they can be 0. Another option is to do an assert(), just in case. But I agree with you it will probably never trigger. > >> But given the importance of this patch, I'll commit it now, as is. Thanks >> for fixing it!! >> >> auto a = reinterpret_cast<uintptr_t>(addr); >>> auto delta = align_up(a, page_size) - a; >>> if (delta > size) { >>> diff --git a/core/mmu.cc b/core/mmu.cc >>> index dc39ddb1..f6036771 100644 >>> --- a/core/mmu.cc >>> +++ b/core/mmu.cc >>> @@ -1852,6 +1852,10 @@ void linear_map(void* _virt, phys addr, size_t >>> size, >>> >>> void free_initial_memory_range(uintptr_t addr, size_t size) >>> { >>> >> In rertospect, it would be nice to add the comment why we are doing it. > It would have also been nice to have a comment that "addr" is a physical address. Don't we even have a type for it, "phys"? + if (!addr) { >>> + ++addr; >>> + --size; >>> + } >>> memory::free_initial_memory_range(phys_cast<void>(addr), size); >>> >> } >>> >>> -- >>> 2.20.1 >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "OSv Development" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to osv...@googlegroups.com. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/osv-dev/20190824041401.28033-1-jwkozaczuk%40gmail.com >>> . >>> >> -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to osv-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/59ac115e-397c-4cf8-bd79-4aea6ca363ed%40googlegroups.com > <https://groups.google.com/d/msgid/osv-dev/59ac115e-397c-4cf8-bd79-4aea6ca363ed%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjuvjpiQ%2Bhm4euu_OcWFowPrcgyXfz32wJco9t26%3DDtG_A%40mail.gmail.com.
