Pointer fields within structs need to be defined as fixed size types in
the x86 boot build environment. Using a typedef for the field type
rather than a struct pointer type enables the type definition to
be changed in the 32-bit boot build and the main hypervisor build,
allowing for a single common structure definition and a common header file.

Introduces DEFINE_STRUCT_PTR_TYPE and DEFINE_PTR_TYPE which will
generate typedefs with a _ptr_t suffix for pointers to the specified
type. This is then used in <xen/bootinfo.h> for pointers within structs
as preparation for using these headers in the x86 boot build.

The 32-bit behaviour is obtained by inclusion of "defs.h" first with a
check for such an existing definition on the <xen/types.h> version.

paddr_t is used in <xen/bootinfo.h> so a definition is added here to
the x86 boot environment defs.h header.

Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

---
Changes since v2: This is two v2 patches merged into one for v3.
Changes since v1: New in v2 of series.

 xen/arch/x86/boot/defs.h            |  9 +++++++++
 xen/arch/x86/include/asm/bootinfo.h |  4 +++-
 xen/include/xen/bootinfo.h          |  9 +++++----
 xen/include/xen/types.h             | 11 +++++++++++
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
index f9840044ec..bc0f1b5cf8 100644
--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -60,4 +60,13 @@ typedef u64 uint64_t;
 #define U16_MAX		((u16)(~0U))
 #define UINT_MAX	(~0U)
 
+typedef unsigned long long paddr_t;
+
+#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
+    typedef uint64_t struct_name ## _ptr_t;
+
+#define DEFINE_PTR_TYPE(type) \
+    typedef uint64_t type ## _ptr_t;
+DEFINE_PTR_TYPE(char);
+
 #endif /* __BOOT_DEFS_H__ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 30c27980e0..989fb7a1da 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -6,6 +6,7 @@ struct arch_bootmodule {
     uint32_t flags;
     unsigned headroom;
 };
