| Message ID | 1456412174-20162-8-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: > --- /dev/null > +++ b/tools/firmware/hvmloader/hvm_start_info.h > @@ -0,0 +1,32 @@ > +#ifndef __HVM_START_INFO_H__ > +#define __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. > + * > + * 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 /* __HVM_START_INFO_H__ */ With there already being such a C representation in libxc, can't we make sure we have only one instance that will need adjustment upon future enhancements? (Perhaps it would still have been a good idea to expose such a C representation in public/xen.h.) > @@ -258,6 +263,7 @@ int main(void) > memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE); > > printf("HVM Loader\n"); > + BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE); Wait - public/xen.h clearly says this is being handed for PVH guests only. How can you even de-reference the pointer unconditionally? Jan
>>> On 29.02.16 at 17:37, <JBeulich@suse.com> wrote: >>>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote: >> @@ -258,6 +263,7 @@ int main(void) >> memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE); >> >> printf("HVM Loader\n"); >> + BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE); > > Wait - public/xen.h clearly says this is being handed for PVH guests > only. How can you even de-reference the pointer unconditionally? Ah, I see this is a result of patch 2, which so far I didn't look at in any detail, the title of which doesn't suggest such a behavioral change, and which also doesn't update the comment in public/xen.h. Jan
diff --git a/tools/firmware/hvmloader/hvm_start_info.h b/tools/firmware/hvmloader/hvm_start_info.h new file mode 100644 index 0000000..b95d60d --- /dev/null +++ b/tools/firmware/hvmloader/hvm_start_info.h @@ -0,0 +1,32 @@ +#ifndef __HVM_START_INFO_H__ +#define __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. + * + * 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 /* __HVM_START_INFO_H__ */ diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index 716d03c..20ec8dc 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -28,6 +28,9 @@ #include "vnuma.h" #include <xen/version.h> #include <xen/hvm/params.h> +#include "hvm_start_info.h" + +const struct hvm_start_info *hvm_start_info; asm ( " .text \n" @@ -46,6 +49,8 @@ asm ( " ljmp $"STR(SEL_CODE32)",$1f \n" "1: movl $stack_top,%esp \n" " movl %esp,%ebp \n" + /* store HVM start info ptr */ + " mov %ebx, hvm_start_info \n" " call main \n" /* Relocate real-mode trampoline to 0x0. */ " mov $trampoline_start,%esi \n" @@ -258,6 +263,7 @@ int main(void) memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE); printf("HVM Loader\n"); + BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE); init_hypercalls();
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Change in V3: - remove cmdline parser - load hvm_start_info pointer earlier, before calling main(). - Add struct hvm_start_info definition to hvmloader. --- tools/firmware/hvmloader/hvm_start_info.h | 32 +++++++++++++++++++++++++++++++ tools/firmware/hvmloader/hvmloader.c | 6 ++++++ 2 files changed, 38 insertions(+) create mode 100644 tools/firmware/hvmloader/hvm_start_info.h