| Message ID | 20160622171545.5304-7-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cc HV maintainers
I'm of course fine with moving this structure somewhere else.
On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
I suspect it would make more sense to move it to public/arch-x86/xen.h.
On 22/06/16 18:15, 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. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > Change in V5: > - remove packed attribute. > > New in V4. > --- > tools/libxc/include/xc_dom.h | 31 ------------------------------- > xen/include/public/xen.h | 27 +++++++++++++++++++++++++++ +1 to the public interface, but I would recommend introducing a new file public/hvm/start_info.h rather than extending the general dumping ground which is xen.h ~Andrew
>>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote: > Cc HV maintainers > > I'm of course fine with moving this structure somewhere else. > > On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote: > >> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > > > I suspect it would make more sense to move it to public/arch-x86/xen.h. The question is whether we really mean this to remain x86 specific: The way it's now there's nothing inherently x86-ish there. But if that's the plan (which the conditional around it supports) then the suggested alternative resting place seems appropriate to me. Jan
>>> On 07.07.16 at 17:02, <andrew.cooper3@citrix.com> wrote: > On 22/06/16 18:15, 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. >> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >> --- >> Change in V5: >> - remove packed attribute. >> >> New in V4. >> --- >> tools/libxc/include/xc_dom.h | 31 ------------------------------- >> xen/include/public/xen.h | 27 +++++++++++++++++++++++++++ > > +1 to the public interface, but I would recommend introducing a new file > public/hvm/start_info.h rather than extending the general dumping ground > which is xen.h But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h? Jan
On 07/07/16 16:09, Jan Beulich wrote: >>>> On 07.07.16 at 17:02, <andrew.cooper3@citrix.com> wrote: >> On 22/06/16 18:15, 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. >>> >>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >>> --- >>> Change in V5: >>> - remove packed attribute. >>> >>> New in V4. >>> --- >>> tools/libxc/include/xc_dom.h | 31 ------------------------------- >>> xen/include/public/xen.h | 27 +++++++++++++++++++++++++++ >> +1 to the public interface, but I would recommend introducing a new file >> public/hvm/start_info.h rather than extending the general dumping ground >> which is xen.h > But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h? Fine by me. ~Andrew
On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote: > >>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote: > > Cc HV maintainers > > > > I'm of course fine with moving this structure somewhere else. > > > > On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote: > > > >> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > > > > > > I suspect it would make more sense to move it to public/arch-x86/xen.h. > > The question is whether we really mean this to remain x86 specific: > The way it's now there's nothing inherently x86-ish there. But if > that's the plan (which the conditional around it supports) then the > suggested alternative resting place seems appropriate to me. > For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM for ARM would be wrong IMHO. I've CC'ed Stefano and Julien to let them express their opinions. Wei. > Jan >
Hi Wei, On 07/07/16 16:28, Wei Liu wrote: > On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote: >>>>> On 07.07.16 at 16:55, <wei.liu2@citrix.com> wrote: >>> Cc HV maintainers >>> >>> I'm of course fine with moving this structure somewhere else. >>> >>> On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote: >>> >>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h >>> >>> >>> I suspect it would make more sense to move it to public/arch-x86/xen.h. >> >> The question is whether we really mean this to remain x86 specific: >> The way it's now there's nothing inherently x86-ish there. But if >> that's the plan (which the conditional around it supports) then the >> suggested alternative resting place seems appropriate to me. >> > > For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM > for ARM would be wrong IMHO. Correct, if you would have to do a comparison with x86 it would be PVH. However, this structure looks useful only if we lack a way to describe those parameters in the firmware. This is not the case for ARM as this could be described easily by the device tree with the generic bindings. Regards,
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/xen/include/public/xen.h b/xen/include/public/xen.h index d9ddee7..defde97 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -859,6 +859,33 @@ 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. + */ +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 /* 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> --- Change in V5: - remove packed attribute. New in V4. --- tools/libxc/include/xc_dom.h | 31 ------------------------------- xen/include/public/xen.h | 27 +++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 31 deletions(-)