[2/2] mm: split out mfn_t / gfn_t / pfn_t definitions and helpers

Commit Message

Jan Beulich Dec. 2, 2020, 2:50 p.m. UTC

xen/mm.h has heavy dependencies, while in a number of cases only these
type definitions are needed. This separation then also allows pulling in
these definitions when including xen/mm.h would cause cyclic
dependencies.

Replace xen/mm.h inclusion where possible in include/xen/. (In
xen/iommu.h also take the opportunity and correct the few remaining
sorting issues.)

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Julien Grall Dec. 2, 2020, 5:35 p.m. UTC | #1

Hi Jan,

On 02/12/2020 14:50, Jan Beulich wrote:
> xen/mm.h has heavy dependencies, while in a number of cases only these
> type definitions are needed. This separation then also allows pulling in
> these definitions when including xen/mm.h would cause cyclic
> dependencies.
> 
> Replace xen/mm.h inclusion where possible in include/xen/. (In
> xen/iommu.h also take the opportunity and correct the few remaining
> sorting issues.)
> 
> Signed-off-by: Jan Beulich <[e-mail]>
> 
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -10,7 +10,6 @@
>    * Slimmed with Xen specific support.
>    */
>   
> -#include <asm/io.h>

This seems to be unrelated of this work.

>   #include <xen/acpi.h>
>   #include <xen/errno.h>
>   #include <xen/iocap.h>
> --- a/xen/drivers/char/meson-uart.c
> +++ b/xen/drivers/char/meson-uart.c
> @@ -18,7 +18,9 @@
>    * License along with this program; If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <xen/errno.h>
>   #include <xen/irq.h>
> +#include <xen/mm.h>

I was going to ask why xen/mm.h needs to be included here. But it looks 
like ioremap_nocache() is defined in mm.h rather than vmap.h.

I will add it as a clean-up on my side.


