On 25.11.20 10:15, Andrea Bastoni wrote: > On 25/11/2020 07:07, Jan Kiszka wrote: >> On 23.11.20 21:45, Andrea Bastoni wrote: >>> Hello Jan, hi all, >>> >>> As discussed a couple of weeks ago, here an updated version of the >>> "cache-coloring" for Jailhouse. >>> >>> A short recap on what's coloring (see Documentation/cache-coloring.md): >>>> Cache coloring is a software technique that permits LLC partitioning, >>>> therefore eliminating mutual core interference, and thus guaranteeing more >>>> predictable performances for memory accesses. >>> >>> In this version of the patch, we've tried to keep the changes in the >>> hypervisor core minimal. Also, since coloring is something that should be >>> transparent to inmates/VM (coloring belongs to the IPA/PA hypervisor >>> translations), the Linux driver has been only minimally modified. >>> >>> Coloring API >>> ------------ >>> >>> The patch adds a small architecture API layer that implements coloring. The >>> API is fully implemented only on arm64. arm32 could be also a possible >>> target for coloring, x86 already has CAT. Despite being implemented by only >>> one architecture, having a small common API layer integrates IMHO better >>> than other solutions in the main code (without adding conditionally >>> compiled code). >>> >>> The API is: >>> arch_color_map_memory_region() // cell_create >>> Color-maps the memory of the cell. Coloring requires multiple >>> passes to keep a contiguous VA, while "punching holes" in the PA according >>> to the allocated colors. >>> >>> arch_color_unmap_memory_region() // cell_destroy >>> The opposite of create: search and give back each piece of colored >>> PA. >>> >>> arch_color_remap_to_root() // cell_load >>> Given that we don't want to modify the driver (i.e., we don't want >>> the driver to do non-contiguous color-ioremap) we let the root cell believe >>> that the IPAs it is ioremapping are contiguous, but in reality the PA are >>> non-contiguous (colored). This is currently done by using a very high VA >>> (IPA) address as "base" for the load-remap-to-root-cell operation. This >>> value is configuration dependent and can be specified in the root cell to >>> avoid (unlikely) conflicts. >>> >>> arch_color_unmap_from_root() // cell_start >>> Returns the mapping setup by the cell_load to the root cell. >>> >>> Two APIs are implemented only by arm64: >>> arm_color_dcache_flush_memory_region() // range flushing >>> Also flushing should be done in a "colored way". >>> >>> arm_color_init() // hook for the initialization >>> of coloring >>> Reads the parameters (size of the LLC way, base offset for the >>> load-remapping operation) needed by coloring. There's also some debugging >>> code to autodetect the cache-geometry and determine the size of the way. >>> >>> Configuration >>> ------------- >>> >>> From the configuration point of view, a colored memory region is a normal >>> memory region with a color attached. The color is expressed directly in the >>> memory region as color-bitmask and the flag JAILHOUSE_MEM_COLORED is used >>> to identify such a region. Contiguous bits in the bitmask identify a color >>> (region) to be used for the memory region. The size of the mapping doesn't >>> change due to coloring. >>> >>> For example, with 16 colors, a full way size is selected by 0xffff, while >>> 1/4 of the way size is selected by 0x000f. >>> >>> (Note: we also experimented with a different version that defined separate >>> "normal" and "colored" memory regions. The approach presented in this >>> version is more robust.) >>> >>> Main coloring "loop" >>> -------------------- >>> >>> The coloring APIs basically boil down to a loop that appropriately selects >>> --according to the color-- the physical addresses where to perform a >>> selected operation. The selection is done efficiently with bit operations >>> on the bitmask. >>> >>> An alternative approach is to hook coloring deep into the mapping >>> functionalities of the hypervisor. Basically, all low level mapping >>> functions support coloring, and the non-colored case becomes the "special >>> case." >>> This approach was not followed because to be implemented "cleanly", all the >>> architecture mapping APIs have to be extended to support coloring, but only >>> one-architecture then really implement it. >>> >>> Instead, the implemented approach to have an additional "coloring" API >>> integrates more nicely. Additionally, coloring will be hopefully only a >>> transitory techniques (some years?) that could be eventually replaced by >>> hardware-based techniques (e.g., MPAM). When this happens, removing the >>> additional coloring API requires less rework than the other approach. >>> >>> >>> Debugging Code >>> -------------- >>> >>> The patches also provide a "debugging-only" code that can be activated by >>> defining CONFIG_DEBUG (it sounded more Jailhouse-oriented than NDEBUG). In >>> debugging mode, the coloring provides an autodetection for arm64 cache >>> geometry. >>> Additionally, I've added an assert() function that can be used at runtime >>> in debugging only contexts (could be used independently from coloring). >>> >>> >>> Feedback and comments welcome. >>> >>> Best, >>> Andrea >>> >>> Andrea Bastoni (12): >>> arm-common: bitops: add most-significant-bit operation >>> hypervisor, driver, archs: add basic empty infrastructure for coloring >>> hypervisor: arm64: add cache coloring implementation >>> hypervisor configuration: hook autodetection for coloring_way_size >>> configs: arm64: add example configuration for colored ZCU102 inmate >>> hypervisor: protect inclusion of control.h >>> hypervisor, driver: add platform information to configure coloring >>> params >>> configs: arm64: hook-in coloring parameters for ZCU102 >>> hypervisor: provide runtime assert() helper for DEBUG only >>> hypervisor: provide implementation for __assert_fail() and group >>> panic-related functions >>> hypervisor: coloring: use assert instead of "BUG" >>> hypervisor: coloring: make cache autodetection debug-only >>> >>> Luca Miccio (2): >>> Documentation: add description and usage of cache coloring support >>> pyjailhouse: add support for colored regions >>> >>> Documentation/cache-coloring.md | 198 ++++++++++++++++++ >>> configs/arm64/zynqmp-zcu102-inmate-demo-col.c | 75 +++++++ >>> configs/arm64/zynqmp-zcu102.c | 6 + >>> driver/cell.c | 10 +- >>> driver/cell.h | 1 + >>> driver/main.c | 12 ++ >>> hypervisor/Makefile | 2 +- >>> hypervisor/arch/arm-common/control.c | 1 + >>> .../arch/arm-common/include/asm/bitops.h | 10 + >>> .../arch/arm-common/include/asm/dcaches.h | 8 + >>> hypervisor/arch/arm-common/mmu_cell.c | 52 +++-- >>> hypervisor/arch/arm/control.c | 1 + >>> hypervisor/arch/arm/include/asm/coloring.h | 59 ++++++ >>> hypervisor/arch/arm/traps.c | 1 + >>> hypervisor/arch/arm64/Kbuild | 5 + >>> hypervisor/arch/arm64/cache_layout.c | 148 +++++++++++++ >>> hypervisor/arch/arm64/coloring.c | 184 ++++++++++++++++ >>> hypervisor/arch/arm64/control.c | 1 + >>> .../arch/arm64/include/asm/cache_layout.h | 21 ++ >>> hypervisor/arch/arm64/include/asm/coloring.h | 137 ++++++++++++ >>> hypervisor/arch/arm64/setup.c | 3 + >>> hypervisor/arch/arm64/traps.c | 1 + >>> hypervisor/arch/x86/amd_iommu.c | 1 + >>> hypervisor/arch/x86/control.c | 1 + >>> hypervisor/arch/x86/efifb.c | 1 + >>> hypervisor/arch/x86/include/asm/coloring.h | 45 ++++ >>> hypervisor/arch/x86/ioapic.c | 1 + >>> hypervisor/arch/x86/svm.c | 1 + >>> hypervisor/arch/x86/vmx.c | 1 + >>> hypervisor/control.c | 111 ++++------ >>> hypervisor/include/jailhouse/assert.h | 37 ++++ >>> hypervisor/include/jailhouse/control.h | 34 +-- >>> hypervisor/include/jailhouse/panic.h | 40 ++++ >>> hypervisor/include/jailhouse/printk.h | 4 + >>> hypervisor/panic.c | 86 ++++++++ >>> hypervisor/pci.c | 1 + >>> hypervisor/printk.c | 1 + >>> hypervisor/uart.c | 1 + >>> include/jailhouse/cell-config.h | 11 + >>> pyjailhouse/config_parser.py | 9 +- >>> 40 files changed, 1201 insertions(+), 121 deletions(-) >>> create mode 100644 Documentation/cache-coloring.md >>> create mode 100644 configs/arm64/zynqmp-zcu102-inmate-demo-col.c >>> create mode 100644 hypervisor/arch/arm/include/asm/coloring.h >>> create mode 100644 hypervisor/arch/arm64/cache_layout.c >>> create mode 100644 hypervisor/arch/arm64/coloring.c >>> create mode 100644 hypervisor/arch/arm64/include/asm/cache_layout.h >>> create mode 100644 hypervisor/arch/arm64/include/asm/coloring.h >>> create mode 100644 hypervisor/arch/x86/include/asm/coloring.h >>> create mode 100644 hypervisor/include/jailhouse/assert.h >>> create mode 100644 hypervisor/include/jailhouse/panic.h >>> create mode 100644 hypervisor/panic.c >>> >> >> Thanks for the update! I assume it's functional and can be used in >> tests? Asking as there seems to be a lot of debug code and comments. > > Yes, it is functional and it can be used for testing. I assumed this wasn't > the > final version, and the debug code + commented out code are still there in the > places where it can be useful to take a look to understand how the machinery > is > working. > > I'm also interested in your point of view regarding having a CONFIG_DEBUG > option > in Jailhouse. Basically a "debugging/development-enabled" version (with > assert() > and extra verbose output), and a "production" one. > >> Scanning through it, I still find it very arm64-centric, rather than > > It is. As mentioned, I don't know if it is meaningful to have a x86 version of > the coloring, and for how long there will be the need to have coloring in > software. So, the patches try to keep the changes in both hypervisor and > driver > to a minimum.
x86 might not be meaningful for modern CPUs with CAT, but it is another test case. But I'm also thinking of RISC-V and what else might come along. > >> generically addressing the topic as I pointed out in previous reviews. >> And keeping modifications of the driver minimal is not necessarily a >> sign we are on the same page already. > > We've experimented also with a version that does "more" in the driver. I think > that more changes in the driver only add complexity. In the end the hypervisor > have to distinguish anyway between colored and non-colored mapping, and > scattering the load logic between driver and hypervisor has more maintenance > effort than a single clean configuration parameter. You are possibly right, it's just hard for me to follow this as there is no code or more concrete design to prove that argument. > >> For testing purposes, please also include a QEMU configuration change in >> the future. Helps validating things and playing the functional changes. > > OK. > >> The patch series should further more be structured in way that a static, >> config-defined way-size can be used before any arch-specific >> auto-detection logic is added. > > Sure. As said, I assumed that discussion was needed, but I think it's better > to > discuss with a draft of how the code will look like. > On the one hand, discussion can be less effort than coding if everyone can follow the arguments and this leads to a common view. On the other hand, code has the advantage of make an even stronger argument as everyone can see the results and compare, and can even do that in the future when that topic pops up again for some reason. 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/054fe4bd-e3ea-dfa5-c8aa-c3d2a2cad6fb%40siemens.com.
