On Fri, Mar 14, 2025 at 09:12:04AM +0000, Brendan Jackman wrote: > On Thu, Mar 13, 2025 at 10:09:21PM +0000, Yosry Ahmed wrote: > > On Thu, Mar 13, 2025 at 06:11:22PM +0000, Brendan Jackman wrote: > > > This is the same thing as lookup_address_in_pgd(), but it returns the > > > pagetable unconditionally instead of returning NULL when the pagetable > > > is none. This will be used for looking up and modifying pages that are > > > *_none() in order to map memory into the ASI restricted address space. > > > > > > For a [PATCH], if this logic is needed, the surrounding code should > > > probably first be somewhat refactored. It now looks pretty repetitive, > > > and it's confusing that lookup_address_in_pgd() returns NULL when > > > pmd_none() but note when pte_none(). For now here's something that > > > works. > > > > My first instinct reading this is that lookup_address_in_pgd() should be > > calling lookup_pgtable_in_pgd(), but I didn't look too closely. > > Yeah. That outer function would get a "generic" PTE pointer isntead of > a strongly-typed p4d_t/pud_t/etc. So we either need to encode > assumptions that all the page tables have the same structure at > different levels for the bits we care about, or we need to have a > switch(*level) and then be careful about pgtable_l5_enabled(). I > think the former is fine but it needs a bit of care and attention to > ensure we don't miss anything and avoid creating > confusion/antipatterns in the code. Hmm another option is to have a common helper that takes in a lot of parameters to control the exact behavior (e.g. do we check for 'none'?), and have both lookup_pgtable_in_pgd() and lookup_address_in_pgd() call it with different parameters. This could avoid the need for a generic pointer, for example. > > And perhaps more importantly, lookup_adress_in_pgd_attr() sets *nx and > *rw based on the level above the entry it returns. E.g. when it > returns a pte_t* it sets *nx pased on pmd_flags(). I haven't looked > into why this is. > > So yeah overall it needs a bit of research and most likely needs a > couple of prep patches. Hopefully it's possible to do it in a way that > leaves the existing code in a clearer state. Agreed. > > Anyway, I was originally planning not to have asi_map()/asi_unmap() in > asi.c at all, and instead just kinda make set_memory.c natively aware > of ASI somehow. At that point I think this code is probably gonna look > a bit different. That's something I ran out of time for and had to > drop from the scope of this RFC. It's definitely not ideal in this > series that e.g. page_alloc.c, asi.c, and set_memory.c are all > implicitly coupled to one another (i.e. they are all colluding to > ensure asi_[un]map() never has to allocate). Maybe I should've called > this out as a TODO on the cover letter actually. Looking forward to seeing how this would look like :)