>   #include <xen/serial.h>
>   #include <xen/vmap.h>
>   #include <asm/io.h>
> --- a/xen/drivers/char/mvebu-uart.c
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -18,7 +18,9 @@
>    * License along with this program; If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <xen/errno.h>
>   #include <xen/irq.h>
> +#include <xen/mm.h>
>   #include <xen/serial.h>
>   #include <xen/vmap.h>
>   #include <asm/io.h>
> --- a/xen/include/asm-x86/io.h
> +++ b/xen/include/asm-x86/io.h
> @@ -49,6 +49,7 @@ __OUT(l,,int)
>   
>   /* Function pointer used to handle platform specific I/O port emulation. */
>   #define IOEMUL_QUIRK_STUB_BYTES 9
> +struct cpu_user_regs;
>   extern unsigned int (*ioemul_handle_quirk)(
>       u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
>   
> --- /dev/null
> +++ b/xen/include/xen/frame-num.h

It would feel more natural to me if the file is named mm-types.h.

Cheers,

Jan Beulich Dec. 3, 2020, 9:39 a.m. UTC | #2

On 02.12.2020 18:35, Julien Grall wrote:
> On 02/12/2020 14:50, Jan Beulich wrote:
>> xen/mm.h has heavy dependencies, while in a number of cases only these
>> type definitions are needed. This separation then also allows pulling in
>> these definitions when including xen/mm.h would cause cyclic
>> dependencies.
>>
>> Replace xen/mm.h inclusion where possible in include/xen/. (In
>> xen/iommu.h also take the opportunity and correct the few remaining
>> sorting issues.)
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>>
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -10,7 +10,6 @@
>>    * Slimmed with Xen specific support.
>>    */
>>   
>> -#include <asm/io.h>
> 
> This seems to be unrelated of this work.

Well spotted, but the answer really is "yes and no". My first
attempt at fixing build issues from this and similar asm/io.h
inclusions was to remove such unnecessary ones. But this didn't
work out - I had to fix the header instead. If you think this
extra cleanup really does any harm here, I can drop it. But I'd
prefer to keep it.

>> --- /dev/null
>> +++ b/xen/include/xen/frame-num.h
> 
> It would feel more natural to me if the file is named mm-types.h.

Indeed I was first meaning to use this name (not the least
because I don't particularly like the one chosen, but I also
couldn't think of a better one). However, then things like
struct page_info would imo also belong there (more precisely in
asm/mm-types.h to be included from here), which is specifically
something I want to avoid. Yes, eventually we may (I'm inclined
to even say "will") want such a header, but I still want to
keep these even more fundamental types in a separate one.
Otherwise we'll again end up with files including mm-types.h
just because of needing e.g. gfn_t for a function declaration.
(Note that the same isn't the case for struct page_info, which
can simply be forward declared.)

Jan

Julien Grall Dec. 4, 2020, 9:29 a.m. UTC | #3

Hi Jan,

On 03/12/2020 09:39, Jan Beulich wrote:
> On 02.12.2020 18:35, Julien Grall wrote:
>> On 02/12/2020 14:50, Jan Beulich wrote:
>>> xen/mm.h has heavy dependencies, while in a number of cases only these
>>> type definitions are needed. This separation then also allows pulling in
>>> these definitions when including xen/mm.h would cause cyclic
>>> dependencies.
>>>
>>> Replace xen/mm.h inclusion where possible in include/xen/. (In
>>> xen/iommu.h also take the opportunity and correct the few remaining
>>> sorting issues.)
>>>
>>> Signed-off-by: Jan Beulich <[e-mail]>
>>>
>>> --- a/xen/arch/x86/acpi/power.c
>>> +++ b/xen/arch/x86/acpi/power.c
>>> @@ -10,7 +10,6 @@
>>>     * Slimmed with Xen specific support.
>>>     */
>>>    
>>> -#include <asm/io.h>
>>
>> This seems to be unrelated of this work.
> 
> Well spotted, but the answer really is "yes and no". My first
> attempt at fixing build issues from this and similar asm/io.h
> inclusions was to remove such unnecessary ones. But this didn't
> work out - I had to fix the header instead. If you think this
> extra cleanup really does any harm here, I can drop it. But I'd
> prefer to keep it.

I am fine with keeping it here. Can you mention it in the commit message?

> 
>>> --- /dev/null
>>> +++ b/xen/include/xen/frame-num.h
>>
>> It would feel more natural to me if the file is named mm-types.h.
> 
> Indeed I was first meaning to use this name (not the least
> because I don't particularly like the one chosen, but I also
> couldn't think of a better one). However, then things like
> struct page_info would imo also belong there (more precisely in
> asm/mm-types.h to be included from here), which is specifically
> something I want to avoid. Yes, eventually we may (I'm inclined
> to even say "will") want such a header, but I still want to
> keep these even more fundamental types in a separate one.
> Otherwise we'll again end up with files including mm-types.h
> just because of needing e.g. gfn_t for a function declaration.
> (Note that the same isn't the case for struct page_info, which
> can simply be forward declared.)
Thanks for the explanation. AFAICT, this file will mostly contain 
typesafe for MM. So how about naming it to mm-typesafe.h? Or maybe 
mm-frame.h?

Cheers,

Jan Beulich Dec. 4, 2020, 9:51 a.m. UTC | #4

On 04.12.2020 10:29, Julien Grall wrote:
> On 03/12/2020 09:39, Jan Beulich wrote:
>> On 02.12.2020 18:35, Julien Grall wrote:
>>> On 02/12/2020 14:50, Jan Beulich wrote:
>>>> xen/mm.h has heavy dependencies, while in a number of cases only these
>>>> type definitions are needed. This separation then also allows pulling in
>>>> these definitions when including xen/mm.h would cause cyclic
>>>> dependencies.
>>>>
>>>> Replace xen/mm.h inclusion where possible in include/xen/. (In
>>>> xen/iommu.h also take the opportunity and correct the few remaining
>>>> sorting issues.)
>>>>
>>>> Signed-off-by: Jan Beulich <[e-mail]>
>>>>
>>>> --- a/xen/arch/x86/acpi/power.c
>>>> +++ b/xen/arch/x86/acpi/power.c
>>>> @@ -10,7 +10,6 @@
>>>>     * Slimmed with Xen specific support.
>>>>     */
>>>>    
>>>> -#include <asm/io.h>
>>>
>>> This seems to be unrelated of this work.
>>
>> Well spotted, but the answer really is "yes and no". My first
>> attempt at fixing build issues from this and similar asm/io.h
>> inclusions was to remove such unnecessary ones. But this didn't
>> work out - I had to fix the header instead. If you think this
>> extra cleanup really does any harm here, I can drop it. But I'd
>> prefer to keep it.
> 
> I am fine with keeping it here. Can you mention it in the commit message?

I've added a paragraph.

>>>> --- /dev/null
>>>> +++ b/xen/include/xen/frame-num.h
>>>
>>> It would feel more natural to me if the file is named mm-types.h.
>>
>> Indeed I was first meaning to use this name (not the least
>> because I don't particularly like the one chosen, but I also
>> couldn't think of a better one). However, then things like
>> struct page_info would imo also belong there (more precisely in
>> asm/mm-types.h to be included from here), which is specifically
>> something I want to avoid. Yes, eventually we may (I'm inclined
>> to even say "will") want such a header, but I still want to
>> keep these even more fundamental types in a separate one.
>> Otherwise we'll again end up with files including mm-types.h
>> just because of needing e.g. gfn_t for a function declaration.
>> (Note that the same isn't the case for struct page_info, which
>> can simply be forward declared.)
> Thanks for the explanation. AFAICT, this file will mostly contain 
> typesafe for MM. So how about naming it to mm-typesafe.h? Or maybe 
> mm-frame.h?

Hmm, yes, why not. I guess I'd slightly prefer the latter.

Jan

Patch

--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -10,7 +10,6 @@ 
  * Slimmed with Xen specific support.
  */
 
-#include <asm/io.h>
 #include <xen/acpi.h>
 #include <xen/errno.h>
 #include <xen/iocap.h>
--- a/xen/drivers/char/meson-uart.c
+++ b/xen/drivers/char/meson-uart.c
@@ -18,7 +18,9 @@ 
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/errno.h>
 #include <xen/irq.h>
+#include <xen/mm.h>
 #include <xen/serial.h>
 #include <xen/vmap.h>
 #include <asm/io.h>
--- a/xen/drivers/char/mvebu-uart.c
+++ b/xen/drivers/char/mvebu-uart.c
@@ -18,7 +18,9 @@ 
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/errno.h>
 #include <xen/irq.h>
+#include <xen/mm.h>
 #include <xen/serial.h>
 #include <xen/vmap.h>
 #include <asm/io.h>
--- a/xen/include/asm-x86/io.h
+++ b/xen/include/asm-x86/io.h
@@ -49,6 +49,7 @@  __OUT(l,,int)
 
 /* Function pointer used to handle platform specific I/O port emulation. */
 #define IOEMUL_QUIRK_STUB_BYTES 9
+struct cpu_user_regs;
 extern unsigned int (*ioemul_handle_quirk)(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
 
--- /dev/null
+++ b/xen/include/xen/frame-num.h
@@ -0,0 +1,96 @@ 
+#ifndef __XEN_FRAME_NUM_H__
+#define __XEN_FRAME_NUM_H__
+
+#include <xen/kernel.h>
+#include <xen/typesafe.h>
+
+TYPE_SAFE(unsigned long, mfn);
+#define PRI_mfn          "05lx"
+#define INVALID_MFN      _mfn(~0UL)
+/*
+ * To be used for global variable initialization. This workaround a bug
+ * in GCC < 5.0.
+ */
+#define INVALID_MFN_INITIALIZER { ~0UL }
+
+#ifndef mfn_t
+#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
+#define _mfn
+#define mfn_x
+#undef mfn_t
+#undef _mfn
+#undef mfn_x
+#endif
+
+static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
+{
+    return _mfn(mfn_x(mfn) + i);
+}
+
+static inline mfn_t mfn_max(mfn_t x, mfn_t y)
+{
+    return _mfn(max(mfn_x(x), mfn_x(y)));
+}
+
+static inline mfn_t mfn_min(mfn_t x, mfn_t y)
+{
+    return _mfn(min(mfn_x(x), mfn_x(y)));
+}
+
+static inline bool_t mfn_eq(mfn_t x, mfn_t y)
+{
+    return mfn_x(x) == mfn_x(y);
+}
+
+TYPE_SAFE(unsigned long, gfn);
+#define PRI_gfn          "05lx"
+#define INVALID_GFN      _gfn(~0UL)
+/*
+ * To be used for global variable initialization. This workaround a bug
+ * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
+ */
+#define INVALID_GFN_INITIALIZER { ~0UL }
+
+#ifndef gfn_t
+#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
+#define _gfn
+#define gfn_x
+#undef gfn_t
+#undef _gfn
+#undef gfn_x
+#endif
+
+static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
+{
+    return _gfn(gfn_x(gfn) + i);
+}
+
+static inline gfn_t gfn_max(gfn_t x, gfn_t y)
+{
+    return _gfn(max(gfn_x(x), gfn_x(y)));
+}
+
+static inline gfn_t gfn_min(gfn_t x, gfn_t y)
+{
+    return _gfn(min(gfn_x(x), gfn_x(y)));
+}
+
+static inline bool_t gfn_eq(gfn_t x, gfn_t y)
+{
+    return gfn_x(x) == gfn_x(y);
+}
+
+TYPE_SAFE(unsigned long, pfn);
+#define PRI_pfn          "05lx"
+#define INVALID_PFN      (~0UL)
+
+#ifndef pfn_t
+#define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
+#define _pfn
+#define pfn_x
+#undef pfn_t
+#undef _pfn
+#undef pfn_x
+#endif
+
+#endif /* __XEN_FRAME_NUM_H__ */
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -23,7 +23,7 @@ 
 #ifndef __XEN_GRANT_TABLE_H__
 #define __XEN_GRANT_TABLE_H__
 
-#include <xen/mm.h>
+#include <xen/frame-num.h>
 #include <xen/rwlock.h>
 #include <public/grant_table.h>
 #include <asm/grant_table.h>
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -19,14 +19,13 @@ 
 #ifndef _IOMMU_H_
 #define _IOMMU_H_
 
+#include <xen/frame-num.h>
 #include <xen/init.h>
 #include <xen/page-defs.h>
-#include <xen/spinlock.h>
 #include <xen/pci.h>
-#include <xen/typesafe.h>
-#include <xen/mm.h>
-#include <public/hvm/ioreq.h>
+#include <xen/spinlock.h>
 #include <public/domctl.h>
+#include <public/hvm/ioreq.h>
 #include <asm/device.h>
 
 TYPE_SAFE(uint64_t, dfn);
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -51,103 +51,13 @@ 
 #define __XEN_MM_H__
 
 #include <xen/compiler.h>
+#include <xen/frame-num.h>
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
-#include <xen/typesafe.h>
-#include <xen/kernel.h>
 #include <xen/perfc.h>
 #include <public/memory.h>
 
-TYPE_SAFE(unsigned long, mfn);
-#define PRI_mfn          "05lx"
-#define INVALID_MFN      _mfn(~0UL)
-/*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0.
- */
-#define INVALID_MFN_INITIALIZER { ~0UL }
-
-#ifndef mfn_t
-#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
-#define _mfn
-#define mfn_x
-#undef mfn_t
-#undef _mfn
-#undef mfn_x
-#endif
-
-static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
-{
-    return _mfn(mfn_x(mfn) + i);
-}
-
-static inline mfn_t mfn_max(mfn_t x, mfn_t y)
-{
-    return _mfn(max(mfn_x(x), mfn_x(y)));
-}
-
-static inline mfn_t mfn_min(mfn_t x, mfn_t y)
-{
-    return _mfn(min(mfn_x(x), mfn_x(y)));
-}
-
-static inline bool_t mfn_eq(mfn_t x, mfn_t y)
-{
-    return mfn_x(x) == mfn_x(y);
-}
-
-TYPE_SAFE(unsigned long, gfn);
-#define PRI_gfn          "05lx"
-#define INVALID_GFN      _gfn(~0UL)
-/*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
- */
-#define INVALID_GFN_INITIALIZER { ~0UL }
-
-#ifndef gfn_t
-#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
-#define _gfn
-#define gfn_x
-#undef gfn_t
-#undef _gfn
-#undef gfn_x
-#endif
-
-static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
-{
-    return _gfn(gfn_x(gfn) + i);
-}
-
-static inline gfn_t gfn_max(gfn_t x, gfn_t y)
-{
-    return _gfn(max(gfn_x(x), gfn_x(y)));
-}
-
-static inline gfn_t gfn_min(gfn_t x, gfn_t y)
-{
-    return _gfn(min(gfn_x(x), gfn_x(y)));
-}
-
-static inline bool_t gfn_eq(gfn_t x, gfn_t y)
-{
-    return gfn_x(x) == gfn_x(y);
-}
-
-TYPE_SAFE(unsigned long, pfn);
-#define PRI_pfn          "05lx"
-#define INVALID_PFN      (~0UL)
-
-#ifndef pfn_t
-#define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
-#define _pfn
-#define pfn_x
-#undef pfn_t
-#undef _pfn
-#undef pfn_x
-#endif
-
 struct page_info;
 
 void put_page(struct page_info *);
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -1,7 +1,7 @@ 
 #ifndef _XEN_P2M_COMMON_H
 #define _XEN_P2M_COMMON_H
 
-#include <xen/mm.h>
+#include <xen/frame-num.h>
 
 /* Remove a page from a domain's p2m table */
 int __must_check
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -1,7 +1,7 @@ 
 #if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
 #define __XEN_VMAP_H__
 
-#include <xen/mm.h>
+#include <xen/frame-num.h>
 #include <xen/page-size.h>
 
 enum vmap_region {