| Message ID | 20230701071835.41599-8-christopher.w.clark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v3: Boot modules for Hyperlaunch | expand |
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 > > > > >
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__ */