[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 13/19] xen/arm: Move fixmap definitions in a separate header



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>



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.