| Message ID | 20220826125111.152261-10-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
Hi Carlo, On 26/08/2022 13:51, Carlo Nonato wrote: > From: Luca Miccio <lucmiccio@gmail.com> > > This reverts commit 3a5d341681af650825bbe3bee9be5d187da35080. Usually, this indicates that this was a clean revert. IOW, there was no clash or modification necessary. Looking at the diff below, this doesn't look to be the case because you are also reverting f8c818848fa6 "xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()" and introduce a new version of create_boot_mappings(). So I think the commit message/title should be reworded to explain this is not a clean revert and what extra changes were made. But see below about re-introducing create_boot_mapping(). > > The cache coloring support will be configurable within the Xen command line, > but it will be initialized before the page-tables; this is necessary > for coloring the hypervisor itself beacuse we will create a specific > mapping for it that could be configured using some command line options. > In order to parse all the needed information from the device tree, we > need to revert the above commit and restore the previous order for > page-tables initialization. > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > --- > xen/arch/arm/mm.c | 33 ++++++++++++++++++++------------- > xen/arch/arm/setup.c | 4 ++-- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index b42cddb1b4..1afa02b4af 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -368,6 +368,17 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va) > return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); > } > > +static void __init create_boot_mappings(unsigned long virt_offset, > + mfn_t base_mfn) > +{ > + lpae_t pte; > + > + pte = mfn_to_xen_entry(base_mfn, MT_NORMAL); > + write_pte(&boot_second[second_table_offset(virt_offset)], pte); > + flush_xen_tlb_local(); > +} Please don't introduce a new function that create mappings. All mappings should be done using map_pages_to_xen(). Looking at the implementation, it should be usable with the following diff: diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index c81c706c8b23..78afb8eb0ec1 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1104,7 +1104,7 @@ static int xen_pt_update(unsigned long virt, * * XXX: Add a check. */ - const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE); + const mfn_t root = maddr_to_mfn(READ_SYSREG64(TTBR0_EL2)); /* * The hardware was configured to forbid mapping both writeable and With that there is no change required in early_fdt_map() and ... > > + /* ... DTB */ > + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)]; > + xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte; > + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)]; > + xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte; > + ... rather than copying the 2 entries, you could call early_fdt_map() after the switch. The advantage is it will avoid to hardoded more page-table entries. As part of my switch_ttbr() rework, I am planning to re-introduce relocation (at least for testing). So I will include the changes I mention above in my series. Cheers,
Hi Julien, On Sat, Sep 10, 2022 at 4:29 PM Julien Grall <julien@xen.org> wrote: > > Hi Carlo, > > On 26/08/2022 13:51, Carlo Nonato wrote: > > From: Luca Miccio <lucmiccio@gmail.com> > > > > This reverts commit 3a5d341681af650825bbe3bee9be5d187da35080. > > Usually, this indicates that this was a clean revert. IOW, there was no > clash or modification necessary. Looking at the diff below, this doesn't > look to be the case because you are also reverting f8c818848fa6 > "xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()" > and introduce a new version of create_boot_mappings(). > > So I think the commit message/title should be reworded to explain this > is not a clean revert and what extra changes were made. > > But see below about re-introducing create_boot_mapping(). > > > > > The cache coloring support will be configurable within the Xen command line, > > but it will be initialized before the page-tables; this is necessary > > for coloring the hypervisor itself beacuse we will create a specific > > mapping for it that could be configured using some command line options. > > In order to parse all the needed information from the device tree, we > > need to revert the above commit and restore the previous order for > > page-tables initialization. > > > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > > Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech> > > --- > > xen/arch/arm/mm.c | 33 ++++++++++++++++++++------------- > > xen/arch/arm/setup.c | 4 ++-- > > 2 files changed, 22 insertions(+), 15 deletions(-) > > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > > index b42cddb1b4..1afa02b4af 100644 > > --- a/xen/arch/arm/mm.c > > +++ b/xen/arch/arm/mm.c > > @@ -368,6 +368,17 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va) > > return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); > > } > > > > +static void __init create_boot_mappings(unsigned long virt_offset, > > + mfn_t base_mfn) > > +{ > > + lpae_t pte; > > + > > + pte = mfn_to_xen_entry(base_mfn, MT_NORMAL); > > + write_pte(&boot_second[second_table_offset(virt_offset)], pte); > > + flush_xen_tlb_local(); > > +} > Please don't introduce a new function that create mappings. All mappings > should be done using map_pages_to_xen(). Looking at the implementation, > it should be usable with the following diff: > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index c81c706c8b23..78afb8eb0ec1 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1104,7 +1104,7 @@ static int xen_pt_update(unsigned long virt, > * > * XXX: Add a check. > */ > - const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE); > + const mfn_t root = maddr_to_mfn(READ_SYSREG64(TTBR0_EL2)); > > /* > * The hardware was configured to forbid mapping both writeable and > > With that there is no change required in early_fdt_map() and ... > > > > > + /* ... DTB */ > > + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)]; > > + xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte; > > + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)]; > > + xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte; > > + > > ... rather than copying the 2 entries, you could call early_fdt_map() > after the switch. The advantage is it will avoid to hardoded more > page-table entries. Thanks for the diff and the suggestions. I just tested it and it works properly! Nice. > > As part of my switch_ttbr() rework, I am planning to re-introduce > relocation (at least for testing). So I will include the changes I > mention above in my series. > > Cheers, > > -- > Julien Grall Thanks. - Carlo Nonato
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b42cddb1b4..1afa02b4af 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -368,6 +368,17 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va) return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL); } +static void __init create_boot_mappings(unsigned long virt_offset, + mfn_t base_mfn) +{ + lpae_t pte; + + pte = mfn_to_xen_entry(base_mfn, MT_NORMAL); + write_pte(&boot_second[second_table_offset(virt_offset)], pte); + flush_xen_tlb_local(); +} + +/* Map the FDT in the early boot page table */ void * __init early_fdt_map(paddr_t fdt_paddr) { /* We are using 2MB superpage for mapping the FDT */ @@ -375,7 +386,6 @@ void * __init early_fdt_map(paddr_t fdt_paddr) paddr_t offset; void *fdt_virt; uint32_t size; - int rc; /* * Check whether the physical FDT address is set and meets the minimum @@ -391,12 +401,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr) /* The FDT is mapped using 2MB superpage */ BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M); - rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr), - SZ_2M >> PAGE_SHIFT, - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); - if ( rc ) - panic("Unable to map the device-tree.\n"); - + create_boot_mappings(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr)); offset = fdt_paddr % SECOND_SIZE; fdt_virt = (void *)BOOT_FDT_VIRT_START + offset; @@ -410,12 +415,8 @@ void * __init early_fdt_map(paddr_t fdt_paddr) if ( (offset + size) > SZ_2M ) { - rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M, - maddr_to_mfn(base_paddr + SZ_2M), - SZ_2M >> PAGE_SHIFT, - PAGE_HYPERVISOR_RO | _PAGE_BLOCK); - if ( rc ) - panic("Unable to map the device-tree\n"); + create_boot_mappings(BOOT_FDT_VIRT_START + SZ_2M, + maddr_to_mfn(base_paddr + SZ_2M)); } return fdt_virt; @@ -514,6 +515,12 @@ void __init setup_pagetables(unsigned long boot_phys_offset) pte.pt.table = 1; xen_second[second_table_offset(FIXMAP_ADDR(0))] = pte; + /* ... DTB */ + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)]; + xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte; + pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)]; + xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte; + #ifdef CONFIG_ARM_64 ttbr = (uintptr_t) xen_pgtable + phys_offset; #else diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 611c93ad7d..bdfc05bf61 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -910,8 +910,6 @@ void __init start_xen(unsigned long boot_phys_offset, /* Initialize traps early allow us to get backtrace when an error occurred */ init_traps(); - setup_pagetables(boot_phys_offset); - smp_clear_cpu_maps(); device_tree_flattened = early_fdt_map(fdt_paddr); @@ -938,6 +936,8 @@ void __init start_xen(unsigned long boot_phys_offset, (paddr_t)(uintptr_t)(_end - _start + 1), false); BUG_ON(!xen_bootmodule); + setup_pagetables(boot_phys_offset); + setup_mm(); /* Parse the ACPI tables for possible boot-time configuration */