On 07.02.21 14:03, Andrea Bastoni wrote: > 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.
You have that extra complexity already because you have the differentiation of colored vs. non-colored in a lot of code paths. Now you will replace with "contiguous vs. colored steps" while dropping a lot of code. I suspect a net gain. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- 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/25d9e564-50f4-4cee-a8ca-b932752a3254%40siemens.com.
