Hi Hakan, Thank you for the detailed review. I am sorry for sending such a large patch. I should have split the work into smaller changes with less assumptions, and talk about every one.
I will respond to your comments one at a time. > +pmap_discover_physical_memory(struct dtb_node *node) You are right; the second patch accidentally included the first patch’s changes because I generated it from the wrong commit range. > This assumes all memory after the kernel image is available. It does > not account for the DTB potentially residing immediately after the > kernel, or near the end of RAM. The bootstrap allocator must reserve the DTB and all earlier boot allocations instead of assuming that memory immediately after _end is free. > phystokv(x) was defined as x + 0xffffffff80000000, so, assuming a > QEMU physical address range, this would wrap back to 0. The translation > mechanism of phystokv() should have been changed before this. (Updated, > see [1]) This code still used the old phystokv()/kvtophys() contract, so I will adopt the updated direct-map translation from high-address-kernel before reworking Sv39. > The physical base is hard-coded to QEMU virt RAM. Other systems (including > my development board Milk-V Mars) place RAM elsewhere. The DTB has already > supplied phys_mem_start, there is no need to hard-code this. I will calculate nr_phys_pages from the DTB-provided phys_mem_start and phys_mem_size instead of assuming RAM begins at 0x80000000. > Why is this capped to 16? A small comment would be helpful. The limit of 16 was arbitrary, so I will remove it and allocate the exact number of L0 tables required for the bootstrap virtual-address range, bounded by the DTB-reported memory and failing explicitly if the available memory is insufficient. > This again assumes that RAM begins at 0x80000000. Same as up. > This hardcodes QEMU's device layout. Device mappings must come from DTB, > we should not assume a specific board. > > The comment also contains a non-ASCII multiplication sign. You are right; I will remove the fixed QEMU device mapping and derive each MMIO region from the DTB, using only the device ranges reported by the target board. > Index 64 assumes that the kernel allocation area starts exactly 128 MiB > into the L1 table. It is not derived from kernel_virtual_start. I will derive the L1 index from kernel_virtual_start and the Sv39 index macros instead of assuming a fixed 128 MiB offset. > These root indices encode a fixed QEMU physical layout. I will derive the root-table indices from the mapped virtual addresses and DTB-discovered regions instead of hard-coding QEMU-specific entries. > We enabled paging, but we're still on the lower address, we are not > jumping to the higher half kernel memory in this patch. In the case we > did, the ldscript lays the VMAs of symbols starting from _START_MAP, so > pointers within globals will be directed to the lower half memory, > which means the lower half can not be safely unmapped. The ldscript > should have been tweaked before this. (Updated, see [1]) > > The comment also contains a non-ASCII arrow. I will establish the higher-half linker layout and trampoline before enabling Sv39, keep the lower identity map only temporarily, and replace the non-ASCII arrow in the comment. (Sorry, IA autocompletion) > A leaf mapping cannot be accepted merely because it covers the requested > virtual address. Its physical address and permissions may differ from > the requested mapping. Silently returning here is not desirable. > > This path also doesn't split a megapage when a page-level mapping or > protection change is required. You are right; I will validate the existing leaf mapping and split megapages when a page-level mapping or protection change is required instead of returning silently. > riscv reserves the W=1, R=0 PTE encoding. A request containing write perms > without read permission would produce an invalid PTE. I will reject or normalize write-only mappings so that every PTE with W=1 also has R=1, avoiding the reserved RISC-V encoding. > This overwrites existing mappings at the hardware-level without maintaining > the software resident and wired counts, or removing the old pmap mappings. Before replacing a PTE, I will remove the existing mapping and update the resident and wired counts, then invalidate the affected TLB entry. > What did you mean by the comment here? Twelve is the page-offset width, > each sv39 table is indexed by nine bits and contains 512 entries. The comment was incorrect. RISCV_PT_SHIFT is the 12-bit page-offset width, while each Sv39 page table uses 9-bit indices and contains 512 entries. > This mask includes reserved upper PTE bits. Specifically, it occupies > 61:10. Sv39 PPN occupies bits 53 through 10, so the mask must not > extend that much. You are right; I will correct RISCV_PTE_PPN_MASK to cover only Sv39 PPN bits 53:10 and leave the upper PTE bits reserved. Like I said, I will be working in a specific function and send them like you recommended. Regards, Diego El mar, 8 sept 2026 a la(s) 5:43 p.m., Hakan Candar ([email protected]) escribió: > Hi Diego, > > Thanks for your patch series! > > I applied the first patch with minor changes. The second patch is also > ambitious and defines a lot of useful stuff we need (CSR definitions, > writes/reads, etc.). However, I can not apply it as is, because there > are correctness and portability concerns. The pmap impl is one of the > most important foundations of the port, so I am being extra careful > in this area. > > Here are some of my concerns, beginning from patch 2: > > > + pmap_discover_physical_memory(&node); > > > > +void > > +pmap_discover_physical_memory(struct dtb_node *node) > > This repeats the physical memory discovery implementation already > introduced in patch 1. Perhaps `git format-patch` was invoked > improperly? > > > + bootstrap_heap = round_page((vm_offset_t) _end); > > This assumes all memory after the kernel image is available. It does > not account for the DTB potentially residing immediately after the > kernel, or near the end of RAM. > > > + kernel_virtual_start = phystokv(round_page(phys_mem_start + > phys_mem_size)); > > + kernel_virtual_end = VM_MAX_KERNEL_ADDRESS - PAGE_SIZE; > > > > + directmap_end = _kvtophys(kernel_virtual_start); > > phystokv(x) was defined as x + 0xffffffff80000000, so, assuming a > QEMU physical address range, this would wrap back to 0. The translation > mechanism of phystokv() should have been changed before this. (Updated, > see [1]) > > > + nr_phys_pages = (directmap_end - 0x80000000UL) / PAGE_SIZE; > > The physical base is hard-coded to QEMU virt RAM. Other systems (including > my development board Milk-V Mars) place RAM elsewhere. The DTB has already > supplied phys_mem_start, there is no need to hard-code this. > > > +#define MAX_L0_TABLES 16 > > > > + num_l0_tables = (nr_phys_pages * 512 + (2 * 1024 * 1024) - 1) > > + / (2 * 1024 * 1024) + 4; > > + if (num_l0_tables < 8) > > + num_l0_tables = 8; > > + if (num_l0_tables > MAX_L0_TABLES) > > + num_l0_tables = MAX_L0_TABLES; > > Why is this capped to 16? A small comment would be helpful. > > > + unsigned int num_kernel = (directmap_end - 0x80000000) > > + / (2*1024*1024); > > > > + for (idx = 0; idx < num_kernel; idx++) { > > + pa = 0x80000000UL + (vm_offset_t)idx * (2 * 1024 * > 1024); > > + l1_kernel[idx] = pa_to_pte(pa) | pte_flags; > > + } > > This again assumes that RAM begins at 0x80000000. > > > + unsigned int num_devices = 136; /* 136 × 2MB = 272MB, > covers up to 0x11000000 */ > > + for (idx = 0; idx < num_devices; idx++) { > > + pa = (vm_offset_t)idx * (2 * 1024 * 1024); > > + l1_devices[idx] = pa_to_pte(pa) | pte_flags; > > + } > > This hardcodes QEMU's device layout. Device mappings must come from DTB, > we should not assume a specific board. > > The comment also contains a non-ASCII multiplication sign. > > > + l1_kernel[64 + idx] = pa_to_pte(l0_pas[idx]) | > ptr_flags; > > Index 64 assumes that the kernel allocation area starts exactly 128 MiB > into the L1 table. It is not derived from kernel_virtual_start. > > > + root_table[0] = pa_to_pte(l1d_pa) | ptr_flags; > > + root_table[2] = pa_to_pte(l1k_pa) | ptr_flags; > > + root_table[510] = pa_to_pte(l1k_pa) | ptr_flags; > > These root indices encode a fixed QEMU physical layout. > > > + satp_write(satp_val); > > + sfence_vma(); > > + } > > > > + /* > > + * Now paging is ON. The identity map at root[2] covers > > + * VA 0x80000000 → PA 0x80000000, so the code at ~0x802xxxxx > > + * keeps working. > > We enabled paging, but we're still on the lower address, we are not > jumping to the higher half kernel memory in this patch. In the case we > did, the ldscript lays the VMAs of symbols starting from _START_MAP, so > pointers within globals will be directed to the lower half memory, > which means the lower half can not be safely unmapped. The ldscript > should have been tweaked before this. (Updated, see [1]) > > The comment also contains a non-ASCII arrow. > > > + if (RISCV_PTE_IS_LEAF(*pte)) { > > + /* > > + * Already mapped by a megapage. For bootstrap, this is > fine — > > + * the direct map already covers this PA. Nothing to do. > > + */ > > + PMAP_READ_UNLOCK(pmap, spl); > > + return; > > + } > > A leaf mapping cannot be accepted merely because it covers the requested > virtual address. Its physical address and permissions may differ from > the requested mapping. Silently returning here is not desirable. > > This path also doesn't split a megapage when a page-level mapping or > protection change is required. > > > + template = pa_to_pte(pa) | RISCV_PTE_V | RISCV_PTE_A | RISCV_PTE_D; > > + if (pmap != kernel_pmap) > > + template |= RISCV_PTE_U; > > + if (prot & VM_PROT_WRITE) > > + template |= RISCV_PTE_W; > > riscv reserves the W=1, R=0 PTE encoding. A request containing write perms > without read permission would produce an invalid PTE. > > > + WRITE_PTE(pte, template); > > + sfence_vma(); > > > > + PMAP_UPDATE_TLBS(pmap, v, v + PAGE_SIZE); > > This overwrites existing mappings at the hardware-level without > maintaining > the software resident and wired counts, or removing the old pmap mappings. > > > +#define RISCV_PT_SHIFT 12 /* page table covers 2^12 > = 4096 entries? No: 2^9 = 512 */ > > What did you mean by the comment here? Twelve is the page-offset width, > each sv39 table is indexed by nine bits and contains 512 entries. > > > +#define RISCV_PTE_PPN_MASK 0x3FFFFFFFFFFFFC00UL > > This mask includes reserved upper PTE bits. Specifically, it occupies > 61:10. Sv39 PPN occupies bits 53 through 10, so the mask must not > extend that much. > > > +/* ---- Compat aliases mapping Intel PTE names to RISC-V ---- */ > > > > +#define INTEL_PTE_VALID RISCV_PTE_V > > +#define INTEL_PTE_WRITE RISCV_PTE_W > > +#define INTEL_PTE_PS 0 > > +#define INTEL_PTE_WIRED 0 > > This is actually unnecessary, we shouldn't keep the baggage carried over > from the initial stub port. x86-specific definitions should be removed, > and only riscv ones should remain. (Updated, see [1]) > > ** > > Those aside, scaffolding part of the second patch (CSR definitions, > write/read helper macros, etc.) was clean and I applied it. > > ** > > [1]: Considering these, and looking at the impartial foundation of > the master branch before beginning sv39 mapping work (i.e. ldscript > not using higher half memory, phystokv/kvtophys contract not being > clear, Intel definitions leaking through), I created a new branch > named `high-address-kernel` where paging work can begin cleanly. > > In this branch (high-address-kernel), I: > * Changed the VMA of the kernel to start from 0xffffffff80000000 instead > of the previous VMA of 0x80000000. The kernel proper should live in > higher half memory. > * Changed the LMA of the kernel to begin from 0 (Similar to Bugaev's), > and use objcopy to flatten the kernel image. QEMU, or any other SBC > will load the kernel at an available address instead of trying and > failing to obtain ELF-LMA physical address area. > * Defined the direct-mapped range to begin from 0xffffffc000000000 > and changed phystokv/kvtophys appropriately. > * Deferred the console initialization, because the updated ns16550 driver > reads from statically initialized memory which embeds a (high) VMA > address. > The console will be enabled after paging. (Earlycon to be impl'd later) > > Also I axed the x86 definitions that were carried over from the initial > stub port (My bad! Those should have never been there) > > I tested the new arrangement via QEMU, and verified that the early > boot phase environment does not dereference high address VMA. The > path to pmap_bootstrap() is clean and ready. > > This prepares the codebase properly so that paging work can begin for > a high-address kernel. > > After paging is implemented and we get printf working from higher half > memory, the changes will be merged back to master. I am opting to not > leave the master branch in an intermediate state and keep the existing > banner demo working. > > If you want to iterate on this based on the high-address-kernel branch, > let me know. Alternatively, since the early bootstrap interacts so heavily > with the updated link layout and direct-mapping convention, I can take > the lead on implementing the initial Sv39 mapping and higher-half > trampoline. > Either way, any code adapted from your patch will keep your attribution. > > A small side note: For the next patches, we should continue on separate > threads, as usual in mailing list based patch review. The diffs should > be provided inline in the mail and not be provided as an attachment, > as the former approach makes the review easier for everyone, including > the reviewer and the observers :) > > To send patches easily from your terminal, you can use git send-email, > which does those automatically for you. i.e. via: > > git format-patch --cover-letter --subject-prefix="PATCH gnumach riscv64" > -o outgoing/ [RANGE] > git send-email --to="[email protected]" outgoing/*.patch > > Let's use the prefix "gnumach riscv64" so that riscv-specific patches > are immediately distinguishable from patches meant for upstream. I think > this will prevent confusion among maintainers and observers of the list. > > This one was a long read, so I appreciate you taking your time to read > it. The patches are valuable, and I am glad we are getting the riscv > port going. > > Have a nice week! > > Hakan >
