Hi Jan, On 07/02/2021 11:17, Jan Kiszka wrote: > On 25.01.21 13:00, Andrea Bastoni wrote: >> Hi Jan, >> >> Here a proof of concept for the cache-coloring with/without driver approach >> (works, but hasn't been tested as much as the old version, and we have seen >> at least one crash in our tests, but it should be sufficient to have a >> discussion about the direction). >> >> The patches until >> 0018-configs-arm64-add-coloring-example-for-qemu-arm64.patch implement the >> previous coloring approach (+ bugfixing + qemu-arm64 integration). The >> patches from 0019-config-always-rely-on-the-availability-of-way_size.patch >> are the proof of concept to realize a colored jailhouse ioremap in the >> driver, keeping the same logic between the hypervisor and the driver. >> >> The "get_bit_range()" is refactored in a single place already >> (0020-coloring-extract-get_bit_range-logic-and-remove-offs.patch), but the >> main logic needs to be replicated between hypervisor and driver [*], >> coloring.c doesn't get considerably shorter, and control.c gets more >> complicated because we need to distinguish when the remapping is colored and >> when it's not. >> >> Most of the code reduction comes from the removal of the cache_layout >> autodetection, which is "debug only" (and could have been removed in the >> previous version as well). >> >> Overall I count for the previous approach (patches 01 - 18): >> 42 files changed, 1337 insertions(+), 121 deletions(-) >> For the new one (patches 01 - 23): >> 41 files changed, 1233 insertions(+), 127 deletions(-) >> > > Thanks for the update. I think we are moving forward. The diffstat for > the hypervisor (with include/) is getting closer to an interesting range: > > 32 files changed, 721 insertions(+), 122 deletions(-)
Thanks for checking the differences between the versions. Which one do you mean here? The previous version (01 - 18) without cache_layout* is smaller, roughly 30 files changed, 710 insertions(+), 117 deletions(-) IIRC we discussed already that cache_layout was to be removed (not even "DEBUG/NDEBUG"). > But we can still do better: > > - make non-colored operations the special case of colored ones (e.g. > map_uncolored(...) = map_colored(all_colors)), that should further > reduce duplications, both in the hypervisor and the driver > - make sure that only the calculation of color bars' start and size > is arch-specific (where I still think it belongs to arm-common...) so > that iteration & Co. can stay in the generic core I think this is a larger design decision than just coloring. I can integrate coloring into the paging and have the non-colored version be a special case, but this means extra complexity for all situations that do not require coloring. I'm not talking about runtime complexity here (paging is likely irrelevant at runtime), but documenting the code for a possible certification (if this is of interest) becomes more costly, for something that e.g., on x86, you don't need. The first version provided a way to identify a coloring-feature (and the corresponding API) you may want to exclude from the scope of a certification and from a possible pre-certification cleanup. If we integrate the coloring into the paging, this is no longer possible. > - pull all coloring unrelated refactorings to the front of the series, > e.g. the panic things, to allow taking them earlier > - avoid needless static inline, rather provide linkable stubs on arch > that do not support an implementation > > In addition, some consistency checks in jailhouse-config-check would > probably help to make users' life easier. Or may even obsolete some > runtime assertions. > > Jan > -- Thanks, Andrea Bastoni -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to jailhouse-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/7d28fae1-29b1-7957-5219-11b11a572681%40tum.de.
