While reviewing Hongyan's "x86/vmap: handle superpages in
vmap_to_mfn()" it became apparent that the interaction of
xen/mm.h and asm/page.h is problematic. Therefore some basic
page size related definitions get moved out of the latter, and
the mfn_t et al ones out of the former, each into new headers.

While various configurations build fine for me with these
changes in place, it's relatively likely that this may break
some more exotic ones. Such breakage ought to be easy to
resolve, so I hope this risk isn't going to be a hindrance of
the changes here going in.

1: include: don't use asm/page.h from common headers
2: mm: split out mfn_t / gfn_t / pfn_t definitions and helpers

Jan

Doing so limits what can be done in (in particular included by) this per-
arch header. Abstract out page shift/size related #define-s, which is all
the repsecitve headers care about. Extend the replacement / removal to
some x86 headers as well; some others now need to include page.h (and
they really should have before).

Arm's VADDR_BITS gets restricted to 32-bit, as its current value is
clearly wrong for 64-bit, but the constant also isn't used anywhere
right now (i.e. the #define could also be dropped altogether).

I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I
kept it and provided a way to override the #define in the common header.

Also drop the dead PAGE_FLAG_MASK altogether at this occasion.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/arm64/lib/clear_page.S
+++ b/xen/arch/arm/arm64/lib/clear_page.S
@@ -14,6 +14,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/page-size.h>
+
 /*
  * Clear page @dest
  *
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -176,11 +176,6 @@
 #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 PAGE_SHIFT              12
-#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
-#define PAGE_MASK           (~(PAGE_SIZE-1))
-#define PAGE_FLAG_MASK      (~0)
-
 #define NR_hypercalls 64
 
 #define STACK_ORDER 3
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -1,6 +1,7 @@
 #ifndef __ARM_CURRENT_H__
 #define __ARM_CURRENT_H__
 
+#include <xen/page-size.h>
 #include <xen/percpu.h>
 
 #include <asm/processor.h>
--- /dev/null
+++ b/xen/include/asm-arm/page-shift.h
@@ -0,0 +1,15 @@
+#ifndef __ARM_PAGE_SHIFT_H__
+#define __ARM_PAGE_SHIFT_H__
+
+#define PAGE_SHIFT              12
+
+#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
+
+#ifdef CONFIG_ARM_64
+#define PADDR_BITS              48
+#else
+#define PADDR_BITS              40
+#define VADDR_BITS              32
+#endif
+
+#endif /* __ARM_PAGE_SHIFT_H__ */
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -2,21 +2,11 @@
 #define __ARM_PAGE_H__
 
 #include <public/xen.h>
+#include <xen/page-size.h>
 #include <asm/processor.h>
 #include <asm/lpae.h>
 #include <asm/sysregs.h>
 
-#ifdef CONFIG_ARM_64
-#define PADDR_BITS              48
-#else
-#define PADDR_BITS              40
-#endif
-#define PADDR_MASK              ((1ULL << PADDR_BITS)-1)
-#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
-
-#define VADDR_BITS              32
-#define VADDR_MASK              (~0UL)
-
 /* Shareability values for the LPAE entries */
 #define LPAE_SH_NON_SHAREABLE 0x0
 #define LPAE_SH_UNPREDICTALE  0x1
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -8,8 +8,8 @@
 #define __X86_CURRENT_H__
 
 #include <xen/percpu.h>
+#include <xen/page-size.h>
 #include <public/xen.h>
-#include <asm/page.h>
 
 /*
  * Xen's cpu stacks are 8 pages (8-page aligned), arranged as:
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -1,6 +1,8 @@
 #ifndef __ARCH_DESC_H
 #define __ARCH_DESC_H
 
+#include <asm/page.h>
+
 /*
  * Xen reserves a memory page of GDT entries.
  * No guest GDT entries exist beyond the Xen reserved area.
--- a/xen/include/asm-x86/fixmap.h
+++ b/xen/include/asm-x86/fixmap.h
@@ -12,7 +12,7 @@
 #ifndef _ASM_FIXMAP_H
 #define _ASM_FIXMAP_H
 
-#include <asm/page.h>
+#include <xen/page-size.h>
 
 #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
 #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
--- a/xen/include/asm-x86/guest/hyperv-hcall.h
+++ b/xen/include/asm-x86/guest/hyperv-hcall.h
@@ -20,12 +20,12 @@
 #define __X86_HYPERV_HCALL_H__
 
 #include <xen/lib.h>
+#include <xen/page-size.h>
 #include <xen/types.h>
 
 #include <asm/asm_defns.h>
 #include <asm/fixmap.h>
 #include <asm/guest/hyperv-tlfs.h>
-#include <asm/page.h>
 
 static inline uint64_t hv_do_hypercall(uint64_t control, paddr_t input_addr,
                                        paddr_t output_addr)
--- a/xen/include/asm-x86/guest/hyperv-tlfs.h
+++ b/xen/include/asm-x86/guest/hyperv-tlfs.h
@@ -10,8 +10,8 @@
 #define _ASM_X86_HYPERV_TLFS_H
 
 #include <xen/bitops.h>
+#include <xen/page-size.h>
 #include <xen/types.h>
-#include <asm/page.h>
 
 /*
  * While not explicitly listed in the TLFS, Hyper-V always runs with a page size
--- a/xen/include/asm-x86/io.h
+++ b/xen/include/asm-x86/io.h
@@ -3,7 +3,6 @@
 
 #include <xen/vmap.h>
 #include <xen/types.h>
-#include <asm/page.h>
 
 #define readb(x) (*(volatile uint8_t  *)(x))
 #define readw(x) (*(volatile uint16_t *)(x))
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -6,6 +6,7 @@
 #include <xen/spinlock.h>
 #include <xen/rwlock.h>
 #include <asm/io.h>
+#include <asm/page.h>
 #include <asm/uaccess.h>
 #include <asm/x86_emulate.h>
 
--- /dev/null
+++ b/xen/include/asm-x86/page-shift.h
@@ -0,0 +1,26 @@
+#ifndef __X86_PAGE_SHIFT_H__
+#define __X86_PAGE_SHIFT_H__
+
+#define L1_PAGETABLE_SHIFT      12
+#define L2_PAGETABLE_SHIFT      21
+#define L3_PAGETABLE_SHIFT      30
+#define L4_PAGETABLE_SHIFT      39
+#define PAGE_SHIFT              L1_PAGETABLE_SHIFT
+#define SUPERPAGE_SHIFT         L2_PAGETABLE_SHIFT
+#define ROOT_PAGETABLE_SHIFT    L4_PAGETABLE_SHIFT
+
+#define PAGETABLE_ORDER         9
+#define L1_PAGETABLE_ENTRIES    (1 << PAGETABLE_ORDER)
+#define L2_PAGETABLE_ENTRIES    (1 << PAGETABLE_ORDER)
+#define L3_PAGETABLE_ENTRIES    (1 << PAGETABLE_ORDER)
+#define L4_PAGETABLE_ENTRIES    (1 << PAGETABLE_ORDER)
+#define ROOT_PAGETABLE_ENTRIES  L4_PAGETABLE_ENTRIES
+
+#define SUPERPAGE_ORDER         PAGETABLE_ORDER
+#define SUPERPAGE_PAGES         (1 << SUPERPAGE_ORDER)
+
+/* These are architectural limits. */
+#define PADDR_BITS              52
+#define VADDR_BITS              48
+
+#endif /* __X86_PAGE_SHIFT_H__ */
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -2,15 +2,7 @@
 #define __X86_PAGE_H__
 
 #include <xen/const.h>
-
-/*
- * It is important that the masks are signed quantities. This ensures that
- * the compiler sign-extends a 32-bit mask to 64 bits if that is required.
- */
-#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
-#define PAGE_MASK           (~(PAGE_SIZE-1))
-#define PAGE_FLAG_MASK      (~0)
-#define PAGE_OFFSET(ptr)    ((unsigned long)(ptr) & ~PAGE_MASK)
+#include <xen/page-size.h>
 
 #define PAGE_ORDER_4K       0
 #define PAGE_ORDER_2M       9
--- a/xen/include/asm-x86/uaccess.h
+++ b/xen/include/asm-x86/uaccess.h
@@ -6,7 +6,6 @@
 #include <xen/errno.h>
 #include <xen/prefetch.h>
 #include <asm/asm_defns.h>
-#include <asm/page.h>
 
 #include <asm/x86_64/uaccess.h>
 
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -2,31 +2,8 @@
 #ifndef __X86_64_PAGE_H__
 #define __X86_64_PAGE_H__
 
-#define L1_PAGETABLE_SHIFT      12
-#define L2_PAGETABLE_SHIFT      21
-#define L3_PAGETABLE_SHIFT      30
-#define L4_PAGETABLE_SHIFT      39
-#define PAGE_SHIFT              L1_PAGETABLE_SHIFT
-#define SUPERPAGE_SHIFT         L2_PAGETABLE_SHIFT
-#define ROOT_PAGETABLE_SHIFT    L4_PAGETABLE_SHIFT
-
-#define PAGETABLE_ORDER         9
-#define L1_PAGETABLE_ENTRIES    (1<<PAGETABLE_ORDER)
-#define L2_PAGETABLE_ENTRIES    (1<<PAGETABLE_ORDER)
-#define L3_PAGETABLE_ENTRIES    (1<<PAGETABLE_ORDER)
-#define L4_PAGETABLE_ENTRIES    (1<<PAGETABLE_ORDER)
-#define ROOT_PAGETABLE_ENTRIES  L4_PAGETABLE_ENTRIES
-#define SUPERPAGE_ORDER         PAGETABLE_ORDER
-#define SUPERPAGE_PAGES         (1<<SUPERPAGE_ORDER)
-
 #define __XEN_VIRT_START        XEN_VIRT_START
 
-/* These are architectural limits. Current CPUs support only 40-bit phys. */
-#define PADDR_BITS              52
-#define VADDR_BITS              48
-#define PADDR_MASK              ((_AC(1,UL) << PADDR_BITS) - 1)
-#define VADDR_MASK              ((_AC(1,UL) << VADDR_BITS) - 1)
-
 #define VADDR_TOP_BIT           (1UL << (VADDR_BITS - 1))
 #define CANONICAL_MASK          (~0UL & ~VADDR_MASK)
 
--- a/xen/include/xen/gdbstub.h
+++ b/xen/include/xen/gdbstub.h
@@ -20,8 +20,8 @@
 #ifndef __XEN_GDBSTUB_H__
 #define __XEN_GDBSTUB_H__
 
+#include <xen/page-size.h>
 #include <asm/atomic.h>
-#include <asm/page.h>
 
 #ifdef CONFIG_CRASH_DEBUG
 
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -26,7 +26,6 @@
 #include <xen/mm.h>
 #include <xen/rwlock.h>
 #include <public/grant_table.h>
-#include <asm/page.h>
 #include <asm/grant_table.h>
 
 #ifdef CONFIG_GRANT_TABLE
--- /dev/null
+++ b/xen/include/xen/page-size.h
@@ -0,0 +1,21 @@
+#ifndef __XEN_PAGE_SIZE_H__
+#define __XEN_PAGE_SIZE_H__
+
+#include <xen/const.h>
+#include <asm/page-shift.h>
+
+/*
+ * It is important that the masks are signed quantities. This ensures that
+ * the compiler sign-extends a 32-bit mask to 64 bits if that is required.
+ */
+#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
+#define PAGE_MASK           (~(PAGE_SIZE-1))
+
+#ifndef PAGE_OFFSET
+# define PAGE_OFFSET(ptr)   ((unsigned long)(ptr) & ~PAGE_MASK)
+#endif
+
+#define PADDR_MASK          ((_AC(1,ULL) << PADDR_BITS) - 1)
+#define VADDR_MASK          (~_AC(0,UL) >> (BITS_PER_LONG - VADDR_BITS))
+
+#endif /* __XEN_PAGE_SIZE__ */
--- a/xen/include/xen/pfn.h
+++ b/xen/include/xen/pfn.h
@@ -1,7 +1,7 @@
 #ifndef __XEN_PFN_H__
 #define __XEN_PFN_H__
 
-#include <asm/page.h>
+#include <xen/page-size.h>
 
 #define PFN_DOWN(x)   ((x) >> PAGE_SHIFT)
 #define PFN_UP(x)     (((x) + PAGE_SIZE-1) >> PAGE_SHIFT)
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -2,7 +2,7 @@
 #define __XEN_VMAP_H__
 
 #include <xen/mm.h>
-#include <asm/page.h>
+#include <xen/page-size.h>
 
 enum vmap_region {
     VMAP_DEFAULT,

Hi Jan,

On 02/12/2020 14:49, Jan Beulich wrote:
> Doing so limits what can be done in (in particular included by) this per-
> arch header. Abstract out page shift/size related #define-s, which is all
> the repsecitve headers care about. Extend the replacement / removal to

s/repsecitve/respective/

> some x86 headers as well; some others now need to include page.h (and
> they really should have before).
> 
> Arm's VADDR_BITS gets restricted to 32-bit, as its current value is
> clearly wrong for 64-bit, but the constant also isn't used anywhere
> right now (i.e. the #define could also be dropped altogether).

Whoops. Thankfully this is not used.

> 
> I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I
> kept it and provided a way to override the #define in the common header.

vaddr_t is defined to 32-bit for arm32 or 64-bit for arm64. So I think 
it would be fine to use the generic PAGE_OFFSET() implementation.

> 
> Also drop the dead PAGE_FLAG_MASK altogether at this occasion.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/arm/arm64/lib/clear_page.S
> +++ b/xen/arch/arm/arm64/lib/clear_page.S
> @@ -14,6 +14,8 @@
>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <xen/page-size.h>
> +
>   /*
>    * Clear page @dest
>    *
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -176,11 +176,6 @@
>   #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 PAGE_SHIFT              12
> -#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> -#define PAGE_MASK           (~(PAGE_SIZE-1))
> -#define PAGE_FLAG_MASK      (~0)
> -
>   #define NR_hypercalls 64
>   
>   #define STACK_ORDER 3
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -1,6 +1,7 @@
>   #ifndef __ARM_CURRENT_H__
>   #define __ARM_CURRENT_H__
>   
> +#include <xen/page-size.h>
>   #include <xen/percpu.h>
>   
>   #include <asm/processor.h>
> --- /dev/null
> +++ b/xen/include/asm-arm/page-shift.h

The name of the file looks a bit odd given that *_BITS are also defined 
in it. So how about renaming to page-size.h?

> @@ -0,0 +1,15 @@
> +#ifndef __ARM_PAGE_SHIFT_H__
> +#define __ARM_PAGE_SHIFT_H__
> +
> +#define PAGE_SHIFT              12
> +
> +#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
> +
> +#ifdef CONFIG_ARM_64
> +#define PADDR_BITS              48

Shouldn't we define VADDR_BITS here? But I wonder whether VADDR_BITS 
should be defined as sizeof(vaddr_t) * 8.

This would require to include asm/types.h.

> +#else
> +#define PADDR_BITS              40
> +#define VADDR_BITS              32
> +#endif
> +
> +#endif /* __ARM_PAGE_SHIFT_H__ */
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -2,21 +2,11 @@
>   #define __ARM_PAGE_H__
>   
>   #include <public/xen.h>
> +#include <xen/page-size.h>
>   #include <asm/processor.h>
>   #include <asm/lpae.h>
>   #include <asm/sysregs.h>
>   
> -#ifdef CONFIG_ARM_64
> -#define PADDR_BITS              48
> -#else
> -#define PADDR_BITS              40
> -#endif
> -#define PADDR_MASK              ((1ULL << PADDR_BITS)-1)
> -#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
> -
> -#define VADDR_BITS              32
> -#define VADDR_MASK              (~0UL)
> -
>   /* Shareability values for the LPAE entries */
>   #define LPAE_SH_NON_SHAREABLE 0x0
>   #define LPAE_SH_UNPREDICTALE  0x1
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -8,8 +8,8 @@
>   #define __X86_CURRENT_H__
>   
>   #include <xen/percpu.h>
> +#include <xen/page-size.h>
>   #include <public/xen.h>
> -#include <asm/page.h>
>   
>   /*
>    * Xen's cpu stacks are 8 pages (8-page aligned), arranged as:
> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -1,6 +1,8 @@
>   #ifndef __ARCH_DESC_H
>   #define __ARCH_DESC_H
>   
> +#include <asm/page.h>

May I ask why you are including <asm/page.h> and not <xen/page-size.h> here?

Cheers,

-- 
Julien Grall

On 02.12.2020 18:14, Julien Grall wrote:
> Hi Jan,
> 
> On 02/12/2020 14:49, Jan Beulich wrote:
>> Doing so limits what can be done in (in particular included by) this per-
>> arch header. Abstract out page shift/size related #define-s, which is all
>> the repsecitve headers care about. Extend the replacement / removal to
> 
> s/repsecitve/respective/
> 
>> some x86 headers as well; some others now need to include page.h (and
>> they really should have before).
>>
>> Arm's VADDR_BITS gets restricted to 32-bit, as its current value is
>> clearly wrong for 64-bit, but the constant also isn't used anywhere
>> right now (i.e. the #define could also be dropped altogether).
> 
> Whoops. Thankfully this is not used.
> 
>>
>> I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I
>> kept it and provided a way to override the #define in the common header.
> 
> vaddr_t is defined to 32-bit for arm32 or 64-bit for arm64. So I think 
> it would be fine to use the generic PAGE_OFFSET() implementation.

Will switch.

>> --- /dev/null
>> +++ b/xen/include/asm-arm/page-shift.h
> 
> The name of the file looks a bit odd given that *_BITS are also defined 
> in it. So how about renaming to page-size.h?

I was initially meaning to use that name, but these headers
specifically don't define any sizes - *_BITS are still shift
values, at least in a way. If the current name isn't liked, my
next best suggestion would then be page-bits.h.

>> @@ -0,0 +1,15 @@
>> +#ifndef __ARM_PAGE_SHIFT_H__
>> +#define __ARM_PAGE_SHIFT_H__
>> +
>> +#define PAGE_SHIFT              12
>> +
>> +#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
>> +
>> +#ifdef CONFIG_ARM_64
>> +#define PADDR_BITS              48
> 
> Shouldn't we define VADDR_BITS here?

See the description - it's unused anyway. I'm fine any of the three
possible ways:
1) keep as is in v1
2) drop altogether
3) also #define for 64-bit (but then you need to tell me whether 64
   is the right value to use, or what the correct one would be)

> But I wonder whether VADDR_BITS 
> should be defined as sizeof(vaddr_t) * 8.
> 
> This would require to include asm/types.h.

Which I'd specifically like to avoid. Plus use of sizeof() also
precludes the use of respective #define-s in #if-s.

>> --- a/xen/include/asm-x86/desc.h
>> +++ b/xen/include/asm-x86/desc.h
>> @@ -1,6 +1,8 @@
>>   #ifndef __ARCH_DESC_H
>>   #define __ARCH_DESC_H
>>   
>> +#include <asm/page.h>
> 
> May I ask why you are including <asm/page.h> and not <xen/page-size.h> here?

Because of

DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e);

and

DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);

at least (didn't check further).

Jan

Hi Jan,

On 03/12/2020 09:27, Jan Beulich wrote:
> On 02.12.2020 18:14, Julien Grall wrote:
>> Hi Jan,
>>
>> On 02/12/2020 14:49, Jan Beulich wrote:
>>> Doing so limits what can be done in (in particular included by) this per-
>>> arch header. Abstract out page shift/size related #define-s, which is all
>>> the repsecitve headers care about. Extend the replacement / removal to
>>
>> s/repsecitve/respective/
>>
>>> some x86 headers as well; some others now need to include page.h (and
>>> they really should have before).
>>>
>>> Arm's VADDR_BITS gets restricted to 32-bit, as its current value is
>>> clearly wrong for 64-bit, but the constant also isn't used anywhere
>>> right now (i.e. the #define could also be dropped altogether).
>>
>> Whoops. Thankfully this is not used.
>>
>>>
>>> I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I
>>> kept it and provided a way to override the #define in the common header.
>>
>> vaddr_t is defined to 32-bit for arm32 or 64-bit for arm64. So I think
>> it would be fine to use the generic PAGE_OFFSET() implementation.
> 
> Will switch.
> 
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/page-shift.h
>>
>> The name of the file looks a bit odd given that *_BITS are also defined
>> in it. So how about renaming to page-size.h?
> 
> I was initially meaning to use that name, but these headers
> specifically don't define any sizes - *_BITS are still shift
> values, at least in a way. If the current name isn't liked, my
> next best suggestion would then be page-bits.h.

I would be happy with page-bits.h.

> 
>>> @@ -0,0 +1,15 @@
>>> +#ifndef __ARM_PAGE_SHIFT_H__
>>> +#define __ARM_PAGE_SHIFT_H__
>>> +
>>> +#define PAGE_SHIFT              12
>>> +
>>> +#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
>>> +
>>> +#ifdef CONFIG_ARM_64
>>> +#define PADDR_BITS              48
>>
>> Shouldn't we define VADDR_BITS here?
> 
> See the description - it's unused anyway. I'm fine any of the three
> possible ways:
> 1) keep as is in v1
> 2) drop altogether
> 3) also #define for 64-bit (but then you need to tell me whether 64
>     is the right value to use, or what the correct one would be)

I would go with 2).

> 
>> But I wonder whether VADDR_BITS
>> should be defined as sizeof(vaddr_t) * 8.
>>
>> This would require to include asm/types.h.
> 
> Which I'd specifically like to avoid. Plus use of sizeof() also
> precludes the use of respective #define-s in #if-s.

Fair point!

>>> --- a/xen/include/asm-x86/desc.h
>>> +++ b/xen/include/asm-x86/desc.h
>>> @@ -1,6 +1,8 @@
>>>    #ifndef __ARCH_DESC_H
>>>    #define __ARCH_DESC_H
>>>    
>>> +#include <asm/page.h>
>>
>> May I ask why you are including <asm/page.h> and not <xen/page-size.h> here?
> 
> Because of
> 
> DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e);
> 
> and
> 
> DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);
> 
> at least (didn't check further).
Thanks for the explanation!


> Jan
> 

Cheers,

-- 
Julien Grall

On 03/12/2020 09:27, Jan Beulich wrote:
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/page-shift.h
>> The name of the file looks a bit odd given that *_BITS are also defined 
>> in it. So how about renaming to page-size.h?
> I was initially meaning to use that name, but these headers
> specifically don't define any sizes - *_BITS are still shift
> values, at least in a way. If the current name isn't liked, my
> next best suggestion would then be page-bits.h.

Pick a generic name, or it will bitrot quickly, and it really wants to
be the same across architectures.

The real issue is that page.h contains far too much stuff now, in both
architectures.  Longterm, we want to split apart the architectural
pagetable definitions, from the Xen APIs to manipulate them, which will
simplify the asm include hierarchy as well.

I'd go with page-bits.h, or just a plain pagetable.h.

~Andrew

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 <jbeulich@suse.com>

--- 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 {

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 <jbeulich@suse.com>
> 
> --- 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,

-- 
Julien Grall

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 <jbeulich@suse.com>
>>
>> --- 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

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 <jbeulich@suse.com>
>>>
>>> --- 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,

-- 
Julien Grall

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 <jbeulich@suse.com>
>>>>
>>>> --- 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