| Message ID | 20160712144251.558-7-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 12, 2016 at 03:42:43PM +0100, Anthony PERARD wrote: > 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. > Commit message is wrong. I think this can be easily handled during committing though. Now we need an ack from Andrew or Jan. Wei.
On 12/07/16 15:42, Anthony PERARD wrote: > +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > + > +/* > + * C representation of the x86/HVM start info layout. > + * > + * The canonical definition of this layout resides in public/xen.h, this You should also move the big comment block from public/xen.h to here, along with the XEN_HVM_START_MAGIC_VALUE define. There is no point having it split across two locations in the public headers. ~Andrew > + * is just a way to represent the layout described there using C types. > + * > + */ > +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 nr_modules; /* Number of modules passed to the kernel. */ > + uint64_t modlist_paddr; /* Physical address of an array of */ > + /* hvm_modlist_entry. */ > + uint64_t cmdline_paddr; /* Physical address of the command line. */ > + uint64_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > + /* structure. */ > +}; > + > +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; > +}; > + > +#endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
On Tue, Jul 12, 2016 at 04:09:59PM +0100, Andrew Cooper wrote: > On 12/07/16 15:42, Anthony PERARD wrote: > > +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > > +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > > + > > +/* > > + * C representation of the x86/HVM start info layout. > > + * > > + * The canonical definition of this layout resides in public/xen.h, this > > You should also move the big comment block from public/xen.h to here, > along with the XEN_HVM_START_MAGIC_VALUE define. Is it fine to move the comment and the define even if there has been one release of Xen with this in xen.h? > There is no point having it split across two locations in the public > headers.
On 12/07/16 16:36, Anthony PERARD wrote: > On Tue, Jul 12, 2016 at 04:09:59PM +0100, Andrew Cooper wrote: >> On 12/07/16 15:42, Anthony PERARD wrote: >>> +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ >>> +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ >>> + >>> +/* >>> + * C representation of the x86/HVM start info layout. >>> + * >>> + * The canonical definition of this layout resides in public/xen.h, this >> You should also move the big comment block from public/xen.h to here, >> along with the XEN_HVM_START_MAGIC_VALUE define. > Is it fine to move the comment and the define even if there has been one > release of Xen with this in xen.h? The comment, absolutely. It is just a comment. The define is more tricky to argue. We currently expect people to copy&paste the public header files into their own project, rather than linking to them, *and* insist on maintaining API compatibility with further #ifdef'ary obfuscating the structures and names. This status-quo is ludicrous and needs to stop. The chances of any out-of-tree users using XEN_HVM_START_MAGIC_VALUE is minimal, and even if not 0, will be from their own local copy. The chances of anyone wanting XEN_HVM_START_MAGIC_VALUE without the rest of this new file is 0. So I am going to go out on a limb and say yes to moving the define. Noone is going to notice or care, and we won't break anyone’s code by doing so. ~Andrew
On Tue, Jul 12, 2016 at 05:07:35PM +0100, Andrew Cooper wrote: > On 12/07/16 16:36, Anthony PERARD wrote: > > On Tue, Jul 12, 2016 at 04:09:59PM +0100, Andrew Cooper wrote: > >> On 12/07/16 15:42, Anthony PERARD wrote: > >>> +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > >>> +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ > >>> + > >>> +/* > >>> + * C representation of the x86/HVM start info layout. > >>> + * > >>> + * The canonical definition of this layout resides in public/xen.h, this > >> You should also move the big comment block from public/xen.h to here, > >> along with the XEN_HVM_START_MAGIC_VALUE define. > > Is it fine to move the comment and the define even if there has been one > > release of Xen with this in xen.h? > > The comment, absolutely. It is just a comment. > > The define is more tricky to argue. > > We currently expect people to copy&paste the public header files into > their own project, rather than linking to them, *and* insist on > maintaining API compatibility with further #ifdef'ary obfuscating the > structures and names. > > This status-quo is ludicrous and needs to stop. > > The chances of any out-of-tree users using XEN_HVM_START_MAGIC_VALUE is > minimal, and even if not 0, will be from their own local copy. > > The chances of anyone wanting XEN_HVM_START_MAGIC_VALUE without the rest > of this new file is 0. > > > So I am going to go out on a limb and say yes to moving the define. > Noone is going to notice or care, and we won't break anyone’s code by > doing so. Ok, I'll move everything, then. Thanks,
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h index 0629971..de7dca9 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 nr_modules; /* Number of modules passed to the kernel. */ - uint64_t modlist_paddr; /* Physical address of an array of */ - /* hvm_modlist_entry. */ - uint64_t cmdline_paddr; /* Physical address of the command line. */ - uint64_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/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index bc2dbb2..0eab8a7 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -32,6 +32,7 @@ #include <xen/foreign/x86_32.h> #include <xen/foreign/x86_64.h> #include <xen/hvm/hvm_info_table.h> +#include <xen/arch-x86/hvm/start_info.h> #include <xen/io/protocols.h> #include "xg_private.h" diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h new file mode 100644 index 0000000..6981187 --- /dev/null +++ b/xen/include/public/arch-x86/hvm/start_info.h @@ -0,0 +1,53 @@ +/* + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Copyright (c) 2016, Citrix Systems, Inc. + */ + +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ + +/* + * 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. + * + */ +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 nr_modules; /* Number of modules passed to the kernel. */ + uint64_t modlist_paddr; /* Physical address of an array of */ + /* hvm_modlist_entry. */ + uint64_t cmdline_paddr; /* Physical address of the command line. */ + uint64_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ + /* structure. */ +}; + +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; +}; + +#endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
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> --- Changes in V6: - move C representation to public/arch-x86/hvm/start_info.h instead of public/xen.h Change in V5: - remove packed attribute. New in V4. --- tools/libxc/include/xc_dom.h | 31 ---------------- tools/libxc/xc_dom_x86.c | 1 + xen/include/public/arch-x86/hvm/start_info.h | 53 ++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 31 deletions(-) create mode 100644 xen/include/public/arch-x86/hvm/start_info.h