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
