hw: clean up runtime page table switching for x86 virtualization

[atopia]

The Vm_page_table implementation in #5218 needs to be cleaned up. At minimum, virtualization technology detection needs to be cached at a higher level. On top of that, it should be considered to implement the runtime switch in a more idiomatic manner, such as by implementing Vm_page_table as a virtual base class and constructing a matching implementation at runtime. Since the actual page table implementations are accessed by hardware, this solution would obviously require wrapper classes that use the correct underlying implementation. The separation of the actual page table implementation in memory (e.g. Ept) and its accessor class (e.g. Vm_page_table_ept) requires changes to the x86-specific implementation of Vm_session_component, as _table would now store a class instance that is separate from the memory provided by _alloc_table(), and memory management as well as providing the physical address to hardware virtualization needs to be adapted accordingly. Commit fe899c7 on branch 5218-hw-nested-page-table-lamdba is a first, discfunctional draft implementation. We need to figure out if the clarity in language constructs outweighs the complexity in making Vm_page_table a virtual class that - contrary to the other architectures in hw - is separate from the actual nested page table.

[nfeske]

Thank you for opening this issue.

Regarding your question about the direction, I'd welcome a solution w/o using virtual functions. Not because of performance considerations but because virtual base classes often yield to entangled designs. In my experience, such interfaces (which embody the often well-intended but overzealous attempt to generalize) tend to introduce ceremony and artificial terminology that is better avoided. Let us strive for maximum clarity, using specific types, as little indirections as possible, and no side effects (like the caching of the type check).

Following this line of thinking, it would be nice to consider separate types entirely and keep the distinction only at the Vm_session_component, very similar to what @atopia is already suggesting but without virtual interfaces. The _component could hold two factories one for each type (SVN, VMX). If the factories are small, there is no need to use a union. Repeated checks for the type can be avoided by hosting the check in a few private _with_... style methods. Note that I'm not entirely against a common base type but I'd keep at as bare-bones as possible.

I assume much of the code between the two types looking very similar. Let us try to factor out these similarities to library-like utility functions and types that can be reused individually by each types. In short, let's try to factor out similarities, not factoring out dissimilarities. This approach can keep the amount of duplicated code reasonably low, documents similarities where they exist, while not tapering over dis-similarities (by using most specific type as the default starting point).

[atopia]

@nfeske please have a look at 05248a4 and at the inline comments in the commit on Github. While the commit may look like I just did the minimum to separate the two classes, I actually went full circle reuniting separate implementations. The key issue for me was trying to contain the complexity of the common functionality, especially the create_vcpu method. Having different page table implementations led to keeping the Kernel::Vm::Identity member (_id) to the derived class, because it contains the physical address of the page table, which is only known once the virtualization technology specific page table has been constructed. However, keeping _id a separate member of the VMX/SVM class led to a less readable specific implementation of create_vcpu which would in turn call methods of the Vm_session_component base class for common tasks etc. As you can see in the inline comments, the current state has the downside of having to modify the physical address of the nested page table in _id from the constructor of the derived class. I'm aware this intermediate result is maybe not what you envisioned. I did attempt to split out common functionality by composition, but I ultimately found that it only led to clunky helper methods whereas especially create_vcpu needed to replicate common logic and basically touch all of the data members of the Vm_session_component across the different virtualization implementations.

Maybe you have a better idea for splitting this up that we can discuss here or in person.

[nfeske]

I think the crux of the misunderstanding is that I'm pressing to abolish the common base class whereas your patch retains a large Vm_session_component. The only thing both classes Vmx_session_component and Svm_session_component should have in common is that each of them is compatible with the Session_object<Vm_session> interface.

Using inheritance for code reuse is a bad pattern. Shared code must have the form of utilities aggregated by the specific implementations. Those utilities can be templates. E.g., create_vcpu (should it be worth sharing) can be a template taking Vmx/Svm-specifics as template arguments.

As a minor nitpick, please let us shorten Svm_vm_ to Svm_, and Vmx_vm_ to Vmx_ respectively.

[atopia]

I was aware that you were pushing for composition over inheritance, I just didn't see a good way to do it. I did previously give the CRTP a thought but of course that wasn't solving my inheritance problem around _id (or any other problem for that matter, as my issue wasn't with the vtable). Thanks for the idea of using a template for the create_cpu logic. I will try to use that for a more composition based approach.

[atopia]

@nfeske please see 4f8c503 for a starting point to refactor the separated Vm_session_component implementations.