| Message ID | 20190610193215.23704-16-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: Rework head.S to make it more compliant with the Arm Arm | expand |
On Mon, 10 Jun 2019, Julien Grall wrote: > At the moment, the fixmap table is only hooked when earlyprintk is used. > This is fine today because in C land, the fixmap is not used by anyone > until the the boot CPU is switching to the runtime page-tables. > > In the future, the boot CPU will not switch between page-tables to avoid > TLB conflict. This means the fixmap table will need to be hooked before > any use. For simplicity, setup_fixmap() will now do that job. Can I ask you to reword this slightly, especially the last sentence? It took me a while to understand what you meant. I suggest: In the future, the boot CPU will not switch between page-tables to avoid any TLB conflicts. Thus, the fixmap table will need to be always hooked before any use. Let's start doing it now in setup_fixmap(). Acked-by: Stefano Stabellini <sstabellini@kernel.org> > Lastly, document the behavior and the main registers usage within the > function. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/arm64/head.S | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 96e85f8834..4f7fa6769f 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -700,8 +700,17 @@ id_map_removed: > ret > ENDPROC(remove_id_map) > > +/* > + * Map the UART in the fixmap (when earlyprintk is used) and hook the > + * fixmap table in the page tables. > + * > + * The fixmap cannot be mapped in create_page_tables because it may > + * clash with the ID map. > + * > + * Clobbers x0 - x1 > + */ > setup_fixmap: > -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ > +#ifdef CONFIG_EARLY_PRINTK I am curious why you made this code style change > /* Add UART to the fixmap table */ > ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ > lsr x2, x23, #THIRD_SHIFT > @@ -709,6 +718,7 @@ setup_fixmap: > mov x3, #PT_DEV_L3 > orr x2, x2, x3 /* x2 := 4K dev map including UART */ > str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */ > +#endif > > /* Map fixmap into boot_second */ > ldr x4, =boot_second /* x4 := vaddr (boot_second) */ > @@ -721,7 +731,6 @@ setup_fixmap: > > /* Ensure any page table updates made above have occurred */ > dsb nshst > -#endif > ret > ENDPROC(setup_fixmap) > > -- > 2.11.0 >
On Mon, 10 Jun 2019, Julien Grall wrote: > At the moment, the fixmap table is only hooked when earlyprintk is used. > This is fine today because in C land, the fixmap is not used by anyone > until the the boot CPU is switching to the runtime page-tables. > > In the future, the boot CPU will not switch between page-tables to avoid > TLB conflict. This means the fixmap table will need to be hooked before > any use. For simplicity, setup_fixmap() will now do that job. > > Lastly, document the behavior and the main registers usage within the > function. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/arm64/head.S | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 96e85f8834..4f7fa6769f 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -700,8 +700,17 @@ id_map_removed: > ret > ENDPROC(remove_id_map) > > +/* > + * Map the UART in the fixmap (when earlyprintk is used) and hook the > + * fixmap table in the page tables. > + * > + * The fixmap cannot be mapped in create_page_tables because it may > + * clash with the ID map. > + * > + * Clobbers x0 - x1 I missed this in the last email: it should be x0 - x4? > + */ > setup_fixmap: > -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ > +#ifdef CONFIG_EARLY_PRINTK > /* Add UART to the fixmap table */ > ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ > lsr x2, x23, #THIRD_SHIFT > @@ -709,6 +718,7 @@ setup_fixmap: > mov x3, #PT_DEV_L3 > orr x2, x2, x3 /* x2 := 4K dev map including UART */ > str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */ > +#endif > > /* Map fixmap into boot_second */ > ldr x4, =boot_second /* x4 := vaddr (boot_second) */ > @@ -721,7 +731,6 @@ setup_fixmap: > > /* Ensure any page table updates made above have occurred */ > dsb nshst > -#endif > ret > ENDPROC(setup_fixmap) > > -- > 2.11.0 >
Hi Stefano, On 6/26/19 8:01 PM, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: >> At the moment, the fixmap table is only hooked when earlyprintk is used. >> This is fine today because in C land, the fixmap is not used by anyone >> until the the boot CPU is switching to the runtime page-tables. >> >> In the future, the boot CPU will not switch between page-tables to avoid >> TLB conflict. This means the fixmap table will need to be hooked before >> any use. For simplicity, setup_fixmap() will now do that job. > > Can I ask you to reword this slightly, especially the last sentence? It > took me a while to understand what you meant. I suggest: > > In the future, the boot CPU will not switch between page-tables to > avoid any TLB conflicts. Thus, the fixmap table will need to be always > hooked before any use. Let's start doing it now in setup_fixmap(). > I will update the commit message. > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > >> Lastly, document the behavior and the main registers usage within the >> function. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/arm64/head.S | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 96e85f8834..4f7fa6769f 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -700,8 +700,17 @@ id_map_removed: >> ret >> ENDPROC(remove_id_map) >> >> +/* >> + * Map the UART in the fixmap (when earlyprintk is used) and hook the >> + * fixmap table in the page tables. >> + * >> + * The fixmap cannot be mapped in create_page_tables because it may >> + * clash with the ID map. >> + * >> + * Clobbers x0 - x1 >> + */ >> setup_fixmap: >> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ >> +#ifdef CONFIG_EARLY_PRINTK > > I am curious why you made this code style change This is the only #if defined within head.S (the rest use #ifdef) so for consistency. Also, it is simpler to read :). Cheers,
Hi Stefano, On 26/06/2019 20:02, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: >> At the moment, the fixmap table is only hooked when earlyprintk is used. >> This is fine today because in C land, the fixmap is not used by anyone >> until the the boot CPU is switching to the runtime page-tables. >> >> In the future, the boot CPU will not switch between page-tables to avoid >> TLB conflict. This means the fixmap table will need to be hooked before >> any use. For simplicity, setup_fixmap() will now do that job. >> >> Lastly, document the behavior and the main registers usage within the >> function. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/arm64/head.S | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index 96e85f8834..4f7fa6769f 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -700,8 +700,17 @@ id_map_removed: >> ret >> ENDPROC(remove_id_map) >> >> +/* >> + * Map the UART in the fixmap (when earlyprintk is used) and hook the >> + * fixmap table in the page tables. >> + * >> + * The fixmap cannot be mapped in create_page_tables because it may >> + * clash with the ID map. >> + * >> + * Clobbers x0 - x1 > > I missed this in the last email: it should be x0 - x4? x0 is not used in the setup_fixmap. So it should be x1 - x4. Cheers,
Hi Stefano, On 26/06/2019 20:30, Julien Grall wrote: > On 6/26/19 8:01 PM, Stefano Stabellini wrote: >> On Mon, 10 Jun 2019, Julien Grall wrote: >>> At the moment, the fixmap table is only hooked when earlyprintk is used. >>> This is fine today because in C land, the fixmap is not used by anyone >>> until the the boot CPU is switching to the runtime page-tables. >>> >>> In the future, the boot CPU will not switch between page-tables to avoid >>> TLB conflict. This means the fixmap table will need to be hooked before >>> any use. For simplicity, setup_fixmap() will now do that job. >> >> Can I ask you to reword this slightly, especially the last sentence? It >> took me a while to understand what you meant. I suggest: >> >> In the future, the boot CPU will not switch between page-tables to >> avoid any TLB conflicts. Thus, the fixmap table will need to be always >> hooked before any use. Let's start doing it now in setup_fixmap(). >> > > I will update the commit message. I realized the commit message I wrote is inaccurate and reflected to your rewording. Not all the platforms will generate a TLB conflict abort. Some of them may just decide to use an amalgamation of two entries (see "TLB matching" page D5-2500 in ARM DDI 0487D.b). I will replace "any TLB conflicts" by "TLB incoherency". > >> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > Let me know if you are happy with the change suggested. Cheers,
On Thu, 27 Jun 2019, Julien Grall wrote: > Hi Stefano, > > On 26/06/2019 20:30, Julien Grall wrote: > > On 6/26/19 8:01 PM, Stefano Stabellini wrote: > > > On Mon, 10 Jun 2019, Julien Grall wrote: > > > > At the moment, the fixmap table is only hooked when earlyprintk is used. > > > > This is fine today because in C land, the fixmap is not used by anyone > > > > until the the boot CPU is switching to the runtime page-tables. > > > > > > > > In the future, the boot CPU will not switch between page-tables to avoid > > > > TLB conflict. This means the fixmap table will need to be hooked before > > > > any use. For simplicity, setup_fixmap() will now do that job. > > > > > > Can I ask you to reword this slightly, especially the last sentence? It > > > took me a while to understand what you meant. I suggest: > > > > > > In the future, the boot CPU will not switch between page-tables to > > > avoid any TLB conflicts. Thus, the fixmap table will need to be always > > > hooked before any use. Let's start doing it now in setup_fixmap(). > > > > > > > I will update the commit message. > > I realized the commit message I wrote is inaccurate and reflected to your > rewording. > > Not all the platforms will generate a TLB conflict abort. Some of them may > just decide to use an amalgamation of two entries (see "TLB matching" page > D5-2500 in ARM DDI 0487D.b). > > I will replace "any TLB conflicts" by "TLB incoherency". > > > > > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > > > Let me know if you are happy with the change suggested. Yes, that's fine
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 96e85f8834..4f7fa6769f 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -700,8 +700,17 @@ id_map_removed: ret ENDPROC(remove_id_map) +/* + * Map the UART in the fixmap (when earlyprintk is used) and hook the + * fixmap table in the page tables. + * + * The fixmap cannot be mapped in create_page_tables because it may + * clash with the ID map. + * + * Clobbers x0 - x1 + */ setup_fixmap: -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */ +#ifdef CONFIG_EARLY_PRINTK /* Add UART to the fixmap table */ ldr x1, =xen_fixmap /* x1 := vaddr (xen_fixmap) */ lsr x2, x23, #THIRD_SHIFT @@ -709,6 +718,7 @@ setup_fixmap: mov x3, #PT_DEV_L3 orr x2, x2, x3 /* x2 := 4K dev map including UART */ str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */ +#endif /* Map fixmap into boot_second */ ldr x4, =boot_second /* x4 := vaddr (boot_second) */ @@ -721,7 +731,6 @@ setup_fixmap: /* Ensure any page table updates made above have occurred */ dsb nshst -#endif ret ENDPROC(setup_fixmap)
At the moment, the fixmap table is only hooked when earlyprintk is used. This is fine today because in C land, the fixmap is not used by anyone until the the boot CPU is switching to the runtime page-tables. In the future, the boot CPU will not switch between page-tables to avoid TLB conflict. This means the fixmap table will need to be hooked before any use. For simplicity, setup_fixmap() will now do that job. Lastly, document the behavior and the main registers usage within the function. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/arm64/head.S | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)