+DEFINE_STRUCT_PTR_TYPE(arch_bootmodule);
 
 struct arch_boot_info {
     uint32_t flags;
@@ -14,11 +15,12 @@ struct arch_boot_info {
 #define BOOTINFO_FLAG_X86_MEMMAP       1U << 6
 #define BOOTINFO_FLAG_X86_LOADERNAME   1U << 9
 
-    char *boot_loader_name;
+    char_ptr_t boot_loader_name;
 
     uint32_t mmap_length;
     paddr_t mmap_addr;
 };
+DEFINE_STRUCT_PTR_TYPE(arch_boot_info);
 
 struct __packed mb_memmap {
     uint32_t size;
diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
index 2f4284a91f..8389da4f72 100644
--- a/xen/include/xen/bootinfo.h
+++ b/xen/include/xen/bootinfo.h
@@ -35,17 +35,18 @@ struct boot_module {
     mfn_t mfn;
     size_t size;
 
-    struct arch_bootmodule *arch;
+    arch_bootmodule_ptr_t arch;
     struct boot_string string;
 };
+DEFINE_STRUCT_PTR_TYPE(boot_module);
 
 struct boot_info {
-    char *cmdline;
+    char_ptr_t cmdline;
 
     unsigned int nr_mods;
-    struct boot_module *mods;
+    boot_module_ptr_t mods;
 
-    struct arch_boot_info *arch;
+    arch_boot_info_ptr_t arch;
 };
 
 #endif
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 6aba80500a..e807ffe255 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -71,4 +71,15 @@ typedef bool bool_t;
 #define test_and_set_bool(b)   xchg(&(b), true)
 #define test_and_clear_bool(b) xchg(&(b), false)
 
+#ifndef DEFINE_STRUCT_PTR_TYPE
+#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
+    typedef struct struct_name * struct_name ## _ptr_t;
+#endif
+
+#ifndef DEFINE_PTR_TYPE
+#define DEFINE_PTR_TYPE(type) \
+    typedef type * type ## _ptr_t;
+DEFINE_PTR_TYPE(char);
+#endif
+
 #endif /* __TYPES_H__ */
-- 
2.25.1

On Sat, 1 Jul 2023, Christopher Clark wrote:
> Pointer fields within structs need to be defined as fixed size types in
> the x86 boot build environment. Using a typedef for the field type
> rather than a struct pointer type enables the type definition to
> be changed in the 32-bit boot build and the main hypervisor build,
> allowing for a single common structure definition and a common header file.

Sorry for my ignorance, but why?

struct boot_module is not used as part of any ABI, right? It is
populated by Xen at boot by hand. Why do we need a specific memory
layout for it?



> Introduces DEFINE_STRUCT_PTR_TYPE and DEFINE_PTR_TYPE which will
> generate typedefs with a _ptr_t suffix for pointers to the specified
> type. This is then used in <xen/bootinfo.h> for pointers within structs
> as preparation for using these headers in the x86 boot build.
> 
> The 32-bit behaviour is obtained by inclusion of "defs.h" first with a
> check for such an existing definition on the <xen/types.h> version.
> 
> paddr_t is used in <xen/bootinfo.h> so a definition is added here to
> the x86 boot environment defs.h header.
> 
> Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>


> ---
> Changes since v2: This is two v2 patches merged into one for v3.
> Changes since v1: New in v2 of series.
> 
>  xen/arch/x86/boot/defs.h            |  9 +++++++++
>  xen/arch/x86/include/asm/bootinfo.h |  4 +++-
>  xen/include/xen/bootinfo.h          |  9 +++++----
>  xen/include/xen/types.h             | 11 +++++++++++
>  4 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
> index f9840044ec..bc0f1b5cf8 100644
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -60,4 +60,13 @@ typedef u64 uint64_t;
>  #define U16_MAX		((u16)(~0U))
>  #define UINT_MAX	(~0U)
>  
> +typedef unsigned long long paddr_t;
> +
> +#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
> +    typedef uint64_t struct_name ## _ptr_t;
> +
> +#define DEFINE_PTR_TYPE(type) \
> +    typedef uint64_t type ## _ptr_t;
> +DEFINE_PTR_TYPE(char);
> +
>  #endif /* __BOOT_DEFS_H__ */
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 30c27980e0..989fb7a1da 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -6,6 +6,7 @@ struct arch_bootmodule {
>      uint32_t flags;
>      unsigned headroom;
>  };
> +DEFINE_STRUCT_PTR_TYPE(arch_bootmodule);
>  
>  struct arch_boot_info {
>      uint32_t flags;
> @@ -14,11 +15,12 @@ struct arch_boot_info {
>  #define BOOTINFO_FLAG_X86_MEMMAP       1U << 6
>  #define BOOTINFO_FLAG_X86_LOADERNAME   1U << 9
>  
> -    char *boot_loader_name;
> +    char_ptr_t boot_loader_name;
>  
>      uint32_t mmap_length;
>      paddr_t mmap_addr;
>  };
> +DEFINE_STRUCT_PTR_TYPE(arch_boot_info);
>  
>  struct __packed mb_memmap {
>      uint32_t size;
> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> index 2f4284a91f..8389da4f72 100644
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -35,17 +35,18 @@ struct boot_module {
>      mfn_t mfn;
>      size_t size;
>  
> -    struct arch_bootmodule *arch;
> +    arch_bootmodule_ptr_t arch;
>      struct boot_string string;
>  };
> +DEFINE_STRUCT_PTR_TYPE(boot_module);
>  
>  struct boot_info {
> -    char *cmdline;
> +    char_ptr_t cmdline;
>  
>      unsigned int nr_mods;
> -    struct boot_module *mods;
> +    boot_module_ptr_t mods;
>  
> -    struct arch_boot_info *arch;
> +    arch_boot_info_ptr_t arch;
>  };
>  
>  #endif
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 6aba80500a..e807ffe255 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -71,4 +71,15 @@ typedef bool bool_t;
>  #define test_and_set_bool(b)   xchg(&(b), true)
>  #define test_and_clear_bool(b) xchg(&(b), false)
>  
> +#ifndef DEFINE_STRUCT_PTR_TYPE
> +#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
> +    typedef struct struct_name * struct_name ## _ptr_t;
> +#endif
> +
> +#ifndef DEFINE_PTR_TYPE
> +#define DEFINE_PTR_TYPE(type) \
> +    typedef type * type ## _ptr_t;
> +DEFINE_PTR_TYPE(char);
> +#endif
> +
>  #endif /* __TYPES_H__ */
> -- 
> 2.25.1
> 
>

On Sat, Jul 8, 2023 at 3:24 PM Stefano Stabellini <sstabellini@kernel.org>
wrote:

> On Sat, 1 Jul 2023, Christopher Clark wrote:
> > Pointer fields within structs need to be defined as fixed size types in
> > the x86 boot build environment. Using a typedef for the field type
> > rather than a struct pointer type enables the type definition to
> > be changed in the 32-bit boot build and the main hypervisor build,
> > allowing for a single common structure definition and a common header
> file.
>
> Sorry for my ignorance, but why?
>
> struct boot_module is not used as part of any ABI, right? It is
> populated by Xen at boot by hand. Why do we need a specific memory
> layout for it?
>

Fair question! In the early x86 boot logic, which runs in 32-bit CPU mode,
struct boot_module is allocated and populated, so the structure needs to be
defined and available to code that is compiled in 32-bit to do that. The
same structures are also accessed later in 64-bit hypervisor logic, and the
memory layout of the structure needs to be the same in both cases, so we
want all the fields to be fixed-width types, and that includes pointers.

These macros help with declaring pointers as always-64-bit-sized struct
fields in a single definition of the struct. They're not strictly necessary
though - providing alternative definitions for typedefs can be used
instead, and I've been looking at doing that since posting this patch.

Christopher



>
>
>
> > Introduces DEFINE_STRUCT_PTR_TYPE and DEFINE_PTR_TYPE which will
> > generate typedefs with a _ptr_t suffix for pointers to the specified
> > type. This is then used in <xen/bootinfo.h> for pointers within structs
> > as preparation for using these headers in the x86 boot build.
> >
> > The 32-bit behaviour is obtained by inclusion of "defs.h" first with a
> > check for such an existing definition on the <xen/types.h> version.
> >
> > paddr_t is used in <xen/bootinfo.h> so a definition is added here to
> > the x86 boot environment defs.h header.
> >
> > Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
>
> > ---
> > Changes since v2: This is two v2 patches merged into one for v3.
> > Changes since v1: New in v2 of series.
> >
> >  xen/arch/x86/boot/defs.h            |  9 +++++++++
> >  xen/arch/x86/include/asm/bootinfo.h |  4 +++-
> >  xen/include/xen/bootinfo.h          |  9 +++++----
> >  xen/include/xen/types.h             | 11 +++++++++++
> >  4 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
> > index f9840044ec..bc0f1b5cf8 100644
> > --- a/xen/arch/x86/boot/defs.h
> > +++ b/xen/arch/x86/boot/defs.h
> > @@ -60,4 +60,13 @@ typedef u64 uint64_t;
> >  #define U16_MAX              ((u16)(~0U))
> >  #define UINT_MAX     (~0U)
> >
> > +typedef unsigned long long paddr_t;
> > +
> > +#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
> > +    typedef uint64_t struct_name ## _ptr_t;
> > +
> > +#define DEFINE_PTR_TYPE(type) \
> > +    typedef uint64_t type ## _ptr_t;
> > +DEFINE_PTR_TYPE(char);
> > +
> >  #endif /* __BOOT_DEFS_H__ */
> > diff --git a/xen/arch/x86/include/asm/bootinfo.h
> b/xen/arch/x86/include/asm/bootinfo.h
> > index 30c27980e0..989fb7a1da 100644
> > --- a/xen/arch/x86/include/asm/bootinfo.h
> > +++ b/xen/arch/x86/include/asm/bootinfo.h
> > @@ -6,6 +6,7 @@ struct arch_bootmodule {
> >      uint32_t flags;
> >      unsigned headroom;
> >  };
> > +DEFINE_STRUCT_PTR_TYPE(arch_bootmodule);
> >
> >  struct arch_boot_info {
> >      uint32_t flags;
> > @@ -14,11 +15,12 @@ struct arch_boot_info {
> >  #define BOOTINFO_FLAG_X86_MEMMAP       1U << 6
> >  #define BOOTINFO_FLAG_X86_LOADERNAME   1U << 9
> >
> > -    char *boot_loader_name;
> > +    char_ptr_t boot_loader_name;
> >
> >      uint32_t mmap_length;
> >      paddr_t mmap_addr;
> >  };
> > +DEFINE_STRUCT_PTR_TYPE(arch_boot_info);
> >
> >  struct __packed mb_memmap {
> >      uint32_t size;
> > diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> > index 2f4284a91f..8389da4f72 100644
> > --- a/xen/include/xen/bootinfo.h
> > +++ b/xen/include/xen/bootinfo.h
> > @@ -35,17 +35,18 @@ struct boot_module {
> >      mfn_t mfn;
> >      size_t size;
> >
> > -    struct arch_bootmodule *arch;
> > +    arch_bootmodule_ptr_t arch;
> >      struct boot_string string;
> >  };
> > +DEFINE_STRUCT_PTR_TYPE(boot_module);
> >
> >  struct boot_info {
> > -    char *cmdline;
> > +    char_ptr_t cmdline;
> >
> >      unsigned int nr_mods;
> > -    struct boot_module *mods;
> > +    boot_module_ptr_t mods;
> >
> > -    struct arch_boot_info *arch;
> > +    arch_boot_info_ptr_t arch;
> >  };
> >
> >  #endif
> > diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> > index 6aba80500a..e807ffe255 100644
> > --- a/xen/include/xen/types.h
> > +++ b/xen/include/xen/types.h
> > @@ -71,4 +71,15 @@ typedef bool bool_t;
> >  #define test_and_set_bool(b)   xchg(&(b), true)
> >  #define test_and_clear_bool(b) xchg(&(b), false)
> >
> > +#ifndef DEFINE_STRUCT_PTR_TYPE
> > +#define DEFINE_STRUCT_PTR_TYPE(struct_name) \
> > +    typedef struct struct_name * struct_name ## _ptr_t;
> > +#endif
> > +
> > +#ifndef DEFINE_PTR_TYPE
> > +#define DEFINE_PTR_TYPE(type) \
> > +    typedef type * type ## _ptr_t;
> > +DEFINE_PTR_TYPE(char);
> > +#endif
> > +
> >  #endif /* __TYPES_H__ */
> > --
> > 2.25.1
> >
> >
>