Re: [PATCH v7 00/23] nova-core: Add memory management support

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Alex,

Thanks for taking the time to go through the series and for the effort
of doing the reordering. Just to clarify, do you mean I should be
sending each of the phases separately for review instead of in one
series?

By any chance, do you have the tree after doing the rearrangement that I
could take a look at? That would be very helpful so I don't repeat your
rearrangement effort.

Some comments below:

> Nice series, a few overall remarks before I dive deeper into each patch:
>
> - Use `gpu: nova-core:` (not just `nova-core:`) as the patch prefix.

Done.

> - There were a few clippy warnings/rustfmt diffs when I built it.
> - There are build failures introduced in patches 11 and 18 - basically
>   the series doesn't build from 11 until the last patch.
> - This patchset is using the `module/mod.rs` pattern instead of
>   `module.rs` for new modules. The latter is the preferred norm IIUC.

I will work on fixing these based on the reordered patches for the next
spin.

> Phase 1: GSP plumbing
> - nova-core: gsp: Return GspStaticInfo and FbLayout from boot()
> - nova-core: gsp: Extract usable FB region from GSP
> - nova-core: fb: Add usable_vram field to FbLayout
>
> These constitute a logical change by themselves, by getting more
> information from the GSP to know how much VRAM we have. You could even
> display the result as a dev_info of dev_dbg to remove the only remaining
> `dead_code`.

This looks good to me.

> Phase 2: PRAMIN support
> - nova-core: mm: Add support to use PRAMIN windows to write to VRAM
> - docs: gpu: nova-core: Document the PRAMIN aperture mechanism
>
> PRAMIN is needed by everything that follows.

This looks good to me.

> Phase 3: GpuMm
> - nova-core: mm: Add common memory management types
> - nova-core: mm: Add TLB flush support
> - nova-core: mm: Add GpuMm centralized memory manager
> - nova-core: mm: Use usable VRAM region for buddy allocator
>
> The common memory management types patch and TLB give us all we need to
> introduce GpuMm, which makes it more accessible that after going through
> all the page table types which it doesn't depend on. This culminates
> with using the result of phase 1, which also allows you to get rid of
> the temporary 1MB window hack if you rearrange the code a bit.

Yeah, that is a nice advantage!

> Phase 4: page tables / VMM
> - nova-core: mm: Add common types for all page table formats
> - nova-core: mm: Add MMU v2 page table types
> - nova-core: mm: Add MMU v3 page table types
> - nova-core: mm: Add unified page table entry wrapper enums
> - nova-core: mm: Add page table walker for MMU v2/v3
> - nova-core: mm: Add Virtual Memory Manager
> - nova-core: mm: Add virtual address range tracking to VMM
> - nova-core: mm: Add multi-page mapping API to VMM
>
> The main course, required for BAR1 - these follow the original order.

Sounds good.

> Phase 5: BAR1
> - nova-core: Add BAR1 aperture type and size constant
> - nova-core: gsp: Add BAR1 PDE base accessors
> - nova-core: mm: Add BAR1 user interface
> - nova-core: mm: Add BarUser to struct Gpu and create at boot

These sound good to me.

> All the BAR1 stuff now happens in one place. There is certainly room for
> merging a few patches to avoid introducing dead code and eliminating
> just after.

Yeah, I tried to keep the commits at a reasonable size to make review
easier. I will look into merging a little more to see where it is possible.

> Phase 6: tests
> - nova-core: mm: Add PRAMIN aperture self-tests
> - nova-core: mm: Add BAR1 memory management self-tests

This sounds good to me.

> I have done this reordering locally and it seems to build fine.

Please share your reorder tree if you still have it. That would likely save
me a lot of effort. Thanks.

> I'll do a patch-by-patch review following this order, but I wanted to
> share it first for other reviewers of this revision as it makes the
> series more accessible IMHO.

Looking forward to it. Thanks!

--
Joel Fernandes