On 5/4/20 8:33 PM, Jan Kiszka wrote: > On 21.04.20 12:03, 'Marco Solieri' via Jailhouse wrote: >> From: Luca Miccio <[email protected]> >> >> Use a bitmask array with fixed size for the cell's color assignment. >> The largest amount of shared last-level segment cache equipping an Arm v8 >> cluster for the embedded segment (i.e. Cortex A5?, A7?), which contains >> up to 8 cores, is 8 MiB with 16-ways associativity. >> Now, assuming the coloring algorithm to be the same as we support, i.e. >> smallest granularity with 4 KiB pages, we compute that up to 128 colors >> available. >> For this reason set the amount of colors supported to 128. >> >> Colored regions defined by the flag JAILHOUSE_MEM_COLORED are mapped >> using the "colored" version of paging_create when needed. >> The colored version of paging_destroy is used only when unmapping from >> the root cell since we are assuming a 1:1 mapping for it. >> >> Signed-off-by: Luca Miccio <[email protected]> >> Signed-off-by: Marco Solieri <[email protected]> >> --- >> hypervisor/arch/arm-common/include/asm/cell.h | 4 ++ >> .../arch/arm-common/include/asm/coloring.h | 22 +++++++ >> hypervisor/arch/arm-common/mmu_cell.c | 62 ++++++++++++++++--- >> 3 files changed, 79 insertions(+), 9 deletions(-) >> create mode 100644 hypervisor/arch/arm-common/include/asm/coloring.h >> >> diff --git a/hypervisor/arch/arm-common/include/asm/cell.h >> b/hypervisor/arch/arm-common/include/asm/cell.h >> index 9c6e8c6f..c5159b46 100644 >> --- a/hypervisor/arch/arm-common/include/asm/cell.h >> +++ b/hypervisor/arch/arm-common/include/asm/cell.h >> @@ -14,6 +14,7 @@ >> #define _JAILHOUSE_ASM_CELL_H >> #include <jailhouse/paging.h> >> +#include <asm/coloring.h> >> struct pvu_tlb_entry; >> @@ -26,6 +27,9 @@ struct arch_cell { >> u8 ent_count; >> struct pvu_tlb_entry *entries; >> } iommu_pvu; /**< ARM PVU specific fields. */ >> + >> + /** Color configuration as a bitmask */ >> + unsigned long color_bitmask[COLOR_BITMASK_SIZE]; > > This field can become generic... >
Yes, I agree. >> }; >> #endif /* !_JAILHOUSE_ASM_CELL_H */ >> diff --git a/hypervisor/arch/arm-common/include/asm/coloring.h >> b/hypervisor/arch/arm-common/include/asm/coloring.h >> new file mode 100644 >> index 00000000..9404948f >> --- /dev/null >> +++ b/hypervisor/arch/arm-common/include/asm/coloring.h >> @@ -0,0 +1,22 @@ >> +/* >> + * Jailhouse, a Linux-based partitioning hypervisor >> + * >> + * Copyright (c) Universita' di Modena e Reggio Emilia, 2020 >> + * >> + * Authors: >> + * Luca Miccio <[email protected]> >> + * Marco Solieri <[email protected]> >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. See >> + * the COPYING file in the top-level directory. >> + */ >> + >> +#ifndef _JAILHOUSE_ASM_COLORING_H >> +#define _JAILHOUSE_ASM_COLORING_H >> + >> +#define COLOR_BITMASK_SIZE 4 >> + > > ...while this is probably arch-specific. But could start with a generic > define as well split that up once some arch with a different need comes > around. > Considering my comments on previous patches regarding the architectural independent coloring interface, I will use this as a generic define for color_bitmask. >> +/* Max. number of colors supported */ >> +#define MAX_COLOR_SUPPORTED 128 > > MAX_COLOR_SUPPORTED = COLOR_BITMASK_SIZE * sizeof(u64)? Or should > COLOR_BITMASK_SIZE be rather derived from the number of colors? But > those depend on each other and are not separate tuneables. > Yes, MAX_COLOR_SUPPORTED = COLOR_BITMASK_SIZE * sizeof(u64). >> + >> +#endif /* !_JAILHOUSE_ASM_COLORING_H */ >> diff --git a/hypervisor/arch/arm-common/mmu_cell.c >> b/hypervisor/arch/arm-common/mmu_cell.c >> index db618960..912d9399 100644 >> --- a/hypervisor/arch/arm-common/mmu_cell.c >> +++ b/hypervisor/arch/arm-common/mmu_cell.c >> @@ -13,6 +13,7 @@ >> #include <jailhouse/control.h> >> #include <jailhouse/paging.h> >> #include <jailhouse/printk.h> >> +#include <jailhouse/coloring.h> >> #include <asm/sysregs.h> >> #include <asm/control.h> >> #include <asm/iommu.h> >> @@ -46,8 +47,29 @@ int arch_map_memory_region(struct cell *cell, >> if (err) >> return err; >> - err = paging_create(&cell->arch.mm, phys_start, mem->size, >> - mem->virt_start, access_flags, paging_flags); >> + if (mem->flags & JAILHOUSE_MEM_COLORED) >> + /** >> + * Identity mapping is necessary only when remapping to the root >> + * cell during destroy phase. To check if we are in the destroy > > I'm not yet sure if I get that case complete: When a cell is destroyed, > the root cell should get access to those resources it had before the > cell was create. In case of colored memory, this would mean: > > - the root cell had a colored mapping over the respective memory range > - the root cell had the colors that were later assign to the non-root > cell > > Now, if the latter case is true, colors will move back to the root cell > (not yet sure if that is actually the case with your design, but it > should be). Once they are, mapping a region back to the root cell will > "magically" establish also the stripes that used to belong to the > non-root cell. > In this design root cell has no concept of "color" and it has no color assignment either (yet). During cell create the hypervisor removes the memory assigned to the inmate by using the paging_destroy_colored, so only the correct pages are removed from the root cell. During cell destruction the hypervisor needs to remap *only* the pages assigned to that cell and since the root cell is 1:1 mapped we need to create a mapping that is strided both virtually and physically. That's why we have the identity mapping argument. This will "fill" all memory gaps created before. If I got your point, you said that we can remap the whole memory region to the root cell. Am I right? In that case, yes we do it but, as I said, we need to do it with paging_create_colored and identity map enabled. >> + * phase the control is made on the memory virtual start and >> + * col_load_address. We cannot have a scenario where these >> + * addresses are equal because: >> + * 1) virt_start == phys_start. >> + * 2) we assume that col_load_address is configured so that it >> + * does not interfere with memory layout. >> + * 3) if col_load_address is equal to phys_start there is a >> + * wrong root-cell configuration. >> + * It means that in the previous wrong scenario col_load_address >> + * overlaps some root-cell memory space. >> + */ >> + err = paging_create_colored(&cell->arch.mm, phys_start, >> + mem->size, mem->virt_start, access_flags, paging_flags, >> + cell->arch.color_bitmask, (cell == &root_cell) && >> + (mem->virt_start != >> + system_config->platform_info.col_load_address)); >> + else >> + err = paging_create(&cell->arch.mm, phys_start, mem->size, >> + mem->virt_start, access_flags, paging_flags); > > BTW, the good thing about having a generic cell->color_bitmask would be > that this field could be fully populated by default, and we would no > longer need to differentiate between colored and non-colored here. > Yes, of course. >> if (err) >> iommu_unmap_memory_region(cell, mem); >> @@ -63,8 +85,19 @@ int arch_unmap_memory_region(struct cell *cell, >> if (err) >> return err; >> - return paging_destroy(&cell->arch.mm, mem->virt_start, mem->size, >> - PAGING_COHERENT); >> + /* >> + * Do not be confused -- since paging_destroy* acts on virtual >> + * addresses, paging_destroy can be physically colored, too. >> + * We need to destroy the mapping using coloring only when unmapping >> + * from the root cell during cell_create so that the correct >> regions are >> + * removed and then used from the cells. >> + */ >> + if (mem->flags & JAILHOUSE_MEM_COLORED && (cell == &root_cell)) >> + return paging_destroy_colored(&cell->arch.mm, mem->virt_start, >> + mem->size, PAGING_COHERENT, cell->arch.color_bitmask); >> + else >> + return paging_destroy(&cell->arch.mm, mem->virt_start, >> + mem->size, PAGING_COHERENT); >> } >> unsigned long arch_paging_gphys2phys(unsigned long gphys, unsigned >> long flags) >> @@ -91,10 +124,20 @@ void arm_cell_dcaches_flush(struct cell *cell, >> enum dcache_flush flush) >> NUM_TEMPORARY_PAGES * PAGE_SIZE); >> /* cannot fail, mapping area is preallocated */ >> - paging_create(&this_cpu_data()->pg_structs, region_addr, >> - size, TEMPORARY_MAPPING_BASE, >> - PAGE_DEFAULT_FLAGS, >> - PAGING_NON_COHERENT | PAGING_NO_HUGE); >> + if (mem->flags & JAILHOUSE_MEM_COLORED) >> + paging_create_colored( >> + &this_cpu_data()->pg_structs, >> + region_addr, size, >> + TEMPORARY_MAPPING_BASE, >> + PAGE_DEFAULT_FLAGS, >> + PAGING_NON_COHERENT | PAGING_NO_HUGE, >> + cell->arch.color_bitmask, false); >> + else >> + paging_create(&this_cpu_data()->pg_structs, >> + region_addr, >> + size, TEMPORARY_MAPPING_BASE, >> + PAGE_DEFAULT_FLAGS, >> + PAGING_NON_COHERENT | PAGING_NO_HUGE); >> arm_dcaches_flush((void *)TEMPORARY_MAPPING_BASE, size, >> flush); >> @@ -120,7 +163,8 @@ int arm_paging_cell_init(struct cell *cell) >> if (!cell->arch.mm.root_table) >> return -ENOMEM; >> - return 0; >> + /* Init coloring configuration of the cell */ >> + return coloring_cell_init(cell); > > This is only added in patch 7. > > We should try to preserve bisectability whenever possible. Reorder or > provide stubs so that things remain buildable - and also runnable! > You're right. I will avoid this in next v3. Thanks, Luca >> } >> void arm_paging_cell_destroy(struct cell *cell) >> > > Jan > -- 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/3b5fe52b-9c01-2849-1d4f-a4b674b393b7%40gmail.com.
signature.asc
Description: OpenPGP digital signature
