intel-vtd.c::vtd_get_n_paging looks so suspicious
Hello all, kernel/src/plat/pc99/machine/intel-vtd.c::vtd_get_n_paging() confused me a lot. Wish to get any help. This func intends to calculate paging pages needed for all RMRRs. The following code 'filter the identical regions by pci bus id', and use them to figure out the final result. ``` for (word_t i = 1; i < rmrr_list->num; i++) { if (vtd_get_root_index(rmrr_list->entries[i].device) != vtd_get_root_index(filtered.entries[filtered.num - 1].device) && rmrr_list->entries[i].base != filtered.entries[filtered.num - 1].base && rmrr_list->entries[i].limit != filtered.entries[filtered.num - 1].limit) { filtered.entries[filtered.num] = rmrr_list->entries[i]; filtered.num++; } } ``` But vtd_map_reserved_page() says, every different devices(with diff bus-dev- func ID) have their own paging structures. Then why do filter here like this , yet the logic looks so confusing. And on line 313, it counts pages according to the specified level address bits. ``` size += get_n_paging(region, 32 - (VTD_PT_INDEX_BITS * i + seL4_PageBits)); ``` So what does 32 mean? Maybe a typo? Thanks, laokz
Hello all,
kernel/src/plat/pc99/machine/intel-vtd.c::vtd_get_n_paging() confused me a lot. Wish to get any help.
This func intends to calculate paging pages needed for all RMRRs. The following code 'filter the identical regions by pci bus id', and use them to figure out the final result. ``` for (word_t i = 1; i < rmrr_list->num; i++) { if (vtd_get_root_index(rmrr_list->entries[i].device) != vtd_get_root_index(filtered.entries[filtered.num - 1].device) && rmrr_list->entries[i].base != filtered.entries[filtered.num - 1].base && rmrr_list->entries[i].limit != filtered.entries[filtered.num - 1].limit) { filtered.entries[filtered.num] = rmrr_list->entries[i]; filtered.num++; } } ``` But vtd_map_reserved_page() says, every different devices(with diff bus-dev- func ID) have their own paging structures. Then why do filter here like this , yet the logic looks so confusing.
And on line 313, it counts pages according to the specified level address bits. ``` size += get_n_paging(region, 32 - (VTD_PT_INDEX_BITS * i + seL4_PageBits)); ``` So what does 32 mean? Maybe a typo?
Thanks, laokz
Hi laokz, `vtd_get_n_paging()` tries to pre-calculate how many page tables will be needed to set up the initial VTD mappings. The value that it returns gets used to preallocate the memory required and then later on in the boot process the page tables are allocated using `it_alloc_paging`. Part of the logic in vtd_get_n_paging appears to try and calculate how many page tables are required for creating the reserved memory mapping regions. As you identify, the logic doesn't seem to be correct in several places. Currently, the final result is that it overcalculates the amount of page tables required by quite a lot, so the consequence for the overall system is some memory gets leaked during boot due to the overallocation. I'm working on a patch and I'll notify you when it's up on GitHub. Thanks for tracking this down! Kent
Hi Kent, Here another problem relevant intel-vtd needs to consider about when working the patch(maybe you've already done that:). In kernel/src/arch/x86/object/iospace.c::decodeX86IOPTInvocation, when do `X86IOPageTableMap`, first we look up the last existing PTE for the new PT mapping: ``` lu_ret = lookupIOPTSlot(vtd_pte, io_address); ``` Because `io_address` comes from user space, it can be any value(bits 0~20 masked out). Let's assume x86KSnumIOPTLevels=4(0~3). If level 3 PT for `io_address` already existed and entry 0 was free, the `lu_ret` would give: ``` { .status = EXCEPTION_NONE; .ioptSlot = free level 3 PTE address; .level = 0; } ``` Thereafter the code would create a valid page mapping instead of PT mapping, and the cap's level field would be set to 4 - violate the seL4 specification I think. So when checking `lookupIOPTSlot` return value, we should also check the `.status = EXCEPTION_NONE && .level = 0` condition, and figure out that it is another EXCEPTION_SYSCALL_ERROR. If I misunderstood something, please let me know. Regards, laokz
participants (2)
-
laokz
-
Mcleod, Kent (Data61, Kensington NSW)