| Message ID | 20220221102218.33785-14-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
On 21.02.2022 11:22, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > To use properly the fixmap definitions, their user would need > also new to include <xen/acpi.h>. This is not very great when > the user itself is not meant to directly use ACPI definitions. > > Including <xen/acpi.h> in <asm/config.h> is not option because > the latter header is included by everyone. So move out the fixmap > entries definition in a new header. > > Take the opportunity to also move {set, clear}_fixmap() prototypes > in the new header. > > Note that most of the definitions in <xen/acpi.h> now need to be > surrounded with #ifndef __ASSEMBLY__ because <asm/fixmap.h> will > be used in assembly (see EARLY_UART_VIRTUAL_ADDRESS). > > The split will become more helpful in a follow-up patch where new > fixmap entries will be defined. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Acked-by: Jan Beulich <jbeulich@suse.com>
On Mon, 21 Feb 2022, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > To use properly the fixmap definitions, their user would need > also new to include <xen/acpi.h>. This is not very great when > the user itself is not meant to directly use ACPI definitions. > > Including <xen/acpi.h> in <asm/config.h> is not option because > the latter header is included by everyone. So move out the fixmap > entries definition in a new header. > > Take the opportunity to also move {set, clear}_fixmap() prototypes > in the new header. > > Note that most of the definitions in <xen/acpi.h> now need to be > surrounded with #ifndef __ASSEMBLY__ because <asm/fixmap.h> will > be used in assembly (see EARLY_UART_VIRTUAL_ADDRESS). > > The split will become more helpful in a follow-up patch where new > fixmap entries will be defined. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > Changes in v3: > - Patch added > --- > xen/arch/arm/acpi/lib.c | 2 ++ > xen/arch/arm/include/asm/config.h | 6 ------ > xen/arch/arm/include/asm/early_printk.h | 1 + > xen/arch/arm/include/asm/fixmap.h | 24 ++++++++++++++++++++++++ > xen/arch/arm/include/asm/mm.h | 4 ---- > xen/arch/arm/kernel.c | 1 + > xen/arch/arm/mm.c | 1 + > xen/include/xen/acpi.h | 18 +++++++++++------- > 8 files changed, 40 insertions(+), 17 deletions(-) > create mode 100644 xen/arch/arm/include/asm/fixmap.h > > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c > index a59cc4074cfb..41d521f720ac 100644 > --- a/xen/arch/arm/acpi/lib.c > +++ b/xen/arch/arm/acpi/lib.c > @@ -25,6 +25,8 @@ > #include <xen/init.h> > #include <xen/mm.h> > > +#include <asm/fixmap.h> > + > static bool fixmap_inuse; > > char *__acpi_map_table(paddr_t phys, unsigned long size) > diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h > index 85d4a510ce8a..51908bf9422c 100644 > --- a/xen/arch/arm/include/asm/config.h > +++ b/xen/arch/arm/include/asm/config.h > @@ -175,12 +175,6 @@ > > #endif > > -/* Fixmap slots */ > -#define FIXMAP_CONSOLE 0 /* The primary UART */ > -#define FIXMAP_MISC 1 /* Ephemeral mappings of hardware */ > -#define FIXMAP_ACPI_BEGIN 2 /* Start mappings of ACPI tables */ > -#define FIXMAP_ACPI_END (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1) /* End mappings of ACPI tables */ > - > #define NR_hypercalls 64 > > #define STACK_ORDER 3 > diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h > index 8dc911cf48a3..c5149b2976da 100644 > --- a/xen/arch/arm/include/asm/early_printk.h > +++ b/xen/arch/arm/include/asm/early_printk.h > @@ -11,6 +11,7 @@ > #define __ARM_EARLY_PRINTK_H__ > > #include <xen/page-size.h> > +#include <asm/fixmap.h> > > #ifdef CONFIG_EARLY_PRINTK > > diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h > new file mode 100644 > index 000000000000..1cee51e52ab9 > --- /dev/null > +++ b/xen/arch/arm/include/asm/fixmap.h > @@ -0,0 +1,24 @@ > +/* > + * fixmap.h: compile-time virtual memory allocation > + */ > +#ifndef __ASM_FIXMAP_H > +#define __ASM_FIXMAP_H > + > +#include <xen/acpi.h> > + > +/* Fixmap slots */ > +#define FIXMAP_CONSOLE 0 /* The primary UART */ > +#define FIXMAP_MISC 1 /* Ephemeral mappings of hardware */ > +#define FIXMAP_ACPI_BEGIN 2 /* Start mappings of ACPI tables */ > +#define FIXMAP_ACPI_END (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1) /* End mappings of ACPI tables */ > + > +#ifndef __ASSEMBLY__ > + > +/* Map a page in a fixmap entry */ > +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); > +/* Remove a mapping from a fixmap entry */ > +extern void clear_fixmap(unsigned map); > + > +#endif /* __ASSEMBLY__ */ > + > +#endif /* __ASM_FIXMAP_H */ It is a good idea to create fixmap.h, but I think it should be acpi.h to include fixmap.h, not the other way around. The appended changes build correctly on top of this patch. diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h index 1cee51e52a..8cf9dbb618 100644 --- a/xen/arch/arm/include/asm/fixmap.h +++ b/xen/arch/arm/include/asm/fixmap.h @@ -4,8 +4,6 @@ #ifndef __ASM_FIXMAP_H #define __ASM_FIXMAP_H -#include <xen/acpi.h> - /* Fixmap slots */ #define FIXMAP_CONSOLE 0 /* The primary UART */ #define FIXMAP_MISC 1 /* Ephemeral mappings of hardware */ @@ -14,6 +12,8 @@ #ifndef __ASSEMBLY__ +#include <xen/mm-frame.h> + /* Map a page in a fixmap entry */ extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); /* Remove a mapping from a fixmap entry */ diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index 1b9c75e68f..148673e77c 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -28,6 +28,8 @@ #define _LINUX #endif +#include <asm/fixmap.h> + /* * Fixmap pages to reserve for ACPI boot-time tables (see * arch/x86/include/asm/fixmap.h or arch/arm/include/asm/fixmap.h),
Hi Stefano, On 05/04/2022 22:12, Stefano Stabellini wrote: >> +/* Map a page in a fixmap entry */ >> +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); >> +/* Remove a mapping from a fixmap entry */ >> +extern void clear_fixmap(unsigned map); >> + >> +#endif /* __ASSEMBLY__ */ >> + >> +#endif /* __ASM_FIXMAP_H */ > > > It is a good idea to create fixmap.h, but I think it should be acpi.h to > include fixmap.h, not the other way around. As I wrote in the commit message, one definition in fixmap.h rely on define from acpi.h (i.e NUM_FIXMAP_ACPI_PAGES). So if we don't include it, then user of FIXMAP_PMAP_BEGIN (see next patch) will requires to include acpi.h in order to build. Re-ordering the values would not help because the problem would exactly be the same but this time the acpi users would have to include pmap.h to define NUM_FIX_PMAP. > > The appended changes build correctly on top of this patch. That's expected because all the users of FIXMAP_ACPI_END will be including acpi.h. But after the next patch, we would need pmap.c to include acpi.h. I don't think this would be right (and quite likely you would ask why this is done). Hence this approach. Cheers,
On Tue, 5 Apr 2022, Julien Grall wrote: > On 05/04/2022 22:12, Stefano Stabellini wrote: > > > +/* Map a page in a fixmap entry */ > > > +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); > > > +/* Remove a mapping from a fixmap entry */ > > > +extern void clear_fixmap(unsigned map); > > > + > > > +#endif /* __ASSEMBLY__ */ > > > + > > > +#endif /* __ASM_FIXMAP_H */ > > > > > > It is a good idea to create fixmap.h, but I think it should be acpi.h to > > include fixmap.h, not the other way around. > > As I wrote in the commit message, one definition in fixmap.h rely on define > from acpi.h (i.e NUM_FIXMAP_ACPI_PAGES). So if we don't include it, then user > of FIXMAP_PMAP_BEGIN (see next patch) will requires to include acpi.h in order > to build. > > Re-ordering the values would not help because the problem would exactly be the > same but this time the acpi users would have to include pmap.h to define > NUM_FIX_PMAP. > > > > > The appended changes build correctly on top of this patch. > > That's expected because all the users of FIXMAP_ACPI_END will be including > acpi.h. But after the next patch, we would need pmap.c to include acpi.h. > > I don't think this would be right (and quite likely you would ask why > this is done). Hence this approach. I premise that I see your point and I don't feel very strongly either way. In my opinion the fixmap is the low level "library" that others make use of, so it should be acpi.h and pmap.h (the clients of the library) that include fixmap.h and not the other way around. So I would rather define NUM_FIXMAP_ACPI_PAGES and NUM_FIX_PMAP in fixmap.h, then have both pmap.h and acpi.h include fixmap.h. It makes more sense to me. However, I won't insist if you don't like it. Rough patch below for reference. diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h index c46a15e59d..a231ebfe25 100644 --- a/xen/arch/arm/include/asm/fixmap.h +++ b/xen/arch/arm/include/asm/fixmap.h @@ -4,8 +4,13 @@ #ifndef __ASM_FIXMAP_H #define __ASM_FIXMAP_H -#include <xen/acpi.h> -#include <xen/pmap.h> +#include <xen/lib.h> +#include <asm/lpae.h> + +#define NUM_FIXMAP_ACPI_PAGES 64 + +/* Large enough for mapping 5 levels of page tables with some headroom */ +#define NUM_FIX_PMAP 8 /* Fixmap slots */ #define FIXMAP_CONSOLE 0 /* The primary UART */ @@ -22,6 +27,10 @@ #ifndef __ASSEMBLY__ +#include <xen/mm-frame.h> + +extern lpae_t xen_fixmap[XEN_PT_LPAE_ENTRIES]; + /* Map a page in a fixmap entry */ extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); /* Remove a mapping from a fixmap entry */ diff --git a/xen/arch/arm/include/asm/pmap.h b/xen/arch/arm/include/asm/pmap.h index 70eafe2891..31d29e021d 100644 --- a/xen/arch/arm/include/asm/pmap.h +++ b/xen/arch/arm/include/asm/pmap.h @@ -2,9 +2,8 @@ #define __ASM_PMAP_H__ #include <xen/mm.h> +#include <asm/fixmap.h> -/* XXX: Find an header to declare it */ -extern lpae_t xen_fixmap[XEN_PT_LPAE_ENTRIES]; static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) { diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index 1b9c75e68f..afcc9d5b4f 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -28,12 +28,7 @@ #define _LINUX #endif -/* - * Fixmap pages to reserve for ACPI boot-time tables (see - * arch/x86/include/asm/fixmap.h or arch/arm/include/asm/fixmap.h), - * 64 pages(256KB) is large enough for most cases.) - */ -#define NUM_FIXMAP_ACPI_PAGES 64 +#include <asm/fixmap.h> #ifndef __ASSEMBLY__ diff --git a/xen/include/xen/pmap.h b/xen/include/xen/pmap.h index 93e61b1087..aa892154c0 100644 --- a/xen/include/xen/pmap.h +++ b/xen/include/xen/pmap.h @@ -1,9 +1,6 @@ #ifndef __XEN_PMAP_H__ #define __XEN_PMAP_H__ -/* Large enough for mapping 5 levels of page tables with some headroom */ -#define NUM_FIX_PMAP 8 - #ifndef __ASSEMBLY__ #include <xen/mm-frame.h>
Hi, On 06/04/2022 01:10, Stefano Stabellini wrote: > On Tue, 5 Apr 2022, Julien Grall wrote: >> On 05/04/2022 22:12, Stefano Stabellini wrote: >>>> +/* Map a page in a fixmap entry */ >>>> +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); >>>> +/* Remove a mapping from a fixmap entry */ >>>> +extern void clear_fixmap(unsigned map); >>>> + >>>> +#endif /* __ASSEMBLY__ */ >>>> + >>>> +#endif /* __ASM_FIXMAP_H */ >>> >>> >>> It is a good idea to create fixmap.h, but I think it should be acpi.h to >>> include fixmap.h, not the other way around. >> >> As I wrote in the commit message, one definition in fixmap.h rely on define >> from acpi.h (i.e NUM_FIXMAP_ACPI_PAGES). So if we don't include it, then user >> of FIXMAP_PMAP_BEGIN (see next patch) will requires to include acpi.h in order >> to build. >> >> Re-ordering the values would not help because the problem would exactly be the >> same but this time the acpi users would have to include pmap.h to define >> NUM_FIX_PMAP. >> >>> >>> The appended changes build correctly on top of this patch. >> >> That's expected because all the users of FIXMAP_ACPI_END will be including >> acpi.h. But after the next patch, we would need pmap.c to include acpi.h. >> >> I don't think this would be right (and quite likely you would ask why >> this is done). Hence this approach. > > > I premise that I see your point and I don't feel very strongly either > way. In my opinion the fixmap is the low level "library" that others > make use of, so it should be acpi.h and pmap.h (the clients of the > library) that include fixmap.h and not the other way around. That's one way to see it. The way I see it is each component decide how much entries they need. So I think it is better to move the definition to each components as they are best suited to decide the value. > However, I won't insist if you don't like it. Rough > patch below for reference. I want to stay close to what x86 is doing to avoid any headers mess. This is what my patch is doing. Now, if x86 folks are happy to move the definition per-arch or in a xen/fixmap.h. Then I can look at it. Andrew, Jan, Roger, Wei, what do you think? Cheers,
diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c index a59cc4074cfb..41d521f720ac 100644 --- a/xen/arch/arm/acpi/lib.c +++ b/xen/arch/arm/acpi/lib.c @@ -25,6 +25,8 @@ #include <xen/init.h> #include <xen/mm.h> +#include <asm/fixmap.h> + static bool fixmap_inuse; char *__acpi_map_table(paddr_t phys, unsigned long size) diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index 85d4a510ce8a..51908bf9422c 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -175,12 +175,6 @@ #endif -/* Fixmap slots */ -#define FIXMAP_CONSOLE 0 /* The primary UART */ -#define FIXMAP_MISC 1 /* Ephemeral mappings of hardware */ -#define FIXMAP_ACPI_BEGIN 2 /* Start mappings of ACPI tables */ -#define FIXMAP_ACPI_END (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1) /* End mappings of ACPI tables */ - #define NR_hypercalls 64 #define STACK_ORDER 3 diff --git a/xen/arch/arm/include/asm/early_printk.h b/xen/arch/arm/include/asm/early_printk.h index 8dc911cf48a3..c5149b2976da 100644 --- a/xen/arch/arm/include/asm/early_printk.h +++ b/xen/arch/arm/include/asm/early_printk.h @@ -11,6 +11,7 @@ #define __ARM_EARLY_PRINTK_H__ #include <xen/page-size.h> +#include <asm/fixmap.h> #ifdef CONFIG_EARLY_PRINTK diff --git a/xen/arch/arm/include/asm/fixmap.h b/xen/arch/arm/include/asm/fixmap.h new file mode 100644 index 000000000000..1cee51e52ab9 --- /dev/null +++ b/xen/arch/arm/include/asm/fixmap.h @@ -0,0 +1,24 @@ +/* + * fixmap.h: compile-time virtual memory allocation + */ +#ifndef __ASM_FIXMAP_H +#define __ASM_FIXMAP_H + +#include <xen/acpi.h> + +/* Fixmap slots */ +#define FIXMAP_CONSOLE 0 /* The primary UART */ +#define FIXMAP_MISC 1 /* Ephemeral mappings of hardware */ +#define FIXMAP_ACPI_BEGIN 2 /* Start mappings of ACPI tables */ +#define FIXMAP_ACPI_END (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1) /* End mappings of ACPI tables */ + +#ifndef __ASSEMBLY__ + +/* Map a page in a fixmap entry */ +extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); +/* Remove a mapping from a fixmap entry */ +extern void clear_fixmap(unsigned map); + +#endif /* __ASSEMBLY__ */ + +#endif /* __ASM_FIXMAP_H */ diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 424aaf28230b..045a8ba4bb63 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -191,10 +191,6 @@ extern void mmu_init_secondary_cpu(void); extern void setup_xenheap_mappings(unsigned long base_mfn, unsigned long nr_mfns); /* Map a frame table to cover physical addresses ps through pe */ extern void setup_frametable_mappings(paddr_t ps, paddr_t pe); -/* Map a 4k page in a fixmap entry */ -extern void set_fixmap(unsigned map, mfn_t mfn, unsigned attributes); -/* Remove a mapping from a fixmap entry */ -extern void clear_fixmap(unsigned map); /* map a physical range in virtual memory */ void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned attributes); diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 8f43caa1866d..25ded1c056d9 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -15,6 +15,7 @@ #include <xen/vmap.h> #include <asm/byteorder.h> +#include <asm/fixmap.h> #include <asm/kernel.h> #include <asm/setup.h> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index f70b8cc7ce87..d6a4b9407c43 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -41,6 +41,7 @@ #include <xen/sizes.h> #include <xen/libfdt/libfdt.h> +#include <asm/fixmap.h> #include <asm/setup.h> /* Override macros from asm/page.h to make them work with mfn_t */ diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index 08834f140266..500aaa538551 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -28,6 +28,15 @@ #define _LINUX #endif +/* + * Fixmap pages to reserve for ACPI boot-time tables (see + * arch/x86/include/asm/fixmap.h or arch/arm/include/asm/fixmap.h), + * 64 pages(256KB) is large enough for most cases.) + */ +#define NUM_FIXMAP_ACPI_PAGES 64 + +#ifndef __ASSEMBLY__ + #include <xen/list.h> #include <acpi/acpi.h> @@ -39,13 +48,6 @@ #define ACPI_MADT_GET_POLARITY(inti) ACPI_MADT_GET_(POLARITY, inti) #define ACPI_MADT_GET_TRIGGER(inti) ACPI_MADT_GET_(TRIGGER, inti) -/* - * Fixmap pages to reserve for ACPI boot-time tables (see - * arch/x86/include/asm/fixmap.h or arch/arm/include/asm/config.h, - * 64 pages(256KB) is large enough for most cases.) - */ -#define NUM_FIXMAP_ACPI_PAGES 64 - #define BAD_MADT_ENTRY(entry, end) ( \ (!(entry)) || (unsigned long)(entry) + sizeof(*(entry)) > (end) || \ (entry)->header.length < sizeof(*(entry))) @@ -207,4 +209,6 @@ void acpi_reboot(void); void acpi_dmar_zap(void); void acpi_dmar_reinstate(void); +#endif /* __ASSEMBLY__ */ + #endif /*_LINUX_ACPI_H*/