| Message ID | 1457978150-27201-7-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; > */ > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 > > +#if defined(__i386__) || defined(__x86_64__) > +/* C representation of the x86/HVM start info layout. > + * > + * The canonical definition of this layout is abrove, this is just a way to > + * represent the layout described there using C types. > + * > + * NB: the packed attribute is not really needed, but it helps us enforce > + * the fact this this is just a representation, and it might indeed > + * be required in the future if there are alignment changes. > + */ > +struct hvm_start_info { > + uint32_t magic; /* Contains the magic value 0x336ec578 */ > + /* ("xEn3" with the 0x80 bit of the "E" set).*/ > + uint32_t version; /* Version of this structure. */ > + uint32_t flags; /* SIF_xxx flags. */ > + uint32_t cmdline_paddr; /* Physical address of the command line. */ > + uint32_t nr_modules; /* Number of modules passed to the kernel. */ > + uint32_t modlist_paddr; /* Physical address of an array of */ > + /* hvm_modlist_entry. */ > + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > + /* structure. */ > +} __attribute__((packed)); > + > +struct hvm_modlist_entry { > + uint64_t paddr; /* Physical address of the module. */ > + uint64_t size; /* Size of the module in bytes. */ > + uint64_t cmdline_paddr; /* Physical address of the command line. */ > + uint64_t reserved; > +} __attribute__((packed)); > +#endif /* x86 */ This needs extra care: Either the packed attributes need to be dropped (Roger - why did they get put there in the first place?), or their use needs to be shielded from compilers not understanding them. Jan
On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote: > >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: > > --- a/xen/include/public/xen.h > > +++ b/xen/include/public/xen.h > > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; > > */ > > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 > > > > +#if defined(__i386__) || defined(__x86_64__) > > +/* C representation of the x86/HVM start info layout. > > + * > > + * The canonical definition of this layout is abrove, this is just a way to > > + * represent the layout described there using C types. > > + * > > + * NB: the packed attribute is not really needed, but it helps us enforce > > + * the fact this this is just a representation, and it might indeed > > + * be required in the future if there are alignment changes. > > + */ > > +struct hvm_start_info { > > + uint32_t magic; /* Contains the magic value 0x336ec578 */ > > + /* ("xEn3" with the 0x80 bit of the "E" set).*/ > > + uint32_t version; /* Version of this structure. */ > > + uint32_t flags; /* SIF_xxx flags. */ > > + uint32_t cmdline_paddr; /* Physical address of the command line. */ > > + uint32_t nr_modules; /* Number of modules passed to the kernel. */ > > + uint32_t modlist_paddr; /* Physical address of an array of */ > > + /* hvm_modlist_entry. */ > > + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > > + /* structure. */ > > +} __attribute__((packed)); > > + > > +struct hvm_modlist_entry { > > + uint64_t paddr; /* Physical address of the module. */ > > + uint64_t size; /* Size of the module in bytes. */ > > + uint64_t cmdline_paddr; /* Physical address of the command line. */ > > + uint64_t reserved; > > +} __attribute__((packed)); > > +#endif /* x86 */ > > This needs extra care: Either the packed attributes need to be > dropped (Roger - why did they get put there in the first place?), Presumarily b/c on 32-bit and 64-bit the size of the structure would be different otherwise... > or their use needs to be shielded from compilers not understanding > them. .. which begs the question - How come we didn't make the structure 64-bit nice? As in add another uint32_t to it? And then we can ditch the packed? > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Tue, Mar 15, 2016 at 08:59:54PM -0400, Konrad Rzeszutek Wilk wrote: > On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote: Adding Boris to the To: > > >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: > > > --- a/xen/include/public/xen.h > > > +++ b/xen/include/public/xen.h > > > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; > > > */ > > > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 > > > > > > +#if defined(__i386__) || defined(__x86_64__) > > > +/* C representation of the x86/HVM start info layout. > > > + * > > > + * The canonical definition of this layout is abrove, this is just a way to > > > + * represent the layout described there using C types. > > > + * > > > + * NB: the packed attribute is not really needed, but it helps us enforce > > > + * the fact this this is just a representation, and it might indeed > > > + * be required in the future if there are alignment changes. > > > + */ > > > +struct hvm_start_info { > > > + uint32_t magic; /* Contains the magic value 0x336ec578 */ > > > + /* ("xEn3" with the 0x80 bit of the "E" set).*/ > > > + uint32_t version; /* Version of this structure. */ > > > + uint32_t flags; /* SIF_xxx flags. */ > > > + uint32_t cmdline_paddr; /* Physical address of the command line. */ > > > + uint32_t nr_modules; /* Number of modules passed to the kernel. */ > > > + uint32_t modlist_paddr; /* Physical address of an array of */ > > > + /* hvm_modlist_entry. */ > > > + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > > > + /* structure. */ > > > +} __attribute__((packed)); > > > + > > > +struct hvm_modlist_entry { > > > + uint64_t paddr; /* Physical address of the module. */ > > > + uint64_t size; /* Size of the module in bytes. */ > > > + uint64_t cmdline_paddr; /* Physical address of the command line. */ > > > + uint64_t reserved; > > > +} __attribute__((packed)); > > > +#endif /* x86 */ > > > > This needs extra care: Either the packed attributes need to be > > dropped (Roger - why did they get put there in the first place?), > > Presumarily b/c on 32-bit and 64-bit the size of the structure would > be different otherwise... > > > or their use needs to be shielded from compilers not understanding > > them. > > .. which begs the question - How come we didn't make the structure 64-bit nice? > As in add another uint32_t to it? And then we can ditch the packed? > > > > > > > Jan > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel
>>> On 16.03.16 at 01:59, <konrad.wilk@oracle.com> wrote: > On Tue, Mar 15, 2016 at 02:09:46AM -0600, Jan Beulich wrote: >> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: >> > --- a/xen/include/public/xen.h >> > +++ b/xen/include/public/xen.h >> > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; >> > */ >> > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 >> > >> > +#if defined(__i386__) || defined(__x86_64__) >> > +/* C representation of the x86/HVM start info layout. >> > + * >> > + * The canonical definition of this layout is abrove, this is just a way to >> > + * represent the layout described there using C types. >> > + * >> > + * NB: the packed attribute is not really needed, but it helps us enforce >> > + * the fact this this is just a representation, and it might indeed >> > + * be required in the future if there are alignment changes. >> > + */ >> > +struct hvm_start_info { >> > + uint32_t magic; /* Contains the magic value 0x336ec578 */ >> > + /* ("xEn3" with the 0x80 bit of the "E" set).*/ >> > + uint32_t version; /* Version of this structure. */ >> > + uint32_t flags; /* SIF_xxx flags. */ >> > + uint32_t cmdline_paddr; /* Physical address of the command line. */ >> > + uint32_t nr_modules; /* Number of modules passed to the kernel. */ >> > + uint32_t modlist_paddr; /* Physical address of an array of */ >> > + /* hvm_modlist_entry. */ >> > + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ >> > + /* structure. */ >> > +} __attribute__((packed)); >> > + >> > +struct hvm_modlist_entry { >> > + uint64_t paddr; /* Physical address of the module. */ >> > + uint64_t size; /* Size of the module in bytes. */ >> > + uint64_t cmdline_paddr; /* Physical address of the command line. */ >> > + uint64_t reserved; >> > +} __attribute__((packed)); >> > +#endif /* x86 */ >> >> This needs extra care: Either the packed attributes need to be >> dropped (Roger - why did they get put there in the first place?), > > Presumarily b/c on 32-bit and 64-bit the size of the structure would > be different otherwise... And that difference in size would result from what? >> or their use needs to be shielded from compilers not understanding >> them. > > .. which begs the question - How come we didn't make the structure 64-bit > nice? > As in add another uint32_t to it? And then we can ditch the packed? Certainly not the odd number of uint32_t-s in there. If there was a mix of uint32_t and uint64_t, I could certainly see any issue... Jan
Hello, On Tue, 15 Mar 2016, Jan Beulich wrote: > >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: > > --- a/xen/include/public/xen.h > > +++ b/xen/include/public/xen.h > > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; > > */ > > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 > > > > +#if defined(__i386__) || defined(__x86_64__) > > +/* C representation of the x86/HVM start info layout. > > + * > > + * The canonical definition of this layout is abrove, this is just a way to > > + * represent the layout described there using C types. > > + * > > + * NB: the packed attribute is not really needed, but it helps us enforce > > + * the fact this this is just a representation, and it might indeed > > + * be required in the future if there are alignment changes. > > + */ ^ Rationale on why the packed attribute was added. > > +struct hvm_start_info { > > + uint32_t magic; /* Contains the magic value 0x336ec578 */ > > + /* ("xEn3" with the 0x80 bit of the "E" set).*/ > > + uint32_t version; /* Version of this structure. */ > > + uint32_t flags; /* SIF_xxx flags. */ > > + uint32_t cmdline_paddr; /* Physical address of the command line. */ > > + uint32_t nr_modules; /* Number of modules passed to the kernel. */ > > + uint32_t modlist_paddr; /* Physical address of an array of */ > > + /* hvm_modlist_entry. */ > > + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > > + /* structure. */ > > +} __attribute__((packed)); > > + > > +struct hvm_modlist_entry { > > + uint64_t paddr; /* Physical address of the module. */ > > + uint64_t size; /* Size of the module in bytes. */ > > + uint64_t cmdline_paddr; /* Physical address of the command line. */ > > + uint64_t reserved; > > +} __attribute__((packed)); > > +#endif /* x86 */ > > This needs extra care: Either the packed attributes need to be > dropped (Roger - why did they get put there in the first place?), > or their use needs to be shielded from compilers not understanding > them. Hello, The rationale behind the packed attribute is in the highlighted comment above. I would really like to avoid placing this in public headers, or else people will think this is the definition of the payload and will forget that this is just a C representation of it, but the definition is in a comment just above. I want to avoid the issues we have already seen by the usage of C structures as definitions of the placement of payloads in memory. If this really has to be there, please guard it with: #if defined(__XEN__) || defined(__XEN_TOOLS__) So only the Xen kernel/tools can use it. Roger.
>>> On 21.03.16 at 18:04, <roger.pau@citrix.com> wrote: > On Tue, 15 Mar 2016, Jan Beulich wrote: >> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote: >> > --- a/xen/include/public/xen.h >> > +++ b/xen/include/public/xen.h >> > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; >> > */ >> > #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 >> > >> > +#if defined(__i386__) || defined(__x86_64__) >> > +/* C representation of the x86/HVM start info layout. >> > + * >> > + * The canonical definition of this layout is abrove, this is just a way to >> > + * represent the layout described there using C types. >> > + * >> > + * NB: the packed attribute is not really needed, but it helps us enforce >> > + * the fact this this is just a representation, and it might indeed >> > + * be required in the future if there are alignment changes. >> > + */ > > ^ Rationale on why the packed attribute was added. Well, I admit to have overlooked this comment, but I don't see how the packed attribute helps enforce anything. Hence, Anthony, I think together with the attribute that part of the comment should be removed. > I would really like to avoid placing this in public headers, or else > people will think this is the definition of the payload and will forget > that this is just a C representation of it, but the definition is in a > comment just above. I want to avoid the issues we have already seen by the > usage of C structures as definitions of the placement of payloads in > memory. > > If this really has to be there, please guard it with: > > #if defined(__XEN__) || defined(__XEN_TOOLS__) > > So only the Xen kernel/tools can use it. And hvmloader can't. Not a good idea imo. Jan
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h index 93f894c..567016c 100644 --- a/tools/libxc/include/xc_dom.h +++ b/tools/libxc/include/xc_dom.h @@ -219,37 +219,6 @@ struct xc_dom_image { struct xc_hvm_firmware_module smbios_module; }; -#if defined(__i386__) || defined(__x86_64__) -/* C representation of the x86/HVM start info layout. - * - * The canonical definition of this layout resides in public/xen.h, this - * is just a way to represent the layout described there using C types. - * - * NB: the packed attribute is not really needed, but it helps us enforce - * the fact this this is just a representation, and it might indeed - * be required in the future if there are alignment changes. - */ -struct hvm_start_info { - uint32_t magic; /* Contains the magic value 0x336ec578 */ - /* ("xEn3" with the 0x80 bit of the "E" set).*/ - uint32_t version; /* Version of this structure. */ - uint32_t flags; /* SIF_xxx flags. */ - uint32_t cmdline_paddr; /* Physical address of the command line. */ - uint32_t nr_modules; /* Number of modules passed to the kernel. */ - uint32_t modlist_paddr; /* Physical address of an array of */ - /* hvm_modlist_entry. */ - uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ - /* structure. */ -} __attribute__((packed)); - -struct hvm_modlist_entry { - uint64_t paddr; /* Physical address of the module. */ - uint64_t size; /* Size of the module in bytes. */ - uint64_t cmdline_paddr; /* Physical address of the command line. */ - uint64_t reserved; -} __attribute__((packed)); -#endif /* x86 */ - /* --- pluggable kernel loader ------------------------------------- */ struct xc_dom_loader { diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 1cfec5c..c270cf1 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -841,6 +841,37 @@ typedef struct start_info start_info_t; */ #define XEN_HVM_START_MAGIC_VALUE 0x336ec578 +#if defined(__i386__) || defined(__x86_64__) +/* C representation of the x86/HVM start info layout. + * + * The canonical definition of this layout is abrove, this is just a way to + * represent the layout described there using C types. + * + * NB: the packed attribute is not really needed, but it helps us enforce + * the fact this this is just a representation, and it might indeed + * be required in the future if there are alignment changes. + */ +struct hvm_start_info { + uint32_t magic; /* Contains the magic value 0x336ec578 */ + /* ("xEn3" with the 0x80 bit of the "E" set).*/ + uint32_t version; /* Version of this structure. */ + uint32_t flags; /* SIF_xxx flags. */ + uint32_t cmdline_paddr; /* Physical address of the command line. */ + uint32_t nr_modules; /* Number of modules passed to the kernel. */ + uint32_t modlist_paddr; /* Physical address of an array of */ + /* hvm_modlist_entry. */ + uint32_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ + /* structure. */ +} __attribute__((packed)); + +struct hvm_modlist_entry { + uint64_t paddr; /* Physical address of the module. */ + uint64_t size; /* Size of the module in bytes. */ + uint64_t cmdline_paddr; /* Physical address of the command line. */ + uint64_t reserved; +} __attribute__((packed)); +#endif /* x86 */ + /* New console union for dom0 introduced in 0x00030203. */ #if __XEN_INTERFACE_VERSION__ < 0x00030203 #define console_mfn console.domU.mfn
Instead of having several representation of hvm_start_info in C, define it in public/xen.h so both libxc and hvmloader can use it. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- New in V4. --- tools/libxc/include/xc_dom.h | 31 ------------------------------- xen/include/public/xen.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 31 deletions(-)