| Message ID | 20230626033443.2943270-13-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
Hi Penny, Title: You want to clarify that this change is arm64 only. So: xen/arm64: mmu: ... On 26/06/2023 04:34, Penny Zheng wrote: > Original setup_fixmap is actually doing two seperate tasks, one is enabling > the early UART when earlyprintk on, and the other is to set up the fixmap > even when earlyprintk is not configured. > > To be more dedicated and precise, the old function shall be split into two > functions, setup_early_uart and new setup_fixmap. While some of the split before would be warrant even without the MPU support. This one is not because there is limited point to have 3 lines function. So I think you want to justify based on what you plan to do with the MPU code. That said, I don't think we need to introduce setup_fixmap. See below. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > v3: > - new patch > --- > xen/arch/arm/arm64/head.S | 3 +++ > xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++------- > 2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index e63886b037..55a4cfe69e 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -258,7 +258,10 @@ real_start_efi: > b enable_boot_mm > > primary_switched: > + bl setup_early_uart > +#ifdef CONFIG_HAS_FIXMAP > bl setup_fixmap > +#endif > #ifdef CONFIG_EARLY_PRINTK > /* Use a virtual address to access the UART. */ > ldr x23, =EARLY_UART_VIRTUAL_ADDRESS > diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S > index 2b209fc3ce..295596aca1 100644 > --- a/xen/arch/arm/arm64/mmu/head.S > +++ b/xen/arch/arm/arm64/mmu/head.S > @@ -367,24 +367,34 @@ identity_mapping_removed: > ENDPROC(remove_identity_mapping) > > /* > - * 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 1:1 mapping. Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), there is no chance that the fixmap will clash with the 1:1 mapping. So rather than introducing a new function, I would move the creation of the fixmap in create_pagetables(). This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S. > + * Map the UART in the fixmap (when earlyprintk is used) > * > * Inputs: > - * x20: Physical offset > * x23: Early UART base physical address > * > * Clobbers x0 - x3 > */ > -ENTRY(setup_fixmap) > +ENTRY(setup_early_uart) > #ifdef CONFIG_EARLY_PRINTK > /* Add UART to the fixmap table */ > ldr x0, =EARLY_UART_VIRTUAL_ADDRESS > create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 > + /* Ensure any page table updates made above have occurred. */ > + dsb nshst > + > + ret The 'ret' needs to be outside of the '#ifdef' block. But, in this case, I would prefer if we don't call setup_early_uart() when !CONFIG_EARLY_PRINTK in head.S > #endif > +ENDPROC(setup_early_uart) > + > +/* > + * Map the fixmap table in the page tables. > + * > + * The fixmap cannot be mapped in create_page_tables because it may > + * clash with the 1:1 mapping. > + * > + * Clobbers x0 - x3 > + */ > +ENTRY(setup_fixmap) > /* Map fixmap into boot_second */ > ldr x0, =FIXMAP_ADDR(0) > create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 Cheeers,
Hi Julien On 2023/7/5 06:25, Julien Grall wrote: > Hi Penny, > > Title: You want to clarify that this change is arm64 only. So: > > xen/arm64: mmu: ... > > On 26/06/2023 04:34, Penny Zheng wrote: >> Original setup_fixmap is actually doing two seperate tasks, one is >> enabling >> the early UART when earlyprintk on, and the other is to set up the fixmap >> even when earlyprintk is not configured. >> >> To be more dedicated and precise, the old function shall be split into >> two >> functions, setup_early_uart and new setup_fixmap. > While some of the split before would be warrant even without the MPU > support. This one is not because there is limited point to have 3 lines > function. So I think you want to justify based on what you plan to do > with the MPU code. > > That said, I don't think we need to introduce setup_fixmap. See below. > > >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> --- >> v3: >> - new patch >> --- >> xen/arch/arm/arm64/head.S | 3 +++ >> xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++------- >> 2 files changed, 20 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index e63886b037..55a4cfe69e 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -258,7 +258,10 @@ real_start_efi: >> b enable_boot_mm >> primary_switched: >> + bl setup_early_uart >> +#ifdef CONFIG_HAS_FIXMAP >> bl setup_fixmap >> +#endif >> #ifdef CONFIG_EARLY_PRINTK >> /* Use a virtual address to access the UART. */ >> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >> diff --git a/xen/arch/arm/arm64/mmu/head.S >> b/xen/arch/arm/arm64/mmu/head.S >> index 2b209fc3ce..295596aca1 100644 >> --- a/xen/arch/arm/arm64/mmu/head.S >> +++ b/xen/arch/arm/arm64/mmu/head.S >> @@ -367,24 +367,34 @@ identity_mapping_removed: >> ENDPROC(remove_identity_mapping) >> /* >> - * 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 1:1 mapping. > > Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), there > is no chance that the fixmap will clash with the 1:1 mapping. So rather > than introducing a new function, I would move the creation of the fixmap > in create_pagetables(). > Understood. I'll move the creation of the fixmap in create_pagetables(). > This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S. > >> + * Map the UART in the fixmap (when earlyprintk is used) >> * >> * Inputs: >> - * x20: Physical offset >> * x23: Early UART base physical address >> * >> * Clobbers x0 - x3 >> */ >> -ENTRY(setup_fixmap) >> +ENTRY(setup_early_uart) >> #ifdef CONFIG_EARLY_PRINTK >> /* Add UART to the fixmap table */ >> ldr x0, =EARLY_UART_VIRTUAL_ADDRESS >> create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, >> type=PT_DEV_L3 >> + /* Ensure any page table updates made above have occurred. */ >> + dsb nshst >> + >> + ret > > The 'ret' needs to be outside of the '#ifdef' block. But, in this case, > I would prefer if we don't call setup_early_uart() when > !CONFIG_EARLY_PRINTK in head.S > okay. I'll move the #ifdef to the caller in head.S. >> #endif >> +ENDPROC(setup_early_uart) >> + >> +/* >> + * Map the fixmap table in the page tables. >> + * >> + * The fixmap cannot be mapped in create_page_tables because it may >> + * clash with the 1:1 mapping. >> + * >> + * Clobbers x0 - x3 >> + */ >> +ENTRY(setup_fixmap) >> /* Map fixmap into boot_second */ >> ldr x0, =FIXMAP_ADDR(0) >> create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3 > > Cheeers, >
Hi Penny, On 05/07/2023 10:03, Penny Zheng wrote: > On 2023/7/5 06:25, Julien Grall wrote: >> Hi Penny, >> >> Title: You want to clarify that this change is arm64 only. So: >> >> xen/arm64: mmu: ... >> >> On 26/06/2023 04:34, Penny Zheng wrote: >>> Original setup_fixmap is actually doing two seperate tasks, one is >>> enabling >>> the early UART when earlyprintk on, and the other is to set up the >>> fixmap >>> even when earlyprintk is not configured. >>> >>> To be more dedicated and precise, the old function shall be split >>> into two >>> functions, setup_early_uart and new setup_fixmap. >> While some of the split before would be warrant even without the MPU >> support. This one is not because there is limited point to have 3 >> lines function. So I think you want to justify based on what you plan >> to do with the MPU code. >> >> That said, I don't think we need to introduce setup_fixmap. See below. >> >> >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>> --- >>> v3: >>> - new patch >>> --- >>> xen/arch/arm/arm64/head.S | 3 +++ >>> xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++------- >>> 2 files changed, 20 insertions(+), 7 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index e63886b037..55a4cfe69e 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -258,7 +258,10 @@ real_start_efi: >>> b enable_boot_mm >>> primary_switched: >>> + bl setup_early_uart >>> +#ifdef CONFIG_HAS_FIXMAP >>> bl setup_fixmap >>> +#endif >>> #ifdef CONFIG_EARLY_PRINTK >>> /* Use a virtual address to access the UART. */ >>> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >>> diff --git a/xen/arch/arm/arm64/mmu/head.S >>> b/xen/arch/arm/arm64/mmu/head.S >>> index 2b209fc3ce..295596aca1 100644 >>> --- a/xen/arch/arm/arm64/mmu/head.S >>> +++ b/xen/arch/arm/arm64/mmu/head.S >>> @@ -367,24 +367,34 @@ identity_mapping_removed: >>> ENDPROC(remove_identity_mapping) >>> /* >>> - * 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 1:1 mapping. >> >> Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), >> there is no chance that the fixmap will clash with the 1:1 mapping. So >> rather than introducing a new function, I would move the creation of >> the fixmap in create_pagetables(). >> > > Understood. I'll move the creation of the fixmap in create_pagetables(). > >> This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S. >> >>> + * Map the UART in the fixmap (when earlyprintk is used) >>> * >>> * Inputs: >>> - * x20: Physical offset >>> * x23: Early UART base physical address >>> * >>> * Clobbers x0 - x3 >>> */ >>> -ENTRY(setup_fixmap) >>> +ENTRY(setup_early_uart) >>> #ifdef CONFIG_EARLY_PRINTK >>> /* Add UART to the fixmap table */ >>> ldr x0, =EARLY_UART_VIRTUAL_ADDRESS >>> create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, >>> type=PT_DEV_L3 >>> + /* Ensure any page table updates made above have occurred. */ >>> + dsb nshst >>> + >>> + ret >> >> The 'ret' needs to be outside of the '#ifdef' block. But, in this >> case, I would prefer if we don't call setup_early_uart() when >> !CONFIG_EARLY_PRINTK in head.S >> > > okay. I'll move the #ifdef to the caller in head.S. Thinking about this again. I think you can actually move the mapping of the UART in create_pagetables() because it will also not clash with the 1:1. For the MPU, the mapping could then be moved in prepare_early_mappings(). This would reduce the number of functions exposed. Cheers,
Hi Julien On 2023/7/5 18:35, Julien Grall wrote: > Hi Penny, > > On 05/07/2023 10:03, Penny Zheng wrote: >> On 2023/7/5 06:25, Julien Grall wrote: >>> Hi Penny, >>> >>> Title: You want to clarify that this change is arm64 only. So: >>> >>> xen/arm64: mmu: ... >>> >>> On 26/06/2023 04:34, Penny Zheng wrote: >>>> Original setup_fixmap is actually doing two seperate tasks, one is >>>> enabling >>>> the early UART when earlyprintk on, and the other is to set up the >>>> fixmap >>>> even when earlyprintk is not configured. >>>> >>>> To be more dedicated and precise, the old function shall be split >>>> into two >>>> functions, setup_early_uart and new setup_fixmap. >>> While some of the split before would be warrant even without the MPU >>> support. This one is not because there is limited point to have 3 >>> lines function. So I think you want to justify based on what you plan >>> to do with the MPU code. >>> >>> That said, I don't think we need to introduce setup_fixmap. See below. >>> >>> >>>> >>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>>> Signed-off-by: Wei Chen <wei.chen@arm.com> >>>> --- >>>> v3: >>>> - new patch >>>> --- >>>> xen/arch/arm/arm64/head.S | 3 +++ >>>> xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++------- >>>> 2 files changed, 20 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>>> index e63886b037..55a4cfe69e 100644 >>>> --- a/xen/arch/arm/arm64/head.S >>>> +++ b/xen/arch/arm/arm64/head.S >>>> @@ -258,7 +258,10 @@ real_start_efi: >>>> b enable_boot_mm >>>> primary_switched: >>>> + bl setup_early_uart >>>> +#ifdef CONFIG_HAS_FIXMAP >>>> bl setup_fixmap >>>> +#endif >>>> #ifdef CONFIG_EARLY_PRINTK >>>> /* Use a virtual address to access the UART. */ >>>> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS >>>> diff --git a/xen/arch/arm/arm64/mmu/head.S >>>> b/xen/arch/arm/arm64/mmu/head.S >>>> index 2b209fc3ce..295596aca1 100644 >>>> --- a/xen/arch/arm/arm64/mmu/head.S >>>> +++ b/xen/arch/arm/arm64/mmu/head.S >>>> @@ -367,24 +367,34 @@ identity_mapping_removed: >>>> ENDPROC(remove_identity_mapping) >>>> /* >>>> - * 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 1:1 mapping. >>> >>> Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), >>> there is no chance that the fixmap will clash with the 1:1 mapping. >>> So rather than introducing a new function, I would move the creation >>> of the fixmap in create_pagetables(). >>> >> >> Understood. I'll move the creation of the fixmap in create_pagetables(). >> >>> This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S. >>> >>>> + * Map the UART in the fixmap (when earlyprintk is used) >>>> * >>>> * Inputs: >>>> - * x20: Physical offset >>>> * x23: Early UART base physical address >>>> * >>>> * Clobbers x0 - x3 >>>> */ >>>> -ENTRY(setup_fixmap) >>>> +ENTRY(setup_early_uart) >>>> #ifdef CONFIG_EARLY_PRINTK >>>> /* Add UART to the fixmap table */ >>>> ldr x0, =EARLY_UART_VIRTUAL_ADDRESS >>>> create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, >>>> type=PT_DEV_L3 >>>> + /* Ensure any page table updates made above have occurred. */ >>>> + dsb nshst >>>> + >>>> + ret >>> >>> The 'ret' needs to be outside of the '#ifdef' block. But, in this >>> case, I would prefer if we don't call setup_early_uart() when >>> !CONFIG_EARLY_PRINTK in head.S >>> >> >> okay. I'll move the #ifdef to the caller in head.S. > > Thinking about this again. I think you can actually move the mapping of > the UART in create_pagetables() because it will also not clash with the > 1:1. > > For the MPU, the mapping could then be moved in prepare_early_mappings(). > > This would reduce the number of functions exposed. Understood! will do. > > Cheers, >
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index e63886b037..55a4cfe69e 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -258,7 +258,10 @@ real_start_efi: b enable_boot_mm primary_switched: + bl setup_early_uart +#ifdef CONFIG_HAS_FIXMAP bl setup_fixmap +#endif #ifdef CONFIG_EARLY_PRINTK /* Use a virtual address to access the UART. */ ldr x23, =EARLY_UART_VIRTUAL_ADDRESS diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S index 2b209fc3ce..295596aca1 100644 --- a/xen/arch/arm/arm64/mmu/head.S +++ b/xen/arch/arm/arm64/mmu/head.S @@ -367,24 +367,34 @@ identity_mapping_removed: ENDPROC(remove_identity_mapping) /* - * 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 1:1 mapping. + * Map the UART in the fixmap (when earlyprintk is used) * * Inputs: - * x20: Physical offset * x23: Early UART base physical address * * Clobbers x0 - x3 */ -ENTRY(setup_fixmap) +ENTRY(setup_early_uart) #ifdef CONFIG_EARLY_PRINTK /* Add UART to the fixmap table */ ldr x0, =EARLY_UART_VIRTUAL_ADDRESS create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3 + /* Ensure any page table updates made above have occurred. */ + dsb nshst + + ret #endif +ENDPROC(setup_early_uart) + +/* + * Map the fixmap table in the page tables. + * + * The fixmap cannot be mapped in create_page_tables because it may + * clash with the 1:1 mapping. + * + * Clobbers x0 - x3 + */ +ENTRY(setup_fixmap) /* Map fixmap into boot_second */ ldr x0, =FIXMAP_ADDR(0) create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3